[2/3] pinctrl: msm: Mux out gpio function with gpio_request()
diff mbox series

Message ID 20180618205255.246104-3-swboyd@chromium.org
State New
Headers show
Series
  • pinctrl: msm interrupt and muxing fixes
Related show

Commit Message

Stephen Boyd June 18, 2018, 8:52 p.m. UTC
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(+)

Comments

Doug Anderson June 18, 2018, 11:54 p.m. UTC | #1
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
Stephen Boyd June 19, 2018, 9:18 p.m. UTC | #2
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
Doug Anderson June 19, 2018, 9:38 p.m. UTC | #3
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
Stephen Boyd June 20, 2018, 5:53 a.m. UTC | #4
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
Bjorn Andersson June 22, 2018, 5:58 p.m. UTC | #5
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
Bjorn Andersson June 22, 2018, 6:31 p.m. UTC | #6
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
Linus Walleij June 28, 2018, 2:25 p.m. UTC | #7
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
Stephen Boyd June 28, 2018, 5:14 p.m. UTC | #8
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
Doug Anderson June 28, 2018, 6:45 p.m. UTC | #9
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
Stephen Boyd July 2, 2018, 5:56 p.m. UTC | #10
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
Bjorn Andersson July 2, 2018, 7:09 p.m. UTC | #11
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
Stephen Boyd July 6, 2018, 5:23 p.m. UTC | #12
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
Linus Walleij July 9, 2018, 1:54 p.m. UTC | #13
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
Stephen Boyd July 9, 2018, 3:37 p.m. UTC | #14
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
Linus Walleij July 13, 2018, 6:59 a.m. UTC | #15
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

Patch
diff mbox series

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,
 };