[PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Shawn Guo]

In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
configs from ACPI table, and call into gpio_chip's .set_config hook for
setting them up.  It enables such support on qcom platform by using
generic config function, which in turn calls into .pin_config_set of
pinconf for setting up hardware.  For qcom platform, it's possible to
reuse pin group config functions for pin config hooks, because every pin
is maintained as a single group.

This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
pin is not set up by the kernel.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes for v2:
- Add pin config functions that simply call into group config ones.

 drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index af6ed7f43058..a59bb4cbd97e 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -489,10 +489,24 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int msm_config_pin_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			      unsigned long *config)
+{
+	return msm_config_group_get(pctldev, pin, config);
+}
+
+static int msm_config_pin_set(struct pinctrl_dev *pctldev, unsigned pin,
+			      unsigned long *configs, unsigned num_configs)
+{
+	return msm_config_group_set(pctldev, pin, configs, num_configs);
+}
+
 static const struct pinconf_ops msm_pinconf_ops = {
 	.is_generic		= true,
 	.pin_config_group_get	= msm_config_group_get,
 	.pin_config_group_set	= msm_config_group_set,
+	.pin_config_get		= msm_config_pin_get,
+	.pin_config_set		= msm_config_pin_set,
 };
 
 static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
@@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
 	.get_direction    = msm_gpio_get_direction,
 	.get              = msm_gpio_get,
 	.set              = msm_gpio_set,
+	.set_config       = gpiochip_generic_config,
 	.request          = gpiochip_generic_request,
 	.free             = gpiochip_generic_free,
 	.dbg_show         = msm_gpio_dbg_show,
-- 
2.17.1

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Andy Shevchenko]

On Wed, Mar 03, 2021 at 09:18:58PM +0800, Shawn Guo wrote:
> In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
> configs from ACPI table, and call into gpio_chip's .set_config hook for
> setting them up.  It enables such support on qcom platform by using
> generic config function, which in turn calls into .pin_config_set of
> pinconf for setting up hardware.  For qcom platform, it's possible to
> reuse pin group config functions for pin config hooks, because every pin
> is maintained as a single group.
> 
> This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
> doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
> pin is not set up by the kernel.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes for v2:
> - Add pin config functions that simply call into group config ones.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index af6ed7f43058..a59bb4cbd97e 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -489,10 +489,24 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +static int msm_config_pin_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			      unsigned long *config)
> +{
> +	return msm_config_group_get(pctldev, pin, config);
> +}
> +
> +static int msm_config_pin_set(struct pinctrl_dev *pctldev, unsigned pin,
> +			      unsigned long *configs, unsigned num_configs)
> +{
> +	return msm_config_group_set(pctldev, pin, configs, num_configs);
> +}
> +
>  static const struct pinconf_ops msm_pinconf_ops = {
>  	.is_generic		= true,
>  	.pin_config_group_get	= msm_config_group_get,
>  	.pin_config_group_set	= msm_config_group_set,
> +	.pin_config_get		= msm_config_pin_get,
> +	.pin_config_set		= msm_config_pin_set,
>  };
>  
>  static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> @@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
>  	.get_direction    = msm_gpio_get_direction,
>  	.get              = msm_gpio_get,
>  	.set              = msm_gpio_set,
> +	.set_config       = gpiochip_generic_config,
>  	.request          = gpiochip_generic_request,
>  	.free             = gpiochip_generic_free,
>  	.dbg_show         = msm_gpio_dbg_show,
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Bjorn Andersson]

On Wed 03 Mar 07:18 CST 2021, Shawn Guo wrote:

> In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
> configs from ACPI table, and call into gpio_chip's .set_config hook for
> setting them up.  It enables such support on qcom platform by using
> generic config function, which in turn calls into .pin_config_set of
> pinconf for setting up hardware.  For qcom platform, it's possible to
> reuse pin group config functions for pin config hooks, because every pin
> is maintained as a single group.
> 
> This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
> doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
> pin is not set up by the kernel.
> 

I like the fact that this solves your gpio configuration issue, but I'm
uncertain if just adding support for configuring pins (in addition to
groups) in the driver is the right solution.

