Hi Doug,
Thanks for reviewing the patch series.
On Tue, Nov 26, 2013 at 4:14 AM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi Leela Krishna,
>
> On Mon, Nov 18, 2013 at 1:49 AM, Leela Krishna Amudala
> <l.krishna@xxxxxxxxxxx> wrote:
> > Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap interface
> > to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers of PMU
> > to mask/unmask enable/disable of watchdog in probe and s2r scenarios.
> >
> > Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>
> > ---
> > .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++-
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/s3c2410_wdt.c | 145 ++++++++++++++++++--
> > 3 files changed, 157 insertions(+), 10 deletions(-)
>
> ...
>
> > +struct s3c2410_wdt_variant {
> > + int disable_reg;
> > + int mask_reset_reg;
> > + int mask_bit;
> > + u32 quirks;
> > +};
>
> Ideally you could add descriptions. I added them in a patch based on
> yours <https://chromium-review.googlesource.com/#/c/177935/1/drivers/watchdog/s3c2410_wdt.c>,
> but since yours hasn't landed perhaps you can do it directly?
>
> /**
> * struct s3c2410_wdt_variant - Per-variant config data
> *
> * @disable_reg: Offset in pmureg for the register that disables the watchdog
> * timer reset functionality.
> * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog
> * timer reset functionality.
> * @mask_bit: Bit number for the watchdog timer in the disable register and the
> * mask reset register.
> * @quirks: A bitfield of quirks.
> */
>
Okay, Added the above descriptions
> > +
> > struct s3c2410_wdt {
> > struct device *dev;
> > struct clk *clock;
> > @@ -94,7 +107,49 @@ struct s3c2410_wdt {
> > unsigned long wtdat_save;
> > struct watchdog_device wdt_device;
> > struct notifier_block freq_transition;
> > + struct s3c2410_wdt_variant *drv_data;
> > + struct regmap *pmureg;
>
> Total nit, but everything else in this structure is tab aligned and
> your new elements are not.
>
Me and Tomasz had a discussion about it and he replied for this comment
> > static int s3c2410wdt_probe(struct platform_device *pdev)
> > {
> > struct device *dev;
> > @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > spin_lock_init(&wdt->lock);
> > wdt->wdt_device = s3c2410_wdd;
> >
> > + wdt->drv_data = get_wdt_drv_data(pdev);
> > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
> > + wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "samsung,syscon-phandle");
> > + if (IS_ERR(wdt->pmureg)) {
> > + dev_err(dev, "syscon regmap lookup failed.\n");
> > + return PTR_ERR(wdt->pmureg);
>
> nit: this function appears to never call "return" directly. You'll
> match other error handling better if you do:
>
> ret = PTR_ERR(wdt->pmureg);
> goto err;
>
> (if someone else has already said they don't like that, just ignore me).
>
Will consider Tomasz suggestion for this
> > + }
> > + }
> > +
> > wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > if (wdt_irq == NULL) {
> > dev_err(dev, "no irq resource specified\n");
> > @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
> > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
> >
> > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
> > + ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
> > + if (ret < 0) {
> > + dev_err(wdt->dev, "failed to update pmu register");
> > + goto err_cpufreq;
> > + }
> > + }
>
> There are a few minor problems here:
>
> 1. You're putting a potential failure condition _after_ printing that
> the watchdog is enabled. You should put your code above the
> "dev_info".
>
Yes moved it like below
static int s3c2410wdt_probe(struct platform_device *pdev)
{
[snip]
if (tmr_atboot && started == 0) {
dev_info(dev, "starting watchdog timer\n");
s3c2410wdt_start(&wdt->wdt_device);
}
[snip]
> 2. Error handling doesn't seem quite right. If you fail here you'll
> be returning an error but you might have started the watchdog already.
> Perhaps you should be moving the "mask_and_disable" up a little and
> then you an undo it if needed?
>
Above mentioned comment will take care of this
> 3. I think it would be fine to put the "dev_err" directly in the
> s3c2410wdt_mask_and_disable_reset() as per Guenter, but still return
> the error code. You'll still use the error code here, right?
>
Yes, used dev_err and moved it into s3c2410wdt_mask_and_disable_reset()
and removed the dev_err message in the if (ret < 0) check
> > +
> > return 0;
> >
> > err_cpufreq:
> > @@ -459,8 +566,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >
> > static int s3c2410wdt_remove(struct platform_device *dev)
> > {
> > + int ret;
> > struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
> >
> > + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
> > + ret = s3c2410wdt_mask_and_disable_reset(wdt, true);
>
> This function does return an error. Why not pass the error on
> through? The system wouldn't be in such a stellar state, but if
> regmap is failing it's probably pretty bad anyway...
>
>
Yes, returning the error here and doing the same in other functions also
like suspend and resume
Will post the next version tomorrow
Best Wishes,
Leela Krishna.
>
> Nothing here is terrible and I wouldn't be upset if you landed these
> as-is, but since it seems that Guenter is requesting a spin. Perhaps
> you could address my comments, too?
>
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
