[PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We access internals of struct gpio_device and struct gpio_desc because
it's easier but it can actually be avoided and we're working towards a
better encapsulation of GPIO data structures across the kernel so let's
start at home.

Instead of checking gpio_desc flags, let's just track the requests of
GPIOs in the driver. We also already store the information about
direction of simulated lines.

For kobjects needed by sysfs callbacks: we can iterate over the children
devices of the top-level platform device and compare their fwnodes
against the one passed to the init function from probe.

While at it: fix one line break and remove the untrue part about
configfs callbacks using dev_get_drvdata() from a comment.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v1 -> v2:
- use get_dev_from_fwnode() instead of dereferencing fwnode directly

v2 -> v3:
- don't use fwnode internal fields, instead: iterate over the platform
  device's children and locate the GPIO device

 drivers/gpio/gpio-sim.c | 66 ++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 271db3639a78..16a865e79559 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/configfs.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/idr.h>
@@ -30,8 +31,6 @@
 #include <linux/string_helpers.h>
 #include <linux/sysfs.h>
 
-#include "gpiolib.h"
-
 #define GPIO_SIM_NGPIO_MAX	1024
 #define GPIO_SIM_PROP_MAX	4 /* Max 3 properties + sentinel. */
 #define GPIO_SIM_NUM_ATTRS	3 /* value, pull and sentinel */
@@ -40,6 +39,9 @@ static DEFINE_IDA(gpio_sim_ida);
 
 struct gpio_sim_chip {
 	struct gpio_chip gc;
+	struct fwnode_handle *swnode;
+	struct device *dev;
+	unsigned long *request_map;
 	unsigned long *direction_map;
 	unsigned long *value_map;
 	unsigned long *pull_map;
@@ -63,16 +65,11 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
 			       unsigned int offset, int value)
 {
 	int irq, irq_type, ret;
-	struct gpio_desc *desc;
-	struct gpio_chip *gc;
-
-	gc = &chip->gc;
-	desc = &gc->gpiodev->descs[offset];
 
 	guard(mutex)(&chip->lock);
 
-	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
-	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
+	if (test_bit(offset, chip->request_map) &&
+	    test_bit(offset, chip->direction_map)) {
 		if (value == !!test_bit(offset, chip->value_map))
 			goto set_pull;
 
@@ -99,8 +96,8 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
 
 set_value:
 	/* Change the value unless we're actively driving the line. */
-	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
-	    !test_bit(FLAG_IS_OUT, &desc->flags))
+	if (!test_bit(offset, chip->request_map) ||
+	    test_bit(offset, chip->direction_map))
 		__assign_bit(offset, chip->value_map, value);
 
 set_pull:
@@ -181,7 +178,7 @@ static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
 }
 
 static int gpio_sim_set_config(struct gpio_chip *gc,
-				  unsigned int offset, unsigned long config)
+			       unsigned int offset, unsigned long config)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
@@ -204,13 +201,25 @@ static int gpio_sim_to_irq(struct gpio_chip *gc, unsigned int offset)
 	return irq_create_mapping(chip->irq_sim, offset);
 }
 
+static int gpio_sim_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	scoped_guard(mutex, &chip->lock)
+		__set_bit(offset, chip->request_map);
+
+	return 0;
+}
+
 static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
 
-	scoped_guard(mutex, &chip->lock)
+	scoped_guard(mutex, &chip->lock) {
 		__assign_bit(offset, chip->value_map,
 			     !!test_bit(offset, chip->pull_map));
+		__clear_bit(offset, chip->request_map);
+	}
 }
 
 static ssize_t gpio_sim_sysfs_val_show(struct device *dev,
@@ -295,7 +304,7 @@ static void gpio_sim_sysfs_remove(void *data)
 {
 	struct gpio_sim_chip *chip = data;
 
-	sysfs_remove_groups(&chip->gc.gpiodev->dev.kobj, chip->attr_groups);
+	sysfs_remove_groups(&chip->dev->kobj, chip->attr_groups);
 }
 
 static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
@@ -352,14 +361,25 @@ static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
 		chip->attr_groups[i] = attr_group;
 	}
 
