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