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

Hi Uwe,

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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |