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
