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 linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html