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

[Vaittinen, Matti]

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@xxxxxxxxxxxxxxxxx>
> > Acked-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > ---
> > +
> >  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