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

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@xilinx.com>
---

 drivers/gpio/zynq_gpio.c | 2 ++
 1 file changed, 2 insertions(+)

On 12 July 2018 at 08:04, Michal Simek <michal.simek@xilinx.com> wrote:
> 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@xilinx.com>
> ---
>
>  drivers/gpio/zynq_gpio.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Normally these would be named A, B, C, etc. Is there no such naming
convention on zynq?

Regards,
Simon

On 16.7.2018 07:19, Simon Glass wrote:
> On 12 July 2018 at 08:04, Michal Simek <michal.simek@xilinx.com> wrote:
>> 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@xilinx.com>
>> ---
>>
>>  drivers/gpio/zynq_gpio.c | 2 ++
>>  1 file changed, 2 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Normally these would be named A, B, C, etc. Is there no such naming
> convention on zynq?

PS(Hard) part has only one gpio controller with several banks with are
using the same address space. We are using from the beginning flat
number scheme that's why only one name is used for all banks.

Thanks,
Michal

Hi Michal,

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

Other drivers use the gpio-bank-name from the device tree.

Best regards
   Stefan

On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote:
> Hi Michal,
> 
> 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@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@ff0a00000: input: 0 [ ]
gpio@ff0a00001: input: 0 [ ]
gpio@ff0a00002: input: 0 [ ]
gpio@ff0a00003: input: 0 [ ]
gpio@ff0a00004: input: 0 [ ]
gpio@ff0a00005: input: 0 [ ]
gpio@ff0a00006: input: 0 [ ]
gpio@ff0a00007: input: 0 [ ]
gpio@ff0a00008: input: 0 [ ]
gpio@ff0a00009: input: 0 [ ]

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.

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

Thanks,
Michal

Hi Michal,


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@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@ff0a00000: input: 0 [ ]
> gpio@ff0a00001: input: 0 [ ]
> gpio@ff0a00002: input: 0 [ ]
> gpio@ff0a00003: input: 0 [ ]
> gpio@ff0a00004: input: 0 [ ]
> gpio@ff0a00005: input: 0 [ ]
> gpio@ff0a00006: input: 0 [ ]
> gpio@ff0a00007: input: 0 [ ]
> gpio@ff0a00008: input: 0 [ ]
> gpio@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.

Best regards
   Stefan

Hi Michal,

On 23 July 2018 at 03:08, Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote:
> > Hi Michal,
> >
> > 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@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@ff0a00000: input: 0 [ ]
> gpio@ff0a00001: input: 0 [ ]
> gpio@ff0a00002: input: 0 [ ]
> gpio@ff0a00003: input: 0 [ ]
> gpio@ff0a00004: input: 0 [ ]
> gpio@ff0a00005: input: 0 [ ]
> gpio@ff0a00006: input: 0 [ ]
> gpio@ff0a00007: input: 0 [ ]
> gpio@ff0a00008: input: 0 [ ]
> gpio@ff0a00009: input: 0 [ ]
>
> 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.
>
> >
> > 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.

Linux doesn't have this concept and has no command line. I am
skeptical they would be interested in adding the property.

If we can get this in by renaming it (e.g. to u-boot,gpio-bank-name)
then that would be OK. Otherwise I think we just have to rely on
having our own binding file.

Regards,
Simon

On 23.7.2018 20:41, Simon Glass wrote:
> Hi Michal,
> 
> On 23 July 2018 at 03:08, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote:
>>> Hi Michal,
>>>
>>> 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@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@ff0a00000: input: 0 [ ]
>> gpio@ff0a00001: input: 0 [ ]
>> gpio@ff0a00002: input: 0 [ ]
>> gpio@ff0a00003: input: 0 [ ]
>> gpio@ff0a00004: input: 0 [ ]
>> gpio@ff0a00005: input: 0 [ ]
>> gpio@ff0a00006: input: 0 [ ]
>> gpio@ff0a00007: input: 0 [ ]
>> gpio@ff0a00008: input: 0 [ ]
>> gpio@ff0a00009: input: 0 [ ]
>>
>> 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.
>>
>>>
>>> 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.
> 
> Linux doesn't have this concept and has no command line. I am
> skeptical they would be interested in adding the property.
> 
> If we can get this in by renaming it (e.g. to u-boot,gpio-bank-name)
> then that would be OK. Otherwise I think we just have to rely on
> having our own binding file.

