lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Feb 2017 06:48:36 -0800 From: Guenter Roeck <linux@...ck-us.net> To: Alexandre Belloni <alexandre.belloni@...e-electrons.com> Cc: Wim Van Sebroeck <wim@...ana.be>, Nicolas Ferre <nicolas.ferre@...el.com>, linux-watchdog@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init 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@...e-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); > >
Powered by blists - more mailing lists