| Message ID | 20230718105213.1275-2-henning.schild@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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
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 >
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 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 >
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
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(+)