From: Ran Bi <ran.bi@mediatek.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>,
Rob Herring <robh+dt@kernel.org>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Mark Rutland <mark.rutland@arm.com>,
Sean Wang <sean.wang@mediatek.com>,
"Sebastian Reichel" <sre@kernel.org>,
Eddie Huang <eddie.huang@mediatek.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Frank Wunderlich <frank-w@public-files.de>,
Thomas Gleixner <tglx@linutronix.de>,
"Richard Fontana" <rfontana@redhat.com>,
Josef Friedl <josef.friedl@speed.at>,
<devicetree@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-mediatek@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <linux-pm@vger.kernel.org>,
<linux-rtc@vger.kernel.org>,
Nicolas Boichat <drinkcat@chromium.org>,
<srv_heupstream@mediatek.com>
Subject: Re: [PATCH v10 4/5] rtc: mt6397: Add support for the MediaTek MT6358 RTC
Date: Fri, 13 Mar 2020 16:30:56 +0800 [thread overview]
Message-ID: <1584088256.16960.9.camel@mhfsdcap03> (raw)
In-Reply-To: <20200313072230.GC3142@dell>
On Fri, 2020-03-13 at 07:22 +0000, Lee Jones wrote:
> On Thu, 12 Mar 2020, Ran Bi wrote:
>
> > On Thu, 2020-03-12 at 07:44 +0000, Lee Jones wrote:
> > > On Wed, 11 Mar 2020, Hsin-Hsiung Wang wrote:
> > >
> > > > From: Ran Bi <ran.bi@mediatek.com>
> > > >
> > > > This add support for the MediaTek MT6358 RTC. Driver using
> > > > compatible data to store different RTC_WRTGR address offset.
> > > > This replace RTC_WRTGR to RTC_WRTGR_MT6323 in mt6323-poweroff
> > > > driver which only needed by armv7 CPU without ATF.
> > > >
> > > > Signed-off-by: Ran Bi <ran.bi@mediatek.com>
> > > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > > ---
> > > > drivers/power/reset/mt6323-poweroff.c | 2 +-
> > > > drivers/rtc/rtc-mt6397.c | 32 ++++++++++++++++++++++++--------
> > > > include/linux/mfd/mt6397/rtc.h | 9 ++++++++-
> > > > 3 files changed, 33 insertions(+), 10 deletions(-)
> > > >
> >
> > <...>
> >
> > > >
> > > > #define RTC_IRQ_STA 0x0002
> > > > #define RTC_IRQ_STA_AL BIT(0)
> > > > @@ -65,6 +67,10 @@
> > > > #define MTK_RTC_POLL_DELAY_US 10
> > > > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> > > >
> > > > +struct mtk_rtc_data {
> > > > + u32 wrtgr;
> > > > +};
> > >
> > > Do you expect to add more properties to this struct?
> > >
> > > If not, it seems a bit overkill.
> > >
> >
> > Yes, we would add more properties here in future patches.
> >
> > > > struct mt6397_rtc {
> > > > struct device *dev;
> > > > struct rtc_device *rtc_dev;
> > > > @@ -74,6 +80,7 @@ struct mt6397_rtc {
> > > > struct regmap *regmap;
> > > > int irq;
> > > > u32 addr_base;
> > > > + const struct mtk_rtc_data *data;
> > >
> > > 'data' is a terrible variable name.
> > >
> > > Why do you need to store this?
> > >
> > > It's one variable which is used once AFAICT.
> >
> > I would rename 'data' to 'config'.
> >
> > This struct will be extended in future patches to achieve more PMIC chip
> > compatibility.
>
> On closer inspection, it looks like wrtgr (also not a great name for a
> variable by the way) is a register address. Is that correct?
> Initially I thought it was a model number, which would have been a
> suitable candidate for entry into OF .data.
>
> However, describing register addresses in OF .data does not sound like
> good practice. It is usually used to identify a platform in the cases
> where platforms cannot be otherwise dynamically interrogated for model
> number via a register read.
>
> Describing register maps via 'config' data is a slippery slope.
>
wrtgr is a special register called "write trigger" which could apply RTC
register change after write 1 to this register. I suppose I could rename
it to "trigger".
Most RTC register offset was same between 6397 and 6358 PMIC chip except
this trigger register. So I would like to store this difference into OF
data. Otherwise, I need a long if-else condition based on model number
or register read.
WARNING: multiple messages have this Message-ID (diff)
From: Ran Bi <ran.bi@mediatek.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Nicolas Boichat <drinkcat@chromium.org>,
srv_heupstream@mediatek.com,
Frank Wunderlich <frank-w@public-files.de>,
Josef Friedl <josef.friedl@speed.at>,
linux-pm@vger.kernel.org, Sean Wang <sean.wang@mediatek.com>,
Sebastian Reichel <sre@kernel.org>,
linux-kernel@vger.kernel.org,
Richard Fontana <rfontana@redhat.com>,
devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>,
Eddie Huang <eddie.huang@mediatek.com>,
Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>,
linux-rtc@vger.kernel.org
Subject: Re: [PATCH v10 4/5] rtc: mt6397: Add support for the MediaTek MT6358 RTC
Date: Fri, 13 Mar 2020 16:30:56 +0800 [thread overview]
Message-ID: <1584088256.16960.9.camel@mhfsdcap03> (raw)
In-Reply-To: <20200313072230.GC3142@dell>
On Fri, 2020-03-13 at 07:22 +0000, Lee Jones wrote:
> On Thu, 12 Mar 2020, Ran Bi wrote:
>
> > On Thu, 2020-03-12 at 07:44 +0000, Lee Jones wrote:
> > > On Wed, 11 Mar 2020, Hsin-Hsiung Wang wrote:
> > >
> > > > From: Ran Bi <ran.bi@mediatek.com>
> > > >
> > > > This add support for the MediaTek MT6358 RTC. Driver using
> > > > compatible data to store different RTC_WRTGR address offset.
> > > > This replace RTC_WRTGR to RTC_WRTGR_MT6323 in mt6323-poweroff
> > > > driver which only needed by armv7 CPU without ATF.
> > > >
> > > > Signed-off-by: Ran Bi <ran.bi@mediatek.com>
> > > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > > ---
> > > > drivers/power/reset/mt6323-poweroff.c | 2 +-
> > > > drivers/rtc/rtc-mt6397.c | 32 ++++++++++++++++++++++++--------
> > > > include/linux/mfd/mt6397/rtc.h | 9 ++++++++-
> > > > 3 files changed, 33 insertions(+), 10 deletions(-)
> > > >
> >
> > <...>
> >
> > > >
> > > > #define RTC_IRQ_STA 0x0002
> > > > #define RTC_IRQ_STA_AL BIT(0)
> > > > @@ -65,6 +67,10 @@
> > > > #define MTK_RTC_POLL_DELAY_US 10
> > > > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> > > >
> > > > +struct mtk_rtc_data {
> > > > + u32 wrtgr;
> > > > +};
> > >
> > > Do you expect to add more properties to this struct?
> > >
> > > If not, it seems a bit overkill.
> > >
> >
> > Yes, we would add more properties here in future patches.
> >
> > > > struct mt6397_rtc {
> > > > struct device *dev;
> > > > struct rtc_device *rtc_dev;
> > > > @@ -74,6 +80,7 @@ struct mt6397_rtc {
> > > > struct regmap *regmap;
> > > > int irq;
> > > > u32 addr_base;
> > > > + const struct mtk_rtc_data *data;
> > >
> > > 'data' is a terrible variable name.
> > >
> > > Why do you need to store this?
> > >
> > > It's one variable which is used once AFAICT.
> >
> > I would rename 'data' to 'config'.
> >
> > This struct will be extended in future patches to achieve more PMIC chip
> > compatibility.
>
> On closer inspection, it looks like wrtgr (also not a great name for a
> variable by the way) is a register address. Is that correct?
> Initially I thought it was a model number, which would have been a
> suitable candidate for entry into OF .data.
>
> However, describing register addresses in OF .data does not sound like
> good practice. It is usually used to identify a platform in the cases
> where platforms cannot be otherwise dynamically interrogated for model
> number via a register read.
>
> Describing register maps via 'config' data is a slippery slope.
>
wrtgr is a special register called "write trigger" which could apply RTC
register change after write 1 to this register. I suppose I could rename
it to "trigger".
Most RTC register offset was same between 6397 and 6358 PMIC chip except
this trigger register. So I would like to store this difference into OF
data. Otherwise, I need a long if-else condition based on model number
or register read.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Ran Bi <ran.bi@mediatek.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Nicolas Boichat <drinkcat@chromium.org>,
srv_heupstream@mediatek.com,
Frank Wunderlich <frank-w@public-files.de>,
Josef Friedl <josef.friedl@speed.at>,
linux-pm@vger.kernel.org, Sean Wang <sean.wang@mediatek.com>,
Sebastian Reichel <sre@kernel.org>,
linux-kernel@vger.kernel.org,
Richard Fontana <rfontana@redhat.com>,
devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>,
Eddie Huang <eddie.huang@mediatek.com>,
Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>,
linux-rtc@vger.kernel.org
Subject: Re: [PATCH v10 4/5] rtc: mt6397: Add support for the MediaTek MT6358 RTC
Date: Fri, 13 Mar 2020 16:30:56 +0800 [thread overview]
Message-ID: <1584088256.16960.9.camel@mhfsdcap03> (raw)
In-Reply-To: <20200313072230.GC3142@dell>
On Fri, 2020-03-13 at 07:22 +0000, Lee Jones wrote:
> On Thu, 12 Mar 2020, Ran Bi wrote:
>
> > On Thu, 2020-03-12 at 07:44 +0000, Lee Jones wrote:
> > > On Wed, 11 Mar 2020, Hsin-Hsiung Wang wrote:
> > >
> > > > From: Ran Bi <ran.bi@mediatek.com>
> > > >
> > > > This add support for the MediaTek MT6358 RTC. Driver using
> > > > compatible data to store different RTC_WRTGR address offset.
> > > > This replace RTC_WRTGR to RTC_WRTGR_MT6323 in mt6323-poweroff
> > > > driver which only needed by armv7 CPU without ATF.
> > > >
> > > > Signed-off-by: Ran Bi <ran.bi@mediatek.com>
> > > > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> > > > ---
> > > > drivers/power/reset/mt6323-poweroff.c | 2 +-
> > > > drivers/rtc/rtc-mt6397.c | 32 ++++++++++++++++++++++++--------
> > > > include/linux/mfd/mt6397/rtc.h | 9 ++++++++-
> > > > 3 files changed, 33 insertions(+), 10 deletions(-)
> > > >
> >
> > <...>
> >
> > > >
> > > > #define RTC_IRQ_STA 0x0002
> > > > #define RTC_IRQ_STA_AL BIT(0)
> > > > @@ -65,6 +67,10 @@
> > > > #define MTK_RTC_POLL_DELAY_US 10
> > > > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> > > >
> > > > +struct mtk_rtc_data {
> > > > + u32 wrtgr;
> > > > +};
> > >
> > > Do you expect to add more properties to this struct?
> > >
> > > If not, it seems a bit overkill.
> > >
> >
> > Yes, we would add more properties here in future patches.
> >
> > > > struct mt6397_rtc {
> > > > struct device *dev;
> > > > struct rtc_device *rtc_dev;
> > > > @@ -74,6 +80,7 @@ struct mt6397_rtc {
> > > > struct regmap *regmap;
> > > > int irq;
> > > > u32 addr_base;
> > > > + const struct mtk_rtc_data *data;
> > >
> > > 'data' is a terrible variable name.
> > >
> > > Why do you need to store this?
> > >
> > > It's one variable which is used once AFAICT.
> >
> > I would rename 'data' to 'config'.
> >
> > This struct will be extended in future patches to achieve more PMIC chip
> > compatibility.
>
> On closer inspection, it looks like wrtgr (also not a great name for a
> variable by the way) is a register address. Is that correct?
> Initially I thought it was a model number, which would have been a
> suitable candidate for entry into OF .data.
>
> However, describing register addresses in OF .data does not sound like
> good practice. It is usually used to identify a platform in the cases
> where platforms cannot be otherwise dynamically interrogated for model
> number via a register read.
>
> Describing register maps via 'config' data is a slippery slope.
>
wrtgr is a special register called "write trigger" which could apply RTC
register change after write 1 to this register. I suppose I could rename
it to "trigger".
Most RTC register offset was same between 6397 and 6358 PMIC chip except
this trigger register. So I would like to store this difference into OF
data. Otherwise, I need a long if-else condition based on model number
or register read.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-03-13 8:31 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-11 9:16 [PATCH v10 0/5] Add Support for MediaTek PMIC MT6358 Hsin-Hsiung Wang
2020-03-11 9:16 ` Hsin-Hsiung Wang
2020-03-11 9:16 ` Hsin-Hsiung Wang
2020-03-11 9:16 ` [PATCH v10 1/5] mfd: mt6397: modify suspend/resume behavior Hsin-Hsiung Wang
2020-03-11 9:16 ` Hsin-Hsiung Wang
2020-03-11 9:16 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` [PATCH v10 2/5] dt-bindings: mfd: Add compatible for the MediaTek MT6358 PMIC Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` [PATCH v10 3/5] mfd: Add support " Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-12 1:41 ` Nicolas Boichat
2020-03-12 1:41 ` Nicolas Boichat
2020-03-12 1:41 ` Nicolas Boichat
2020-03-25 9:43 ` Lee Jones
2020-03-25 9:43 ` Lee Jones
2020-03-25 9:43 ` Lee Jones
2020-04-01 8:15 ` Hsin-hsiung Wang
2020-04-01 8:15 ` Hsin-hsiung Wang
2020-04-01 8:15 ` Hsin-hsiung Wang
2020-04-01 8:26 ` Hsin-hsiung Wang
2020-04-01 8:26 ` Hsin-hsiung Wang
2020-04-01 8:26 ` Hsin-hsiung Wang
2020-03-11 9:17 ` [PATCH v10 4/5] rtc: mt6397: Add support for the MediaTek MT6358 RTC Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 22:24 ` Sebastian Reichel
2020-03-11 22:24 ` Sebastian Reichel
2020-03-11 22:24 ` Sebastian Reichel
2020-03-12 1:47 ` Nicolas Boichat
2020-03-12 1:47 ` Nicolas Boichat
2020-03-12 1:47 ` Nicolas Boichat
2020-03-12 7:44 ` Lee Jones
2020-03-12 7:44 ` Lee Jones
2020-03-12 7:44 ` Lee Jones
2020-03-12 8:57 ` Ran Bi
2020-03-12 8:57 ` Ran Bi
2020-03-12 8:57 ` Ran Bi
2020-03-13 7:22 ` Lee Jones
2020-03-13 7:22 ` Lee Jones
2020-03-13 7:22 ` Lee Jones
2020-03-13 7:47 ` Alexandre Belloni
2020-03-13 7:47 ` Alexandre Belloni
2020-03-13 7:47 ` Alexandre Belloni
2020-03-13 8:30 ` Ran Bi [this message]
2020-03-13 8:30 ` Ran Bi
2020-03-13 8:30 ` Ran Bi
2020-03-11 9:17 ` [PATCH v10 5/5] arm64: dts: mt6358: add PMIC MT6358 related nodes Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
2020-03-11 9:17 ` Hsin-Hsiung Wang
Reply instructions:
You may reply publicly 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=1584088256.16960.9.camel@mhfsdcap03 \
--to=ran.bi@mediatek.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=drinkcat@chromium.org \
--cc=eddie.huang@mediatek.com \
--cc=frank-w@public-files.de \
--cc=hsin-hsiung.wang@mediatek.com \
--cc=josef.friedl@speed.at \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=rfontana@redhat.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=sre@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=tglx@linutronix.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.