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

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