Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Doug Anderson]

Hi,

On Tue, Jun 19, 2018 at 2:18 PM, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> Quoting Doug Anderson (2018-06-18 16:54:49)
>> Hi,
>>
>> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> > Cc: Doug Anderson <dianders@xxxxxxxxxxxx>
>> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxxxx>

-Doug