[PATCH 0/4] hwmon: (oxp-sensors) Cleanup device type handling

[PATCH 0/4] hwmon: (oxp-sensors) Cleanup device type handling

[tjakobi]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

Hello,

this series tries to cleanup the handling of the various device types
in the oxp-sensors drivers. The current approach is error-prone
since it relies on ever-growing switch-statements, which are
present in multiple functions. Adding a single new device involves
touching a bunch of functions.

That this is problematic is already demonstrated by this bug in
the driver: While oxp_pwm_disable() handles the aya_neo_air_1s
model, the inverse function oxp_pwm_enable() does not handle the
model. This is obviously wrong and a result of the current design.

Introduce a different design with better separation of logic and
data. While at it, also fix some typos, wording and add a cache
for the PWM enable mode.

With best wishes,
Tobias

Tobias Jakobi (4):
  hwmon: (oxp-sensors) Separate logic from device-specific data
  hwmon: (oxp-sensors) Fix typos in documentation
  hwmon: (oxp-sensors) Fix wording in code comment
  hwmon: (oxp-sensors) Cache state of PWM enable mode

 Documentation/hwmon/oxp-sensors.rst |   2 +-
 drivers/hwmon/oxp-sensors.c         | 576 +++++++++++++---------------
 2 files changed, 275 insertions(+), 303 deletions(-)

-- 
2.45.2

[PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[tjakobi]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

We currently have large switch-statements in all functions that
write to EC registers, even though the bulk of the supported
devices functions more or less the same.

Factor the device-specific data out into a struct oxp_config. This
only leaves logic in the corresponding functions and should make
adding future devices much easier and less error-prone.

Also introduce struct oxp_data which is going to be used in a
later commit to cache device state.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
 1 file changed, 215 insertions(+), 302 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 83730d931824..fbd1463d1494 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -42,27 +42,6 @@ static bool unlock_global_acpi_lock(void)
 	return ACPI_SUCCESS(acpi_release_global_lock(oxp_mutex));
 }
 
-enum oxp_board {
-	aok_zoe_a1 = 1,
-	aya_neo_2,
-	aya_neo_air,
-	aya_neo_air_1s,
-	aya_neo_air_plus_mendo,
-	aya_neo_air_pro,
-	aya_neo_flip,
-	aya_neo_geek,
-	aya_neo_kun,
-	orange_pi_neo,
-	oxp_2,
-	oxp_fly,
-	oxp_mini_amd,
-	oxp_mini_amd_a07,
-	oxp_mini_amd_pro,
-	oxp_x1,
-};
-
-static enum oxp_board board;
-
 /* Fan reading and PWM */
 #define OXP_SENSOR_FAN_REG             0x76 /* Fan reading is 2 registers long */
 #define OXP_2_SENSOR_FAN_REG           0x58 /* Fan reading is 2 registers long */
@@ -89,125 +68,212 @@ static enum oxp_board board;
 
 #define OXP_TURBO_RETURN_VAL           0x00 /* Common return val */
 
+enum oxp_feature_bits {
+	OXP_FEATURE_SENSOR_FAN,
+	OXP_FEATURE_PWM,
+	OXP_FEATURE_TURBO,
+	OXP_FEATURE_ONLY_AMD,
+};
+
+struct oxp_config {
+	unsigned long features;
+
+	u8 sensor_pwm_enable_reg;
+	u8 sensor_pwm_reg;
+	u8 sensor_pwm_scale[2];
+	u8 sensor_fan_reg;
+
+	u8 turbo_switch_reg;
+	u8 turbo_enable_val;
+};
+
+struct oxp_data {
+	struct device *hwmon_dev;
+	const struct oxp_config *config;
+};
+
+static const struct oxp_config config_oxp = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM),
+
+	.sensor_pwm_enable_reg = OXP_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = OXP_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {0, 100},
+	.sensor_fan_reg = OXP_SENSOR_FAN_REG,
+};
+
+static const struct oxp_config config_mini_amd = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM) | BIT(OXP_FEATURE_ONLY_AMD),
+
+	.sensor_pwm_enable_reg = OXP_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = OXP_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {0, 100},
+	.sensor_fan_reg = OXP_SENSOR_FAN_REG,
+};
+
+static const struct oxp_config config_mini_amd_a07 = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM) | BIT(OXP_FEATURE_TURBO),
+
+	.sensor_pwm_enable_reg = OXP_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = OXP_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {0, 100},
+	.sensor_fan_reg = OXP_SENSOR_FAN_REG,
+
+	.turbo_switch_reg = OXP_MINI_TURBO_SWITCH_REG,
+	.turbo_enable_val = OXP_MINI_TURBO_TAKE_VAL,
+};
+
+static const struct oxp_config config_oxp2_turbo = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM) | BIT(OXP_FEATURE_TURBO),
+
+	.sensor_pwm_enable_reg = OXP_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = OXP_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {0, 184},
+	.sensor_fan_reg = OXP_2_SENSOR_FAN_REG,
+
+	.turbo_switch_reg = OXP_2_TURBO_SWITCH_REG,
+	.turbo_enable_val = OXP_TURBO_TAKE_VAL,
+};
+
+static const struct oxp_config config_orange = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM),
+
+	.sensor_pwm_enable_reg = ORANGEPI_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = ORANGEPI_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {1, 244},
+	.sensor_fan_reg = ORANGEPI_SENSOR_FAN_REG,
+};
+
+static const struct oxp_config config_aok_zoe = {
+	.features = BIT(OXP_FEATURE_SENSOR_FAN) | BIT(OXP_FEATURE_PWM) | BIT(OXP_FEATURE_TURBO),
+
+	.sensor_pwm_enable_reg = OXP_SENSOR_PWM_ENABLE_REG,
+	.sensor_pwm_reg = OXP_SENSOR_PWM_REG,
+	.sensor_pwm_scale = {0, 255},
+	.sensor_fan_reg = OXP_SENSOR_FAN_REG,
+
+	.turbo_switch_reg = OXP_TURBO_SWITCH_REG,
+	.turbo_enable_val = OXP_TURBO_TAKE_VAL,
+};
+
 static const struct dmi_system_id dmi_table[] = {
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AOKZOE"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AOKZOE A1 AR07"),
 		},