@Linus, to summarize, the Qualcomm TLMM configures pingroups, but all
gpios are defined as a single pin. pinctrl_gpio_set_config() is invoked
based on the configuration provided in the ACPI tables, so Shawn's
proposal is to just implement "config by pin" as well.
Would this not be a problem shared with all pinctrl drivers that
configure gpios in groups?

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes for v2:
> - Add pin config functions that simply call into group config ones.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index af6ed7f43058..a59bb4cbd97e 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -489,10 +489,24 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +static int msm_config_pin_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			      unsigned long *config)
> +{
> +	return msm_config_group_get(pctldev, pin, config);
> +}
> +
> +static int msm_config_pin_set(struct pinctrl_dev *pctldev, unsigned pin,
> +			      unsigned long *configs, unsigned num_configs)
> +{
> +	return msm_config_group_set(pctldev, pin, configs, num_configs);
> +}
> +
>  static const struct pinconf_ops msm_pinconf_ops = {
>  	.is_generic		= true,
>  	.pin_config_group_get	= msm_config_group_get,
>  	.pin_config_group_set	= msm_config_group_set,
> +	.pin_config_get		= msm_config_pin_get,
> +	.pin_config_set		= msm_config_pin_set,
>  };
>  
>  static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> @@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
>  	.get_direction    = msm_gpio_get_direction,
>  	.get              = msm_gpio_get,
>  	.set              = msm_gpio_set,
> +	.set_config       = gpiochip_generic_config,

Generally the pinconf/pinmux part of the driver deals with groups, and
the gpio_chip deals with gpio numbers. So I think that either
gpiochip_generic_config() should somehow do the translation, or we
should use a different function that does it (even though there's no
translation).

Regards,
Bjorn

>  	.request          = gpiochip_generic_request,
>  	.free             = gpiochip_generic_free,
>  	.dbg_show         = msm_gpio_dbg_show,
> -- 
> 2.17.1
> 

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Shawn Guo]

On Wed, Mar 03, 2021 at 08:51:06AM -0600, Bjorn Andersson wrote:
> > @@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
> >  	.get_direction    = msm_gpio_get_direction,
> >  	.get              = msm_gpio_get,
> >  	.set              = msm_gpio_set,
> > +	.set_config       = gpiochip_generic_config,
> 
> Generally the pinconf/pinmux part of the driver deals with groups, and
> the gpio_chip deals with gpio numbers. So I think that either
> gpiochip_generic_config() should somehow do the translation, or we
> should use a different function that does it (even though there's no
> translation).

The transition from GPIO to PINCTRL world is being done by
pinctrl_gpio_set_config() which is wrapped by gpiochip_generic_config().
This is nothing new from gpiochip_generic_request() and
gpiochip_generic_free() right below.

> >  	.request          = gpiochip_generic_request,
> >  	.free             = gpiochip_generic_free,
> >  	.dbg_show         = msm_gpio_dbg_show,

Shawn

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Bjorn Andersson]

On Wed 03 Mar 20:24 CST 2021, Shawn Guo wrote:

> On Wed, Mar 03, 2021 at 08:51:06AM -0600, Bjorn Andersson wrote:
> > > @@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
> > >  	.get_direction    = msm_gpio_get_direction,
> > >  	.get              = msm_gpio_get,
> > >  	.set              = msm_gpio_set,
> > > +	.set_config       = gpiochip_generic_config,
> > 
> > Generally the pinconf/pinmux part of the driver deals with groups, and
> > the gpio_chip deals with gpio numbers. So I think that either
> > gpiochip_generic_config() should somehow do the translation, or we
> > should use a different function that does it (even though there's no
> > translation).
> 
> The transition from GPIO to PINCTRL world is being done by
> pinctrl_gpio_set_config() which is wrapped by gpiochip_generic_config().
> This is nothing new from gpiochip_generic_request() and
> gpiochip_generic_free() right below.
> 

You're right, this seems analog to the two other gpiochip_generic_*
helpers used below, I should have made this comment on the previous
hunk.

I don't think it's right to have the driver implement both group based
and pin based pin_conf_set/get functions (at least not for the same
entities), but I've not found the time to review the core code to
determine if this would cause any issues.

Regards,
Bjorn

> > >  	.request          = gpiochip_generic_request,
> > >  	.free             = gpiochip_generic_free,
> > >  	.dbg_show         = msm_gpio_dbg_show,
> 
> Shawn

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Linus Walleij]