There is a label property which could be used instead.

Thanks,
Michal

On 23.7.2018 20:29, Stefan Herbrechtsmeier wrote:
> Hi Michal,
> 
> 
> 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@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@ff0a00000: input: 0 [ ]
>> gpio@ff0a00001: input: 0 [ ]
>> gpio@ff0a00002: input: 0 [ ]
>> gpio@ff0a00003: input: 0 [ ]
>> gpio@ff0a00004: input: 0 [ ]
>> gpio@ff0a00005: input: 0 [ ]
>> gpio@ff0a00006: input: 0 [ ]
>> gpio@ff0a00007: input: 0 [ ]
>> gpio@ff0a00008: input: 0 [ ]
>> gpio@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?
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.
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.
Then in gpio status -a can be

Device gpio@a0001000:
Bank:
...

but not sure how gpio commands will work to address exact pin from
prompt. Because this is normally working
gpio toggle gpio@a00010001

Thanks,
Michal

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@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@ff0a00000: input: 0 [ ]
>>> gpio@ff0a00001: input: 0 [ ]
>>> gpio@ff0a00002: input: 0 [ ]
>>> gpio@ff0a00003: input: 0 [ ]
>>> gpio@ff0a00004: input: 0 [ ]
>>> gpio@ff0a00005: input: 0 [ ]
>>> gpio@ff0a00006: input: 0 [ ]
>>> gpio@ff0a00007: input: 0 [ ]
>>> gpio@ff0a00008: input: 0 [ ]
>>> gpio@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@a0001000:
> Bank:
> ...
>
> but not sure how gpio commands will work to address exact pin from
> prompt. Because this is normally working
> gpio toggle gpio@a00010001

With an optional device name this would be:
gpio toggle gpio@a0001000 1

Alternative the gpio command could support the requested labels:
gpio toggle second-gpio

Best regards
   Stefan

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@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@ff0a00000: input: 0 [ ]
>>>> gpio@ff0a00001: input: 0 [ ]
>>>> gpio@ff0a00002: input: 0 [ ]
>>>> gpio@ff0a00003: input: 0 [ ]
>>>> gpio@ff0a00004: input: 0 [ ]
>>>> gpio@ff0a00005: input: 0 [ ]
>>>> gpio@ff0a00006: input: 0 [ ]
>>>> gpio@ff0a00007: input: 0 [ ]
>>>> gpio@ff0a00008: input: 0 [ ]
>>>> gpio@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@a0001000:
>> Bank:
>> ...
>>
>> but not sure how gpio commands will work to address exact pin from
>> prompt. Because this is normally working
>> gpio toggle gpio@a00010001
> 
> With an optional device name this would be:
> gpio toggle gpio@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.

Thanks,
Michal

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@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@ff0a00000: input: 0 [ ]
>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>> gpio@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@a0001000:
>>> Bank:
>>> ...
>>>
>>> but not sure how gpio commands will work to address exact pin from
>>> prompt. Because this is normally working
>>> gpio toggle gpio@a00010001
>> With an optional device name this would be:
>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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

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@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@ff0a00000: input: 0 [ ]
>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>> gpio@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@a0001000:
>>>> Bank:
>>>> ...
>>>>
>>>> but not sure how gpio commands will work to address exact pin from
>>>> prompt. Because this is normally working
>>>> gpio toggle gpio@a00010001
>>> With an optional device name this would be:
>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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

Am 26.07.2018 um 10:22 schrieb Michal Simek:
> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>> gpio@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@a0001000:
>>>>> Bank:
>>>>> ...
>>>>>
>>>>> but not sure how gpio commands will work to address exact pin from
>>>>> prompt. Because this is normally working
>>>>> gpio toggle gpio@a00010001
>>>> With an optional device name this would be:
>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.

