[U-Boot] [RFC PATCH] gpio: zynq: Setup bank_name to dev->name

[Michal Simek]

On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
> Hi Michal,
> 
> Am 25.07.2018 um 08:07 schrieb Michal Simek:
>> On 24.7.2018 21:39, Stefan Herbrechtsmeier wrote:
>>> Am 24.07.2018 um 10:37 schrieb Michal Simek:
>>>> On 23.7.2018 20:29, Stefan Herbrechtsmeier wrote:
>>>>> Am 23.07.2018 um 11:08 schrieb Michal Simek:
>>>>>> On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote:
>>>>>>> Am 12.07.2018 um 16:04 schrieb Michal Simek:
>>>>>>>> There should be proper bank name setup to distiguish between
>>>>>>>> different
>>>>>>>> gpio drivers. Use dev->name for it.
>>>>>>>>
>>>>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/gpio/zynq_gpio.c | 2 ++
>>>>>>>>      1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
>>>>>>>> index 26f69b1a713f..f793ee5754a8 100644
>>>>>>>> --- a/drivers/gpio/zynq_gpio.c
>>>>>>>> +++ b/drivers/gpio/zynq_gpio.c
>>>>>>>> @@ -337,6 +337,8 @@ static int zynq_gpio_probe(struct udevice *dev)
>>>>>>>>          struct zynq_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>          struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>>>>>>>      +    uc_priv->bank_name = dev->name;
>>>>>>>> +
>>>>>>>>          if (priv->p_data)
>>>>>>>>              uc_priv->gpio_count = priv->p_data->ngpio;
>>>>>>>>      
>>>>>>> Does this not lead to ugly names because the gpio number is
>>>>>>> append to
>>>>>>> the bank_name? Have you check the "gpio status -a" output?
>>>>>> Yes I was checking it. Names are composed together but also just
>>>>>> numbers
>>>>>> works as before.
>>>>>>
>>>>>> gpio at ff0a00000: input: 0 [ ]
>>>>>> gpio at ff0a00001: input: 0 [ ]
>>>>>> gpio at ff0a00002: input: 0 [ ]
>>>>>> gpio at ff0a00003: input: 0 [ ]
>>>>>> gpio at ff0a00004: input: 0 [ ]
>>>>>> gpio at ff0a00005: input: 0 [ ]
>>>>>> gpio at ff0a00006: input: 0 [ ]
>>>>>> gpio at ff0a00007: input: 0 [ ]
>>>>>> gpio at ff0a00008: input: 0 [ ]
>>>>>> gpio at ff0a00009: input: 0 [ ]
>>>>> Do you think that this are meaningful names? It isn't possible to
>>>>> separate the device and pin number as well as it mix hex and decimal
>>>>> numbers.
>>>>>
>>>>>> If you know better way how to setup a bank name please let me know
>>>>>> but I
>>>>>> need to distinguish ps gpio from pl one and for pl we need to know
>>>>>> the
>>>>>> address.
>>>>> I know the use case.
>>>>>
>>>>> A lot of drivers use the bank_name from the device tree, some drivers
>>>>> append an underscore to the bank name and others add the req_seq of
>>>>> the
>>>>> device to an alphabetic character.
>>>>>
>>>>>>> Other drivers use the gpio-bank-name from the device tree.
>>>>>> I can't see this property inside Linux kernel. If this has been
>>>>>> reviewed
>>>>>> by dt guys please let me know.
>>>>> This property is only used by u-boot. I think it isn't needed by the
>>>>> Linux kernel.
>>>> I am happy to use consistent solution but what's that?
>>> Consistent solution between what?
>> all drivers. Name should be composed consistently among all drivers.
>> It means drivers shouldn't add additional "_" in driver code for example.
>>
>>>> Mixing name with hex and int is not nice but adding "_" or something
>>>> else is just a pain in driver code. If this is done in core I am fine
>>>> with that but adding this code to all drivers don't look like generic
>>>> solution at all.
>>> Normally the bank name is an alphabetic character or string. Maybe we
>>> could add the device name to the gpio_lookup_name function and add an
>>> additional optional device name parameter to the gpio command.
>>>
>>>> Using additional u-boot property is not good too.
>>>>
>>>> I have mentioned in "gpio: xilinx: Convert driver to DM"
>>>> (sha1:10441ec9224d0d269dc512819a32c0785a6338d3)
>>>> that uc-priv->name is completely unused. Maybe this should be dev->name
>>>> and bank_name should be really used for banks.
>>> Isn't the uc-priv->name used for the label of the request?
>> Last time when I looked at it and I didn't see this name listed anywhere
>> in output.
>>
>>
>>>> Then in gpio status -a can be
>>>>
>>>> Device gpio at a0001000:
>>>> Bank:
>>>> ...
>>>>
>>>> but not sure how gpio commands will work to address exact pin from
>>>> prompt. Because this is normally working
>>>> gpio toggle gpio at a00010001
>>> With an optional device name this would be:
>>> gpio toggle gpio at a0001000 1
>>>
>>> Alternative the gpio command could support the requested labels:
>>> gpio toggle second-gpio
>> I am open to see this solution. Feel free to invest your time support
>> this but I have no time for that.
> 
> What does this mean?
> 
> I understand that you don't have the time to develop a new common solution.
> 
> But the discussion disappoints me. You misuse the bank name in a poor
> way and decline alternative solutions with additional requirements even
> if they are already used in u-boot.
> 
> The name "gpio at a000100011" for pin 11 of the device gpio at a0001000 isn't
> consistent between the u-boot drivers nor is the name used in Linux. A
> bank-name from the device tree is used by several u-boot drivers even if
> it isn't consistent between the drivers.

