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

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

[Henning Schild]

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

[PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Henning Schild]

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
 	select WATCHDOG_CORE
 	select P2SB
 	help
-- 
2.41.0

[PATCH 2/3] leds: simatic-ipc-leds: default config switch to platform switch

[Henning Schild]

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.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/simple/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 44fa0f93cb3b..9666bf22d554 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -2,6 +2,7 @@
 config LEDS_SIEMENS_SIMATIC_IPC
 	tristate "LED driver for Siemens Simatic IPCs"
 	depends on SIEMENS_SIMATIC_IPC
+	default SIEMENS_SIMATIC_IPC
 	help
 	  This option enables support for the LEDs of several Industrial PCs
 	  from Siemens.
-- 
2.41.0

[PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Henning Schild]

Users without a Siemens Simatic IPC will not care about any of these
drivers. Users who do care can enable the submenu and all drivers behind
it will be enabled.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 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
 10 files changed, 90 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%)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 487d3d8f4da9..f5fcb1ca1b63 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1074,64 +1074,7 @@ config INTEL_SCU_IPC_UTIL
 	  low level access for debug work and updating the firmware. Say
 	  N unless you will be doing this on an Intel MID platform.
 
-config SIEMENS_SIMATIC_IPC
-	tristate "Siemens Simatic IPC Class driver"
-	help
-	  This Simatic IPC class driver is the central of several drivers. It
-	  is mainly used for system identification, after which drivers in other
-	  classes will take care of driving specifics of those machines.
-	  i.e. LEDs and watchdog.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called simatic-ipc.
-
-config SIEMENS_SIMATIC_IPC_BATT
-	tristate "CMOS battery driver for Siemens Simatic IPCs"
-	depends on HWMON
-	depends on SIEMENS_SIMATIC_IPC
-	default SIEMENS_SIMATIC_IPC
-	help
-	  This option enables support for monitoring the voltage of the CMOS
-	  batteries of several Industrial PCs from Siemens.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called simatic-ipc-batt.
-
-config SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE
-	tristate "CMOS Battery monitoring for Simatic IPCs based on Apollo Lake GPIO"
-	depends on PINCTRL_BROXTON
-	depends on SIEMENS_SIMATIC_IPC_BATT
-	default SIEMENS_SIMATIC_IPC_BATT
-	help
-	  This option enables CMOS battery monitoring for Simatic Industrial PCs
-	  from Siemens based on Apollo Lake GPIO.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called simatic-ipc-batt-apollolake.
-
-config SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE
-	tristate "CMOS Battery monitoring for Simatic IPCs based on Elkhart Lake GPIO"
-	depends on PINCTRL_ELKHARTLAKE
-	depends on SIEMENS_SIMATIC_IPC_BATT
-	default SIEMENS_SIMATIC_IPC_BATT
-	help
-	  This option enables CMOS battery monitoring for Simatic Industrial PCs
-	  from Siemens based on Elkhart Lake GPIO.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called simatic-ipc-batt-elkhartlake.
-
-config SIEMENS_SIMATIC_IPC_BATT_F7188X
-	tristate "CMOS Battery monitoring for Simatic IPCs based on Nuvoton GPIO"
-	depends on GPIO_F7188X
-	depends on SIEMENS_SIMATIC_IPC_BATT
-	default SIEMENS_SIMATIC_IPC_BATT
-	help
-	  This option enables CMOS battery monitoring for Simatic Industrial PCs
-	  from Siemens based on Nuvoton GPIO.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called simatic-ipc-batt-elkhartlake.
+source "drivers/platform/x86/siemens/Kconfig"
 
 config WINMATE_FM07_KEYS
 	tristate "Winmate FM07/FM07P front-panel keys driver"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 522da0d1584d..f3bf4b90b878 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -131,11 +131,7 @@ obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+= intel_scu_ipcutil.o
 obj-$(CONFIG_X86_INTEL_LPSS)		+= pmc_atom.o
 
 # Siemens Simatic Industrial PCs
-obj-$(CONFIG_SIEMENS_SIMATIC_IPC)			+= simatic-ipc.o
-obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT)			+= simatic-ipc-batt.o
-obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE)	+= simatic-ipc-batt-apollolake.o
-obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE)	+= simatic-ipc-batt-elkhartlake.o
-obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_F7188X)		+= simatic-ipc-batt-f7188x.o
+obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+= siemens/
 
 # Winmate
 obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
