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