On 15 May 2015 21:20 Guenter Roeck, > > > > + > > > > +/* E_WDG_WARN interrupt handler */ > > > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void*data) > > > > +{ > > > > + struct da9062_watchdog *wdt = data; > > > > + > > > > + dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n"); > > > > + return IRQ_HANDLED; > > > > +} > > > > + > > > > On 15 May 2015 13:58 Guenter Roeck wrote: > > > > [...] > > > > > >>> + > > > >>> + irq = platform_get_irq_byname(pdev, "WDG_WARN"); > > > >>> + if (irq < 0) { > > > >>> + dev_err(wdt->hw->dev, "Failed to get IRQ.\n"); > > > >>> + ret = irq; > > > >>> + goto error; > > > > [...] > > > > > > > > > >> Also, is the interrupt mandatory ? All it does is to display a message. > > > >> Looks very optional to me. > > > > > > > > It is a place holder for something more application specific. > > > > I could remove it, but I figured it would just get re-added when somebody takes the > > > > driver and modifies it for their needs. > > > > > > > > If this is a problem however, it can go. > > > > Please advise .. > > > > > > > > > > Then this someone should add the code. For the time being, it just increases > > > kernel size and may cause the driver to fail for no good reason. Plus, given > > > the driver apparently works without interrupt, even then it should be > > > optional, and the driver does not have to fail loading if it is not supported on a > > > given platform. > > > > > > > Hi Guenter, > > > > I'm not sure if I got my previous point across there ... > > > > Leaving this in wouldn't really do any real harm I think. If this feature is not supported > > in somebody's platform then there wouldn't be a problem, the IRQ would fire (as a > > warning that the watchdog was about to time-out), the handler function would be > > executed, it would handle the IRQ -- and that would be it. Nothing would happen apart > > from a debug print. > > I didn't get my point across either. Problem is that your driver fails to load > if the interrupt is not there. With the interrupt really being optional, I don't > see the point in making it mandatory just to display a message if it fires. > Hi Guenter, Ok. I see now. It's not a mandatory interrupt and so it should not fail the whole driver upon an error from the devm_request_threaded_irq() request ... I will let it pass through if there is a problem and just display a debug message. Something like this: @@ -234,11 +234,9 @@ static int da9062_wdt_probe(struct platform_device *pdev) da9062_wdt_wdg_warn_irq_handler, IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, "WDG_WARN", wdt); - if (ret) { - dev_err(wdt->hw->dev, + if (ret) + dev_dbg(wdt->hw->dev, "Failed to request watchdog device IRQ.\n"); - return ret; - } ret = watchdog_register_device(&wdt->wdtdev); if (ret < 0) { I think I've understood now. I guess this is it.. I'll send a PATCH V3 with this change. Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html