Re: [PATCH] af9035: add remote control support

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

Em 18-04-2012 17:42, Hans-Frieder Vogt escreveu:
> Am Mittwoch, 18. April 2012 schrieb Mauro Carvalho Chehab:
>> Em 18-04-2012 11:57, Antti Palosaari escreveu:
>>> I haven't tried to and not commented it. But I see clearly few problems.
>>>
>>> On 18.04.2012 17:17, Mauro Carvalho Chehab wrote:
>>>> Em 07-04-2012 14:24, Hans-Frieder Vogt escreveu:
>>>>> af9035: support remote controls. Currently, for remotes using the NEC
>>>>> protocol, the map of the TERRATEC_CINERGY_XS remote is loaded, for RC6
>>>>> the map of RC_MAP_RC6_MCE.
>>>>>
>>>>> Signed-off-by: Hans-Frieder Vogt<hfvogt@gmx.net>
>>>>>
>>>>>   drivers/media/dvb/dvb-usb/af9035.c |   72
>>>>>   +++++++++++++++++++++++++++++++++++++
>>>>>   drivers/media/dvb/dvb-usb/af9035.h |    3 +
>>>>>   2 files changed, 75 insertions(+)
>>>>>
>>>>> diff -Nupr a/drivers/media/dvb/dvb-usb/af9035.c
>>>>> b/drivers/media/dvb/dvb-usb/af9035.c ---
>>>>> a/drivers/media/dvb/dvb-usb/af9035.c    2012-04-07 15:59:56.000000000
>>>>> +0200 +++ b/drivers/media/dvb/dvb-usb/af9035.c    2012-04-07
>>>>> 19:17:55.044874329 +0200 @@ -313,6 +313,41 @@ static struct
>>>>> i2c_algorithm af9035_i2c_a
>>>>>
>>>>>       .functionality = af9035_i2c_functionality,
>>>>>   
>>>>>   };
>>>>>
>>>>> +#define AF9035_POLL 250
>>>>> +static int af9035_rc_query(struct dvb_usb_device *d)
>>>>> +{
>>>>> +    unsigned int key;
>>>>> +    unsigned char b[4];
>>>>> +    int ret;
>>>>> +    struct usb_req req = { CMD_IR_GET, 0, 0, NULL, 4, b };
>>>>> +
>>>>> +    if (!af9035_config.raw_ir)
>>>>> +        return 0;
>>>>> +
>>>>> +    ret = af9035_ctrl_msg(d->udev,&req);
>>>>> +    if (ret<  0)
>>>>> +        goto err;
>>>>> +
>>>>> +    if ((b[2] + b[3]) == 0xff) {
>>>>> +        if ((b[0] + b[1]) == 0xff) {
>>>>> +            /* NEC */
>>>>> +            key = b[0]<<  8 | b[2];
>>>>> +        } else {
>>>>> +            /* ext. NEC */
>>>>> +            key = b[0]<<  16 | b[1]<<  8 | b[2];
>>>>> +        }
>>>>> +    } else {
>>>>> +        key = b[0]<<  24 | b[1]<<  16 | b[2]<<  8 | b[3];
>>>>> +    }
>>>>> +
>>>>> +    if (d->rc_dev != NULL)
>>>>> +        rc_keydown(d->rc_dev, key, 0);
>>>
>>> Is that checking needed and why? If there is no rc_device why we even
>>> call poll for it? Better to fix some core routines if that is true.
>>>
>>> Also rc_keydown() takes 2nd param as int, but in that case it does not
>>> matter. Anyhow, 3rd param is toggle which is used by RC5/6. IIRC I have
>>> never implemented RC5 or RC6 remote receiver, so I am not sure if it is
>>> needed and in which case.
>>
>> It is better to implement the toggle, when it is available/known, as the
>> core will use it to detect when the same key was pressed quickly twice, or
>> if someone just kept it pressed by a long time.
>>
>> When this is not implemented and someone presses the same key quickly twice
>> (a "double click"), the second click will be ignored, if the time is lower
>> than REP_DELAY (by default, 500 ms).
> 
> The IR_GET command only delivers 4 bytes, which give no indication of a 
> repeated key.
> 
>>
>> Not all protocols/decoders can detect it though. NEC protocol can't.
>> RC-5/RC-6 can do it. Yet, not all hardware reports the toggle big on RC-5.
>>
>>>>> +
>>>>> +err:
>>>>> +    /* ignore errors */
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>
>>>>>   static int af9035_init(struct dvb_usb_device *d)
>>>>>   {
>>>>>   
>>>>>       int ret, i;
>>>>>
>>>>> @@ -627,6 +662,34 @@ static int af9035_read_mac_address(struc
>>>>>
>>>>>       for (i = 0; i<  af9035_properties[0].num_adapters; i++)
>>>>>       
>>>>>           af9035_af9033_config[i].clock = clock_lut[tmp];
>>>>>
>>>>> +    ret = af9035_rd_reg(d, EEPROM_IR_MODE,&tmp);
>>>>> +    if (ret<  0)
>>>>> +        goto err;
>>>>> +    pr_debug("%s: ir_mode=%02x\n", __func__, tmp);
>>>>> +    af9035_config.raw_ir = tmp == 5;
>>>
>>> This looks odd for my eyes. Maybe x = (y == z); is better. Checkpatch
>>> didn't complain it?
>>
>> I think checkpatch will accept that. I generally prefer to use:
>>
>> 	foo = (tmp == 5) = true : false;
> 
> shouldn't it rather be
>         foo = (tmp == 5) ? true : false;

Yes.
>>
>> as some source code analyzers complain about statements like the above.
>>
>>>>> +
>>>>> +    if (af9035_config.raw_ir) {
>>>>> +        ret = af9035_rd_reg(d, EEPROM_IR_TYPE,&tmp);
>>>
>>> No space between x,y, IIRC checkpatch reports that.
> 
> the only errors that checkpatch is reporting is ERROR: trailing whitespace, 
> but that seems to be normal for lines in the patch that are unchanged (I run 
> checkpatch.pl --no-tree --file ...patch).

Use checkpatch for your patch, not for the file.

Unfortunately, when there are function parameters are all upercase, checkpatch.pl
doesn't work well. Once I complained, it was said this is a feature, in order
to avoid some false-positives.

> 
>>>
>>>>> +        if (ret<  0)
>>>>> +            goto err;
>>>>> +        pr_debug("%s: ir_type=%02x\n", __func__, tmp);
>>>>> +
>>>>> +        switch (tmp) {
>>>>> +        case 0: /* NEC */
>>>>> +        default:
>>>>> +            af9035_config.ir_rc6 = false;
>>>
>>> unused variable
> 
> agreed.
> 
>>>
>>>>> +            d->props.rc.core.protocol = RC_TYPE_NEC;
>>>>> +            d->props.rc.core.rc_codes =
>>>>> +                RC_MAP_NEC_TERRATEC_CINERGY_XS;
>>>>> +            break;
>>>>> +        case 1: /* RC6 */
>>>>> +            af9035_config.ir_rc6 = true;
>>>>> +            d->props.rc.core.protocol = RC_TYPE_RC6;
>>>>> +            d->props.rc.core.rc_codes = RC_MAP_RC6_MCE;
>>>>> +            break;
>>>>> +        }
>>>
>>> I hate to default some random remote controller keytable. Use EMPTY map,
>>> it is for that.
>>>
> Good idea.
> 
>>>>> +    }
>>>>> +
>>>>>
>>>>>       return 0;
>>>>>   
>>>>>   err:
>>>>> @@ -1003,6 +1066,15 @@ static struct dvb_usb_device_properties
>>>>>
>>>>>           .i2c_algo =&af9035_i2c_algo,
>>>>>
>>>>> +        .rc.core = {
>>>>> +            .protocol       = RC_TYPE_NEC,
>>>>> +            .module_name    = "af9035",
>>>>> +            .rc_query       = af9035_rc_query,
>>>>> +            .rc_interval    = AF9035_POLL,
>>>>> +            .allowed_protos = RC_TYPE_NEC | RC_TYPE_RC6,
>>>
>>> Does this mean we promise userspace we can do both NEC and RC6? Does it
>>> mean we should offer method to change protocol in that case? I suspect
>>> it is not even possible to switch from remote protocol to other unless
>>> eeprom change or firmware hack.
>>
>> Yes, that assumes a callback to allow to switch the protocol, OR that the
>> device can automatically recognize both protocols (there are a few that
>> are able to handle both NEC and RC-5 or RC-6 without any specific command).
>> The RC and NEC timings are very different, so, auto-detecting it is quite
>> easy.
>>
>> If this is the case for af9035, all that it is needed test the protocol
>> auto-detection is to replace the table from one protocol to the other and
>> use an IR compatible with the new table.
> 
> I think the af9035 doesn't autodetect the protocol. I tested a device which is 
> configured for RC6 (as layed down in the eeprom) and it doesn't read any raw 
> code from a NEC rc. 

Then, you need to make allowed_protocols equal to protocol, for the userspace
tools to work properly with it.

>> In the way this code was written, it leaves the reviewer without any af9035
>> device to believe that auto-detection is supported by af9035 (and also
>> because there's no command sent to the device in order to switch the mode).
>>
>> It is easy to check if the device accepts both automatically: just load
>> a different table with ir-keycode and test the remote with a different
>> protocol.
>>
>> If this is not the case, then rc.core.allowed_protocols should be equal to
>> rc.core.protocol.
> 
> Thanks.
> 
>>
>>>>> +            .rc_codes       = RC_MAP_EMPTY, /* may be changed in
>>>>> +                           af9035_read_mac_address */
>>>
>>> Commented that earlier. But RC_MAP_EMPTY is correct choice for default.
>>>
>>>> This is just a minor thing, but the comment here seems to be outdated,
>>>> as this is actually set at af9035_init().
>>>>
>>>>> +        },
>>>>>
>>>>>           .num_device_descs = 5,
>>>>>           .devices = {
>>>>>           
>>>>>               {
>>>>>
>>>>> diff -Nupr a/drivers/media/dvb/dvb-usb/af9035.h
>>>>> b/drivers/media/dvb/dvb-usb/af9035.h ---
>>>>> a/drivers/media/dvb/dvb-usb/af9035.h    2012-04-07 15:58:43.000000000
>>>>> +0200 +++ b/drivers/media/dvb/dvb-usb/af9035.h    2012-04-07
>>>>> 17:35:08.517840044 +0200 @@ -49,6 +49,8 @@ struct usb_req {
>>>>>
>>>>>   struct config {
>>>>>   
>>>>>       bool dual_mode;
>>>>>
>>>>> +    bool raw_ir;
>>>>> +    bool ir_rc6;
>>>
>>> Both of these new configs are unused and not needed. Please do not add
>>> new configuration option unless needed (to pass config data from
>>> function to other inside driver).
> 
> raw_ir is indeed used (see af9035_rc_query). However I agree that I could 
> implement this switch in a different way without the need of an extra config 
> variable.

If you choose to keep it, please don't call it as "raw_ir". Raw IR's are the
ones that sends a sequence of pulse-space events to the Kernel, for them to
be handled by a software decoder. What you're adding is, instead, a hardware
decoded IR device.
> 
>>>
>>>>>       bool hw_not_supported;
>>>>>   
>>>>>   };
>>>>>
>>>>> @@ -96,6 +98,7 @@ u32 clock_lut_it9135[] = {
>>>>>
>>>>>   #define CMD_MEM_WR                  0x01
>>>>>   #define CMD_I2C_RD                  0x02
>>>>>   #define CMD_I2C_WR                  0x03
>>>>>
>>>>> +#define CMD_IR_GET                  0x18
>>>>>
>>>>>   #define CMD_FW_DL                   0x21
>>>>>   #define CMD_FW_QUERYINFO            0x22
>>>>>   #define CMD_FW_BOOT                 0x23
>>>>>
>>>>> Hans-Frieder Vogt                       e-mail: hfvogt<at>  gmx .dot.
>>>>> net
>>>>
>>>> Except for that minor mistake at the comment above, the rest looks fine
>>>> on my eyes.
>>>
>>> I added some comments. And there was some basic remote controller issues
>>> - I didn't checked those, but those were commented as what is my
>>> understanding and some may be even wrong. In all cases please fix
>>> properly or explain I was wrong.
>>>
>>> regards
>>> Antti
> 
> Thanks, Mauro and Antti, for your comments. Expect an improved patch soon.
> 
> Cheers,
> Hans-Frieder
> 
> Hans-Frieder Vogt                       e-mail: hfvogt <at> gmx .dot. net