Re: Hacking on ati_remote driver

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 27-09-2010 13:07, Jarod Wilson escreveu:
> On Mon, Sep 27, 2010 at 10:42:19AM -0400, George Spelvin wrote:
>>> IMO, ati_remote.c and ati_remote2.c should be adapted to use the ir-core
>>> (soon to be rc-core) interfaces. I've got remote2 hardware myself, so
>>> doing the conversion for that driver is already on my personal TODO list,
>>> but its a long queue of more important things ahead of it first...
>>
>> Just what I want, more scope creep... I'm not immovably opposed to a
>> larger conversion job, but can you tell me that the ir-core/rc-core
>> layer gives me over the raw input layer?
> 
> - Hopefully, a simplified interface, with less code duplication. Quite a
> few remote drivers reimplement the same things over and over. I've not
> actually looked at ati_remote.c to see exactly what its got, but
> ati_remote2.c has some low-level evbit, keybit, __set_bit, etc.,
> manipulations that would mostly disappear w/ir-core, in favor of using
> common shared code.
> 
> - Userspace remote-specific manipulation tools in v4l-utils
> 
> - Sysfs hooks that reveal its a remote in the first place -- which may be
>   of benefit to a userspace daemon, udev loading a new keymap
>   automatically, or to media center apps that want to look directly at a
>   remote control's input device until such time as X sucks less and will
>   pass through keycodes larger than 8-bit.
> 
>> I see how there are a bunch
>> of utilities for decoding raw pulse streams, but there's only one
>> RC_DRIVER_SCANCODE driver, imon.c, and even that seems to use the RC6
>> protocol sometimes.
> 
> imon is always scancode, but some of the devices can be configured to use
> one device or another. One of them is an RC6A MCE remote. But it still
> does decoding in hardware and passes along scancodes. There's tm6000 in
> staging that is RC_DRIVER_SCANCODE, and I swear more devices under
> drivers/media/ are scancode-only and simply haven't been ported fully over
> to ir-core just yet, but I could be wrong about that.

dib0700 is also scancode only, already ported to rc core. Also, partially
ported, we have saa7134 and em28xx drivers (only scancode).

>> Looking at the code leads to a few obvious questions:
>> ir-common.h:
>> 	Why is the linux keycode u32, when the input layer's
>> 	key codes are __u16?  And why is keypressed an int
>> 	rather than a bool?
>>
>> 	And why is the type __u64?  It appears top be a bitmap,
>> 	with about 6 values defined so far...
> 
> ir-common.h is going away RSN, this is from an older pre-{ir,rc}-core IR
> layer in the media tree. Its not used by imon, mceusb or streamzap,
> anyway.

The type is to handle the different IR protocols. As it is a bitmap, I
opted to define it as u64, as I was afraid that we might run out of space
with just 32 bits.

>> ir-sysfs.c:
>> 	Why is store_protocol, which appears to be turning an ASCII
>> 	string into an ir_type bitmap, documented as "triggered by
>> 	READING /sys/class/rc/rc?/current_protocol"?  Why doesn't that
>> 	code support the rc6 protocol?
> 
> Um... what? I see:
> 
>  * This routine is a callback routine for changing the IR protocol type.
>  * It is trigged by writing to /sys/class/rc/rc?/protocols.
>   
> And it definitely supports rc6.

George, you're probably looking at an older implementation.

>> In general, I'm bit confused about what it means to change_protocol
>> to a bitmap with multiple bits set.  Are you sure ir_type needs to be
>> a bitmap?  The only place I can see its bitmapness actually used is in
>> show_supported_protocols(), and that could be replaced by an array of a
>> few chars or something.  (Most devices support only one or two protocols.)
>> Reserving 0 for "end of list" would make the structure initialization
>> simpler.
> 
> May or may not need to be a bitmap, I didn't write that part. Mauro may be
> able to shed some light here. Generally, you'll only change_protocol to a
> single proto (that's the case w/imon), but there could be receivers where
> more than one can be enabled simultaneously.

The same bitmap is also used by raw decoders. So, you may enable just a few
protocols. There are some devices with scancodes that are able to handle
more than one protocol at the same time. So, a bitmap is more flexible than
a list of values.
> 
>> One thing that would be nice is to have the permissions on the sysfs
>> protocol file depend on the existence of a change_protocol method.
>>
>> Um.. I also notice that change_protocol isn't even supported on
>> non-RC_DRIVER_SCANCODE drivers.  Is this all a big kludge invented for
>> the imon driver?
> 
> No. It existed before the imon driver. git grep for change_protocol, and
> you'll see its used in a number of places besides imon.c.

This were unified at the newer implementations. From userspace POV, both
raw and non-raw IR's now have the same way to enable/disable protocols.

>> ir-functions.c: Is ir->last_bit actually used for anything?  I can't
>> find an assignement to a non-zero value anywhere...
>> Oh, wait, drivers/media/video/cx23885/cx23885-input.c and
>> drivers/media/video/saa7134/saa7134-input.c
>> Does that need to be in the generic structure?
> 
> Not sure, haven't ever touched it myself.
> 
ir-functions will die as soon as we move all drivers to use rc-core.

Cheers,
Mauro.