| Message ID | 1516915209-28295-1-git-send-email-timur@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: msm: allow the gpio base to be configurable | expand |
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 -- 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 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.
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/ -- 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 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 -- 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 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?
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 -- 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, 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 -- 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 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.
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,
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(-)