Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Tomasz Figa <tomasz.figa@gmail.com>]

On Monday 25 of November 2013 14:44:01 Doug Anderson wrote:
> > +
> >  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.

That would mean adding extra tabs in lines above to align them with the
longest drv_data field.

AFAIK we're observing a trend of moving away from such field alignment,
so IMHO it would be better to keep this as is in this version and just
send a separate patch removing the alignment of remaining fields.

> 
> >  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;

Jumping away just to a single return statement isn't really a good idea.
If there is nothing to do, returning right away seems less confusing IMHO
(and saves you one line of code per each such error case as a side
effect).

A separate patch could be possibly made to clean-up remaining error cases
and remove the err label.

As for all the remaining points, I agree with Doug.

Best regards,
Tomasz