[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] af9035: add remote control support



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@xxxxxxx>
> >>> 
> >>>   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;
> 
> 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).

> > 
> >>> +        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. 

> 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.

> > 
> >>>       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
--
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


[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux