| Message ID | 1414248522-16055-3-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Ezequiel Garcia, It's really nitpicking, but I find the usage of empty newlines rather unfortunate in the below function. On Sat, 25 Oct 2014 11:48:40 -0300, Ezequiel Garcia wrote: > +static int armada375_wdt_clock_init(struct platform_device *pdev, > + struct orion_watchdog *dev) > +{ > + int ret; > + > + dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed"); > + if (!IS_ERR(dev->clk)) { > + This empty new line seems not needed. > + ret = clk_prepare_enable(dev->clk); > + if (ret) { > + clk_put(dev->clk); > + return ret; > + } > + > + atomic_io_modify(dev->reg + TIMER_CTRL, > + WDT_AXP_FIXED_ENABLE_BIT, > + WDT_AXP_FIXED_ENABLE_BIT); > + dev->clk_rate = clk_get_rate(dev->clk); > + return 0; But one empty new line before the return would be good. > + } > + > + /* Mandatory fallback for proper devicetree backward compatibility */ > + dev->clk = clk_get(&pdev->dev, NULL); > + if (IS_ERR(dev->clk)) > + return PTR_ERR(dev->clk); An empty newline would be good here as well. > + ret = clk_prepare_enable(dev->clk); > + if (ret) { > + clk_put(dev->clk); > + return ret; > + } > + > + atomic_io_modify(dev->reg + TIMER_CTRL, > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT), > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT)); > + dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO; And here. With this fixed: Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Wim, On Sat, Oct 25, 2014 at 11:48:40AM -0300, Ezequiel Garcia wrote: > The 25 MHz reference clock has better stability so its use is preferred over the > core clock. Change the Armada 375 clock initialization to use this reference > clock. To ensure the driver is compatible with an old devicetree, also provide > a fallback path which will silently return to the previous behavior. > > While here, add the clock specification to the binding documentation. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > .../devicetree/bindings/watchdog/marvel.txt | 13 +++++++ > drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++- > 2 files changed, 52 insertions(+), 1 deletion(-) Acked-by: Jason Cooper <jason@lakedaemon.net> thx, Jason.
Ezequiel, Wim, On Fri, Oct 31, 2014 at 01:50:02PM +0100, Thomas Petazzoni wrote: > Dear Ezequiel Garcia, > > It's really nitpicking, but I find the usage of empty newlines rather > unfortunate in the below function. oops, I thought this was against patch #3. Ezequiel, please feel free to add my Ack once these changes are made. thx, Jason. > On Sat, 25 Oct 2014 11:48:40 -0300, Ezequiel Garcia wrote: > > > +static int armada375_wdt_clock_init(struct platform_device *pdev, > > + struct orion_watchdog *dev) > > +{ > > + int ret; > > + > > + dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed"); > > + if (!IS_ERR(dev->clk)) { > > + > > This empty new line seems not needed. > > > + ret = clk_prepare_enable(dev->clk); > > + if (ret) { > > + clk_put(dev->clk); > > + return ret; > > + } > > + > > + atomic_io_modify(dev->reg + TIMER_CTRL, > > + WDT_AXP_FIXED_ENABLE_BIT, > > + WDT_AXP_FIXED_ENABLE_BIT); > > + dev->clk_rate = clk_get_rate(dev->clk); > > + return 0; > > But one empty new line before the return would be good. > > > + } > > + > > + /* Mandatory fallback for proper devicetree backward compatibility */ > > + dev->clk = clk_get(&pdev->dev, NULL); > > + if (IS_ERR(dev->clk)) > > + return PTR_ERR(dev->clk); > > An empty newline would be good here as well. > > > + ret = clk_prepare_enable(dev->clk); > > + if (ret) { > > + clk_put(dev->clk); > > + return ret; > > + } > > + > > + atomic_io_modify(dev->reg + TIMER_CTRL, > > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT), > > + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT)); > > + dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO; > > And here. > > With this fixed: > > Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt index 97223fd..858ed92 100644 --- a/Documentation/devicetree/bindings/watchdog/marvel.txt +++ b/Documentation/devicetree/bindings/watchdog/marvel.txt @@ -17,6 +17,18 @@ For "marvell,armada-375-wdt" and "marvell,armada-380-wdt": - reg : A third entry is mandatory and should contain the shared mask/unmask RSTOUT address. +Clocks required for compatibles = "marvell,orion-wdt", + "marvell,armada-370-wdt": +- clocks : Must contain a single entry describing the clock input + +Clocks required for compatibles = "marvell,armada-xp-wdt" + "marvell,armada-375-wdt" + "marvell,armada-380-wdt": +- clocks : Must contain an entry for each entry in clock-names. +- clock-names : Must include the following entries: + "nbclk" (L2/coherency fabric clock), + "fixed" (Reference 25 MHz fixed-clock). + Optional properties: - interrupts : Contains the IRQ for watchdog expiration @@ -30,4 +42,5 @@ Example: interrupts = <3>; timeout-sec = <10>; status = "okay"; + clocks = <&gate_clk 7>; }; diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c index 00d0741..6452fa2 100644 --- a/drivers/watchdog/orion_wdt.c +++ b/drivers/watchdog/orion_wdt.c @@ -114,6 +114,44 @@ static int armada370_wdt_clock_init(struct platform_device *pdev, return 0; } +static int armada375_wdt_clock_init(struct platform_device *pdev, + struct orion_watchdog *dev) +{ + int ret; + + dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed"); + if (!IS_ERR(dev->clk)) { + + ret = clk_prepare_enable(dev->clk); + if (ret) { + clk_put(dev->clk); + return ret; + } + + atomic_io_modify(dev->reg + TIMER_CTRL, + WDT_AXP_FIXED_ENABLE_BIT, + WDT_AXP_FIXED_ENABLE_BIT); + dev->clk_rate = clk_get_rate(dev->clk); + return 0; + } + + /* Mandatory fallback for proper devicetree backward compatibility */ + dev->clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(dev->clk)) + return PTR_ERR(dev->clk); + ret = clk_prepare_enable(dev->clk); + if (ret) { + clk_put(dev->clk); + return ret; + } + + atomic_io_modify(dev->reg + TIMER_CTRL, + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT), + WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT)); + dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO; + return 0; +} + static int armadaxp_wdt_clock_init(struct platform_device *pdev, struct orion_watchdog *dev) { @@ -394,7 +432,7 @@ static const struct orion_watchdog_data armada375_data = { .rstout_mask_bit = BIT(10), .wdt_enable_bit = BIT(8), .wdt_counter_offset = 0x34, - .clock_init = armada370_wdt_clock_init, + .clock_init = armada375_wdt_clock_init, .enabled = armada375_enabled, .start = armada375_start, .stop = armada375_stop,
The 25 MHz reference clock has better stability so its use is preferred over the core clock. Change the Armada 375 clock initialization to use this reference clock. To ensure the driver is compatible with an old devicetree, also provide a fallback path which will silently return to the previous behavior. While here, add the clock specification to the binding documentation. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- .../devicetree/bindings/watchdog/marvel.txt | 13 +++++++ drivers/watchdog/orion_wdt.c | 40 +++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-)