| Message ID | 20180618205255.246104-3-swboyd@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: msm interrupt and muxing fixes | expand |
Hi, On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote: > We rely on devices to use pinmuxing configurations in DT to select the > GPIO function (function 0) if they're going to use the gpio in GPIO > mode. Let's simplify things for driver authors by implementing > gpio_request_enable() for this pinctrl driver to mux out the GPIO > function when the gpio is use from gpiolib. > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 3563c4394837..eacfc5b85f7f 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > return 0; > } > > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned offset) > +{ > + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; > + > + /* No funcs? Probably ACPI so can't do anything here */ > + if (!g->nfuncs) > + return 0; Is there a reason why you'd want to return 0 instead of some sort of error code? Wouldn't you want to know that this pin can't be a GPIO? Another non-ACPI example is sdc2 on sdm845 and it seems like you'd want to know if someone tried to set one of those as a GPIO. ...oh, but I guess ufs_reset also has no funcs but it still probably wants to use the GPIO framework to write something. Hrmmm... Maybe check if either in_bit or out_bit is not -1? > + > + /* For now assume function 0 is GPIO because it always is */ > + return msm_pinmux_set_mux(pctldev, 0, offset); nit: should you be consistent with msm_pinmux_set_mux() and call it "group" instead of "offset"? It looks like it's supposed to be the same thing... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-06-18 16:54:49) > Hi, > > On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote: > > We rely on devices to use pinmuxing configurations in DT to select the > > GPIO function (function 0) if they're going to use the gpio in GPIO > > mode. Let's simplify things for driver authors by implementing > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > function when the gpio is use from gpiolib. > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > Cc: Doug Anderson <dianders@chromium.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > --- > > drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 3563c4394837..eacfc5b85f7f 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > > return 0; > > } > > > > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, > > + struct pinctrl_gpio_range *range, > > + unsigned offset) > > +{ > > + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; > > + > > + /* No funcs? Probably ACPI so can't do anything here */ > > + if (!g->nfuncs) > > + return 0; > > Is there a reason why you'd want to return 0 instead of some sort of > error code? Wouldn't you want to know that this pin can't be a GPIO? On ACPI there aren't any functions and thus all pins are GPIO mode and only GPIO mode if they're used as GPIOs. At least that's my understanding of how the ACPI version of this driver works. > Another non-ACPI example is sdc2 on sdm845 and it seems like you'd > want to know if someone tried to set one of those as a GPIO. > > ...oh, but I guess ufs_reset also has no funcs but it still probably > wants to use the GPIO framework to write something. Hrmmm... Maybe > check if either in_bit or out_bit is not -1? ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't think we need to care? At least I can't convince myself that those pins would eventually call into the this function. We could check if offset is greater than ngpios for the chip but that seems useless if higher layers are handling this already. > > > > + > > + /* For now assume function 0 is GPIO because it always is */ > > + return msm_pinmux_set_mux(pctldev, 0, offset); > > nit: should you be consistent with msm_pinmux_set_mux() and call it > "group" instead of "offset"? It looks like it's supposed to be the > same thing... > This is copying the generic one and the function pointer prototype, so I think it's best to leave it as 'offset'. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Jun 19, 2018 at 2:18 PM, Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Doug Anderson (2018-06-18 16:54:49) >> Hi, >> >> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote: >> > We rely on devices to use pinmuxing configurations in DT to select the >> > GPIO function (function 0) if they're going to use the gpio in GPIO >> > mode. Let's simplify things for driver authors by implementing >> > gpio_request_enable() for this pinctrl driver to mux out the GPIO >> > function when the gpio is use from gpiolib. >> > >> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> >> > Cc: Doug Anderson <dianders@chromium.org> >> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> >> > --- >> > drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ >> > 1 file changed, 16 insertions(+) >> > >> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> > index 3563c4394837..eacfc5b85f7f 100644 >> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, >> > return 0; >> > } >> > >> > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, >> > + struct pinctrl_gpio_range *range, >> > + unsigned offset) >> > +{ >> > + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); >> > + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; >> > + >> > + /* No funcs? Probably ACPI so can't do anything here */ >> > + if (!g->nfuncs) >> > + return 0; >> >> Is there a reason why you'd want to return 0 instead of some sort of >> error code? Wouldn't you want to know that this pin can't be a GPIO? > > On ACPI there aren't any functions and thus all pins are GPIO mode and > only GPIO mode if they're used as GPIOs. At least that's my > understanding of how the ACPI version of this driver works. OK. I have no understanding of how the ACPI version of this driver works, so your understanding is much more likely to be right than mine. I guess this is just "pinctrl-qdf2xxx.c"? >> Another non-ACPI example is sdc2 on sdm845 and it seems like you'd >> want to know if someone tried to set one of those as a GPIO. >> >> ...oh, but I guess ufs_reset also has no funcs but it still probably >> wants to use the GPIO framework to write something. Hrmmm... Maybe >> check if either in_bit or out_bit is not -1? > > ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't > think we need to care? At least I can't convince myself that those pins > would eventually call into the this function. We could check if offset > is greater than ngpios for the chip but that seems useless if higher > layers are handling this already. Ah, I see what you mean. These pins do have numbers in the code: PINCTRL_PIN(150, "SDC2_CLK"), PINCTRL_PIN(151, "SDC2_CMD"), PINCTRL_PIN(152, "SDC2_DATA"), PINCTRL_PIN(153, "UFS_RESET"), ...but those are effectively made up numbers and they are all past the "ngpios" (150). ...and the higher level code seems to be already checking that. OK, thought I've already proven my cluelessness about this driver, FWIW this patch makes sense to me now so FWIW: Reviewed-by: Douglas Anderson <dianders@chromium.org> -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-06-19 14:38:57) > On Tue, Jun 19, 2018 at 2:18 PM, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2018-06-18 16:54:49) > >> > >> Is there a reason why you'd want to return 0 instead of some sort of > >> error code? Wouldn't you want to know that this pin can't be a GPIO? > > > > On ACPI there aren't any functions and thus all pins are GPIO mode and > > only GPIO mode if they're used as GPIOs. At least that's my > > understanding of how the ACPI version of this driver works. > > OK. I have no understanding of how the ACPI version of this driver > works, so your understanding is much more likely to be right than > mine. I guess this is just "pinctrl-qdf2xxx.c"? > Yes that's the single ACPI driver. > > >> Another non-ACPI example is sdc2 on sdm845 and it seems like you'd > >> want to know if someone tried to set one of those as a GPIO. > >> > >> ...oh, but I guess ufs_reset also has no funcs but it still probably > >> wants to use the GPIO framework to write something. Hrmmm... Maybe > >> check if either in_bit or out_bit is not -1? > > > > ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't > > think we need to care? At least I can't convince myself that those pins > > would eventually call into the this function. We could check if offset > > is greater than ngpios for the chip but that seems useless if higher > > layers are handling this already. > > Ah, I see what you mean. These pins do have numbers in the code: > > PINCTRL_PIN(150, "SDC2_CLK"), > PINCTRL_PIN(151, "SDC2_CMD"), > PINCTRL_PIN(152, "SDC2_DATA"), > PINCTRL_PIN(153, "UFS_RESET"), > > ...but those are effectively made up numbers and they are all past the > "ngpios" (150). ...and the higher level code seems to be already > checking that. Right. Hopefully that saves us from this trouble. > > > OK, thought I've already proven my cluelessness about this driver, > FWIW this patch makes sense to me now so FWIW: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > We rely on devices to use pinmuxing configurations in DT to select the > GPIO function (function 0) if they're going to use the gpio in GPIO > mode. Let's simplify things for driver authors by implementing > gpio_request_enable() for this pinctrl driver to mux out the GPIO > function when the gpio is use from gpiolib. > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > Cc: Doug Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 3563c4394837..eacfc5b85f7f 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > return 0; > } > > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned offset) > +{ > + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; > + > + /* No funcs? Probably ACPI so can't do anything here */ > + if (!g->nfuncs) > + return 0; > + > + /* For now assume function 0 is GPIO because it always is */ > + return msm_pinmux_set_mux(pctldev, 0, offset); > +} > + > static const struct pinmux_ops msm_pinmux_ops = { > .request = msm_pinmux_request, > .get_functions_count = msm_get_functions_count, > .get_function_name = msm_get_function_name, > .get_function_groups = msm_get_function_groups, > + .gpio_request_enable = msm_pinmux_request_gpio, > .set_mux = msm_pinmux_set_mux, > }; > > -- > Sent by a computer through tubes > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > We rely on devices to use pinmuxing configurations in DT to select the > > GPIO function (function 0) if they're going to use the gpio in GPIO > > mode. Let's simplify things for driver authors by implementing > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > function when the gpio is use from gpiolib. > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > On second thought, while reading patch 3, when would this be used? While both patch 2 and 3 are convenient ways to get around the annoyance of having to specify a pinmux state both patches then ends up relying on some default pinconf state; which I think is bad. Further more in situations like i2c-qup (downstream), where the pins are requested as gpios in order to "bitbang" a reset this would mean that the driver has to counter the convenience; by either switching in the default pinmux at the end of probe or postponing the gpio_request() to the invocation of reset and then, after issuing the gpio_release, switching in the default pinmux explicitly again. So I'm not sure we want this. Regards, Bjorn > Regards, > Bjorn > > > Cc: Doug Anderson <dianders@chromium.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > --- > > drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 3563c4394837..eacfc5b85f7f 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > > return 0; > > } > > > > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, > > + struct pinctrl_gpio_range *range, > > + unsigned offset) > > +{ > > + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; > > + > > + /* No funcs? Probably ACPI so can't do anything here */ > > + if (!g->nfuncs) > > + return 0; > > + > > + /* For now assume function 0 is GPIO because it always is */ > > + return msm_pinmux_set_mux(pctldev, 0, offset); > > +} > > + > > static const struct pinmux_ops msm_pinmux_ops = { > > .request = msm_pinmux_request, > > .get_functions_count = msm_get_functions_count, > > .get_function_name = msm_get_function_name, > > .get_function_groups = msm_get_function_groups, > > + .gpio_request_enable = msm_pinmux_request_gpio, > > .set_mux = msm_pinmux_set_mux, > > }; > > > > -- > > Sent by a computer through tubes > > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > > > We rely on devices to use pinmuxing configurations in DT to select the > > > GPIO function (function 0) if they're going to use the gpio in GPIO > > > mode. Let's simplify things for driver authors by implementing > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > > function when the gpio is use from gpiolib. > > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> (...) > While both patch 2 and 3 are convenient ways to get around the annoyance > of having to specify a pinmux state both patches then ends up relying on > some default pinconf state; which I think is bad. Nothing stops you from setting up a default conf in this callback though. But admittedly this call was added for simpler hardware. > Further more in situations like i2c-qup (downstream), where the pins are > requested as gpios in order to "bitbang" a reset this would mean that > the driver has to counter the convenience; by either switching in the > default pinmux at the end of probe or postponing the gpio_request() to > the invocation of reset and then, after issuing the gpio_release, > switching in the default pinmux explicitly again. That's a bigger problem. If the system is using device and GPIO mode orthogonally, it'd be good to leave like this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Linus Walleij (2018-06-28 07:25:46) > On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > > > > > We rely on devices to use pinmuxing configurations in DT to select the > > > > GPIO function (function 0) if they're going to use the gpio in GPIO > > > > mode. Let's simplify things for driver authors by implementing > > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > > > function when the gpio is use from gpiolib. > > > > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > (...) > > While both patch 2 and 3 are convenient ways to get around the annoyance > > of having to specify a pinmux state both patches then ends up relying on > > some default pinconf state; which I think is bad. What default state are we relying on? The reset state of the pins? I'm very confused by this statement. These last two patches are making sure that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO mode. If this code is not in place, then we'll allow drivers to request a GPIO but pinmuxing won't be called to actually mux that pin to GPIO mode unless they have a DT conf for function = "gpio". That seems entirely unexpected and easy to mess up. > > Nothing stops you from setting up a default conf in > this callback though. > > But admittedly this call was added for simpler hardware. > > > Further more in situations like i2c-qup (downstream), where the pins are > > requested as gpios in order to "bitbang" a reset this would mean that > > the driver has to counter the convenience; by either switching in the > > default pinmux at the end of probe or postponing the gpio_request() to > > the invocation of reset and then, after issuing the gpio_release, > > switching in the default pinmux explicitly again. > > That's a bigger problem. If the system is using device and GPIO > mode orthogonally, it'd be good to leave like this. > Doesn't that driver need to explicitly mux GPIO mode vs. device mode right now? So having gpio_request() do the muxing to GPIO mode and then explicit muxing of the pin to device mode would be what we have after this patch, while before this patch we would have mux to GPIO, request GPIO (nop), mux to device. We saved an explicit pinmux call? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Jun 28, 2018 at 10:14 AM, Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Linus Walleij (2018-06-28 07:25:46) >> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson >> <bjorn.andersson@linaro.org> wrote: >> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: >> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: >> > > >> > > > We rely on devices to use pinmuxing configurations in DT to select the >> > > > GPIO function (function 0) if they're going to use the gpio in GPIO >> > > > mode. Let's simplify things for driver authors by implementing >> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO >> > > > function when the gpio is use from gpiolib. >> > > > >> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> >> > > >> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> (...) >> > While both patch 2 and 3 are convenient ways to get around the annoyance >> > of having to specify a pinmux state both patches then ends up relying on >> > some default pinconf state; which I think is bad. > > What default state are we relying on? The reset state of the pins? I'm > very confused by this statement. These last two patches are making sure > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO > mode. If this code is not in place, then we'll allow drivers to request > a GPIO but pinmuxing won't be called to actually mux that pin to GPIO > mode unless they have a DT conf for function = "gpio". That seems > entirely unexpected and easy to mess up. > >> >> Nothing stops you from setting up a default conf in >> this callback though. >> >> But admittedly this call was added for simpler hardware. >> >> > Further more in situations like i2c-qup (downstream), where the pins are >> > requested as gpios in order to "bitbang" a reset this would mean that >> > the driver has to counter the convenience; by either switching in the >> > default pinmux at the end of probe or postponing the gpio_request() to >> > the invocation of reset and then, after issuing the gpio_release, >> > switching in the default pinmux explicitly again. >> >> That's a bigger problem. If the system is using device and GPIO >> mode orthogonally, it'd be good to leave like this. >> > > Doesn't that driver need to explicitly mux GPIO mode vs. device mode > right now? So having gpio_request() do the muxing to GPIO mode and then > explicit muxing of the pin to device mode would be what we have after > this patch, while before this patch we would have mux to GPIO, request > GPIO (nop), mux to device. We saved an explicit pinmux call? After reading these replies I'm slightly worried about some of the stuff Bjorn is worried about too. In Bjorn's example I think that the "default" state of the pins is probably i2c-mode and that it might switch to GPIO mode only when it needs to do the special unwedge of the i2c port. So maybe the i2c driver does this: 1. "Default" pinmux state of i2c is i2c-mode, "unwedge" pinmux state is gpio-mode. 2. In probe, request the GPIOs for use later in case we need to unwedge. Don't use them right now since we don't need to unwedge. Assumes pinmux state stays as "default". 3. When you decide you need to unwedge the bus, pinmux to the special i2c mode and drive the pins you requested in probe. Then pinmux back. ...I think your patch will break this because when you request the GPIOs you'll have an implicit (and unexpected?) pinmux transition. What do you think? NOTE: I think even if we don't want patch #2, we might still want at least part of patch #3? It feels like if you request a GPIO as an IRQ that it should be OK to at least automatically change it to an input... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-06-28 11:45:30) > Hi, > > On Thu, Jun 28, 2018 at 10:14 AM, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Linus Walleij (2018-06-28 07:25:46) > >> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson > >> <bjorn.andersson@linaro.org> wrote: > >> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > >> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > >> > > > >> > > > We rely on devices to use pinmuxing configurations in DT to select the > >> > > > GPIO function (function 0) if they're going to use the gpio in GPIO > >> > > > mode. Let's simplify things for driver authors by implementing > >> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > >> > > > function when the gpio is use from gpiolib. > >> > > > > >> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > >> > > > >> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > >> (...) > >> > While both patch 2 and 3 are convenient ways to get around the annoyance > >> > of having to specify a pinmux state both patches then ends up relying on > >> > some default pinconf state; which I think is bad. > > > > What default state are we relying on? The reset state of the pins? I'm > > very confused by this statement. These last two patches are making sure > > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO > > mode. If this code is not in place, then we'll allow drivers to request > > a GPIO but pinmuxing won't be called to actually mux that pin to GPIO > > mode unless they have a DT conf for function = "gpio". That seems > > entirely unexpected and easy to mess up. > > > >> > >> Nothing stops you from setting up a default conf in > >> this callback though. > >> > >> But admittedly this call was added for simpler hardware. > >> > >> > Further more in situations like i2c-qup (downstream), where the pins are > >> > requested as gpios in order to "bitbang" a reset this would mean that > >> > the driver has to counter the convenience; by either switching in the > >> > default pinmux at the end of probe or postponing the gpio_request() to > >> > the invocation of reset and then, after issuing the gpio_release, > >> > switching in the default pinmux explicitly again. > >> > >> That's a bigger problem. If the system is using device and GPIO > >> mode orthogonally, it'd be good to leave like this. > >> > > > > Doesn't that driver need to explicitly mux GPIO mode vs. device mode > > right now? So having gpio_request() do the muxing to GPIO mode and then > > explicit muxing of the pin to device mode would be what we have after > > this patch, while before this patch we would have mux to GPIO, request > > GPIO (nop), mux to device. We saved an explicit pinmux call? > > After reading these replies I'm slightly worried about some of the > stuff Bjorn is worried about too. In Bjorn's example I think that the > "default" state of the pins is probably i2c-mode and that it might > switch to GPIO mode only when it needs to do the special unwedge of > the i2c port. > > So maybe the i2c driver does this: > > 1. "Default" pinmux state of i2c is i2c-mode, "unwedge" pinmux state > is gpio-mode. > > 2. In probe, request the GPIOs for use later in case we need to > unwedge. Don't use them right now since we don't need to unwedge. > Assumes pinmux state stays as "default". > > 3. When you decide you need to unwedge the bus, pinmux to the special > i2c mode and drive the pins you requested in probe. Then pinmux back. > > > ...I think your patch will break this because when you request the > GPIOs you'll have an implicit (and unexpected?) pinmux transition. > > What do you think? > > I could do with some more clarity from Linus in the "Drivers needing both pin control and GPIOs" section of Documentation/driver-api/pinctl.rst but I read that section as stating that the GPIO driver needs to mux the pin as a GPIO by requesting the pinctrl backend to do so, unless the hardware overrides the muxed function selection when the GPIO is used, without involving pinctrl software. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 28 Jun 10:14 PDT 2018, Stephen Boyd wrote: > Quoting Linus Walleij (2018-06-28 07:25:46) > > On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > > > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > > > > > > > We rely on devices to use pinmuxing configurations in DT to select the > > > > > GPIO function (function 0) if they're going to use the gpio in GPIO > > > > > mode. Let's simplify things for driver authors by implementing > > > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > > > > function when the gpio is use from gpiolib. > > > > > > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > (...) > > > While both patch 2 and 3 are convenient ways to get around the annoyance > > > of having to specify a pinmux state both patches then ends up relying on > > > some default pinconf state; which I think is bad. > > What default state are we relying on? The reset state of the pins? I'm > very confused by this statement. These last two patches are making sure > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO > mode. Yes and this is convenient, as the TLMM is both multiplexor and gpio controller this is probably what people would expect. However looking at the downstream code people don't think this way (i.e. many drivers calls gpio_request() to get some sort of exclusive access to its pins - not to request gpio to be the muxed function). But my concern is related to pinconf, not pinmux - automating pinmuxing doesn't change the fact that the systems integrator should make sure to configure appropriate pull properties on the pins. > If this code is not in place, then we'll allow drivers to request > a GPIO but pinmuxing won't be called to actually mux that pin to GPIO > mode unless they have a DT conf for function = "gpio". That seems > entirely unexpected and easy to mess up. > I do agree with this, but the opposite doesn't feel crystal clear either. > > > > Nothing stops you from setting up a default conf in > > this callback though. > > > > But admittedly this call was added for simpler hardware. > > > > > Further more in situations like i2c-qup (downstream), where the pins are > > > requested as gpios in order to "bitbang" a reset this would mean that > > > the driver has to counter the convenience; by either switching in the > > > default pinmux at the end of probe or postponing the gpio_request() to > > > the invocation of reset and then, after issuing the gpio_release, > > > switching in the default pinmux explicitly again. > > > > That's a bigger problem. If the system is using device and GPIO > > mode orthogonally, it'd be good to leave like this. > > > > Doesn't that driver need to explicitly mux GPIO mode vs. device mode > right now? So having gpio_request() do the muxing to GPIO mode and then > explicit muxing of the pin to device mode would be what we have after > this patch, while before this patch we would have mux to GPIO, request > GPIO (nop), mux to device. We saved an explicit pinmux call? > It's currently possible to call gpio_request() in the i2c driver's probe function and then mux when needed. With this patch you would have to either follow the gpio_request with a mux-to-default or defer the gpio_request until the point where they today would have an explicit mux-to-gpio. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Bjorn Andersson (2018-07-02 12:09:22) > On Thu 28 Jun 10:14 PDT 2018, Stephen Boyd wrote: > > > Quoting Linus Walleij (2018-06-28 07:25:46) > > > On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote: > > > > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > > > > > > > > > We rely on devices to use pinmuxing configurations in DT to select the > > > > > > GPIO function (function 0) if they're going to use the gpio in GPIO > > > > > > mode. Let's simplify things for driver authors by implementing > > > > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO > > > > > > function when the gpio is use from gpiolib. > > > > > > > > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > > > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > (...) > > > > While both patch 2 and 3 are convenient ways to get around the annoyance > > > > of having to specify a pinmux state both patches then ends up relying on > > > > some default pinconf state; which I think is bad. > > > > What default state are we relying on? The reset state of the pins? I'm > > very confused by this statement. These last two patches are making sure > > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO > > mode. > > Yes and this is convenient, as the TLMM is both multiplexor and gpio > controller this is probably what people would expect. However looking at > the downstream code people don't think this way (i.e. many drivers calls > gpio_request() to get some sort of exclusive access to its pins - not > to request gpio to be the muxed function). > > But my concern is related to pinconf, not pinmux - automating pinmuxing > doesn't change the fact that the systems integrator should make sure to > configure appropriate pull properties on the pins. Ok. Pinconf wouldn't change here, and I would expect system integrators to set pinconfs in their DTS board files like always. > > > > > > > Nothing stops you from setting up a default conf in > > > this callback though. > > > > > > But admittedly this call was added for simpler hardware. > > > > > > > Further more in situations like i2c-qup (downstream), where the pins are > > > > requested as gpios in order to "bitbang" a reset this would mean that > > > > the driver has to counter the convenience; by either switching in the > > > > default pinmux at the end of probe or postponing the gpio_request() to > > > > the invocation of reset and then, after issuing the gpio_release, > > > > switching in the default pinmux explicitly again. > > > > > > That's a bigger problem. If the system is using device and GPIO > > > mode orthogonally, it'd be good to leave like this. > > > > > > > Doesn't that driver need to explicitly mux GPIO mode vs. device mode > > right now? So having gpio_request() do the muxing to GPIO mode and then > > explicit muxing of the pin to device mode would be what we have after > > this patch, while before this patch we would have mux to GPIO, request > > GPIO (nop), mux to device. We saved an explicit pinmux call? > > > > It's currently possible to call gpio_request() in the i2c driver's probe > function and then mux when needed. With this patch you would have to > either follow the gpio_request with a mux-to-default or defer the > gpio_request until the point where they today would have an explicit > mux-to-gpio. > Ok, got it. I don't know if we can do any better though, so the question is if gpio_request() muxing the pin for gpio operation is matching driver expectations or not. Also, see my reply to Doug here on the same topic. The documentation file isn't crystal clear to me. If Linus can clear things up a bit I think we can all get on the same page. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote: > I could do with some more clarity from Linus in the "Drivers needing > both pin control and GPIOs" section of > Documentation/driver-api/pinctl.rst but I read that section as stating > that the GPIO driver needs to mux the pin as a GPIO by requesting the > pinctrl backend to do so, unless the hardware overrides the muxed > function selection when the GPIO is used, without involving pinctrl > software. Yeah that text is especially terse :/ What it says (or what I meant to say) is that there is a choice between letting the pin control and GPIO functionality on the same pin be handled orthogonally or implementing these gpio_*() callbacks into the pin control backend, but in either case the two APIs must be used in sequence: pin control setting comes first, second the GPIO subsystem can request the GPIO line. I'll see if I can clarify. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Linus Walleij (2018-07-09 06:54:01) > On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > I could do with some more clarity from Linus in the "Drivers needing > > both pin control and GPIOs" section of > > Documentation/driver-api/pinctl.rst but I read that section as stating > > that the GPIO driver needs to mux the pin as a GPIO by requesting the > > pinctrl backend to do so, unless the hardware overrides the muxed > > function selection when the GPIO is used, without involving pinctrl > > software. > > Yeah that text is especially terse :/ > > What it says (or what I meant to say) is that there is a choice > between letting the pin control and GPIO functionality on the > same pin be handled orthogonally or implementing these > gpio_*() callbacks into the pin control backend, but in either case > the two APIs must be used in sequence: > pin control setting comes first, second the GPIO subsystem can > request the GPIO line. > > I'll see if I can clarify. > Ok. Is my interpretation correct though? The fundamental question here is if gpio_request() should remux the GPIO for the GPIO function or if drivers are expected to have pinmux settings to use their pin as a GPIO. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 9, 2018 at 5:37 PM Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Linus Walleij (2018-07-09 06:54:01) > > On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > I could do with some more clarity from Linus in the "Drivers needing > > > both pin control and GPIOs" section of > > > Documentation/driver-api/pinctl.rst but I read that section as stating > > > that the GPIO driver needs to mux the pin as a GPIO by requesting the > > > pinctrl backend to do so, unless the hardware overrides the muxed > > > function selection when the GPIO is used, without involving pinctrl > > > software. > > > > Yeah that text is especially terse :/ > > > > What it says (or what I meant to say) is that there is a choice > > between letting the pin control and GPIO functionality on the > > same pin be handled orthogonally or implementing these > > gpio_*() callbacks into the pin control backend, but in either case > > the two APIs must be used in sequence: > > pin control setting comes first, second the GPIO subsystem can > > request the GPIO line. > > > > I'll see if I can clarify. > > Ok. Is my interpretation correct though? The fundamental question here > is if gpio_request() should remux the GPIO for the GPIO function or if > drivers are expected to have pinmux settings to use their pin as a GPIO. It's an either/or situation. So there are two ways to do it, as the gpio_request() callback to pinctrl_gpio_request() etc are not compulsory to implement. For any one specific system, it is either done such that gpio_request() does it by calling down to pinctrl_gpio_request() and talking to the pinctrl back-end, OR the pin muxing is done as a side dish without any interaction with the GPIO subsystem. So pick one... I know this is not very consistent. Sorry for the inconvenience :( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 3563c4394837..eacfc5b85f7f 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, return 0; } +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned offset) +{ + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; + + /* No funcs? Probably ACPI so can't do anything here */ + if (!g->nfuncs) + return 0; + + /* For now assume function 0 is GPIO because it always is */ + return msm_pinmux_set_mux(pctldev, 0, offset); +} + static const struct pinmux_ops msm_pinmux_ops = { .request = msm_pinmux_request, .get_functions_count = msm_get_functions_count, .get_function_name = msm_get_function_name, .get_function_groups = msm_get_function_groups, + .gpio_request_enable = msm_pinmux_request_gpio, .set_mux = msm_pinmux_set_mux, };
We rely on devices to use pinmuxing configurations in DT to select the GPIO function (function 0) if they're going to use the gpio in GPIO mode. Let's simplify things for driver authors by implementing gpio_request_enable() for this pinctrl driver to mux out the GPIO function when the gpio is use from gpiolib. Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Doug Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)