Re: [PATCH v6 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver
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 > > Looks good, though I do have one comment ... > > > + > > + hwmon_dev = devm_hwmon_device_register_with_groups(>dev, > > + "menf21bmc", drv_data, > > + menf21bmc_hwmon_groups); > > + if (IS_ERR(hwmon_dev)) > > + return PTR_ERR(hwmon_dev); > > + > > + dev_info(>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 > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver
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.wer...@men.de 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 li...@roeck-us.net 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver
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 Looks good, though I do have one comment ... > + > + hwmon_dev = devm_hwmon_device_register_with_groups(>dev, > +"menf21bmc", drv_data, > +menf21bmc_hwmon_groups); > + if (IS_ERR(hwmon_dev)) > + return PTR_ERR(hwmon_dev); > + > + dev_info(>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, Reviewed-by: Guenter Roeck 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver
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.wer...@men.de 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, Reviewed-by: Guenter Roeck li...@roeck-us.net 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
