[v1,02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

From: Sean Wang <sean.wang@mediatek.com>

Add device-tree binding for MediaTek PMIC based RTC.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt

Hi,

On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add device-tree binding for MediaTek PMIC based RTC.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> new file mode 100644
> index 0000000..83ff6be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> @@ -0,0 +1,39 @@
> +Device-Tree bindings for MediaTek PMIC based RTC
> +
> +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> +is working as a multi-function device (MFD). And the RTC can be configured and
> +set up via PMIC wrapper bus. Which is also common resource shared among the
> +other functions present on the PMIC.
> +
> +For MediaTek PMIC wrapper bus bindings, see:
> +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +
> +Required parent node:
> +- pmic
> +  For MediaTek PMIC MFD bindings, see:
> +  Documentation/devicetree/bindings/mfd/mt6397.txt
> +
> +Required properties:
> +- compatible: Should be one of follows
> +	"mediatek,mt6323-rtc": for MT6323 PMIC
> +	"mediatek,mt6397-rtc": for MT6397 PMIC
> +
> +Optional child node:
> +- power-off
> +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> +
> +Example:
> +
> +	pmic {
> +		compatible = "mediatek,mt6323";
> +
> +		...
> +		rtc {
> +			compatible = "mediatek,mt6323-rtc";
> +
> +			power-off {
> +				compatible = "mediatek,mt6323-rtc-poweroff";
> +			};

I'm pretty sure the whole point of mfd is to avoid having the poweroff
controller under the rtc

On 23/03/2018 at 10:41:18 +0100, Alexandre Belloni wrote:
> Hi,
> 
> On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Add device-tree binding for MediaTek PMIC based RTC.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > new file mode 100644
> > index 0000000..83ff6be
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > @@ -0,0 +1,39 @@
> > +Device-Tree bindings for MediaTek PMIC based RTC
> > +
> > +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> > +is working as a multi-function device (MFD). And the RTC can be configured and
> > +set up via PMIC wrapper bus. Which is also common resource shared among the
> > +other functions present on the PMIC.
> > +
> > +For MediaTek PMIC wrapper bus bindings, see:
> > +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> > +
> > +Required parent node:
> > +- pmic
> > +  For MediaTek PMIC MFD bindings, see:
> > +  Documentation/devicetree/bindings/mfd/mt6397.txt
> > +
> > +Required properties:
> > +- compatible: Should be one of follows
> > +	"mediatek,mt6323-rtc": for MT6323 PMIC
> > +	"mediatek,mt6397-rtc": for MT6397 PMIC
> > +
> > +Optional child node:
> > +- power-off
> > +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> > +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > +
> > +Example:
> > +
> > +	pmic {
> > +		compatible = "mediatek,mt6323";
> > +
> > +		...
> > +		rtc {
> > +			compatible = "mediatek,mt6323-rtc";
> > +
> > +			power-off {
> > +				compatible = "mediatek,mt6323-rtc-poweroff";
> > +			};
> 
> I'm pretty sure the whole point of mfd is to avoid having the poweroff
> controller under the rtc
> 

BTW, I think it is enough to have that documented in only one file (the
MFD one is enough)

> 
> -- 
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

On Fri, 2018-03-23 at 11:15 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 10:41:18 +0100, Alexandre Belloni wrote:
> > Hi,
> > 
> > On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> > > From: Sean Wang <sean.wang@mediatek.com>
> > > 
> > > Add device-tree binding for MediaTek PMIC based RTC.
> > > 
> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > new file mode 100644
> > > index 0000000..83ff6be
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > @@ -0,0 +1,39 @@
> > > +Device-Tree bindings for MediaTek PMIC based RTC
> > > +
> > > +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> > > +is working as a multi-function device (MFD). And the RTC can be configured and
> > > +set up via PMIC wrapper bus. Which is also common resource shared among the
> > > +other functions present on the PMIC.
> > > +
> > > +For MediaTek PMIC wrapper bus bindings, see:
> > > +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> > > +
> > > +Required parent node:
> > > +- pmic
> > > +  For MediaTek PMIC MFD bindings, see:
> > > +  Documentation/devicetree/bindings/mfd/mt6397.txt
> > > +
> > > +Required properties:
> > > +- compatible: Should be one of follows
> > > +	"mediatek,mt6323-rtc": for MT6323 PMIC
> > > +	"mediatek,mt6397-rtc": for MT6397 PMIC
> > > +
> > > +Optional child node:
> > > +- power-off
> > > +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> > > +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > > +
> > > +Example:
> > > +
> > > +	pmic {
> > > +		compatible = "mediatek,mt6323";
> > > +
> > > +		...
> > > +		rtc {
> > > +			compatible = "mediatek,mt6323-rtc";
> > > +
> > > +			power-off {
> > > +				compatible = "mediatek,mt6323-rtc-poweroff";
> > > +			};
> > 
> > I'm pretty sure the whole point of mfd is to avoid having the poweroff
> > controller under the rtc
> > 
> 
> BTW, I think it is enough to have that documented in only one file (the
> MFD one is enough)
> 

just reply both replies in the same mail

1.) the power-off device is a part of rtc, use the same registers rtc
has and thus it is put as child nodes under the node rtc to reflect the
reality of characteristics the rtc has.

Or am I wrong for a certain aspect in these opinions?

2) the other sub-functions for the same pmic already created its own
dt-binding document belonged to its corresponding subsystem. Don't we
really want to follow it them all?

> > 
> > -- 
> > Alexandre Belloni, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>

On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> just reply both replies in the same mail
> 
> 1.) the power-off device is a part of rtc, use the same registers rtc
> has and thus it is put as child nodes under the node rtc to reflect the
> reality of characteristics the rtc has.
> 
> Or am I wrong for a certain aspect in these opinions?
> 

My point is that it is also part of the PMIC so it may as well be
registers from the mfd driver which already registers a bunch of devices
instead of doing unusual stuff from the rtc driver.

mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
benefit that if the RTC driver probe fails for some reason, you may
still be able to probe the reset driver.

I don't tink there is any benefit having it as a child of the rtc
device.

> 2) the other sub-functions for the same pmic already created its own
> dt-binding document belonged to its corresponding subsystem. Don't we
> really want to follow it them all?
> 

Ok, that's fine.

On Tue, 2018-03-27 at 17:18 +0200, Alexandre Belloni wrote:
> On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> > just reply both replies in the same mail
> > 
> > 1.) the power-off device is a part of rtc, use the same registers rtc
> > has and thus it is put as child nodes under the node rtc to reflect the
> > reality of characteristics the rtc has.
> > 
> > Or am I wrong for a certain aspect in these opinions?
> > 
> 
> My point is that it is also part of the PMIC so it may as well be
> registers from the mfd driver which already registers a bunch of devices
> instead of doing unusual stuff from the rtc driver.
> 
> mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
> benefit that if the RTC driver probe fails for some reason, you may
> still be able to probe the reset driver.
> 
> I don't tink there is any benefit having it as a child of the rtc
> device.
> 


really thanks! it's an optional solution I thought it 's fine and worth
doing

but so far I cannot fully make sure of whether mfd can accept two
devices holding overlay IORESOURCE_MEM.

Or do you like Rob's suggestion in [1] ? By which, I tend to embed a
sub-device with platform_device_register_data api in the rtc probe()
instead of treating it as a dt node under rtc node, but which seems
something a bit violates your preferences :(

Just confirm to know which way I should step into before I produce next
version.

[1]
http://lists.infradead.org/pipermail/linux-mediatek/2018-March/012576.html

> > 2) the other sub-functions for the same pmic already created its own
> > dt-binding document belonged to its corresponding subsystem. Don't we
> > really want to follow it them all?
> > 
> 
> Ok, that's fine.
>

On 28/03/2018 at 11:53:17 +0800, Sean Wang wrote:
> On Tue, 2018-03-27 at 17:18 +0200, Alexandre Belloni wrote:
> > On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> > > just reply both replies in the same mail
> > > 
> > > 1.) the power-off device is a part of rtc, use the same registers rtc
> > > has and thus it is put as child nodes under the node rtc to reflect the
> > > reality of characteristics the rtc has.
> > > 
> > > Or am I wrong for a certain aspect in these opinions?
> > > 
> > 
> > My point is that it is also part of the PMIC so it may as well be
> > registers from the mfd driver which already registers a bunch of devices
> > instead of doing unusual stuff from the rtc driver.
> > 
> > mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
> > benefit that if the RTC driver probe fails for some reason, you may
> > still be able to probe the reset driver.
> > 
> > I don't tink there is any benefit having it as a child of the rtc
> > device.
> > 
> 
> 
> really thanks! it's an optional solution I thought it 's fine and worth
> doing
> 
> but so far I cannot fully make sure of whether mfd can accept two
> devices holding overlay IORESOURCE_MEM.
> 

There is no overlay because you are using a regmap which handles
concurrency for you.

What your patch is doing is:

struct mt6397_rtc *rtc = dev_get_drvdata(pdev->dev.parent);

then you use rtc->regmap

But in the rtc driver, you have:

struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
struct mt6397_rtc *rtc;

rtc->regmap = mt6397_chip->regmap;

So there is no benefit from being the child of the rtc, you could just
do the following in your reset driver:

struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);

and then use mt6397_chip->regmap.