Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32

[Michal Simek <michal.simek@xilinx.com>]

On 07. 01. 21 11:17, Linus Walleij wrote:
> On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> 
>> Add check to see if gpio-width property does not exceed 32.
>> If it exceeds then return -EINVAL.
>>
>> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> 
> Aha
> 
>> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev)
>>         if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0]))
>>                 chip->gpio_width[0] = 32;
> 
> This xlnx,gpio-width seems very much like the standard ngpios property
> from Documentation/devicetree/bindings/gpio/gpio.txt
> but I guess not much to do about that now. :/
> 
> Do you think you can add support for both?

support for both is definitely possible but we need to handle also gpio
width for second channel referenced by xlnx,gpio2-widht now.

It means we could end up in situation which can be misleading for users
where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we
have 20 gpios.

I think that it is better not to start to mess with ngpios property not
to confuse people which are coming from other SOCs because ngpios can
suggest all gpios assigned to this controller.

And in second case where ngpios is total number of gpios and if
xlnx,gpio2-width is defined you can find width for first bank.
But it is questionable if this improve situation here.

Please correct me if my logic is not correct.
Definitely this should be done separately out of this patch.

> 
>> +       if (chip->gpio_width[0] > 32)
>> +               return -EINVAL;
> 
> This looks OK.

Does it mean ack for this patch?

Thanks,
Michal