Re: [PATCH v8 11/14] platform/x86: oxpec: Adhere to sysfs-class-hwmon and enable pwm on 2

[Antheas Kapenekakis]

On Fri, 11 Apr 2025 at 17:13, Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> 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@xxxxxxxxx/
> > Reviewed-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
> > Reviewed-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > ---
> >  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.