[U-Boot] [PATCH v3 3/5] ARM: dm: mxs: gpio: Add support for DM/DTS in the mxs_gpio.c driver (DM_GPIO)

[Marek Vasut]

On 6/17/19 3:01 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/17/19 10:37 AM, Lukasz Majewski wrote:
>>> Hi Marek,
>>>   
>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:  
>>>>> This commit    
>>>>
>>>> This is not a commit, it's a change. It only becomes a commit when
>>>> applied.
>>>>  
>>>>> adds support for DM in the mxs_gpio.c driver when DM_GPIO
>>>>> is enabled in Kconfig.    
>>>>
>>>> But this also adds support for DT probing, which is orthogonal to
>>>> DM support, yet it's not documented in the commit message.  
>>>
>>> Ok. Will rewrite the commit message to reflect the changes in the
>>> commit.
>>>   
>>>>  
>>>>> Signed-off-by: Lukasz Majewski <lukma at denx.de>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Set more apropriate tags
>>>>>
>>>>> Changes in v2:
>>>>> - Use #if !CONFIG_IS_ENABLED(DM_GPIO) instead of plain #ifdef
>>>>> CONFIG_DM_GPIO
>>>>>
>>>>>  arch/arm/include/asm/arch-mxs/gpio.h |  28 +++++++
>>>>>  drivers/gpio/mxs_gpio.c              | 149
>>>>> +++++++++++++++++++++++++++++++++++ 2 files changed, 177
>>>>> insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/arch-mxs/gpio.h
>>>>> b/arch/arm/include/asm/arch-mxs/gpio.h index
>>>>> 34fa421945..0c152e25e2 100644 ---
>>>>> a/arch/arm/include/asm/arch-mxs/gpio.h +++
>>>>> b/arch/arm/include/asm/arch-mxs/gpio.h @@ -15,4 +15,32 @@ void
>>>>> mxs_gpio_init(void); inline void mxs_gpio_init(void) {}
>>>>>  #endif
>>>>>  
>>>>> +#if defined(CONFIG_MX28) && CONFIG_IS_ENABLED(DM_GPIO)
>>>>> +/*
>>>>> + * According to i.MX28 Reference Manual:
>>>>> + * 'i.MX28 Applications Processor Reference Manual, Rev. 1, 2010'
>>>>> + * The i.MX28 has following number of GPIOs available:
>>>>> + * Bank 0: 0-28 -> 29 PINS
>>>>> + * Bank 1: 0-31 -> 32 PINS
>>>>> + * Bank 2: 0-27 -> 28 PINS
>>>>> + * Bank 3: 0-30 -> 31 PINS
>>>>> + * Bank 4: 0-20 -> 21 PINS
>>>>> + */
>>>>> +#define IMX28_GPIO_BANK0_PIN_NR 29
>>>>> +#define IMX28_GPIO_BANK1_PIN_NR 32
>>>>> +#define IMX28_GPIO_BANK2_PIN_NR 28
>>>>> +#define IMX28_GPIO_BANK3_PIN_NR 31
>>>>> +#define IMX28_GPIO_BANK4_PIN_NR 21
>>>>> +#define IMX28_GPIO_BANK_NR 5    
>>>>
>>>> Please make a complete conversion, not partial one.
>>>> You want to use gpio-ranges in DT and then parse the size of each
>>>> GPIO bank from those gpio-ranges , not hard-code it into the
>>>> driver.  
>>>
>>> I would have used the gpio-ranges, but the original Linux code [1]
>>> (imx28.dtsi) doesn't define them.  
>>
>> You can add them to imx28-u-boot.dtsi , 
> 
> No, IMHO this is not the best solution. My point is:
> 
> 1. i.MX28 driver in Linux is stable and it works (without gpio-ranges).
> I do not have any interest in fixing code which is already working. If
> that were new driver then no issue to use gpio-ranges from the outset.
> Also if Linux kernel driver would support it - then also no problems
> with adding it.

Linux is continuously improving, so is U-Boot, code is being constantly
rewritten and improved. So this isn't really a convincing argument.

> 2. The proposed code is small - only 24 LOC and doesn't require any
> extra parsing of DTS (including pinctrl driver's properties).
> 
> Why shall I make the driver more verbose if I can avoid it?

It also adds churn into the driver which does not have to be there. In
fact, DT is a hardware description, it should describe hardware and it
has facilities to do so -- gpio-ranges . Will you keep adding more and
more new macros into the code with every new/old iteration of this GPIO
block ?

If you were to put that information into DT, where it belongs, the
driver would be much simple(r) and wouldn't grow unnecessarily.

> 3. It is easily reusable in SPL.

And with gpio-ranges it's not ?

>> and submit patch for mainline
>> Linux, it's easy.
> 
> Submitting patches to Linux is easy - but to have them already accepted
> and pulled is not :-).

Linux GPIO maintainer is real friendly.

>>> Also, the dts files which include [1] don't extend original gpio
>>> nodes to have one.
>>>
>>> As it is not available in the Linux kernel, I don't see any good
>>> reason to add the gpio-ranges to U-Boot. The same approach, as
>>> presented here, is used in zynq_gpio.c file (which also uses
>>> DM/DTS).
>>>
>>> Adding per u-boot property - like 'u-boot,mx28-gpio-ranges' is also
>>> less appealing than 24 lines of code, which can be then easily
>>> re-used with OF_PLATDATA (without DM/DTS suport) in SPL (u-boot.sb
>>> to be precise).  
>>
>> It is well possible the zynq DTs predate the gpio-ranges .
> 
> No, it is not.
> 
>> Read the
>> documentation for it at [2] . It even explicitly states it's used for
>> interaction between pincontrol and gpio controllers.
> 
> In those cases where both support it. The i.MX28 Linux GPIO driver is
> NOT supporting gpio-ranges.

See above -- add support for it and it'd even simplify the driver.

>>
>> [2]
>> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L239
>>
>>>>> +struct mxs_gpio_platdata {
>>>>> +	u32 gpio_nr_of_bank_pins[IMX28_GPIO_BANK_NR];
>>>>> +	u32 gpio_base_nr[IMX28_GPIO_BANK_NR];
>>>>> +	u32 ngpio;
>>>>> +};
>>>>> +
>>>>> +extern const struct mxs_gpio_platdata mxs_gpio_def;
>>>>> +#define IMX_GPIO_NR(port, index)
>>>>> (mxs_gpio_def.gpio_base_nr[(port)] + (index))    
>>>>
>>>> So this should be completely unnecessary when using the DM GPIO,
>>>> this was only needed for non-DM-GPIO .  
>>>
>>> This is a compatibility layer - for some reason ALL iMX ports
>>> define it
>>> - i.e. imx8, imx7 - which shall use DM/DTS for gpio from the outset.
>>>
>>> With the in-board code the dm_gpio_* set of functions shall (and
>>> will) be used (as done in opos6ul.c file).  
>>
>> Then use them and drop this.
> 
> I will use the new dm_gpio_* API where applicable. However, to be in
> sync with other iMX ports the IMX_GPIO_NR() shall be also defined.
Are there any users which will actually have to use the old stuff ? If
not, just don't add it.

[...]

-- 
Best regards,
Marek Vasut