On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote:
> On 8/21/19 7:17 PM, Andy Shevchenko wrote:
> > Allow all valid GPIOs to be used in the driver.
> >
> > Fixes: 17354bfe8527 ("leds: Add gpio-led trigger")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > - if (!gpio) {
> > - if (gpio_data->gpio != 0)
> > + if (!gpio_is_valid(gpio)) {
> > + if (gpio_is_valid(gpio_data->gpio))
> > free_irq(gpio_to_irq(gpio_data->gpio), led);
> > - gpio_data->gpio = 0;
> > + gpio_data->gpio = gpio;
>
> It looks odd to me. I'd just assign invalid constant gpio number
> e.g. -1.
Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine
invalid GPIO line number to -1 or so, but does it worth it? And actually I
would prefer -EINVAL or -ENOENT in such cases.
> Note that we should also do that in gpio_trig_activate(), where
> gpio_data->gpio is initialized to 0 by kzalloc(). That later can
> have nasty side effect in gpio_trig_gpio_store() when gpio to set
> is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate
> to true and gpio_trig_irq() handler will not be registered.
Thanks for spotting this!
--
With Best Regards,
Andy Shevchenko
