Re: [PATCH v2 07/12] hwmon: (oxp-sensors) Move pwm_enable read to its own function

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On February 22, 2025 8:18:18 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>Currently, this driver breaks sysfs by using auto as 0 and manual as 1.

It breaks hwmon ABI convention, the sysfs is fully functional. Please be more accurate as maintainers may misunderstand the problem here. This comment applies to 8/12 as well. You should probably link the discussion where this was identified as well for context.

<https://lore.kernel.org/linux-hwmon/20241027174836.8588-1-derekjohn.clark@gmail.com/T/#u>

>However, for pwm_enable, 0 is full speed, 1 is manual, and 2 is auto.
>For the correction to be possible, this means that the pwm_enable
>endpoint will need access to both pwm enable and value (as for
>the 0th value, the fan needs to be set to full power).
>
>Therefore, begin by moving the current pwm_enable read to its own
>function, oxp_pwm_enable.
>
>Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>---
> drivers/hwmon/oxp-sensors.c | 50 ++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
>index 9c43ec0fc994..1da1e1655f96 100644
>--- a/drivers/hwmon/oxp-sensors.c
>+++ b/drivers/hwmon/oxp-sensors.c
>@@ -762,6 +762,32 @@ static int oxp_pwm_disable(void)
> 	}
> }
> 
>+static int oxp_pwm_read(long *val)
>+{
>+	switch (board) {
>+	case orange_pi_neo:
>+		return read_from_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, 1, val);
>+	case aok_zoe_a1:
>+	case aya_neo_2:
>+	case aya_neo_air:
>+	case aya_neo_air_1s:
>+	case aya_neo_air_plus_mendo:
>+	case aya_neo_air_pro:
>+	case aya_neo_flip:
>+	case aya_neo_geek:
>+	case aya_neo_kun:
>+	case oxp_2:
>+	case oxp_fly:
>+	case oxp_mini_amd:
>+	case oxp_mini_amd_a07:
>+	case oxp_mini_amd_pro:
>+	case oxp_x1:
>+		return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
> /* Callbacks for hwmon interface */
> static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
> 				       enum hwmon_sensor_types type, u32 attr, int channel)
>@@ -859,29 +885,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> 			}
> 			return 0;
> 		case hwmon_pwm_enable:
>-			switch (board) {
>-			case orange_pi_neo:
>-				return read_from_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, 1, val);
>-			case aok_zoe_a1:
>-			case aya_neo_2:
>-			case aya_neo_air:
>-			case aya_neo_air_1s:
>-			case aya_neo_air_plus_mendo:
>-			case aya_neo_air_pro:
>-			case aya_neo_flip:
>-			case aya_neo_geek:
>-			case aya_neo_kun:
>-			case oxp_2:
>-			case oxp_fly:
>-			case oxp_mini_amd:
>-			case oxp_mini_amd_a07:
>-			case oxp_mini_amd_pro:
>-			case oxp_x1:
>-				return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
>-			default:
>-				break;
>-			}
>-			break;
>+			return oxp_pwm_read(val);
> 		default:
> 			break;
> 		}

- Derek