-		.driver_data = (void *)aok_zoe_a1,
+		.driver_data = (void *)&config_aok_zoe,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AOKZOE"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AOKZOE A1 Pro"),
 		},
-		.driver_data = (void *)aok_zoe_a1,
+		.driver_data = (void *)&config_aok_zoe,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_MATCH(DMI_BOARD_NAME, "AYANEO 2"),
 		},
-		.driver_data = (void *)aya_neo_2,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AIR"),
 		},
-		.driver_data = (void *)aya_neo_air,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AIR 1S"),
 		},
-		.driver_data = (void *)aya_neo_air_1s,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AB05-Mendocino"),
 		},
-		.driver_data = (void *)aya_neo_air_plus_mendo,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AIR Pro"),
 		},
-		.driver_data = (void *)aya_neo_air_pro,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_MATCH(DMI_BOARD_NAME, "FLIP"),
 		},
-		.driver_data = (void *)aya_neo_flip,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_MATCH(DMI_BOARD_NAME, "GEEK"),
 		},
-		.driver_data = (void *)aya_neo_geek,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "KUN"),
 		},
-		.driver_data = (void *)aya_neo_kun,
+		.driver_data = (void *)&config_oxp,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "OrangePi"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "NEO-01"),
 		},
-		.driver_data = (void *)orange_pi_neo,
+		.driver_data = (void *)&config_orange,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONE XPLAYER"),
 		},
-		.driver_data = (void *)oxp_mini_amd,
+		.driver_data = (void *)&config_mini_amd,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_MATCH(DMI_BOARD_NAME, "ONEXPLAYER 2"),
 		},
-		.driver_data = (void *)oxp_2,
+		.driver_data = (void *)&config_oxp2_turbo,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER F1"),
 		},
-		.driver_data = (void *)oxp_fly,
+		.driver_data = (void *)&config_aok_zoe,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER mini A07"),
 		},
-		.driver_data = (void *)oxp_mini_amd_a07,
+		.driver_data = (void *)&config_mini_amd_a07,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_EXACT_MATCH(DMI_BOARD_NAME, "ONEXPLAYER Mini Pro"),
 		},
-		.driver_data = (void *)oxp_mini_amd_pro,
+		.driver_data = (void *)&config_aok_zoe,
 	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_BOARD_VENDOR, "ONE-NETBOOK"),
 			DMI_MATCH(DMI_BOARD_NAME, "ONEXPLAYER X1"),
 		},
-		.driver_data = (void *)oxp_x1,
+		.driver_data = (void *)&config_oxp2_turbo,
 	},
 	{},
 };
@@ -252,83 +318,59 @@ static int write_to_ec(u8 reg, u8 value)
 	return ret;
 }
 
-/* Turbo button toggle functions */
-static int tt_toggle_enable(void)
+/* Rescale a (HW) sensor PWM value to userspace range. */
+static long rescale_sensor_pwm_to_user(const struct oxp_config *config, long val)
 {
-	u8 reg;
-	u8 val;
+	const long range = config->sensor_pwm_scale[1] - config->sensor_pwm_scale[0];
 
-	switch (board) {
-	case oxp_mini_amd_a07:
-		reg = OXP_MINI_TURBO_SWITCH_REG;
-		val = OXP_MINI_TURBO_TAKE_VAL;
-		break;
-	case aok_zoe_a1:
-	case oxp_fly:
-	case oxp_mini_amd_pro:
-		reg = OXP_TURBO_SWITCH_REG;
-		val = OXP_TURBO_TAKE_VAL;
-		break;
-	case oxp_2:
-	case oxp_x1:
-		reg = OXP_2_TURBO_SWITCH_REG;
-		val = OXP_TURBO_TAKE_VAL;
-		break;
-	default:
-		return -EINVAL;
-	}
-	return write_to_ec(reg, val);
+	if (range == 255)
+		return val;
+
+	return ((val - config->sensor_pwm_scale[0]) * 255) / range;
 }
 
