| Message ID | 20231201110840.37408-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add DA9062 PMIC and built-in RTC support for RZ/G2UL SMARC EVK | expand |
Hi Biju, On Fri, Dec 1, 2023 at 12:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/rtc/rtc-da9063.c > +++ b/drivers/rtc/rtc-da9063.c > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct platform_device *pdev) > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "ALARM", rtc); > if (ret) > - dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n", > - irq_alarm, ret); > + return dev_err_probe(&pdev->dev, ret, > + "Failed to request ALARM IRQ %d\n", > + irq_alarm); This changes behavior: before, this was not considered fatal. > > ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); > if (ret) The rest LGTM, so with the above fixed/clarified: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > Hi Biju, > > On Fri, Dec 1, 2023 at 12:08 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/rtc/rtc-da9063.c > > +++ b/drivers/rtc/rtc-da9063.c > > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct platform_device > *pdev) > > IRQF_TRIGGER_LOW | > IRQF_ONESHOT, > > "ALARM", rtc); > > if (ret) > > - dev_err(&pdev->dev, "Failed to request ALARM > IRQ %d: %d\n", > > - irq_alarm, ret); > > + return dev_err_probe(&pdev->dev, ret, > > + "Failed to request ALARM > IRQ %d\n", > > + irq_alarm); > > This changes behavior: before, this was not considered fatal. Agreed. Maybe a separate patch? if there is no irqhandler on platform with IRQ populated nothing will work, RTC won't work as "rtc_update_irq " updated in irq handler. I think it is a fatal condition. Cheers, Biju > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); > > if (ret) > > The rest LGTM, so with the above fixed/clarified: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
On 01/12/2023 13:30:05+0000, Biju Das wrote: > Hi Geert, > > Thanks for the feedback. > > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > Hi Biju, > > > > On Fri, Dec 1, 2023 at 12:08 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/rtc/rtc-da9063.c > > > +++ b/drivers/rtc/rtc-da9063.c > > > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct platform_device > > *pdev) > > > IRQF_TRIGGER_LOW | > > IRQF_ONESHOT, > > > "ALARM", rtc); > > > if (ret) > > > - dev_err(&pdev->dev, "Failed to request ALARM > > IRQ %d: %d\n", > > > - irq_alarm, ret); > > > + return dev_err_probe(&pdev->dev, ret, > > > + "Failed to request ALARM > > IRQ %d\n", > > > + irq_alarm); > > > > This changes behavior: before, this was not considered fatal. > > Agreed. Maybe a separate patch? > > if there is no irqhandler on platform with IRQ populated nothing will work, > RTC won't work as "rtc_update_irq " updated in irq handler. > I think it is a fatal condition. This is not true, an RTC can wake up a system without an IRQ. > > Cheers, > Biju > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); > > > if (ret) > > > > The rest LGTM, so with the above fixed/clarified: > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Gr{oetje,eeting}s, > > > > Geert > > > > -- > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > > m68k.org > > > > In personal conversations with technical people, I call myself a hacker. > > But when I'm talking to journalists I just say "programmer" or something > > like that. > > -- Linus Torvalds
Hi Alexandre Belloni, Thanks for the feedback. > -----Original Message----- > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > On 01/12/2023 13:30:05+0000, Biju Das wrote: > > Hi Geert, > > > > Thanks for the feedback. > > > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > > > Hi Biju, > > > > > > On Fri, Dec 1, 2023 at 12:08 PM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > > > --- a/drivers/rtc/rtc-da9063.c > > > > +++ b/drivers/rtc/rtc-da9063.c > > > > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct > > > > platform_device > > > *pdev) > > > > IRQF_TRIGGER_LOW | > > > IRQF_ONESHOT, > > > > "ALARM", rtc); > > > > if (ret) > > > > - dev_err(&pdev->dev, "Failed to request ALARM > > > IRQ %d: %d\n", > > > > - irq_alarm, ret); > > > > + return dev_err_probe(&pdev->dev, ret, > > > > + "Failed to request > > > > + ALARM > > > IRQ %d\n", > > > > + irq_alarm); > > > > > > This changes behavior: before, this was not considered fatal. > > > > Agreed. Maybe a separate patch? > > > > if there is no irqhandler on platform with IRQ populated nothing will > > work, RTC won't work as "rtc_update_irq " updated in irq handler. > > I think it is a fatal condition. > > This is not true, an RTC can wake up a system without an IRQ. Agreed, I will restore the behaviour for this case. It may not be fatal. But basic "hwclock -r" from userspace won't work as " RTC_FEATURE_UPDATE_INTERRUPT" set and there is no irqhandler. Cheers, Biju Cheers, Biju > > > > > Cheers, > > Biju > > > > > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); > > > > if (ret) > > > > > > The rest LGTM, so with the above fixed/clarified: > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > Gr{oetje,eeting}s, > > > > > > Geert > > > > > > -- > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > > > geert@linux- m68k.org > > > > > > In personal conversations with technical people, I call myself a > hacker. > > > But when I'm talking to journalists I just say "programmer" or > > > something like that. > > > -- Linus Torvalds > > -- > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > engineering > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb699f48656d34a923a640 > 8dbf28104af%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837040817604431 > 5%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=E9tDi08sBRoh4tBccQB%2B8az%2BqQ4%2 > FtQOpFjdPgU8zQXc%3D&reserved=0
On 01/12/2023 15:43:27+0000, Biju Das wrote: > Hi Alexandre Belloni, > > Thanks for the feedback. > > > -----Original Message----- > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > On 01/12/2023 13:30:05+0000, Biju Das wrote: > > > Hi Geert, > > > > > > Thanks for the feedback. > > > > > > > -----Original Message----- > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > > > > > Hi Biju, > > > > > > > > On Fri, Dec 1, 2023 at 12:08 PM Biju Das > > > > <biju.das.jz@bp.renesas.com> > > > > wrote: > > > > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > > > Thanks for your patch! > > > > > > > > > --- a/drivers/rtc/rtc-da9063.c > > > > > +++ b/drivers/rtc/rtc-da9063.c > > > > > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct > > > > > platform_device > > > > *pdev) > > > > > IRQF_TRIGGER_LOW | > > > > IRQF_ONESHOT, > > > > > "ALARM", rtc); > > > > > if (ret) > > > > > - dev_err(&pdev->dev, "Failed to request ALARM > > > > IRQ %d: %d\n", > > > > > - irq_alarm, ret); > > > > > + return dev_err_probe(&pdev->dev, ret, > > > > > + "Failed to request > > > > > + ALARM > > > > IRQ %d\n", > > > > > + irq_alarm); > > > > > > > > This changes behavior: before, this was not considered fatal. > > > > > > Agreed. Maybe a separate patch? > > > > > > if there is no irqhandler on platform with IRQ populated nothing will > > > work, RTC won't work as "rtc_update_irq " updated in irq handler. > > > I think it is a fatal condition. > > > > This is not true, an RTC can wake up a system without an IRQ. > > Agreed, I will restore the behaviour for this case. > It may not be fatal. But basic "hwclock -r" from userspace won't > work as " RTC_FEATURE_UPDATE_INTERRUPT" set and there is no irqhandler. > RTC_FEATURE_ALARM is what you should clear and you have to fallback to the irq is not present case. > Cheers, > Biju > > > > Cheers, > Biju > > > > > > > > > Cheers, > > > Biju > > > > > > > > > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); > > > > > if (ret) > > > > > > > > The rest LGTM, so with the above fixed/clarified: > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > Gr{oetje,eeting}s, > > > > > > > > Geert > > > > > > > > -- > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > > > > geert@linux- m68k.org > > > > > > > > In personal conversations with technical people, I call myself a > > hacker. > > > > But when I'm talking to journalists I just say "programmer" or > > > > something like that. > > > > -- Linus Torvalds > > > > -- > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > > engineering > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb699f48656d34a923a640 > > 8dbf28104af%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837040817604431 > > 5%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=E9tDi08sBRoh4tBccQB%2B8az%2BqQ4%2 > > FtQOpFjdPgU8zQXc%3D&reserved=0
Hi Alexandre Belloni, > -----Original Message----- > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > On 01/12/2023 15:43:27+0000, Biju Das wrote: > > Hi Alexandre Belloni, > > > > Thanks for the feedback. > > > > > -----Original Message----- > > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > > > On 01/12/2023 13:30:05+0000, Biju Das wrote: > > > > Hi Geert, > > > > > > > > Thanks for the feedback. > > > > > > > > > -----Original Message----- > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > > > > > > > > > Hi Biju, > > > > > > > > > > On Fri, Dec 1, 2023 at 12:08 PM Biju Das > > > > > <biju.das.jz@bp.renesas.com> > > > > > wrote: > > > > > > Replace dev_err()->dev_err_probe() to simpilfy probe(). > > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > --- a/drivers/rtc/rtc-da9063.c > > > > > > +++ b/drivers/rtc/rtc-da9063.c > > > > > > @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct > > > > > > platform_device > > > > > *pdev) > > > > > > > > > > > > IRQF_TRIGGER_LOW | > > > > > IRQF_ONESHOT, > > > > > > "ALARM", rtc); > > > > > > if (ret) > > > > > > - dev_err(&pdev->dev, "Failed to request > ALARM > > > > > IRQ %d: %d\n", > > > > > > - irq_alarm, ret); > > > > > > + return dev_err_probe(&pdev->dev, ret, > > > > > > + "Failed to > > > > > > + request ALARM > > > > > IRQ %d\n", > > > > > > + irq_alarm); > > > > > > > > > > This changes behavior: before, this was not considered fatal. > > > > > > > > Agreed. Maybe a separate patch? > > > > > > > > if there is no irqhandler on platform with IRQ populated nothing > > > > will work, RTC won't work as "rtc_update_irq " updated in irq > handler. > > > > I think it is a fatal condition. > > > > > > This is not true, an RTC can wake up a system without an IRQ. > > > > Agreed, I will restore the behaviour for this case. > > It may not be fatal. But basic "hwclock -r" from userspace won't work > > as " RTC_FEATURE_UPDATE_INTERRUPT" set and there is no irqhandler. > > > > RTC_FEATURE_ALARM is what you should clear and you have to fallback to the > irq is not present case. Ok,Will update Patch#3 with clearing "RTC_FEATURE_ALARM" as the fallback for the irqhandler error case On patch#1, I will clear both RTC_FEATURE_ALARM" and "RTC_FEATURE_UPDATE_INTERRUPT", as with just clearing "RTC_FEATURE_ALARM", I get below error. root@smarc-rzg2ul:~# date -s "2023-08-06 19:30:00" Sun Aug 6 19:30:00 UTC 2023 root@smarc-rzg2ul:~# hwclock -w root@smarc-rzg2ul:~# hwclock -r hwclock: select() to /dev/rtc0 to wait for clock tick timed out root@smarc-rzg2ul:~# Cheers, Biju > > > > > > > > > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, > irq_alarm); > > > > > > if (ret) > > > > > > > > > > The rest LGTM, so with the above fixed/clarified: > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > Gr{oetje,eeting}s, > > > > > > > > > > Geert > > > > > > > > > > -- > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > > > > > geert@linux- m68k.org > > > > > > > > > > In personal conversations with technical people, I call myself a > > > hacker. > > > > > But when I'm talking to journalists I just say "programmer" or > > > > > something like that. > > > > > -- Linus Torvalds > > > > > > -- > > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and > > > Kernel engineering > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin% > 2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c608dbf > 285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638370429169364269%7C > Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zGt9Zsk6AYZ3zwOTU6l0zmN3KF1rGqOTAe3XR > hxPWaA%3D&reserved=0. > > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb699f48656d34a9 > > > 23a640 > > > 8dbf28104af%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837040817 > > > 604431 > > > 5%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi > > > I6Ik1h > > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=E9tDi08sBRoh4tBccQB%2B8az%2 > > > BqQ4%2 > > > FtQOpFjdPgU8zQXc%3D&reserved=0 > > -- > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > engineering > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c60 > 8dbf285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837042916952053 > 1%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WULSktePlojGqVPbQJ%2BDelJnQEOUIh% > 2BaSJm2Ra4OsRI%3D&reserved=0
On 01/12/2023 16:40:25+0000, Biju Das wrote: > > RTC_FEATURE_ALARM is what you should clear and you have to fallback to the > > irq is not present case. > > > Ok,Will update Patch#3 with clearing "RTC_FEATURE_ALARM" as the fallback for the irqhandler error case > > On patch#1, I will clear both RTC_FEATURE_ALARM" and "RTC_FEATURE_UPDATE_INTERRUPT", > > as with just clearing "RTC_FEATURE_ALARM", I get below error. > > root@smarc-rzg2ul:~# date -s "2023-08-06 19:30:00" > Sun Aug 6 19:30:00 UTC 2023 > root@smarc-rzg2ul:~# hwclock -w > root@smarc-rzg2ul:~# hwclock -r > hwclock: select() to /dev/rtc0 to wait for clock tick timed out > root@smarc-rzg2ul:~# > > I can't believe this is true because the rtc core code will take the same path when RTC_FEATURE_ALARM or RTC_FEATURE_UPDATE_INTERRUPT is cleared: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/interface.c#L574 RTC_FEATURE_UPDATE_INTERRUPT must be cleared only when RTC_FEATURE_ALARM is set. > Cheers, > Biju > > > > > > > > > > > > > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, > > irq_alarm); > > > > > > > if (ret) > > > > > > > > > > > > The rest LGTM, so with the above fixed/clarified: > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > > > Gr{oetje,eeting}s, > > > > > > > > > > > > Geert > > > > > > > > > > > > -- > > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > > > > > > geert@linux- m68k.org > > > > > > > > > > > > In personal conversations with technical people, I call myself a > > > > hacker. > > > > > > But when I'm talking to journalists I just say "programmer" or > > > > > > something like that. > > > > > > -- Linus Torvalds > > > > > > > > -- > > > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and > > > > Kernel engineering > > > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin% > > 2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c608dbf > > 285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638370429169364269%7C > > Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi > > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zGt9Zsk6AYZ3zwOTU6l0zmN3KF1rGqOTAe3XR > > hxPWaA%3D&reserved=0. > > > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb699f48656d34a9 > > > > 23a640 > > > > 8dbf28104af%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837040817 > > > > 604431 > > > > 5%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi > > > > I6Ik1h > > > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=E9tDi08sBRoh4tBccQB%2B8az%2 > > > > BqQ4%2 > > > > FtQOpFjdPgU8zQXc%3D&reserved=0 > > > > -- > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > > engineering > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c60 > > 8dbf285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837042916952053 > > 1%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WULSktePlojGqVPbQJ%2BDelJnQEOUIh% > > 2BaSJm2Ra4OsRI%3D&reserved=0
On 20/12/2023 00:49:57+0100, Alexandre Belloni wrote: > On 01/12/2023 16:40:25+0000, Biju Das wrote: > > > RTC_FEATURE_ALARM is what you should clear and you have to fallback to the > > > irq is not present case. > > > > > > Ok,Will update Patch#3 with clearing "RTC_FEATURE_ALARM" as the fallback for the irqhandler error case > > > > On patch#1, I will clear both RTC_FEATURE_ALARM" and "RTC_FEATURE_UPDATE_INTERRUPT", > > > > as with just clearing "RTC_FEATURE_ALARM", I get below error. > > > > root@smarc-rzg2ul:~# date -s "2023-08-06 19:30:00" > > Sun Aug 6 19:30:00 UTC 2023 > > root@smarc-rzg2ul:~# hwclock -w > > root@smarc-rzg2ul:~# hwclock -r > > hwclock: select() to /dev/rtc0 to wait for clock tick timed out > > root@smarc-rzg2ul:~# > > > > > > I can't believe this is true because the rtc core code will take the > same path when RTC_FEATURE_ALARM or RTC_FEATURE_UPDATE_INTERRUPT is > cleared: > > https://elixir.bootlin.com/linux/latest/source/drivers/rtc/interface.c#L574 > > RTC_FEATURE_UPDATE_INTERRUPT must be cleared only when RTC_FEATURE_ALARM > is set. Are you sure you didn't break this test: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-da9063.c#L479 > > > > Cheers, > > Biju > > > > > > > > > > > > > > > > > > > > > > > > > ret = dev_pm_set_wake_irq(&pdev->dev, > > > irq_alarm); > > > > > > > > if (ret) > > > > > > > > > > > > > > The rest LGTM, so with the above fixed/clarified: > > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > > > > > > > Gr{oetje,eeting}s, > > > > > > > > > > > > > > Geert > > > > > > > > > > > > > > -- > > > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > > > > > > > geert@linux- m68k.org > > > > > > > > > > > > > > In personal conversations with technical people, I call myself a > > > > > hacker. > > > > > > > But when I'm talking to journalists I just say "programmer" or > > > > > > > something like that. > > > > > > > -- Linus Torvalds > > > > > > > > > > -- > > > > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and > > > > > Kernel engineering > > > > > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin% > > > 2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c608dbf > > > 285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638370429169364269%7C > > > Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi > > > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zGt9Zsk6AYZ3zwOTU6l0zmN3KF1rGqOTAe3XR > > > hxPWaA%3D&reserved=0. > > > > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7Cb699f48656d34a9 > > > > > 23a640 > > > > > 8dbf28104af%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837040817 > > > > > 604431 > > > > > 5%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi > > > > > I6Ik1h > > > > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=E9tDi08sBRoh4tBccQB%2B8az%2 > > > > > BqQ4%2 > > > > > FtQOpFjdPgU8zQXc%3D&reserved=0 > > > > > > -- > > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > > > engineering > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > > > com%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C72e93f2d3b25447789c60 > > > 8dbf285e823%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63837042916952053 > > > 1%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > > > aWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WULSktePlojGqVPbQJ%2BDelJnQEOUIh% > > > 2BaSJm2Ra4OsRI%3D&reserved=0 > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Alexandre Belloni, > -----Original Message----- > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > Sent: Tuesday, December 19, 2023 11:50 PM > Subject: Re: [PATCH 3/6] rtc: da9063: Use dev_err_probe() > > On 01/12/2023 16:40:25+0000, Biju Das wrote: > > > RTC_FEATURE_ALARM is what you should clear and you have to fallback > > > to the irq is not present case. > > > > > > Ok,Will update Patch#3 with clearing "RTC_FEATURE_ALARM" as the > > fallback for the irqhandler error case > > > > On patch#1, I will clear both RTC_FEATURE_ALARM" and > > "RTC_FEATURE_UPDATE_INTERRUPT", > > > > as with just clearing "RTC_FEATURE_ALARM", I get below error. > > > > root@smarc-rzg2ul:~# date -s "2023-08-06 19:30:00" > > Sun Aug 6 19:30:00 UTC 2023 > > root@smarc-rzg2ul:~# hwclock -w > > root@smarc-rzg2ul:~# hwclock -r > > hwclock: select() to /dev/rtc0 to wait for clock tick timed out > > root@smarc-rzg2ul:~# > > > > > > I can't believe this is true because the rtc core code will take the same > path when RTC_FEATURE_ALARM or RTC_FEATURE_UPDATE_INTERRUPT is > cleared: > > > RTC_FEATURE_UPDATE_INTERRUPT must be cleared only when RTC_FEATURE_ALARM > is set. I rechecked this with latest next and it is working with clearing RTC_FEATURE_ALARM. I will send v3 with this change. root@smarc-rzg2ul:~# date -s "2023-08-06 19:30:00" Sun Aug 6 19:30:00 UTC 2023 root@smarc-rzg2ul:~# hwclock -w root@smarc-rzg2ul:~# hwclock -r 2023-08-06 19:30:11.664350+00:00 root@smarc-rzg2ul:~# Cheers, Biju
diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c index a1ee0705ca88..4d7664340091 100644 --- a/drivers/rtc/rtc-da9063.c +++ b/drivers/rtc/rtc-da9063.c @@ -407,57 +407,49 @@ static int da9063_rtc_probe(struct platform_device *pdev) config->rtc_enable_reg, config->rtc_enable_mask, config->rtc_enable_mask); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to enable RTC\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, "Failed to enable RTC\n"); ret = regmap_update_bits(rtc->regmap, config->rtc_enable_32k_crystal_reg, config->rtc_crystal_mask, config->rtc_crystal_mask); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to run 32kHz oscillator\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to run 32kHz oscillator\n"); ret = regmap_update_bits(rtc->regmap, config->rtc_alarm_secs_reg, config->rtc_alarm_status_mask, 0); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to access RTC alarm register\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to access RTC alarm register\n"); ret = regmap_update_bits(rtc->regmap, config->rtc_alarm_secs_reg, DA9063_ALARM_STATUS_ALARM, DA9063_ALARM_STATUS_ALARM); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to access RTC alarm register\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to access RTC alarm register\n"); ret = regmap_update_bits(rtc->regmap, config->rtc_alarm_year_reg, config->rtc_tick_on_mask, 0); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to disable TICKs\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to disable TICKs\n"); data[RTC_SEC] = 0; ret = regmap_bulk_read(rtc->regmap, config->rtc_alarm_secs_reg, &data[config->rtc_data_start], config->rtc_alarm_len); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to read initial alarm data: %d\n", - ret); - return ret; - } + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to read initial alarm data\n"); platform_set_drvdata(pdev, rtc); @@ -488,8 +480,9 @@ static int da9063_rtc_probe(struct platform_device *pdev) IRQF_TRIGGER_LOW | IRQF_ONESHOT, "ALARM", rtc); if (ret) - dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n", - irq_alarm, ret); + return dev_err_probe(&pdev->dev, ret, + "Failed to request ALARM IRQ %d\n", + irq_alarm); ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm); if (ret)
Replace dev_err()->dev_err_probe() to simpilfy probe(). Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/rtc/rtc-da9063.c | 47 +++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 27 deletions(-)