| Message ID | 20240326141108.1079993-2-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
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
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
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
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 --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 */
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(+)