Thanks a lot for taking the time to write a detail explanation.

> 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).

I agree with you.

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

Does the kernel accept properly allocated but not used entries?

> 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 [ ]

This doesn't look good but therefore the pin name looks okay.

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

I have the same problem.

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

The problem I see is that you introduce a suboptimal API which make it 
hard to changed later or do you accept incompatible changes?

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

This would be okay for me.

But maybe there is a better solution. Would it be okay to add an device 
tree alias for the gpios? In this case we could use the seq number to 
select the device:

gpio set 0 2
gpio set 1 6

The first number would be the seq and the second the offset. This would 
make the bank name obsolete and could be backward compatible implemented 
in the gpio command.

Best regards
   Stefan

On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>>> gpio@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@a0001000:
>>>>>> Bank:
>>>>>> ...
>>>>>>
>>>>>> but not sure how gpio commands will work to address exact pin from
>>>>>> prompt. Because this is normally working
>>>>>> gpio toggle gpio@a00010001
>>>>> With an optional device name this would be:
>>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.
> 
> Thanks a lot for taking the time to write a detail explanation.
> 
>> 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).
> 
> I agree with you.
> 
>> 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.
> 
> Does the kernel accept properly allocated but not used entries?

kernel like a code doesn't care and ignores what it is not used. But DT
maintainers as far as I know don't like these options.

> 
>> 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 [ ]
> 
> This doesn't look good but therefore the pin name looks okay.

yes.

> 
>> 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.
> 
> I have the same problem.

ok.

> 
>> 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.
> 
> The problem I see is that you introduce a suboptimal API which make it
> hard to changed later or do you accept incompatible changes?

I didn't introduce any API. API was there and I haven't done any change
there. I just setup a name which the whole interface expects.
gpio numbers are still working without any change. It means gpio toggle
170 just works without any breakage.
This change is new and will be new in 2018.09 release and we haven't
reached rc1 yet. It means we still have enough time to keep this, revert
this, find a better solution or use temporary solution (with that _ for
example).
The same stuff is used by zynq and axi gpio drivers.

> 
>> 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.
> 
> This would be okay for me.
> 
> But maybe there is a better solution. Would it be okay to add an device
> tree alias for the gpios? 

I have convinced Linus to accept dt alias wiring for zynq gpio added by
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17

but in general statement from the is that gpios don't need to use
aliases. In this case alias ID setup base chip ID for sysfs interface
which is deprecated.

> In this case we could use the seq number to
> select the device:
> 
> gpio set 0 2
> gpio set 1 6
> 
> The first number would be the seq and the second the offset. This would
> make the bank name obsolete and could be backward compatible implemented
> in the gpio command.

seq number is setup based on aliases(if enabled - and in this gpio case
not recommended) or based on bind/probe order.

aliases - we know that this is not good to do
probe order - depends on DT, clock, etc

I don't think it is a good idea to use seq in this case.

Thanks,
Michal

Am 27.07.2018 um 08:42 schrieb Michal Simek:
> On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
>> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>>>> gpio@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@a0001000:
>>>>>>> Bank:
>>>>>>> ...
>>>>>>>
>>>>>>> but not sure how gpio commands will work to address exact pin from
>>>>>>> prompt. Because this is normally working
>>>>>>> gpio toggle gpio@a00010001
>>>>>> With an optional device name this would be:
>>>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.
>> Thanks a lot for taking the time to write a detail explanation.
>>
>>> 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).
>> I agree with you.
>>
>>> 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.
>> Does the kernel accept properly allocated but not used entries?
> kernel like a code doesn't care and ignores what it is not used. But DT
> maintainers as far as I know don't like these options.
>
>>> 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 [ ]
>> This doesn't look good but therefore the pin name looks okay.
> yes.
>
>>> 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.
>> I have the same problem.
> ok.
>
>>> 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.
>> The problem I see is that you introduce a suboptimal API which make it
>> hard to changed later or do you accept incompatible changes?
> I didn't introduce any API. API was there and I haven't done any change
> there. I just setup a name which the whole interface expects.
> gpio numbers are still working without any change. It means gpio toggle
> 170 just works without any breakage.