diff --git a/drivers/platform/x86/siemens/Kconfig b/drivers/platform/x86/siemens/Kconfig
new file mode 100644
index 000000000000..64479f83698c
--- /dev/null
+++ b/drivers/platform/x86/siemens/Kconfig
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Siemens X86 Platform Specific Drivers
+#
+
+menuconfig X86_PLATFORM_DRIVERS_SIEMENS
+	bool "Siemens X86 Platform Specific Device Drivers"
+	help
+	  Say Y here to get to see options for device drivers for various
+	  Siemens x86 platforms, mainly Simatic Industrial PCs.
+	  This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if X86_PLATFORM_DRIVERS_SIEMENS
+
+config SIEMENS_SIMATIC_IPC
+	tristate "Siemens Simatic IPC Class driver"
+	default m
+	help
+	  This Simatic IPC class driver is the central of several drivers. It
+	  is mainly used for system identification, after which drivers in other
+	  classes will take care of driving specifics of those machines.
+	  i.e. LEDs and watchdog.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc.
+
+config SIEMENS_SIMATIC_IPC_BATT
+	tristate "CMOS battery driver for Siemens Simatic IPCs"
+	depends on HWMON
+	depends on SIEMENS_SIMATIC_IPC
+	default SIEMENS_SIMATIC_IPC
+	help
+	  This option enables support for monitoring the voltage of the CMOS
+	  batteries of several Industrial PCs from Siemens.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc-batt.
+
+config SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE
+	tristate "CMOS Battery monitoring for Simatic IPCs based on Apollo Lake GPIO"
+	depends on PINCTRL_BROXTON
+	depends on SIEMENS_SIMATIC_IPC_BATT
+	default SIEMENS_SIMATIC_IPC_BATT
+	help
+	  This option enables CMOS battery monitoring for Simatic Industrial PCs
+	  from Siemens based on Apollo Lake GPIO.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc-batt-apollolake.
+
+config SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE
+	tristate "CMOS Battery monitoring for Simatic IPCs based on Elkhart Lake GPIO"
+	depends on PINCTRL_ELKHARTLAKE
+	depends on SIEMENS_SIMATIC_IPC_BATT
+	default SIEMENS_SIMATIC_IPC_BATT
+	help
+	  This option enables CMOS battery monitoring for Simatic Industrial PCs
+	  from Siemens based on Elkhart Lake GPIO.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc-batt-elkhartlake.
+
+config SIEMENS_SIMATIC_IPC_BATT_F7188X
+	tristate "CMOS Battery monitoring for Simatic IPCs based on Nuvoton GPIO"
+	depends on GPIO_F7188X
+	depends on SIEMENS_SIMATIC_IPC_BATT
+	default SIEMENS_SIMATIC_IPC_BATT
+	help
+	  This option enables CMOS battery monitoring for Simatic Industrial PCs
+	  from Siemens based on Nuvoton GPIO.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc-batt-elkhartlake.
+
+endif # X86_PLATFORM_DRIVERS_SIEMENS
diff --git a/drivers/platform/x86/siemens/Makefile b/drivers/platform/x86/siemens/Makefile
new file mode 100644
index 000000000000..2b384b4cb8ba
--- /dev/null
+++ b/drivers/platform/x86/siemens/Makefile
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/x86/siemens
+# Siemens x86 Platform-Specific Drivers
+#
+
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC)			+= simatic-ipc.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT)			+= simatic-ipc-batt.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE)	+= simatic-ipc-batt-apollolake.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE)	+= simatic-ipc-batt-elkhartlake.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_F7188X)		+= simatic-ipc-batt-f7188x.o
diff --git a/drivers/platform/x86/simatic-ipc-batt-apollolake.c b/drivers/platform/x86/siemens/simatic-ipc-batt-apollolake.c
similarity index 100%
rename from drivers/platform/x86/simatic-ipc-batt-apollolake.c
rename to drivers/platform/x86/siemens/simatic-ipc-batt-apollolake.c
diff --git a/drivers/platform/x86/simatic-ipc-batt-elkhartlake.c b/drivers/platform/x86/siemens/simatic-ipc-batt-elkhartlake.c
similarity index 100%
rename from drivers/platform/x86/simatic-ipc-batt-elkhartlake.c
rename to drivers/platform/x86/siemens/simatic-ipc-batt-elkhartlake.c
diff --git a/drivers/platform/x86/simatic-ipc-batt-f7188x.c b/drivers/platform/x86/siemens/simatic-ipc-batt-f7188x.c
similarity index 100%
rename from drivers/platform/x86/simatic-ipc-batt-f7188x.c
rename to drivers/platform/x86/siemens/simatic-ipc-batt-f7188x.c
diff --git a/drivers/platform/x86/simatic-ipc-batt.c b/drivers/platform/x86/siemens/simatic-ipc-batt.c
similarity index 100%
rename from drivers/platform/x86/simatic-ipc-batt.c
rename to drivers/platform/x86/siemens/simatic-ipc-batt.c
diff --git a/drivers/platform/x86/simatic-ipc-batt.h b/drivers/platform/x86/siemens/simatic-ipc-batt.h
similarity index 100%
rename from drivers/platform/x86/simatic-ipc-batt.h
rename to drivers/platform/x86/siemens/simatic-ipc-batt.h
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/siemens/simatic-ipc.c
similarity index 100%
rename from drivers/platform/x86/simatic-ipc.c
rename to drivers/platform/x86/siemens/simatic-ipc.c
-- 
2.41.0

