| Message ID | 1516915209-28295-1-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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/
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
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
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
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(-)