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%)
--
2.41.0
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 <[email protected]>
---
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
select WATCHDOG_CORE
select P2SB
help
--
2.41.0
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
--
With Best Regards,
Andy Shevchenko
Am Tue, 18 Jul 2023 17:20:48 +0300
schrieb Andy Shevchenko <[email protected]>:
> 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
>
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 <[email protected]>
> ---
> 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
>
On 7/18/23 07:42, Henning Schild wrote:
> Am Tue, 18 Jul 2023 17:20:48 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
>> 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
Am Tue, 18 Jul 2023 08:10:09 -0700
schrieb Guenter Roeck <[email protected]>:
> On 7/18/23 07:42, Henning Schild wrote:
> > Am Tue, 18 Jul 2023 17:20:48 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> >
> >> 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
>
Am Tue, 18 Jul 2023 08:07:52 -0700
schrieb Guenter Roeck <[email protected]>:
> 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 <[email protected]>
> > ---
> > 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
> >
On 7/19/23 00:18, Henning Schild wrote:
> Am Tue, 18 Jul 2023 08:10:09 -0700
> schrieb Guenter Roeck <[email protected]>:
>
>> On 7/18/23 07:42, Henning Schild wrote:
>>> Am Tue, 18 Jul 2023 17:20:48 +0300
>>> schrieb Andy Shevchenko <[email protected]>:
>>>
>>>> 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
Am Wed, 19 Jul 2023 06:27:19 -0700
schrieb Guenter Roeck <[email protected]>:
> On 7/19/23 00:18, Henning Schild wrote:
> > Am Tue, 18 Jul 2023 08:10:09 -0700
> > schrieb Guenter Roeck <[email protected]>:
> >
> >> On 7/18/23 07:42, Henning Schild wrote:
> >>> Am Tue, 18 Jul 2023 17:20:48 +0300
> >>> schrieb Andy Shevchenko <[email protected]>:
> >>>
> >>>> 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
>