Re: Re: Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
On 06/07/2019 18:15:20+0200, Frank Wunderlich wrote: > > Gesendet: Freitag, 05. Juli 2019 um 23:24 Uhr > > Von: "Alexandre Belloni" > > > 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. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Aw: Re: Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
> Gesendet: Freitag, 05. Juli 2019 um 23:24 Uhr > Von: "Alexandre Belloni" > 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
Re: Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
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"
> > > - 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(>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(>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(>dev, 1);
> > >
> > > + rtc->rtc_dev = devm_rtc_allocate_device(>dev);
> > > + if (IS_ERR(rtc->rtc_dev))
> > > + return PTR_ERR(rtc->rtc_dev);
> > > +
> > > rtc->rtc_dev->ops = _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.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Aw: Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
Hi Alexander,
thank you for the Review
> Gesendet: Donnerstag, 04. Juli 2019 um 22:43 Uhr
> Von: "Alexandre Belloni"
> > - 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(>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(>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(>dev, 1);
> >
> > + rtc->rtc_dev = devm_rtc_allocate_device(>dev);
> > + if (IS_ERR(rtc->rtc_dev))
> > + return PTR_ERR(rtc->rtc_dev);
> > +
> > rtc->rtc_dev->ops = _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
Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
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(>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(>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(>dev, 1);
>
> + rtc->rtc_dev = devm_rtc_allocate_device(>dev);
> + if (IS_ERR(rtc->rtc_dev))
> + return PTR_ERR(rtc->rtc_dev);
> +
> rtc->rtc_dev->ops = _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 = _pm_ops,
> },
> .probe = mtk_rtc_probe,
> - .remove = mtk_rtc_remove,
> };
>
> module_platform_driver(mtk_rtc_driver);
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Aw: Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
> Gesendet: Donnerstag, 04. Juli 2019 um 11:13 Uhr > Von: "Matthias Brugger" > 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
Re: [PATCH v2 3/7] rtc: mt6397: improvements of rtc driver
On 03/07/2019 18:48, Frank Wunderlich wrote: > From: Josef Friedl > > - 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 > =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 Missing in the CC list.
