Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32
On 07. 01. 21 11:47, Linus Walleij wrote: > On Thu, Jan 7, 2021 at 11:29 AM Michal Simek wrote: >> On 07. 01. 21 11:17, Linus Walleij wrote: >>> On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli >>> wrote: > @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) if (of_property_read_u32(np, "xlnx,gpio-width", >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. > > OK that is confusing. Let's not do that then. > >> 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. > > OK I agree. > + if (chip->gpio_width[0] > 32) + return -EINVAL; >>> >>> This looks OK. >> >> Does it mean ack for this patch? > > Yeah after explanations this patch is fine: > Acked-by: Linus Walleij > > It's just that this hardware with paired controllers is a bit weird so it will > lead to discussions all the time because it's hard to understand. Maybe it should be described a little bit differently in DT. Just to have gpio node and every bank could be described as a child node where standard properties could be used and irq will be shared. Thanks, Michal
Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32
On Thu, Jan 7, 2021 at 11:29 AM Michal Simek wrote: > On 07. 01. 21 11:17, Linus Walleij wrote: > > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli > > wrote: > >> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) > >> if (of_property_read_u32(np, "xlnx,gpio-width", > >> >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. OK that is confusing. Let's not do that then. > 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. OK I agree. > >> + if (chip->gpio_width[0] > 32) > >> + return -EINVAL; > > > > This looks OK. > > Does it mean ack for this patch? Yeah after explanations this patch is fine: Acked-by: Linus Walleij It's just that this hardware with paired controllers is a bit weird so it will lead to discussions all the time because it's hard to understand. Yours, Linus Walleij
Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32
On 07. 01. 21 11:17, Linus Walleij wrote: > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli > wrote: > >> Add check to see if gpio-width property does not exceed 32. >> If it exceeds then return -EINVAL. >> >> Signed-off-by: Srinivas Neeli > > Aha > >> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) >> if (of_property_read_u32(np, "xlnx,gpio-width", >> >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
Re: [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli wrote: > Add check to see if gpio-width property does not exceed 32. > If it exceeds then return -EINVAL. > > Signed-off-by: Srinivas Neeli Aha > @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) > if (of_property_read_u32(np, "xlnx,gpio-width", >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? > + if (chip->gpio_width[0] > 32) > + return -EINVAL; This looks OK. Yours, Linus Walleij
[PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32
Add check to see if gpio-width property does not exceed 32. If it exceeds then return -EINVAL. Signed-off-by: Srinivas Neeli --- Changes in V4: -New patch. --- drivers/gpio/gpio-xilinx.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c index e47ae08167f8..ddec718e114c 100644 --- a/drivers/gpio/gpio-xilinx.c +++ b/drivers/gpio/gpio-xilinx.c @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) if (of_property_read_u32(np, "xlnx,gpio-width", >gpio_width[0])) chip->gpio_width[0] = 32; + if (chip->gpio_width[0] > 32) + return -EINVAL; + spin_lock_init(>gpio_lock); if (of_property_read_u32(np, "xlnx,is-dual", _dual)) @@ -615,6 +618,8 @@ static int xgpio_probe(struct platform_device *pdev) >gpio_width[1])) chip->gpio_width[1] = 32; + if (chip->gpio_width[1] > 32) + return -EINVAL; } chip->gc.base = -1; -- 2.7.4