Re: [PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Henning Schild]

Hans,

this is based on your tag "ib-pdx86-simatic-v6.6" as you requested.

Henning

Am Tue, 18 Jul 2023 12:52:13 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Users without a Siemens Simatic IPC will not care about any of these
> drivers. Users who do care can enable the submenu and all drivers
> behind it will be enabled.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  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
>  10 files changed, 90 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%)
> 
> diff --git a/drivers/platform/x86/Kconfig
> b/drivers/platform/x86/Kconfig index 487d3d8f4da9..f5fcb1ca1b63 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1074,64 +1074,7 @@ config INTEL_SCU_IPC_UTIL
>  	  low level access for debug work and updating the firmware.
> Say N unless you will be doing this on an Intel MID platform.
>  
> -config SIEMENS_SIMATIC_IPC
> -	tristate "Siemens Simatic IPC Class driver"
> -	help
> -	  This Simatic IPC class driver is the central of several
> drivers. It
> -	  is mainly used for system identification, after which
> drivers in other
> -	  classes will take care of driving specifics of those
> machines.
> -	  i.e. LEDs and watchdog.
> -
> -	  To compile this driver as a module, choose M here: the
> module
> -	  will be called simatic-ipc.
> -
> -config SIEMENS_SIMATIC_IPC_BATT
> -	tristate "CMOS battery driver for Siemens Simatic IPCs"
> -	depends on HWMON
> -	depends on SIEMENS_SIMATIC_IPC
> -	default SIEMENS_SIMATIC_IPC
> -	help
> -	  This option enables support for monitoring the voltage of
> the CMOS
> -	  batteries of several Industrial PCs from Siemens.
> -
> -	  To compile this driver as a module, choose M here: the
> module
> -	  will be called simatic-ipc-batt.
> -
> -config SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE
> -	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Apollo Lake GPIO"
> -	depends on PINCTRL_BROXTON
> -	depends on SIEMENS_SIMATIC_IPC_BATT
> -	default SIEMENS_SIMATIC_IPC_BATT
> -	help
> -	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> -	  from Siemens based on Apollo Lake GPIO.
> -
> -	  To compile this driver as a module, choose M here: the
> module
> -	  will be called simatic-ipc-batt-apollolake.
> -
> -config SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE
> -	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Elkhart Lake GPIO"
> -	depends on PINCTRL_ELKHARTLAKE
> -	depends on SIEMENS_SIMATIC_IPC_BATT
> -	default SIEMENS_SIMATIC_IPC_BATT
> -	help
> -	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> -	  from Siemens based on Elkhart Lake GPIO.
> -
> -	  To compile this driver as a module, choose M here: the
> module
> -	  will be called simatic-ipc-batt-elkhartlake.
> -
> -config SIEMENS_SIMATIC_IPC_BATT_F7188X
> -	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Nuvoton GPIO"
> -	depends on GPIO_F7188X
> -	depends on SIEMENS_SIMATIC_IPC_BATT
> -	default SIEMENS_SIMATIC_IPC_BATT
> -	help
> -	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> -	  from Siemens based on Nuvoton GPIO.
> -
> -	  To compile this driver as a module, choose M here: the
> module
> -	  will be called simatic-ipc-batt-elkhartlake.
> +source "drivers/platform/x86/siemens/Kconfig"
>  
>  config WINMATE_FM07_KEYS
>  	tristate "Winmate FM07/FM07P front-panel keys driver"
> diff --git a/drivers/platform/x86/Makefile
> b/drivers/platform/x86/Makefile index 522da0d1584d..f3bf4b90b878
> 100644 --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -131,11 +131,7 @@ obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+=
> intel_scu_ipcutil.o obj-$(CONFIG_X86_INTEL_LPSS)		+=
> pmc_atom.o 
>  # Siemens Simatic Industrial PCs
> -obj-$(CONFIG_SIEMENS_SIMATIC_IPC)			+=
> simatic-ipc.o -obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT)
> 	+= simatic-ipc-batt.o
> -obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE)	+=
> simatic-ipc-batt-apollolake.o
> -obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE)	+=
> simatic-ipc-batt-elkhartlake.o
> -obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_F7188X)		+=
> simatic-ipc-batt-f7188x.o
> +obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+=
> siemens/ # Winmate obj-$(CONFIG_WINMATE_FM07_KEYS)		+=
> winmate-fm07-keys.o diff --git a/drivers/platform/x86/siemens/Kconfig
> b/drivers/platform/x86/siemens/Kconfig new file mode 100644 index
> 000000000000..64479f83698c --- /dev/null +++
> b/drivers/platform/x86/siemens/Kconfig @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Siemens X86 Platform Specific Drivers
> +#
> +
> +menuconfig X86_PLATFORM_DRIVERS_SIEMENS
> +	bool "Siemens X86 Platform Specific Device Drivers"
> +	help
> +	  Say Y here to get to see options for device drivers for
> various
> +	  Siemens x86 platforms, mainly Simatic Industrial PCs.
> +	  This option alone does not add any kernel code.
> +
> +	  If you say N, all options in this submenu will be skipped
> and disabled. +
> +if X86_PLATFORM_DRIVERS_SIEMENS
> +
> +config SIEMENS_SIMATIC_IPC
> +	tristate "Siemens Simatic IPC Class driver"
> +	default m
> +	help
> +	  This Simatic IPC class driver is the central of several
> drivers. It
> +	  is mainly used for system identification, after which
> drivers in other
> +	  classes will take care of driving specifics of those
> machines.
> +	  i.e. LEDs and watchdog.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc.
> +
> +config SIEMENS_SIMATIC_IPC_BATT
> +	tristate "CMOS battery driver for Siemens Simatic IPCs"
> +	depends on HWMON
> +	depends on SIEMENS_SIMATIC_IPC
> +	default SIEMENS_SIMATIC_IPC
> +	help
> +	  This option enables support for monitoring the voltage of
> the CMOS
> +	  batteries of several Industrial PCs from Siemens.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc-batt.
> +
> +config SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE
> +	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Apollo Lake GPIO"
> +	depends on PINCTRL_BROXTON
> +	depends on SIEMENS_SIMATIC_IPC_BATT
> +	default SIEMENS_SIMATIC_IPC_BATT
> +	help
> +	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> +	  from Siemens based on Apollo Lake GPIO.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc-batt-apollolake.
> +
> +config SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE
> +	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Elkhart Lake GPIO"
> +	depends on PINCTRL_ELKHARTLAKE
> +	depends on SIEMENS_SIMATIC_IPC_BATT
> +	default SIEMENS_SIMATIC_IPC_BATT
> +	help
> +	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> +	  from Siemens based on Elkhart Lake GPIO.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc-batt-elkhartlake.
> +
> +config SIEMENS_SIMATIC_IPC_BATT_F7188X
> +	tristate "CMOS Battery monitoring for Simatic IPCs based on
> Nuvoton GPIO"
> +	depends on GPIO_F7188X
> +	depends on SIEMENS_SIMATIC_IPC_BATT
> +	default SIEMENS_SIMATIC_IPC_BATT
> +	help
> +	  This option enables CMOS battery monitoring for Simatic
> Industrial PCs
> +	  from Siemens based on Nuvoton GPIO.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc-batt-elkhartlake.
> +
> +endif # X86_PLATFORM_DRIVERS_SIEMENS
> diff --git a/drivers/platform/x86/siemens/Makefile
> b/drivers/platform/x86/siemens/Makefile new file mode 100644
> index 000000000000..2b384b4cb8ba
> --- /dev/null
> +++ b/drivers/platform/x86/siemens/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for linux/drivers/platform/x86/siemens
> +# Siemens x86 Platform-Specific Drivers
> +#
> +
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC)			+=
> simatic-ipc.o +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT)
> 	+= simatic-ipc-batt.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_APOLLOLAKE)	+=
> simatic-ipc-batt-apollolake.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_ELKHARTLAKE)	+=
> simatic-ipc-batt-elkhartlake.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_BATT_F7188X)		+=
> simatic-ipc-batt-f7188x.o diff --git
> a/drivers/platform/x86/simatic-ipc-batt-apollolake.c
> b/drivers/platform/x86/siemens/simatic-ipc-batt-apollolake.c
> similarity index 100% rename from
> drivers/platform/x86/simatic-ipc-batt-apollolake.c rename to
> drivers/platform/x86/siemens/simatic-ipc-batt-apollolake.c diff --git
> a/drivers/platform/x86/simatic-ipc-batt-elkhartlake.c
> b/drivers/platform/x86/siemens/simatic-ipc-batt-elkhartlake.c
> similarity index 100% rename from
> drivers/platform/x86/simatic-ipc-batt-elkhartlake.c rename to
> drivers/platform/x86/siemens/simatic-ipc-batt-elkhartlake.c diff
> --git a/drivers/platform/x86/simatic-ipc-batt-f7188x.c
> b/drivers/platform/x86/siemens/simatic-ipc-batt-f7188x.c similarity
> index 100% rename from drivers/platform/x86/simatic-ipc-batt-f7188x.c
> rename to drivers/platform/x86/siemens/simatic-ipc-batt-f7188x.c diff
> --git a/drivers/platform/x86/simatic-ipc-batt.c
> b/drivers/platform/x86/siemens/simatic-ipc-batt.c similarity index
> 100% rename from drivers/platform/x86/simatic-ipc-batt.c rename to
> drivers/platform/x86/siemens/simatic-ipc-batt.c diff --git
> a/drivers/platform/x86/simatic-ipc-batt.h
> b/drivers/platform/x86/siemens/simatic-ipc-batt.h similarity index
> 100% rename from drivers/platform/x86/simatic-ipc-batt.h rename to
> drivers/platform/x86/siemens/simatic-ipc-batt.h diff --git
> a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/siemens/simatic-ipc.c similarity index 100%
> rename from drivers/platform/x86/simatic-ipc.c rename to
> drivers/platform/x86/siemens/simatic-ipc.c

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Andy Shevchenko]

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

