On 17/02/2017 at 06:48:36 -0800, Guenter Roeck wrote:
> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> > .config is used to cache a part of WDT_MR at probe time and is not used
> > afterwards. Also functions are used while they always return 0. Simplify
> > the flow by having everything in .probe().
> >
>
> Does that really improve anything ? It makes the code harder to read
> and, ultimately, the dropped value in sama5d4_wdt is added back in a
> later patch as 'mr'.
>
I don't find the current code to be particularly easy to read but maybe
it is a matter of taste.
I would still remove the test on pdev->dev.of_node because it will never
be false anyway.
> Guenter
>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - completely get rid of .config instead of caching it
> > - merge init functions in probe()
> >
> > drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
> > 1 file changed, 34 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index a49634cdc1cc..2c6f5a70ae67 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -28,7 +28,6 @@
> > struct sama5d4_wdt {
> > struct watchdog_device wdd;
> > void __iomem *reg_base;
> > - u32 config;
> > };
> >
> > static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> > @@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> > -{
> > - const char *tmp;
> > -
> > - wdt->config = AT91_WDT_WDDIS;
> > -
> > - if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> > - !strcmp(tmp, "software"))
> > - wdt->config |= AT91_WDT_WDFIEN;
> > - else
> > - wdt->config |= AT91_WDT_WDRSTEN;
> > -
> > - if (of_property_read_bool(np, "atmel,idle-halt"))
> > - wdt->config |= AT91_WDT_WDIDLEHLT;
> > -
> > - if (of_property_read_bool(np, "atmel,dbg-halt"))
> > - wdt->config |= AT91_WDT_WDDBGHLT;
> > -
> > - return 0;
> > -}
> > -
> > -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> > -{
> > - struct watchdog_device *wdd = &wdt->wdd;
> > - u32 value = WDT_SEC2TICKS(wdd->timeout);
> > - u32 reg;
> > -
> > - /*
> > - * Because the fields WDV and WDD must not be modified when the WDDIS
> > - * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> > - */
> > - reg = wdt_read(wdt, AT91_WDT_MR);
> > - reg &= ~AT91_WDT_WDDIS;
> > - wdt_write(wdt, AT91_WDT_MR, reg);
> > -
> > - reg = wdt->config;
> > - reg |= AT91_WDT_SET_WDD(value);
> > - reg |= AT91_WDT_SET_WDV(value);
> > -
> > - wdt_write(wdt, AT91_WDT_MR, reg);
> > -
> > - return 0;
> > -}
> > -
> > static int sama5d4_wdt_probe(struct platform_device *pdev)
> > {
> > struct watchdog_device *wdd;
> > struct sama5d4_wdt *wdt;
> > struct resource *res;
> > void __iomem *regs;
> > - u32 irq = 0;
> > + struct device_node *np = pdev->dev.of_node;
> > + const char *tmp;
> > + u32 mr, reg, irq = 0;
> > int ret;
> >
> > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > @@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> >
> > wdt->reg_base = regs;
> >
> > - if (pdev->dev.of_node) {
> > - irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > - if (!irq)
> > - dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> > + irq = irq_of_parse_and_map(np, 0);
> > + if (!irq)
> > + dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> >
> > - ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> > - if (ret)
> > - return ret;
> > - }
> >
> > - if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> > + mr = AT91_WDT_WDDIS;
> > +
> > + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> > + !strcmp(tmp, "software"))
> > + mr |= AT91_WDT_WDFIEN;
> > + else
> > + mr |= AT91_WDT_WDRSTEN;
> > +
> > + if (of_property_read_bool(np, "atmel,idle-halt"))
> > + mr |= AT91_WDT_WDIDLEHLT;
> > +
> > + if (of_property_read_bool(np, "atmel,dbg-halt"))
> > + mr |= AT91_WDT_WDDBGHLT;
> > +
> > + if ((mr & AT91_WDT_WDFIEN) && irq) {
> > ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
> > IRQF_SHARED | IRQF_IRQPOLL |
> > IRQF_NO_SUSPEND, pdev->name, pdev);
> > @@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > - ret = sama5d4_wdt_init(wdt);
> > - if (ret)
> > - return ret;
> > + /*
> > + * WDV and WDD must not be modified when the WDDIS bit is set, so clear
> > + * the WDDIS bit before writing the WDT_MR.
> > + */
> > + reg = wdt_read(wdt, AT91_WDT_MR);
> > + reg &= ~AT91_WDT_WDDIS;
> > + wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > + reg = mr;
> > + reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
> > + reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
> > +
> > + wdt_write(wdt, AT91_WDT_MR, reg);
> >
> > watchdog_set_nowayout(wdd, nowayout);
> >
> >
>
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
