Previously, GPIO lines in the aggregator had empty names. Now that the
configfs interface supports custom names, update the GPIO forwarder to
use them.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 71 ++++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 3263d99bfe69..268b9b580ada 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
}
#endif /* !CONFIG_OF_GPIO */
+static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
+{
+ int num = device_property_string_array_count(dev, "gpio-line-names");
+ if (!num)
+ return 0;
+ if (num > len) {
+ pr_warn("gpio-line-names contains %d lines while %d expected",
+ num, len);
+ num = len;
+ }
+ return device_property_read_string_array(dev, "gpio-line-names",
+ names, num);
+}
+
/**
* gpiochip_fwd_create() - Create a new GPIO forwarder
* @dev: Parent device pointer
@@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
{
const char *label = dev_name(dev);
struct gpiochip_fwd *fwd;
+ const char **line_names;
struct gpio_chip *chip;
unsigned int i;
int error;
@@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
chip = &fwd->chip;
+ if (!dev_of_node(dev)) {
+ line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
+ if (!line_names)
+ return ERR_PTR(-ENOMEM);
+
+ error = gpiochip_fwd_line_names(dev, line_names, ngpios);
+ if (error < 0)
+ return ERR_PTR(-ENOMEM);
+ }
+
/*
* If any of the GPIO lines are sleeping, then the entire forwarder
* will be sleeping.
@@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
chip->ngpio = ngpios;
fwd->descs = descs;
+ if (!dev_of_node(dev))
+ chip->names = line_names;
+
if (chip->can_sleep)
mutex_init(&fwd->mlock);
else
@@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
return container_of(group, struct gpio_aggregator_line, group);
}
+static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
+{
+ char **line_names __free(kfree) = NULL;
+ struct property_entry properties[2];
+ struct gpio_aggregator_line *line;
+ size_t num_lines;
+ int n = 0;
+
+ memset(properties, 0, sizeof(properties));
+
+ num_lines = aggr_count_lines(aggr);
+ if (num_lines == 0)
+ return NULL;
+
+ line_names = kcalloc(num_lines, sizeof(*line_names), GFP_KERNEL);
+ if (!line_names)
+ return ERR_PTR(-ENOMEM);
+
+ /* The list is always sorted as new elements are inserted in order. */
+ list_for_each_entry(line, &aggr->list_head, entry)
+ line_names[n++] = line->name ?: "";
+
+ properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+ "gpio-line-names",
+ line_names, num_lines);
+
+ return fwnode_create_software_node(properties, NULL);
+}
+
static int aggr_activate(struct gpio_aggregator *aggr)
{
struct platform_device_info pdevinfo;
struct gpio_aggregator_line *line;
+ struct fwnode_handle *swnode;
struct platform_device *pdev;
unsigned int n = 0;
int ret = 0;
@@ -546,9 +604,14 @@ static int aggr_activate(struct gpio_aggregator *aggr)
if (!aggr->lookups)
return -ENOMEM;
+ swnode = aggr_make_device_swnode(aggr);
+ if (IS_ERR(swnode))
+ goto err_remove_lookups;
+
memset(&pdevinfo, 0, sizeof(pdevinfo));
pdevinfo.name = DRV_NAME;
pdevinfo.id = aggr->id;
+ pdevinfo.fwnode = swnode;
/* The list is always sorted as new elements are inserted in order. */
list_for_each_entry(line, &aggr->list_head, entry) {
@@ -560,7 +623,7 @@ static int aggr_activate(struct gpio_aggregator *aggr)
*/
if (!line->key || line->idx != n) {
ret = -EINVAL;
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
if (line->offset < 0)
@@ -568,13 +631,13 @@ static int aggr_activate(struct gpio_aggregator *aggr)
else
ret = aggr_add_gpio(aggr, line->key, line->offset, &n);
if (ret)
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
if (!aggr->lookups->dev_id) {
ret = -ENOMEM;
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
gpiod_add_lookup_table(aggr->lookups);
@@ -607,6 +670,8 @@ static int aggr_activate(struct gpio_aggregator *aggr)
err_remove_lookup_table:
kfree(aggr->lookups->dev_id);
gpiod_remove_lookup_table(aggr->lookups);
+err_remove_swnode:
+ fwnode_remove_software_node(swnode);
err_remove_lookups:
kfree(aggr->lookups);
--
2.45.2Hi Den-san,
Thanks for your patch!
On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> Previously, GPIO lines in the aggregator had empty names. Now that the
That is only true for aggregators created through the sysfs interface,
right? When created from DT, gpio-line-names is already supported:
https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72
> configfs interface supports custom names, update the GPIO forwarder to
> use them.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
> }
> #endif /* !CONFIG_OF_GPIO */
>
> +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> +{
> + int num = device_property_string_array_count(dev, "gpio-line-names");
> + if (!num)
> + return 0;
> + if (num > len) {
> + pr_warn("gpio-line-names contains %d lines while %d expected",
> + num, len);
> + num = len;
> + }
> + return device_property_read_string_array(dev, "gpio-line-names",
> + names, num);
> +}
> +
> /**
> * gpiochip_fwd_create() - Create a new GPIO forwarder
> * @dev: Parent device pointer
> @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> {
> const char *label = dev_name(dev);
> struct gpiochip_fwd *fwd;
> + const char **line_names;
> struct gpio_chip *chip;
> unsigned int i;
> int error;
> @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
> chip = &fwd->chip;
>
> + if (!dev_of_node(dev)) {
> + line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
So this is always allocated, even when no names are specified?
> + if (!line_names)
> + return ERR_PTR(-ENOMEM);
> +
> + error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> + if (error < 0)
> + return ERR_PTR(-ENOMEM);
> + }
> +
> /*
> * If any of the GPIO lines are sleeping, then the entire forwarder
> * will be sleeping.
> @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> chip->ngpio = ngpios;
> fwd->descs = descs;
>
> + if (!dev_of_node(dev))
> + chip->names = line_names;
> +
Do you actually need to collect the names yourself?
Below, the driver does:
error = devm_gpiochip_add_data(dev, chip, fwd);
and gpiochip_add_data_with_key() already calls gpiochip_set_names()
to retrieve the names from the gpio-line-names property.
What is missing to make that work?
> if (chip->can_sleep)
> mutex_init(&fwd->mlock);
> else
> @@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
> return container_of(group, struct gpio_aggregator_line, group);
> }
>
> +static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
> +{
> + char **line_names __free(kfree) = NULL;
const (needed when gpio_aggregator_line.name becomes const)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Feb 12, 2025 at 03:44:46PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
>
> Thanks for your patch!
>
> On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > Previously, GPIO lines in the aggregator had empty names. Now that the
>
> That is only true for aggregators created through the sysfs interface,
> right? When created from DT, gpio-line-names is already supported:
> https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72
You're absolutely right, I'll fix this commit message, along with the
needless codes you mentioned below.
>
> > configfs interface supports custom names, update the GPIO forwarder to
> > use them.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
>
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
> > }
> > #endif /* !CONFIG_OF_GPIO */
> >
> > +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> > +{
> > + int num = device_property_string_array_count(dev, "gpio-line-names");
> > + if (!num)
> > + return 0;
> > + if (num > len) {
> > + pr_warn("gpio-line-names contains %d lines while %d expected",
> > + num, len);
> > + num = len;
> > + }
> > + return device_property_read_string_array(dev, "gpio-line-names",
> > + names, num);
> > +}
> > +
> > /**
> > * gpiochip_fwd_create() - Create a new GPIO forwarder
> > * @dev: Parent device pointer
> > @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> > {
> > const char *label = dev_name(dev);
> > struct gpiochip_fwd *fwd;
> > + const char **line_names;
> > struct gpio_chip *chip;
> > unsigned int i;
> > int error;
> > @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> >
> > chip = &fwd->chip;
> >
> > + if (!dev_of_node(dev)) {
> > + line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);
>
> So this is always allocated, even when no names are specified?
Yes. This is unreasonable. I'll fix this in v3. Thanks.
>
> > + if (!line_names)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> > + if (error < 0)
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > /*
> > * If any of the GPIO lines are sleeping, then the entire forwarder
> > * will be sleeping.
> > @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> > chip->ngpio = ngpios;
> > fwd->descs = descs;
> >
> > + if (!dev_of_node(dev))
> > + chip->names = line_names;
> > +
>
> Do you actually need to collect the names yourself?
> Below, the driver does:
>
> error = devm_gpiochip_add_data(dev, chip, fwd);
>
> and gpiochip_add_data_with_key() already calls gpiochip_set_names()
> to retrieve the names from the gpio-line-names property.
> What is missing to make that work?
Thanks for pointing it out. It was just my oversight. I'll drop the
needless parts in v3.
>
> > if (chip->can_sleep)
> > mutex_init(&fwd->mlock);
> > else
> > @@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
> > return container_of(group, struct gpio_aggregator_line, group);
> > }
> >
> > +static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
> > +{
> > + char **line_names __free(kfree) = NULL;
>
> const (needed when gpio_aggregator_line.name becomes const)
Will fix in v3.
Thank you!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
© 2016 - 2026 Red Hat, Inc.