Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild <henning.schild@siemens.com>]

Am Thu, 2 Mar 2023 10:02:51 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/2/23 09:47, Henning Schild wrote:
> > Am Wed, 1 Mar 2023 19:04:01 +0100
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> >   
> >> Hi,
> >>
> >> On 3/1/23 18:02, Henning Schild wrote:  
> >>> To describe the dependency chain better and allow for potential
> >>> fine-grained config tuning, introduce Kconfig switch for the
> >>> individual GPIO based drivers.
> >>>
> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >>> ---
> >>>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
> >>>  drivers/leds/simple/Makefile |  7 +++----
> >>>  2 files changed, 31 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/leds/simple/Kconfig
> >>> b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
> >>> 100644 --- a/drivers/leds/simple/Kconfig
> >>> +++ b/drivers/leds/simple/Kconfig
> >>> @@ -1,11 +1,36 @@
> >>>  # SPDX-License-Identifier: GPL-2.0-only
> >>>  config LEDS_SIEMENS_SIMATIC_IPC
> >>>  	tristate "LED driver for Siemens Simatic IPCs"
> >>> -	depends on LEDS_GPIO    
> >>
> >> Since it is simatic-ipc-leds-gpio-core.c which actually registers
> >> the LEDs GPIO platform device, this one should stay IMHO.  
> > 
> > No this one is now only for the port-IO based driver. The GPIO core
> > is built under the two new switches only.  
> 
> You are right, I thought this would enable building
> simatic-ipc-leds-gpio-core.o into its own .ko which would
> then be used by both other gpio LED drivers. But upon a closer
> look at the Makefile changes I see that is not the case.
> 
> Note that with your current solution you are linking that into
> the kernel twice.
> 
> As long this is build into modules that is fine. But if both are
> builtin I *think* you may get linker errors because of duplicate
> symbols?
> 
> I believe this is why Andy asked to try a build with all 3 options
> set to Y.

Thanks. There are no linker errors so i think there is no need for yet
another CONFIG especially for the core. I would try to keep it simple
as the name of the directory suggests, so go with what we have.

Henning

> 
> >>>  	depends on SIEMENS_SIMATIC_IPC
> >>>  	help
> >>>  	  This option enables support for the LEDs of several
> >>> Industrial PCs from Siemens.
> >>>  
> >>> -	  To compile this driver as a module, choose M here: the
> >>> modules
> >>> -	  will be called simatic-ipc-leds and
> >>> simatic-ipc-leds-gpio.
> >>> +	  To compile this driver as a module, choose M here: the
> >>> module
> >>> +	  will be called simatic-ipc-leds.
> >>> +
> >>> +config LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE
> >>> +	tristate "LED driver for Siemens Simatic IPCs based on
> >>> Intel Apollo Lake GPIO"
> >>> +	depends on LEDS_GPIO    
> >>
> >> And then it can be dropped here.
> >>  
> >>> +	depends on PINCTRL_BROXTON    
> >>  
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
> >> actually uses symbol from that module.  
> > 
> > Same as above, the GPIO based drivers do not depend on the port-IO
> > based driver.  
> 
> Ack.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> >>> +	default LEDS_SIEMENS_SIMATIC_IPC
> >>> +	help
> >>> +	  This option enables support for the LEDs of several
> >>> Industrial PCs
> >>> +	  from Siemens based on Apollo Lake GPIO i.e. IPC127E.
> >>> +
> >>> +	  To compile this driver as a module, choose M here: the
> >>> module
> >>> +	  will be called simatic-ipc-leds-gpio-apollolake.
> >>> +
> >>> +config LEDS_SIEMENS_SIMATIC_IPC_F7188X
> >>> +	tristate "LED driver for Siemens Simatic IPCs based on
> >>> Nuvoton GPIO"
> >>> +	depends on LEDS_GPIO    
> >>
> >> Idem.
> >>  
> >>> +	depends on GPIO_F7188X
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> Idem.
> >>  
> >>> +	default LEDS_SIEMENS_SIMATIC_IPC
> >>> +	help
> >>> +	  This option enables support for the LEDs of several
> >>> Industrial PCs
> >>> +	  from Siemens based on Nuvoton GPIO i.e. IPC227G.
> >>> +
> >>> +	  To compile this driver as a module, choose M here: the
> >>> module
> >>> +	  will be called simatic-ipc-leds-gpio-f7188x.
> >>> diff --git a/drivers/leds/simple/Makefile
> >>> b/drivers/leds/simple/Makefile index ed9057f7b6da..e3e840cea275
> >>> 100644 --- a/drivers/leds/simple/Makefile
> >>> +++ b/drivers/leds/simple/Makefile
> >>> @@ -1,5 +1,4 @@
> >>>  # SPDX-License-Identifier: GPL-2.0
> >>> -obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> >>> simatic-ipc-leds.o -obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)  +=
> >>> simatic-ipc-leds-gpio-core.o
> >>> -obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> >>> simatic-ipc-leds-gpio-apollolake.o
> >>> -obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> >>> simatic-ipc-leds-gpio-f7188x.o
> >>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)			+=
> >>> simatic-ipc-leds.o
> >>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE)	+=
> >>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-apollolake.o
> >>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
> >>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o    
> >>
> >> Regards,
> >>
> >> Hans
> >>  
> >   
>