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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

Fri, Mar 29, 2024 at 11:47:28AM +0000, Charles Keepax kirjoitti:
> 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.

...

> +const struct software_node swnode_gpio_undefined = {
> +	.name = "gpio-internal-undefined",

This name is still too generic, perhaps "software-node-undefined-gpio"?

> +};
> +EXPORT_SYMBOL_GPL(swnode_gpio_undefined);

Maybe we can start using namespaces?

...

> +	if (!strcmp(gdev_node->name, "gpio-internal-undefined"))

Probably it needs a definition to avoid typos in case somebody wants to change
the name.

#define	GPIOLIB_SWNODE_UNDEFINED	"..."

?

> +		return ERR_PTR(-ENOENT);

...

>  static int __init gpiolib_debugfs_init(void)
>  {
> +	int ret;
> +
> +	ret = software_node_register(&swnode_gpio_undefined);
> +	if (ret < 0) {
> +		pr_err("gpiolib: failed to register swnode: %d\n", ret);
> +		return ret;
> +	}
> +
>  	/* /sys/kernel/debug/gpio */
>  	debugfs_create_file("gpio", 0444, NULL, NULL, &gpiolib_fops);
> +
>  	return 0;
>  }
>  subsys_initcall(gpiolib_debugfs_init);

I believe now it's time to get __exitcall to be implemented. Actually the above
already needs that. Besides that, what you are adding doesn't belong to debugfs
AFAICS. And why it can't be done in gpiolib-swnode.c?

-- 
With Best Regards,
Andy Shevchenko