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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 8/26/19 11:57 AM, Andy Shevchenko wrote:
> 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

Ah, right, missed that.

> invalid GPIO line number to -1 or so, but does it worth it?  And actually I
> would prefer -EINVAL or -ENOENT in such cases.

OK, we can keep your "= gpio" assignment in view of the above, but need
to return error instead of "n".

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

-- 
Best regards,
Jacek Anaszewski