diff mbox series

[v2,3/7] rtc: mt6397: improvements of rtc driver

Message ID 20190703164822.17924-4-frank-w@public-files.de
State Changes Requested
Headers show
Series implement poweroff for mt6323/6397 | expand

Commit Message

Frank Wunderlich July 3, 2019, 4:48 p.m. UTC 
From: Josef Friedl <josef.friedl@speed.at>

- use regmap_read_poll_timeout to drop while-loop
- use devm-api to drop remove-callback
- add new compatible for mt6323

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/rtc/rtc-mt6397.c | 55 ++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)

--
2.17.1

Comments

Matthias Brugger July 4, 2019, 9:13 a.m. UTC | #1 
On 03/07/2019 18:48, Frank Wunderlich wrote:
> From: Josef Friedl <josef.friedl@speed.at>
> 
> - use regmap_read_poll_timeout to drop while-loop
> - use devm-api to drop remove-callback
> - add new compatible for mt6323
>

It's up to the maintainer but I don't like patches doing clean-ups together with
adding support for new HW, although it's a trivial one here.


> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> =2D--
>  drivers/rtc/rtc-mt6397.c | 55 ++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index c08ee5edf865..e5ddf0d0b6f1 100644
> =2D-- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -4,16 +4,19 @@
>  * Author: Tianping.Fang <tianping.fang@mediatek.com>

Missing in the CC list.
Frank Wunderlich July 4, 2019, 11:08 a.m. UTC | #2 
> Gesendet: Donnerstag, 04. Juli 2019 um 11:13 Uhr
> Von: "Matthias Brugger" <matthias.bgg@gmail.com>

> It's up to the maintainer but I don't like patches doing clean-ups together with
> adding support for new HW, although it's a trivial one here.

i can split again to have clean-up and new functions separated
Alexandre Belloni July 4, 2019, 8:43 p.m. UTC | #3 
On 03/07/2019 18:48:18+0200, Frank Wunderlich wrote:
> @@ -271,14 +268,11 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> 
>  	platform_set_drvdata(pdev, rtc);
> 
> -	rtc->rtc_dev = devm_rtc_allocate_device(rtc->dev);
> -	if (IS_ERR(rtc->rtc_dev))
> -		return PTR_ERR(rtc->rtc_dev);
> +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +					mtk_rtc_irq_handler_thread,
> +					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +					"mt6397-rtc", rtc);
> 

This change may lead to a crash and the allocation was intentionally
placed before the irq request.

> -	ret = request_threaded_irq(rtc->irq, NULL,
> -				   mtk_rtc_irq_handler_thread,
> -				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -				   "mt6397-rtc", rtc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>  			rtc->irq, ret);
> @@ -287,6 +281,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> 
>  	device_init_wakeup(&pdev->dev, 1);
> 
> +	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(rtc->rtc_dev))
> +		return PTR_ERR(rtc->rtc_dev);
> +
>  	rtc->rtc_dev->ops = &mtk_rtc_ops;
> 
>  	ret = rtc_register_device(rtc->rtc_dev);
> @@ -302,15 +300,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  	return ret;
>  }
> 
> -static int mtk_rtc_remove(struct platform_device *pdev)
> -{
> -	struct mt6397_rtc *rtc = platform_get_drvdata(pdev);
> -
> -	free_irq(rtc->irq, rtc);
> -
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int mt6397_rtc_suspend(struct device *dev)
>  {
> @@ -337,6 +326,7 @@ 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", },

Unrelated change, this is not an improvement and must be accompanied by
a documentation change.

>  	{ .compatible = "mediatek,mt6397-rtc", },
>  	{ }
>  };
> @@ -349,7 +339,6 @@ static struct platform_driver mtk_rtc_driver = {
>  		.pm = &mt6397_pm_ops,
>  	},
>  	.probe	= mtk_rtc_probe,
> -	.remove = mtk_rtc_remove,
>  };
> 
>  module_platform_driver(mtk_rtc_driver);
> --
> 2.17.1
>
Frank Wunderlich July 5, 2019, 3:35 p.m. UTC | #4 
Hi Alexander,

thank you for the Review

