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

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

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

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