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

[David HÃrdeman]

On 2014-10-26 20:33, Tomas Melin wrote:
> On Sat, Oct 25, 2014 at 12:03 PM, David HÃrdeman <david@xxxxxxxxxxx> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/