> Gesendet: Donnerstag, 04. Juli 2019 um 22:43 Uhr
> Von: "Alexandre Belloni" <alexandre.belloni@bootlin.com>
> > -	rtc->rtc_dev = devm_rtc_allocate_device(rtc->dev);
> > -	if (IS_ERR(rtc->rtc_dev))
> > -		return PTR_ERR(rtc->rtc_dev);
> > +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > +					mtk_rtc_irq_handler_thread,
> > +					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > +					"mt6397-rtc", rtc);
> >
>
> This change may lead to a crash and the allocation was intentionally
> placed before the irq request.

i got no crash till now, but i will try to move the allocation before irq-request

> > -	ret = request_threaded_irq(rtc->irq, NULL,
> > -				   mtk_rtc_irq_handler_thread,
> > -				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > -				   "mt6397-rtc", rtc);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> >  			rtc->irq, ret);
> > @@ -287,6 +281,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >
> >  	device_init_wakeup(&pdev->dev, 1);
> >
> > +	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> > +	if (IS_ERR(rtc->rtc_dev))
> > +		return PTR_ERR(rtc->rtc_dev);
> > +
> >  	rtc->rtc_dev->ops = &mtk_rtc_ops;


> >  static const struct of_device_id mt6397_rtc_of_match[] = {
> > +	{ .compatible = "mediatek,mt6323-rtc", },
>
> Unrelated change, this is not an improvement and must be accompanied by
> a documentation change.

documentation is changed in 1/7 defining this compatible. i called it improvement because existing driver now supports another chip

regards Frank
Alexandre Belloni July 5, 2019, 9:24 p.m. UTC | #5 
On 05/07/2019 17:35:46+0200, Frank Wunderlich wrote:
> Hi Alexander,
> 
> thank you for the Review
> 
> > Gesendet: Donnerstag, 04. Juli 2019 um 22:43 Uhr
> > Von: "Alexandre Belloni" <alexandre.belloni@bootlin.com>
> > > -	rtc->rtc_dev = devm_rtc_allocate_device(rtc->dev);
> > > -	if (IS_ERR(rtc->rtc_dev))
> > > -		return PTR_ERR(rtc->rtc_dev);
> > > +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > > +					mtk_rtc_irq_handler_thread,
> > > +					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > > +					"mt6397-rtc", rtc);
> > >
> >
> > This change may lead to a crash and the allocation was intentionally
> > placed before the irq request.
> 
> i got no crash till now, but i will try to move the allocation before irq-request
> 

Let's say the RTC has been used to start your platform, then the irq
handler will be called as soon as the irq is requested, leading to a
null pointer dereference.

