diff mbox series

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

Message ID 20230907082751.22996-1-brgl@bgdev.pl
State Superseded
Headers show
Series [v4] gpio: sim: don't fiddle with GPIOLIB private members | expand

Commit Message

Bartosz Golaszewski Sept. 7, 2023, 8:27 a.m. UTC 
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

v3 -> v4:
- use device_find_child() (not bus_device_find_by_fwnode() as it's
  pointless to iterate over all platform devices)
- use device_match_fwnode() for matching the device by fwnode

 drivers/gpio/gpio-sim.c | 75 ++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 19 deletions(-)

Comments

Andy Shevchenko Sept. 7, 2023, 2:13 p.m. UTC | #1 
On Thu, Sep 07, 2023 at 10:27:51AM +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.

Will LGTM with the couple of remarks being addressed.

...

>  #include <linux/completion.h>
>  #include <linux/configfs.h>
>  #include <linux/device.h>

> +#include <linux/device/bus.h>

No need, the device.h guarantees that.

...

> +static int gpio_sim_dev_match_fwnode(struct device *dev, void *data)
> +{
> +	/*
> +	 * We can't pass this directly to device_find_child() due to pointer
> +	 * type mismatch.
> +	 */

Not sure if this comment adds any value.

> +	return device_match_fwnode(dev, data);
> +}
Bartosz Golaszewski Sept. 8, 2023, 12:39 p.m. UTC | #2 
On Thu, Sep 7, 2023 at 4:13 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Sep 07, 2023 at 10:27:51AM +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.
>
> Will LGTM with the couple of remarks being addressed.
>
> ...
>
> >  #include <linux/completion.h>
> >  #include <linux/configfs.h>
> >  #include <linux/device.h>
>
> > +#include <linux/device/bus.h>
>
> No need, the device.h guarantees that.
>

Wait, wasn't you the one who always suggests including headers
directly if we're using any symbols defined in them? Like when I said
that we don't need to include linux/notifier.h because it's already
included in gpiolib.h and you argued the opposite? :)

device_match_fwnode() is defined in linux/device/bus.h so I thought
it's in order to include it.

> ...
>
> > +static int gpio_sim_dev_match_fwnode(struct device *dev, void *data)
> > +{
> > +     /*
> > +      * We can't pass this directly to device_find_child() due to pointer
> > +      * type mismatch.
> > +      */
>
> Not sure if this comment adds any value.
>

I disagree - I would have used device_match_fwnode() as argument
passed directly to device_find_child() but I cannot due to pointer
type mismatch error so we need this wrapper and it's useful to say
why.

Bart

> > +     return device_match_fwnode(dev, data);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko Sept. 11, 2023, 10 a.m. UTC | #3 
On Fri, Sep 08, 2023 at 02:39:28PM +0200, Bartosz Golaszewski wrote:
> On Thu, Sep 7, 2023 at 4:13 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Sep 07, 2023 at 10:27:51AM +0200, Bartosz Golaszewski wrote:

...

> > >  #include <linux/completion.h>
> > >  #include <linux/configfs.h>
> > >  #include <linux/device.h>
> >
> > > +#include <linux/device/bus.h>
> >
> > No need, the device.h guarantees that.
> 
> Wait, wasn't you the one who always suggests including headers
> directly if we're using any symbols defined in them? Like when I said
> that we don't need to include linux/notifier.h because it's already
> included in gpiolib.h and you argued the opposite? :)
> 
> device_match_fwnode() is defined in linux/device/bus.h so I thought
> it's in order to include it.

Yes, but I am not radical with it, I am for a compromise when some headers
guarantee to include some others. That is the case I believe, I don't think
device.h ever will be broken to the parts that are not include each other
(too many things to change right now, if it happens, not in the feasible
 future).

...

