| Message ID | 20230307182557.42215-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 7aa90f9055c16c79c0ad174c726f95723d122fe6 |
| Headers | show |
| Series | gpiolib: cleanups WRT GPIO device handling | expand |
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?
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 > >
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?
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 > >
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)
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(-)