| Message ID | 20231006125557.212681-1-m.majewski2@samsung.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix Samsung pinctrl driver static allocation of GPIO base warning | expand |
On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote: > > The object of this work is fixing the following warning, which appears > on all targets using that driver: > > gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. > > This needs a small refactor to how we interact with the pinctrl > subsystem. Finally, we remove some bookkeeping that has only been > necessary to allocate GPIO bases correctly. > > Mateusz Majewski (4): > pinctrl: samsung: defer pinctrl_enable > pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges > pinctrl: samsung: choose GPIO numberspace base dynamically > pinctrl: samsung: do not offset pinctrl numberspaces > > drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++----------- > drivers/pinctrl/samsung/pinctrl-samsung.h | 4 +- > 2 files changed, 31 insertions(+), 29 deletions(-) > > -- Hi Mateusz, Thank you for handling this! Those deprecation warnings have been bugging me for some time :) While testing this series on my E850-96 board (Exynos850 based), I noticed some changes in /sys/kernel/debug/gpio file, like these: 8<------------------------------------------------------------------------------------------>8 -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0: - gpio-7 ( |Volume Up ) in hi IRQ ACTIVE LOW +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0: + gpio-519 ( |Volume Up ) in hi IRQ ACTIVE LOW -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1: - gpio-8 ( |Volume Down ) in hi IRQ ACTIVE LOW +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1: + gpio-520 ( |Volume Down ) in hi IRQ ACTIVE LOW -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2: +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2: ... 8<------------------------------------------------------------------------------------------>8 So basically it looks like all line numbers were offset by 512. Can you please comment on this? Is it an intentional change, and why it's happening? Despite of that change, everything seems to be working fine. But I kinda liked the numeration starting from 0 better :) Thanks! > 2.42.0 >
On 07/10/2023 04:14, Sam Protsenko wrote: > On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote: >> >> The object of this work is fixing the following warning, which appears >> on all targets using that driver: >> >> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. >> >> This needs a small refactor to how we interact with the pinctrl >> subsystem. Finally, we remove some bookkeeping that has only been >> necessary to allocate GPIO bases correctly. >> >> Mateusz Majewski (4): >> pinctrl: samsung: defer pinctrl_enable >> pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges >> pinctrl: samsung: choose GPIO numberspace base dynamically >> pinctrl: samsung: do not offset pinctrl numberspaces >> >> drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++----------- >> drivers/pinctrl/samsung/pinctrl-samsung.h | 4 +- >> 2 files changed, 31 insertions(+), 29 deletions(-) >> >> -- > > Hi Mateusz, > > Thank you for handling this! Those deprecation warnings have been > bugging me for some time :) While testing this series on my E850-96 > board (Exynos850 based), I noticed some changes in > /sys/kernel/debug/gpio file, like these: > > 8<------------------------------------------------------------------------------------------>8 > -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0: > - gpio-7 ( |Volume Up ) in hi IRQ ACTIVE LOW > +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0: > + gpio-519 ( |Volume Up ) in hi IRQ ACTIVE LOW > > -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1: > - gpio-8 ( |Volume Down ) in hi IRQ ACTIVE LOW > +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1: > + gpio-520 ( |Volume Down ) in hi IRQ ACTIVE LOW > > -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2: > +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2: > > ... > 8<------------------------------------------------------------------------------------------>8 > > So basically it looks like all line numbers were offset by 512. Can > you please comment on this? Is it an intentional change, and why it's > happening? > > Despite of that change, everything seems to be working fine. But I > kinda liked the numeration starting from 0 better :) Could it be the reason of dynamic allocation? Best regards, Krzysztof
On Sun, Oct 8, 2023 at 8:09 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 07/10/2023 04:14, Sam Protsenko wrote: > > On Fri, Oct 6, 2023 at 8:01 AM Mateusz Majewski <m.majewski2@samsung.com> wrote: > >> > >> The object of this work is fixing the following warning, which appears > >> on all targets using that driver: > >> > >> gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. > >> > >> This needs a small refactor to how we interact with the pinctrl > >> subsystem. Finally, we remove some bookkeeping that has only been > >> necessary to allocate GPIO bases correctly. > >> > >> Mateusz Majewski (4): > >> pinctrl: samsung: defer pinctrl_enable > >> pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges > >> pinctrl: samsung: choose GPIO numberspace base dynamically > >> pinctrl: samsung: do not offset pinctrl numberspaces > >> > >> drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++----------- > >> drivers/pinctrl/samsung/pinctrl-samsung.h | 4 +- > >> 2 files changed, 31 insertions(+), 29 deletions(-) > >> > >> -- > > > > Hi Mateusz, > > > > Thank you for handling this! Those deprecation warnings have been > > bugging me for some time :) While testing this series on my E850-96 > > board (Exynos850 based), I noticed some changes in > > /sys/kernel/debug/gpio file, like these: > > > > 8<------------------------------------------------------------------------------------------>8 > > -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0: > > - gpio-7 ( |Volume Up ) in hi IRQ ACTIVE LOW > > +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0: > > + gpio-519 ( |Volume Up ) in hi IRQ ACTIVE LOW > > > > -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1: > > - gpio-8 ( |Volume Down ) in hi IRQ ACTIVE LOW > > +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1: > > + gpio-520 ( |Volume Down ) in hi IRQ ACTIVE LOW > > > > -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2: > > +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2: > > > > ... > > 8<------------------------------------------------------------------------------------------>8 > > > > So basically it looks like all line numbers were offset by 512. Can > > you please comment on this? Is it an intentional change, and why it's > > happening? > > > > Despite of that change, everything seems to be working fine. But I > > kinda liked the numeration starting from 0 better :) > > Could it be the reason of dynamic allocation? > I just asked because I didn't know :) But ok, if you want me to do some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not necessarily the reason of dynamic allocation, but instead just a way to keep 0-512 range for legacy GPIO drivers which might use that area to allocate GPIO numbers statically. It's mentioned here: /* * At the end we want all GPIOs to be dynamically allocated from 0. * However, some legacy drivers still perform fixed allocation. * Until they are all fixed, leave 0-512 space for them. */ #define GPIO_DYNAMIC_BASE 512 As mentioned in another comment in gpiochip_add_data_with_key(), that numberspace shouldn't matter and in the end should go away, as GPIO sysfs interface is pretty much deprecated at this point, and everybody should stick to GPIO descriptors. Anyway, now that it's clear that the base number change was intended and shouldn't matter, for all patches in the series: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> Tested-by: Sam Protsenko <semen.protsenko@linaro.org> Thanks! > > Best regards, > Krzysztof >
On 08/10/2023 20:45, Sam Protsenko wrote: >>> >>> Thank you for handling this! Those deprecation warnings have been >>> bugging me for some time :) While testing this series on my E850-96 >>> board (Exynos850 based), I noticed some changes in >>> /sys/kernel/debug/gpio file, like these: >>> >>> 8<------------------------------------------------------------------------------------------>8 >>> -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0: >>> - gpio-7 ( |Volume Up ) in hi IRQ ACTIVE LOW >>> +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0: >>> + gpio-519 ( |Volume Up ) in hi IRQ ACTIVE LOW >>> >>> -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1: >>> - gpio-8 ( |Volume Down ) in hi IRQ ACTIVE LOW >>> +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1: >>> + gpio-520 ( |Volume Down ) in hi IRQ ACTIVE LOW >>> >>> -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2: >>> +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2: >>> >>> ... >>> 8<------------------------------------------------------------------------------------------>8 >>> >>> So basically it looks like all line numbers were offset by 512. Can >>> you please comment on this? Is it an intentional change, and why it's >>> happening? >>> >>> Despite of that change, everything seems to be working fine. But I >>> kinda liked the numeration starting from 0 better :) >> >> Could it be the reason of dynamic allocation? >> > > I just asked because I didn't know :) But ok, if you want me to do > some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not > necessarily the reason of dynamic allocation, but instead just a way > to keep 0-512 range for legacy GPIO drivers which might use that area > to allocate GPIO numbers statically. It's mentioned here: > > /* > * At the end we want all GPIOs to be dynamically allocated from 0. > * However, some legacy drivers still perform fixed allocation. > * Until they are all fixed, leave 0-512 space for them. > */ > #define GPIO_DYNAMIC_BASE 512 > > As mentioned in another comment in gpiochip_add_data_with_key(), that > numberspace shouldn't matter and in the end should go away, as GPIO > sysfs interface is pretty much deprecated at this point, and everybody > should stick to GPIO descriptors. > > Anyway, now that it's clear that the base number change was intended > and shouldn't matter, for all patches in the series: > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Tested-by: Sam Protsenko <semen.protsenko@linaro.org> If all the GPIOs changed due to switch to dynamic allocation, aren't we breaking all user-space users? Best regards, Krzysztof
On 09.10.2023 11:42, Krzysztof Kozlowski wrote: > On 08/10/2023 20:45, Sam Protsenko wrote: >>>> Thank you for handling this! Those deprecation warnings have been >>>> bugging me for some time :) While testing this series on my E850-96 >>>> board (Exynos850 based), I noticed some changes in >>>> /sys/kernel/debug/gpio file, like these: >>>> >>>> 8<------------------------------------------------------------------------------------------>8 >>>> -gpiochip0: GPIOs 0-7, parent: platform/11850000.pinctrl, gpa0: >>>> - gpio-7 ( |Volume Up ) in hi IRQ ACTIVE LOW >>>> +gpiochip0: GPIOs 512-519, parent: platform/11850000.pinctrl, gpa0: >>>> + gpio-519 ( |Volume Up ) in hi IRQ ACTIVE LOW >>>> >>>> -gpiochip1: GPIOs 8-15, parent: platform/11850000.pinctrl, gpa1: >>>> - gpio-8 ( |Volume Down ) in hi IRQ ACTIVE LOW >>>> +gpiochip1: GPIOs 520-527, parent: platform/11850000.pinctrl, gpa1: >>>> + gpio-520 ( |Volume Down ) in hi IRQ ACTIVE LOW >>>> >>>> -gpiochip2: GPIOs 16-23, parent: platform/11850000.pinctrl, gpa2: >>>> +gpiochip2: GPIOs 528-535, parent: platform/11850000.pinctrl, gpa2: >>>> >>>> ... >>>> 8<------------------------------------------------------------------------------------------>8 >>>> >>>> So basically it looks like all line numbers were offset by 512. Can >>>> you please comment on this? Is it an intentional change, and why it's >>>> happening? >>>> >>>> Despite of that change, everything seems to be working fine. But I >>>> kinda liked the numeration starting from 0 better :) >>> Could it be the reason of dynamic allocation? >>> >> I just asked because I didn't know :) But ok, if you want me to do >> some digging... It seems like having GPIO_DYNAMIC_BASE=512 is not >> necessarily the reason of dynamic allocation, but instead just a way >> to keep 0-512 range for legacy GPIO drivers which might use that area >> to allocate GPIO numbers statically. It's mentioned here: >> >> /* >> * At the end we want all GPIOs to be dynamically allocated from 0. >> * However, some legacy drivers still perform fixed allocation. >> * Until they are all fixed, leave 0-512 space for them. >> */ >> #define GPIO_DYNAMIC_BASE 512 >> >> As mentioned in another comment in gpiochip_add_data_with_key(), that >> numberspace shouldn't matter and in the end should go away, as GPIO >> sysfs interface is pretty much deprecated at this point, and everybody >> should stick to GPIO descriptors. >> >> Anyway, now that it's clear that the base number change was intended >> and shouldn't matter, for all patches in the series: >> >> Reviewed-by: Sam Protsenko<semen.protsenko@linaro.org> >> Tested-by: Sam Protsenko<semen.protsenko@linaro.org> > If all the GPIOs changed due to switch to dynamic allocation, aren't we > breaking all user-space users? This /sys based GPIO interface is deprecated, so I don't think that stable numbers is something that we should care. Userspace, if still uses /sys interface, should depend on the GPIO bank name. I remember that the GPIO numbers varied between different kernel versions (also compared to the 'vendor kernels'), although I don't remember if this was Exynos related case or other. Best regards
On 06.10.2023 14:55, Mateusz Majewski wrote: > The object of this work is fixing the following warning, which appears > on all targets using that driver: > > gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. > > This needs a small refactor to how we interact with the pinctrl > subsystem. Finally, we remove some bookkeeping that has only been > necessary to allocate GPIO bases correctly. > > Mateusz Majewski (4): > pinctrl: samsung: defer pinctrl_enable > pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges > pinctrl: samsung: choose GPIO numberspace base dynamically > pinctrl: samsung: do not offset pinctrl numberspaces > > drivers/pinctrl/samsung/pinctrl-samsung.c | 56 ++++++++++++----------- > drivers/pinctrl/samsung/pinctrl-samsung.h | 4 +- > 2 files changed, 31 insertions(+), 29 deletions(-) Just to let everyone know - I've tested this patchset on our test farm and found no regressions on various Exynos based boards. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Best regards
On Fri, 06 Oct 2023 14:55:53 +0200, Mateusz Majewski wrote: > The object of this work is fixing the following warning, which appears > on all targets using that driver: > > gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. > > This needs a small refactor to how we interact with the pinctrl > subsystem. Finally, we remove some bookkeeping that has only been > necessary to allocate GPIO bases correctly. > > [...] Applied, thanks! [1/4] pinctrl: samsung: defer pinctrl_enable https://git.kernel.org/pinctrl/samsung/c/2aca5c591ef4ecc4bcb9be3c9a9360d3d5238866 [2/4] pinctrl: samsung: use add_pin_ranges method to add pinctrl ranges https://git.kernel.org/pinctrl/samsung/c/bf128c1f0fe1fd4801fb84660c324095990c533a [3/4] pinctrl: samsung: choose GPIO numberspace base dynamically https://git.kernel.org/pinctrl/samsung/c/deb79167e1dadc0ac0a9e3aa67130e60c5d011ef [4/4] pinctrl: samsung: do not offset pinctrl numberspaces https://git.kernel.org/pinctrl/samsung/c/8aec97decfd0f444a69a765b2f00d64b42752824 Best regards,
Hi Mateusz, On Fri, Oct 6, 2023 at 3:00 PM Mateusz Majewski <m.majewski2@samsung.com> wrote: > The object of this work is fixing the following warning, which appears > on all targets using that driver: > > gpio gpiochip0: Static allocation of GPIO base is deprecated, use dynamic allocation. > > This needs a small refactor to how we interact with the pinctrl > subsystem. Finally, we remove some bookkeeping that has only been > necessary to allocate GPIO bases correctly. I see that Krzysztof has already taken care of this series so I just wait for a pull request (some days work is a bliss, thanks Krzysztof!) Yours, Linus Walleij