Re: Winbond WPCD376I Support

[Maxim L. <max...@gm...>]

On Fri, 2009-09-04 at 23:12 -0400, Jarod Wilson wrote:
> On 09/04/2009 06:04 PM, Maxim Levitsky wrote:
> >>> The LIRC kernel drivers are in a pretty good shape by now and all it
> >>> >  >  needs
> >>> >  >  is someone who can invest some time to post patches on LKML to get it
> >>> >  >  accepted. I know that Jarod plans to do this, but I guess he would
> >>> >  >  be glad
> >>> >  >  if someone could help.
> >> >
> >> >
> >> >  Indeed. Most of the commonly used drivers are in pretty good shape
> >> >  now, and I've got vacation coming up shortly, during which I plan to
> >> >  submit at least lirc_dev, lirc_imon, lirc_mceusb, lirc_serial,
> >> >  lirc_streamzap, lirc_i2c and lirc_zilog to lkml for review again. I
> >> >  will definitely need reviewers for the code at that time. Any drivers
> >> >  not mentioned probably aren't being submitted, as they've had less
> >> >  attention paid to them, and could really use a good in-depth technical
> >> >  read-through and possibly some cleanups before they get sent in.
> >> >
> >> >  So yeah, I'm actually planning another upstream submission attempt in
> >> >  about 2 weeks.
> >> >
> > I got few notes:
> >
> >
> > * I foresee that every driver that uses LIRCCODE will be met with forks
> > and torches.
> > Devices that work with finite subset of remotes, and send keycodes,
> > really should be handled by a input driver.
> > (And this is the only case)
> > So maybe hold the lirc_i2c?
> 
> That's probably prudent.
> 
> > * Currently lirc_dev lacks support for transmitting, thus each and every
> > driver implements their own.
> 
> Well, there's really only three drivers that support transmitting (if 
> I'm thinking clearly), and they're all quite different (lirc_serial, 
> lirc_mceusb and lirc_zilog), so I'm not sure how much shared code there 
> would be. I suppose it could be worth merging anything that is shared 
> into lirc_dev though.
What I think is that, in long term, lirc_dev support should get away
from asking drivers to define their fops.
Even a tiny wrapper over fops will be enough.
Also, I think that set_use_inc, and set_use_dec are misnamed.

I think that proper layout for driver operations might be:


/* prepare hardware for work - replaces .set_use_inc */
int init(struct lirc_driver *drv)

/* shutdown hardware - replaces .set_use_dec */
void deinit(struct lirc_driver *drv)


/* receive buffer of data - replaces  .add_to_buf */
int receive (struct lirc_driver *drv, struct lirc_buffer *buf)


/* transmit a packet*/
int transmit (struct lirc_driver *drv, int* packet, int len)


/* flash a led */
int flash_led(struct lirc_driver *drv, int flags);


/* set range for for RX carrier. (-1,-1 means use wide receiver)*/
int set_rx_carrier(struct lirc_driver *drv,
			int low, int high);

/* set TX carrier*/
int set_tx_carrier(struct lirc_driver *drv, int carrier);


/* duty cycle settings - why we need RX duty cycle?*/
int set_rx_duty_cycle(struct lirc_driver *drv, int duty_cycle);
int set_tx_duty_cycle(struct lirc_driver *drv, int duty_cycle);


/* set recording mode*/
int set_rx_mode(struct lirc_driver *drv, int mode);
int set_tx_mode(struct lirc_driver *drv, int mode);



All 'settings' functions return 0 on success, and last set value is then
stored in lirc_driver structure. Thus no need for _get functions.


Also, on resume from low power state (ram/disk) lirc core calls these
functions, to set up device again.

Absence of functions indicate that device doesn't support this or that
feature.

Will be glad to hear your comments.



> 
> > Btw, the less drivers are submitted initially, the better.
> 
> Yeah, my list got a little longer than I was initially imagining. Those 
> are all the ones that I've personally worked on in recent months, and 
> *know* first-hand are fully functional. I'm not opposed to limiting it 
> to lirc_dev, lirc_imon, lirc_mceusb, lirc_zilog and maybe lirc_serial 
> though to start out.
> 
> Note that for lirc_imon, lirc_mceusb and lirc_streamzap, I'm thinking 
> more and more that I'd like to add input device support to all of these. 
> Their default mode of operation with no lirc client attached would be to 
> send the keycodes for their default remotes via the input subsystem, but 
> once an lirc client attaches, they'd send their data via the lirc 
> interface. This way, stock remotes work out-of-the-box with no 
> configuration, but you've still got the ability to use lircd for 
> decoding everything, which I think is particularly important for 
> receivers that support umpteen different IR protocols and remotes.
Nice idea, but I suspect you will need a RC6 decoding in kernel to
decode default remote for lirc_mceusb?



> 
> See lirc_imon for an example of what I'm talking about. It actually 
> *has* this behavior already, more or less, for the default remote's pad 
> and mouse buttons. Rene Harder and I actually talked about extending 
> lirc_imon this way several weeks back. The more I think about it, the 
> more I like the idea, and want to carry it over to other drivers.
> 
> > I don't even ask to submit mine, although I did test it against
> > checkpatch.
> 
> Note that I didn't mean to suggest your driver wasn't ready, just that 
> I'm not comfortable submitting it, as I've only briefly looked at the 
> code, and couldn't answer any questions about it with much confidence. 
> I'd definitely prefer to see *you* submit it, once we get the first foot 
> in the door. :)
Exactly!

As soon as core is in, it will be trivial to submit drivers, and I will
submit mine.


> 
> > The core problem is the lirc_dev. As soon as it is accepted, drivers can
> > be sent/cleaned up one after another.
> 
> Yup.
> 
> > And, maybe redefine this:
> >
> > #define IRCTL_DEV_NAME    "BaseRemoteCtl"
> 
> I'm not particularly attached to that name or any other. Suggestions 
> welcomed. Trivial detail though, and easy to fix if upstream demands it.
> 
Sure,
#define IRCTL_DEV_NAME "lirc" ?


Best regards
	Maxim Levitsky