linux-kernel - Re: [PATCH v8 09/12] rtc: bd70528: add BD71828 support

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]

Message-ID: <539ae83e249d50a57f86df1b6855f420767de312.camel@fi.rohmeurope.com>
Date:   Wed, 8 Jan 2020 08:11:45 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "lee.jones@...aro.org" <lee.jones@...aro.org>
CC:     "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "dmurphy@...com" <dmurphy@...com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "jacek.anaszewski@...il.com" <jacek.anaszewski@...il.com>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        "pavel@....cz" <pavel@....cz>,
        "broonie@...nel.org" <broonie@...nel.org>
Subject: Re: [PATCH v8 09/12] rtc: bd70528: add BD71828 support

Hello Lee,

Thanks for taking a look at this.

On Tue, 2020-01-07 at 12:57 +0000, Lee Jones wrote:
> On Mon, 30 Dec 2019, Matti Vaittinen wrote:
> 
> > ROHM BD71828 PMIC RTC block is from many parts similar to one
> > on BD70528. Support BD71828 RTC using BD70528 RTC driver and
> > avoid re-inventing the wheel.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> > Acked-by: Alexandre Belloni <alexandre.belloni@...tlin.com>
> > ---
> > +
> >  struct bd70528_rtc_alm {
> >  	struct bd70528_rtc_data data;
> >  	u8 alm_mask;
> > @@ -45,6 +53,8 @@ struct bd70528_rtc_alm {
> >  struct bd70528_rtc {
> >  	struct rohm_regmap_dev *mfd;
> 
> I think it would be better if you fixed this up be more forthcoming.
> It took some grepping to find out what this actually meant.  An MFD
> isn't really a thing, we made it up.  Here you are referring to this
> platform device's parent's device data.

I like MFD. Multi Function Device is a real thing. Device with multiple
functionalities meld in. It describes many PMICs or FPGA designs
terribly well. But the naming is not something I like fighting for - if
MFD is not nice to your eyes we can change it. But let's do it in
separate patch set Ok? Changing the "rohm_regmap_dev" will involve
changing bunch of existing drivers and is not by any means related with
adding the support for BD71828.

> 
> With that in mind I offer some suggestions:
> 
>   'struct rohm_parent_ddata pddata'
>   'struct rohm_parent_ddata parent'

Both are fine with me but this change is reflected to drivers not
related to BD71828 like:
bd70528-regulator.c
gpio-bd70528.c
watchdog/bd70528_wdt.c

I'd rather not change WDT with this series. So I'd prefer incremental
patch for this in the release following this series.
 
> >  /* WDT masks */
> > diff --git a/include/linux/mfd/rohm-bd71828.h
> > b/include/linux/mfd/rohm-bd71828.h
> > index d013e03f742d..017a4c01cb31 100644
> > --- a/include/linux/mfd/rohm-bd71828.h
> > +++ b/include/linux/mfd/rohm-bd71828.h
> > @@ -5,6 +5,7 @@
> >  #define __LINUX_MFD_BD71828_H__
> >  
> >  #include <linux/mfd/rohm-generic.h>
> > +#include <linux/mfd/rohm-shared.h>
> 
> Isn't generic shared?

Good point. The rohm-shared contains stuff common for only few of the
PMICs (currently BD70528 and BD71828) where as rohm-generic is intended
to be used for stuff that is generic to more or less all of the PMICs.
Or that was my initial idea. But as I've been told - naming-is-hard :)
Suggestions?

> 
> > b/include/linux/mfd/rohm-shared.h
> > new file mode 100644
> > index 000000000000..f16fc3b5000e
> > --- /dev/null
> > +++ b/include/linux/mfd/rohm-shared.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/* Copyright (C) 2018 ROHM Semiconductors */
> 
> This is very out of data now!

Ok.

> > +/*
> > + * RTC definitions shared between
> > + *
> > + * BD70528
> > + * and BD71828
> 
> This reads poorly.
> 
> Either form a bullet pointed list, or just write it out.

Ok


Best Regards
	Matti