But we couldn't change the bank name after the release.

> This change is new and will be new in 2018.09 release and we haven't
> reached rc1 yet. It means we still have enough time to keep this, revert
> this, find a better solution or use temporary solution (with that _ for
> example).
> The same stuff is used by zynq and axi gpio drivers.

Okay

>>> 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.
>> This would be okay for me.
>>
>> But maybe there is a better solution. Would it be okay to add an device
>> tree alias for the gpios?
> I have convinced Linus to accept dt alias wiring for zynq gpio added by
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17

Okay

> but in general statement from the is that gpios don't need to use
> aliases. In this case alias ID setup base chip ID for sysfs interface
> which is deprecated.

This means the alias is okay but shouldn't be used from user space?

>> In this case we could use the seq number to
>> select the device:
>>
>> gpio set 0 2
>> gpio set 1 6
>>
>> The first number would be the seq and the second the offset. This would
>> make the bank name obsolete and could be backward compatible implemented
>> in the gpio command.
> seq number is setup based on aliases(if enabled - and in this gpio case
> not recommended) or based on bind/probe order.

Why isn't it recommended?

> aliases - we know that this is not good to do
> probe order - depends on DT, clock, etc

But isn't this widely used in u-boot?

> I don't think it is a good idea to use seq in this case.

I don't understand why it is widely used if it isn't a good idea.

At the moment the gpio framework use a bank name to distinguish between 
different controllers of the same type. We need to distinguish between 
different gpio controllers and hardware configurations. The right place 
for hardware configuration is the device tree. This means we have to add 
additional information into the device tree. This could be a bank-name 
per controller or an device tree alias. The later approach is already 
used by different other frameworks. It implements a low level interface 
for u-boot and makes the bank name obsolete.

Best regards
   Stefan

On 27.7.2018 11:14, Stefan Herbrechtsmeier wrote:
> Am 27.07.2018 um 08:42 schrieb Michal Simek:
>> On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
>>> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>>>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>>>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>>>>> gpio@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@a0001000:
>>>>>>>> Bank:
>>>>>>>> ...
>>>>>>>>
>>>>>>>> but not sure how gpio commands will work to address exact pin from
>>>>>>>> prompt. Because this is normally working
>>>>>>>> gpio toggle gpio@a00010001
>>>>>>> With an optional device name this would be:
>>>>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.
>>> Thanks a lot for taking the time to write a detail explanation.
>>>
>>>> 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).
>>> I agree with you.
>>>
>>>> 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.
>>> Does the kernel accept properly allocated but not used entries?
>> kernel like a code doesn't care and ignores what it is not used. But DT
>> maintainers as far as I know don't like these options.
>>
>>>> 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 [ ]
>>> This doesn't look good but therefore the pin name looks okay.
>> yes.
>>
>>>> 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.
>>> I have the same problem.
>> ok.
>>
>>>> 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.
>>> The problem I see is that you introduce a suboptimal API which make it
>>> hard to changed later or do you accept incompatible changes?
>> I didn't introduce any API. API was there and I haven't done any change
>> there. I just setup a name which the whole interface expects.
>> gpio numbers are still working without any change. It means gpio toggle
>> 170 just works without any breakage.
> 
> But we couldn't change the bank name after the release.

It will be the best to have one stable - yes.

> 
>> This change is new and will be new in 2018.09 release and we haven't
>> reached rc1 yet. It means we still have enough time to keep this, revert
>> this, find a better solution or use temporary solution (with that _ for
>> example).
>> The same stuff is used by zynq and axi gpio drivers.
> 
> Okay
> 
>>>> 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.
>>> This would be okay for me.
>>>
>>> But maybe there is a better solution. Would it be okay to add an device
>>> tree alias for the gpios?
>> I have convinced Linus to accept dt alias wiring for zynq gpio added by
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17
>>
> 
> Okay
> 
>> but in general statement from the is that gpios don't need to use
>> aliases. In this case alias ID setup base chip ID for sysfs interface
>> which is deprecated.
> 
> This means the alias is okay but shouldn't be used from user space?
> 
>>> In this case we could use the seq number to
>>> select the device:
>>>
>>> gpio set 0 2
>>> gpio set 1 6
>>>
>>> The first number would be the seq and the second the offset. This would
>>> make the bank name obsolete and could be backward compatible implemented
>>> in the gpio command.
>> seq number is setup based on aliases(if enabled - and in this gpio case
>> not recommended) or based on bind/probe order.
> 
> Why isn't it recommended?