I am sorry that you feel like that. It is not about declining
alternative solution. I want to know what's the right solution is.

Using bank-name/gpio-bank-name via DT is something what is IMHO not
correct.
The first thing is if this is used just by u-boot it should have at
least u-boot prefix. It means u-boot,bank-name, u-boot,gpio-bank-name.
(Even I am not sure if u-boot prefix is properly allocated and can be
allocated via sort of vendor-prefix).

The second thing is that I don't think it is good to have two different
dts files. One in the kernel and second in u-boot. Even I know we have
problem with that but we are trying to sync it as much as possible.

Regarding others options:

at91_gpio: plat->bank_name - which is not filled for OF case. (2 chars
via platdata)
da8xx - plat->port_name - which is not filled for OF case and also no user

altera_pio, hsdk, msm, pm8916, sandbox: gpio-bank-name
intel_broadwell, intel_ich6: bank-name:

pcf8575 - gpio-bank-name or fdt_get_name

atmel_pio4, s5p, vybrid: fdt_get_name
bcm6345, rcar: dev->name


hi6220, imx, mxc, omap: "GPIO%d_" plat->bank_index or banknum
mpc8xxx: "MPC@%lx_" data->addr
pca953x: "%s@%x_", label, info->addr or "gpio@%x_", info->addr
axp_gpio : AXP0- hardcoded - "-" prefix
bcm2835 - GPIO - without anything
sunxi: PA + bank
tegra, tegra186:  2char names via list
mvebu: 'A' + dev->req_seq
pic32, rk: 'A' + bank + some ligc around dev->name

stm32f7: st,bank-name

###################################################################
Sum:
2 are not OF
1 is using one prefix (st)
7,5 are using gpio-bank-name or bank-name
5.5 are using dev->name (+2 xilinx which are not listed now)
14 are using own prefixes - made or hardcoded
	6 of them are ending with _
	1 ends with -
	7 don't end with - or _

Cases with gpio-bank-name, bank-name I have described above.

In case of "_" or "-" suffix Bank name will be listed with this suffix
which also doesn't look good. GPIO names below with appended number
looks good.

ZynqMP> gpio status -a
Bank GPIO_: <================ HERE
GPIO_0: input: 0 [ ]
GPIO_1: input: 0 [ ]
GPIO_2: input: 0 [ ]
GPIO_3: input: 0 [ ]
GPIO_4: input: 0 [ ]
GPIO_5: input: 0 [ ]
GPIO_6: input: 0 [ ]
GPIO_7: input: 0 [ ]
GPIO_8: input: 0 [ ]

And I hope it is clear that I can't make this bank_name empty or we will
end up with this when PL gpios are included which is total mess.

172: input: 0 [ ]
173: output: 0 [ ]
0: input: 0 [ ]
1: input: 0 [ ]
2: input: 0 [ ]
3: input: 0 [ ]
4: input: 0 [ ]
5: input: 1 [ ]
6: input: 1 [ ]
7: input: 0 [ ]
8: input: 0 [ ]
9: input: 0 [ ]
10: input: 0 [ ]
11: input: 1 [ ]
12: input: 1 [ ]
0: output: 0 [ ]
1: output: 0 [ ]
2: output: 0 [ ]

It means what I have used and send patch for is used in 5,5 other cases
and they could have the same issue which we are talking about.

If you think that we should append _ in the driver then I would expect
that we should also remove _ it from name when Bank XXX_ is listed.

Thanks,
Michal