On Sat, Jun 5, 2010 at 1:24 PM, Andy Walls <awalls@xxxxxxxxxxxxxxxx> wrote:
> On Fri, 2010-06-04 at 14:57 -0400, Jon Smirl wrote:
>> On Fri, Jun 4, 2010 at 2:38 PM, Mauro Carvalho Chehab
>> <mchehab@xxxxxxxxxx> wrote:
>> > Em 04-06-2010 12:51, Christoph Bartelmus escreveu:
>> >> Hi Mauro,
>> >>
>> >> on 04 Jun 10 at 01:10, Mauro Carvalho Chehab wrote:
>> >>> Em 03-06-2010 19:06, Jarod Wilson escreveu:
>> >> [...]
>> >>>> As for the compat bits... I actually pulled them out of the Fedora kernel
>> >>>> and userspace for a while, and there were only a few people who really ran
>> >>>> into issues with it, but I think if the new userspace and kernel are rolled
>> >>>> out at the same time in a new distro release (i.e., Fedora 14, in our
>> >>>> particular case), it should be mostly transparent to users.
>> >>
>> >>> For sure this will happen on all distros that follows upstream: they'll
>> >>> update lirc to fulfill the minimal requirement at Documentation/Changes.
>> >>>
>> >>> The issue will appear only to people that manually compile kernel and lirc.
>> >>> Those users are likely smart enough to upgrade to a newer lirc version if
>> >>> they notice a trouble, and to check at the forums.
>> >>
>> >>>> Christoph
>> >>>> wasn't a fan of the change, and actually asked me to revert it, so I'm
>> >>>> cc'ing him here for further feedback, but I'm inclined to say that if this
>> >>>> is the price we pay to get upstream, so be it.
>> >>
>> >>> I understand Christoph view, but I think that having to deal with compat
>> >>> stuff forever is a high price to pay, as the impact of this change is
>> >>> transitory and shouldn't be hard to deal with.
>> >>
>> >> I'm not against doing this change, but it has to be coordinated between
>> >> drivers and user-space.
>> >> Just changing lirc.h is not enough. You also have to change all user-space
>> >> applications that use the affected ioctls to use the correct types.
>> >> That's what Jarod did not address last time so I asked him to revert the
>> >> change.
>> >
>> > For sure coordination between kernel and userspace is very important. I'm sure
>> > that Jarod can help with this sync. Also, after having the changes implemented
>> > on userspace, I expect one patch from you adding the minimal lirc requirement
>> > at Documentation/Changes.
>>
>> Keep the get_version() ioctl stable. The first thing user space would
>> do it call the get_version() ioctl. If user space doesn't support the
>> kernel version error out and print a message with the url of the
>> project web site and tell the user to upgrade.
>>
>> If you have the ability to upgrade your kernel you should also have
>> the ability to upgrade user space too. I'm no fan of keeping backwards
>> compatibility code around. It just becomes piles of clutter that
>> nobody maintains. A reliable mechanism to determine version mismatch
>> is all that is needed.
>>
>> PS - I really don't like having to fix bug reports in compatibility
>> code. That is an incredible waste of support time that can be easily
>> fixed by upgrading the user.
>>
>> >
>> >> And I'd also like to collect all other change request to the API
>> >> if there are any and do all changes in one go.
>> >
>> > You and Jarod are the most indicated people to point for such needs. Also, Jon
>> > and David may have some comments.
>> >
>> > From my side, as I said before, I'd like to see a documentation of the defined API bits,
>> > and the removal of the currently unused ioctls (they can be added later, together
>> > with the patches that will introduce the code that handles them) to give my final ack.
>> >
>> > From what I'm seeing, those are the current used ioctls:
>> >
>> > +#define LIRC_GET_FEATURES _IOR('i', 0x00000000, unsigned long)
>> >
>> > +#define LIRC_GET_LENGTH _IOR('i', 0x0000000f, unsigned long)
>>
>> Has this been set into stone yet? if not a 64b word would be more future proof.
>> Checking the existence of features might be better as sysfs
>> attributes. You can have as many sysfs attributes as you want.
>>
>> > +#define LIRC_GET_MIN_TIMEOUT _IOR('i', 0x00000008, uint32_t)
>> > +#define LIRC_GET_MAX_TIMEOUT _IOR('i', 0x00000009, uint32_t)
>> >
>> > There is also a defined LIRC_GET_REC_MODE that just returns a mask of GET_FEATURES
>> > bits, and a LIRC_SET_REC_MODE that do nothing.
>> >
>> > I can't comment about the other ioctls, as currently there's no code using them, but
>> > it seems that some of them would be better implemented as ioctl, like the ones that
>> > send carrier/burst, etc.
>
> So I'll comment on the LIRC ioctl()'s that cx23885 driver/cx23888-ir.c
> implementation will support. These ioctl()'s are from the
> lirc-0.8.7pre1 CVS snapshot I just downloaded.
>
>
> LIRC_GET_FEATURES
> The driver will support this.
>
> LIRC_GET_SEND_MODE
> LIRC_GET_REC_MODE
> The driver will support these. It will return hard-coded values, as the
> hardware and cx23888-ir.c only deal with raw pulse time values because
> there is no hardware decoding.
>
> LIRC_SET_SEND_MODE
> LIRC_SET_REC_MODE
> The driver won't support these. The CX23888 only supports 1 mode for Tx
> and 1 mode for Rx (namely raw pulses), but these ioctl()'s are marked
> obsolete anyway.
>
> LIRC_GET_SEND_CARRIER
> LIRC_GET_REC_CARRIER
> The driver will support these. It will return that actual value the
> hardware is working with. For the CX23888, that may not be *exactly*
> what the user set, but instead the closest value the hardware could be
> set to, given what the user requested. (e.g. 35,904 Hz when 36,000 Hz
> was requested).
>
> LIRC_SET_SEND_CARRIER
> The driver will support this. Setting it is only valid when transmit
> modulation is enabled. It will be set in hardware to a value as close
> as possible to the user's desired setting, but it is not guaranteed to
> be exact (e.g. 35,904 Hz when 36,000 Hz was requested)
> If the driver is set for basedband transmit, then it is ignored.
>
>
> LIRC_SET_REC_CARRIER
> LIRC_SET_REC_CARRIER_RANGE
> The driver will support these. Setting them is only valid when receive
> demodulation is enabled. They will be set in hardware to values as
> close as possible to the user's desired settings, but not guaranteed to
> be exact. If setting REC_CARRIER, a default CARRIER_RANGE window will
> be set in the hardware. If setting both, they really should be set
> atomically, as the possible window settings in hardware really depend on
> the desired receive carrier center freq.
> If receive demodulation is disabled, the values are ignored.
>
>
>
> LIRC_GET_SEND_DUTY_CYCLE
> LIRC_SET_SEND_DUTY_CYCLE
> The driver will support these. The value returned for GET will be what
> the hardware is set to, given what the user requested. That value may
> not be exactly what the user asked for. The CX23888 hardware supports
> settings of n/16 * 100% for n in [1,16]. Note, that setting a value of
> 100% is *not* the same as setting baseband transmit modulation as far as
> the hardware is concerned (although the physical effect may be the
> same).
>
>
> LIRC_GET_REC_DUTY_CYCLE
> LIRC_SET_REC_DUTY_CYCLE
> LIRC_SET_REC_DUTY_CYCLE_RANGE
> The driver won't support these. There is no hardware support.
>
>
> LIRC_GET_REC_RESOLUTION
> The driver will support this. For CX23888 hardware, the value returned
> is not a constant, as the current pulse measurment resolution depends on
> how the hardware is set up.
> The driver could also support this for Tx as well, but there is no lirc
> ioctl().
>
>
> LIRC_GET_MIN_TIMEOUT
> LIRC_GET_MAX_TIMEOUT
> The driver will support this. With Rx demodulation enabled, the value
> returned will be the same for both MIN and MAX, and it is a function of
> the selected carrier frequency. With Rx demodulation disabled (baseband
> pulse reception), two distinct values can be returned: MIN_TIMEOUT would
> be the one that corresponds to settings for maximum measurement
> resolution of pulses, MAX_TIMEOUT corresponds to to settings for minimum
> measurment resolution of pulses.
>
>
> LIRC_SET_REC_TIMEOUT
> The driver will support this. The current cx23888-ir.c code implements
> it as setting the maximum measurable pulse width when RX demodulation is
> disabled (baseband pulses being received.). Note that this setting
> affects the Rx measurment resolution. When Rx demodulation is enabled,
> the receive tiomeout is determined by the Rx carrier setting and this
> setting will be ignored.
> Currently, since the cx23888-ir.c implementation is interrupt driven,
> the driver does not have code to support a setting of 0. To me a
> setting of 0 implies polling the Rx hardware FIFO.
>
>
> (I'll note the baseband Rx reception is not moot for some TV cards.
> Some cards with an CX23888, like the HVR-1850, only provide baseband Rx
> pulses to the CX23888 receive circuitry. Apparently such cards have
> external components integrating the received carrier pulses into
> baseband pulses. For these cards, the CX23888 only sees baseband pulses
> on receive and Rx demodulation must remain disabled in normal
> operation.)
>
>
> LIRC_SET_REC_TIMEOUT_REPORTS
> The driver will support this. When the width measurment timer expires,
> a special measurement value is put on the Rx FIFO indicating a timeout
> (in band signaling). The driver uses this internally and can pass the
> value along if requested.
>
>
> LIRC_GET_MIN_FILTER_PULSE
> LIRC_GET_MAX_FILTER_PULSE
> The driver will support these. The CX23888 has a hardware glith filter
> for pulses. It also has a hardware setting for inverting polarity of
> the receive signal before the glitch filter, so the cx2888-ir.c will be
> able to ensure the glitch filer is always applied to pulses and never
> spaces. The values returned for this are not constant but depend on the
> carrier frequency if Rx demodulation is enabled, or the pulse measurment
> resolution if Rx demodulation is disabled.
>
>
> LIRC_GET_MIN_FILTER_SPACE
> LIRC_GET_MAX_FILTER_SPACE
> The driver for the CX23888 will not support these. Older Conexant IR
> core implementations don't have hardware to invert incoming Rx signals,
> so it could be the case that the hardware glitch filter is applied to
> spaces instead of pulses depending on how the TV card is wired up.
> Polarity inversion is done in software after the glitch filter for those
> older chips. There are no IR drivers currently in kernel for these
> older chips
>
>
> LIRC_SET_REC_FILTER_PULSE
> LIRC_SET_REC_FILTER
> The driver will support these. They will affect the pulse glitch
> filter.
>
> LIRC_SET_REC_FILTER_SPACE
> The driver will not support this.
>
> LIRC_GET_LENGTH
> The driver will not support this.
>
> LIRC_SET_TRANSMITTER_MASK
> The driver will not support this. The CX23888 can support up to two
> output LEDs, but I doubt I'll ever see a video card wired up to do such.
> Also the current cx23888-ir.c doesn't have code to support it.
>
> LIRC_SET_MEASURE_CARRIER_MODE
> The driver will not support this.
>
> LIRC_NOTIFY_DECODE
> I'm not sure what this one is for. If it means send up a notification
> when the hardware pulse measurment times out (so start decoding), then
> the driver can support it.
>
>
> LIRC_SETUP_START
> LIRC_SETUP_END
> The driver will support these (I beleive I asked for them). To change
> anything with the transmitter or receiver on the CX23888, the
> transmitter or receiver (respectively) needs to be disabled, have its
> interrupts masked, the setting made, the interrupts unmasked, and the
> unit reenabled. These ioctl()'s would allow setting parameters in a
> batch, which is more efficient and probably smoother than operating with
> patial settings where one could possibly get interrupts unexpectedly.
LIRC_TRANSACTION_BEGIN
LIRC_TRANSACTION_END
Would be the generic form of this.
>
>
> What I don't see in the LIRC ioctl() set are:
>
> 1. ioctl()'s to enable/disable Tx modulation onto a carrier and Rx
> demodulation from a carrier.
>
> 2. an ioctl() to enable hardware loopback or other test modes.
>
> I'm not sure if they would be useful beyond testing anyway.
>
>
>> If the ioctls haven't been implemented, leave them as comments in the
>> h file. Don't implement them with stubs. Stubs will set the parameters
>> into stone and the parameters may be wrong.
>>
>> > One discussion that may be pertinent is if we should add ioctls for those controls,
>> > or if it would be better to just add sysfs nodes for them.
>> >
>> > As all those ioctls are related to config stuff, IMO, using sysfs would be better, but
>> > I haven't a closed opinion about that matter.
>
>
> I personally don't like sysfs:
>
> Cons of sysfs:
> - it's linux specific (Linux's own version of vendor lock-in: Where's
> the standards document?)
> - the hierarchy can easily get too deep
> - difficult to set multiple things atomically
> - Oops'es from races with sysfs nodes that appears and disappear and
> user space has managed to open it before it disappears. (firmware
> loading process is an example)
>
> Pros of sysfs
> - long term scalability for linux kernel configuration
> - manipulated with human readable, and typeable, strings.
>
> Maybe it's just that I've been playing with Unix systems since 1989 and
> am very used to ioctl().
>
>
>
> However, in my experience, the best interfaces are the ones with the
> most complete documentation. :)
>
>
>> In general sysfs is used to set options that are static or only change
>> infrequently.
>>
>> If the parameter is set on every request, use an IOCTL.
>
> That makes sense to me.
>
> Regards,
> Andy
>
>
>
>
--
Jon Smirl
jonsmirl@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