-	ret = sysfs_create_groups(&chip->gc.gpiodev->dev.kobj,
-				  chip->attr_groups);
+	ret = sysfs_create_groups(&chip->dev->kobj, chip->attr_groups);
 	if (ret)
 		return ret;
 
 	return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
 }
 
+static int gpio_sim_chip_set_device(struct device *dev, void *data)
+{
+	struct gpio_sim_chip *chip = data;
+
+	if (chip->swnode == dev->fwnode) {
+		chip->dev = dev;
+		return 1;
+	}
+
+	return 0;
+}
+
 static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 {
 	struct gpio_sim_chip *chip;
@@ -387,6 +407,10 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (!chip)
 		return -ENOMEM;
 
+	chip->request_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->request_map)
+		return -ENOMEM;
+
 	chip->direction_map = devm_bitmap_alloc(dev, num_lines, GFP_KERNEL);
 	if (!chip->direction_map)
 		return -ENOMEM;
@@ -432,6 +456,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	gc->get_direction = gpio_sim_get_direction;
 	gc->set_config = gpio_sim_set_config;
 	gc->to_irq = gpio_sim_to_irq;
+	gc->request = gpio_sim_request;
 	gc->free = gpio_sim_free;
 	gc->can_sleep = true;
 
@@ -439,8 +464,13 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (ret)
 		return ret;
 
-	/* Used by sysfs and configfs callbacks. */
-	dev_set_drvdata(&gc->gpiodev->dev, chip);
+	chip->swnode = swnode;
+	ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
+	if (!ret)
+		return -ENODEV;
+
+	/* Used by sysfs callbacks. */
+	dev_set_drvdata(chip->dev, chip);
 
 	return gpio_sim_setup_sysfs(chip);
 }
-- 
2.39.2

Re: [PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 10:24:13AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We access internals of struct gpio_device and struct gpio_desc because
> it's easier but it can actually be avoided and we're working towards a
> better encapsulation of GPIO data structures across the kernel so let's
> start at home.
> 
> Instead of checking gpio_desc flags, let's just track the requests of
> GPIOs in the driver. We also already store the information about
> direction of simulated lines.
> 
> For kobjects needed by sysfs callbacks: we can iterate over the children
> devices of the top-level platform device and compare their fwnodes
> against the one passed to the init function from probe.
> 
> While at it: fix one line break and remove the untrue part about
> configfs callbacks using dev_get_drvdata() from a comment.

...

> v2 -> v3:
> - don't use fwnode internal fields, instead: iterate over the platform
>   device's children and locate the GPIO device

Thank you!

...

> @@ -181,7 +178,7 @@ static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)

>  static int gpio_sim_set_config(struct gpio_chip *gc,
> -				  unsigned int offset, unsigned long config)
> +			       unsigned int offset, unsigned long config)

Looking at other prototypes, it can be

static int gpio_sim_set_config(struct gpio_chip *gc, unsigned int offset,
			       unsigned long config)

...

> +static int gpio_sim_chip_set_device(struct device *dev, void *data)
> +{
> +	struct gpio_sim_chip *chip = data;

> +	if (chip->swnode == dev->fwnode) {

Please do not dereference fwnode from the struct device, we have an API!
device_match_fwnode()

> +		chip->dev = dev;
> +		return 1;
> +	}
> +
> +	return 0;
> +}

...

> +	chip->swnode = swnode;
> +	ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
> +	if (!ret)
> +		return -ENODEV;

Can bus_find_device_by_fwnode() be used here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 03:05:17PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 05, 2023 at 10:24:13AM +0200, Bartosz Golaszewski wrote:

...

> > +	chip->swnode = swnode;
> > +	ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
> > +	if (!ret)
> > +		return -ENODEV;
> 
> Can bus_find_device_by_fwnode() be used here?

