| Message ID | 20230718105213.1275-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | platform/x86: move simatic ipc drivers into subdir | expand |
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
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.
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 >
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.
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 >
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
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 >
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 > >
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.
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
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
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 >
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%)