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