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

[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.

>>>  	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
>>
>