pinctrl: msm: allow the gpio base to be configurable

PATCH New Timur Tabi 4 participants 9 messages Jan 25, 2018 – Feb 7, 2018

Subsystems (5)

Arm Qualcomm mailing list GPIO Pin control Pin controller - Qualcomm +1 more subsystems

lore

Share & tools

lore

Timur Tabi

author

2018-01-25 21:20 UTC #0 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

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 <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 2 +-
 drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

drivers/pinctrl/qcom/pinctrl-msm.c +1 -1

index b7b6849625ec..4dc76e15bd14 100644
@@ -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;

drivers/pinctrl/qcom/pinctrl-msm.h +2 -0

index 9b9feea540ff..cab26f99011d 100644
@@ -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,

Signature · 3 lines

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.

Linus Walleij

2018-01-26 13:01 UTC #1 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Hi Timur,

thanks for the patch!

Quoted 4 lines · 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.

Sorry, but NACK.

Quoted 4 lines

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.

Quoted 1 line

gpiochip_find_base: found new base at 437

(...)

Quoted 1 line

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

Timur Tabi

author

2018-01-26 13:16 UTC #2 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Quoted 4 lines · Linus Walleij

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.

Quoted 9 lines

Quoted 1 line
gpiochip_find_base: found new base at 437
(...)
Quoted 1 line
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

Quoted 1 line

/sys/class/gpio/export", so I guess that's not the right way to do

things.

Signature · 3 lines

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.

Bartosz Golaszewski

2018-01-26 22:13 UTC #3 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

2018-01-26 14:16 GMT+01:00 Timur Tabi <[email protected]>:

Quoted 32 lines · Linus Walleij

Quoted 5 lines
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.
Quoted 8 lines
Quoted 1 line
gpiochip_find_base: found new base at 437
(...)
Quoted 2 lines
gpiochip_find_base: found new base at 362
These are awesome bases, just beautiful. Use this.

Quoted 9 lines

Quoted 4 lines
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/

Bjorn Andersson

2018-01-28 23:23 UTC #4 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

In reply to Timur Tabi

Quoted 9 lines · 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:

What platform has multiple TLMMs?

[..]

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 904

Quoted 9 lines

drivers/pinctrl/qcom/pinctrl-msm.c +0 -1

index b7b6849625ec..4dc76e15bd14 100644
@@ -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

Timur Tabi

author

2018-01-28 23:29 UTC #5 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Quoted 3 lines · Bjorn Andersson

What platform has multiple TLMMs?
[..]

An upcoming one.

Quoted 11 lines

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 904
Quoted 9 lines
drivers/pinctrl/qcom/pinctrl-msm.c +0 -1
index b7b6849625ec..4dc76e15bd14 100644
@@ -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?

Signature · 3 lines

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.

Bjorn Andersson

2018-01-29 00:51 UTC #6 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Quoted 7 lines · Timur Tabi

Quoted 3 lines · Bjorn Andersson
What platform has multiple TLMMs?
[..]
An upcoming one.

Cool :)

Quoted 11 lines

Quoted 10 lines
drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 903
Quoted 8 lines
drivers/pinctrl/qcom/pinctrl-msm.c +0 -1
index b7b6849625ec..4dc76e15bd14 100644
@@ -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.

Quoted 4 lines

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.

Quoted 2 lines

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

Linus Walleij

2018-02-07 13:19 UTC #7 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Quoted 7 lines · Bjorn Andersson

Quoted 6 lines
Quoted 1 line
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.

Quoted 5 lines

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

Timur Tabi

author

2018-02-07 14:50 UTC #8 lore raw

PATCH 1/1 pinctrl: msm: allow the gpio base to be configurable

Quoted 4 lines · Linus Walleij

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.

Signature · 3 lines

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.