| Message ID | 1583918223-22506-5-git-send-email-hsin-hsiung.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Support for MediaTek PMIC MT6358 | expand |
Hi, On Wed, Mar 11, 2020 at 05:17:02PM +0800, 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> > --- Should be fine to simply merge this through RTC: Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian > 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(-) > > diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c > index 1caf43d..0532803 100644 > --- a/drivers/power/reset/mt6323-poweroff.c > +++ b/drivers/power/reset/mt6323-poweroff.c > @@ -30,7 +30,7 @@ static void mt6323_do_pwroff(void) > int ret; > > regmap_write(pwrc->regmap, pwrc->base + RTC_BBPU, RTC_BBPU_KEY); > - regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR, 1); > + regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR_MT6323, 1); > > ret = regmap_read_poll_timeout(pwrc->regmap, > pwrc->base + RTC_BBPU, val, > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > index cda238d..7a5a9e2 100644 > --- a/drivers/rtc/rtc-mt6397.c > +++ b/drivers/rtc/rtc-mt6397.c > @@ -9,18 +9,38 @@ > #include <linux/mfd/mt6397/core.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/rtc.h> > #include <linux/mfd/mt6397/rtc.h> > #include <linux/mod_devicetable.h> > > +static const struct mtk_rtc_data mt6358_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6358, > +}; > + > +static const struct mtk_rtc_data mt6397_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6397, > +}; > + > +static const struct of_device_id mt6397_rtc_of_match[] = { > + { .compatible = "mediatek,mt6323-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + { .compatible = "mediatek,mt6358-rtc", > + .data = (void *)&mt6358_rtc_data, }, > + { .compatible = "mediatek,mt6397-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > + > static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) > { > int ret; > u32 data; > > - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1); > + ret = regmap_write(rtc->regmap, rtc->addr_base + rtc->data->wrtgr, 1); > if (ret < 0) > return ret; > > @@ -269,6 +289,9 @@ static int mtk_rtc_probe(struct platform_device *pdev) > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > rtc->addr_base = res->start; > > + rtc->data = (struct mtk_rtc_data *) > + of_device_get_match_data(&pdev->dev); > + > rtc->irq = platform_get_irq(pdev, 0); > if (rtc->irq < 0) > return rtc->irq; > @@ -325,13 +348,6 @@ static int mt6397_rtc_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend, > mt6397_rtc_resume); > > -static const struct of_device_id mt6397_rtc_of_match[] = { > - { .compatible = "mediatek,mt6323-rtc", }, > - { .compatible = "mediatek,mt6397-rtc", }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > - > static struct platform_driver mtk_rtc_driver = { > .driver = { > .name = "mt6397-rtc", > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h > index 7dfb63b..66534ed 100644 > --- a/include/linux/mfd/mt6397/rtc.h > +++ b/include/linux/mfd/mt6397/rtc.h > @@ -18,7 +18,9 @@ > #define RTC_BBPU_CBUSY BIT(6) > #define RTC_BBPU_KEY (0x43 << 8) > > -#define RTC_WRTGR 0x003c > +#define RTC_WRTGR_MT6358 0x3a > +#define RTC_WRTGR_MT6397 0x3c > +#define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397 > > #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; > +}; > + > 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; > }; > > #endif /* _LINUX_MFD_MT6397_RTC_H_ */ > -- > 2.6.4
On Wed, Mar 11, 2020 at 5:17 PM Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com> 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(-) > > diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c > index 1caf43d..0532803 100644 > --- a/drivers/power/reset/mt6323-poweroff.c > +++ b/drivers/power/reset/mt6323-poweroff.c > @@ -30,7 +30,7 @@ static void mt6323_do_pwroff(void) > int ret; > > regmap_write(pwrc->regmap, pwrc->base + RTC_BBPU, RTC_BBPU_KEY); > - regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR, 1); > + regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR_MT6323, 1); > > ret = regmap_read_poll_timeout(pwrc->regmap, > pwrc->base + RTC_BBPU, val, > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > index cda238d..7a5a9e2 100644 > --- a/drivers/rtc/rtc-mt6397.c > +++ b/drivers/rtc/rtc-mt6397.c > @@ -9,18 +9,38 @@ > #include <linux/mfd/mt6397/core.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/rtc.h> > #include <linux/mfd/mt6397/rtc.h> > #include <linux/mod_devicetable.h> > > +static const struct mtk_rtc_data mt6358_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6358, > +}; > + > +static const struct mtk_rtc_data mt6397_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6397, > +}; > + > +static const struct of_device_id mt6397_rtc_of_match[] = { > + { .compatible = "mediatek,mt6323-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + { .compatible = "mediatek,mt6358-rtc", > + .data = (void *)&mt6358_rtc_data, }, > + { .compatible = "mediatek,mt6397-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > + > static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) > { > int ret; > u32 data; > > - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1); > + ret = regmap_write(rtc->regmap, rtc->addr_base + rtc->data->wrtgr, 1); > if (ret < 0) > return ret; > > @@ -269,6 +289,9 @@ static int mtk_rtc_probe(struct platform_device *pdev) > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > rtc->addr_base = res->start; > > + rtc->data = (struct mtk_rtc_data *) > + of_device_get_match_data(&pdev->dev); Sorry for not noticing earlier, the cast is not needed. (also, you cast a const pointer into a non-const, which doesn't matter anyway as rtc->data is const again, but still...). > + > rtc->irq = platform_get_irq(pdev, 0); > if (rtc->irq < 0) > return rtc->irq; > @@ -325,13 +348,6 @@ static int mt6397_rtc_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend, > mt6397_rtc_resume); > > -static const struct of_device_id mt6397_rtc_of_match[] = { > - { .compatible = "mediatek,mt6323-rtc", }, > - { .compatible = "mediatek,mt6397-rtc", }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > - Why are you moving the MODULE_DEVICE_TABLE to the top of the file? I think you can keep it here with the mt63xx_rtc_data structs? > static struct platform_driver mtk_rtc_driver = { > .driver = { > .name = "mt6397-rtc", > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h > index 7dfb63b..66534ed 100644 > --- a/include/linux/mfd/mt6397/rtc.h > +++ b/include/linux/mfd/mt6397/rtc.h > @@ -18,7 +18,9 @@ > #define RTC_BBPU_CBUSY BIT(6) > #define RTC_BBPU_KEY (0x43 << 8) > > -#define RTC_WRTGR 0x003c > +#define RTC_WRTGR_MT6358 0x3a > +#define RTC_WRTGR_MT6397 0x3c > +#define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397 > > #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; > +}; > + > 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; > }; > > #endif /* _LINUX_MFD_MT6397_RTC_H_ */ > -- > 2.6.4
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(-) > > diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c > index 1caf43d..0532803 100644 > --- a/drivers/power/reset/mt6323-poweroff.c > +++ b/drivers/power/reset/mt6323-poweroff.c > @@ -30,7 +30,7 @@ static void mt6323_do_pwroff(void) > int ret; > > regmap_write(pwrc->regmap, pwrc->base + RTC_BBPU, RTC_BBPU_KEY); > - regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR, 1); > + regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR_MT6323, 1); > > ret = regmap_read_poll_timeout(pwrc->regmap, > pwrc->base + RTC_BBPU, val, > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > index cda238d..7a5a9e2 100644 > --- a/drivers/rtc/rtc-mt6397.c > +++ b/drivers/rtc/rtc-mt6397.c > @@ -9,18 +9,38 @@ > #include <linux/mfd/mt6397/core.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/rtc.h> > #include <linux/mfd/mt6397/rtc.h> > #include <linux/mod_devicetable.h> > > +static const struct mtk_rtc_data mt6358_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6358, > +}; > + > +static const struct mtk_rtc_data mt6397_rtc_data = { > + .wrtgr = RTC_WRTGR_MT6397, > +}; > + > +static const struct of_device_id mt6397_rtc_of_match[] = { > + { .compatible = "mediatek,mt6323-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + { .compatible = "mediatek,mt6358-rtc", > + .data = (void *)&mt6358_rtc_data, }, > + { .compatible = "mediatek,mt6397-rtc", > + .data = (void *)&mt6397_rtc_data, }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > + > static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) > { > int ret; > u32 data; > > - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1); > + ret = regmap_write(rtc->regmap, rtc->addr_base + rtc->data->wrtgr, 1); > if (ret < 0) > return ret; > > @@ -269,6 +289,9 @@ static int mtk_rtc_probe(struct platform_device *pdev) > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > rtc->addr_base = res->start; > > + rtc->data = (struct mtk_rtc_data *) > + of_device_get_match_data(&pdev->dev); > + > rtc->irq = platform_get_irq(pdev, 0); > if (rtc->irq < 0) > return rtc->irq; > @@ -325,13 +348,6 @@ static int mt6397_rtc_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend, > mt6397_rtc_resume); > > -static const struct of_device_id mt6397_rtc_of_match[] = { > - { .compatible = "mediatek,mt6323-rtc", }, > - { .compatible = "mediatek,mt6397-rtc", }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); > - > static struct platform_driver mtk_rtc_driver = { > .driver = { > .name = "mt6397-rtc", > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h > index 7dfb63b..66534ed 100644 > --- a/include/linux/mfd/mt6397/rtc.h > +++ b/include/linux/mfd/mt6397/rtc.h > @@ -18,7 +18,9 @@ > #define RTC_BBPU_CBUSY BIT(6) > #define RTC_BBPU_KEY (0x43 << 8) > > -#define RTC_WRTGR 0x003c > +#define RTC_WRTGR_MT6358 0x3a > +#define RTC_WRTGR_MT6397 0x3c > +#define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397 > > #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. > 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. > }; > > #endif /* _LINUX_MFD_MT6397_RTC_H_ */
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. > > }; > > > > #endif /* _LINUX_MFD_MT6397_RTC_H_ */ >
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.
On 13/03/2020 07:22:30+0000, Lee Jones wrote: > > > > 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. I'm not sure I get what you mean, there are dozens if not hundreds of drivers doing it exactly that way. What is the difference between having .data pointing to a register map and having .data containing a model number and then use that model number to get the register map? of_device_id::data definitively isn't config data as the DT describes the hardware, not the configuration.
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.
diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c index 1caf43d..0532803 100644 --- a/drivers/power/reset/mt6323-poweroff.c +++ b/drivers/power/reset/mt6323-poweroff.c @@ -30,7 +30,7 @@ static void mt6323_do_pwroff(void) int ret; regmap_write(pwrc->regmap, pwrc->base + RTC_BBPU, RTC_BBPU_KEY); - regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR, 1); + regmap_write(pwrc->regmap, pwrc->base + RTC_WRTGR_MT6323, 1); ret = regmap_read_poll_timeout(pwrc->regmap, pwrc->base + RTC_BBPU, val, diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c index cda238d..7a5a9e2 100644 --- a/drivers/rtc/rtc-mt6397.c +++ b/drivers/rtc/rtc-mt6397.c @@ -9,18 +9,38 @@ #include <linux/mfd/mt6397/core.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/rtc.h> #include <linux/mfd/mt6397/rtc.h> #include <linux/mod_devicetable.h> +static const struct mtk_rtc_data mt6358_rtc_data = { + .wrtgr = RTC_WRTGR_MT6358, +}; + +static const struct mtk_rtc_data mt6397_rtc_data = { + .wrtgr = RTC_WRTGR_MT6397, +}; + +static const struct of_device_id mt6397_rtc_of_match[] = { + { .compatible = "mediatek,mt6323-rtc", + .data = (void *)&mt6397_rtc_data, }, + { .compatible = "mediatek,mt6358-rtc", + .data = (void *)&mt6358_rtc_data, }, + { .compatible = "mediatek,mt6397-rtc", + .data = (void *)&mt6397_rtc_data, }, + {} +}; +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); + static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) { int ret; u32 data; - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1); + ret = regmap_write(rtc->regmap, rtc->addr_base + rtc->data->wrtgr, 1); if (ret < 0) return ret; @@ -269,6 +289,9 @@ static int mtk_rtc_probe(struct platform_device *pdev) res = platform_get_resource(pdev, IORESOURCE_MEM, 0); rtc->addr_base = res->start; + rtc->data = (struct mtk_rtc_data *) + of_device_get_match_data(&pdev->dev); + rtc->irq = platform_get_irq(pdev, 0); if (rtc->irq < 0) return rtc->irq; @@ -325,13 +348,6 @@ static int mt6397_rtc_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend, mt6397_rtc_resume); -static const struct of_device_id mt6397_rtc_of_match[] = { - { .compatible = "mediatek,mt6323-rtc", }, - { .compatible = "mediatek,mt6397-rtc", }, - { } -}; -MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match); - static struct platform_driver mtk_rtc_driver = { .driver = { .name = "mt6397-rtc", diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h index 7dfb63b..66534ed 100644 --- a/include/linux/mfd/mt6397/rtc.h +++ b/include/linux/mfd/mt6397/rtc.h @@ -18,7 +18,9 @@ #define RTC_BBPU_CBUSY BIT(6) #define RTC_BBPU_KEY (0x43 << 8) -#define RTC_WRTGR 0x003c +#define RTC_WRTGR_MT6358 0x3a +#define RTC_WRTGR_MT6397 0x3c +#define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397 #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; +}; + 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; }; #endif /* _LINUX_MFD_MT6397_RTC_H_ */