lkml.org 
[lkml]   [2017]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init
From
Date
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);
>
>

\
 
 \ /
  Last update: 2017-02-17 15:50    [W:0.071 / U:0.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site