Re: [PATCH 1/6] rtc: da9063: Make IRQ as optional

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	 Support Opensource <support.opensource@diasemi.com>,
	linux-rtc@vger.kernel.org,
	 Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Biju Das <biju.das.au@gmail.com>,
	 linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 1/6] rtc: da9063: Make IRQ as optional
Date: Fri, 1 Dec 2023 14:00:06 +0100	[thread overview]
Message-ID: <CAMuHMdUQaUaJdj-mNBZXNGmCAvgD9ne1ULbhzeoS+WVuvJOM0w@mail.gmail.com> (raw)
In-Reply-To: <20231201110840.37408-2-biju.das.jz@bp.renesas.com>

Hi Biju,

On Fri, Dec 1, 2023 at 12:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> On some platforms (eg: RZ/{G2UL,Five} SMARC EVK), there is no IRQ
> populated by default. Add irq optional support.
>
> 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
> @@ -485,25 +485,26 @@ static int da9063_rtc_probe(struct platform_device *pdev)
>                 clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtc_dev->features);
>         }
>
> -       irq_alarm = platform_get_irq_byname(pdev, "ALARM");
> -       if (irq_alarm < 0)
> -               return irq_alarm;
> -
> -       ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
> -                                       da9063_alarm_event,
> -                                       IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> -                                       "ALARM", rtc);
> -       if (ret)
> -               dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
> -                       irq_alarm, ret);
> -
> -       ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm);
> -       if (ret)
> -               dev_warn(&pdev->dev,
> -                        "Failed to set IRQ %d as a wake IRQ: %d\n",
> -                        irq_alarm, ret);
> -
> -       device_init_wakeup(&pdev->dev, true);
> +       irq_alarm = platform_get_irq_byname_optional(pdev, "ALARM");
> +       if (irq_alarm >= 0) {
> +               ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
> +                                               da9063_alarm_event,
> +                                               IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +                                               "ALARM", rtc);
> +               if (ret)
> +                       dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
> +                               irq_alarm, ret);
> +
> +               ret = dev_pm_set_wake_irq(&pdev->dev, irq_alarm);
> +               if (ret)
> +                       dev_warn(&pdev->dev,
> +                                "Failed to set IRQ %d as a wake IRQ: %d\n",
> +                                irq_alarm, ret);
> +
> +               device_init_wakeup(&pdev->dev, true);
> +       } else {
> +               clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtc_dev->features);

This does not handle and propagate real errors (e.g. -EPROBE_DEFER).

    } else if (irq_alarm != -ENXIO) {
            return irq_alarm;
    } else {
            ....
    }

(I think -ENXIO is the correct error to check for,
 platform_get_irq_byname_optional() really should start returning
 zero for not found)

> +       }
>
>         return devm_rtc_register_device(rtc->rtc_dev);

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