On 08/14/2014 06:26 PM, Alexander Shiyan wrote:
> Thu, 14 Aug 2014 18:57:08 +0300 от Grygorii Strashko
> <[email protected]>:
> ...
>>>>>> + 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?
>
> Here is a link to first version:
> https://www.mail-archive.com/[email protected]/msg01616.html
10x
>
> Unfortunately, I lost thread for DT-related stuffs.
>
Oh, I've just checked ARM dts directory and found that
constructions like this:
vendor,syscon = <&syscon_phandle>; /// or even this vendor,syscon =
<&syscon_phandle 0x60 2>;
are widely used as of now.
Also, According to the bindings doc, the Syscon is not a Linux specific
definition (mfd/syscon.txt).
Regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
