On 09.01.2025 19:00, Guenter Roeck wrote: > On Thu, Jan 09, 2025 at 06:03:17PM +0100, Heiner Kallweit wrote: >> A number of network PHY drivers use the following code: >> >> name = devm_hwmon_sanitize_name(dev, dev_name(dev)); >> if (IS_ERR(name)) >> return PTR_ERR(name); >> devm_hwmon_device_register_with_info(dev, name, ..); >> >> Make this a generic fallback option and use the device name if no name >> is provided to devm_hwmon_device_register_with_info(). This would allow >> to simplify the affected drivers. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >> --- >> drivers/hwmon/hwmon.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c >> index 685d4ce8d..1fd3d94e1 100644 >> --- a/drivers/hwmon/hwmon.c >> +++ b/drivers/hwmon/hwmon.c >> @@ -1170,6 +1170,11 @@ devm_hwmon_device_register_with_info(struct device *dev, const char *name, >> if (!dev) >> return ERR_PTR(-EINVAL); >> >> + if (!name) >> + name = devm_hwmon_sanitize_name(dev, dev_name(dev)); >> + if (IS_ERR(name)) >> + return ERR_CAST(name); >> + > > That introduces an undiscussed change: It handles an ERR_PTR() > passed as name parameter, not just NULL. Why would that be warranted ? > It may not look like much, but it introduces an inconsistency: Other > pointers (dev, chip) are not checked against ERR_PTR(). It is also not > immediately obvious to me why the driver would want to check if the > passed name is an ER_PTR(). > If a caller would pass an ERRPTR as name, we'd be in trouble in __hwmon_device_register(), when the following code is executed: if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) By doing the IS_ERR() check outside the if(!name) clause we get a little bit more of argument checking for free, at least with no code overhead. If that's not wanted I can also move the check into the if(!name) clause. > Guenter > Heiner