> > > -	ret = request_threaded_irq(rtc->irq, NULL,
> > > -				   mtk_rtc_irq_handler_thread,
> > > -				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > > -				   "mt6397-rtc", rtc);
> > >  	if (ret) {
> > >  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> > >  			rtc->irq, ret);
> > > @@ -287,6 +281,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> > >
> > >  	device_init_wakeup(&pdev->dev, 1);
> > >
> > > +	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> > > +	if (IS_ERR(rtc->rtc_dev))
> > > +		return PTR_ERR(rtc->rtc_dev);
> > > +
> > >  	rtc->rtc_dev->ops = &mtk_rtc_ops;
> 
> 
> > >  static const struct of_device_id mt6397_rtc_of_match[] = {
> > > +	{ .compatible = "mediatek,mt6323-rtc", },
> >
> > Unrelated change, this is not an improvement and must be accompanied by
> > a documentation change.
> 
> documentation is changed in 1/7 defining this compatible. i called it improvement because existing driver now supports another chip
> 

Yes and IIRC, I did comment that the rtc change also had to be separated
from 1/7.

Also, I really doubt this new compatible is necessary at all as you
could simply directly use mediatek,mt6397-rtc.
Frank Wunderlich July 6, 2019, 4:15 p.m. UTC | #6 
> Gesendet: Freitag, 05. Juli 2019 um 23:24 Uhr
> Von: "Alexandre Belloni" <alexandre.belloni@bootlin.com>

> Let's say the RTC has been used to start your platform, then the irq
> handler will be called as soon as the irq is requested, leading to a
> null pointer dereference.

i cannot test this with my platform, but i have changed it in my repo

https://github.com/frank-w/BPI-R2-4.14/commits/5.2-poweroff-mainline

> Yes and IIRC, I did comment that the rtc change also had to be separated
> from 1/7.

also this is put in separate commit, can you take a look before i post v3?

> Also, I really doubt this new compatible is necessary at all as you
> could simply directly use mediatek,mt6397-rtc.

imho this can confuse because the wrong chip-name is used in dts

regards Frank
Alexandre Belloni July 6, 2019, 8:04 p.m. UTC | #7 
On 06/07/2019 18:15:20+0200, Frank Wunderlich wrote:
> > Gesendet: Freitag, 05. Juli 2019 um 23:24 Uhr
> > Von: "Alexandre Belloni" <alexandre.belloni@bootlin.com>
> 
> > Let's say the RTC has been used to start your platform, then the irq
> > handler will be called as soon as the irq is requested, leading to a
> > null pointer dereference.
> 
> i cannot test this with my platform, but i have changed it in my repo
> 
> https://github.com/frank-w/BPI-R2-4.14/commits/5.2-poweroff-mainline
> 
> > Yes and IIRC, I did comment that the rtc change also had to be separated
> > from 1/7.
> 
> also this is put in separate commit, can you take a look before i post v3?
> 
> > Also, I really doubt this new compatible is necessary at all as you
> > could simply directly use mediatek,mt6397-rtc.
> 
> imho this can confuse because the wrong chip-name is used in dts
> 

This is not true, we do that all the time and the immediate benefit of
using the mt6397 compatible is that then there is no need to
synchronize between subsystems. If you want to be absolutely
conservative, you could use

compatible = "mediatek,mt6323-rtc", "mediatek,mt6397-rtc";

in your DT.
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index c08ee5edf865..e5ddf0d0b6f1 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -4,16 +4,19 @@ 
 * Author: Tianping.Fang <tianping.fang@mediatek.com>
 */

-#include <linux/delay.h>
-#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/mt6397/core.h>
 #include <linux/module.h>
+#include <linux/mutex.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 int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
 {
-	unsigned long timeout = jiffies + HZ;
 	int ret;
 	u32 data;

@@ -21,19 +24,13 @@  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
 	if (ret < 0)
 		return ret;

-	while (1) {
-		ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_BBPU,
-				  &data);
-		if (ret < 0)
-			break;
-		if (!(data & RTC_BBPU_CBUSY))
-			break;
-		if (time_after(jiffies, timeout)) {
-			ret = -ETIMEDOUT;
-			break;
-		}
-		cpu_relax();
-	}
+	ret = regmap_read_poll_timeout(rtc->regmap,
+					rtc->addr_base + RTC_BBPU, data,
+					!(data & RTC_BBPU_CBUSY),
+					MTK_RTC_POLL_DELAY_US,
+					MTK_RTC_POLL_TIMEOUT);
+	if (ret < 0)
+		dev_err(rtc->dev, "failed to write WRTGE: %d\n", ret);

 	return ret;
 }
@@ -271,14 +268,11 @@  static int mtk_rtc_probe(struct platform_device *pdev)

 	platform_set_drvdata(pdev, rtc);

-	rtc->rtc_dev = devm_rtc_allocate_device(rtc->dev);
-	if (IS_ERR(rtc->rtc_dev))
-		return PTR_ERR(rtc->rtc_dev);
+	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+					mtk_rtc_irq_handler_thread,
+					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+					"mt6397-rtc", rtc);

-	ret = request_threaded_irq(rtc->irq, NULL,
-				   mtk_rtc_irq_handler_thread,
-				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-				   "mt6397-rtc", rtc);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			rtc->irq, ret);
@@ -287,6 +281,10 @@  static int mtk_rtc_probe(struct platform_device *pdev)

 	device_init_wakeup(&pdev->dev, 1);

+	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(rtc->rtc_dev))
+		return PTR_ERR(rtc->rtc_dev);
+
 	rtc->rtc_dev->ops = &mtk_rtc_ops;

 	ret = rtc_register_device(rtc->rtc_dev);
@@ -302,15 +300,6 @@  static int mtk_rtc_probe(struct platform_device *pdev)
 	return ret;
 }

-static int mtk_rtc_remove(struct platform_device *pdev)
-{
-	struct mt6397_rtc *rtc = platform_get_drvdata(pdev);
-
-	free_irq(rtc->irq, rtc);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int mt6397_rtc_suspend(struct device *dev)
 {
@@ -337,6 +326,7 @@  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", },
 	{ }
 };
@@ -349,7 +339,6 @@  static struct platform_driver mtk_rtc_driver = {
 		.pm = &mt6397_pm_ops,
 	},
 	.probe	= mtk_rtc_probe,
-	.remove = mtk_rtc_remove,
 };

 module_platform_driver(mtk_rtc_driver);