On 07. 01. 21 11:47, Linus Walleij wrote:

On Thu, Jan 7, 2021 at 11:29 AM Michal Simek [off-list ref] wrote:

quoted

On 07. 01. 21 11:17, Linus Walleij wrote:

quoted

On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli [off-list ref] wrote:

quoted

quoted

quoted

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

OK that is confusing. Let's not do that then.

quoted

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.

quoted

quoted

quoted

+       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 <redacted>

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