diff mbox series

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

Message ID 20240326141108.1079993-2-ckeepax@opensource.cirrus.com
State New
Headers show
Series Add bridged amplifiers to cs42l43 | expand

Commit Message

Charles Keepax March 26, 2024, 2:11 p.m. UTC
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@opensource.cirrus.com>
---
 drivers/gpio/gpiolib-swnode.c | 8 ++++++++
 drivers/gpio/gpiolib.c        | 9 +++++++++
 include/linux/gpio/consumer.h | 4 ++++
 3 files changed, 21 insertions(+)

Comments

Linus Walleij April 4, 2024, 8:16 a.m. UTC | #1
On Tue, Mar 26, 2024 at 3:11 PM Charles Keepax
<ckeepax@opensource.cirrus.com> 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@opensource.cirrus.com>

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
Charles Keepax April 8, 2024, 1:21 p.m. UTC | #2
On Thu, Apr 04, 2024 at 10:16:35AM +0200, Linus Walleij wrote:
> On Tue, Mar 26, 2024 at 3:11 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > +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.
> 

Happy to add the comment, less sure about how to make it
optional. I could ifdef it based the SPI config, but whilst that
is the current user the mechanism feels like it is more generic
than that and could be used in other bindings as well.

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

Happy to add a comment here as well.

Thanks,
Charles
Linus Walleij April 9, 2024, 7:12 a.m. UTC | #3
On Mon, Apr 8, 2024 at 3:21 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Apr 04, 2024 at 10:16:35AM +0200, Linus Walleij wrote:
> > On Tue, Mar 26, 2024 at 3:11 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > +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.
> >
>
> Happy to add the comment, less sure about how to make it
> optional. I could ifdef it based the SPI config, but whilst that
> is the current user the mechanism feels like it is more generic
> than that and could be used in other bindings as well.

That's a fair point.
Maybe a new bool Kconfig symbol that the SPI drivers or
other potential users can select?

Yours,
Linus Walleij
Charles Keepax April 9, 2024, 8:44 a.m. UTC | #4
On Tue, Apr 09, 2024 at 09:12:01AM +0200, Linus Walleij wrote:
> On Mon, Apr 8, 2024 at 3:21 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Thu, Apr 04, 2024 at 10:16:35AM +0200, Linus Walleij wrote:
> > > On Tue, Mar 26, 2024 at 3:11 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > +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.
> > >
> >
> > Happy to add the comment, less sure about how to make it
> > optional. I could ifdef it based the SPI config, but whilst that
> > is the current user the mechanism feels like it is more generic
> > than that and could be used in other bindings as well.
> 
> That's a fair point.
> Maybe a new bool Kconfig symbol that the SPI drivers or
> other potential users can select?
> 

OK I will add a Kconfig to enable this feature.

Thanks,
Charles
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index fa52bdb1a29a..801b5a660307 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -17,6 +17,11 @@ 
 #include "gpiolib.h"
 #include "gpiolib-swnode.h"
 
+const struct software_node swnode_gpio_undefined = {
+	.name = "gpio-internal-undefined",
+};
+EXPORT_SYMBOL_GPL(swnode_gpio_undefined);
+
 static void swnode_format_propname(const char *con_id, char *propname,
 				   size_t max_size)
 {
@@ -40,6 +45,9 @@  static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 	if (!gdev_node || !gdev_node->name)
 		return ERR_PTR(-EINVAL);
 
+	if (!strcmp(gdev_node->name, "gpio-internal-undefined"))
+		return ERR_PTR(-ENOENT);
+
 	gdev = gpio_device_find_by_label(gdev_node->name);
 	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ce94e37bcbee..e3a7e2a3a323 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4892,8 +4892,17 @@  DEFINE_SEQ_ATTRIBUTE(gpiolib);
 
 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);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edb..e685fac43398 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -12,6 +12,8 @@  struct fwnode_handle;
 struct gpio_array;
 struct gpio_desc;
 
+struct software_node;
+
 /**
  * struct gpio_descs - Struct containing an array of descriptors that can be
  *                     obtained using gpiod_get_array()
@@ -54,6 +56,8 @@  enum gpiod_flags {
 	GPIOD_OUT_HIGH_OPEN_DRAIN = GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_OPEN_DRAIN,
 };
 
+extern const struct software_node swnode_gpio_undefined;
+
 #ifdef CONFIG_GPIOLIB
 
 /* Return the number of GPIOs associated with a device / function */