Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582894 > unrolled thread

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

Started byAlexandre Belloni <alexandre.belloni@free-electrons.com>
First post2017-02-16 20:40 +0100
Last post2017-02-19 18:00 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/3] watchdog: sama5d4: Cleanup init Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-16 20:40 +0100
    Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init Guenter Roeck <linux@roeck-us.net> - 2017-02-17 15:50 +0100
      Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-17 16:40 +0100
        Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init Guenter Roeck <linux@roeck-us.net> - 2017-02-19 18:00 +0100

#1582894 — [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-02-16 20:40 +0100
Subject[PATCH v2 1/3] watchdog: sama5d4: Cleanup init
Message-ID<tbA4i-2sg-9@gated-at.bofh.it>
.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().

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
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);
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1583488

FromGuenter Roeck <linux@roeck-us.net>
Date2017-02-17 15:50 +0100
Message-ID<tbS1b-5xl-9@gated-at.bofh.it>
In reply to#1582894
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'.

Guenter

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> 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);
>
>

[toc] | [prev] | [next] | [standalone]

#1583534

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-02-17 16:40 +0100
Message-ID<tbSNz-63T-19@gated-at.bofh.it>
In reply to#1583488
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@free-electrons.com>
> > ---
> > 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

[toc] | [prev] | [next] | [standalone]

#1584177

FromGuenter Roeck <linux@roeck-us.net>
Date2017-02-19 18:00 +0100
Message-ID<tcD05-1kg-7@gated-at.bofh.it>
In reply to#1583534
On 02/17/2017 07:35 AM, Alexandre Belloni wrote:
> 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 always thought it is commonly accepted that splitting code into smaller
functions tends to improve readability. I for my part still think it does,
and splitting out reading configuration data and initializing the chip
are good candidates.

[ Not that the functions are necessarily named well here, but that is
a different issue ]

> I would still remove the test on pdev->dev.of_node because it will never
> be false anyway.
>
Agreed.

Guenter

>
>> Guenter
>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>> 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);
>>>
>>>
>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web