Re: [PATCH 2/3] leds: simatic-ipc-leds: default config switch to platform switch

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 12:52:13PM +0200, Henning Schild wrote:
> Users without a Siemens Simatic IPC will not care about any of these
> drivers. Users who do care can enable the submenu and all drivers behind
> it will be enabled.

...

>  # Siemens Simatic Industrial PCs
> +obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+= siemens/

Do you need conditional here? We have stumbled over similar for entire intel
subfolder, it might affect the rest as well when you don't expect it.

obj-y		+= siemens/

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Henning Schild]

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  
> 

Re: [PATCH 2/3] leds: simatic-ipc-leds: default config switch to platform switch

[Henning Schild]

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.

Re: [PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Henning Schild]

Am Tue, 18 Jul 2023 17:23:30 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Jul 18, 2023 at 12:52:13PM +0200, Henning Schild wrote:
> > Users without a Siemens Simatic IPC will not care about any of these
> > drivers. Users who do care can enable the submenu and all drivers
> > behind it will be enabled.  
> 
> ...
> 
> >  # Siemens Simatic Industrial PCs
> > +obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+=
> > siemens/  
> 
> Do you need conditional here? We have stumbled over similar for
> entire intel subfolder, it might affect the rest as well when you
> don't expect it.
> 
> obj-y		+= siemens/
> 
> ?
> 

