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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Apr 11, 2024 at 12:06 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

Here, the

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

...

> +config GPIO_SWNODE_UNDEFINED
> +       bool

But why did you remove the help? Please, put it back.

...

> +       /*
> +        * Check for special node that identifies undefined GPIOs, this is

for a special

> +        * primarily used as a key for internal chip selects in SPI bindings.
> +        */
> +       if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
> +               return ERR_PTR(-ENOENT);

This is a dead code when the respective config option is not selected.
Or actually a potential flaw if somebody else names their swnode like
this.

...

> +       ret = software_node_register(&swnode_gpio_undefined);
> +       if (ret < 0)
> +               pr_err("gpiolib: failed to register swnode: %d\n", ret);

Instead of this prefix, define pr_fmt()

> +       return ret;

...

> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h

Why? I already said that the best place is gpio/property.h as it's
exactly for swnode related code.

-- 
With Best Regards,
Andy Shevchenko