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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/15/23 12:41, Henning Schild wrote:
> 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.
>>
>>>>>  	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.  
> 
> Hi Hans,
> 
>> Ack.
> 
> was that for that last patch or all three of them? I am preparing a v3
> which will include ACKs, i will have to assume p3 only because it came
> in reply to that patch.

You may add my:

Acked-by: Hans de Goede <hdegoede@redhat.com>

To all 3 patches in the series.

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