It was requested to be done like that by Hans, he wanted me to do a
similar thing that
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1e1ea516721d1ea0b21327ff9e6cb2c2bb86e28
is doing.

And that is what i did. If there was a y ... the whole "one switch to
rule them all" story would not work out anymore.

Thanks for the review though.

Henning

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Guenter Roeck]

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
> 

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Guenter Roeck]

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

Re: [PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 04:47:27PM +0200, Henning Schild wrote:
> Am Tue, 18 Jul 2023 17:23:30 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> 
> > On Tue, Jul 18, 2023 at 12:52:13PM +0200, Henning Schild wrote:
> > > Users without a Siemens Simatic IPC will not care about any of these
> > > drivers. Users who do care can enable the submenu and all drivers
> > > behind it will be enabled.  

...

> > >  # Siemens Simatic Industrial PCs
> > > +obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+=
> > > siemens/  
> > 
> > Do you need conditional here? We have stumbled over similar for
> > entire intel subfolder, it might affect the rest as well when you
> > don't expect it.
> > 
> > obj-y		+= siemens/
> > 
> > ?
> 
> It was requested to be done like that by Hans, he wanted me to do a
> similar thing that

"Similar" is not the "same". :-)

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1e1ea516721d1ea0b21327ff9e6cb2c2bb86e28
> is doing.
> 
> And that is what i did. If there was a y ... the whole "one switch to
> rule them all" story would not work out anymore.

