diff mbox series

[1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver

Message ID HK2PR01MB328111AB2520315A7D4A1172FA970@HK2PR01MB3281.apcprd01.prod.exchangelabs.com (mailing list archive)
State Rejected
Headers show
Series Use MFD for Dallas/Maxim DS1374 driver series | expand

Commit Message

Johnson CH Chen (陳昭勳) June 22, 2020, 10:03 a.m. UTC 
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

Comments

Lee Jones June 22, 2020, 11:14 a.m. UTC | #1 
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
Guenter Roeck June 22, 2020, 2:26 p.m. UTC | #2 
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
Alexandre Belloni June 22, 2020, 3:01 p.m. UTC | #3 
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.
kernel test robot June 22, 2020, 3:38 p.m. UTC | #4 
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
Johnson CH Chen (陳昭勳) June 23, 2020, 7:12 a.m. UTC | #5 
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
diff mbox series

Patch

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");