mbox series

[0/3] platform/x86: move simatic ipc drivers into subdir

Message ID 20230718105213.1275-1-henning.schild@siemens.com
Headers show
Series platform/x86: move simatic ipc drivers into subdir | expand

Message

Henning Schild July 18, 2023, 10:52 a.m. UTC
This series does two things. It builds up a Kconfig inheritance chain
for all platform device drivers, namely Watchdog and LED. And then it
puts all Siemens Simatic IPC drivers in the platform/x86/ directory in
a subdirectory called "siemens". Where we create a new Kconfig item to
allow users to centrally enable support for Siemens devices, which will
pull in the rest.

Henning Schild (3):
  watchdog: make Siemens Simatic watchdog driver default on platform
  leds: simatic-ipc-leds: default config switch to platform switch
  platform/x86: Move all simatic ipc drivers to the subdirectory siemens

 drivers/leds/simple/Kconfig                   |  1 +
 drivers/platform/x86/Kconfig                  | 59 +-------------
 drivers/platform/x86/Makefile                 |  6 +-
 drivers/platform/x86/siemens/Kconfig          | 77 +++++++++++++++++++
 drivers/platform/x86/siemens/Makefile         | 11 +++
 .../simatic-ipc-batt-apollolake.c             |  0
 .../simatic-ipc-batt-elkhartlake.c            |  0
 .../{ => siemens}/simatic-ipc-batt-f7188x.c   |  0
 .../x86/{ => siemens}/simatic-ipc-batt.c      |  0
 .../x86/{ => siemens}/simatic-ipc-batt.h      |  0
 .../platform/x86/{ => siemens}/simatic-ipc.c  |  0
 drivers/watchdog/Kconfig                      |  1 +
 12 files changed, 92 insertions(+), 63 deletions(-)
 create mode 100644 drivers/platform/x86/siemens/Kconfig
 create mode 100644 drivers/platform/x86/siemens/Makefile
 rename drivers/platform/x86/{ => siemens}/simatic-ipc-batt-apollolake.c (100%)
 rename drivers/platform/x86/{ => siemens}/simatic-ipc-batt-elkhartlake.c (100%)
 rename drivers/platform/x86/{ => siemens}/simatic-ipc-batt-f7188x.c (100%)
 rename drivers/platform/x86/{ => siemens}/simatic-ipc-batt.c (100%)
 rename drivers/platform/x86/{ => siemens}/simatic-ipc-batt.h (100%)
 rename drivers/platform/x86/{ => siemens}/simatic-ipc.c (100%)

Comments

Andy Shevchenko July 18, 2023, 2:20 p.m. UTC | #1
On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
> If a user did choose to enable Siemens Simatic platform support they
> likely want that driver to be enabled without having to flip more config
> switches. So we make the watchdog driver config switch default to the
> platform driver switches value.

A nit-pick below.

...

>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
>  	depends on SIEMENS_SIMATIC_IPC

> +	default SIEMENS_SIMATIC_IPC

It's more natural to group tristate and default, vs. depends and select.

>  	select WATCHDOG_CORE
>  	select P2SB
Andy Shevchenko July 18, 2023, 2:21 p.m. UTC | #2
On Tue, Jul 18, 2023 at 12:52:12PM +0200, Henning Schild wrote:
> If a user did choose to enable Siemens Simatic platform support they
> likely want the LED drivers to be enabled without having to flip more
> config switches. So we make the LED drivers config switch default to
> the platform driver switches value.

Same as per previous patch.
Henning Schild July 18, 2023, 2:42 p.m. UTC | #3
Am Tue, 18 Jul 2023 17:20:48 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
> > If a user did choose to enable Siemens Simatic platform support they
> > likely want that driver to be enabled without having to flip more
> > config switches. So we make the watchdog driver config switch
> > default to the platform driver switches value.  
> 
> A nit-pick below.
> 
> ...
> 
> >  config SIEMENS_SIMATIC_IPC_WDT
> >  	tristate "Siemens Simatic IPC Watchdog"
> >  	depends on SIEMENS_SIMATIC_IPC  
> 
> > +	default SIEMENS_SIMATIC_IPC  
> 
> It's more natural to group tristate and default, vs. depends and
> select.

Will be ignored unless maintainer insists.

Henning