Answering to myself: you already mentioned that this should cover any bus,
so the answer is "no".

But also we have device_find_child() if I understood the purpose of the above
it should suit better, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Bartosz Golaszewski]

On Tue, Sep 5, 2023 at 2:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 10:24:13AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We access internals of struct gpio_device and struct gpio_desc because
> > it's easier but it can actually be avoided and we're working towards a
> > better encapsulation of GPIO data structures across the kernel so let's
> > start at home.
> >
> > Instead of checking gpio_desc flags, let's just track the requests of
> > GPIOs in the driver. We also already store the information about
> > direction of simulated lines.
> >
> > For kobjects needed by sysfs callbacks: we can iterate over the children
> > devices of the top-level platform device and compare their fwnodes
> > against the one passed to the init function from probe.
> >
> > While at it: fix one line break and remove the untrue part about
> > configfs callbacks using dev_get_drvdata() from a comment.
>
> ...
>
> > v2 -> v3:
> > - don't use fwnode internal fields, instead: iterate over the platform
> >   device's children and locate the GPIO device
>
> Thank you!
>
> ...
>
> > @@ -181,7 +178,7 @@ static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
>
> >  static int gpio_sim_set_config(struct gpio_chip *gc,
> > -                               unsigned int offset, unsigned long config)
> > +                            unsigned int offset, unsigned long config)
>
> Looking at other prototypes, it can be
>
> static int gpio_sim_set_config(struct gpio_chip *gc, unsigned int offset,
>                                unsigned long config)
>
> ...
>
> > +static int gpio_sim_chip_set_device(struct device *dev, void *data)
> > +{
> > +     struct gpio_sim_chip *chip = data;
>
> > +     if (chip->swnode == dev->fwnode) {
>
> Please do not dereference fwnode from the struct device, we have an API!
> device_match_fwnode()
>
> > +             chip->dev = dev;
> > +             return 1;
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +     chip->swnode = swnode;
> > +     ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
> > +     if (!ret)
> > +             return -ENODEV;
>
> Can bus_find_device_by_fwnode() be used here?
>

I can but then we're iterating over all platform devices and not just
children of this GPIO simulator. If you think it's better for even
less fwnode juggling then I can go with it.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Bartosz Golaszewski]

On Tue, Sep 5, 2023 at 2:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 03:05:17PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 05, 2023 at 10:24:13AM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > +   chip->swnode = swnode;
> > > +   ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
> > > +   if (!ret)
> > > +           return -ENODEV;
> >
> > Can bus_find_device_by_fwnode() be used here?
>
> Answering to myself: you already mentioned that this should cover any bus,
> so the answer is "no".
>

I think I mentioned it under the gpio-consumer where it's true. Here
we are sure it's on the platform bus.

> But also we have device_find_child() if I understood the purpose of the above
> it should suit better, no?
>

Right, it's a better match.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3] gpio: sim: don't fiddle with GPIOLIB private members

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 02:10:38PM +0200, Bartosz Golaszewski wrote:
> On Tue, Sep 5, 2023 at 2:09 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Sep 05, 2023 at 03:05:17PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 05, 2023 at 10:24:13AM +0200, Bartosz Golaszewski wrote:

...

> > > > +   chip->swnode = swnode;
> > > > +   ret = device_for_each_child(dev, chip, gpio_sim_chip_set_device);
> > > > +   if (!ret)
> > > > +           return -ENODEV;
> > >
> > > Can bus_find_device_by_fwnode() be used here?
> >
> > Answering to myself: you already mentioned that this should cover any bus,
> > so the answer is "no".
> 
> I think I mentioned it under the gpio-consumer where it's true. Here
> we are sure it's on the platform bus.

Then bus_find_device_by_fwnode() can be used here, no?

> > But also we have device_find_child() if I understood the purpose of the above
> > it should suit better, no?
> 
> Right, it's a better match.

In either case be careful about reference counting on the returned dev.

-- 
With Best Regards,
Andy Shevchenko