Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls

[Linus Walleij]

On Thu, Mar 23, 2017 at 8:10 PM, Uwe Kleine-KÃnig
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote:
>> On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
>> > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
>> > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-KÃnig
>> > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>> > >
>> > > > Maybe we can make gpiod_get_optional look like this:
>> > > >
>> > > >         if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
>> > > >                 return NULL;
>> > > >         else
>> > > >                 return -ENOSYS;
>> > > >
>> > > > I don't know how isnt_a_acpi_device looks like, probably it involves
>> > > > CONFIG_ACPI and/or dev->acpi_node.
>> > > >
>> > > > This should be safe and still comfortable for legacy platforms, isn't it?
>> > >
>> > > I like the looks of this.
>> > >
>> > > Can we revert Dmitry's patch and apply something like this instead?
>> > >
>> > > Dmitry, how do you feel about this?
>> >
>> > I frankly do not see the point. It still makes driver code more complex
>
> Note that this code is in the gpio header, and not in driver code. So
> the driver just does
>
>         gpiod = gpiod_get_optional(...)
>         if (IS_ERR(gpiod))
>                 return PTR_ERR(gpiod);
>
> (as it is supposed to do now). I think that's nice.

It does look nice. Compare this to what we must do for optional regulators:

        st->reg = devm_regulator_get_optional(&spi->dev, "vref");
        if (IS_ERR(st->reg)) {
                /* Any other error indicates that the regulator does exist */
                if (PTR_ERR(st->reg) != -ENODEV)
                        return PTR_ERR(st->reg);
                /* Use internal reference */
        }

So for optional regulators we get -ENODEV if we don't have it,
and then proceed on an alternate path, such as using an internal
reference voltage.

However the fact that regulators and GPIO optionals are already
handled differently is creating "cognitive dissonance" or what I
should call it.

Yours,
Linus Walleij