On Wed, Mar 3, 2021 at 3:51 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> I like the fact that this solves your gpio configuration issue, but I'm
> uncertain if just adding support for configuring pins (in addition to
> groups) in the driver is the right solution.
>
> @Linus, to summarize, the Qualcomm TLMM configures pingroups, but all
> gpios are defined as a single pin. pinctrl_gpio_set_config() is invoked
> based on the configuration provided in the ACPI tables, so Shawn's
> proposal is to just implement "config by pin" as well.
> Would this not be a problem shared with all pinctrl drivers that
> configure gpios in groups?

It is done as Shawn does it in e.g. the Intel drivers.

This is a side effect of ACPI: ACPI thinks about the world mostly
in term of GPIO pins, there was a pin ctrl draft at one point but I don't
think it ever got off the ground. The standards committe just has not
been able to think about the world in terms of pin control. Or they
think the pin control abstraction is just wrong. Could be either.

This means that on ACPI systems pin config will be done with
this mechanism but on DT systems it will be done another way.
The mechanisms are essentially orthogonal usecase-wise, it should
work as long as there is some proper testing and concern
for both cases.

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Andy Shevchenko]

On Thu, Mar 04, 2021 at 09:41:05AM +0100, Linus Walleij wrote:
> On Wed, Mar 3, 2021 at 3:51 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> 
> > I like the fact that this solves your gpio configuration issue, but I'm
> > uncertain if just adding support for configuring pins (in addition to
> > groups) in the driver is the right solution.
> >
> > @Linus, to summarize, the Qualcomm TLMM configures pingroups, but all
> > gpios are defined as a single pin. pinctrl_gpio_set_config() is invoked
> > based on the configuration provided in the ACPI tables, so Shawn's
> > proposal is to just implement "config by pin" as well.
> > Would this not be a problem shared with all pinctrl drivers that
> > configure gpios in groups?
> 
> It is done as Shawn does it in e.g. the Intel drivers.
> 
> This is a side effect of ACPI: ACPI thinks about the world mostly
> in term of GPIO pins, there was a pin ctrl draft at one point but I don't
> think it ever got off the ground. The standards committe just has not
> been able to think about the world in terms of pin control. Or they
> think the pin control abstraction is just wrong. Could be either.

Pin control has been though thru and implemented in the ACPICA, but we have no
time to fulfil this work to cover pin control subsystem in the Linux kernel.

> This means that on ACPI systems pin config will be done with
> this mechanism but on DT systems it will be done another way.
> The mechanisms are essentially orthogonal usecase-wise, it should
> work as long as there is some proper testing and concern
> for both cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Linus Walleij]

On Thu, Mar 4, 2021 at 1:40 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:

> Pin control has been though thru and implemented in the ACPICA, but we have no
> time to fulfil this work to cover pin control subsystem in the Linux kernel.

Oh sweet! I suppose it means that firmware using it could appear,
and at that point it will become a matter of priority to get it done in the
kernel.

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Andy Shevchenko]

On Tue, Mar 09, 2021 at 05:22:51PM +0100, Linus Walleij wrote:
> On Thu, Mar 4, 2021 at 1:40 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> 
> > Pin control has been though thru and implemented in the ACPICA, but we have no
> > time to fulfil this work to cover pin control subsystem in the Linux kernel.
> 
> Oh sweet! I suppose it means that firmware using it could appear,
> and at that point it will become a matter of priority to get it done in the
> kernel.

Yes, but so far it didn't appear.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Bjorn Andersson]

On Wed 03 Mar 07:18 CST 2021, Shawn Guo wrote:

> In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
> configs from ACPI table, and call into gpio_chip's .set_config hook for
> setting them up.  It enables such support on qcom platform by using
> generic config function, which in turn calls into .pin_config_set of
> pinconf for setting up hardware.  For qcom platform, it's possible to
> reuse pin group config functions for pin config hooks, because every pin
> is maintained as a single group.
> 
> This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
> doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
> pin is not set up by the kernel.
> 

