Re: [PATCH 1/3] gpio: swnode: Add ability to specify native chip selects for SPI

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Mar 26, 2024 at 3:11 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:

> SPI devices can specify a cs-gpios property to enumerate their
> chip selects. Under device tree, a zero entry in this property can
> be used to specify that a particular chip select is using the SPI
> controllers native chip select, for example:
>
>         cs-gpios = <&gpio1 0 0>, <0>;
>
> Here the second chip select is native. However, when using swnodes
> there is currently no way to specify a native chip select. The
> proposal here is to register a swnode_gpio_undefined software node,
> that can be specified to allow the indication of a native chip
> select. For example:
>
> static const struct software_node_ref_args device_cs_refs[] = {
>         {
>                 .node  = &device_gpiochip_swnode,
>                 .nargs = 2,
>                 .args  = { 0, GPIO_ACTIVE_LOW },
>         },
>         {
>                 .node  = &swnode_gpio_undefined,
>                 .nargs = 0,
>         },
> };
>
> Register the swnode as the gpiolib is initialised and
> check in swnode_get_gpio_device if the returned node matches
> swnode_gpio_undefined and return -ENOENT, which matches the behaviour
> of the device tree system when it encounters a 0 phandle.
>
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>

Hm that's an interesting corner case.

> +const struct software_node swnode_gpio_undefined = {
> +       .name = "gpio-internal-undefined",
> +};
> +EXPORT_SYMBOL_GPL(swnode_gpio_undefined);

This needs a comment in the code telling exactly why this is here.
It is also taking up space and code here on systems that have no use
for it, so I wonder if it is possible to make this optional.

> +       if (!strcmp(gdev_node->name, "gpio-internal-undefined"))
> +               return ERR_PTR(-ENOENT);

This needs a comment stating why this check is here, it's not
obvious.

Yours,
Linus Walleij