[PATCH v1 0/3] gpiolib: cleanups WRT GPIO device handling

[PATCH v1 0/3] gpiolib: cleanups WRT GPIO device handling

[Andy Shevchenko]

A few cleanups to GPIO device handling in the library code.

Andy Shevchenko (3):
  gpiolib: Access device's fwnode via dev_fwnode()
  gpiolib: Get rid of gpio_bus_match() forward declaration
  gpiolib: Move gpiodevice_*() to gpiodev namespace

 drivers/gpio/gpiolib.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

-- 
2.39.1

[PATCH v1 1/3] gpiolib: Access device's fwnode via dev_fwnode()

[Andy Shevchenko]

GPIO device's fwnode should be accessed via dev_fwnode().
Make sure that gpiochip_setup_dev() follows that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b554ad435245..c7f35f0e7d15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -589,14 +589,15 @@ static void gpiodevice_release(struct device *dev)
 
 static int gpiochip_setup_dev(struct gpio_device *gdev)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
 	int ret;
 
 	/*
 	 * If fwnode doesn't belong to another device, it's safe to clear its
 	 * initialized flag.
 	 */
-	if (gdev->dev.fwnode && !gdev->dev.fwnode->dev)
-		fwnode_dev_initialized(gdev->dev.fwnode, false);
+	if (fwnode && !fwnode->dev)
+		fwnode_dev_initialized(fwnode, false);
 
 	ret = gcdev_register(gdev, gpio_devt);
 	if (ret)
-- 
2.39.1

Re: [PATCH v1 1/3] gpiolib: Access device's fwnode via dev_fwnode()

[Linus Walleij]

On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> GPIO device's fwnode should be accessed via dev_fwnode().
> Make sure that gpiochip_setup_dev() follows that.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 2/3] gpiolib: Get rid of gpio_bus_match() forward declaration

[Andy Shevchenko]

There is nothing specific about gpio_bus_match(), so we may
simply move it to the top of the file and get rid of forward
declaration.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c7f35f0e7d15..a44a1b0f2766 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -62,7 +62,20 @@
 static DEFINE_IDA(gpio_ida);
 static dev_t gpio_devt;
 #define GPIO_DEV_MAX 256 /* 256 GPIO chip devices supported */
-static int gpio_bus_match(struct device *dev, struct device_driver *drv);
+
+static int gpio_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	/*
+	 * Only match if the fwnode doesn't already have a proper struct device
+	 * created for it.
+	 */
+	if (fwnode && fwnode->dev != dev)
+		return 0;
+	return 1;
+}
+
 static struct bus_type gpio_bus_type = {
 	.name = "gpio",
 	.match = gpio_bus_match,
@@ -4417,20 +4430,6 @@ void gpiod_put_array(struct gpio_descs *descs)
 }
 EXPORT_SYMBOL_GPL(gpiod_put_array);
 