Per Linus comment that this is how others are doing it, I guess we can
do it too...

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes for v2:
> - Add pin config functions that simply call into group config ones.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index af6ed7f43058..a59bb4cbd97e 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -489,10 +489,24 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +static int msm_config_pin_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			      unsigned long *config)
> +{
> +	return msm_config_group_get(pctldev, pin, config);
> +}
> +
> +static int msm_config_pin_set(struct pinctrl_dev *pctldev, unsigned pin,
> +			      unsigned long *configs, unsigned num_configs)
> +{
> +	return msm_config_group_set(pctldev, pin, configs, num_configs);
> +}
> +
>  static const struct pinconf_ops msm_pinconf_ops = {
>  	.is_generic		= true,
>  	.pin_config_group_get	= msm_config_group_get,
>  	.pin_config_group_set	= msm_config_group_set,
> +	.pin_config_get		= msm_config_pin_get,
> +	.pin_config_set		= msm_config_pin_set,
>  };
>  
>  static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> @@ -717,6 +731,7 @@ static const struct gpio_chip msm_gpio_template = {
>  	.get_direction    = msm_gpio_get_direction,
>  	.get              = msm_gpio_get,
>  	.set              = msm_gpio_set,
> +	.set_config       = gpiochip_generic_config,
>  	.request          = gpiochip_generic_request,
>  	.free             = gpiochip_generic_free,
>  	.dbg_show         = msm_gpio_dbg_show,
> -- 
> 2.17.1
> 

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Linus Walleij]

On Wed, Mar 3, 2021 at 2:19 PM Shawn Guo <shawn.guo@linaro.org> wrote:

> In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
> configs from ACPI table, and call into gpio_chip's .set_config hook for
> setting them up.  It enables such support on qcom platform by using
> generic config function, which in turn calls into .pin_config_set of
> pinconf for setting up hardware.  For qcom platform, it's possible to
> reuse pin group config functions for pin config hooks, because every pin
> is maintained as a single group.
>
> This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
> doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
> pin is not set up by the kernel.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes for v2:
> - Add pin config functions that simply call into group config ones.

Patch applied!

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Bjorn Andersson]

On Wed 10 Mar 17:03 CST 2021, Linus Walleij wrote:

> On Wed, Mar 3, 2021 at 2:19 PM Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> > In case of ACPI boot, GPIO core does the right thing to parse GPIO pin
> > configs from ACPI table, and call into gpio_chip's .set_config hook for
> > setting them up.  It enables such support on qcom platform by using
> > generic config function, which in turn calls into .pin_config_set of
> > pinconf for setting up hardware.  For qcom platform, it's possible to
> > reuse pin group config functions for pin config hooks, because every pin
> > is maintained as a single group.
> >
> > This change fixes the problem that Touchpad of Lenovo Flex 5G laptop
> > doesn't work with ACPI boot, because PullUp config of Touchpad GpioInt
> > pin is not set up by the kernel.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > Changes for v2:
> > - Add pin config functions that simply call into group config ones.
> 
> Patch applied!
> 

As discussed in [1]; several key Qualcomm platforms have a gpio-ranges
representing the number of real GPIOs, but we then expose the UFS reset
GPO (no input) pin as a GPIO as well - making the two numbers differ.

I've moved ahead and merged the update to gpio-ranges, to make the two
match, but Shawn reports that with the introduction of .set_config() all
existing DTBs fail to probe storage because of the UFS code getting
EPROBE_DEFER back on its gpiod_get_optional().

I don't know how to make the transition, but can you please revert this
patch, to avoid breaking compatibility with DTBs out there?

[1] https://lore.kernel.org/linux-arm-msm/20210311230924.GX17424@dragon/#t

Regards,
Bjorn

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Linus Walleij]

On Fri, Mar 12, 2021 at 12:22 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> I don't know how to make the transition, but can you please revert this
> patch, to avoid breaking compatibility with DTBs out there?

OK reverted for now. Does this imply I cannot apply Shawn's ACPI
support patch either? I.e. is this a prerequisite?

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: qcom: support gpio_chip .set_config call

[Bjorn Andersson]

On Mon 15 Mar 10:36 CDT 2021, Linus Walleij wrote:

> On Fri, Mar 12, 2021 at 12:22 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> 
> > I don't know how to make the transition, but can you please revert this
> > patch, to avoid breaking compatibility with DTBs out there?
> 
> OK reverted for now. Does this imply I cannot apply Shawn's ACPI
> support patch either? I.e. is this a prerequisite?
> 

I presume you're referring to [1], which should be fine to merge.

Iiuc the problem that this (.set_config) patch resolves is that
definitions of gpios as interrupts will trickle down to a .set_config
call, which is necessary to get appropriate bias.

[1] https://lore.kernel.org/linux-arm-msm/20210311024102.15450-1-shawn.guo@linaro.org/

Regards,
Bjorn