Re: [PATCH 1/4] watchdog: qcom: Propagate errors from optional IRQ lookup

[Guenter Roeck]

On 8/9/26 03:01, Bui Duc Phuc wrote:
> Hi Guenter
>
> Thank you for your review .
>
> > >    irq = platform_get_irq_optional(pdev, 0);
> > > +if (irq < 0 && irq != -ENXIO)
> > > +        return irq;
> >
> > This is still wrong. If there is no pretimeout, it does not matter if there is an error.
>
> If checking data->pretimeout is required here, I'd propose one of the
> following approaches
> let me know which one you'd prefer:
>
> Option A (minimal diff, keep existing structure):
> ---------------
> irq = platform_get_irq_optional(pdev, 0);
> if (data->pretimeout && irq > 0) {
>          ret = devm_request_irq(dev, irq, qcom_wdt_isr, 0,
>                                                 "wdt_bark", &wdt->wdd);
>          if (ret)
>                  return ret;
>
>          wdt->wdd.info = &qcom_wdt_pt_info;
>          wdt->wdd.pretimeout = 1;
> } else {
>          if (data->pretimeout && irq < 0 && irq != -ENXIO)
>                  return irq;
>
>          wdt->wdd.info = &qcom_wdt_info;
> }
> ------------------
>
> Option B (check moved out, before the if/else):
>
> ------------------
> irq = platform_get_irq_optional(pdev, 0);
> if (data->pretimeout && irq < 0 && irq != -ENXIO)
>          return irq;
>
> if (data->pretimeout && irq > 0) {
>          ret = devm_request_irq(dev, irq, qcom_wdt_isr, 0,
>                                                 "wdt_bark", &wdt->wdd);
>          if (ret)
>                  return ret;
>
>          wdt->wdd.info = &qcom_wdt_pt_info;
>          wdt->wdd.pretimeout = 1;
> } else {
>          wdt->wdd.info = &qcom_wdt_info;
> }
> ----------------------
>
> Option C (default-then-override, only look up the IRQ when pretimeout
> is supported):
>
> ----------------------
> wdt->wdd.info = &qcom_wdt_info;
>
> if (data->pretimeout) {
>          irq = platform_get_irq_optional(pdev, 0);
>          if(irq < 0 && irq != -ENXIO)
>                  return irq;
>
>          if (irq > 0){
>                  ret = devm_request_irq(dev, irq, qcom_wdt_isr, 0,
>                                                         "wdt_bark", &wdt->wdd);
>                  if(ret)
>                          return ret;
>
>                  wdt->wdd.info= &qcom_wdt_pt_info;
>                  wdt->wdd.pretimeout = 1;
>          }
> }
> -----------------------
>
> Let me know which one you think fits best, or if you'd prefer something else.

I wpuld probably implement something like

	if (data->pretimeout) {
		irq = platform_get_irq_optional(pdev, 0);
		if (irq < 0 && irq != -ENXIO)
			return irq;
	        ret = devm_request_irq(dev, irq, qcom_wdt_isr, 0,
				       "wdt_bark", &wdt->wdd);
	        if (ret)
        	        return ret;

	        wdt->wdd.info = &qcom_wdt_pt_info;
        	wdt->wdd.pretimeout = 1;
	} else {
        	wdt->wdd.info = &qcom_wdt_info;
	}

(which I think would be a combination of B and C) but ultimately it is
POV and doesn't really matter.

Thanks,
Guenter