| Message ID | 20210303131858.3976-1-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: qcom: support gpio_chip .set_config call | expand |
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 >
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
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
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
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
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
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
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 >
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
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
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
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
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,
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(+)