> 
> >  	select WATCHDOG_CORE
> >  	select P2SB  
>
Henning Schild July 18, 2023, 2:43 p.m. UTC | #4
Am Tue, 18 Jul 2023 17:21:04 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Jul 18, 2023 at 12:52:12PM +0200, Henning Schild wrote:
> > If a user did choose to enable Siemens Simatic platform support they
> > likely want the LED drivers to be enabled without having to flip
> > more config switches. So we make the LED drivers config switch
> > default to the platform driver switches value.  
> 
> Same as per previous patch.
> 

Same as per previous patch.
Guenter Roeck July 18, 2023, 3:07 p.m. UTC | #5
On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
> If a user did choose to enable Siemens Simatic platform support they
> likely want that driver to be enabled without having to flip more config
> switches. So we make the watchdog driver config switch default to the
> platform driver switches value.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/watchdog/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index ee97d89dfc11..ccdbd1109a32 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1681,6 +1681,7 @@ config NIC7018_WDT
>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
>  	depends on SIEMENS_SIMATIC_IPC
> +	default SIEMENS_SIMATIC_IPC

Why not just "default y" ? That does the same (it will be set to m if
SIEMENS_SIMATIC_IPC=m) without the complexity.

Guenter

>  	select WATCHDOG_CORE
>  	select P2SB
>  	help
> -- 
> 2.41.0
>
Guenter Roeck July 18, 2023, 3:10 p.m. UTC | #6
On 7/18/23 07:42, Henning Schild wrote:
> Am Tue, 18 Jul 2023 17:20:48 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
>> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
>>> If a user did choose to enable Siemens Simatic platform support they
>>> likely want that driver to be enabled without having to flip more
>>> config switches. So we make the watchdog driver config switch
>>> default to the platform driver switches value.
>>
>> A nit-pick below.
>>
>> ...
>>
>>>   config SIEMENS_SIMATIC_IPC_WDT
>>>   	tristate "Siemens Simatic IPC Watchdog"
>>>   	depends on SIEMENS_SIMATIC_IPC
>>
>>> +	default SIEMENS_SIMATIC_IPC
>>
>> It's more natural to group tristate and default, vs. depends and
>> select.
> 
> Will be ignored unless maintainer insists.
> 

Maintainer wants to know why "default SIEMENS_SIMATIC_IPC" is needed
or warranted instead of the much simpler and easier to understand
"default y".

Guenter
Henning Schild July 19, 2023, 7:18 a.m. UTC | #7
Am Tue, 18 Jul 2023 08:10:09 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 7/18/23 07:42, Henning Schild wrote:
> > Am Tue, 18 Jul 2023 17:20:48 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> >> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:  
> >>> If a user did choose to enable Siemens Simatic platform support
> >>> they likely want that driver to be enabled without having to flip
> >>> more config switches. So we make the watchdog driver config switch
> >>> default to the platform driver switches value.  
> >>
> >> A nit-pick below.
> >>
> >> ...
> >>  
> >>>   config SIEMENS_SIMATIC_IPC_WDT
> >>>   	tristate "Siemens Simatic IPC Watchdog"
> >>>   	depends on SIEMENS_SIMATIC_IPC  
> >>  
> >>> +	default SIEMENS_SIMATIC_IPC  
> >>
> >> It's more natural to group tristate and default, vs. depends and
> >> select.  
> > 
> > Will be ignored unless maintainer insists.
> >   
> 
> Maintainer wants to know why "default SIEMENS_SIMATIC_IPC" is needed
> or warranted instead of the much simpler and easier to understand
> "default y".

I thought a "default y" or "default m" was maybe not the best idea for
a platform that is not super common. That is why i did not dare to even
think about defaulting any of the Simatic stuff to not-no.

But it seems that this would be ok after all. And i would be very happy
to do so because it means less work on distro configs.

SIEMENS_SIMATIC_IPC_WDT will drive a platform device which gets
registered by SIEMENS_SIMATIC_IPC and nothing else. That is why
"default SIEMENS_SIMATIC_IPC" was chosen.

But if i may i would change that to "default m", not "y" because there
is an out of tree driver package which if installed on top, should be
able to override the in-tree drivers.

So i will go ahead and make that one "default m"

regards,
Henning

