Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

["David Härdeman" <david@hardeman.nu>]

On 2014-10-26 20:33, Tomas Melin wrote:
> On Sat, Oct 25, 2014 at 12:03 PM, David Härdeman <david@hardeman.nu> 
> wrote:
>> Wouldn't something like this be a simpler way of achieving the same
>> result? (untested):
> 
> The idea was to remove the empty change_protocol function that had
> been added in the breaking commit.

The empty function was added for a reason? The presence of a 
change_protocol function implies that the receiver supports protocol 
changing (whether it's via the raw IR decoding or in hardware).

> IMHO, it would be better to not have functions that don't do anything.
> Actually, another problem with that empty function is that if the
> driver first sets up a "real" change_protocol function and related
> data, and then calls rc_register_device, the driver defined
> change_protocol function would be overwritten.

change_protocol is only set if it's a driver that does in-kernel 
decoding...meaning that it generates pulse/space timings...meaning that 
hardware protocol changes aren't necessary?

>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index a7991c7..d521f20 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -1421,6 +1421,9 @@ int rc_register_device(struct rc_dev *dev)
>> 
>>         if (dev->change_protocol) {
>>                 u64 rc_type = (1 << rc_map->rc_type);
>> +               if (dev->driver_type == RC_DRIVER_IR_RAW)
>> +                       rc_type |= RC_BIT_LIRC;
>> +
>>                 rc = dev->change_protocol(dev, &rc_type);
>>                 if (rc < 0)
>>                         goto out_raw;
> 
> But otherwise yes, your suggestion could work, with the addition that
> we still need to update enabled_protocols (and not init
> enabled_protocols anymore in ir_raw_event_register() ).

First, enabled_protocols is already taken care of in the above patch 
(the line after "goto out_raw" is "dev->enabled_protocols = rc_type;")?

Second, initializing enabled_protocols to some default in 
ir_raw_event_register() might not be strictly necessary but it also 
doesn't hurt since that happens before dev->change_protocol() is called 
in rc_register_device()?

> +               dev->enabled_protocols = (rc_type | RC_BIT_LIRC);
> 
> Please let me know your preferences on which you prefer, and, if
> needed, I'll make a new patch version.

I'd prefer the above, minimal, approach. But it's Mauro who decides in 
the end.

Regards,
David