> > > +static int gpio_sim_dev_match_fwnode(struct device *dev, void *data)
> > > +{
> > > +     /*
> > > +      * We can't pass this directly to device_find_child() due to pointer
> > > +      * type mismatch.
> > > +      */
> >
> > Not sure if this comment adds any value.
> 
> I disagree - I would have used device_match_fwnode() as argument
> passed directly to device_find_child() but I cannot due to pointer
> type mismatch error so we need this wrapper and it's useful to say
> why.

Yes, and we have dozen(s ?) of the similar wrappers without a comment.
So, I'm still for removing it.

> > > +     return device_match_fwnode(dev, data);
> > > +}
Bartosz Golaszewski Sept. 11, 2023, 10:48 a.m. UTC | #4 
On Mon, Sep 11, 2023 at 12:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 08, 2023 at 02:39:28PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Sep 7, 2023 at 4:13 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Sep 07, 2023 at 10:27:51AM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > >  #include <linux/completion.h>
> > > >  #include <linux/configfs.h>
> > > >  #include <linux/device.h>
> > >
> > > > +#include <linux/device/bus.h>
> > >
> > > No need, the device.h guarantees that.
> >
> > Wait, wasn't you the one who always suggests including headers
> > directly if we're using any symbols defined in them? Like when I said
> > that we don't need to include linux/notifier.h because it's already
> > included in gpiolib.h and you argued the opposite? :)
> >
> > device_match_fwnode() is defined in linux/device/bus.h so I thought
> > it's in order to include it.
>
> Yes, but I am not radical with it, I am for a compromise when some headers
> guarantee to include some others. That is the case I believe, I don't think
> device.h ever will be broken to the parts that are not include each other
> (too many things to change right now, if it happens, not in the feasible
>  future).
>
> ...
>
> > > > +static int gpio_sim_dev_match_fwnode(struct device *dev, void *data)
> > > > +{
> > > > +     /*
> > > > +      * We can't pass this directly to device_find_child() due to pointer
> > > > +      * type mismatch.
> > > > +      */
> > >
> > > Not sure if this comment adds any value.
> >
> > I disagree - I would have used device_match_fwnode() as argument
> > passed directly to device_find_child() but I cannot due to pointer
> > type mismatch error so we need this wrapper and it's useful to say
> > why.
>
> Yes, and we have dozen(s ?) of the similar wrappers without a comment.
> So, I'm still for removing it.
>

Meh, fair enough.

Bart
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 271db3639a78..a58387f2cdc6 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -12,6 +12,8 @@ 
 #include <linux/completion.h>
 #include <linux/configfs.h>
 #include <linux/device.h>
+#include <linux/device/bus.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/idr.h>
@@ -30,8 +32,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 +40,8 @@  static DEFINE_IDA(gpio_sim_ida);
 
 struct gpio_sim_chip {
 	struct gpio_chip gc;
+	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:
@@ -180,8 +177,8 @@  static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
 	return direction ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
 }
 
-static int gpio_sim_set_config(struct gpio_chip *gc,
-				  unsigned int offset, unsigned long config)
+static int gpio_sim_set_config(struct gpio_chip *gc, 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,
@@ -282,6 +291,13 @@  static void gpio_sim_mutex_destroy(void *data)
 	mutex_destroy(lock);
 }
 
+static void gpio_sim_put_device(void *data)
+{
+	struct device *dev = data;
+
+	put_device(dev);
+}
+
 static void gpio_sim_dispose_mappings(void *data)
 {
 	struct gpio_sim_chip *chip = data;
@@ -295,7 +311,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 +368,22 @@  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_dev_match_fwnode(struct device *dev, void *data)
+{
+	/*
+	 * We can't pass this directly to device_find_child() due to pointer
+	 * type mismatch.
+	 */
+	return device_match_fwnode(dev, data);
+}
+
 static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 {
 	struct gpio_sim_chip *chip;
@@ -387,6 +411,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 +460,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 +468,16 @@  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->dev = device_find_child(dev, swnode, gpio_sim_dev_match_fwnode);
+	if (!chip->dev)
+		return -ENODEV;
+
+	ret = devm_add_action_or_reset(dev, gpio_sim_put_device, chip->dev);
+	if (ret)
+		return ret;
+
+	/* Used by sysfs callbacks. */
+	dev_set_drvdata(chip->dev, chip);
 
 	return gpio_sim_setup_sysfs(chip);
 }