-
-static int gpio_bus_match(struct device *dev, struct device_driver *drv)
-{
-	struct fwnode_handle *fwnode = dev_fwnode(dev);
-
-	/*
-	 * Only match if the fwnode doesn't already have a proper struct device
-	 * created for it.
-	 */
-	if (fwnode && fwnode->dev != dev)
-		return 0;
-	return 1;
-}
-
 static int gpio_stub_drv_probe(struct device *dev)
 {
 	/*
-- 
2.39.1

Re: [PATCH v1 2/3] gpiolib: Get rid of gpio_bus_match() forward declaration

[Linus Walleij]

On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> There is nothing specific about gpio_bus_match(), so we may
> simply move it to the top of the file and get rid of forward
> declaration.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Andy Shevchenko]

The functions that operates on the same device object would
have the same namespace for better code understanding and
maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a44a1b0f2766..45f79aee451a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -573,7 +573,7 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 
-static void gpiodevice_release(struct device *dev)
+static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
 	unsigned long flags;
@@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 		return ret;
 
 	/* From this point, the .release() function cleans up gpio_device */
-	gdev->dev.release = gpiodevice_release;
+	gdev->dev.release = gpiodev_release;
 
 	ret = gpiochip_sysfs_register(gdev);
 	if (ret)
-- 
2.39.1

Re: [PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Bartosz Golaszewski]

On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The functions that operates on the same device object would
> have the same namespace for better code understanding and
> maintenance.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a44a1b0f2766..45f79aee451a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -573,7 +573,7 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
>
> -static void gpiodevice_release(struct device *dev)
> +static void gpiodev_release(struct device *dev)
>  {
>         struct gpio_device *gdev = to_gpio_device(dev);
>         unsigned long flags;
> @@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
>                 return ret;
>
>         /* From this point, the .release() function cleans up gpio_device */
> -       gdev->dev.release = gpiodevice_release;
> +       gdev->dev.release = gpiodev_release;
>
>         ret = gpiochip_sysfs_register(gdev);
>         if (ret)
> --
> 2.39.1
>

But the only other function that's in the gpiodev_ namespace operates
on struct gpio_device so that change doesn't make much sense to me.

Bart

Re: [PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Andy Shevchenko]

On Wed, Mar 08, 2023 at 11:49:53AM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The functions that operates on the same device object would
> > have the same namespace for better code understanding and
> > maintenance.

...

> > -static void gpiodevice_release(struct device *dev)
> > +static void gpiodev_release(struct device *dev)
> >  {
> >         struct gpio_device *gdev = to_gpio_device(dev);
> >         unsigned long flags;
> > @@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> >                 return ret;
> >
> >         /* From this point, the .release() function cleans up gpio_device */
> > -       gdev->dev.release = gpiodevice_release;
> > +       gdev->dev.release = gpiodev_release;
> >
> >         ret = gpiochip_sysfs_register(gdev);
> >         if (ret)

> But the only other function that's in the gpiodev_ namespace operates
> on struct gpio_device so that change doesn't make much sense to me.

I'm not sure I understood the comment.
After this change we will have

static int gpiodev_add_to_list(struct gpio_device *gdev)
static void gpiodev_release(struct device *dev)

There are also gpio_device_*() I have noticed, so may be these should be
actually in that namespace?

And we have

static int gpiochip_setup_dev(struct gpio_device *gdev)
static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)

That said, what do you think is the best to make this more consistent?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Bartosz Golaszewski]

On Thu, Mar 9, 2023 at 7:52 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 08, 2023 at 11:49:53AM +0100, Bartosz Golaszewski wrote:
> > On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > The functions that operates on the same device object would
> > > have the same namespace for better code understanding and
> > > maintenance.
>
> ...
>
> > > -static void gpiodevice_release(struct device *dev)
> > > +static void gpiodev_release(struct device *dev)
> > >  {
> > >         struct gpio_device *gdev = to_gpio_device(dev);
> > >         unsigned long flags;
> > > @@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> > >                 return ret;
> > >
> > >         /* From this point, the .release() function cleans up gpio_device */
> > > -       gdev->dev.release = gpiodevice_release;
> > > +       gdev->dev.release = gpiodev_release;
> > >
> > >         ret = gpiochip_sysfs_register(gdev);
> > >         if (ret)
>
> > But the only other function that's in the gpiodev_ namespace operates
> > on struct gpio_device so that change doesn't make much sense to me.
>
> I'm not sure I understood the comment.
> After this change we will have
>
> static int gpiodev_add_to_list(struct gpio_device *gdev)
> static void gpiodev_release(struct device *dev)
>

Do you want to use the same prefix for both because struct device in
the latter is embedded in struct gpio_device in the former?

Bart

> There are also gpio_device_*() I have noticed, so may be these should be
> actually in that namespace?
>
> And we have
>
> static int gpiochip_setup_dev(struct gpio_device *gdev)
> static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
>
> That said, what do you think is the best to make this more consistent?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Andy Shevchenko]

On Fri, Mar 10, 2023 at 05:48:39PM +0100, Bartosz Golaszewski wrote:
> On Thu, Mar 9, 2023 at 7:52 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 08, 2023 at 11:49:53AM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > The functions that operates on the same device object would
> > > > have the same namespace for better code understanding and
> > > > maintenance.

...

> > > > -static void gpiodevice_release(struct device *dev)
> > > > +static void gpiodev_release(struct device *dev)
> > > >  {
> > > >         struct gpio_device *gdev = to_gpio_device(dev);
> > > >         unsigned long flags;
> > > > @@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> > > >                 return ret;
> > > >
> > > >         /* From this point, the .release() function cleans up gpio_device */
> > > > -       gdev->dev.release = gpiodevice_release;
> > > > +       gdev->dev.release = gpiodev_release;
> > > >
> > > >         ret = gpiochip_sysfs_register(gdev);
> > > >         if (ret)
> >
> > > But the only other function that's in the gpiodev_ namespace operates
> > > on struct gpio_device so that change doesn't make much sense to me.
> >
> > I'm not sure I understood the comment.
> > After this change we will have
> >
> > static int gpiodev_add_to_list(struct gpio_device *gdev)
> > static void gpiodev_release(struct device *dev)
> >
> 
> Do you want to use the same prefix for both because struct device in
> the latter is embedded in struct gpio_device in the former?

Yes, the logic to have logically grouped namespace for these APIs.
Meaning on what they are taking as an effective object to proceed
with.

> > There are also gpio_device_*() I have noticed, so may be these should be
> > actually in that namespace?
> >
> > And we have
> >
> > static int gpiochip_setup_dev(struct gpio_device *gdev)
> > static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
> >
> > That said, what do you think is the best to make this more consistent?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] gpiolib: Move gpiodevice_*() to gpiodev namespace

[Bartosz Golaszewski]

On Fri, Mar 10, 2023 at 6:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Mar 10, 2023 at 05:48:39PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Mar 9, 2023 at 7:52 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Mar 08, 2023 at 11:49:53AM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > The functions that operates on the same device object would
> > > > > have the same namespace for better code understanding and
> > > > > maintenance.
>
> ...
>
> > > > > -static void gpiodevice_release(struct device *dev)
> > > > > +static void gpiodev_release(struct device *dev)
> > > > >  {
> > > > >         struct gpio_device *gdev = to_gpio_device(dev);
> > > > >         unsigned long flags;
> > > > > @@ -617,7 +617,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> > > > >                 return ret;
> > > > >
> > > > >         /* From this point, the .release() function cleans up gpio_device */
> > > > > -       gdev->dev.release = gpiodevice_release;
> > > > > +       gdev->dev.release = gpiodev_release;
> > > > >
> > > > >         ret = gpiochip_sysfs_register(gdev);
> > > > >         if (ret)
> > >
> > > > But the only other function that's in the gpiodev_ namespace operates
> > > > on struct gpio_device so that change doesn't make much sense to me.
> > >
> > > I'm not sure I understood the comment.
> > > After this change we will have
> > >
> > > static int gpiodev_add_to_list(struct gpio_device *gdev)
> > > static void gpiodev_release(struct device *dev)
> > >
> >
> > Do you want to use the same prefix for both because struct device in
> > the latter is embedded in struct gpio_device in the former?
>
> Yes, the logic to have logically grouped namespace for these APIs.
> Meaning on what they are taking as an effective object to proceed
> with.
>

I don't have a better idea so applied it.

Bart

> > > There are also gpio_device_*() I have noticed, so may be these should be
> > > actually in that namespace?
> > >
> > > And we have
> > >
> > > static int gpiochip_setup_dev(struct gpio_device *gdev)
> > > static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
> > >
> > > That said, what do you think is the best to make this more consistent?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v1 0/3] gpiolib: cleanups WRT GPIO device handling

[Bartosz Golaszewski]

On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> A few cleanups to GPIO device handling in the library code.
>
> Andy Shevchenko (3):
>   gpiolib: Access device's fwnode via dev_fwnode()
>   gpiolib: Get rid of gpio_bus_match() forward declaration
>   gpiolib: Move gpiodevice_*() to gpiodev namespace
>
>  drivers/gpio/gpiolib.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> --
> 2.39.1
>

I applied the first two patches, for the third I have a comment.

Bart

Re: [PATCH v1 0/3] gpiolib: cleanups WRT GPIO device handling

[Andy Shevchenko]

On Wed, Mar 08, 2023 at 11:50:49AM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 7, 2023 at 7:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > A few cleanups to GPIO device handling in the library code.
> >
> > Andy Shevchenko (3):
> >   gpiolib: Access device's fwnode via dev_fwnode()
> >   gpiolib: Get rid of gpio_bus_match() forward declaration
> >   gpiolib: Move gpiodevice_*() to gpiodev namespace

> I applied the first two patches, for the third I have a comment.

Thank you!

-- 
With Best Regards,
Andy Shevchenko