Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: "lee.jones@linaro.org" <lee.jones@linaro.org>
Cc: "linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
	"dmurphy@ti.com" <dmurphy@ti.com>,
	"linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"mazziesaccount@gmail.com" <mazziesaccount@gmail.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"jacek.anaszewski@gmail.com" <jacek.anaszewski@gmail.com>,
	"a.zummo@towertech.it" <a.zummo@towertech.it>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"pavel@ucw.cz" <pavel@ucw.cz>,
	"broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [PATCH v8 09/12] rtc: bd70528: add BD71828 support
Date: Wed, 8 Jan 2020 08:11:45 +0000
Message-ID: <539ae83e249d50a57f86df1b6855f420767de312.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <20200107125751.GJ14821@dell>

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@fi.rohmeurope.com>
> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.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


  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-30  9:33 [PATCH v8 00/12] Support ROHM BD71828 PMIC Matti Vaittinen
2019-12-30  9:34 ` [PATCH v8 01/12] dt-bindings: leds: ROHM BD71282 PMIC LED driver Matti Vaittinen
2019-12-30  9:35 ` [PATCH v8 02/12] dt-bindings: mfd: Document ROHM BD71828 bindings Matti Vaittinen
2019-12-30  9:36 ` [PATCH v8 03/12] mfd: rohm PMICs - use platform_device_id to match MFD sub-devices Matti Vaittinen
2019-12-30  9:36 ` [PATCH v8 04/12] mfd: bd718x7: Add compatible for BD71850 Matti Vaittinen
2020-01-07 12:36   ` Lee Jones
2019-12-30  9:37 ` [PATCH v8 05/12] mfd: bd71828: Support ROHM BD71828 PMIC - core Matti Vaittinen
2019-12-30  9:38 ` [PATCH v8 06/12] mfd: input: bd71828: Add power-key support Matti Vaittinen
2019-12-30  9:38 ` [PATCH v8 07/12] clk: bd718x7: Support ROHM BD71828 clk block Matti Vaittinen
2019-12-30  9:39 ` [PATCH v8 08/12] regulator: bd718x7: Split driver to common and bd718x7 specific parts Matti Vaittinen
2020-01-07 12:41   ` Lee Jones
2020-01-08  8:34     ` Vaittinen, Matti
2020-01-13 10:53       ` Lee Jones
2020-01-13 11:49         ` Vaittinen, Matti
2020-01-13 12:11           ` Lee Jones
2020-01-15  8:29             ` Lee Jones
2020-01-15  8:34               ` Vaittinen, Matti
2019-12-30  9:40 ` [PATCH v8 09/12] rtc: bd70528: add BD71828 support Matti Vaittinen
2020-01-07 12:57   ` Lee Jones
2020-01-08  8:11     ` Vaittinen, Matti [this message]
2019-12-30  9:40 ` [PATCH v8 10/12] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs Matti Vaittinen
2019-12-30  9:45 ` [PATCH v8 11/12] leds: Add common LED binding parsing support to LED class/core Matti Vaittinen
2019-12-30  9:45 ` [PATCH v8 12/12] led: bd71828: Support LED outputs on ROHM BD71828 PMIC Matti Vaittinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=539ae83e249d50a57f86df1b6855f420767de312.camel@fi.rohmeurope.com \
    --to=matti.vaittinen@fi.rohmeurope.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mazziesaccount@gmail.com \
    --cc=mturquette@baylibre.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git