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

[Stefan Herbrechtsmeier]

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.

Regards
   Stefan