Feel free to check with them but my understanding is that if alias is
not used by core/drivers then it shouldn't be listed because it is not
clear what it is for. Rob was asking me to remove these gpio aliases
,which I had in xilinx tree, when I was pushing zynqmp dts to mainline.


>> aliases - we know that this is not good to do
>> probe order - depends on DT, clock, etc
> 
> But isn't this widely used in u-boot?

Some uclasses enables that options. In u-boot seq alias is present for
gpio uclass.
Also keep in your mind that it depends on option which can be disabled
(DM_SEQ_ALIAS)


>> I don't think it is a good idea to use seq in this case.
> 
> I don't understand why it is widely used if it isn't a good idea.
> 
> At the moment the gpio framework use a bank name to distinguish between
> different controllers of the same type. We need to distinguish between
> different gpio controllers and hardware configurations. The right place
> for hardware configuration is the device tree. This means we have to add
> additional information into the device tree. This could be a bank-name
> per controller or an device tree alias. The later approach is already
> used by different other frameworks. It implements a low level interface
> for u-boot and makes the bank name obsolete.

It is about the flow and recent discussion with had in connection to
CMD_DM. To find which seq is which controller you need to run dm command
(which is for debug only) or list fdt(where u-boot dt address is not
setup by default and also it doesn't reflect if driver was binded).
Maybe it is not issue for others but looking for information which
controller is which one seems to me additional step. Feel free to enable
this option. dev->seq is filled by core automatically.

Regarding dt property. I have no problem to use property which are
already the part of binding. It means using label which describe what it
is for seems to be the best candidate to use (used by pca953x_gpio in
one case).
But again we have the same issue with this approach when name ends with
hex number and normally the name won't contain "_" suffix.

Anyway I have not a problem with this change.

diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
index 6fbaafb3fa3c..aa8d51e2f709 100644
--- a/drivers/gpio/zynq_gpio.c
+++ b/drivers/gpio/zynq_gpio.c
@@ -336,8 +336,18 @@ static int zynq_gpio_probe(struct udevice *dev)
 {
        struct zynq_gpio_platdata *platdata = dev_get_platdata(dev);
        struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
-
-       uc_priv->bank_name = dev->name;
+       const void *label_ptr;
+       void *label_c;
+       int size;
+
+       label_ptr = dev_read_prop(dev, "label", &size);
+       if (label_ptr) {
+               label_c = calloc(1, size);
+               memcpy(label_c, label_ptr, size);
+               uc_priv->bank_name = label_c;
+       } else {
+               uc_priv->bank_name = dev->name;
+       }

        if (platdata->p_data)
                uc_priv->gpio_count = platdata->p_data->ngpio;

Thanks,
Michal

Am 30.07.2018 um 10:00 schrieb Michal Simek:
> On 27.7.2018 11:14, Stefan Herbrechtsmeier wrote:
>> Am 27.07.2018 um 08:42 schrieb Michal Simek:
>>> On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
>>>> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>>>>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>>>>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>>>>>> gpio@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@a0001000:
>>>>>>>>> Bank:
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> but not sure how gpio commands will work to address exact pin from
>>>>>>>>> prompt. Because this is normally working
>>>>>>>>> gpio toggle gpio@a00010001
>>>>>>>> With an optional device name this would be:
>>>>>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.
>>>> Thanks a lot for taking the time to write a detail explanation.
>>>>
>>>>> 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).
>>>> I agree with you.
>>>>
>>>>> 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.
>>>> Does the kernel accept properly allocated but not used entries?
>>> kernel like a code doesn't care and ignores what it is not used. But DT
>>> maintainers as far as I know don't like these options.
>>>
>>>>> 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 [ ]
>>>> This doesn't look good but therefore the pin name looks okay.
>>> yes.
>>>
>>>>> 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.
>>>> I have the same problem.
>>> ok.
>>>
>>>>> 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.
>>>> The problem I see is that you introduce a suboptimal API which make it
>>>> hard to changed later or do you accept incompatible changes?
>>> I didn't introduce any API. API was there and I haven't done any change
>>> there. I just setup a name which the whole interface expects.
>>> gpio numbers are still working without any change. It means gpio toggle
>>> 170 just works without any breakage.
>> But we couldn't change the bank name after the release.
> It will be the best to have one stable - yes.
>
>>> This change is new and will be new in 2018.09 release and we haven't
>>> reached rc1 yet. It means we still have enough time to keep this, revert
>>> this, find a better solution or use temporary solution (with that _ for
>>> example).
>>> The same stuff is used by zynq and axi gpio drivers.
>> Okay
>>
>>>>> 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.
>>>> This would be okay for me.
>>>>
>>>> But maybe there is a better solution. Would it be okay to add an device
>>>> tree alias for the gpios?
>>> I have convinced Linus to accept dt alias wiring for zynq gpio added by
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17
>>>
>> Okay
>>
>>> but in general statement from the is that gpios don't need to use
>>> aliases. In this case alias ID setup base chip ID for sysfs interface
>>> which is deprecated.
>> This means the alias is okay but shouldn't be used from user space?
>>
>>>> In this case we could use the seq number to
>>>> select the device:
>>>>
>>>> gpio set 0 2
>>>> gpio set 1 6
>>>>
>>>> The first number would be the seq and the second the offset. This would
>>>> make the bank name obsolete and could be backward compatible implemented
>>>> in the gpio command.
>>> seq number is setup based on aliases(if enabled - and in this gpio case
>>> not recommended) or based on bind/probe order.
>> Why isn't it recommended?
> Feel free to check with them but my understanding is that if alias is
> not used by core/drivers then it shouldn't be listed because it is not
> clear what it is for. Rob was asking me to remove these gpio aliases
> ,which I had in xilinx tree, when I was pushing zynqmp dts to mainline.
>
>
>>> aliases - we know that this is not good to do
>>> probe order - depends on DT, clock, etc
>> But isn't this widely used in u-boot?
> Some uclasses enables that options. In u-boot seq alias is present for
> gpio uclass.
> Also keep in your mind that it depends on option which can be disabled
> (DM_SEQ_ALIAS)
>
>
>>> I don't think it is a good idea to use seq in this case.
>> I don't understand why it is widely used if it isn't a good idea.
>>
>> At the moment the gpio framework use a bank name to distinguish between
>> different controllers of the same type. We need to distinguish between
>> different gpio controllers and hardware configurations. The right place
>> for hardware configuration is the device tree. This means we have to add
>> additional information into the device tree. This could be a bank-name
>> per controller or an device tree alias. The later approach is already
>> used by different other frameworks. It implements a low level interface
>> for u-boot and makes the bank name obsolete.
> It is about the flow and recent discussion with had in connection to
> CMD_DM. To find which seq is which controller you need to run dm command
> (which is for debug only) or list fdt(where u-boot dt address is not
> setup by default and also it doesn't reflect if driver was binded).
> Maybe it is not issue for others but looking for information which
> controller is which one seems to me additional step. Feel free to enable
> this option. dev->seq is filled by core automatically.

Thanks for the informations.

> Regarding dt property. I have no problem to use property which are
> already the part of binding. It means using label which describe what it
> is for seems to be the best candidate to use (used by pca953x_gpio in
> one case).
> But again we have the same issue with this approach when name ends with
> hex number and normally the name won't contain "_" suffix.
>
> Anyway I have not a problem with this change.
>
> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
> index 6fbaafb3fa3c..aa8d51e2f709 100644
> --- a/drivers/gpio/zynq_gpio.c
> +++ b/drivers/gpio/zynq_gpio.c
> @@ -336,8 +336,18 @@ static int zynq_gpio_probe(struct udevice *dev)
>   {
>          struct zynq_gpio_platdata *platdata = dev_get_platdata(dev);
>          struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> -
> -       uc_priv->bank_name = dev->name;
> +       const void *label_ptr;
> +       void *label_c;
> +       int size;
> +
> +       label_ptr = dev_read_prop(dev, "label", &size);
> +       if (label_ptr) {
> +               label_c = calloc(1, size);
> +               memcpy(label_c, label_ptr, size);
> +               uc_priv->bank_name = label_c;
> +       } else {
> +               uc_priv->bank_name = dev->name;
> +       }
>
>          if (platdata->p_data)
>                  uc_priv->gpio_count = platdata->p_data->ngpio;

This would be okay for me.

Best regards
   Stefan

On 1.8.2018 20:23, Stefan Herbrechtsmeier wrote:
> Am 30.07.2018 um 10:00 schrieb Michal Simek:
>> On 27.7.2018 11:14, Stefan Herbrechtsmeier wrote:
>>> Am 27.07.2018 um 08:42 schrieb Michal Simek:
>>>> On 26.7.2018 22:04, Stefan Herbrechtsmeier wrote:
>>>>> Am 26.07.2018 um 10:22 schrieb Michal Simek:
>>>>>> On 25.7.2018 21:17, Stefan Herbrechtsmeier wrote:
>>>>>>> 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@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@ff0a00000: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00001: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00002: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00003: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00004: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00005: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00006: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00007: input: 0 [ ]
>>>>>>>>>>>> gpio@ff0a00008: input: 0 [ ]
>>>>>>>>>>>> gpio@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@a0001000:
>>>>>>>>>> Bank:
>>>>>>>>>> ...
>>>>>>>>>>
>>>>>>>>>> but not sure how gpio commands will work to address exact pin
>>>>>>>>>> from
>>>>>>>>>> prompt. Because this is normally working
>>>>>>>>>> gpio toggle gpio@a00010001
>>>>>>>>> With an optional device name this would be:
>>>>>>>>> gpio toggle gpio@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@a000100011" for pin 11 of the device gpio@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.
>>>>> Thanks a lot for taking the time to write a detail explanation.
>>>>>
>>>>>> 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).
>>>>> I agree with you.
>>>>>
>>>>>> 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.
>>>>> Does the kernel accept properly allocated but not used entries?
>>>> kernel like a code doesn't care and ignores what it is not used. But DT
>>>> maintainers as far as I know don't like these options.
>>>>
>>>>>> 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 [ ]
>>>>> This doesn't look good but therefore the pin name looks okay.
>>>> yes.
>>>>
>>>>>> 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.
>>>>> I have the same problem.
>>>> ok.
>>>>
>>>>>> 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.
>>>>> The problem I see is that you introduce a suboptimal API which make it
>>>>> hard to changed later or do you accept incompatible changes?
>>>> I didn't introduce any API. API was there and I haven't done any change
>>>> there. I just setup a name which the whole interface expects.
>>>> gpio numbers are still working without any change. It means gpio toggle
>>>> 170 just works without any breakage.
>>> But we couldn't change the bank name after the release.
>> It will be the best to have one stable - yes.
>>
>>>> This change is new and will be new in 2018.09 release and we haven't
>>>> reached rc1 yet. It means we still have enough time to keep this,
>>>> revert
>>>> this, find a better solution or use temporary solution (with that _ for
>>>> example).
>>>> The same stuff is used by zynq and axi gpio drivers.
>>> Okay
>>>
>>>>>> 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.
>>>>> This would be okay for me.
>>>>>
>>>>> But maybe there is a better solution. Would it be okay to add an
>>>>> device
>>>>> tree alias for the gpios?
>>>> I have convinced Linus to accept dt alias wiring for zynq gpio added by
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpio-zynq.c?h=v4.18-rc6&id=060f3ebf6a9a4a92dd92149e6ebffae10679ed17
>>>>
>>>>
>>> Okay
>>>
>>>> but in general statement from the is that gpios don't need to use
>>>> aliases. In this case alias ID setup base chip ID for sysfs interface
>>>> which is deprecated.
>>> This means the alias is okay but shouldn't be used from user space?
>>>
>>>>> In this case we could use the seq number to
>>>>> select the device:
>>>>>
>>>>> gpio set 0 2
>>>>> gpio set 1 6
>>>>>
>>>>> The first number would be the seq and the second the offset. This
>>>>> would
>>>>> make the bank name obsolete and could be backward compatible
>>>>> implemented
>>>>> in the gpio command.
>>>> seq number is setup based on aliases(if enabled - and in this gpio case
>>>> not recommended) or based on bind/probe order.
>>> Why isn't it recommended?
>> Feel free to check with them but my understanding is that if alias is
>> not used by core/drivers then it shouldn't be listed because it is not
>> clear what it is for. Rob was asking me to remove these gpio aliases
>> ,which I had in xilinx tree, when I was pushing zynqmp dts to mainline.
>>
>>
>>>> aliases - we know that this is not good to do
>>>> probe order - depends on DT, clock, etc
>>> But isn't this widely used in u-boot?
>> Some uclasses enables that options. In u-boot seq alias is present for
>> gpio uclass.
>> Also keep in your mind that it depends on option which can be disabled
>> (DM_SEQ_ALIAS)
>>
>>
>>>> I don't think it is a good idea to use seq in this case.
>>> I don't understand why it is widely used if it isn't a good idea.
>>>
>>> At the moment the gpio framework use a bank name to distinguish between
>>> different controllers of the same type. We need to distinguish between
>>> different gpio controllers and hardware configurations. The right place
>>> for hardware configuration is the device tree. This means we have to add
>>> additional information into the device tree. This could be a bank-name
>>> per controller or an device tree alias. The later approach is already
>>> used by different other frameworks. It implements a low level interface
>>> for u-boot and makes the bank name obsolete.
>> It is about the flow and recent discussion with had in connection to
>> CMD_DM. To find which seq is which controller you need to run dm command
>> (which is for debug only) or list fdt(where u-boot dt address is not
>> setup by default and also it doesn't reflect if driver was binded).
>> Maybe it is not issue for others but looking for information which
>> controller is which one seems to me additional step. Feel free to enable
>> this option. dev->seq is filled by core automatically.
> 
> Thanks for the informations.
> 
>> Regarding dt property. I have no problem to use property which are
>> already the part of binding. It means using label which describe what it
>> is for seems to be the best candidate to use (used by pca953x_gpio in
>> one case).
>> But again we have the same issue with this approach when name ends with
>> hex number and normally the name won't contain "_" suffix.
>>
>> Anyway I have not a problem with this change.
>>
>> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c
>> index 6fbaafb3fa3c..aa8d51e2f709 100644
>> --- a/drivers/gpio/zynq_gpio.c
>> +++ b/drivers/gpio/zynq_gpio.c
>> @@ -336,8 +336,18 @@ static int zynq_gpio_probe(struct udevice *dev)
>>   {
>>          struct zynq_gpio_platdata *platdata = dev_get_platdata(dev);
>>          struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> -
>> -       uc_priv->bank_name = dev->name;
>> +       const void *label_ptr;
>> +       void *label_c;
>> +       int size;
>> +
>> +       label_ptr = dev_read_prop(dev, "label", &size);
>> +       if (label_ptr) {
>> +               label_c = calloc(1, size);
>> +               memcpy(label_c, label_ptr, size);
>> +               uc_priv->bank_name = label_c;
>> +       } else {
>> +               uc_priv->bank_name = dev->name;
>> +       }
>>
>>          if (platdata->p_data)
>>                  uc_priv->gpio_count = platdata->p_data->ngpio;
> 
> This would be okay for me.

Ok. I have sent regular patch for that.

Thanks,
Michal