Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO

2020-07-08 Thread Wolfgang Wallner
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

2020-07-07 Thread Simon Glass
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

2020-06-30 Thread Bin Meng
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