linux-kernel - Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Jun 2020 07:26:56 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Johnson CH Chen (陳昭勳) 
        <JohnsonCH.Chen@...a.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>
Subject: Re: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core
 driver

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@...a.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