On 08/14/2014 03:12 PM, Alexander Shiyan wrote:

Thu, 14 Aug 2014 15:13:39 +0300 от Grygorii Strashko [off-list ref]:

quoted

Hi Alexander,

On 08/13/2014 07:06 PM, Alexander Shiyan wrote:

quoted

Wed, 13 Aug 2014 19:16:22 +0300 от Grygorii Strashko [off-list ref]:

quoted

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>;
+		};

...

quoted

+		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.

quoted

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.

quoted

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