Hi Uwe,

On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-K?nig
[off-list ref] wrote:

On Thu, Mar 23, 2017 at 01:03:56PM +0100, Geert Uytterhoeven wrote:

quoted

On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-K?nig
[off-list ref] wrote:

quoted

quoted

Make sure to enable all drivers and subsystems you need when building
your kernel. That's always true. And may indeed be hard to debug (e.g. what
kernel options do I need to make systemd work?).

It's worse here. If you forget to enable a driver the device isn't bound
and that's obvious to diagnose. When ignoring an optional GPIO there
might be a device that claims to work but fails to do so. (e.g. you
write to memory, write() returns 0, but the data never landed there.)

quoted

quoted

write(2) and close(2) succeed most of the time, too. Still it's not a
good idea to not check the return value. Or let the kernel return
success unconditionally.

Writing all bytes passed in the buffer is "optional" in another sense than
an "optional" GPIO: you must retry the write, while you can continue if
an optional GPIO is not present.

And that is the point. You can continue *iff* the optional GPIO is not
present. The patch in question removes the ability to determine if that
GPIO is present and claims it is not present.

If you forget to enable a driver/subsystem, you sometimes cannot determine
if the device is present or not neither.

Hence it boils down to "knowing" if there is a GPIO or not.
So, when can there be a GPIO?
  1. The GPIO is described in DT.
     => Not an issue, as DT GPIO implies GPIOLIB,
  2. The GPIO is described in legacy platform data.
     => The platform code should make sure GPIOLIB is selected when needed.

Issue solved?

I like it better to not rely on platform code to do the right thing.

;-)

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?

Yes, that should do the trick.

No feedback from me about ACPI.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds