Re: [PATCH] pinctrl: baytrail: show output gpio state correctly on Intel Baytrail

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Tue, Nov 04, 2014 at 01:51:53PM -0800, David Cohen wrote:
> On Tue, Nov 04, 2014 at 09:34:24PM +0200, Mika Westerberg wrote:
> > On Tue, Nov 04, 2014 at 11:11:16AM -0800, David Cohen wrote:
> > > > It is not implicit at all.
> > > > 
> > > > The user of the GPIO in ACPI DSDT table says something like:
> > > > 
> > > > 	Name (_DEP, Package () { \_SB.GPO2 })
> > > > 
> > > > or similar. That is *explicit* dependency. Here \_SB.GPO2 is one of the
> > > > GPIO banks.
> > > 
> > > Either kernel knows on-the-fly or statically the required dependency.
> > > The static dependency is well described by Kconfig. An on-the-fly
> > > dependency could be a probe execution failing because it couldn't access
> > > part of required resources. If the dependency is temporarily not
> > > described this way, it would still be acceptable a documentation
> > > somewhere explaining that we do have this hidden thing going on.
> > 
> > The only thing kernel knows about this is when it finds that the
> > device in question has _DEP object. Once that happens and it evaluates
> 
> That's all kernel needs to know.
> 
> > to a list of devices we depend on, we can defer this particular driver
> > going further in probe until all the dependencies listed in _DEP are
> > resolved.
> 
> That's the best way to prevent this issue IMHO, but looks like it's
> already being addressed:
> https://lkml.org/lkml/2014/10/27/455

Yes, that's the patch series.

> > 
> > The documentation you are after is ACPI 5.1 specification downloadable
> > freely at uefi.org/acpi.
> 
> Nope. The documentation I am referring to is the one that doesn't exist.
> It supposed to be a simple comment on pinctrl_baytrail.c's file, or
> maybe a comment on Kconfig. Wherever it could tell ppl not involved with
> original development that there might be an implicit dependency to this
> gpio driver by other drivers. And because of that, it's recommended to
> let this driver probed as soon as possible on Bay Trail platforms.

Are we going to add similar to other drivers as well? I2C core for
example installs I2C operation region handlers for every I2C controller
that happens to have an ACPI handle.

> BTW if we can't find the dependency either on Kconfig or on kernel
> codes, if no dependent is checking for this resource when probing, if
> all we have to define that this dependency exists is by looking random
> ACPI tables (which are neither generic, nor part of kernel), it is then
> implicit WRT kernel. Kernel cannot find it, nor developers can foresee
> it.

OK.

> > > > > But IMHO all dependency to a driver should be explicitly described
> > > > > (e.g. on Kconfigs, or maybe failing probe). With current situation if we
> > > > > do not select pinctrl_baytrail, instead of affecting just the drivers
> > > > > that explicitly depend on that, it affects others which we are unable to
> > > > > easily identify.
> > > > 
> > > > So how do you propose we describe the dependency? It is completely in
> > > > firmware. Should we make i2c-hid.c dependent on pinctrl-baytrail.c just
> > > > because some underlying firmware method (_PSx for example) needs the
> > > > GPIO but the driver itself does not?
> > > 
> > > i2c-hid.c should fail, WARN, yell, scream or whatever :)
> > > This way one could say: hey, we needed GPIO.
> > 
> > But i2c-hid.c does not know anything about GPIOS in the first place.
> > Like I said the dependency is in the firmware level. It may need GPIOs
> > to do something or not but the driver never sees those GPIOs.
> 
> AFAIU any gpio usage by FW would start from a call from i2c-hid.c

Not necesarily. Even before we enter the probe of the i2c-hid.c, the
ACPI power domain might try to power on the device. That might involve
toggling GPIOs from the AML code.

Same thing during system suspend/resume.

The driver is not involved in this but it is the ACPI device related to
the i2c-hid.c that has the GPIO dependency.

> driver. Maybe this call could fail, and then nothing implicit happens.
> But i2c-hid.c does know about the dependency if checks its ACPI table
> during the probe. But that's being addressed by the thread I posted
> above.

Indeed.

However, now that I look at the _DEP implementation closely it seems to
leave this to the drivers and not automatically prevent probing a driver
if the device in questions has unmet dependencies...
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html