linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
@ 2020-06-22 10:03 Johnson CH Chen (陳昭勳)
  2020-06-22 11:14 ` Lee Jones
  2020-06-22 15:38 ` kernel test robot
  0 siblings, 2 replies; 6+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-06-22 10:03 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
	Wim Van Sebroeck, linux, Lee Jones

Dallas/Maxim DS1374 is a counter designed to continuously count
time in seconds. It provides an I2C interface to the host to
access RTC clock or Alarm/Watchdog timer.

Add MFD Core driver, supporting the I2C communication to RTC and
Watchdog devices.

Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
---
 drivers/mfd/Kconfig  |  11 +++++
 drivers/mfd/Makefile |   2 +
 drivers/mfd/ds1374.c | 101 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 drivers/mfd/ds1374.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 687e9c848053..d16031f4b487 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1980,6 +1980,17 @@ config MFD_STM32_LPTIMER
 	  To compile this driver as a module, choose M here: the
 	  module will be called stm32-lptimer.
 
+config MFD_DS1374
+	tristate "Support for Dallas/Maxim DS1374"
+	depends on I2C
+	select MFD_CORE
+	help
+	  Say yes here to add support for Dallas/Maxim DS1374 Semiconductor.
+	  This driver provides RTC and watchdog.
+
+	  This driver can also be built as a module. If so, module will be
+	  called ds1374.
+
 config MFD_STM32_TIMERS
 	tristate "Support for STM32 Timers"
 	depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index bea2be419822..a6463dd4aa33 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -243,6 +243,8 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC)	+= intel_soc_pmic_chtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
 mt6397-objs	:= mt6397-core.o mt6397-irq.o
 obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
+obj-$(CONFIG_MFD_DS1374) 	+= ds1374.o
+
 obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
