[RFC] v4l2_subdev_ir_ops

Message ID 1250906940.3159.20.camel@palomino.walls.org (mailing list archive)
State RFC, archived
Headers

Commit Message

Andy Walls Aug. 22, 2009, 2:09 a.m. UTC
  In the course of implementing code to run the Consumer IR circuitry in
the CX2584[0123] chip and similar cores, I need to add
v4l2_subdev_ir_ops for manipulating the device and fetch and send data.

In line below is a proposal at what I think those subdev ops might be.
Feel free to take shots at it.

The insipration for these comes from two sources, the LIRC v0.8.5 source
code and what the CX2584x chip is capable of doing:

http://prdownloads.sourceforge.net/lirc/lirc-0.8.5.tar.bz2
http://dl.ivtvdriver.org/datasheets/video/cx25840.pdf

Note the Consumer IR in the CX2584x can theoretically measure
unmodulated marks and spaces with a resolution of 37.037... ns (1/27
MHz), hence the references to nanoseconds in the proposal below.  Most
IR devices use pulses on the order of microseconds in real life.

Regards,
Andy



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Comments

Hans Verkuil Aug. 22, 2009, 7:42 a.m. UTC | #1
On Saturday 22 August 2009 04:09:00 Andy Walls wrote:
> In the course of implementing code to run the Consumer IR circuitry in
> the CX2584[0123] chip and similar cores, I need to add
> v4l2_subdev_ir_ops for manipulating the device and fetch and send data.
>
> In line below is a proposal at what I think those subdev ops might be.
> Feel free to take shots at it.
>
> The insipration for these comes from two sources, the LIRC v0.8.5 source
> code and what the CX2584x chip is capable of doing:
>
> http://prdownloads.sourceforge.net/lirc/lirc-0.8.5.tar.bz2
> http://dl.ivtvdriver.org/datasheets/video/cx25840.pdf
>
> Note the Consumer IR in the CX2584x can theoretically measure
> unmodulated marks and spaces with a resolution of 37.037... ns (1/27
> MHz), hence the references to nanoseconds in the proposal below.  Most
> IR devices use pulses on the order of microseconds in real life.

Hi Andy,

My first impressions are that you are trying to do too much here. What you 
want is a simple API to access an IR transmitter or receiver. These devices 
do not care about lirc or any other high-level APIs. All they need 
basically are a setup command and read/write commands. There is probably no 
need for querying capabilities since the flow of information in V4L2 is 
usually the other way around:

if you know the card, then you know the remote, then you know the settings, 
then you can tell the IR module. Based on just the IR module you cannot 
know what the IR timings/modulation etc. will be.

If there is a demonstrable need to get some capabilities for lirc, then I 
would also suggest to combine them all into one g_caps call that returns 
all caps in a struct. Much easier than breaking it all up in small 
functions.

Don't use is_ns BTW, just do everything in ns. I don't see any problem with 
that.

Making a set of ops for IR support is a very good idea, but I think you need 
to actually implement and use it in a driver first. Just the plain act of 
implementing something will show you what the strenghts and weaknesses of 
an API are and will help you prototype a better solution.

Remember that this subdev API is an internal API. It was *designed* for 
change. So it is perfectly reasonable to start off with a subset and extend 
and modify it over time. What I do not want to see are unused ops. So 
everything in there also has to be used somewhere (or known to be used very 
soon).

Regarding interrupt handlers: there is a notify callback in v4l2_device that 
could be used for that purpose as well, rather than setting up separate 
callback functions. Whether that's a good idea or not depends on the way it 
will actually be used. So when you are working on actual code you can try 
each approach and see which works best.

Regards,

	Hans

