From: Grygorii Strashko <grygorii.strashko@ti.com> To: Alexander Shiyan <shc_work@mail.ru> Cc: Alexandre Courbot <gnurou@gmail.com>, devicetree@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, linux-gpio@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, santosh.shilimkar@ti.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 4/4] ARM: dts: keystone-k2hk: add dsp gpio controllers nodes Date: Thu, 14 Aug 2014 18:57:08 +0300 [thread overview] Message-ID: <53ECDC54.3060005@ti.com> (raw) In-Reply-To: <1408018321.928492982@f270.i.mail.ru> On 08/14/2014 03:12 PM, Alexander Shiyan wrote: > Thu, 14 Aug 2014 15:13:39 +0300 от Grygorii Strashko <grygorii.strashko@ti.com>: >> Hi Alexander, >> >> On 08/13/2014 07:06 PM, Alexander Shiyan wrote: >>> Wed, 13 Aug 2014 19:16:22 +0300 от Grygorii Strashko <grygorii.strashko@ti.com>: >>>> Add Keystone 2 DSP GPIO nodes. >>>> DSP GPIO banks 0-7 correspond to DSP0-DSP7 >>>> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 56 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi >>>> index 321ba2f..009e180 100644 >>>> --- a/arch/arm/boot/dts/k2hk.dtsi >>>> +++ b/arch/arm/boot/dts/k2hk.dtsi >>>> @@ -50,5 +50,61 @@ >>>> #interrupt-cells = <1>; >>>> ti,syscon-dev = <&devctrl 0x2a0>; >>>> }; >>>> + >>>> + dspgpio0: keystone_dsp_gpio@02620240 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x240>; >>>> + }; >>>> + >>>> + dspgpio1: keystone_dsp_gpio@2620244 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x244>; >>>> + }; >>> ... >>>> + dspgpio7: keystone_dsp_gpio@262025C { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x25c>; >>>> + }; >>> >>> So, devctrl is a syscon device and this DTS introduce several >>> identical GPIO descriptions? >>> >>> On my opinion this should be placed in the gpio-syscon.c, >>> where you can add support for ti,keystone-dsp0{..7}-gpio. >>> Such change will avoid parts 2 and 3 of this patch. >>> >>> static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { >>> .compatible = "ti,keystone-syscon", >>> .dat_bit_offset = 0x240 * 8, >>> ... >>> .set = etc... >>> }; >> >> So, if I understand you right, you propose to add 8 additional compatible >> strings just to encode different register offsets. Is it? >> 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" > > Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. > >> 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c >> (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) >> >> 3) add 8 additional items in array syscon_gpio_ids >> { >> .compatible = "ti,keystone-mctrl-gpio0", >> .data = &keystone_mctrl_gpio0, >> }, ... >> >> I can do it if you strictly insist, BUT I don't like it :( >> - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) >> - as I mentioned in cover letter and commit messages even each SoC revision can have >> different Syscon implementation with different registers offsets and with different >> number of Syscon register ranges (for example for Keystone 2 we already have two >> Syscon devices defined in DT). > > The initial version of this driver contains addresses and offsets in, but this approach has > been criticized by DT maintainers. Could you provide link on this thread^, pls? Curious, it looks like Rob Herring has just proposed to encode offsets & bits in DT: "Re: [PATCH 2/6] leds: add device tree bindings for syscon LEDs" http://www.spinics.net/lists/arm-kernel/msg354182.html > > "different Syscon with different registers" - is OK, since we use compatible string in definition. > If you have two identical syscon, just append an unique additional string and use it in this driver. > >> Now we already have 3 Keystone 2 SoC revisions (K2HK, K2L, K2E) and there are no guarantee >> that the next revision k2X will have the same register offset for GPIO0. > > Then use more exact string for syscon-gpio, like ti,keystone-k2hk-dsp-gpio0. Nothing to say :) - DT is funny thing. It has such good feature as phandle, but people continue hard-coding relation between HW blocks in code :( Any way, Thanks for your comments. I'll wait a bit then update and resend. Best regards, -grygorii -- 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
WARNING: multiple messages have this Message-ID (diff)
From: grygorii.strashko@ti.com (Grygorii Strashko) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 4/4] ARM: dts: keystone-k2hk: add dsp gpio controllers nodes Date: Thu, 14 Aug 2014 18:57:08 +0300 [thread overview] Message-ID: <53ECDC54.3060005@ti.com> (raw) In-Reply-To: <1408018321.928492982@f270.i.mail.ru> On 08/14/2014 03:12 PM, Alexander Shiyan wrote: > Thu, 14 Aug 2014 15:13:39 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: >> Hi Alexander, >> >> On 08/13/2014 07:06 PM, Alexander Shiyan wrote: >>> Wed, 13 Aug 2014 19:16:22 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: >>>> Add Keystone 2 DSP GPIO nodes. >>>> DSP GPIO banks 0-7 correspond to DSP0-DSP7 >>>> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 56 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi >>>> index 321ba2f..009e180 100644 >>>> --- a/arch/arm/boot/dts/k2hk.dtsi >>>> +++ b/arch/arm/boot/dts/k2hk.dtsi >>>> @@ -50,5 +50,61 @@ >>>> #interrupt-cells = <1>; >>>> ti,syscon-dev = <&devctrl 0x2a0>; >>>> }; >>>> + >>>> + dspgpio0: keystone_dsp_gpio at 02620240 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x240>; >>>> + }; >>>> + >>>> + dspgpio1: keystone_dsp_gpio at 2620244 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x244>; >>>> + }; >>> ... >>>> + dspgpio7: keystone_dsp_gpio at 262025C { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x25c>; >>>> + }; >>> >>> So, devctrl is a syscon device and this DTS introduce several >>> identical GPIO descriptions? >>> >>> On my opinion this should be placed in the gpio-syscon.c, >>> where you can add support for ti,keystone-dsp0{..7}-gpio. >>> Such change will avoid parts 2 and 3 of this patch. >>> >>> static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { >>> .compatible = "ti,keystone-syscon", >>> .dat_bit_offset = 0x240 * 8, >>> ... >>> .set = etc... >>> }; >> >> So, if I understand you right, you propose to add 8 additional compatible >> strings just to encode different register offsets. Is it? >> 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" > > Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. > >> 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c >> (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) >> >> 3) add 8 additional items in array syscon_gpio_ids >> { >> .compatible = "ti,keystone-mctrl-gpio0", >> .data = &keystone_mctrl_gpio0, >> }, ... >> >> I can do it if you strictly insist, BUT I don't like it :( >> - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) >> - as I mentioned in cover letter and commit messages even each SoC revision can have >> different Syscon implementation with different registers offsets and with different >> number of Syscon register ranges (for example for Keystone 2 we already have two >> Syscon devices defined in DT). > > The initial version of this driver contains addresses and offsets in, but this approach has > been criticized by DT maintainers. Could you provide link on this thread^, pls? Curious, it looks like Rob Herring has just proposed to encode offsets & bits in DT: "Re: [PATCH 2/6] leds: add device tree bindings for syscon LEDs" http://www.spinics.net/lists/arm-kernel/msg354182.html > > "different Syscon with different registers" - is OK, since we use compatible string in definition. > If you have two identical syscon, just append an unique additional string and use it in this driver. > >> Now we already have 3 Keystone 2 SoC revisions (K2HK, K2L, K2E) and there are no guarantee >> that the next revision k2X will have the same register offset for GPIO0. > > Then use more exact string for syscon-gpio, like ti,keystone-k2hk-dsp-gpio0. Nothing to say :) - DT is funny thing. It has such good feature as phandle, but people continue hard-coding relation between HW blocks in code :( Any way, Thanks for your comments. I'll wait a bit then update and resend. Best regards, -grygorii
next prev parent reply other threads:[~2014-08-14 15:13 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-08-13 16:16 [PATCH 0/4] gpio: syscon: reuse for keystone 2 socs Grygorii Strashko 2014-08-13 16:16 ` Grygorii Strashko 2014-08-13 16:16 ` [PATCH 1/4] gpio: syscon: add soc specific callback to assign output value Grygorii Strashko 2014-08-13 16:16 ` Grygorii Strashko 2014-08-29 6:19 ` Linus Walleij 2014-08-29 6:19 ` Linus Walleij 2014-09-01 14:55 ` Grygorii Strashko 2014-09-01 14:55 ` Grygorii Strashko 2014-08-13 16:16 ` [PATCH 2/4] gpio: syscon: retrive syscon node and regs offsets from dt Grygorii Strashko 2014-08-13 16:16 ` Grygorii Strashko 2014-08-13 16:16 ` [PATCH 3/4] gpio: syscon: reuse for keystone 2 socs Grygorii Strashko 2014-08-13 16:16 ` Grygorii Strashko 2014-08-29 5:53 ` Linus Walleij 2014-08-29 5:53 ` Linus Walleij 2014-08-13 16:16 ` [PATCH 4/4] ARM: dts: keystone-k2hk: add dsp gpio controllers nodes Grygorii Strashko 2014-08-13 16:16 ` Grygorii Strashko 2014-08-13 16:06 ` Alexander Shiyan [not found] ` <1407946582-20927-5-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org> 2014-08-13 16:06 ` Alexander Shiyan 2014-08-13 16:06 ` Alexander Shiyan 2014-08-13 16:06 ` Alexander Shiyan 2014-08-14 12:13 ` Grygorii Strashko 2014-08-14 12:13 ` Grygorii Strashko 2014-08-14 12:12 ` Alexander Shiyan 2014-08-14 12:12 ` Alexander Shiyan 2014-08-14 15:57 ` Grygorii Strashko [this message] 2014-08-14 15:57 ` Grygorii Strashko 2014-08-14 15:26 ` Alexander Shiyan 2014-08-14 15:26 ` Alexander Shiyan 2014-08-14 16:54 ` Grygorii Strashko 2014-08-14 16:54 ` Grygorii Strashko 2014-08-21 16:23 ` [PATCH v2 0/3] gpio: syscon: reuse for keystone 2 socs Grygorii Strashko 2014-08-21 16:23 ` Grygorii Strashko 2014-08-21 16:23 ` [PATCH v2 1/3] gpio: syscon: add soc specific callback to assign output value Grygorii Strashko 2014-08-21 16:23 ` Grygorii Strashko 2014-08-21 16:23 ` [PATCH v2 2/3] gpio: syscon: reuse for keystone 2 socs Grygorii Strashko 2014-08-21 16:23 ` Grygorii Strashko 2014-08-21 16:23 ` [PATCH v2 3/3] ARM: dts: keystone-k2hk: add dsp gpio controllers nodes Grygorii Strashko 2014-08-21 16:23 ` Grygorii Strashko 2014-08-21 16:47 ` Alexander Shiyan 2014-08-21 16:47 ` Alexander Shiyan 2014-08-21 16:47 ` Alexander Shiyan [not found] ` <1408638203-8246-4-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org> 2014-08-21 16:47 ` Alexander Shiyan 2014-08-21 16:51 ` [PATCH v2 0/3] gpio: syscon: reuse for keystone 2 socs Alexander Shiyan 2014-08-21 16:51 ` Alexander Shiyan 2014-08-28 17:32 ` Grygorii Strashko 2014-08-28 17:32 ` Grygorii Strashko [not found] ` <1408638203-8246-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org> 2014-08-21 16:51 ` Alexander Shiyan 2014-08-21 16:51 ` Alexander Shiyan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=53ECDC54.3060005@ti.com \ --to=grygorii.strashko@ti.com \ --cc=devicetree@vger.kernel.org \ --cc=gnurou@gmail.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=robh+dt@kernel.org \ --cc=santosh.shilimkar@ti.com \ --cc=shc_work@mail.ru \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.