Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[Alexandre Belloni]

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