| Message ID | 20240326141108.1079993-2-ckeepax@opensource.cirrus.com |
|---|---|
| State | New |
| 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
On Tue, Mar 26, 2024 at 02:11:06PM +0000, Charles Keepax 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, > }, > }; I am sorry, I am very late to the party, but wouldn't it all work by simply setting ".node" to NULL? As far as I can see we have in software_node_get_reference_args(): ... if (index * sizeof(*ref) >= prop->length) return -ENOENT; ref_array = prop->pointer; ref = &ref_array[index]; refnode = software_node_fwnode(ref->node); if (!refnode) return -ENOENT; if ref->node is NULL then software_node_fwnode(ref->node) will return NULL and we'll get -ENOENT which will bubble up to gpiod_get_index_optional() in SPI core. Thanks.
On Tue, Dec 10, 2024 at 04:15:18PM -0800, Dmitry Torokhov wrote: > On Tue, Mar 26, 2024 at 02:11:06PM +0000, Charles Keepax 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, > > }, > > }; > > I am sorry, I am very late to the party, but wouldn't it all work by > simply setting ".node" to NULL? As far as I can see we have in > software_node_get_reference_args(): > > ... > > if (index * sizeof(*ref) >= prop->length) > return -ENOENT; > > ref_array = prop->pointer; > ref = &ref_array[index]; > > refnode = software_node_fwnode(ref->node); > if (!refnode) > return -ENOENT; > > if ref->node is NULL then software_node_fwnode(ref->node) will return > NULL and we'll get -ENOENT which will bubble up to > gpiod_get_index_optional() in SPI core. > I did definitely try that at the time, and there was definitely some issue with that approach. I think it might have related to something around how the GPIO stuff structures things. Alas, it is long enough ago that the details are escaping me, if I manage to find a spare minute I will try to refresh my memory and report back. 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(+)