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

[Stefan Herbrechtsmeier]

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?

> 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?

> 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

Best regards
   Stefan