Re: [PATCH v6 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

[Andreas Werner]

On Wed, Aug 27, 2014 at 11:38:43AM -0700, Guenter Roeck wrote:
> On Wed, Aug 27, 2014 at 07:53:06PM +0200, Andreas Werner wrote:
> > Added driver to support the 14F021P00 BMC Hardware Monitoring.
> > The BMC is a Board Management Controller including monitoring of the
> > board voltages.
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@xxxxxx>
> 
> Looks good, though I do have one comment ...
> 
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> > +						   "menf21bmc", drv_data,
> > +						   menf21bmc_hwmon_groups);
> > +	if (IS_ERR(hwmon_dev))
> > +		return PTR_ERR(hwmon_dev);
> > +
> > +	dev_info(&pdev->dev, "MEN 14F021P00 BMC hwmon device enabled");
> > +
> You removed the dev_info from the mfd driver but left it in the client
> drivers.  If you'd drop it here as well, you could use
> 'return PTR_ERR_OR_ZERO(hwmon_dev);'.  Just a suggestion, not mandatory.
> 
> Either case,

It is a nice info for every slave driver which are the drivers for accessing
the features in the BMC. The mfd is just the driver who will instantiate the
slaves. For the mfd the most important info is if there was an error.

Anyway good idea with PTR_ERR_OR_ZERO.

Thanks for the nice review and the comments.

Regards
Andy

> 
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> 
> I assume this will go in through the mfd tree.
> 
> Thanks,
> Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/