> Guenter
>
Henning Schild July 19, 2023, 7:20 a.m. UTC | #8
Am Tue, 18 Jul 2023 08:07:52 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
> > If a user did choose to enable Siemens Simatic platform support they
> > likely want that driver to be enabled without having to flip more
> > config switches. So we make the watchdog driver config switch
> > default to the platform driver switches value.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/watchdog/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index ee97d89dfc11..ccdbd1109a32 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1681,6 +1681,7 @@ config NIC7018_WDT
> >  config SIEMENS_SIMATIC_IPC_WDT
> >  	tristate "Siemens Simatic IPC Watchdog"
> >  	depends on SIEMENS_SIMATIC_IPC
> > +	default SIEMENS_SIMATIC_IPC  
> 
> Why not just "default y" ? That does the same (it will be set to m if
> SIEMENS_SIMATIC_IPC=m) without the complexity.

I see. Thanks! In that case i will go for "default y" and not "default
m" which i wrote about in the other mail.

Henning

> Guenter
> 
> >  	select WATCHDOG_CORE
> >  	select P2SB
> >  	help
> > -- 
> > 2.41.0
> >
Lee Jones July 19, 2023, 8:43 a.m. UTC | #9
On Tue, 18 Jul 2023, Henning Schild wrote:

> Am Tue, 18 Jul 2023 17:21:04 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > On Tue, Jul 18, 2023 at 12:52:12PM +0200, Henning Schild wrote:
> > > If a user did choose to enable Siemens Simatic platform support they
> > > likely want the LED drivers to be enabled without having to flip
> > > more config switches. So we make the LED drivers config switch
> > > default to the platform driver switches value.  
> > 
> > Same as per previous patch.
> > 
> 
> Same as per previous patch.

Same as per previous patch.
Henning Schild July 19, 2023, 10:54 a.m. UTC | #10
Am Wed, 19 Jul 2023 09:43:01 +0100
schrieb Lee Jones <lee@kernel.org>:

> On Tue, 18 Jul 2023, Henning Schild wrote:
> 
> > Am Tue, 18 Jul 2023 17:21:04 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> > > On Tue, Jul 18, 2023 at 12:52:12PM +0200, Henning Schild wrote:  
> > > > If a user did choose to enable Siemens Simatic platform support
> > > > they likely want the LED drivers to be enabled without having
> > > > to flip more config switches. So we make the LED drivers config
> > > > switch default to the platform driver switches value.    
> > > 
> > > Same as per previous patch.
> > >   
> > 
> > Same as per previous patch.  
> 
> Same as per previous patch.
> 

I will assume that you are asking me to "default y", as discussed on
that other patch. And will send a v2 accordingly.

In case the ordering of default, depends etc matters to you, please say
so.

Henning
Guenter Roeck July 19, 2023, 1:27 p.m. UTC | #11
On 7/19/23 00:18, Henning Schild wrote:
> Am Tue, 18 Jul 2023 08:10:09 -0700
> schrieb Guenter Roeck <linux@roeck-us.net>:
> 
>> On 7/18/23 07:42, Henning Schild wrote:
>>> Am Tue, 18 Jul 2023 17:20:48 +0300
>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
>>>    
>>>> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:
>>>>> If a user did choose to enable Siemens Simatic platform support
>>>>> they likely want that driver to be enabled without having to flip
>>>>> more config switches. So we make the watchdog driver config switch
>>>>> default to the platform driver switches value.
>>>>
>>>> A nit-pick below.
>>>>
>>>> ...
>>>>   
>>>>>    config SIEMENS_SIMATIC_IPC_WDT
>>>>>    	tristate "Siemens Simatic IPC Watchdog"
>>>>>    	depends on SIEMENS_SIMATIC_IPC
>>>>   
>>>>> +	default SIEMENS_SIMATIC_IPC
>>>>
>>>> It's more natural to group tristate and default, vs. depends and
>>>> select.
>>>
>>> Will be ignored unless maintainer insists.
>>>    
>>
>> Maintainer wants to know why "default SIEMENS_SIMATIC_IPC" is needed
>> or warranted instead of the much simpler and easier to understand
>> "default y".
> 
> I thought a "default y" or "default m" was maybe not the best idea for
> a platform that is not super common. That is why i did not dare to even
> think about defaulting any of the Simatic stuff to not-no.
> 
> But it seems that this would be ok after all. And i would be very happy
> to do so because it means less work on distro configs.
> 
> SIEMENS_SIMATIC_IPC_WDT will drive a platform device which gets
> registered by SIEMENS_SIMATIC_IPC and nothing else. That is why
> "default SIEMENS_SIMATIC_IPC" was chosen.
> 

