[PATCH] pinctrl: msm: allow the gpio base to be configurable

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Timur Tabi]

Add an integer to the msm_pinctrl_soc_data struct that pinctrl-msm
client drivers can use to specify the gpio base.  This is useful
if the client driver wants to register multiple TLMM devices, because
each one needs a distinct base.

pinctrl-msm currently sets the base to 0, which ensures that GPIOs
of the first TLMM are numbered 0..n-1.  It could specify -1 as the
base, which would tell gpiolib to choose a unique base, but this
has the side-effect of choosing a non-zero base for all TLMMs:

gpiochip_find_base: found new base at 437
gpio gpiochip0: (QCOM8002:00): added GPIO chardev (254:0)
gpiochip_setup_dev: registered GPIOs 437 to 511 on device: gpiochip0 (QCOM8002:00)
gpio gpiochip0: (QCOM8002:00): created GPIO range 0->74 ==> QCOM8002:00 PIN 0->74
gpiochip_find_base: found new base at 362
gpio gpiochip1: (QCOM8002:01): added GPIO chardev (254:1)
gpiochip_setup_dev: registered GPIOs 362 to 436 on device: gpiochip1 (QCOM8002:01)
gpio gpiochip1: (QCOM8002:01): created GPIO range 0->74 ==> QCOM8002:01 PIN 0->74

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 2 +-
 drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index b7b6849625ec..4dc76e15bd14 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -901,7 +901,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return -EINVAL;
 
 	chip = &pctrl->chip;
-	chip->base = 0;
+	chip->base = pctrl->soc->base;
 	chip->ngpio = ngpio;
 	chip->label = dev_name(pctrl->dev);
 	chip->parent = pctrl->dev;
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..cab26f99011d 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -107,6 +107,7 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @base:	    The base GPIO (normally 0 if only one TLMM block)
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -117,6 +118,7 @@ struct msm_pinctrl_soc_data {
 	unsigned ngroups;
 	unsigned ngpios;
 	bool pull_no_keeper;
+	int base;
 };
 
 int msm_pinctrl_probe(struct platform_device *pdev,
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Linus Walleij]

Hi Timur,

thanks for the patch!

On Thu, Jan 25, 2018 at 10:20 PM, Timur Tabi <timur@codeaurora.org> wrote:

> Add an integer to the msm_pinctrl_soc_data struct that pinctrl-msm
> client drivers can use to specify the gpio base.  This is useful
> if the client driver wants to register multiple TLMM devices, because
> each one needs a distinct base.

Sorry, but NACK.

> pinctrl-msm currently sets the base to 0, which ensures that GPIOs
> of the first TLMM are numbered 0..n-1.  It could specify -1 as the
> base, which would tell gpiolib to choose a unique base, but this
> has the side-effect of choosing a non-zero base for all TLMMs:

This is a feature not a bug. It encourages people not to
depend on the global GPIO numberspace.

Just set it to -1.

> gpiochip_find_base: found new base at 437
(...)
> gpiochip_find_base: found new base at 362

These are awesome bases, just beautiful. Use this.

If you don't like seeing GPIO base numbers like this: use things
like the chardev and the tools in tools/gpio or libgpiod when
developing, and you will never see them. They should not make
a difference anyway.

Yours,
Linus Walleij

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Timur Tabi]

On 1/26/18 7:01 AM, Linus Walleij wrote:
> This is a feature not a bug. It encourages people not to
> depend on the global GPIO numberspace.
> 
> Just set it to -1.

If I change it to -1, then I think I'm going to break every existing MSM 
platform that depends on the base address being 0, because then every 
MSM driver will have a non-zero base, and none of the existing drivers 
register more than one GPIO device.

So how about this:

	static int base = 0;

	chip->base = base;
	base = -1;

This way, existing code works as before.  If any driver registers two 
GPIO devices, the first one will get a base of 0, and the second one 
will get some other base.

>> gpiochip_find_base: found new base at 437
> (...)
>> gpiochip_find_base: found new base at 362
> These are awesome bases, just beautiful. Use this.
> 
> If you don't like seeing GPIO base numbers like this: use things
> like the chardev and the tools in tools/gpio or libgpiod when
> developing, and you will never see them. They should not make
> a difference anyway.

Can you tell me more about the chardev?  I've always been using "echo X 
 > /sys/class/gpio/export", so I guess that's not the right way to do 
things.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Bartosz Golaszewski]

2018-01-26 14:16 GMT+01:00 Timur Tabi <timur@codeaurora.org>:
> On 1/26/18 7:01 AM, Linus Walleij wrote:
>>
>> This is a feature not a bug. It encourages people not to
>> depend on the global GPIO numberspace.
>>
>> Just set it to -1.
>
>
> If I change it to -1, then I think I'm going to break every existing MSM
> platform that depends on the base address being 0, because then every MSM
> driver will have a non-zero base, and none of the existing drivers register
> more than one GPIO device.
>
> So how about this:
>
>         static int base = 0;
>
>         chip->base = base;
>         base = -1;
>
> This way, existing code works as before.  If any driver registers two GPIO
> devices, the first one will get a base of 0, and the second one will get
> some other base.
>
>>> gpiochip_find_base: found new base at 437
>>
>> (...)
>>>
>>> gpiochip_find_base: found new base at 362
>>
>> These are awesome bases, just beautiful. Use this.
>>
>> If you don't like seeing GPIO base numbers like this: use things
>> like the chardev and the tools in tools/gpio or libgpiod when
>> developing, and you will never see them. They should not make
>> a difference anyway.
>
>
> Can you tell me more about the chardev?  I've always been using "echo X >
> /sys/class/gpio/export", so I guess that's not the right way to do things.
>

