Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Johan Jonker <jbx6244@gmail.com>]

On 3/19/23 13:20, Jonas Karlman wrote:
> Hi Johan,
> On 2023-03-19 12:34, Johan Jonker wrote:
>>
>>
>> On 3/18/23 21:20, Simon Glass wrote:
>>> Hi Johan,
>>>
>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@gmail.com> wrote:
>>>>
>>>> The current divider to calculate the bank ID can change.
>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>>
>>
>>> What is the motivation for this patch?
>>
>> The gpio-ranges property format:
>>
>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>                 [pin controller offset], [number of pins]>;
>>
>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
> 
> Is there a reason why gpio-ranges is used to determine bank id?
> 
> In the series at [1], add support for rk35xx gpio banks, sent yesterday
> I changed this to rely on the dev alias id, same as in the linux driver.
>
> Any reason why using the device alias id won't work for the same purpose
> in u-boot?

Aliases are board/user orientated based on availability.
See discussion elsewhere(by Arnd Bergmann and others, see lore link unknown for now)
All aliases should be moved out of the dtsi files, so there's no guaranty that they are there anymore.
Aliases should follow the Rockchip TRM naming to prevent confusion.(by Heiko)
Special rules apply to mmc aliases: based on availability, reg order and without number gap. 

U-Boot/bootloader specific:
Aliases get lost with reduced DT's and are problematic with overlays,etc(ie. it needs special handling on top of that).
Aliases are not in the dtb platdata structure made by dtoc for TPL/SPL.(current drivers don't work yet, but in the future they may)

Best is to have a property inside the node for parsing and reduced nodes.
gpio-ranges is the only option currently available.
(not saying that is perfect, but at least don't invent new properties that needs to be continuously supported as legacy)


> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/
> 
> Regards,
> Jonas
> 
>>
>> ===
>>
>> Theoretical example:
>>
>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>
>> vs.
>>
>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>
>> Both descriptions are valid.
>> The number of pins in the second example is reduced to 16 per item.
>> Using that as divider will give a wrong bank number.
>> Use a constant instead.
>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>
>> From gpio.txt:
>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>> in practice these ranges are often lumped in discrete sets.
>>
>>>
>>>>
>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>>> ---
>>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>>>
>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>> index f7ad4d68..0a2acf18 100644
>>>> --- a/drivers/gpio/rk_gpio.c
>>>> +++ b/drivers/gpio/rk_gpio.c
>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>                                              0, &args);
>>>>         if (!ret || ret != -ENOENT) {
>>>>                 uc_priv->gpio_count = args.args[2];
>>>> -               priv->bank = args.args[1] / args.args[2];
>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>         } else {
>>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>                 end = strrchr(dev->name, '@');
>>>> --
>>>> 2.20.1
>>>>
>>>
>>> Regards,
>>> SImon
>