Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Hi Wolfgang,
On Wed, 8 Jul 2020 at 05:06, Wolfgang Wallner
wrote:
>
> Hi Simon, Bin,
>
> -"Simon Glass" schrieb: -----
> > Betreff: Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI
> > info for a GPIO
> >
> > Hi Bin,
> >
> > On Tue, 30 Jun 2020 at 01:47, Bin Meng wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Jun 15, 2020 at 11:58 AM Simon Glass wrote:
> > > >
> > > > Implement the method that converts a GPIO into the form used by ACPI, so
> > > > that GPIOs can be added to ACPI tables.
> > > >
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > Changes in v1:
> > > > - Use acpi_get_path() to get device path
> > > >
> > > > drivers/gpio/intel_gpio.c | 34 ++
> > > > 1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > > > index b4d5be97da..6a3a8c4cfa 100644
> > > > --- a/drivers/gpio/intel_gpio.c
> > > > +++ b/drivers/gpio/intel_gpio.c
> > > > @@ -12,6 +12,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -19,6 +20,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > >
> > > > static int intel_gpio_direction_input(struct udevice *dev, uint offset)
> > > > @@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice
> > > > *orig_dev, struct gpio_desc *desc,
> > > > return 0;
> > > > }
> > > >
> > > > +#if CONFIG_IS_ENABLED(ACPIGEN)
> > > > +static int intel_gpio_get_acpi(const struct gpio_desc *desc,
> > > > + struct acpi_gpio *gpio)
> > > > +{
> > > > + struct udevice *pinctrl;
> > > > + int ret;
> > > > +
> > > > + if (!dm_gpio_is_valid(desc))
> > > > + return -ENOENT;
> > > > + pinctrl = dev_get_parent(desc->dev);
> > > > +
> > > > + memset(gpio, '\0', sizeof(*gpio));
> > > > +
> > > > + gpio->type = ACPI_GPIO_TYPE_IO;
> > > > + gpio->pull = ACPI_GPIO_PULL_DEFAULT;
> > > > + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
> > > > + gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
> > >
> > > Is there a way to figure out these properties from DT, instead of
> > > hardcoding?
> >
> > The answer is similar to your previous comment. But also, each pinctrl
> > driver has its own settings and limitations. If we want to support
> > different config for the pinctrl we would likely add it to the DT
> > binding for the pinctrl driver. So far I haven't seen a need but it
> > might happen with a future arch.
>
> Could gpio->pull and gpio->polarity be set depending on desc->flags?
To some extent, except that we don't have an equivalent to
ACPI_GPIO_PULL_DEFAULT.
>
> What is the reason for setting gpio->io_restrict to
> ACPI_GPIO_IO_RESTRICT_OUTPUT?
At present we only support outputs.
>
> > [..]
>
> regards, Wolfgang
Regards,
SImon
Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Hi Simon, Bin,
-"Simon Glass" schrieb: -
> Betreff: Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info
> for a GPIO
>
> Hi Bin,
>
> On Tue, 30 Jun 2020 at 01:47, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jun 15, 2020 at 11:58 AM Simon Glass wrote:
> > >
> > > Implement the method that converts a GPIO into the form used by ACPI, so
> > > that GPIOs can be added to ACPI tables.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v1:
> > > - Use acpi_get_path() to get device path
> > >
> > > drivers/gpio/intel_gpio.c | 34 ++
> > > 1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > > index b4d5be97da..6a3a8c4cfa 100644
> > > --- a/drivers/gpio/intel_gpio.c
> > > +++ b/drivers/gpio/intel_gpio.c
> > > @@ -12,6 +12,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -19,6 +20,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > static int intel_gpio_direction_input(struct udevice *dev, uint offset)
> > > @@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice
> > > *orig_dev, struct gpio_desc *desc,
> > > return 0;
> > > }
> > >
> > > +#if CONFIG_IS_ENABLED(ACPIGEN)
> > > +static int intel_gpio_get_acpi(const struct gpio_desc *desc,
> > > + struct acpi_gpio *gpio)
> > > +{
> > > + struct udevice *pinctrl;
> > > + int ret;
> > > +
> > > + if (!dm_gpio_is_valid(desc))
> > > + return -ENOENT;
> > > + pinctrl = dev_get_parent(desc->dev);
> > > +
> > > + memset(gpio, '\0', sizeof(*gpio));
> > > +
> > > + gpio->type = ACPI_GPIO_TYPE_IO;
> > > + gpio->pull = ACPI_GPIO_PULL_DEFAULT;
> > > + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
> > > + gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
> >
> > Is there a way to figure out these properties from DT, instead of
> > hardcoding?
>
> The answer is similar to your previous comment. But also, each pinctrl
> driver has its own settings and limitations. If we want to support
> different config for the pinctrl we would likely add it to the DT
> binding for the pinctrl driver. So far I haven't seen a need but it
> might happen with a future arch.
Could gpio->pull and gpio->polarity be set depending on desc->flags?
What is the reason for setting gpio->io_restrict to
ACPI_GPIO_IO_RESTRICT_OUTPUT?
> [..]
regards, Wolfgang
Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Hi Bin,
On Tue, 30 Jun 2020 at 01:47, Bin Meng wrote:
>
> Hi Simon,
>
> On Mon, Jun 15, 2020 at 11:58 AM Simon Glass wrote:
> >
> > Implement the method that converts a GPIO into the form used by ACPI, so
> > that GPIOs can be added to ACPI tables.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > Changes in v1:
> > - Use acpi_get_path() to get device path
> >
> > drivers/gpio/intel_gpio.c | 34 ++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > index b4d5be97da..6a3a8c4cfa 100644
> > --- a/drivers/gpio/intel_gpio.c
> > +++ b/drivers/gpio/intel_gpio.c
> > @@ -12,6 +12,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -19,6 +20,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > static int intel_gpio_direction_input(struct udevice *dev, uint offset)
> > @@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice *orig_dev,
> > struct gpio_desc *desc,
> > return 0;
> > }
> >
> > +#if CONFIG_IS_ENABLED(ACPIGEN)
> > +static int intel_gpio_get_acpi(const struct gpio_desc *desc,
> > + struct acpi_gpio *gpio)
> > +{
> > + struct udevice *pinctrl;
> > + int ret;
> > +
> > + if (!dm_gpio_is_valid(desc))
> > + return -ENOENT;
> > + pinctrl = dev_get_parent(desc->dev);
> > +
> > + memset(gpio, '\0', sizeof(*gpio));
> > +
> > + gpio->type = ACPI_GPIO_TYPE_IO;
> > + gpio->pull = ACPI_GPIO_PULL_DEFAULT;
> > + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
> > + gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
>
> Is there a way to figure out these properties from DT, instead of hardcoding?
The answer is similar to your previous comment. But also, each pinctrl
driver has its own settings and limitations. If we want to support
different config for the pinctrl we would likely add it to the DT
binding for the pinctrl driver. So far I haven't seen a need but it
might happen with a future arch.
[..]
Regards,
Simon
Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Hi Simon,
On Mon, Jun 15, 2020 at 11:58 AM Simon Glass wrote:
>
> Implement the method that converts a GPIO into the form used by ACPI, so
> that GPIOs can be added to ACPI tables.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v1:
> - Use acpi_get_path() to get device path
>
> drivers/gpio/intel_gpio.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> index b4d5be97da..6a3a8c4cfa 100644
> --- a/drivers/gpio/intel_gpio.c
> +++ b/drivers/gpio/intel_gpio.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -19,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static int intel_gpio_direction_input(struct udevice *dev, uint offset)
> @@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice *orig_dev,
> struct gpio_desc *desc,
> return 0;
> }
>
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +static int intel_gpio_get_acpi(const struct gpio_desc *desc,
> + struct acpi_gpio *gpio)
> +{
> + struct udevice *pinctrl;
> + int ret;
> +
> + if (!dm_gpio_is_valid(desc))
> + return -ENOENT;
> + pinctrl = dev_get_parent(desc->dev);
> +
> + memset(gpio, '\0', sizeof(*gpio));
> +
> + gpio->type = ACPI_GPIO_TYPE_IO;
> + gpio->pull = ACPI_GPIO_PULL_DEFAULT;
> + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
> + gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
Is there a way to figure out these properties from DT, instead of hardcoding?
> + gpio->pin_count = 1;
> + gpio->pins[0] = intel_pinctrl_get_acpi_pin(pinctrl, desc->offset);
> + gpio->pin0_addr = intel_pinctrl_get_config_reg_addr(pinctrl,
> + desc->offset);
> + ret = acpi_get_path(pinctrl, gpio->resource, sizeof(gpio->resource));
> + if (ret)
> + return log_msg_ret("resource", ret);
> +
> + return 0;
> +}
> +#endif
> +
> static int intel_gpio_probe(struct udevice *dev)
> {
> return 0;
> @@ -152,6 +183,9 @@ static const struct dm_gpio_ops gpio_intel_ops = {
> .set_value = intel_gpio_set_value,
> .get_function = intel_gpio_get_function,
> .xlate = intel_gpio_xlate,
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> + .get_acpi = intel_gpio_get_acpi,
> +#endif
> };
>
> static const struct udevice_id intel_intel_gpio_ids[] = {
> --
Regards,
Bin
[PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Implement the method that converts a GPIO into the form used by ACPI, so
that GPIOs can be added to ACPI tables.
Signed-off-by: Simon Glass
---
Changes in v1:
- Use acpi_get_path() to get device path
drivers/gpio/intel_gpio.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index b4d5be97da..6a3a8c4cfa 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -19,6 +20,7 @@
#include
#include
#include
+#include
#include
static int intel_gpio_direction_input(struct udevice *dev, uint offset)
@@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice *orig_dev,
struct gpio_desc *desc,
return 0;
}
+#if CONFIG_IS_ENABLED(ACPIGEN)
+static int intel_gpio_get_acpi(const struct gpio_desc *desc,
+ struct acpi_gpio *gpio)
+{
+ struct udevice *pinctrl;
+ int ret;
+
+ if (!dm_gpio_is_valid(desc))
+ return -ENOENT;
+ pinctrl = dev_get_parent(desc->dev);
+
+ memset(gpio, '\0', sizeof(*gpio));
+
+ gpio->type = ACPI_GPIO_TYPE_IO;
+ gpio->pull = ACPI_GPIO_PULL_DEFAULT;
+ gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
+ gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
+ gpio->pin_count = 1;
+ gpio->pins[0] = intel_pinctrl_get_acpi_pin(pinctrl, desc->offset);
+ gpio->pin0_addr = intel_pinctrl_get_config_reg_addr(pinctrl,
+ desc->offset);
+ ret = acpi_get_path(pinctrl, gpio->resource, sizeof(gpio->resource));
+ if (ret)
+ return log_msg_ret("resource", ret);
+
+ return 0;
+}
+#endif
+
static int intel_gpio_probe(struct udevice *dev)
{
return 0;
@@ -152,6 +183,9 @@ static const struct dm_gpio_ops gpio_intel_ops = {
.set_value = intel_gpio_set_value,
.get_function = intel_gpio_get_function,
.xlate = intel_gpio_xlate,
+#if CONFIG_IS_ENABLED(ACPIGEN)
+ .get_acpi = intel_gpio_get_acpi,
+#endif
};
static const struct udevice_id intel_intel_gpio_ids[] = {
--
2.27.0.290.gba653c62da-goog