diff --git a/drivers/mfd/ds1374.c b/drivers/mfd/ds1374.c
new file mode 100644
index 000000000000..6e9041aba5d2
--- /dev/null
+++ b/drivers/mfd/ds1374.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Mamim/Dallas DS1374 MFD Core Driver.
+ *
+ *  Copyright (C) 2020 Johnson Chen <johnsonch.chen@moxa.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+
+
+static struct mfd_cell ds1374_cell[] = {
+	{ .name = "ds1374_rtc", },
+	{ .name = "ds1374_wdt", },
+};
+
+static int ds1374_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	int ret;
+
+	ret = i2c_check_functionality(client->adapter,
+				      I2C_FUNC_SMBUS_BYTE_DATA |
+				      I2C_FUNC_SMBUS_WORD_DATA |
+				      I2C_FUNC_SMBUS_BYTE);
+	if (!ret)
+		return -ENODEV;
+
+	ret = devm_mfd_add_devices(&client->dev, 0, ds1374_cell,
+				   ARRAY_SIZE(ds1374_cell), NULL, 0, NULL);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to add DS1374 sub-devices\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ds1374_remove(struct i2c_client *client)
+{
+	mfd_remove_devices(&client->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ds1374_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	if (client->irq > 0 && device_may_wakeup(&client->dev))
+		enable_irq_wake(client->irq);
+	return 0;
+}
+
+static int ds1374_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	if (client->irq > 0 && device_may_wakeup(&client->dev))
+		disable_irq_wake(client->irq);
+	return 0;
+}
+#endif
+
+static const struct i2c_device_id ds1374_id[] = {
+	{ "ds1374", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ds1374_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ds1374_of_match[] = {
+	{ .compatible = "dallas,ds1374" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ds1374_of_match);
+#endif
+
+static SIMPLE_DEV_PM_OPS(ds1374_pm, ds1374_suspend, ds1374_resume);
+
+static struct i2c_driver ds1374_driver = {
+	.driver                 = {
+		.name           = "ds1374",
+		.of_match_table = of_match_ptr(ds1374_of_match),
+		.pm             = &ds1374_pm,
+	},
+	.probe          = ds1374_probe,
+	.remove	        = ds1374_remove,
+	.id_table       = ds1374_id,
+};
+
+module_i2c_driver(ds1374_driver);
+
+MODULE_DESCRIPTION("Dallas/Maxim DS1374 MFD Core Driver");
+MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
+MODULE_LICENSE("GPL");
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
  2020-06-22 10:03 [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver Johnson CH Chen (陳昭勳)
@ 2020-06-22 11:14 ` Lee Jones
  2020-06-22 14:26   ` Guenter Roeck
  2020-06-22 15:38 ` kernel test robot
  1 sibling, 1 reply; 6+ messages in thread
From: Lee Jones @ 2020-06-22 11:14 UTC (permalink / raw)
  To: Johnson CH Chen (陳昭勳)
  Cc: linux-kernel, linux-rtc, linux-watchdog, Alessandro Zummo,
	Alexandre Belloni, Wim Van Sebroeck, linux

On Mon, 22 Jun 2020, Johnson CH Chen (陳昭勳) wrote:

> Dallas/Maxim DS1374 is a counter designed to continuously count
> time in seconds. It provides an I2C interface to the host to
> access RTC clock or Alarm/Watchdog timer.
> 
> Add MFD Core driver, supporting the I2C communication to RTC and
> Watchdog devices.
> 
> Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> ---
>  drivers/mfd/Kconfig  |  11 +++++
>  drivers/mfd/Makefile |   2 +
>  drivers/mfd/ds1374.c | 101 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 114 insertions(+)
>  create mode 100644 drivers/mfd/ds1374.c

Not sure I see the point of this driver.

How/where is the device part registered?

Is it DT only?  If not, what else?

Also where are the bindings?

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 687e9c848053w.d16031f4b487 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1980,6 +1980,17 @@ config MFD_STM32_LPTIMER
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stm32-lptimer.
>  
> +config MFD_DS1374
> +	tristate "Support for Dallas/Maxim DS1374"
> +	depends on I2C
> +	select MFD_CORE
> +	help
> +	  Say yes here to add support for Dallas/Maxim DS1374 Semiconductor.
> +	  This driver provides RTC and watchdog.
> +
> +	  This driver can also be built as a module. If so, module will be
> +	  called ds1374.
> +
>  config MFD_STM32_TIMERS
>  	tristate "Support for STM32 Timers"
>  	depends on (ARCH_STM32 && OF) || COMPILE_TEST

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
  2020-06-22 11:14 ` Lee Jones
@ 2020-06-22 14:26   ` Guenter Roeck
  2020-06-22 15:01     ` Alexandre Belloni
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2020-06-22 14:26 UTC (permalink / raw)
  To: Lee Jones
  Cc: Johnson CH Chen (陳昭勳),
	linux-kernel, linux-rtc, linux-watchdog, Alessandro Zummo,
	Alexandre Belloni, Wim Van Sebroeck

On Mon, Jun 22, 2020 at 12:14:13PM +0100, Lee Jones wrote:
> On Mon, 22 Jun 2020, Johnson CH Chen (陳昭勳) wrote:
> 
> > Dallas/Maxim DS1374 is a counter designed to continuously count
> > time in seconds. It provides an I2C interface to the host to
> > access RTC clock or Alarm/Watchdog timer.
> > 
> > Add MFD Core driver, supporting the I2C communication to RTC and
> > Watchdog devices.
> > 
> > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > ---
> >  drivers/mfd/Kconfig  |  11 +++++
> >  drivers/mfd/Makefile |   2 +
> >  drivers/mfd/ds1374.c | 101 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 114 insertions(+)
> >  create mode 100644 drivers/mfd/ds1374.c
> 
> Not sure I see the point of this driver.
> 

Not entirely sure either. Seems to me the idea is to use the watchdog
subsystem for watchdog functionality, but that is just a guess and not
really necessary (the conversion could be done in the rtc driver).
I don't think the code as written works - the rtc code uses a mutex
which the watchdog driver obviously isn't aware of. The mutex would
have to be moved into the mfd code, with respective access functions.

Overall this adds a lot of complexity, and it seems the interdependencies
between rtc and watchdog functionality are not well understood. Plus,
other watchdog drivers have recently been added to other rtc clock chips,
so this adds some inconsistencies in the rtc subsystem. Are we going
to see this change for all those combined rtc/watchdog drivers ?
If so, it might make sense to communicate that now to ensure consistency.

Guenter

> How/where is the device part registered?
> 
> Is it DT only?  If not, what else?
> 
> Also where are the bindings?
> 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 687e9c848053w.d16031f4b487 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1980,6 +1980,17 @@ config MFD_STM32_LPTIMER
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called stm32-lptimer.
> >  
> > +config MFD_DS1374
> > +	tristate "Support for Dallas/Maxim DS1374"
> > +	depends on I2C
> > +	select MFD_CORE
> > +	help
> > +	  Say yes here to add support for Dallas/Maxim DS1374 Semiconductor.
> > +	  This driver provides RTC and watchdog.
> > +
> > +	  This driver can also be built as a module. If so, module will be
> > +	  called ds1374.
> > +
> >  config MFD_STM32_TIMERS
> >  	tristate "Support for STM32 Timers"
> >  	depends on (ARCH_STM32 && OF) || COMPILE_TEST
> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
  2020-06-22 14:26   ` Guenter Roeck
@ 2020-06-22 15:01     ` Alexandre Belloni
  2020-06-23  7:12       ` Johnson CH Chen (陳昭勳)
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2020-06-22 15:01 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lee Jones, Johnson CH Chen (陳昭勳),
	linux-kernel, linux-rtc, linux-watchdog, Alessandro Zummo,
	Wim Van Sebroeck

On 22/06/2020 07:26:56-0700, Guenter Roeck wrote:
> On Mon, Jun 22, 2020 at 12:14:13PM +0100, Lee Jones wrote:
> > On Mon, 22 Jun 2020, Johnson CH Chen (陳昭勳) wrote:
> > 
> > > Dallas/Maxim DS1374 is a counter designed to continuously count
> > > time in seconds. It provides an I2C interface to the host to
> > > access RTC clock or Alarm/Watchdog timer.
> > > 
> > > Add MFD Core driver, supporting the I2C communication to RTC and
> > > Watchdog devices.
> > > 
> > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > > ---
> > >  drivers/mfd/Kconfig  |  11 +++++
> > >  drivers/mfd/Makefile |   2 +
> > >  drivers/mfd/ds1374.c | 101 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 114 insertions(+)
> > >  create mode 100644 drivers/mfd/ds1374.c
> > 
> > Not sure I see the point of this driver.
> > 
> 
> Not entirely sure either. Seems to me the idea is to use the watchdog
> subsystem for watchdog functionality, but that is just a guess and not
> really necessary (the conversion could be done in the rtc driver).
> I don't think the code as written works - the rtc code uses a mutex
> which the watchdog driver obviously isn't aware of. The mutex would
> have to be moved into the mfd code, with respective access functions.
> 
> Overall this adds a lot of complexity, and it seems the interdependencies
> between rtc and watchdog functionality are not well understood. Plus,
> other watchdog drivers have recently been added to other rtc clock chips,
> so this adds some inconsistencies in the rtc subsystem. Are we going
> to see this change for all those combined rtc/watchdog drivers ?
> If so, it might make sense to communicate that now to ensure consistency.
> 

I read the datasheet again and I agree the watchdog part can live in the
rtc driver. As only the RTC alarm and the watchdog are mutually
exclusive. I don't think an MFD driver is necessary. Converting the
current driver to the watchdog subsystem seems to be the correct way
forward.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
  2020-06-22 10:03 [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver Johnson CH Chen (陳昭勳)
  2020-06-22 11:14 ` Lee Jones
@ 2020-06-22 15:38 ` kernel test robot
  1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2020-06-22 15:38 UTC (permalink / raw)
  To: Johnson CH Chen (陳昭勳), linux-kernel
  Cc: kbuild-all, linux-rtc, linux-watchdog, Alessandro Zummo,
	Alexandre Belloni, Wim Van Sebroeck, linux, Lee Jones

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

Hi "Johnson,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on linux/master linus/master v5.8-rc2 next-20200622]
[cannot apply to ljones-mfd/for-mfd-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johnson-CH-Chen/Use-MFD-for-Dallas-Maxim-DS1374-driver-series/20200622-180544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mfd/ds1374.c: In function 'ds1374_suspend':
>> drivers/mfd/ds1374.c:56:3: error: implicit declaration of function 'enable_irq_wake' [-Werror=implicit-function-declaration]
      56 |   enable_irq_wake(client->irq);
         |   ^~~~~~~~~~~~~~~
   drivers/mfd/ds1374.c: In function 'ds1374_resume':
>> drivers/mfd/ds1374.c:65:3: error: implicit declaration of function 'disable_irq_wake' [-Werror=implicit-function-declaration]
      65 |   disable_irq_wake(client->irq);
         |   ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/enable_irq_wake +56 drivers/mfd/ds1374.c

    49	
    50	#ifdef CONFIG_PM_SLEEP
    51	static int ds1374_suspend(struct device *dev)
    52	{
    53		struct i2c_client *client = to_i2c_client(dev);
    54	
    55		if (client->irq > 0 && device_may_wakeup(&client->dev))
  > 56			enable_irq_wake(client->irq);
    57		return 0;
    58	}
    59	
    60	static int ds1374_resume(struct device *dev)
    61	{
    62		struct i2c_client *client = to_i2c_client(dev);
    63	
    64		if (client->irq > 0 && device_may_wakeup(&client->dev))
  > 65			disable_irq_wake(client->irq);
    66		return 0;
    67	}
    68	#endif
    69	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63049 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
  2020-06-22 15:01     ` Alexandre Belloni
@ 2020-06-23  7:12       ` Johnson CH Chen (陳昭勳)
  0 siblings, 0 replies; 6+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-06-23  7:12 UTC (permalink / raw)
  To: Alexandre Belloni, Guenter Roeck
  Cc: Lee Jones, linux-kernel, linux-rtc, linux-watchdog,
	Alessandro Zummo, Wim Van Sebroeck

Hi,
 
> On 22/06/2020 07:26:56-0700, Guenter Roeck wrote:
> > On Mon, Jun 22, 2020 at 12:14:13PM +0100, Lee Jones wrote:
> > > On Mon, 22 Jun 2020, Johnson CH Chen (陳昭勳) wrote:
> > >
> > > > Dallas/Maxim DS1374 is a counter designed to continuously count
> > > > time in seconds. It provides an I2C interface to the host to
> > > > access RTC clock or Alarm/Watchdog timer.
> > > >
> > > > Add MFD Core driver, supporting the I2C communication to RTC and
> > > > Watchdog devices.
> > > >
> > > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > > > ---
> > > >  drivers/mfd/Kconfig  |  11 +++++
> > > >  drivers/mfd/Makefile |   2 +
> > > >  drivers/mfd/ds1374.c | 101
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 114 insertions(+)  create mode 100644
> > > > drivers/mfd/ds1374.c
> > >
> > > Not sure I see the point of this driver.
> > >
> >
> > Not entirely sure either. Seems to me the idea is to use the watchdog
> > subsystem for watchdog functionality, but that is just a guess and not
> > really necessary (the conversion could be done in the rtc driver).
> > I don't think the code as written works - the rtc code uses a mutex
> > which the watchdog driver obviously isn't aware of. The mutex would
> > have to be moved into the mfd code, with respective access functions.
> >
> > Overall this adds a lot of complexity, and it seems the
> > interdependencies between rtc and watchdog functionality are not well
> > understood. Plus, other watchdog drivers have recently been added to
> > other rtc clock chips, so this adds some inconsistencies in the rtc
> > subsystem. Are we going to see this change for all those combined
> rtc/watchdog drivers ?
> > If so, it might make sense to communicate that now to ensure consistency.
> >
> 
> I read the datasheet again and I agree the watchdog part can live in the rtc
> driver. As only the RTC alarm and the watchdog are mutually exclusive. I don't
> think an MFD driver is necessary. Converting the current driver to the
> watchdog subsystem seems to be the correct way forward.
> 
Thanks for your good thinking. If we want to add watchdog function such as 
"nowayout" to the driver, it's good to try to upstream this in rtc-ds1374.c in 
rtc subsystem?

It seems like more complexity if we want to separate rtc and watchdog for one 
chip supports. For one chip which supports rtc/alarm and watchdog, can we 
upstream rtc/alarm and watchdog functions to these driver no matter where it's 
in rtc or watchdog subsystem?


> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Best regards,
Johnson

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-23  7:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 10:03 [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver Johnson CH Chen (陳昭勳)
2020-06-22 11:14 ` Lee Jones
2020-06-22 14:26   ` Guenter Roeck
2020-06-22 15:01     ` Alexandre Belloni
2020-06-23  7:12       ` Johnson CH Chen (陳昭勳)
2020-06-22 15:38 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).