>
> Regards,
> Andy
>
> diff -r 44282114d1e3 linux/include/media/v4l2-subdev.h
> --- a/linux/include/media/v4l2-subdev.h	Fri Aug 21 13:26:01 2009 -0400
> +++ b/linux/include/media/v4l2-subdev.h	Fri Aug 21 21:51:52 2009 -0400
> @@ -229,11 +229,206 @@
>  	int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
> v4l2_frmivalenum *fival); };
>
> +/*
> +   interrupt_service_routine: Called by the bridge chip's interrupt
> service +	handler, when an IR interrupt status has be raised due to this
> subdev, +	so that this subdev can handle the details.  It may schedule
> work to be +	performed later.  It must not sleep.  *Called from an IRQ
> context*. +
> +   g_features: Return IR features supported by this device.  Intended to
> be +	used to support the LIRC_GET_FEATURES ioctl() and to return the same
> +	flags.
> +
> +   g_code_length: Return the cooked IR code length.  Intended to be used
> to +	support the LIRC_GET_LENGTH ioctl(), returning the length of a code
> +	in bits.  Currently only used in lirc for the LIRC_MODE_LIRCCODE. +
> +   [rt]x_s_notify_callback: Allows the subdev caller to set a callback
> for +	notification of events due to the IR recevier or transmitter.
> +
> +   rx_read_pulse_widths: Reads received data in the form of consective
> space and +	mark pulse widths in microseconds, or nanoseconds if in_ns is
> true.  The +	semantics are similar to a non-blocking read() call.
> +
> +   tx_write_pulse_widths: Reads received data in the form of consective
> mark and +	space pulse widths in microseconds, or nanoseconds if in_ns is
> true. +	The semantics are similar to a non-blocking write() call.
> +
> +   rx_read: Reads received codes or other non-pulse width data.
> +	The semantics are similar to a non-blocking read() call.
> +
> +   tx_write: Writes codes or other non-pulse width data to transmit.
> +	The semantics are similar to a non-blocking write() call.
> +
> +   [rt]x_enable: enable or disable the receiver or transmitter using or
> +	preserving the current setting.
> +
> +   [rt]x_shutdown: disable the receiver or transmitter and adjust all
> setting +	to shut off or slow down hardware and disable interrupts.
> +
> +   [rt]x_s_interrupt_enable: enable or diable the receiver or
> transmitter +	interrupts.
> +
> +   rx_s_demodulation: enable demodulation of received pulses from a
> carrier or +	disable demodulation and read "baseband" light pulses.
> +
> +   rx_g_demodulation: query if demodulation of received pulses from a
> carrier is +	enabled.
> +
> +   tx_s_modulation: enable modulation of transmitted pulses onto a
> carrier or +	disable modulation and transmit "baseband" light pulses.
> +
> +   tx_g_modulation: query if modulation of transmitted pulses onto a
> carrier is +	enabled.
> +
> +   rx_s_noise_filter: set the threshold pulse width for a received pulse
> to be +	considered valid and not a glitch or noise.  A value of 0
> disables the +	noise filter.
> +
> +   rx_g_noise_filter: query the threshold pulse width for a received
> pulse to be +	considered valid and not a glitch or noise.  A value of 0
> means the +	noise filter is disabled.
> +
> +   [rt]x_s_max_pulse_width: sets the max valid pulse width expected to
> be +	received or transmitted, when receiving or transmitting baseband
> pulses, +	in order to optimize the pulse width timer's resolution.  This
> call will +	likely have the side effect of disabling
> demodulation/modulation of +	pulses from/onto a carrier.
> +
> +   [rt]x_g_max_pulse_width: gets the max valid pulse width expected to
> be +	received or transmitted, when receiving or transmitting baseband
> pulses. +	This call should return error if demodulation/modulation of
> +	pulses from/onto a carrier is enabled.
> +
> +   [rt]x_[sg]_lirc_mode: Set or get the LIRC "mode" for the receiver or
> +	transmitter.  Intended to support the LIRC_{SET,GET}_{REC,SEND}_MODE
> +	ioctl() calls.
> +
> +   [rt]x_[sg]_carrier: Set or get the carrier frequency for the receiver
> or +	transmitter.  Frequency is in Hz.  Hardware limitations may
> +	mean the actual frequency set varies from the desired frequency.  The
> +	_g_ calls should return the precise frequency set.  The _s_ calls will
> +	have the side effect of enabling demodulation/modulation when the
> +	freq is not 0, and disabling demodulation/modulation when the
> +	freq is 0.  These calls can also be used to support the
> +	LIRC_{SET,GET}_{REC,SEND}_CARRIER ioctl() calls.
> +
> +   rx_s_carrier_range: Set a window of expected carrier frequencies for
> the +	receiver.  Intended to support the LIRC_SET_REC_CARRIER_RANGE
> ioctl(). +	Due to hardware limitations, the full range may not be
> supportable and +	the center of the supportable range may not be at the
> exact center of +	the desired range.
> +
> +   [rt]x_[sg]_duty_cycle: Set or get the carrier duty cycle for the
> receiver or +	transmitter.  Hardware limitations may mean the actual duty
> cycle set +	varies from the desired duty_cycle.  The _g_ calls should
> return the +	precise duty cycle set.  These calls can also be used to
> +	support the LIRC_{SET,GET}_{REC,SEND}_DUTY_CYCLE ioctl() calls.
> +
> +   rx_s_duty_cycle_range: Set a window of expected received carrier duty
> cycles +	Intended to support the LIRC_SET_REC_DUTY_CYCLE_RANGE ioctl().
> +	Due to hardware limitations, the full range may not be supportable and
> +	the center of the supportable range may not be at the exact center of
> +	the desired range.
> +
> +   rx_g_resolution: get the pulse measurment resolution of the receiver.
> +	Intended to support the LIRC_GET_REC_RESOLUTION ioctl().
> +
> +   tx_s_mask: set the mask of enabled transmitters for devices that have
> +	more than one trasnmitter.  Intended to support the
> +	LIRC_SET_TRANSMITTER_MASK ioctl().
> + */
> +enum v4l2_subdev_ir_event {
> +	V4L2_SUBDEV_IR_RX_DATA_READY     = 0,
> +	V4L2_SUBDEV_IR_TX_READY_FOR_DATA = 1,
> +};
> +
> +typedef int (*v4l2_subdev_ir_notify_callback)(void *priv,
> +					      struct v4l2_subdev *sd,
> +					      enum v4l2_subdev_ir_event event);
> +
> +struct v4l2_subdev_ir_ops {
> +	/* Common to receiver and transmitter */
> +	int (*interrupt_service_routine)(struct v4l2_subdev *sd,
> +						u32 status, bool *handled);
> +
> +	/* LIRC ioctl inspired calls */
> +	int (*g_features)(struct v4l2_subdev *sd, u32 *features);
> +	int (*g_code_length)(struct v4l2_subdev *sd, u32 *bits);
> +
> +	/* Receiver */
> +	int (*rx_s_notify_callback)(struct v4l2_subdev *sd,
> +					v4l2_subdev_ir_notify_callback callback,
> +					void *priv);
> +
> +	int (*rx_read_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
> +					size_t count, bool in_ns, ssize_t *num);
> +	int (*rx_read)(struct v4l2_subdev *sd, u8 *buf, size_t count,
> +				ssize_t *num);
> +
> +	int (*rx_enable)(struct v4l2_subdev *sd, bool enable);
> +	int (*rx_shutdown)(struct v4l2_subdev *sd);
> +
> +	int (*rx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
> +	int (*rx_s_demodulation)(struct v4l2_subdev *sd, bool enable);
> +	int (*rx_s_noise_filter)(struct v4l2_subdev *sd, u32 min_width_ns);
> +	int (*rx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
> +
> +	int (*rx_g_demodulation)(struct v4l2_subdev *sd, bool *enabled);
> +	int (*rx_g_noise_filter)(struct v4l2_subdev *sd, u32 *min_width_ns);
> +	int (*rx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
> +
> +	/* LIRC receiver ioctl inspired calls */
> +	int (*rx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
> +	int (*rx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
> +	int (*rx_s_carrier_range)(struct v4l2_subdev *sd,
> +					u32 lower, u32 upper);
> +	int (*rx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
> +	int (*rx_s_duty_cycle_range)(struct v4l2_subdev *sd,
> +					u32 lower, u32 upper);
> +
> +	int (*rx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
> +	int (*rx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
> +	int (*rx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
> +	int (*rx_g_resolution)(struct v4l2_subdev *sd, u32 *nsec);
> +
> +	/* Transmitter */
> +	int (*tx_s_notify_callback)(struct v4l2_subdev *sd,
> +					v4l2_subdev_ir_notify_callback callback,
> +					void *priv);
> +
> +	int (*tx_write_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
> +					size_t count, bool in_ns, ssize_t *num);
> +	int (*tx_write)(struct v4l2_subdev *sd, u8 *buf, size_t count,
> +				ssize_t *num);
> +
> +	int (*tx_enable)(struct v4l2_subdev *sd, bool enable);
> +	int (*tx_shutdown)(struct v4l2_subdev *sd);
> +
> +	int (*tx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
> +	int (*tx_s_modulation)(struct v4l2_subdev *sd, bool enable);
> +	int (*tx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
> +
> +	int (*tx_g_modulation)(struct v4l2_subdev *sd, bool *enabled);
> +	int (*tx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
> +
> +	/* LIRC transmitter ioctl inspired calls */
> +	int (*tx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
> +	int (*tx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
> +	int (*tx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
> +	int (*tx_s_mask)(struct v4l2_subdev *sd, u32 enabled_transmitters);
> +
> +	int (*tx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
> +	int (*tx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
> +	int (*tx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
> +};
> +
>  struct v4l2_subdev_ops {
>  	const struct v4l2_subdev_core_ops  *core;
>  	const struct v4l2_subdev_tuner_ops *tuner;
>  	const struct v4l2_subdev_audio_ops *audio;
>  	const struct v4l2_subdev_video_ops *video;
> +	const struct v4l2_subdev_ir_ops    *ir;
>  };
>
>  #define V4L2_SUBDEV_NAME_SIZE 32
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Andy Walls Aug. 22, 2009, 2:47 p.m. UTC | #2
On Sat, 2009-08-22 at 09:42 +0200, Hans Verkuil wrote:
> On Saturday 22 August 2009 04:09:00 Andy Walls wrote:
> > In the course of implementing code to run the Consumer IR circuitry in
> > the CX2584[0123] chip and similar cores, I need to add
> > v4l2_subdev_ir_ops for manipulating the device and fetch and send data.
> >
> > In line below is a proposal at what I think those subdev ops might be.
> > Feel free to take shots at it.
> >
> > The insipration for these comes from two sources, the LIRC v0.8.5 source
> > code and what the CX2584x chip is capable of doing:
> >
> > http://prdownloads.sourceforge.net/lirc/lirc-0.8.5.tar.bz2
> > http://dl.ivtvdriver.org/datasheets/video/cx25840.pdf
> >
> > Note the Consumer IR in the CX2584x can theoretically measure
> > unmodulated marks and spaces with a resolution of 37.037... ns (1/27
> > MHz), hence the references to nanoseconds in the proposal below.  Most
> > IR devices use pulses on the order of microseconds in real life.
> 
> Hi Andy,

Hans,

That you for your comments.


> My first impressions are that you are trying to do too much here. What you 
> want is a simple API to access an IR transmitter or receiver.

OK.  


> These devices 
> do not care about lirc or any other high-level APIs. All they need 
> basically are a setup command and read/write commands.

This particular device is basicaly a collection of hardware counters and
timers.

I can lump most of the controls into a setup() type call, but I suspect
I'll still need to support the granularity of changing one parameter at
a time.  The only solution for IR blasting that I know of is LIRC, and
it manipulates parameters one at a time.


>  There is probably no 
> need for querying capabilities since the flow of information in V4L2 is 
> usually the other way around:

Hmmm.  I see your point.  I was aiming for the bridge driver not having
to "care" about what type of IR subdev it was manipulating, and just use
a common set of calls whether it was a microcontroller or a collection
of timers and counters like this device.  This was especially useful if
acting as a go between between LIRC and a subdevice, or between an in
kernel IR protocol code and a subdevice. 

In other words I wanted to keep IR subdevice specific knowledge out of
the bridge driver and encapsulate it in the subdev definition.

As an example:

The hardware I'm working with can only return and accept pulse widths as
data. Other hardware actually decodes pulses and returns codes of
various lengths.

Should that knowledge be buried in the bridge driver or the subdevice
definition?



> if you know the card, then you know the remote, then you know the settings, 
> then you can tell the IR module.  Based on just the IR module you cannot 
> know what the IR timings/modulation etc. will be.


Stictly speaking the board doesn't tell you the remote.  Retail cards
can come with remotes with an RC-5 protocol. However Media center
versions of the same card may come with remotes using an RC-6 protocol.
The settings are different I believe (I have to go back and check to
make sure).

For the case of using in-kernel IR receive handling, based on the card,
I was going to use a reasonable default for the protocol and remote
codes.  This is information the bridge driver would have - excepting of
course RC-5 vs. RC-6 remotes which it can't know if the vendor offered
both types.


Actually, with CX25843 and similar IR hardware you can use any remote
with the card.  With that hardware, the IR protocol has to be
implemented in host CPU software: in the linux kernel or in userspace.
That hardware can only return and accept pulse timings.  The subdev
struck me as the the wrong place to reimplement RC-5, RC-6, or other
protocols.

For transmit, there are obviuosly no good assumptions one can make based
on the card.


> If there is a demonstrable need to get some capabilities for lirc, then I 
> would also suggest to combine them all into one g_caps call that returns 
> all caps in a struct.

That's what g_features() and g_code_length() was for.  I can combine
them into a g_caps() call.

I now note that reading back of how things were set, e.g.
rx_g_carrier(), aren't all strictly needed.  LIRC has hooks for them,
but no code in LIRC userspace appears to actually call a few of them -
e.g. nothing ever calls the LIRC_GET_REC_CARRIER ioctl().


>  Much easier than breaking it all up in small 
> functions.

I assume you mean all the "read back the current state" stuff.  OK.
Are calls named rx_g_state() and tx_g_state() acceptable?


> Don't use is_ns BTW, just do everything in ns. I don't see any problem with 
> that.

Thinking out loud...

Well, if I did everything in ns, reading and writing pulse widths would
require a u64 type to report the longest possible pulse that can be
measured for unmodulated IR:

4 * 0xffff * (0xffff + 1) / 54 MHz = 318,140,871,111 ns
^^^^^^^^^^   ^^^^^^^^^^^    ^^^^^^
width cnt    clock div      clock

which requires 39 bits to represent as nanoseconds.  Of course a 318
second pulse is not useful for consumer devices.  32 bits could
represent a maximum 4.29 second pulse in nanoseconds.

I suspect people can live with a 4.3 second maximum pulse width limit
for unmodulated IR.  Yes, it all be reported in ns.



> Making a set of ops for IR support is a very good idea, but I think you need 
> to actually implement and use it in a driver first. Just the plain act of 
> implementing something will show you what the strenghts and weaknesses of 
> an API are and will help you prototype a better solution.

I am in process in the cx23885 driver:

http://www.kernellabs.com/blog/?p=592
http://linuxtv.org/hg/~awalls/cx23888-ir/

Work from there will map almost directly over into the cx25840 driver.
There is only a minor conceptual difference between the basic CX23888
Consumer IR functions and the baseline Consumer IR support implemented
in all the cores supported by the cx25840 module.


> Remember that this subdev API is an internal API. It was *designed* for 
> change. So it is perfectly reasonable to start off with a subset and extend 
> and modify it over time. What I do not want to see are unused ops. So 
> everything in there also has to be used somewhere (or known to be used very 
> soon).

Understood.  I'll pare things down.

Dead, unused code raises more questions than answers.  In my examination
of LIRC code, I found plenty of it. :?

First order of business is to implement in kernel RC-5 receive for the
CX23888.

"Known to be used very soon" will have to apply to the IR transmitter
code in the subdev.  It is easy to implement the Tx side of the subdev
at the same time I implment the Rx side while I still rememebr how the
harwdare works and the equations are fresh in my mind.  As there is no
in kernel solution for IR blasting, the LIRC ioctl()s for IR blasting
present a defined set of controls to support that I doubt will change.
I will check LIRC for what is actually used.



> 
> Regarding interrupt handlers: there is a notify callback in v4l2_device that 
> could be used for that purpose as well, rather than setting up separate 
> callback functions. Whether that's a good idea or not depends on the way it 
> will actually be used. So when you are working on actual code you can try 
> each approach and see which works best.

I looked at the v4l2_device notify callback.

The direction is wrong for the interrupt service handler hook.

For the "Rx data available" and "Tx ready for data" notifications, I
thought there might be problems if someone actually used a CX231xx,
CX2584[0123], or CX2583[67] for IR.  The "unsigned int notification"
values would then need to be coordinated among the CX23885 bridge driver
and other drivers that used the IR presented by the cx25840 module.
Maybe that's not a big deal.


> Regards,
> 
> 	Hans


Thanks.  I'll try to resubmit something soon after more research.

Regards,
Andy


> >
> > Regards,
> > Andy
> >
> > diff -r 44282114d1e3 linux/include/media/v4l2-subdev.h
> > --- a/linux/include/media/v4l2-subdev.h	Fri Aug 21 13:26:01 2009 -0400
> > +++ b/linux/include/media/v4l2-subdev.h	Fri Aug 21 21:51:52 2009 -0400
> > @@ -229,11 +229,206 @@
> >  	int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
> > v4l2_frmivalenum *fival); };
> >
> > +/*
> > +   interrupt_service_routine: Called by the bridge chip's interrupt
> > service +	handler, when an IR interrupt status has be raised due to this
> > subdev, +	so that this subdev can handle the details.  It may schedule
> > work to be +	performed later.  It must not sleep.  *Called from an IRQ
> > context*. +
> > +   g_features: Return IR features supported by this device.  Intended to
> > be +	used to support the LIRC_GET_FEATURES ioctl() and to return the same
> > +	flags.
> > +
> > +   g_code_length: Return the cooked IR code length.  Intended to be used
> > to +	support the LIRC_GET_LENGTH ioctl(), returning the length of a code
> > +	in bits.  Currently only used in lirc for the LIRC_MODE_LIRCCODE. +
> > +   [rt]x_s_notify_callback: Allows the subdev caller to set a callback
> > for +	notification of events due to the IR recevier or transmitter.
> > +
> > +   rx_read_pulse_widths: Reads received data in the form of consective
> > space and +	mark pulse widths in microseconds, or nanoseconds if in_ns is
> > true.  The +	semantics are similar to a non-blocking read() call.
> > +
> > +   tx_write_pulse_widths: Reads received data in the form of consective
> > mark and +	space pulse widths in microseconds, or nanoseconds if in_ns is
> > true. +	The semantics are similar to a non-blocking write() call.
> > +
> > +   rx_read: Reads received codes or other non-pulse width data.
> > +	The semantics are similar to a non-blocking read() call.
> > +
> > +   tx_write: Writes codes or other non-pulse width data to transmit.
> > +	The semantics are similar to a non-blocking write() call.
> > +
> > +   [rt]x_enable: enable or disable the receiver or transmitter using or
> > +	preserving the current setting.
> > +
> > +   [rt]x_shutdown: disable the receiver or transmitter and adjust all
> > setting +	to shut off or slow down hardware and disable interrupts.
> > +
> > +   [rt]x_s_interrupt_enable: enable or diable the receiver or
> > transmitter +	interrupts.
> > +
> > +   rx_s_demodulation: enable demodulation of received pulses from a
> > carrier or +	disable demodulation and read "baseband" light pulses.
> > +
> > +   rx_g_demodulation: query if demodulation of received pulses from a
> > carrier is +	enabled.
> > +
> > +   tx_s_modulation: enable modulation of transmitted pulses onto a
> > carrier or +	disable modulation and transmit "baseband" light pulses.
> > +
> > +   tx_g_modulation: query if modulation of transmitted pulses onto a
> > carrier is +	enabled.
> > +
> > +   rx_s_noise_filter: set the threshold pulse width for a received pulse
> > to be +	considered valid and not a glitch or noise.  A value of 0
> > disables the +	noise filter.
> > +
> > +   rx_g_noise_filter: query the threshold pulse width for a received
> > pulse to be +	considered valid and not a glitch or noise.  A value of 0
> > means the +	noise filter is disabled.
> > +
> > +   [rt]x_s_max_pulse_width: sets the max valid pulse width expected to
> > be +	received or transmitted, when receiving or transmitting baseband
> > pulses, +	in order to optimize the pulse width timer's resolution.  This
> > call will +	likely have the side effect of disabling
> > demodulation/modulation of +	pulses from/onto a carrier.
> > +
> > +   [rt]x_g_max_pulse_width: gets the max valid pulse width expected to
> > be +	received or transmitted, when receiving or transmitting baseband
> > pulses. +	This call should return error if demodulation/modulation of
> > +	pulses from/onto a carrier is enabled.
> > +
> > +   [rt]x_[sg]_lirc_mode: Set or get the LIRC "mode" for the receiver or
> > +	transmitter.  Intended to support the LIRC_{SET,GET}_{REC,SEND}_MODE
> > +	ioctl() calls.
> > +
> > +   [rt]x_[sg]_carrier: Set or get the carrier frequency for the receiver
> > or +	transmitter.  Frequency is in Hz.  Hardware limitations may
> > +	mean the actual frequency set varies from the desired frequency.  The
> > +	_g_ calls should return the precise frequency set.  The _s_ calls will
> > +	have the side effect of enabling demodulation/modulation when the
> > +	freq is not 0, and disabling demodulation/modulation when the
> > +	freq is 0.  These calls can also be used to support the
> > +	LIRC_{SET,GET}_{REC,SEND}_CARRIER ioctl() calls.
> > +
> > +   rx_s_carrier_range: Set a window of expected carrier frequencies for
> > the +	receiver.  Intended to support the LIRC_SET_REC_CARRIER_RANGE
> > ioctl(). +	Due to hardware limitations, the full range may not be
> > supportable and +	the center of the supportable range may not be at the
> > exact center of +	the desired range.
> > +
> > +   [rt]x_[sg]_duty_cycle: Set or get the carrier duty cycle for the
> > receiver or +	transmitter.  Hardware limitations may mean the actual duty
> > cycle set +	varies from the desired duty_cycle.  The _g_ calls should
> > return the +	precise duty cycle set.  These calls can also be used to
> > +	support the LIRC_{SET,GET}_{REC,SEND}_DUTY_CYCLE ioctl() calls.
> > +
> > +   rx_s_duty_cycle_range: Set a window of expected received carrier duty
> > cycles +	Intended to support the LIRC_SET_REC_DUTY_CYCLE_RANGE ioctl().
> > +	Due to hardware limitations, the full range may not be supportable and
> > +	the center of the supportable range may not be at the exact center of
> > +	the desired range.
> > +
> > +   rx_g_resolution: get the pulse measurment resolution of the receiver.
> > +	Intended to support the LIRC_GET_REC_RESOLUTION ioctl().
> > +
> > +   tx_s_mask: set the mask of enabled transmitters for devices that have
> > +	more than one trasnmitter.  Intended to support the
> > +	LIRC_SET_TRANSMITTER_MASK ioctl().
> > + */
> > +enum v4l2_subdev_ir_event {
> > +	V4L2_SUBDEV_IR_RX_DATA_READY     = 0,
> > +	V4L2_SUBDEV_IR_TX_READY_FOR_DATA = 1,
> > +};
> > +
> > +typedef int (*v4l2_subdev_ir_notify_callback)(void *priv,
> > +					      struct v4l2_subdev *sd,
> > +					      enum v4l2_subdev_ir_event event);
> > +
> > +struct v4l2_subdev_ir_ops {
> > +	/* Common to receiver and transmitter */
> > +	int (*interrupt_service_routine)(struct v4l2_subdev *sd,
> > +						u32 status, bool *handled);
> > +
> > +	/* LIRC ioctl inspired calls */
> > +	int (*g_features)(struct v4l2_subdev *sd, u32 *features);
> > +	int (*g_code_length)(struct v4l2_subdev *sd, u32 *bits);
> > +
> > +	/* Receiver */
> > +	int (*rx_s_notify_callback)(struct v4l2_subdev *sd,
> > +					v4l2_subdev_ir_notify_callback callback,
> > +					void *priv);
> > +
> > +	int (*rx_read_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
> > +					size_t count, bool in_ns, ssize_t *num);
> > +	int (*rx_read)(struct v4l2_subdev *sd, u8 *buf, size_t count,
> > +				ssize_t *num);
> > +
> > +	int (*rx_enable)(struct v4l2_subdev *sd, bool enable);
> > +	int (*rx_shutdown)(struct v4l2_subdev *sd);
> > +
> > +	int (*rx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
> > +	int (*rx_s_demodulation)(struct v4l2_subdev *sd, bool enable);
> > +	int (*rx_s_noise_filter)(struct v4l2_subdev *sd, u32 min_width_ns);
> > +	int (*rx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
> > +
> > +	int (*rx_g_demodulation)(struct v4l2_subdev *sd, bool *enabled);
> > +	int (*rx_g_noise_filter)(struct v4l2_subdev *sd, u32 *min_width_ns);
> > +	int (*rx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
> > +
> > +	/* LIRC receiver ioctl inspired calls */
> > +	int (*rx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
> > +	int (*rx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
> > +	int (*rx_s_carrier_range)(struct v4l2_subdev *sd,
> > +					u32 lower, u32 upper);
> > +	int (*rx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
> > +	int (*rx_s_duty_cycle_range)(struct v4l2_subdev *sd,
> > +					u32 lower, u32 upper);
> > +
> > +	int (*rx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
> > +	int (*rx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
> > +	int (*rx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
> > +	int (*rx_g_resolution)(struct v4l2_subdev *sd, u32 *nsec);
> > +
> > +	/* Transmitter */
> > +	int (*tx_s_notify_callback)(struct v4l2_subdev *sd,
> > +					v4l2_subdev_ir_notify_callback callback,
> > +					void *priv);
> > +
> > +	int (*tx_write_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
> > +					size_t count, bool in_ns, ssize_t *num);
> > +	int (*tx_write)(struct v4l2_subdev *sd, u8 *buf, size_t count,
> > +				ssize_t *num);
> > +
> > +	int (*tx_enable)(struct v4l2_subdev *sd, bool enable);
> > +	int (*tx_shutdown)(struct v4l2_subdev *sd);
> > +
> > +	int (*tx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
> > +	int (*tx_s_modulation)(struct v4l2_subdev *sd, bool enable);
> > +	int (*tx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
> > +
> > +	int (*tx_g_modulation)(struct v4l2_subdev *sd, bool *enabled);
> > +	int (*tx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
> > +
> > +	/* LIRC transmitter ioctl inspired calls */
> > +	int (*tx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
> > +	int (*tx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
> > +	int (*tx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
> > +	int (*tx_s_mask)(struct v4l2_subdev *sd, u32 enabled_transmitters);
> > +
> > +	int (*tx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
> > +	int (*tx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
> > +	int (*tx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
> > +};
> > +
> >  struct v4l2_subdev_ops {
> >  	const struct v4l2_subdev_core_ops  *core;
> >  	const struct v4l2_subdev_tuner_ops *tuner;
> >  	const struct v4l2_subdev_audio_ops *audio;
> >  	const struct v4l2_subdev_video_ops *video;
> > +	const struct v4l2_subdev_ir_ops    *ir;
> >  };
> >
> >  #define V4L2_SUBDEV_NAME_SIZE 32
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Aug. 22, 2009, 4:04 p.m. UTC | #3
Em Fri, 21 Aug 2009 22:09:00 -0400
Andy Walls <awalls@radix.net> escreveu:

> In the course of implementing code to run the Consumer IR circuitry in
> the CX2584[0123] chip and similar cores, I need to add
> v4l2_subdev_ir_ops for manipulating the device and fetch and send data.
> 
> In line below is a proposal at what I think those subdev ops might be.
> Feel free to take shots at it.
> 
> The insipration for these comes from two sources, the LIRC v0.8.5 source
> code and what the CX2584x chip is capable of doing:
> 
> http://prdownloads.sourceforge.net/lirc/lirc-0.8.5.tar.bz2
> http://dl.ivtvdriver.org/datasheets/video/cx25840.pdf
> 
> Note the Consumer IR in the CX2584x can theoretically measure
> unmodulated marks and spaces with a resolution of 37.037... ns (1/27
> MHz), hence the references to nanoseconds in the proposal below.  Most
> IR devices use pulses on the order of microseconds in real life.

LIRC uses a very low level way to communicate with IR since it were originally
designed to work with just an IR receiver plugged at some serial port.

While on some very low cost devices you can still see this kind of approach,
modern devices has some IR decoding chip that can handle one or more IR
protocol directly, returning back scan codes directly. There are even some
video chips with such capability inside, like em2860 and upper designs.

In a matter of fact, it is very common to have the same bridge driver used with
both dedicated IR decoding chips and with low cost IR's. For example, if you
take a look on saa7134 driver, you'll see devices where:
	1) the IR is directly connected to a GPIO pin;
	2) IR is at a GPIO pin that generates interrupts;
	3) chips like ks007 (a low-cost micro controller with a IR decoding
firmware inside) connected in parallel at several GPIO pins;
	4) i2c IR chips.

Also, for the low cost devices, ir-functions.c already has the code to
transform several common protocols into scan codes.

From the way you've designed the API, I think that you're wanting to write some
interface to communicate to LIRC, right? If this is the case, I think that there
are several missing parts that needs to be addressed, in terms of architecture.

The more important one is what kind of IR interfaces a V4L2 device will open to
lirc. For now (upstream), the only "official" interface defined is events
interface, used by lirc-events daemon. Still, it misses one feature: there's no
way to ask a V4L2 device to use a different IR protocol. So, we need a set of
GET_CAP, GET, SET at events interface especially designed to be used by IR.

Currently, there's no uniform way for doing it. For example, if you take a look
at lirc_gpio.c, you'll see that it depends on a patch, to be applied on
bttv-if.c[1], that exposes the board name and a wait_queue that is waked when an
IRQ with a IR remote key is generated. Btw, lirc_gpio.c only knows how to handle
with the bttv driver. IMHO, it should be named instead as lirc_bttv.

If you take a look at lirc_i2.c, it directly sends ir commands to the i2c device:

        i2c_master_send(&ir->c, keybuf, 1);
        /* poll IR chip */
        if (i2c_master_recv(&ir->c, keybuf, sizeof(keybuf)) != sizeof(keybuf)) {
                dprintk("read error\n");
                return -EIO;
        }

Btw, as there's no lock between send and receive ops, there's a risk of the driver to
try to use the I2C bus for something else in the middle of the send/recv, causing
data corruption - eventually, this might explain some cases of eeprom corruption
reported at ML.

In brief, IMO, we should first write a v4l2 device interface for it, capable of
working with all types of IR devices, using input event interface where
applied, and then address v4l2 subdev interfaces where needed.

In order to better discuss it, it would be nice to have some patches for a
driver showing some use cases.

[1] http://www.lirc.org/software/snapshots/lirc-bttv-linux-2.6.24.patch

Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Andy Walls Aug. 22, 2009, 9:27 p.m. UTC | #4
On Sat, 2009-08-22 at 13:04 -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 21 Aug 2009 22:09:00 -0400
> Andy Walls <awalls@radix.net> escreveu:
> 
> > In the course of implementing code to run the Consumer IR circuitry in
> > the CX2584[0123] chip and similar cores, I need to add
> > v4l2_subdev_ir_ops for manipulating the device and fetch and send data.
> > 
> > In line below is a proposal at what I think those subdev ops might be.
> > Feel free to take shots at it.
> > 
> > The insipration for these comes from two sources, the LIRC v0.8.5 source
> > code and what the CX2584x chip is capable of doing:
> > 
> > http://prdownloads.sourceforge.net/lirc/lirc-0.8.5.tar.bz2
> > http://dl.ivtvdriver.org/datasheets/video/cx25840.pdf
> > 
> > Note the Consumer IR in the CX2584x can theoretically measure
> > unmodulated marks and spaces with a resolution of 37.037... ns (1/27
> > MHz), hence the references to nanoseconds in the proposal below.  Most
> > IR devices use pulses on the order of microseconds in real life.

Mauro,

Thank you for your comments.


> LIRC uses a very low level way to communicate with IR since it were originally
> designed to work with just an IR receiver plugged at some serial port.
> 
> While on some very low cost devices you can still see this kind of approach,
> modern devices has some IR decoding chip that can handle one or more IR
> protocol directly, returning back scan codes directly. There are even some
> video chips with such capability inside, like em2860 and upper designs.
> 
> In a matter of fact, it is very common to have the same bridge driver used with
> both dedicated IR decoding chips and with low cost IR's. For example, if you
> take a look on saa7134 driver, you'll see devices where:
> 	1) the IR is directly connected to a GPIO pin;
> 	2) IR is at a GPIO pin that generates interrupts;
> 	3) chips like ks007 (a low-cost micro controller with a IR decoding
> firmware inside) connected in parallel at several GPIO pins;
> 	4) i2c IR chips.

Right, so we have several type of IR devices, and the v4l2_subdev_ir_ops
interface needs to accomadte all of them eventually - in a way that
makes sense.

Just to reiterate what you said (to make sure I understand), the IR
receiver devices are:

1) A simple bit (GPIO pin) that must be polled and manually timed to
extract pulse widths

2) A bit (GPIO pin) that can generate an interrupt but timing
measurement is still manual 

3) A microcontroller with a parallel (GPIO pins) output of the codes.

4) An i2c connected microcontroller or IR chip that outputs codes.


I have two devices that don't quite fit in that set:

a. The CX23885 has an IR device, which is part of a larger I2C-connected
integrated subdevice, that provides pulse timings via i2c register
reads.

b. The CX23888 has an integrated IR device that provides pulse timings
via local register reads.  

I think these fall somewhere between 2) and 3) in the above list.  None
of the code in saa7134 quite does what I need, but thank you for the
high level overview to help me figure it out.


> Also, for the low cost devices, ir-functions.c already has the code to
> transform several common protocols into scan codes.

Yes I was planning on leveraging those.  I was probably going to add
some.  I hadn't gotten there yet.

I figured if the in kernel ir-functions.c file didn't have a pulse width
to RC-5 protocol conversion routine, that I would have to add one.

Once I have that, I can use the already existing ir_decode_biphase()
function, remote keymaps, etc.

This was all something I thought should not be in the v4l2_subdev for
the CX2388x IR.



> >From the way you've designed the API, I think that you're wanting to write some
> interface to communicate to LIRC, right?  If this is the case, I think that there
> are several missing parts that needs to be addressed, in terms of architecture.

I was trying to develop an interface that supported both the in kernnel
ir-functions.c functions and LIRC.   Yes, you are correct.  Both will
need more glue than what the v4l2_subdev_ir_ops interface will provide.

My intent was to provide an interface that made that "glue" as simple as
possible (for LIRC for sure) and hopefully for an in kernel
implementation using the ir-functions.c functions.

> The more important one is what kind of IR interfaces a V4L2 device will open to
> lirc.

I was thinking a lirc-v4l2-subdev plugin for lirc-dev.  If the
v4l2_subdev_ir_ops interface aligned or can easily translate to
supporting things LIRC needed, then follow on support for all video
related IR devices would fall out from implementing the
v4l2_subdev_ir_ops interface.  (In my optimistic version of the
future ;] ).

What I wouldn't want to happen iss a lirc-conexant plugin for lirc-dev,
and a lirc-foo plugin for lirc-dev, and etc.

It was my desire to try to keep the bridge drivers mostly out of the
loop aside from device setup and hook-up to another layer: be that the
input layer or LIRC somehow.


>  For now (upstream), the only "official" interface defined is events
> interface, used by lirc-events daemon.

Yup.  The input/events interface will be the first interface I
implement.  I suspect, it satsifies the >90% use case.


>  Still, it misses one feature: there's no
> way to ask a V4L2 device to use a different IR protocol. So, we need a set of
> GET_CAP, GET, SET at events interface especially designed to be used by IR.
> 
> Currently, there's no uniform way for doing it. For example, if you take a look
> at lirc_gpio.c, you'll see that it depends on a patch, to be applied on
> bttv-if.c[1], that exposes the board name and a wait_queue that is waked when an
> IRQ with a IR remote key is generated.



>  Btw, lirc_gpio.c only knows how to handle
> with the bttv driver. IMHO, it should be named instead as lirc_bttv.

I didn't know that.

All the more reason to get a uniform v4l2_subdev_ir_ops interface and
start encapsulating things.

I'd much rather see a migration to a lirc_v4l2_subdev plugin for
lirc-dev.  That would get rid of the "stray cats & dogs" and could even
unify the I2C vs. non-I2c IR devices into one interface.  (The whole
point of Hans' v4l2_subdev class).

Of course LIRC is out of kernel right now, so it is not a priority I
assume.


> If you take a look at lirc_i2.c, it directly sends ir commands to the i2c device:
> 
>         i2c_master_send(&ir->c, keybuf, 1);
>         /* poll IR chip */
>         if (i2c_master_recv(&ir->c, keybuf, sizeof(keybuf)) != sizeof(keybuf)) {
>                 dprintk("read error\n");
>                 return -EIO;
>         }
> 
> Btw, as there's no lock between send and receive ops, there's a risk of the driver to
> try to use the I2C bus for something else in the middle of the send/recv, causing
> data corruption - eventually, this might explain some cases of eeprom corruption
> reported at ML.

Oooh.  Thanks for reminding me.  That's not an issue for the CX23888 but
will be for the CX23885.


> In brief, IMO, we should first write a v4l2 device interface for it, capable of
> working with all types of IR devices, using input event interface where
> applied, and then address v4l2 subdev interfaces where needed.
> 
> In order to better discuss it, it would be nice to have some patches for a
> driver showing some use cases.

I'm in the process of writing something for the CX23888.  What I first
implement will be closer to the minimal set needed as Hans suggested.  I
will keep in mind all the types of IR implementations that you
mentioned.

The input event interface will be a separate set of routines in the
cx23885 driver that uses what code I can from ir-functions.c.

Hopefully I'll have the initial CX23888 driver working before the LPC.
I can then talk face to face with Hans at least. IIRC you stated you
were not going to the LPC because of the Kernel Summit.


Again, thanks for information and comments.

Regards,
Andy

> [1] http://www.lirc.org/software/snapshots/lirc-bttv-linux-2.6.24.patch
> 
> Cheers,
> Mauro.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Aug. 23, 2009, 6 p.m. UTC | #5
Em Sat, 22 Aug 2009 17:27:50 -0400
Andy Walls <awalls@radix.net> escreveu:

> Right, so we have several type of IR devices, and the v4l2_subdev_ir_ops
> interface needs to accomadte all of them eventually - in a way that
> makes sense.
> 
> Just to reiterate what you said (to make sure I understand), the IR
> receiver devices are:
> 
> 1) A simple bit (GPIO pin) that must be polled and manually timed to
> extract pulse widths
> 
> 2) A bit (GPIO pin) that can generate an interrupt but timing
> measurement is still manual 
> 
> 3) A microcontroller with a parallel (GPIO pins) output of the codes.
> 
> 4) An i2c connected microcontroller or IR chip that outputs codes.
> 
> 
> I have two devices that don't quite fit in that set:
> 
> a. The CX23885 has an IR device, which is part of a larger I2C-connected
> integrated subdevice, that provides pulse timings via i2c register
> reads.
> 
> b. The CX23888 has an integrated IR device that provides pulse timings
> via local register reads.  
> 
> I think these fall somewhere between 2) and 3) in the above list.  None
> of the code in saa7134 quite does what I need, but thank you for the
> high level overview to help me figure it out.

Ok, so, there are currently 5 known different sets of IR. I think I've seen at
the datasheets or at the driver code for something like (b).

> > Also, for the low cost devices, ir-functions.c already has the code to
> > transform several common protocols into scan codes.
> 
> Yes I was planning on leveraging those.  I was probably going to add
> some.  I hadn't gotten there yet.
> 
> I figured if the in kernel ir-functions.c file didn't have a pulse width
> to RC-5 protocol conversion routine, that I would have to add one.
> 
> Once I have that, I can use the already existing ir_decode_biphase()
> function, remote keymaps, etc.

You'll see rc5 conversion code at bttv-input (bttv_rc5_irq), and a nec
conversion code at saa7134-input (saa7134_nec_timer, nec_task and
saa7134_nec_irq). It shouldn't be hard to move they to ir-common.c, to be used on
other places where they're needed.

> > The more important one is what kind of IR interfaces a V4L2 device will open to
> > lirc.
> 
> I was thinking a lirc-v4l2-subdev plugin for lirc-dev.  If the
> v4l2_subdev_ir_ops interface aligned or can easily translate to
> supporting things LIRC needed, then follow on support for all video
> related IR devices would fall out from implementing the
> v4l2_subdev_ir_ops interface.  (In my optimistic version of the
> future ;] ).

This won't work, due to two reasons:

1) a subdev interface works for the devices with i2c interfaces (e. g. the subdev's),
but it won't work on other cases;

2) even when you'll have this at i2c, you may still need to properly lock the IR code
to avoid that the IR operation to happen in the middle of another i2c transfer.

So, IMO, we need to create a lirc type of ops handled by the device itself.
Those ops will take care of interfacing with subdevs in the case that this
applies.

> What I wouldn't want to happen iss a lirc-conexant plugin for lirc-dev,
> and a lirc-foo plugin for lirc-dev, and etc.
> 
> It was my desire to try to keep the bridge drivers mostly out of the
> loop aside from device setup and hook-up to another layer: be that the
> input layer or LIRC somehow.

This seems to be the proper approach to me.

> >  For now (upstream), the only "official" interface defined is events
> > interface, used by lirc-events daemon.
> 
> Yup.  The input/events interface will be the first interface I
> implement.  I suspect, it satsifies the >90% use case.

Yes, I think so. We need to discuss with the events people a way to allow
selecting, from userspace, what IR protocols are supported by a given device
and allow to get/set the kind of IR protocol that will be used. 

> >  Btw, lirc_gpio.c only knows how to handle
> > with the bttv driver. IMHO, it should be named instead as lirc_bttv.
> 
> I didn't know that.
> 
> All the more reason to get a uniform v4l2_subdev_ir_ops interface and
> start encapsulating things.

Agreed. IMHO, in the specific case of lirc_gpio, I suspect that most (or maybe all) of the
code should be merged inside bttv-input, to be easier for new board additions to receive
their appropriate setup to retrieve IR keys also since their addition at the kernel.

> I'd much rather see a migration to a lirc_v4l2_subdev plugin for
> lirc-dev.  That would get rid of the "stray cats & dogs" and could even
> unify the I2C vs. non-I2c IR devices into one interface.  (The whole
> point of Hans' v4l2_subdev class).

See above. This should be unified outside subdev, since, on most cases, the IR
is handled directly by the bridge code. Another alternative would be to create
a fake subdev for the IR handling code, but, IMHO, such change will be large and
I don't see much gain on doing it. Also, since on several cases it shares the bridge
IRQ code, the IR handling code will be broken on just a layer interface at the
subdev, but the real work will be handled at the bridge driver.
> 
> Of course LIRC is out of kernel right now, so it is not a priority I
> assume.

In the case of the event interface, this is independent of lirc, since there's no
need of having an extra kernel driver for it.

Yet, at lirc side, at least with the version I have on RHEL5 (0.8.4a), it
didn't work well with usb devices, since those devices are hot-pluggable.
In order to work, lirc needs the name of the event interface at the command
line:

$ lircd -H devinput -d /dev/input/event6

However, the event interface is only known after plugging the device. So, you
cannot have lircd initialized during boot time, if your device is not already
plugged. Also, if you unplug the device while there are some client listening
to it, the daemon dies.

IMHO, it should be providing a driver that checks for the creation of e
at /dev/video? and, if a node is found, it should do the a procedure similar to
what is done by v4l2-apps/util/v4l2-sysfs-path (the latest version at
http://linuxtv.org/hg/v4l-dvb tree), to determine the associated event device.
For example, with my HVR-950, it returns:

device     = /dev/video0
bus info   = usb-0000:00:1d.7-8
sysfs path = /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8
Associated devices:
        i2c-adapter:i2c-4
        input:input12:event6 (dev 13,70)
        sound:pcmC1D0c (dev 116,9)
        sound:dsp1 (dev 14,19)
        sound:audio1 (dev 14,20)
        sound:controlC1 (dev 116,10)
        sound:mixer1 (dev 14,16)
        dvb:dvb0.frontend0 (dev 212,0)
        dvb:dvb0.demux0 (dev 212,1)
        dvb:dvb0.dvr0 (dev 212,2)
        dvb:dvb0.net0 (dev 212,3)
        usb_endpoint:usbdev1.11_ep00 (dev 252,20)

So, the event interface associated with /dev/video0 is /dev/input/event6 (minor
13, major 70), as reported above.

> > In order to better discuss it, it would be nice to have some patches for a
> > driver showing some use cases.
> 
> I'm in the process of writing something for the CX23888.  What I first
> implement will be closer to the minimal set needed as Hans suggested.  I
> will keep in mind all the types of IR implementations that you
> mentioned.
> 
> The input event interface will be a separate set of routines in the
> cx23885 driver that uses what code I can from ir-functions.c.

I suggest you to have them at *-input.c file, to keep the same convention as
the other IR drivers. This makes easier for people to know where the IR
specific code is.

> Hopefully I'll have the initial CX23888 driver working before the LPC.
> I can then talk face to face with Hans at least. IIRC you stated you
> were not going to the LPC because of the Kernel Summit.

I would love to go to LPC, but it is hard to attend to both events, since they'll be
close and both are very long trips. Also, IMO, being in Japan for KS and JLC
will be an interesting opportunity to meet with people in Asia and spread our
work there. I'll try to be at LPC for the next year.

> Again, thanks for information and comments.

Anytime.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil Aug. 23, 2009, 7:16 p.m. UTC | #6
On Sunday 23 August 2009 20:00:00 Mauro Carvalho Chehab wrote:
> Em Sat, 22 Aug 2009 17:27:50 -0400
>
> Andy Walls <awalls@radix.net> escreveu:
> > > The more important one is what kind of IR interfaces a V4L2 device
> > > will open to lirc.
> >
> > I was thinking a lirc-v4l2-subdev plugin for lirc-dev.  If the
> > v4l2_subdev_ir_ops interface aligned or can easily translate to
> > supporting things LIRC needed, then follow on support for all video
> > related IR devices would fall out from implementing the
> > v4l2_subdev_ir_ops interface.  (In my optimistic version of the
> > future ;] ).
>
> This won't work, due to two reasons:
>
> 1) a subdev interface works for the devices with i2c interfaces (e. g.
> the subdev's), but it won't work on other cases;

Subdev is actually independent of i2c, that was the one of the reasons for 
making v4l2_subdev in the first place. Both the cx18 and ivtv drivers use a 
non-i2c subdev in fact. It does rely on there being a parent v4l2_device 
struct with which it will be registered. In other words: while there may be 
good reasons for not using v4l2_subdev, this reason isn't one of them.

> 2) even when you'll have this at i2c, you may still need to properly lock
> the IR code to avoid that the IR operation to happen in the middle of
> another i2c transfer.

I could well be wrong, but isn't the i2c_adapter already locking at the bus 
level?

> So, IMO, we need to create a lirc type of ops handled by the device
> itself. Those ops will take care of interfacing with subdevs in the case
> that this applies.
>
> > What I wouldn't want to happen iss a lirc-conexant plugin for lirc-dev,
> > and a lirc-foo plugin for lirc-dev, and etc.
> >
> > It was my desire to try to keep the bridge drivers mostly out of the
> > loop aside from device setup and hook-up to another layer: be that the
> > input layer or LIRC somehow.
>
> This seems to be the proper approach to me.
>
> > >  For now (upstream), the only "official" interface defined is events
> > > interface, used by lirc-events daemon.
> >
> > Yup.  The input/events interface will be the first interface I
> > implement.  I suspect, it satsifies the >90% use case.
>
> Yes, I think so. We need to discuss with the events people a way to allow
> selecting, from userspace, what IR protocols are supported by a given
> device and allow to get/set the kind of IR protocol that will be used.
>
> > >  Btw, lirc_gpio.c only knows how to handle
> > > with the bttv driver. IMHO, it should be named instead as lirc_bttv.
> >
> > I didn't know that.
> >
> > All the more reason to get a uniform v4l2_subdev_ir_ops interface and
> > start encapsulating things.

BTW, Andy: I liked your latest proposal for these ops much better. Seems to 
me to be an excellent starting point.

> Agreed. IMHO, in the specific case of lirc_gpio, I suspect that most (or
> maybe all) of the code should be merged inside bttv-input, to be easier
> for new board additions to receive their appropriate setup to retrieve IR
> keys also since their addition at the kernel.
>
> > I'd much rather see a migration to a lirc_v4l2_subdev plugin for
> > lirc-dev.  That would get rid of the "stray cats & dogs" and could even
> > unify the I2C vs. non-I2c IR devices into one interface.  (The whole
> > point of Hans' v4l2_subdev class).
>
> See above. This should be unified outside subdev, since, on most cases,
> the IR is handled directly by the bridge code. Another alternative would
> be to create a fake subdev for the IR handling code, but, IMHO, such
> change will be large and I don't see much gain on doing it. Also, since
> on several cases it shares the bridge IRQ code, the IR handling code will
> be broken on just a layer interface at the subdev, but the real work will
> be handled at the bridge driver.

It is a perfectly legitimate approach to use a subdev for IR handling. 
Usually it is some separate set of registers or IC block anyway, whether it 
is a bridge driver or a complex chip like cx2584x. The v4l2_subdev approach 
allows one to model such things in a relatively high-level manner. Andy has 
experience doing exactly that in the cx18 driver, so he is very well placed 
to investigate which approach is best.

It is very important that everyone realizes that the sub-device construct 
basically is an abstract model of the hardware, independent of any busses 
or where that hardware actually is (i.e. as a part of a larger IC or a 
self-contained i2c device). In 95% of the cases it does indeed model an i2c 
device, but that is just because these are so common.

In fact, it could be especially useful in cases where the i2c support is 
sometimes in a bridge, sometimes in an i2c device (or more than one). If 
the actual API is through a v4l2_subdev, then that will make everything 
nicely consistent.

Of course, the proof of the pudding is in the eating, so try it in a few 
drivers. You generally discover very quickly whether something is a good 
API or not.

> > Of course LIRC is out of kernel right now, so it is not a priority I
> > assume.
>
> In the case of the event interface, this is independent of lirc, since
> there's no need of having an extra kernel driver for it.
>
> Yet, at lirc side, at least with the version I have on RHEL5 (0.8.4a), it
> didn't work well with usb devices, since those devices are hot-pluggable.
> In order to work, lirc needs the name of the event interface at the
> command line:
>
> $ lircd -H devinput -d /dev/input/event6
>
> However, the event interface is only known after plugging the device. So,
> you cannot have lircd initialized during boot time, if your device is not
> already plugged. Also, if you unplug the device while there are some
> client listening to it, the daemon dies.

Yeah, that really sucks. I had the same problems, and it's very annoying.

> IMHO, it should be providing a driver that checks for the creation of e
> at /dev/video? and, if a node is found, it should do the a procedure
> similar to what is done by v4l2-apps/util/v4l2-sysfs-path (the latest
> version at http://linuxtv.org/hg/v4l-dvb tree), to determine the
> associated event device. For example, with my HVR-950, it returns:
>
> device     = /dev/video0
> bus info   = usb-0000:00:1d.7-8
> sysfs path = /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8
> Associated devices:
>         i2c-adapter:i2c-4
>         input:input12:event6 (dev 13,70)
>         sound:pcmC1D0c (dev 116,9)
>         sound:dsp1 (dev 14,19)
>         sound:audio1 (dev 14,20)
>         sound:controlC1 (dev 116,10)
>         sound:mixer1 (dev 14,16)
>         dvb:dvb0.frontend0 (dev 212,0)
>         dvb:dvb0.demux0 (dev 212,1)
>         dvb:dvb0.dvr0 (dev 212,2)
>         dvb:dvb0.net0 (dev 212,3)
>         usb_endpoint:usbdev1.11_ep00 (dev 252,20)
>
> So, the event interface associated with /dev/video0 is /dev/input/event6
> (minor 13, major 70), as reported above.

Mauro, you should consider using libudev to map major/minor device numbers 
to the actual device nodes. Right now no udev rules are taken into account. 
So if someone renames a device node you will never find it.

> > > In order to better discuss it, it would be nice to have some patches
> > > for a driver showing some use cases.
> >
> > I'm in the process of writing something for the CX23888.  What I first
> > implement will be closer to the minimal set needed as Hans suggested. 
> > I will keep in mind all the types of IR implementations that you
> > mentioned.
> >
> > The input event interface will be a separate set of routines in the
> > cx23885 driver that uses what code I can from ir-functions.c.
>
> I suggest you to have them at *-input.c file, to keep the same convention
> as the other IR drivers. This makes easier for people to know where the
> IR specific code is.
>
> > Hopefully I'll have the initial CX23888 driver working before the LPC.
> > I can then talk face to face with Hans at least. IIRC you stated you
> > were not going to the LPC because of the Kernel Summit.
>
> I would love to go to LPC, but it is hard to attend to both events, since
> they'll be close and both are very long trips. Also, IMO, being in Japan
> for KS and JLC will be an interesting opportunity to meet with people in
> Asia and spread our work there. I'll try to be at LPC for the next year.
>
> > Again, thanks for information and comments.
>
> Anytime.

Regards,

	Hans
  
Mauro Carvalho Chehab Aug. 24, 2009, 12:05 a.m. UTC | #7
Em Sun, 23 Aug 2009 21:16:36 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Sunday 23 August 2009 20:00:00 Mauro Carvalho Chehab wrote:
> > Em Sat, 22 Aug 2009 17:27:50 -0400
> >
> > Andy Walls <awalls@radix.net> escreveu:
> > > > The more important one is what kind of IR interfaces a V4L2 device
> > > > will open to lirc.
> > >
> > > I was thinking a lirc-v4l2-subdev plugin for lirc-dev.  If the
> > > v4l2_subdev_ir_ops interface aligned or can easily translate to
> > > supporting things LIRC needed, then follow on support for all video
> > > related IR devices would fall out from implementing the
> > > v4l2_subdev_ir_ops interface.  (In my optimistic version of the
> > > future ;] ).
> >
> > This won't work, due to two reasons:
> >
> > 1) a subdev interface works for the devices with i2c interfaces (e. g.
> > the subdev's), but it won't work on other cases;
> 
> Subdev is actually independent of i2c, that was the one of the reasons for 
> making v4l2_subdev in the first place. Both the cx18 and ivtv drivers use a 
> non-i2c subdev in fact. It does rely on there being a parent v4l2_device 
> struct with which it will be registered. In other words: while there may be 
> good reasons for not using v4l2_subdev, this reason isn't one of them.

See bellow.

> > 2) even when you'll have this at i2c, you may still need to properly lock
> > the IR code to avoid that the IR operation to happen in the middle of
> > another i2c transfer.
> 
> I could well be wrong, but isn't the i2c_adapter already locking at the bus 
> level?

It won't lock. The issue is outside i2c code. Probably you haven't seen the
code I've posted on my previous email. Lirc does 2 separate transfers, one for
send and another for receive:

        i2c_master_send(&ir->c, keybuf, 1);
        /* poll IR chip */
        if (i2c_master_recv(&ir->c, keybuf, sizeof(keybuf)) != sizeof(keybuf)) {
                dprintk("read error\n");
                return -EIO;
        }

Between the two independent transactions, an ioctl or a kernel thread may
happen doing some stuff at i2c. You'll then have an i2c send where an i2c
receive operation were expected to happen. This is known to cause some problems
with some i2c implementations on some chips, including some eeprom's.

In this specific case, one possible solution could be to replace them by
i2c_master_xfer().

> > See above. This should be unified outside subdev, since, on most cases,
> > the IR is handled directly by the bridge code. Another alternative would
> > be to create a fake subdev for the IR handling code, but, IMHO, such
> > change will be large and I don't see much gain on doing it. Also, since
> > on several cases it shares the bridge IRQ code, the IR handling code will
> > be broken on just a layer interface at the subdev, but the real work will
> > be handled at the bridge driver.
> 
> It is a perfectly legitimate approach to use a subdev for IR handling. 
> Usually it is some separate set of registers or IC block anyway, whether it 
> is a bridge driver or a complex chip like cx2584x. 

No. Having a different sets of register for IR is generally the exception. I
guess that, on 80-90% of the cases, it uses the same GPIO set of registers used
by other parts of the driver. Just taking a few real examples: almost all bttv
and cx88 are based on GPIO poll; on saa7134, just a few uses i2c. The great
majority has a mix of GPIO poll or GPIO IRQ. If you take a look at the IRQ
code, it is the same code used also by videobuf. On em28xx, you have 3 types:
i2c, GPIO poll (mostly on devices with em282x and em284x) and some dedicated IR
registers found on hardware with em286x/em288x (yet, a few of those devices
still uses the old strategy).

Ok, it would be possible to artificially add a layer in order to deal with some
bits of those common registers differently, and to create an IRQ ops abstraction
layer for IR but this is overkill. It will just add an extra abstraction layer
for nothing. It may even interfere at the IR protocol handling, as, on those
GPIO poll code, extra delays may interfere at the decoding, since the processor
is responsible for measuring the times.

> The v4l2_subdev approach 
> allows one to model such things in a relatively high-level manner. Andy has 
> experience doing exactly that in the cx18 driver, so he is very well placed 
> to investigate which approach is best.

The v4l2_subdev approach works fine for the communication between a bridge
driver and their ancillary sub-devices. There's nothing wrong on proposing an
IR interface for that usage, in the cases where a separate IC chip is measuring
the time or returning scan codes, like the cases where Andy mentioned.

However, just letting lirc (or any other IR driver) to directly access a
v4l_subdev without passing it via the bridge driver is risky, since bridge
cannot protect the shared registers or avoid that another subdev call won't be
interfered by IR.

In other words, I'm seeing those valid scenarios:

(1)	lirc -> v4l_device -> v4l_subdev -> IR;
(2)	lirc -> v4l_device -> v4l_subdev -> subdev gpio -> IR;
(3)	lirc -> v4l_device -> IR.

But those scenarios:
(4)	lirc -> v4l_subdev -> IR
(5)	lirc -> v4l_subdev -> subdev gpio -> IR;

Shouldn't be allowed since:

1) it will break IR support for the scenario (3), as no v4l_subdev knows how to
deal with almost all of bttv, cx88, saa7134 and em28xx IR's;

2) some sort of memory barrier may be needed at v4l_device.

> It is very important that everyone realizes that the sub-device construct 
> basically is an abstract model of the hardware, independent of any busses 
> or where that hardware actually is (i.e. as a part of a larger IC or a 
> self-contained i2c device). In 95% of the cases it does indeed model an i2c 
> device, but that is just because these are so common.
> 
> In fact, it could be especially useful in cases where the i2c support is 
> sometimes in a bridge, sometimes in an i2c device (or more than one). If 
> the actual API is through a v4l2_subdev, then that will make everything 
> nicely consistent.

> Of course, the proof of the pudding is in the eating, so try it in a few 
> drivers. You generally discover very quickly whether something is a good 
> API or not.

Ok, those are valid uses, but it is not the better solution for sharing device
resources like IRQ and GPIO's. I'm all for using it in the cases that Andy
pointed in scenarios (1), (2) and (3).

> > IMHO, it should be providing a driver that checks for the creation of e
> > at /dev/video? and, if a node is found, it should do the a procedure
> > similar to what is done by v4l2-apps/util/v4l2-sysfs-path (the latest
> > version at http://linuxtv.org/hg/v4l-dvb tree), to determine the
> > associated event device. For example, with my HVR-950, it returns:
> >
> > device     = /dev/video0
> > bus info   = usb-0000:00:1d.7-8
> > sysfs path = /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8
> > Associated devices:
> >         i2c-adapter:i2c-4
> >         input:input12:event6 (dev 13,70)
> >         sound:pcmC1D0c (dev 116,9)
> >         sound:dsp1 (dev 14,19)
> >         sound:audio1 (dev 14,20)
> >         sound:controlC1 (dev 116,10)
> >         sound:mixer1 (dev 14,16)
> >         dvb:dvb0.frontend0 (dev 212,0)
> >         dvb:dvb0.demux0 (dev 212,1)
> >         dvb:dvb0.dvr0 (dev 212,2)
> >         dvb:dvb0.net0 (dev 212,3)
> >         usb_endpoint:usbdev1.11_ep00 (dev 252,20)
> >
> > So, the event interface associated with /dev/video0 is /dev/input/event6
> > (minor 13, major 70), as reported above.
> 
> Mauro, you should consider using libudev to map major/minor device numbers 
> to the actual device nodes. Right now no udev rules are taken into account. 
> So if someone renames a device node you will never find it.

Thank you for your suggestion.

I've considered to use also libudev for it, but, while seeking the net for some
coding example, this patch appeared:

http://lists.lm-sensors.org/pipermail/lm-sensors/2007-November/022043.html

There, Jean proposed a patch to get rid of libudev since, on that time, libudev
weren't maintained anymore, according to him. I haven't check if this is still
true or not, but, as all I wanted with that code is to get the event name and
the device, I took, instead, the approach that gave me the quickest solution.
For sure using a well maintained library is much better than having to reinvent
the wheel.

In a matter of fact, to be really useful, the better would be if
v4l2-sysfs-path could return the path of the devices instead of just the
major/minor.

Please, feel free to propose some patches to improve it, if you have time for it.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil Aug. 24, 2009, 8 a.m. UTC | #8
On Monday 24 August 2009 02:05:06 Mauro Carvalho Chehab wrote:
> Em Sun, 23 Aug 2009 21:16:36 +0200
>
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > On Sunday 23 August 2009 20:00:00 Mauro Carvalho Chehab wrote:
> > > Em Sat, 22 Aug 2009 17:27:50 -0400
> > >
> > > Andy Walls <awalls@radix.net> escreveu:
> > > > > The more important one is what kind of IR interfaces a V4L2
> > > > > device will open to lirc.
> > > >
> > > > I was thinking a lirc-v4l2-subdev plugin for lirc-dev.  If the
> > > > v4l2_subdev_ir_ops interface aligned or can easily translate to
> > > > supporting things LIRC needed, then follow on support for all video
> > > > related IR devices would fall out from implementing the
> > > > v4l2_subdev_ir_ops interface.  (In my optimistic version of the
> > > > future ;] ).
> > >
> > > This won't work, due to two reasons:
> > >
> > > 1) a subdev interface works for the devices with i2c interfaces (e.
> > > g. the subdev's), but it won't work on other cases;
> >
> > Subdev is actually independent of i2c, that was the one of the reasons
> > for making v4l2_subdev in the first place. Both the cx18 and ivtv
> > drivers use a non-i2c subdev in fact. It does rely on there being a
> > parent v4l2_device struct with which it will be registered. In other
> > words: while there may be good reasons for not using v4l2_subdev, this
> > reason isn't one of them.
>
> See bellow.
>
> > > 2) even when you'll have this at i2c, you may still need to properly
> > > lock the IR code to avoid that the IR operation to happen in the
> > > middle of another i2c transfer.
> >
> > I could well be wrong, but isn't the i2c_adapter already locking at the
> > bus level?
>
> It won't lock. The issue is outside i2c code. Probably you haven't seen
> the code I've posted on my previous email. Lirc does 2 separate
> transfers, one for send and another for receive:
>
>         i2c_master_send(&ir->c, keybuf, 1);
>         /* poll IR chip */
>         if (i2c_master_recv(&ir->c, keybuf, sizeof(keybuf)) !=
> sizeof(keybuf)) { dprintk("read error\n");
>                 return -EIO;
>         }
>
> Between the two independent transactions, an ioctl or a kernel thread may
> happen doing some stuff at i2c. You'll then have an i2c send where an i2c
> receive operation were expected to happen. This is known to cause some
> problems with some i2c implementations on some chips, including some
> eeprom's.
>
> In this specific case, one possible solution could be to replace them by
> i2c_master_xfer().
>
> > > See above. This should be unified outside subdev, since, on most
> > > cases, the IR is handled directly by the bridge code. Another
> > > alternative would be to create a fake subdev for the IR handling
> > > code, but, IMHO, such change will be large and I don't see much gain
> > > on doing it. Also, since on several cases it shares the bridge IRQ
> > > code, the IR handling code will be broken on just a layer interface
> > > at the subdev, but the real work will be handled at the bridge
> > > driver.
> >
> > It is a perfectly legitimate approach to use a subdev for IR handling.
> > Usually it is some separate set of registers or IC block anyway,
> > whether it is a bridge driver or a complex chip like cx2584x.
>
> No. Having a different sets of register for IR is generally the
> exception. I guess that, on 80-90% of the cases, it uses the same GPIO
> set of registers used by other parts of the driver. Just taking a few
> real examples: almost all bttv and cx88 are based on GPIO poll; on
> saa7134, just a few uses i2c. The great majority has a mix of GPIO poll
> or GPIO IRQ. If you take a look at the IRQ code, it is the same code used
> also by videobuf. On em28xx, you have 3 types: i2c, GPIO poll (mostly on
> devices with em282x and em284x) and some dedicated IR registers found on
> hardware with em286x/em288x (yet, a few of those devices still uses the
> old strategy).
>
> Ok, it would be possible to artificially add a layer in order to deal
> with some bits of those common registers differently, and to create an
> IRQ ops abstraction layer for IR but this is overkill. It will just add
> an extra abstraction layer for nothing. It may even interfere at the IR
> protocol handling, as, on those GPIO poll code, extra delays may
> interfere at the decoding, since the processor is responsible for
> measuring the times.
>
> > The v4l2_subdev approach
> > allows one to model such things in a relatively high-level manner. Andy
> > has experience doing exactly that in the cx18 driver, so he is very
> > well placed to investigate which approach is best.
>
> The v4l2_subdev approach works fine for the communication between a
> bridge driver and their ancillary sub-devices. There's nothing wrong on
> proposing an IR interface for that usage, in the cases where a separate
> IC chip is measuring the time or returning scan codes, like the cases
> where Andy mentioned.
>
> However, just letting lirc (or any other IR driver) to directly access a
> v4l_subdev without passing it via the bridge driver is risky, since
> bridge cannot protect the shared registers or avoid that another subdev
> call won't be interfered by IR.
>
> In other words, I'm seeing those valid scenarios:
>
> (1)	lirc -> v4l_device -> v4l_subdev -> IR;
> (2)	lirc -> v4l_device -> v4l_subdev -> subdev gpio -> IR;
> (3)	lirc -> v4l_device -> IR.
>
> But those scenarios:
> (4)	lirc -> v4l_subdev -> IR
> (5)	lirc -> v4l_subdev -> subdev gpio -> IR;
>
> Shouldn't be allowed since:

I agree with that. For now at least the v4l2_subdev struct should not be 
used outside of v4l2 drivers.

>
> 1) it will break IR support for the scenario (3), as no v4l_subdev knows
> how to deal with almost all of bttv, cx88, saa7134 and em28xx IR's;
>
> 2) some sort of memory barrier may be needed at v4l_device.
>
> > It is very important that everyone realizes that the sub-device
> > construct basically is an abstract model of the hardware, independent
> > of any busses or where that hardware actually is (i.e. as a part of a
> > larger IC or a self-contained i2c device). In 95% of the cases it does
> > indeed model an i2c device, but that is just because these are so
> > common.
> >
> > In fact, it could be especially useful in cases where the i2c support
> > is sometimes in a bridge, sometimes in an i2c device (or more than
> > one). If the actual API is through a v4l2_subdev, then that will make
> > everything nicely consistent.
> >
> > Of course, the proof of the pudding is in the eating, so try it in a
> > few drivers. You generally discover very quickly whether something is a
> > good API or not.
>
> Ok, those are valid uses, but it is not the better solution for sharing
> device resources like IRQ and GPIO's. I'm all for using it in the cases
> that Andy pointed in scenarios (1), (2) and (3).
>
> > > IMHO, it should be providing a driver that checks for the creation of
> > > e at /dev/video? and, if a node is found, it should do the a
> > > procedure similar to what is done by v4l2-apps/util/v4l2-sysfs-path
> > > (the latest version at http://linuxtv.org/hg/v4l-dvb tree), to
> > > determine the associated event device. For example, with my HVR-950,
> > > it returns:
> > >
> > > device     = /dev/video0
> > > bus info   = usb-0000:00:1d.7-8
> > > sysfs path = /sys/devices/pci0000:00/0000:00:1d.7/usb1/1-8
> > > Associated devices:
> > >         i2c-adapter:i2c-4
> > >         input:input12:event6 (dev 13,70)
> > >         sound:pcmC1D0c (dev 116,9)
> > >         sound:dsp1 (dev 14,19)
> > >         sound:audio1 (dev 14,20)
> > >         sound:controlC1 (dev 116,10)
> > >         sound:mixer1 (dev 14,16)
> > >         dvb:dvb0.frontend0 (dev 212,0)
> > >         dvb:dvb0.demux0 (dev 212,1)
> > >         dvb:dvb0.dvr0 (dev 212,2)
> > >         dvb:dvb0.net0 (dev 212,3)
> > >         usb_endpoint:usbdev1.11_ep00 (dev 252,20)
> > >
> > > So, the event interface associated with /dev/video0 is
> > > /dev/input/event6 (minor 13, major 70), as reported above.
> >
> > Mauro, you should consider using libudev to map major/minor device
> > numbers to the actual device nodes. Right now no udev rules are taken
> > into account. So if someone renames a device node you will never find
> > it.
>
> Thank you for your suggestion.
>
> I've considered to use also libudev for it, but, while seeking the net
> for some coding example, this patch appeared:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2007-November/022043.htm
>l
>
> There, Jean proposed a patch to get rid of libudev since, on that time,
> libudev weren't maintained anymore, according to him. I haven't check if
> this is still true or not, but, as all I wanted with that code is to get
> the event name and the device, I took, instead, the approach that gave me
> the quickest solution. For sure using a well maintained library is much
> better than having to reinvent the wheel.

Kay Sievers is actively maintaining this library. In fact, features like 
resolving major/minor numbers is something that he added in the last year 
if I am not mistaken. I discussed this problem with him during the Embedded 
Linux Conference early this year and he says that using libudev is the 
right approach. Apparently other people needed the same functionality so he 
had added it to the library.

Actually Andy Walls experimented with it and he posted some example code for 
this not too long ago. It was pretty trivial.

See: http://osdir.com/ml/linux-media/2009-07/msg00098.html

> In a matter of fact, to be really useful, the better would be if
> v4l2-sysfs-path could return the path of the devices instead of just the
> major/minor.
>
> Please, feel free to propose some patches to improve it, if you have time
> for it.

Regards,

	Hans
  

Patch

diff -r 44282114d1e3 linux/include/media/v4l2-subdev.h
--- a/linux/include/media/v4l2-subdev.h	Fri Aug 21 13:26:01 2009 -0400
+++ b/linux/include/media/v4l2-subdev.h	Fri Aug 21 21:51:52 2009 -0400
@@ -229,11 +229,206 @@ 
 	int (*enum_frameintervals)(struct v4l2_subdev *sd, struct v4l2_frmivalenum *fival);
 };
 
+/*
+   interrupt_service_routine: Called by the bridge chip's interrupt service
+	handler, when an IR interrupt status has be raised due to this subdev,
+	so that this subdev can handle the details.  It may schedule work to be
+	performed later.  It must not sleep.  *Called from an IRQ context*.
+
+   g_features: Return IR features supported by this device.  Intended to be
+	used to support the LIRC_GET_FEATURES ioctl() and to return the same
+	flags.
+
+   g_code_length: Return the cooked IR code length.  Intended to be used to
+	support the LIRC_GET_LENGTH ioctl(), returning the length of a code
+	in bits.  Currently only used in lirc for the LIRC_MODE_LIRCCODE.
+
+   [rt]x_s_notify_callback: Allows the subdev caller to set a callback for
+	notification of events due to the IR recevier or transmitter.
+
+   rx_read_pulse_widths: Reads received data in the form of consective space and
+	mark pulse widths in microseconds, or nanoseconds if in_ns is true.  The
+	semantics are similar to a non-blocking read() call.
+
+   tx_write_pulse_widths: Reads received data in the form of consective mark and
+	space pulse widths in microseconds, or nanoseconds if in_ns is true.
+	The semantics are similar to a non-blocking write() call.
+
+   rx_read: Reads received codes or other non-pulse width data.
+	The semantics are similar to a non-blocking read() call.
+
+   tx_write: Writes codes or other non-pulse width data to transmit.
+	The semantics are similar to a non-blocking write() call.
+
+   [rt]x_enable: enable or disable the receiver or transmitter using or
+	preserving the current setting.
+
+   [rt]x_shutdown: disable the receiver or transmitter and adjust all setting
+	to shut off or slow down hardware and disable interrupts.
+
+   [rt]x_s_interrupt_enable: enable or diable the receiver or transmitter
+	interrupts.
+
+   rx_s_demodulation: enable demodulation of received pulses from a carrier or
+	disable demodulation and read "baseband" light pulses.
+
+   rx_g_demodulation: query if demodulation of received pulses from a carrier is
+	enabled.
+
+   tx_s_modulation: enable modulation of transmitted pulses onto a carrier or
+	disable modulation and transmit "baseband" light pulses.
+
+   tx_g_modulation: query if modulation of transmitted pulses onto a carrier is
+	enabled.
+
+   rx_s_noise_filter: set the threshold pulse width for a received pulse to be
+	considered valid and not a glitch or noise.  A value of 0 disables the
+	noise filter.
+
+   rx_g_noise_filter: query the threshold pulse width for a received pulse to be
+	considered valid and not a glitch or noise.  A value of 0 means the
+	noise filter is disabled.
+
+   [rt]x_s_max_pulse_width: sets the max valid pulse width expected to be
+	received or transmitted, when receiving or transmitting baseband pulses,
+	in order to optimize the pulse width timer's resolution.  This call will
+	likely have the side effect of disabling demodulation/modulation of
+	pulses from/onto a carrier.
+
+   [rt]x_g_max_pulse_width: gets the max valid pulse width expected to be
+	received or transmitted, when receiving or transmitting baseband pulses.
+	This call should return error if demodulation/modulation of
+	pulses from/onto a carrier is enabled.
+
+   [rt]x_[sg]_lirc_mode: Set or get the LIRC "mode" for the receiver or
+	transmitter.  Intended to support the LIRC_{SET,GET}_{REC,SEND}_MODE
+	ioctl() calls.
+
+   [rt]x_[sg]_carrier: Set or get the carrier frequency for the receiver or
+	transmitter.  Frequency is in Hz.  Hardware limitations may
+	mean the actual frequency set varies from the desired frequency.  The
+	_g_ calls should return the precise frequency set.  The _s_ calls will
+	have the side effect of enabling demodulation/modulation when the
+	freq is not 0, and disabling demodulation/modulation when the
+	freq is 0.  These calls can also be used to support the
+	LIRC_{SET,GET}_{REC,SEND}_CARRIER ioctl() calls.
+
+   rx_s_carrier_range: Set a window of expected carrier frequencies for the
+	receiver.  Intended to support the LIRC_SET_REC_CARRIER_RANGE ioctl().
+	Due to hardware limitations, the full range may not be supportable and
+	the center of the supportable range may not be at the exact center of
+	the desired range.
+
+   [rt]x_[sg]_duty_cycle: Set or get the carrier duty cycle for the receiver or
+	transmitter.  Hardware limitations may mean the actual duty cycle set
+	varies from the desired duty_cycle.  The _g_ calls should return the
+	precise duty cycle set.  These calls can also be used to
+	support the LIRC_{SET,GET}_{REC,SEND}_DUTY_CYCLE ioctl() calls.
+
+   rx_s_duty_cycle_range: Set a window of expected received carrier duty cycles
+	Intended to support the LIRC_SET_REC_DUTY_CYCLE_RANGE ioctl().
+	Due to hardware limitations, the full range may not be supportable and
+	the center of the supportable range may not be at the exact center of
+	the desired range.
+
+   rx_g_resolution: get the pulse measurment resolution of the receiver.
+	Intended to support the LIRC_GET_REC_RESOLUTION ioctl().
+
+   tx_s_mask: set the mask of enabled transmitters for devices that have
+	more than one trasnmitter.  Intended to support the
+	LIRC_SET_TRANSMITTER_MASK ioctl().
+ */
+enum v4l2_subdev_ir_event {
+	V4L2_SUBDEV_IR_RX_DATA_READY     = 0,
+	V4L2_SUBDEV_IR_TX_READY_FOR_DATA = 1,
+};
+
+typedef int (*v4l2_subdev_ir_notify_callback)(void *priv,
+					      struct v4l2_subdev *sd,
+					      enum v4l2_subdev_ir_event event);
+
+struct v4l2_subdev_ir_ops {
+	/* Common to receiver and transmitter */
+	int (*interrupt_service_routine)(struct v4l2_subdev *sd,
+						u32 status, bool *handled);
+
+	/* LIRC ioctl inspired calls */
+	int (*g_features)(struct v4l2_subdev *sd, u32 *features);
+	int (*g_code_length)(struct v4l2_subdev *sd, u32 *bits);
+
+	/* Receiver */
+	int (*rx_s_notify_callback)(struct v4l2_subdev *sd,
+					v4l2_subdev_ir_notify_callback callback,
+					void *priv);
+
+	int (*rx_read_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
+					size_t count, bool in_ns, ssize_t *num);
+	int (*rx_read)(struct v4l2_subdev *sd, u8 *buf, size_t count,
+				ssize_t *num);
+
+	int (*rx_enable)(struct v4l2_subdev *sd, bool enable);
+	int (*rx_shutdown)(struct v4l2_subdev *sd);
+
+	int (*rx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
+	int (*rx_s_demodulation)(struct v4l2_subdev *sd, bool enable);
+	int (*rx_s_noise_filter)(struct v4l2_subdev *sd, u32 min_width_ns);
+	int (*rx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
+
+	int (*rx_g_demodulation)(struct v4l2_subdev *sd, bool *enabled);
+	int (*rx_g_noise_filter)(struct v4l2_subdev *sd, u32 *min_width_ns);
+	int (*rx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
+
+	/* LIRC receiver ioctl inspired calls */
+	int (*rx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
+	int (*rx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
+	int (*rx_s_carrier_range)(struct v4l2_subdev *sd,
+					u32 lower, u32 upper);
+	int (*rx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
+	int (*rx_s_duty_cycle_range)(struct v4l2_subdev *sd,
+					u32 lower, u32 upper);
+
+	int (*rx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
+	int (*rx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
+	int (*rx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
+	int (*rx_g_resolution)(struct v4l2_subdev *sd, u32 *nsec);
+
+	/* Transmitter */
+	int (*tx_s_notify_callback)(struct v4l2_subdev *sd,
+					v4l2_subdev_ir_notify_callback callback,
+					void *priv);
+
+	int (*tx_write_pulse_widths)(struct v4l2_subdev *sd, u32 *widths,
+					size_t count, bool in_ns, ssize_t *num);
+	int (*tx_write)(struct v4l2_subdev *sd, u8 *buf, size_t count,
+				ssize_t *num);
+
+	int (*tx_enable)(struct v4l2_subdev *sd, bool enable);
+	int (*tx_shutdown)(struct v4l2_subdev *sd);
+
+	int (*tx_s_interrupt_enable)(struct v4l2_subdev *sd, bool enable);
+	int (*tx_s_modulation)(struct v4l2_subdev *sd, bool enable);
+	int (*tx_s_max_pulse_width)(struct v4l2_subdev *sd, u64 max_width_ns);
+
+	int (*tx_g_modulation)(struct v4l2_subdev *sd, bool *enabled);
+	int (*tx_g_max_pulse_width)(struct v4l2_subdev *sd, u64 *max_width_ns);
+
+	/* LIRC transmitter ioctl inspired calls */
+	int (*tx_s_lirc_mode)(struct v4l2_subdev *sd, u32 mode);
+	int (*tx_s_carrier)(struct v4l2_subdev *sd, u32 freq);
+	int (*tx_s_duty_cycle)(struct v4l2_subdev *sd, u32 duty_cycle);
+	int (*tx_s_mask)(struct v4l2_subdev *sd, u32 enabled_transmitters);
+
+	int (*tx_g_lirc_mode)(struct v4l2_subdev *sd, u32 *mode);
+	int (*tx_g_carrier)(struct v4l2_subdev *sd, u32 *freq);
+	int (*tx_g_duty_cycle)(struct v4l2_subdev *sd, u32 *duty_cycle);
+};
+
 struct v4l2_subdev_ops {
 	const struct v4l2_subdev_core_ops  *core;
 	const struct v4l2_subdev_tuner_ops *tuner;
 	const struct v4l2_subdev_audio_ops *audio;
 	const struct v4l2_subdev_video_ops *video;
+	const struct v4l2_subdev_ir_ops    *ir;
 };
 
 #define V4L2_SUBDEV_NAME_SIZE 32