| Message ID | 20200922184544.2920969-27-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 1e4073b8559615509af45f2f826d157c39841fd0 |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | dm: Add programatic generation of ACPI tables (part D) | expand |
On Tue, Sep 22, 2020 at 12:45:43PM -0600, Simon Glass wrote: > Add some documentation provided by Andy Shevchenko to describe how to > use struct acpi_gpio. Thanks! I see Bin already applied this, perhaps follow up fix is needed. See below. ... > + * Note that GpioIo doesn't have any means of Active Low / High setting, so a GpioIo -> GpioIo() > + * _DSD must be provided to mitigate this. Plus the following: "This parameter does not make sense for GpioInt() since it has its own means to define it." > + * GpioIo doesn't properly communicate the initial state of the output pin, GpioIo -> GpioIo() > + * thus Linux assumes the simple rule: > + * > + * Pull Bias Polarity Requested... > + * > + * Implicit x AS IS (assumed firmware configured for us) > + * Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), > + * assuming non-active (Polarity = !Pull Bias) > + * > + * Down Low as low, assuming active > + * Down High as high, assuming non-active Should be read: " * Down High as low, assuming non-active" > + * Up Low as high, assuming non-active > + * Up High as high, assuming active > + * > + * GpioIo() can be used as interrupt and in this case the IoRestriction mustn't > + * be OutputOnly. > + * It also requires active_low flag from _DSD in cases where it's > + * needed (better to always provide than rely on above assumption made on OS > + * level).
Hi Andy, On Thu, 29 Oct 2020 at 06:22, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 22, 2020 at 12:45:43PM -0600, Simon Glass wrote: > > Add some documentation provided by Andy Shevchenko to describe how to > > use struct acpi_gpio. > > Thanks! > > I see Bin already applied this, perhaps follow up fix is needed. See below. > > ... > > > + * Note that GpioIo doesn't have any means of Active Low / High setting, so a > > GpioIo -> GpioIo() > > > + * _DSD must be provided to mitigate this. > > Plus the following: > > "This parameter does not make sense for GpioInt() since it has its own means > to define it." > > > + * GpioIo doesn't properly communicate the initial state of the output pin, > > GpioIo -> GpioIo() > > > + * thus Linux assumes the simple rule: > > + * > > + * Pull Bias Polarity Requested... > > + * > > + * Implicit x AS IS (assumed firmware configured for us) > > + * Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), > > + * assuming non-active (Polarity = !Pull Bias) > > + * > > + * Down Low as low, assuming active > > > + * Down High as high, assuming non-active > > Should be read: > > " * Down High as low, assuming non-active" > > > + * Up Low as high, assuming non-active > > + * Up High as high, assuming active > > + * > > + * GpioIo() can be used as interrupt and in this case the IoRestriction mustn't > > + * be OutputOnly. > > + * It also requires active_low flag from _DSD in cases where it's > > + * needed (better to always provide than rely on above assumption made on OS > > + * level). > Thanks, I sent a patch for these. Regards, Simon
diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h index 1b838fcb857..007b7e7caf1 100644 --- a/include/acpi/acpi_device.h +++ b/include/acpi/acpi_device.h @@ -170,6 +170,28 @@ enum acpi_gpio_polarity { * @io_shared; true if GPIO is shared * @io_restrict: I/O restriction setting * @polarity: GPIO polarity + * + * Note that GpioIo doesn't have any means of Active Low / High setting, so a + * _DSD must be provided to mitigate this. + * + * GpioIo doesn't properly communicate the initial state of the output pin, + * thus Linux assumes the simple rule: + * + * Pull Bias Polarity Requested... + * + * Implicit x AS IS (assumed firmware configured for us) + * Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), + * assuming non-active (Polarity = !Pull Bias) + * + * Down Low as low, assuming active + * Down High as high, assuming non-active + * Up Low as high, assuming non-active + * Up High as high, assuming active + * + * GpioIo() can be used as interrupt and in this case the IoRestriction mustn't + * be OutputOnly. It also requires active_low flag from _DSD in cases where it's + * needed (better to always provide than rely on above assumption made on OS + * level). */ struct acpi_gpio { int pin_count;
Add some documentation provided by Andy Shevchenko to describe how to use struct acpi_gpio. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v4: - Add Andy's documentation to struct acpi_gpio include/acpi/acpi_device.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)