Re: [PATCH v1 1/2] leds: trigger: gpio: GPIO 0 is valid

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@linux.intel.com>

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