It depends on SIEMENS_SIMATIC_IPC. "default y" would make it y if
SIEMENS_SIMATIC_IPC=y, and m if SIEMENS_SIMATIC_IPC=m. If
SIEMENS_SIMATIC_IPC=n, it won't even be offered as option, and
default={m,y} will be ignored.

> But if i may i would change that to "default m", not "y" because there
> is an out of tree driver package which if installed on top, should be
> able to override the in-tree drivers.
> 
> So i will go ahead and make that one "default m"
> 

Why make it m as default even if SIEMENS_SIMATIC_IPC=y for whatever
reason ? Presumably anyone selecting SIEMENS_SIMATIC_IPC=y would
also want SIEMENS_SIMATIC_IPC_WDT=y, which is what you had before.
Sorry, I don't understand your logic.

Guenter
Henning Schild July 19, 2023, 2:40 p.m. UTC | #12
Am Wed, 19 Jul 2023 06:27:19 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 7/19/23 00:18, Henning Schild wrote:
> > Am Tue, 18 Jul 2023 08:10:09 -0700
> > schrieb Guenter Roeck <linux@roeck-us.net>:
> >   
> >> On 7/18/23 07:42, Henning Schild wrote:  
> >>> Am Tue, 18 Jul 2023 17:20:48 +0300
> >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >>>      
> >>>> On Tue, Jul 18, 2023 at 12:52:11PM +0200, Henning Schild wrote:  
> >>>>> If a user did choose to enable Siemens Simatic platform support
> >>>>> they likely want that driver to be enabled without having to
> >>>>> flip more config switches. So we make the watchdog driver
> >>>>> config switch default to the platform driver switches value.  
> >>>>
> >>>> A nit-pick below.
> >>>>
> >>>> ...
> >>>>     
> >>>>>    config SIEMENS_SIMATIC_IPC_WDT
> >>>>>    	tristate "Siemens Simatic IPC Watchdog"
> >>>>>    	depends on SIEMENS_SIMATIC_IPC  
> >>>>     
> >>>>> +	default SIEMENS_SIMATIC_IPC  
> >>>>
> >>>> It's more natural to group tristate and default, vs. depends and
> >>>> select.  
> >>>
> >>> Will be ignored unless maintainer insists.
> >>>      
> >>
> >> Maintainer wants to know why "default SIEMENS_SIMATIC_IPC" is
> >> needed or warranted instead of the much simpler and easier to
> >> understand "default y".  
> > 
> > I thought a "default y" or "default m" was maybe not the best idea
> > for a platform that is not super common. That is why i did not dare
> > to even think about defaulting any of the Simatic stuff to not-no.
> > 
> > But it seems that this would be ok after all. And i would be very
> > happy to do so because it means less work on distro configs.
> > 
> > SIEMENS_SIMATIC_IPC_WDT will drive a platform device which gets
> > registered by SIEMENS_SIMATIC_IPC and nothing else. That is why
> > "default SIEMENS_SIMATIC_IPC" was chosen.
> >   
> 
> It depends on SIEMENS_SIMATIC_IPC. "default y" would make it y if
> SIEMENS_SIMATIC_IPC=y, and m if SIEMENS_SIMATIC_IPC=m. If
> SIEMENS_SIMATIC_IPC=n, it won't even be offered as option, and
> default={m,y} will be ignored.
> 
> > But if i may i would change that to "default m", not "y" because
> > there is an out of tree driver package which if installed on top,
> > should be able to override the in-tree drivers.
> > 
> > So i will go ahead and make that one "default m"
> >   
> 
> Why make it m as default even if SIEMENS_SIMATIC_IPC=y for whatever
> reason ? Presumably anyone selecting SIEMENS_SIMATIC_IPC=y would
> also want SIEMENS_SIMATIC_IPC_WDT=y, which is what you had before.
> Sorry, I don't understand your logic.

At the time of writing i did not know what you described above. That y
with depends does not result in y.

Next round will have a "default y", Thanks.

Henning

> Guenter
>