| Message ID | 20250322103606.680401-12-lkml@antheas.dev |
|---|---|
| State | New |
| Headers | show |
| Series | hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86 | expand |
On Sat, 22 Mar 2025, Antheas Kapenekakis wrote: > Currently, the driver does not adhere to the sysfs-class-hwmon > specification: 0 is used for auto fan control and 1 is used for manual > control. However, it is expected that 0 sets the fan to full speed, > 1 sets the fan to manual, and then 2 is used for automatic control. > > Therefore, change the sysfs API to reflect this and enable pwm on 2. > > As we are breaking the ABI for this driver, rename oxpec to oxp_ec, > reflecting the naming convention used by other drivers, to allow for > a smooth migration in current userspace programs. So there's no gracious deprecation of the previous interface? It doesn't sound "smooth" by any means from userspace perspective. > Closes: https://lore.kernel.org/linux-hwmon/20241027174836.8588-1-derekjohn.clark@gmail.com/ > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/platform/x86/oxpec.c | 37 ++++++++++++++++++++++++++++++++---- > 1 file changed, 33 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c > index 5b84647569931..3bf2c597e9b00 100644 > --- a/drivers/platform/x86/oxpec.c > +++ b/drivers/platform/x86/oxpec.c > @@ -731,7 +731,27 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, > case hwmon_pwm_input: > return oxp_pwm_input_read(val); > case hwmon_pwm_enable: > - return oxp_pwm_read(val); > + ret = oxp_pwm_read(val); > + if (ret) > + return ret; > + > + /* Check for auto and return 2 */ > + if (!*val) { > + *val = 2; > + return 0; > + } > + > + /* Return 0 if at full fan speed, 1 otherwise */ > + ret = oxp_pwm_fan_speed(val); > + if (ret) > + return ret; > + > + if (*val == 255) > + *val = 0; > + else > + *val = 1; Why all these literals? These should be named properly with defines are there some defines/enums for some of them already if this hwmon ABI? > + > + return 0; > default: > break; > } > @@ -745,15 +765,24 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, > static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel, long val) > { > + int ret; > + > switch (type) { > case hwmon_pwm: > switch (attr) { > case hwmon_pwm_enable: > if (val == 1) > return oxp_pwm_enable(); > - else if (val == 0) > + else if (val == 2) > return oxp_pwm_disable(); > - return -EINVAL; > + else if (val != 0) Literals here too should be changed. > + return -EINVAL; > + > + /* Enable PWM and set to max speed */ > + ret = oxp_pwm_enable(); > + if (ret) > + return ret; > + return oxp_pwm_input_write(255); > case hwmon_pwm_input: > return oxp_pwm_input_write(val); > default: > @@ -818,7 +847,7 @@ static int oxp_platform_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device *hwdev; > > - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, > + hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL, > &oxp_ec_chip_info, NULL); > > return PTR_ERR_OR_ZERO(hwdev); >
On Fri, 11 Apr 2025 at 17:13, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Sat, 22 Mar 2025, Antheas Kapenekakis wrote: > > > Currently, the driver does not adhere to the sysfs-class-hwmon > > specification: 0 is used for auto fan control and 1 is used for manual > > control. However, it is expected that 0 sets the fan to full speed, > > 1 sets the fan to manual, and then 2 is used for automatic control. > > > > Therefore, change the sysfs API to reflect this and enable pwm on 2. > > > > As we are breaking the ABI for this driver, rename oxpec to oxp_ec, > > reflecting the naming convention used by other drivers, to allow for > > a smooth migration in current userspace programs. > > So there's no gracious deprecation of the previous interface? It doesn't > sound "smooth" by any means from userspace perspective. The previous interface was not compliant with the hwmon ABI, so any software that used it without being cognisant of that would misbehave. This driver got really fleshed out with 6.12, before that there was one userspace software that relied on this. We made sure to update all software that binds to oxpec specifically so it is not a problem now. By adding a dash at the name at the same time as the change is done it is possible to handle both the previous interface and this one at the same time. It is not ideal by any means, but if we don't change it now we will not be able to again. > > Closes: https://lore.kernel.org/linux-hwmon/20241027174836.8588-1-derekjohn.clark@gmail.com/ > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > drivers/platform/x86/oxpec.c | 37 ++++++++++++++++++++++++++++++++---- > > 1 file changed, 33 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c > > index 5b84647569931..3bf2c597e9b00 100644 > > --- a/drivers/platform/x86/oxpec.c > > +++ b/drivers/platform/x86/oxpec.c > > @@ -731,7 +731,27 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, > > case hwmon_pwm_input: > > return oxp_pwm_input_read(val); > > case hwmon_pwm_enable: > > - return oxp_pwm_read(val); > > + ret = oxp_pwm_read(val); > > + if (ret) > > + return ret; > > + > > + /* Check for auto and return 2 */ > > + if (!*val) { > > + *val = 2; > > + return 0; > > + } > > + > > + /* Return 0 if at full fan speed, 1 otherwise */ > > + ret = oxp_pwm_fan_speed(val); > > + if (ret) > > + return ret; > > + > > + if (*val == 255) > > + *val = 0; > > + else > > + *val = 1; > > Why all these literals? These should be named properly with defines are > there some defines/enums for some of them already if this hwmon ABI? Specifically for pwm_enable I don't think so actually. With a quick grep all drivers I checked use literals. > > + > > + return 0; > > default: > > break; > > } > > @@ -745,15 +765,24 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, > > static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type, > > u32 attr, int channel, long val) > > { > > + int ret; > > + > > switch (type) { > > case hwmon_pwm: > > switch (attr) { > > case hwmon_pwm_enable: > > if (val == 1) > > return oxp_pwm_enable(); > > - else if (val == 0) > > + else if (val == 2) > > return oxp_pwm_disable(); > > - return -EINVAL; > > + else if (val != 0) > > Literals here too should be changed. > > > + return -EINVAL; > > + > > + /* Enable PWM and set to max speed */ > > + ret = oxp_pwm_enable(); > > + if (ret) > > + return ret; > > + return oxp_pwm_input_write(255); > > case hwmon_pwm_input: > > return oxp_pwm_input_write(val); > > default: > > @@ -818,7 +847,7 @@ static int oxp_platform_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct device *hwdev; > > > > - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, > > + hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL, > > &oxp_ec_chip_info, NULL); > > > > return PTR_ERR_OR_ZERO(hwdev); > > > > -- > i.
diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c index 5b84647569931..3bf2c597e9b00 100644 --- a/drivers/platform/x86/oxpec.c +++ b/drivers/platform/x86/oxpec.c @@ -731,7 +731,27 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, case hwmon_pwm_input: return oxp_pwm_input_read(val); case hwmon_pwm_enable: - return oxp_pwm_read(val); + ret = oxp_pwm_read(val); + if (ret) + return ret; + + /* Check for auto and return 2 */ + if (!*val) { + *val = 2; + return 0; + } + + /* Return 0 if at full fan speed, 1 otherwise */ + ret = oxp_pwm_fan_speed(val); + if (ret) + return ret; + + if (*val == 255) + *val = 0; + else + *val = 1; + + return 0; default: break; } @@ -745,15 +765,24 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long val) { + int ret; + switch (type) { case hwmon_pwm: switch (attr) { case hwmon_pwm_enable: if (val == 1) return oxp_pwm_enable(); - else if (val == 0) + else if (val == 2) return oxp_pwm_disable(); - return -EINVAL; + else if (val != 0) + return -EINVAL; + + /* Enable PWM and set to max speed */ + ret = oxp_pwm_enable(); + if (ret) + return ret; + return oxp_pwm_input_write(255); case hwmon_pwm_input: return oxp_pwm_input_write(val); default: @@ -818,7 +847,7 @@ static int oxp_platform_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device *hwdev; - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL, + hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL, &oxp_ec_chip_info, NULL); return PTR_ERR_OR_ZERO(hwdev);