-static int tt_toggle_disable(void)
+/* Rescale a (userspace) sensor PWM value to hw range. */
+static long rescale_sensor_pwm_to_hw(const struct oxp_config *config, long val)
 {
-	u8 reg;
-	u8 val;
+	const long range = config->sensor_pwm_scale[1] - config->sensor_pwm_scale[0];
 
-	switch (board) {
-	case oxp_mini_amd_a07:
-		reg = OXP_MINI_TURBO_SWITCH_REG;
-		val = OXP_TURBO_RETURN_VAL;
-		break;
-	case aok_zoe_a1:
-	case oxp_fly:
-	case oxp_mini_amd_pro:
-		reg = OXP_TURBO_SWITCH_REG;
-		val = OXP_TURBO_RETURN_VAL;
-		break;
-	case oxp_2:
-	case oxp_x1:
-		reg = OXP_2_TURBO_SWITCH_REG;
-		val = OXP_TURBO_RETURN_VAL;
-		break;
-	default:
+	if (range == 255)
+		return val;
+
+	return config->sensor_pwm_scale[0] + (val * range) / 255;
+}
+
+/* Turbo button toggle functions */
+static int tt_toggle_enable(const struct oxp_config *config)
+{
+	if (!test_bit(OXP_FEATURE_TURBO, &config->features))
 		return -EINVAL;
-	}
-	return write_to_ec(reg, val);
+
+	return write_to_ec(config->turbo_switch_reg, config->turbo_enable_val);
+}
+
+static int tt_toggle_disable(const struct oxp_config *config)
+{
+	if (!test_bit(OXP_FEATURE_TURBO, &config->features))
+		return -EINVAL;
+
+	return write_to_ec(config->turbo_switch_reg, OXP_TURBO_RETURN_VAL);
 }
 
 /* Callbacks for turbo toggle attribute */
 static umode_t tt_toggle_is_visible(struct kobject *kobj,
 				    struct attribute *attr, int n)
 {
-	switch (board) {
-	case aok_zoe_a1:
-	case oxp_2:
-	case oxp_fly:
-	case oxp_mini_amd_a07:
-	case oxp_mini_amd_pro:
-	case oxp_x1:
-		return attr->mode;
-	default:
-		break;
-	}
-	return 0;
+	return attr->mode;
 }
 
 static ssize_t tt_toggle_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
 {
+	struct oxp_data *data = dev_get_drvdata(dev);
+	const struct oxp_config *config = data->config;
+
 	int rval;
 	bool value;
 
@@ -337,9 +379,9 @@ static ssize_t tt_toggle_store(struct device *dev,
 		return rval;
 
 	if (value) {
-		rval = tt_toggle_enable();
+		rval = tt_toggle_enable(config);
 	} else {
-		rval = tt_toggle_disable();
+		rval = tt_toggle_disable(config);
 	}
 	if (rval)
 		return rval;
@@ -350,28 +392,15 @@ static ssize_t tt_toggle_store(struct device *dev,
 static ssize_t tt_toggle_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
+	struct oxp_data *data = dev_get_drvdata(dev);
+	const struct oxp_config *config = data->config;
 	int retval;
-	u8 reg;
 	long val;
 
-	switch (board) {
-	case oxp_mini_amd_a07:
-		reg = OXP_MINI_TURBO_SWITCH_REG;
-		break;
-	case aok_zoe_a1:
-	case oxp_fly:
-	case oxp_mini_amd_pro:
-		reg = OXP_TURBO_SWITCH_REG;
-		break;
-	case oxp_2:
-	case oxp_x1:
-		reg = OXP_2_TURBO_SWITCH_REG;
-		break;
-	default:
+	if (!test_bit(OXP_FEATURE_TURBO, &config->features))
 		return -EINVAL;
-	}
 
-	retval = read_from_ec(reg, 1, &val);
+	retval = read_from_ec(config->turbo_switch_reg, 1, &val);
 	if (retval)
 		return retval;
 
@@ -381,55 +410,20 @@ static ssize_t tt_toggle_show(struct device *dev,
 static DEVICE_ATTR_RW(tt_toggle);
 
 /* PWM enable/disable functions */
-static int oxp_pwm_enable(void)
+static int oxp_pwm_enable(const struct oxp_config *config)
 {
-	switch (board) {
-	case orange_pi_neo:
-		return write_to_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, PWM_MODE_MANUAL);
-	case aok_zoe_a1:
-	case aya_neo_2:
-	case aya_neo_air:
-	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 write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, PWM_MODE_MANUAL);
-	default:
-		return -EINVAL;
-	}
+	if (test_bit(OXP_FEATURE_PWM, &config->features))
+		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_MANUAL);
+
+	return -EINVAL;
 }
 
-static int oxp_pwm_disable(void)
+static int oxp_pwm_disable(const struct oxp_config *config)
 {
-	switch (board) {
-	case orange_pi_neo:
-		return write_to_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
-	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 write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
-	default:
-		return -EINVAL;
-	}
+	if (test_bit(OXP_FEATURE_PWM, &config->features))
+		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
+
+	return -EINVAL;
 }
 
 /* Callbacks for hwmon interface */
@@ -449,108 +443,33 @@ static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
 static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 			     u32 attr, int channel, long *val)
 {
-	int ret;
+	struct oxp_data *data = dev_get_drvdata(dev);
+	const struct oxp_config *config = data->config;
 
 	switch (type) {
 	case hwmon_fan:
-		switch (attr) {
-		case hwmon_fan_input:
-			switch (board) {
-			case orange_pi_neo:
-				return read_from_ec(ORANGEPI_SENSOR_FAN_REG, 2, val);
-			case oxp_2:
-			case oxp_x1:
-				return read_from_ec(OXP_2_SENSOR_FAN_REG, 2, 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_fly:
-			case oxp_mini_amd:
-			case oxp_mini_amd_a07:
-			case oxp_mini_amd_pro:
-				return read_from_ec(OXP_SENSOR_FAN_REG, 2, val);
-			default:
-				break;
-			}
-			break;
-		default:
-			break;
-		}
+		if (attr == hwmon_fan_input && test_bit(OXP_FEATURE_SENSOR_FAN, &config->features))
+			return read_from_ec(config->sensor_fan_reg, 2, val);
 		break;
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_input:
-			switch (board) {
-			case orange_pi_neo:
-				ret = read_from_ec(ORANGEPI_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [1-244] */
-				*val = ((*val - 1) * 254 / 243) + 1;
-				break;
-			case oxp_2:
-			case oxp_x1:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [0-184] */
-				*val = (*val * 255) / 184;
-				break;
-			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_mini_amd:
-			case oxp_mini_amd_a07:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
-				if (ret)
-					return ret;
-				/* scale from range [0-100] */
-				*val = (*val * 255) / 100;
-				break;
-			case aok_zoe_a1:
-			case oxp_fly:
-			case oxp_mini_amd_pro:
-			default:
-				ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
+			if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+				int ret;
+				long tmp;
+
+				ret = read_from_ec(config->sensor_pwm_reg, 1, &tmp);
 				if (ret)
 					return ret;
-				break;
+
+				*val = rescale_sensor_pwm_to_user(config, tmp);
+
+				return 0;
 			}
-			return 0;
+			break;
 		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;
-			}
+			if (test_bit(OXP_FEATURE_PWM, &config->features))
+				return read_from_ec(config->sensor_pwm_enable_reg, 1, val);
 			break;
 		default:
 			break;
@@ -559,53 +478,32 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 	default:
 		break;
 	}
+
 	return -EOPNOTSUPP;
 }
 
 static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long val)
 {
+	struct oxp_data *data = dev_get_drvdata(dev);
+	const struct oxp_config *config = data->config;
+
 	switch (type) {
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_enable:
 			if (val == 1)
-				return oxp_pwm_enable();
+				return oxp_pwm_enable(config);
 			else if (val == 0)
-				return oxp_pwm_disable();
+				return oxp_pwm_disable(config);
 			return -EINVAL;
 		case hwmon_pwm_input:
 			if (val < 0 || val > 255)
 				return -EINVAL;
-			switch (board) {
-			case orange_pi_neo:
-				/* scale to range [1-244] */
-				val = ((val - 1) * 243 / 254) + 1;
-				return write_to_ec(ORANGEPI_SENSOR_PWM_REG, val);
-			case oxp_2:
-			case oxp_x1:
-				/* scale to range [0-184] */
-				val = (val * 184) / 255;
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			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_mini_amd:
-			case oxp_mini_amd_a07:
-				/* scale to range [0-100] */
-				val = (val * 100) / 255;
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			case aok_zoe_a1:
-			case oxp_fly:
-			case oxp_mini_amd_pro:
-				return write_to_ec(OXP_SENSOR_PWM_REG, val);
-			default:
-				break;
+			if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+				const long hw_val = rescale_sensor_pwm_to_hw(config, val);
+
+				return write_to_ec(config->sensor_pwm_reg, hw_val);
 			}
 			break;
 		default:
@@ -657,12 +555,43 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
 static int oxp_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device *hwdev;
+	struct oxp_data *data;
 
-	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
-						     &oxp_ec_chip_info, NULL);
+	const struct dmi_system_id *dmi_entry;
+	const struct oxp_config *config;
 
-	return PTR_ERR_OR_ZERO(hwdev);
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dmi_entry = dmi_first_match(dmi_table);
+	if (!dmi_entry)
+		return -ENODEV;
+
+	config = dmi_entry->driver_data;
+
+	/*
+	 * Have to check for AMD processor here because DMI strings are the same
+	 * between Intel and AMD boards on older OneXPlayer devices, the only way
+	 * to tell them apart is the CPU. Old Intel boards have an unsupported EC.
+	 */
+	if (test_bit(OXP_FEATURE_ONLY_AMD, &config->features) &&
+	    boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return -ENODEV;
+
+	/* If we lack the turbo feature, don't expose the turbo toggle attribute. */
+	if (!test_bit(OXP_FEATURE_TURBO, &config->features))
+		dev_attr_tt_toggle.attr.mode = 0;
+
+	data->config = config;
+	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "oxpec", data,
+							       &oxp_ec_chip_info, NULL);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
 }
 
 static struct platform_driver oxp_platform_driver = {
@@ -677,22 +606,6 @@ static struct platform_device *oxp_platform_device;
 
 static int __init oxp_platform_init(void)
 {
-	const struct dmi_system_id *dmi_entry;
-
-	dmi_entry = dmi_first_match(dmi_table);
-	if (!dmi_entry)
-		return -ENODEV;
-
-	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
-
-	/*
-	 * Have to check for AMD processor here because DMI strings are the same
-	 * between Intel and AMD boards on older OneXPlayer devices, the only way
-	 * to tell them apart is the CPU. Old Intel boards have an unsupported EC.
-	 */
-	if (board == oxp_mini_amd && boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
-		return -ENODEV;
-
 	oxp_platform_device =
 		platform_create_bundle(&oxp_platform_driver,
 				       oxp_platform_probe, NULL, 0, NULL, 0);
-- 
2.45.2

[PATCH 2/4] hwmon: (oxp-sensors) Fix typos in documentation

[tjakobi]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

No functional changes.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 Documentation/hwmon/oxp-sensors.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/hwmon/oxp-sensors.rst b/Documentation/hwmon/oxp-sensors.rst
index 581c4dafbfa1..5f70ccce0b0a 100644
--- a/Documentation/hwmon/oxp-sensors.rst
+++ b/Documentation/hwmon/oxp-sensors.rst
@@ -74,7 +74,7 @@ fan1_input
 
 pwm1_enable
   Read Write. Enable manual fan control. Write "1" to set to manual, write "0"
-  to let the EC control de fan speed. Read this attribute to see current status.
+  to let the EC control the fan speed. Read this attribute to see current status.
 
 pwm1
   Read Write. Read this attribute to see current duty cycle in the range [0-255].
-- 
2.45.2

[PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

[tjakobi]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

Despite what the current comment says, the register is used
both for reading and writing the PWM value.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/hwmon/oxp-sensors.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index fbd1463d1494..8089349fa508 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void)
 #define OXP_SENSOR_FAN_REG             0x76 /* Fan reading is 2 registers long */
 #define OXP_2_SENSOR_FAN_REG           0x58 /* Fan reading is 2 registers long */
 #define OXP_SENSOR_PWM_ENABLE_REG      0x4A /* PWM enable is 1 register long */
-#define OXP_SENSOR_PWM_REG             0x4B /* PWM reading is 1 register long */
+#define OXP_SENSOR_PWM_REG             0x4B /* PWM control is 1 register long */
 #define PWM_MODE_AUTO                  0x00
 #define PWM_MODE_MANUAL                0x01
 
 /* OrangePi fan reading and PWM */
 #define ORANGEPI_SENSOR_FAN_REG        0x78 /* Fan reading is 2 registers long */
 #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */
-#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM reading is 1 register long */
+#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM control is 1 register long */
 
 /* Turbo button takeover function
  * Different boards have different values and EC registers
-- 
2.45.2

[PATCH 4/4] hwmon: (oxp-sensors) Cache state of PWM enable mode

[tjakobi]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

The driver is in full control of the enable mode, so we
don't need to read it from HW every single time.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/hwmon/oxp-sensors.c | 81 ++++++++++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 8089349fa508..6790bc9e0da3 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -90,6 +90,8 @@ struct oxp_config {
 struct oxp_data {
 	struct device *hwmon_dev;
 	const struct oxp_config *config;
+
+	bool pwm_auto; /* Is the EC controlling the PWM automatically? */
 };
 
 static const struct oxp_config config_oxp = {
@@ -318,6 +320,22 @@ static int write_to_ec(u8 reg, u8 value)
 	return ret;
 }
 
+static int pwm_auto_from_hw(struct oxp_data *data)
+{
+	const struct oxp_config *config = data->config;
+
+	long tmp;
+	int ret;
+
+	ret = read_from_ec(config->sensor_pwm_enable_reg, 1, &tmp);
+	if (ret < 0)
+		return ret;
+
+	data->pwm_auto = tmp == PWM_MODE_AUTO;
+
+	return ret;
+}
+
 /* Rescale a (HW) sensor PWM value to userspace range. */
 static long rescale_sensor_pwm_to_user(const struct oxp_config *config, long val)
 {
@@ -410,18 +428,48 @@ static ssize_t tt_toggle_show(struct device *dev,
 static DEVICE_ATTR_RW(tt_toggle);
 
 /* PWM enable/disable functions */
-static int oxp_pwm_enable(const struct oxp_config *config)
+static int oxp_pwm_enable(struct oxp_data *data)
 {
-	if (test_bit(OXP_FEATURE_PWM, &config->features))
-		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_MANUAL);
+	const struct oxp_config *config;
+	int ret;
+
+	if (!data->pwm_auto)
+		return 0;
+
+	config = data->config;
+
+	if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+		ret = write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_MANUAL);
+		if (ret < 0)
+			return ret;
+
+		data->pwm_auto = false;
+
+		return 0;
+	}
 
 	return -EINVAL;
 }
 
-static int oxp_pwm_disable(const struct oxp_config *config)
+static int oxp_pwm_disable(struct oxp_data *data)
 {
-	if (test_bit(OXP_FEATURE_PWM, &config->features))
-		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
+	const struct oxp_config *config;
+	int ret;
+
+	if (data->pwm_auto)
+		return 0;
+
+	config = data->config;
+
+	if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+		ret = write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
+		if (ret < 0)
+			return ret;
+
+		data->pwm_auto = true;
+
+		return 0;
+	}
 
 	return -EINVAL;
 }
