| Message ID | 20230221122414.24874-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | leds: simatic-ipc-leds-gpio: split up | expand |
On Tue, Feb 21, 2023 at 01:24:12PM +0100, Henning Schild wrote: > There are two special pins needed to init the LEDs. We used to have them > at the end of the gpiod_lookup table to give to "leds-gpio". A cleaner > way is to have a dedicated table for the special pins. ... > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e_extra = { > + .dev_id = NULL, No need. ... > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g_extra = { > + .dev_id = NULL, Ditto.
On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: > In order to clearly describe the dependencies between the gpio GPIO > controller drivers and the users the driver is split up into two and one one --> a > common header. ... > + * Authors: (everywhere where it is a single author, 's' is redundant) ... > +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ This header doesn't look right. Have you run `make W=1 ...` against your patches? Even if it doesn't show defined but unused errors the idea is that this should be a C-file, called, let's say, ...-core.c.
Am Tue, 21 Feb 2023 15:51:03 +0200 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: > > In order to clearly describe the dependencies between the gpio > > GPIO > > > controller drivers and the users the driver is split up into two > > and one > > one --> a > > > common header. > > ... > > > + * Authors: > > (everywhere where it is a single author, 's' is redundant) > > ... > > > +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > > +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > > > +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ > > This header doesn't look right. > > Have you run `make W=1 ...` against your patches? No reports. > Even if it doesn't show defined but unused errors > the idea is that this should be a C-file, called, > let's say, ...-core.c. When i started i kind of had a -common.c in mind as well. But then the header idea came and i gave it a try, expecting questions in the review. It might be a bit unconventional but it seems to do the trick pretty well. Do you see a concrete problem or a violation of a rule? Henning
On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote: > Am Tue, 21 Feb 2023 15:51:03 +0200 > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: > > > In order to clearly describe the dependencies between the gpio ... > > > +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > > > +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > > > > > +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ > > > > This header doesn't look right. > > > > Have you run `make W=1 ...` against your patches? > > No reports. > > > Even if it doesn't show defined but unused errors > > the idea is that this should be a C-file, called, > > let's say, ...-core.c. > > When i started i kind of had a -common.c in mind as well. But then the > header idea came and i gave it a try, expecting questions in the review. > > It might be a bit unconventional but it seems to do the trick pretty > well. Do you see a concrete problem or a violation of a rule? Exactly as described above. The header approach means that *all* static definitions must be used by each user of that file. Otherwise you will get "defined but not used" compiler warning. And approach itself is considered (at least by me) as a hackish way to achieve what usually should be done via C-file. So, if maintainers are okay, I wouldn't have objections, but again I do not think it's a correct approach.
Hi, On 2/21/23 15:51, Andy Shevchenko wrote: > On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote: >> Am Tue, 21 Feb 2023 15:51:03 +0200 >> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: >>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: >>>> In order to clearly describe the dependencies between the gpio > > ... > >>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO >>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO >>> >>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ >>> >>> This header doesn't look right. >>> >>> Have you run `make W=1 ...` against your patches? >> >> No reports. >> >>> Even if it doesn't show defined but unused errors >>> the idea is that this should be a C-file, called, >>> let's say, ...-core.c. >> >> When i started i kind of had a -common.c in mind as well. But then the >> header idea came and i gave it a try, expecting questions in the review. >> >> It might be a bit unconventional but it seems to do the trick pretty >> well. Do you see a concrete problem or a violation of a rule? > > Exactly as described above. The header approach means that *all* static > definitions must be used by each user of that file. Otherwise you will > get "defined but not used" compiler warning. > > And approach itself is considered (at least by me) as a hackish way to > achieve what usually should be done via C-file. > > So, if maintainers are okay, I wouldn't have objections, but again > I do not think it's a correct approach. I agree with Andy here, please add a -common.o file with a shared probe() helper which gets the 2 different gpiod_lookup_table-s as parameter and then put the actual probe() function calling the helper inside the 2 different .c files. And all the: +static struct platform_driver simatic_ipc_led_gpio_driver = { + .probe = simatic_ipc_leds_gpio_probe, + .remove = simatic_ipc_leds_gpio_remove, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +module_platform_driver(simatic_ipc_led_gpio_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); bits should then also go into the 2 different .c file files. Really putting something like module_platform_driver() or MODULE_LICENSE() / MODULE_ALIAS() inside a .h file is just wrong IMHO. Regards, Hans
On Wed, 01 Mar 2023, Hans de Goede wrote: > Hi, > > On 2/21/23 15:51, Andy Shevchenko wrote: > > On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote: > >> Am Tue, 21 Feb 2023 15:51:03 +0200 > >> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: > >>>> In order to clearly describe the dependencies between the gpio > > > > ... > > > >>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > >>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > >>> > >>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ > >>> > >>> This header doesn't look right. > >>> > >>> Have you run `make W=1 ...` against your patches? > >> > >> No reports. > >> > >>> Even if it doesn't show defined but unused errors > >>> the idea is that this should be a C-file, called, > >>> let's say, ...-core.c. > >> > >> When i started i kind of had a -common.c in mind as well. But then the > >> header idea came and i gave it a try, expecting questions in the review. > >> > >> It might be a bit unconventional but it seems to do the trick pretty > >> well. Do you see a concrete problem or a violation of a rule? > > > > Exactly as described above. The header approach means that *all* static > > definitions must be used by each user of that file. Otherwise you will > > get "defined but not used" compiler warning. > > > > And approach itself is considered (at least by me) as a hackish way to > > achieve what usually should be done via C-file. > > > > So, if maintainers are okay, I wouldn't have objections, but again > > I do not think it's a correct approach. > > I agree with Andy here, please add a -common.o file with a shared > probe() helper which gets the 2 different gpiod_lookup_table-s > as parameter and then put the actual probe() function calling > the helper inside the 2 different .c files. Thanks for your reviews guys, I really appreciate the help.
On Wed, Mar 01, 2023 at 04:06:09PM +0000, Lee Jones wrote: > On Wed, 01 Mar 2023, Hans de Goede wrote: ... > Thanks for your reviews guys, I really appreciate the help. You're welcome!
Am Wed, 1 Mar 2023 15:53:04 +0100 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi, > > On 2/21/23 15:51, Andy Shevchenko wrote: > > On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote: > >> Am Tue, 21 Feb 2023 15:51:03 +0200 > >> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote: > >>>> In order to clearly describe the dependencies between the gpio > >>>> > > > > ... > > > >>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > >>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO > >>> > >>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */ > >>> > >>> This header doesn't look right. > >>> > >>> Have you run `make W=1 ...` against your patches? > >> > >> No reports. > >> > >>> Even if it doesn't show defined but unused errors > >>> the idea is that this should be a C-file, called, > >>> let's say, ...-core.c. > >> > >> When i started i kind of had a -common.c in mind as well. But then > >> the header idea came and i gave it a try, expecting questions in > >> the review. > >> > >> It might be a bit unconventional but it seems to do the trick > >> pretty well. Do you see a concrete problem or a violation of a > >> rule? > > > > Exactly as described above. The header approach means that *all* > > static definitions must be used by each user of that file. > > Otherwise you will get "defined but not used" compiler warning. > > > > And approach itself is considered (at least by me) as a hackish way > > to achieve what usually should be done via C-file. > > > > So, if maintainers are okay, I wouldn't have objections, but again > > I do not think it's a correct approach. > > I agree with Andy here, please add a -common.o file with a shared > probe() helper which gets the 2 different gpiod_lookup_table-s > as parameter and then put the actual probe() function calling > the helper inside the 2 different .c files. > > And all the: > > +static struct platform_driver simatic_ipc_led_gpio_driver = { > + .probe = simatic_ipc_leds_gpio_probe, > + .remove = simatic_ipc_leds_gpio_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +module_platform_driver(simatic_ipc_led_gpio_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > bits should then also go into the 2 different .c file files. > > Really putting something like module_platform_driver() or > MODULE_LICENSE() / MODULE_ALIAS() inside a .h file is > just wrong IMHO. Thanks for getting back, after Andys review i happen to have just that already prepared for v2 as "likely needed". Will send that v2 soon. Henning > Regards, > > Hans >
This series mainly splits the one GPIO driver into two. The split allows to clearly model runtime and compile time dependencies on the GPIO chip drivers. p1 is kind of not too related to that split but also prepares for more GPIO based drivers to come. p2 takes the driver we had and puts most of its content into a header, to be included by two drivers. p3 deals with more fine-grained configuration posibilities and compile time dependencies. It is based on [PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO providing driver Henning Schild (3): leds: simatic-ipc-leds-gpio: move two extra gpio pins into another table leds: simatic-ipc-leds-gpio: split up into multiple drivers leds: simatic-ipc-leds-gpio: introduce more Kconfig switches drivers/leds/simple/Kconfig | 31 ++++++++- drivers/leds/simple/Makefile | 5 +- .../simple/simatic-ipc-leds-gpio-apollolake.c | 34 ++++++++++ .../simple/simatic-ipc-leds-gpio-f7188x.c | 34 ++++++++++ ...pc-leds-gpio.c => simatic-ipc-leds-gpio.h} | 67 ++++++------------- drivers/platform/x86/simatic-ipc.c | 7 +- 6 files changed, 123 insertions(+), 55 deletions(-) create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c rename drivers/leds/simple/{simatic-ipc-leds-gpio.c => simatic-ipc-leds-gpio.h} (51%)