Hi Timur,

take a look at the in-project documentation[1] and read the article[2]
about libgpiod. That should get you started.

Let me know if anything's not clear.

Thanks,
Bartosz

[1] https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
[2] https://www.cnx-software.com/2017/11/03/learn-more-about-linuxs-new-gpio-user-space-subsystem-libgpiod/

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Bjorn Andersson]

On Thu 25 Jan 13:20 PST 2018, Timur Tabi wrote:

> Add an integer to the msm_pinctrl_soc_data struct that pinctrl-msm
> client drivers can use to specify the gpio base.  This is useful
> if the client driver wants to register multiple TLMM devices, because
> each one needs a distinct base.
> 
> pinctrl-msm currently sets the base to 0, which ensures that GPIOs
> of the first TLMM are numbered 0..n-1.  It could specify -1 as the
> base, which would tell gpiolib to choose a unique base, but this
> has the side-effect of choosing a non-zero base for all TLMMs:

What platform has multiple TLMMs?

[..]

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index b7b6849625ec..4dc76e15bd14 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -901,7 +901,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  		return -EINVAL;
>  
>  	chip = &pctrl->chip;
> -	chip->base = 0;

My bad, this should have been -1.

Regards,
Bjorn

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Timur Tabi]

On 1/28/18 5:23 PM, Bjorn Andersson wrote:
> What platform has multiple TLMMs?
> 
> [..]

An upcoming one.

>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index b7b6849625ec..4dc76e15bd14 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -901,7 +901,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>   		return -EINVAL;
>>   
>>   	chip = &pctrl->chip;
>> -	chip->base = 0;

> My bad, this should have been -1.

Perhaps, but it's been 0 for a very long time, so I don't want to break 
any existing platforms by suddenly relocating all GPIOs across all 
Qualcomm platforms.

What do you think about my other idea?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Bjorn Andersson]

On Sun 28 Jan 15:29 PST 2018, Timur Tabi wrote:

> On 1/28/18 5:23 PM, Bjorn Andersson wrote:
> > What platform has multiple TLMMs?
> > 
> > [..]
> 
> An upcoming one.
> 

Cool :)

> > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > > index b7b6849625ec..4dc76e15bd14 100644
> > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > > @@ -901,7 +901,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> > >   		return -EINVAL;
> > >   	chip = &pctrl->chip;
> > > -	chip->base = 0;
> 
> > My bad, this should have been -1.
> 
> Perhaps, but it's been 0 for a very long time, so I don't want to break any
> existing platforms by suddenly relocating all GPIOs across all Qualcomm
> platforms.
> 

Yeah, I see that I got this wrong when I wrote the driver 4 years ago...

There should be no in-kernel users depending on these numbers being hard
coded, so anyone depending on these numbers starting at 0 would be user
space - doing so incorrectly.

> What do you think about my other idea?
> 

With static numbering of gpios you end up having cross-instance and
cross-driver tweaks to make things fit the number space. In particular
when you combine different gpio chips in different ways for different
devices this becomes a mess.

That's why the idea of static gpio numbering was abandoned a long long
time ago. So while it does solve an immediate problem for you it is
proven not to be the right solution in the long run...

Regards,
Bjorn

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Linus Walleij]

On Mon, Jan 29, 2018 at 1:51 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

>> > My bad, this should have been -1.
>>
>> Perhaps, but it's been 0 for a very long time, so I don't want to break any
>> existing platforms by suddenly relocating all GPIOs across all Qualcomm
>> platforms.
>>
>
> Yeah, I see that I got this wrong when I wrote the driver 4 years ago...
>
> There should be no in-kernel users depending on these numbers being hard
> coded, so anyone depending on these numbers starting at 0 would be user
> space - doing so incorrectly.

There is a good point in what Tmur is saying here. We never break
userspace, if we can avoid it.

If there is a real problem with setting this base to -1 then I suggest

if (tlmm_has_only_one_instance)
   base = 0;
else
   base = -1;

Especially for an upcoming platform we can start with a clean slate,
it certainly does not have any legacy userspace.

If no problems are detected, let's just use -1.

Yours,
Linus Walleij

[PATCH] pinctrl: msm: allow the gpio base to be configurable

[Timur Tabi]

On 2/7/18 7:19 AM, Linus Walleij wrote:
> if (tlmm_has_only_one_instance)
>     base = 0;
> else
>     base = -1;

This is effectively what my patch does.  The first instance gets 0.  The 
second instance, if it exists, gets -1.

When the first instance is registered, there's no way to know whether 
there will be a second instance.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.