@@ -468,8 +516,11 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
 			}
 			break;
 		case hwmon_pwm_enable:
-			if (test_bit(OXP_FEATURE_PWM, &config->features))
-				return read_from_ec(config->sensor_pwm_enable_reg, 1, val);
+			if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+				*val = data->pwm_auto ? PWM_MODE_AUTO : PWM_MODE_MANUAL;
+
+				return 0;
+			}
 			break;
 		default:
 			break;
@@ -493,12 +544,12 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
 		switch (attr) {
 		case hwmon_pwm_enable:
 			if (val == 1)
-				return oxp_pwm_enable(config);
+				return oxp_pwm_enable(data);
 			else if (val == 0)
-				return oxp_pwm_disable(config);
+				return oxp_pwm_disable(data);
 			return -EINVAL;
 		case hwmon_pwm_input:
-			if (val < 0 || val > 255)
+			if (val < 0 || val > 255 || data->pwm_auto)
 				return -EINVAL;
 			if (test_bit(OXP_FEATURE_PWM, &config->features)) {
 				const long hw_val = rescale_sensor_pwm_to_hw(config, val);
@@ -591,6 +642,14 @@ static int oxp_platform_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
+	if (test_bit(OXP_FEATURE_PWM, &config->features)) {
+		int ret;
+
+		ret = pwm_auto_from_hw(data);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.45.2

Re: [PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

[Guenter Roeck]

On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> Despite what the current comment says, the register is used
> both for reading and writing the PWM value.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/hwmon/oxp-sensors.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index fbd1463d1494..8089349fa508 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void)
>  #define OXP_SENSOR_FAN_REG             0x76 /* Fan reading is 2 registers long */
>  #define OXP_2_SENSOR_FAN_REG           0x58 /* Fan reading is 2 registers long */
>  #define OXP_SENSOR_PWM_ENABLE_REG      0x4A /* PWM enable is 1 register long */
> -#define OXP_SENSOR_PWM_REG             0x4B /* PWM reading is 1 register long */
> +#define OXP_SENSOR_PWM_REG             0x4B /* PWM control is 1 register long */

I think that, if anything, this is more confusing than before.
"control" is, for example, enabling or disabling pwm management,
setting automatic or manual mode, or setting the pwm polarity.
Together ith the next two defines, "control" would suggest that
PWM_MODE_AUTO and PWM_MODE_MANUAL are set through that register -
which is not the case. "value" maybe, but "control" is just wrong.

Guenter

>  #define PWM_MODE_AUTO                  0x00
>  #define PWM_MODE_MANUAL                0x01
>  
>  /* OrangePi fan reading and PWM */
>  #define ORANGEPI_SENSOR_FAN_REG        0x78 /* Fan reading is 2 registers long */
>  #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */
> -#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM reading is 1 register long */
> +#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM control is 1 register long */
>  
>  /* Turbo button takeover function
>   * Different boards have different values and EC registers
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Guenter Roeck]

On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> We currently have large switch-statements in all functions that
> write to EC registers, even though the bulk of the supported
> devices functions more or less the same.
> 
> Factor the device-specific data out into a struct oxp_config. This
> only leaves logic in the corresponding functions and should make
> adding future devices much easier and less error-prone.
> 
> Also introduce struct oxp_data which is going to be used in a
> later commit to cache device state.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
>  1 file changed, 215 insertions(+), 302 deletions(-)
> 
...
> +
>  static const struct dmi_system_id dmi_table[] = {
>  	{
>  		.matches = {
>  			DMI_MATCH(DMI_BOARD_VENDOR, "AOKZOE"),
>  			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AOKZOE A1 AR07"),
>  		},
> -		.driver_data = (void *)aok_zoe_a1,
> +		.driver_data = (void *)&config_aok_zoe,

I have not looked at hte rest of the code, but the whole point of
void * is that a tyoe cast to or from it is not necessary.

Guenter

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Guenter Roeck]

On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> We currently have large switch-statements in all functions that
> write to EC registers, even though the bulk of the supported
> devices functions more or less the same.
> 
> Factor the device-specific data out into a struct oxp_config. This
> only leaves logic in the corresponding functions and should make
> adding future devices much easier and less error-prone.
> 
> Also introduce struct oxp_data which is going to be used in a
> later commit to cache device state.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
>  1 file changed, 215 insertions(+), 302 deletions(-)
> 
>  
...
> -static int oxp_pwm_disable(void)
> +static int oxp_pwm_disable(const struct oxp_config *config)
>  {
> -	switch (board) {
> -	case orange_pi_neo:
> -		return write_to_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
> -	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 write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
> -	default:
> -		return -EINVAL;
> -	}
> +	if (test_bit(OXP_FEATURE_PWM, &config->features))
> +		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
> +

This and all the other feature checks are completely wrong.
Those checks whould happen once in the is_visible functions,
and there should not be any such runtime checks. If a feature
is not available, the associated attributes should not be created
in the first place.

If such checks happen in the current code, that should be fixed
in the is_visible functions. Any existing runtime feature checks
should be removed.

Guenter

Re: [PATCH 4/4] hwmon: (oxp-sensors) Cache state of PWM enable mode

[Guenter Roeck]

On Thu, Dec 26, 2024 at 06:00:19PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> The driver is in full control of the enable mode, so we
> don't need to read it from HW every single time.
> 

That is not a reason for adding that much additional code.
What is the problem that is being solved, and why is it worth that much
additional code ?

Plus, again, all those runtime feature checks in attribute handling
code are completely wrong.

Guenter

Re: [PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

[Tobias Jakobi]

On 12/26/24 21:52, Guenter Roeck wrote:
> On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@math.uni-bielefeld.de wrote:
>> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>
>> Despite what the current comment says, the register is used
>> both for reading and writing the PWM value.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>   drivers/hwmon/oxp-sensors.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
>> index fbd1463d1494..8089349fa508 100644
>> --- a/drivers/hwmon/oxp-sensors.c
>> +++ b/drivers/hwmon/oxp-sensors.c
>> @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void)
>>   #define OXP_SENSOR_FAN_REG             0x76 /* Fan reading is 2 registers long */
>>   #define OXP_2_SENSOR_FAN_REG           0x58 /* Fan reading is 2 registers long */
>>   #define OXP_SENSOR_PWM_ENABLE_REG      0x4A /* PWM enable is 1 register long */
>> -#define OXP_SENSOR_PWM_REG             0x4B /* PWM reading is 1 register long */
>> +#define OXP_SENSOR_PWM_REG             0x4B /* PWM control is 1 register long */
> 
> I think that, if anything, this is more confusing than before.
> "control" is, for example, enabling or disabling pwm management,
> setting automatic or manual mode, or setting the pwm polarity.
> Together ith the next two defines, "control" would suggest that
> PWM_MODE_AUTO and PWM_MODE_MANUAL are set through that register -
> which is not the case. "value" maybe, but "control" is just wrong.
Noted. What do you think about "target" then?

My main point here was that reading implies that this register is 
read-only. Which it clearly isn't. And the documentation (which could be 
certainly be improved in general) should reflect that.

With best wishes,
Tobias

> 
> Guenter
> 
>>   #define PWM_MODE_AUTO                  0x00
>>   #define PWM_MODE_MANUAL                0x01
>>   
>>   /* OrangePi fan reading and PWM */
>>   #define ORANGEPI_SENSOR_FAN_REG        0x78 /* Fan reading is 2 registers long */
>>   #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */
>> -#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM reading is 1 register long */
>> +#define ORANGEPI_SENSOR_PWM_REG        0x38 /* PWM control is 1 register long */
>>   
>>   /* Turbo button takeover function
>>    * Different boards have different values and EC registers
>> -- 
>> 2.45.2
>>
>>

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Tobias Jakobi]

On 12/26/24 21:54, Guenter Roeck wrote:
> On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@math.uni-bielefeld.de wrote:
>> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>
>> We currently have large switch-statements in all functions that
>> write to EC registers, even though the bulk of the supported
>> devices functions more or less the same.
>>
>> Factor the device-specific data out into a struct oxp_config. This
>> only leaves logic in the corresponding functions and should make
>> adding future devices much easier and less error-prone.
>>
>> Also introduce struct oxp_data which is going to be used in a
>> later commit to cache device state.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>   drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
>>   1 file changed, 215 insertions(+), 302 deletions(-)
>>
> ...
>> +
>>   static const struct dmi_system_id dmi_table[] = {
>>   	{
>>   		.matches = {
>>   			DMI_MATCH(DMI_BOARD_VENDOR, "AOKZOE"),
>>   			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AOKZOE A1 AR07"),
>>   		},
>> -		.driver_data = (void *)aok_zoe_a1,
>> +		.driver_data = (void *)&config_aok_zoe,
> 
> I have not looked at hte rest of the code, but the whole point of
> void * is that a tyoe cast to or from it is not necessary.
> 
> Guenter
I'm also not happy with the cast. But it's either the cast or a warning, 
that the const qualifier is lost.

I'm open to suggestions here. But I don't think that leaving warnings 
around is a good idea.

With best wishes,
Tobias

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Tobias Jakobi]

On 12/26/24 22:04, Guenter Roeck wrote:
> On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@math.uni-bielefeld.de wrote:
>> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>
>> We currently have large switch-statements in all functions that
>> write to EC registers, even though the bulk of the supported
>> devices functions more or less the same.
>>
>> Factor the device-specific data out into a struct oxp_config. This
>> only leaves logic in the corresponding functions and should make
>> adding future devices much easier and less error-prone.
>>
>> Also introduce struct oxp_data which is going to be used in a
>> later commit to cache device state.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>   drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
>>   1 file changed, 215 insertions(+), 302 deletions(-)
>>
>>   
> ...
>> -static int oxp_pwm_disable(void)
>> +static int oxp_pwm_disable(const struct oxp_config *config)
>>   {
>> -	switch (board) {
>> -	case orange_pi_neo:
>> -		return write_to_ec(ORANGEPI_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
>> -	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 write_to_ec(OXP_SENSOR_PWM_ENABLE_REG, PWM_MODE_AUTO);
>> -	default:
>> -		return -EINVAL;
>> -	}
>> +	if (test_bit(OXP_FEATURE_PWM, &config->features))
>> +		return write_to_ec(config->sensor_pwm_enable_reg, PWM_MODE_AUTO);
>> +
> 
> This and all the other feature checks are completely wrong.
> Those checks whould happen once in the is_visible functions,
> and there should not be any such runtime checks. If a feature
> is not available, the associated attributes should not be created
> in the first place.
Hmm, so if the feature checks are wrong _now_, then the board check was 
also wrong to begin with. This is my first time working on hwmon code. 
If we can just drop the checks here (and elsewhere) and just have them 
in the is_visible function -- I'm all for it. Less code is better code.


> If such checks happen in the current code, that should be fixed
> in the is_visible functions. Any existing runtime feature checks
> should be removed.
OK, so to reiterate: We don't want any feature checks during runtime. 
Only during probe time. And during probe we just create the attributes 
that make sense for the device. What we can't decide on the level of 
attributes, we do in the is_visible functions. Is this correct?

With best wishes,
Tobias


> 
> Guenter

Re: [PATCH 4/4] hwmon: (oxp-sensors) Cache state of PWM enable mode

[Tobias Jakobi]

On 12/26/24 22:05, Guenter Roeck wrote:
> On Thu, Dec 26, 2024 at 06:00:19PM +0100, tjakobi@math.uni-bielefeld.de wrote:
>> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>
>> The driver is in full control of the enable mode, so we
>> don't need to read it from HW every single time.
>>
> 
> That is not a reason for adding that much additional code.
> What is the problem that is being solved, and why is it worth that much
> additional code ?
I don't think it's that much additional code, but anyway: Reading from 
EC is not exactly fast, and I want this value cached for another reason. 
It turns out that some devices use a different scaling for the PWM value 
depending on whether the PWM is controlled automatically by the EC, or 
manually through the driver. And I don't want to do an additional EC 
read to figure this out, if I can avoid it.

With best wishes,
Tobias

> 
> Plus, again, all those runtime feature checks in attribute handling
> code are completely wrong.
> 
> Guenter

Re: [PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

[Guenter Roeck]

On Thu, Dec 26, 2024 at 11:56:12PM +0100, Tobias Jakobi wrote:
> 
> On 12/26/24 21:52, Guenter Roeck wrote:
> > On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> > > From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> > > 
> > > Despite what the current comment says, the register is used
> > > both for reading and writing the PWM value.
> > > 
> > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> > > ---
> > >   drivers/hwmon/oxp-sensors.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > > index fbd1463d1494..8089349fa508 100644
> > > --- a/drivers/hwmon/oxp-sensors.c
> > > +++ b/drivers/hwmon/oxp-sensors.c
> > > @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void)
> > >   #define OXP_SENSOR_FAN_REG             0x76 /* Fan reading is 2 registers long */
> > >   #define OXP_2_SENSOR_FAN_REG           0x58 /* Fan reading is 2 registers long */
> > >   #define OXP_SENSOR_PWM_ENABLE_REG      0x4A /* PWM enable is 1 register long */
> > > -#define OXP_SENSOR_PWM_REG             0x4B /* PWM reading is 1 register long */
> > > +#define OXP_SENSOR_PWM_REG             0x4B /* PWM control is 1 register long */
> > 
> > I think that, if anything, this is more confusing than before.
> > "control" is, for example, enabling or disabling pwm management,
> > setting automatic or manual mode, or setting the pwm polarity.
> > Together ith the next two defines, "control" would suggest that
> > PWM_MODE_AUTO and PWM_MODE_MANUAL are set through that register -
> > which is not the case. "value" maybe, but "control" is just wrong.
> Noted. What do you think about "target" then?
> 
> My main point here was that reading implies that this register is read-only.
> Which it clearly isn't. And the documentation (which could be certainly be
> improved in general) should reflect that.
> 

"reading and writing the PWM value". "target" implies writing. "register"
implies neither, so you could use that.

Guenter

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Guenter Roeck]