See these:
https://git.kernel.org/torvalds/c/8bd836feb6ca
https://git.kernel.org/torvalds/c/4f6c131c3c31

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Henning Schild]

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
> 

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Henning Schild]

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

Re: [PATCH 3/3] platform/x86: Move all simatic ipc drivers to the subdirectory siemens

[Henning Schild]

Am Tue, 18 Jul 2023 18:15:06 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Tue, Jul 18, 2023 at 04:47:27PM +0200, Henning Schild wrote:
> > Am Tue, 18 Jul 2023 17:23:30 +0300
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> >   
> > > On Tue, Jul 18, 2023 at 12:52:13PM +0200, Henning Schild wrote:  
> > > > Users without a Siemens Simatic IPC will not care about any of
> > > > these drivers. Users who do care can enable the submenu and all
> > > > drivers behind it will be enabled.    
> 
> ...
> 
> > > >  # Siemens Simatic Industrial PCs
> > > > +obj-$(CONFIG_X86_PLATFORM_DRIVERS_SIEMENS)		+=
> > > > siemens/    
> > > 
> > > Do you need conditional here? We have stumbled over similar for
> > > entire intel subfolder, it might affect the rest as well when you
> > > don't expect it.
> > > 
> > > obj-y		+= siemens/
> > > 
> > > ?  
> > 
> > It was requested to be done like that by Hans, he wanted me to do a
> > similar thing that  
> 
> "Similar" is not the "same". :-)
> 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f1e1ea516721d1ea0b21327ff9e6cb2c2bb86e28
> > is doing.
> > 
> > And that is what i did. If there was a y ... the whole "one switch
> > to rule them all" story would not work out anymore.  
> 
> See these:
> https://git.kernel.org/torvalds/c/8bd836feb6ca
> https://git.kernel.org/torvalds/c/4f6c131c3c31

Ok i will switch that to a simple y without a CONFIG item to it, and
start that inheritance chain at SIEMENS_SIMATIC_IPC.

That in fact also makes sure the interface does not change and "make
olddefconfig" does not turn the Siemens stuff back off because the
newly introduced guard is blocking/hiding what used to be turned on.

Thanks!
Henning

Re: [PATCH 2/3] leds: simatic-ipc-leds: default config switch to platform switch

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/3] leds: simatic-ipc-leds: default config switch to platform switch

[Henning Schild]

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

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Guenter Roeck]

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

Re: [PATCH 1/3] watchdog: make Siemens Simatic watchdog driver default on platform

[Henning Schild]

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
>