On Thu, Dec 26, 2024 at 11:59:53PM +0100, Tobias Jakobi wrote:
> On 12/26/24 21:54, Guenter Roeck wrote:
> > On Thu, Dec 26, 2024 at 06:00:16PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> > > From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> > > 
> > > We currently have large switch-statements in all functions that
> > > write to EC registers, even though the bulk of the supported
> > > devices functions more or less the same.
> > > 
> > > Factor the device-specific data out into a struct oxp_config. This
> > > only leaves logic in the corresponding functions and should make
> > > adding future devices much easier and less error-prone.
> > > 
> > > Also introduce struct oxp_data which is going to be used in a
> > > later commit to cache device state.
> > > 
> > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> > > ---
> > >   drivers/hwmon/oxp-sensors.c | 517 +++++++++++++++---------------------
> > >   1 file changed, 215 insertions(+), 302 deletions(-)
> > > 
> > ...
> > > +
> > >   static const struct dmi_system_id dmi_table[] = {
> > >   	{
> > >   		.matches = {
> > >   			DMI_MATCH(DMI_BOARD_VENDOR, "AOKZOE"),
> > >   			DMI_EXACT_MATCH(DMI_BOARD_NAME, "AOKZOE A1 AR07"),
> > >   		},
> > > -		.driver_data = (void *)aok_zoe_a1,
> > > +		.driver_data = (void *)&config_aok_zoe,
> > 
> > I have not looked at hte rest of the code, but the whole point of
> > void * is that a tyoe cast to or from it is not necessary.
> > 
> > Guenter
> I'm also not happy with the cast. But it's either the cast or a warning,
> that the const qualifier is lost.

Your code is introducing that const qualifier.

> 
> I'm open to suggestions here. But I don't think that leaving warnings around
> is a good idea.

Overriding a const qualifier isn't really a good idea either.

You could have used an array such as

static const struct oxp_config config_oxp[] = {
	[aok_zoe_a1] = {
		...
	},
	...
};

If multiple devices, such as aok_zoe_a1 and aya_neo_2, really don't need separate
feature flags, the unnecessary ones are just confusing and should be removed.

Thanks,
Guenter

Re: [PATCH 1/4] hwmon: (oxp-sensors) Separate logic from device-specific data

[Guenter Roeck]

On Fri, Dec 27, 2024 at 12:05:27AM +0100, Tobias Jakobi wrote:
> On 12/26/24 22:04, Guenter Roeck wrote:
> 
...
> 
> > If such checks happen in the current code, that should be fixed
> > in the is_visible functions. Any existing runtime feature checks
> > should be removed.
> OK, so to reiterate: We don't want any feature checks during runtime. Only
> during probe time. And during probe we just create the attributes that make
> sense for the device. What we can't decide on the level of attributes, we do
> in the is_visible functions. Is this correct?
> 
Correct.

Thanks,
Guenter

Re: [PATCH 4/4] hwmon: (oxp-sensors) Cache state of PWM enable mode

[Guenter Roeck]

On Fri, Dec 27, 2024 at 12:13:40AM +0100, Tobias Jakobi wrote:
> On 12/26/24 22:05, Guenter Roeck wrote:
> > On Thu, Dec 26, 2024 at 06:00:19PM +0100, tjakobi@math.uni-bielefeld.de wrote:
> > > From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> > > 
> > > The driver is in full control of the enable mode, so we
> > > don't need to read it from HW every single time.
> > > 
> > 
> > That is not a reason for adding that much additional code.
> > What is the problem that is being solved, and why is it worth that much
> > additional code ?
> I don't think it's that much additional code, but anyway: Reading from EC is
> not exactly fast, and I want this value cached for another reason. It turns
> out that some devices use a different scaling for the PWM value depending on
> whether the PWM is controlled automatically by the EC, or manually through
> the driver. And I don't want to do an additional EC read to figure this out,
> if I can avoid it.

Maybe it isn't that much code after the runtime feature checks are removed.
Either case, since there are now two operations (reading/writing from/to
the EC and caching the result), all associated operations will need to be
mutex protected.

Guenter