* [PATCH 1/3] hwmon/adt7470: allow 'fan*_{min,max}' to be reset to '0'
2025-01-05 19:55 [PATCH 0/3] hwmon/adt7470: fix 'fan*_{min,max}', 'temp*_alarm' and add 'temp_fan_norm_alarm' Adrian DC
@ 2025-01-05 19:55 ` Adrian DC
2025-01-06 19:02 ` Guenter Roeck
2025-01-05 19:55 ` [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values read output Adrian DC
2025-01-05 19:55 ` [PATCH 3/3] hwmon/adt7470: create 'temp_fan_norm_alarm' attribute for 'NORM' alarm Adrian DC
2 siblings, 1 reply; 7+ messages in thread
From: Adrian DC @ 2025-01-05 19:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel; +Cc: Adrian DC
Tested with the following script and values
---
{
# Access hwmon
cd /sys/class/hwmon/hwmon1/
# Set to 1 => 82
echo -n ' [TEST] Set to 1 : '
echo '1' >./fan1_max
cat ./fan1_max
# Set to 1234 => 1234
echo -n ' [TEST] Set to 1234 : '
echo '1234' >./fan1_max
cat ./fan1_max
# Reset to 0 => 0
echo -n ' [TEST] Set to 0 : '
echo '0' >./fan1_max
cat ./fan1_max
}
---
Signed-off-by: Adrian DC <radian.dc@gmail.com>
---
drivers/hwmon/adt7470.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index dbee6926fa05..712bc41b4a0d 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -662,11 +662,15 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val
struct adt7470_data *data = dev_get_drvdata(dev);
int err;
- if (val <= 0)
+ if (val < 0)
return -EINVAL;
- val = FAN_RPM_TO_PERIOD(val);
- val = clamp_val(val, 1, 65534);
+ if (val) {
+ val = FAN_RPM_TO_PERIOD(val);
+ val = clamp_val(val, 1, 65534);
+ } else {
+ val = FAN_PERIOD_INVALID;
+ }
switch (attr) {
case hwmon_fan_min:
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] hwmon/adt7470: allow 'fan*_{min,max}' to be reset to '0'
2025-01-05 19:55 ` [PATCH 1/3] hwmon/adt7470: allow 'fan*_{min,max}' to be reset to '0' Adrian DC
@ 2025-01-06 19:02 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2025-01-06 19:02 UTC (permalink / raw)
To: Adrian DC, Jean Delvare, linux-hwmon, linux-kernel
On 1/5/25 11:55, Adrian DC wrote:
> Tested with the following script and values
The patch description is supposed to explain the reason for the changes,
not test results. Test results are useful, but only appropriate after "---".
Guenter
> ---
>
> {
> # Access hwmon
> cd /sys/class/hwmon/hwmon1/
>
> # Set to 1 => 82
> echo -n ' [TEST] Set to 1 : '
> echo '1' >./fan1_max
> cat ./fan1_max
>
> # Set to 1234 => 1234
> echo -n ' [TEST] Set to 1234 : '
> echo '1234' >./fan1_max
> cat ./fan1_max
>
> # Reset to 0 => 0
> echo -n ' [TEST] Set to 0 : '
> echo '0' >./fan1_max
> cat ./fan1_max
> }
> ---
>
> Signed-off-by: Adrian DC <radian.dc@gmail.com>
> ---
> drivers/hwmon/adt7470.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index dbee6926fa05..712bc41b4a0d 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -662,11 +662,15 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val
> struct adt7470_data *data = dev_get_drvdata(dev);
> int err;
>
> - if (val <= 0)
> + if (val < 0)
> return -EINVAL;
>
> - val = FAN_RPM_TO_PERIOD(val);
> - val = clamp_val(val, 1, 65534);
> + if (val) {
> + val = FAN_RPM_TO_PERIOD(val);
> + val = clamp_val(val, 1, 65534);
> + } else {
> + val = FAN_PERIOD_INVALID;
> + }
>
> switch (attr) {
> case hwmon_fan_min:
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values read output
2025-01-05 19:55 [PATCH 0/3] hwmon/adt7470: fix 'fan*_{min,max}', 'temp*_alarm' and add 'temp_fan_norm_alarm' Adrian DC
2025-01-05 19:55 ` [PATCH 1/3] hwmon/adt7470: allow 'fan*_{min,max}' to be reset to '0' Adrian DC
@ 2025-01-05 19:55 ` Adrian DC
2025-01-06 18:59 ` Guenter Roeck
2025-01-05 19:55 ` [PATCH 3/3] hwmon/adt7470: create 'temp_fan_norm_alarm' attribute for 'NORM' alarm Adrian DC
2 siblings, 1 reply; 7+ messages in thread
From: Adrian DC @ 2025-01-05 19:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel; +Cc: Adrian DC
Tested with the following script:
---
{
# Access hwmon
cd /sys/class/hwmon/hwmon1/
# Check alarms
alarms() {
echo -n ' [READ]'
echo -n " fan[1234]_alarm = $(cat ./fan[1234]_alarm | tr '\n' ' ')"
echo -n " temp{[123456789],10}_alarm = $(cat ./temp{[123456789],10}_alarm | tr '\n' ' ')"
echo ''
}
# Configure hardware
echo '0' >./alarm_mask
echo '10' >./num_temp_sensors
# Test fans
for fan in $(seq 1 4); do
echo " [TEST] fan${fan}_min : Min FAN speed ${fan} under limit"
echo '65535' >"./fan${fan}_min"
sleep 4
alarms
echo '0' >"./fan${fan}_min"
sleep 5
done
# Test temperatures
for temp in $(seq 1 10); do
echo " [TEST] temp${temp}_max : Max temperature ${temp} over limit"
echo '-126000' >"./temp${temp}_max"
sleep 5
alarms
echo '127000' >"./temp${temp}_max"
sleep 5
done
# Test clean
echo ' [TEST] Final state'
alarms
}
---
Faulty values:
[TEST] temp1_max : Max temperature 1 over limit
[READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 1 0 1 0 1 0 1
[TEST] temp2_max : Max temperature 2 over limit
[READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
---
Fixed values:
[TEST] fan1_min : Min FAN speed 1 under limit
[READ] fan[1234]_alarm = 1 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
[TEST] fan2_min : Min FAN speed 2 under limit
[READ] fan[1234]_alarm = 0 1 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
[TEST] fan3_min : Min FAN speed 3 under limit
[READ] fan[1234]_alarm = 0 0 1 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
[TEST] fan4_min : Min FAN speed 4 under limit
[READ] fan[1234]_alarm = 0 0 0 1 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
[TEST] temp1_max : Max temperature 1 over limit
[READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 1 0 0 0 0 0 0 0 0 0
[TEST] temp2_max : Max temperature 2 over limit
[READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 0 0 0 0 0 0 0
[TEST] temp3_max : Max temperature 3 over limit
[READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 1 0 0 0 0 0 0 0
...
---
Signed-off-by: Adrian DC <radian.dc@gmail.com>
---
drivers/hwmon/adt7470.c | 52 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 712bc41b4a0d..afb881385dbb 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -551,7 +551,40 @@ static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *va
*val = 1000 * data->temp_max[channel];
break;
case hwmon_temp_alarm:
- *val = !!(data->alarm & channel);
+ switch (channel) {
+ case 0:
+ *val = !!(data->alarm & ADT7470_R1T_ALARM);
+ break;
+ case 1:
+ *val = !!(data->alarm & ADT7470_R2T_ALARM);
+ break;
+ case 2:
+ *val = !!(data->alarm & ADT7470_R3T_ALARM);
+ break;
+ case 3:
+ *val = !!(data->alarm & ADT7470_R4T_ALARM);
+ break;
+ case 4:
+ *val = !!(data->alarm & ADT7470_R5T_ALARM);
+ break;
+ case 5:
+ *val = !!(data->alarm & ADT7470_R6T_ALARM);
+ break;
+ case 6:
+ *val = !!(data->alarm & ADT7470_R7T_ALARM);
+ break;
+ case 7:
+ *val = !!(data->alarm & (ADT7470_R8T_ALARM << 8));
+ break;
+ case 8:
+ *val = !!(data->alarm & (ADT7470_R9T_ALARM << 8));
+ break;
+ case 9:
+ *val = !!(data->alarm & (ADT7470_R10T_ALARM << 8));
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
break;
default:
return -EOPNOTSUPP;
@@ -648,7 +681,22 @@ static int adt7470_fan_read(struct device *dev, u32 attr, int channel, long *val
*val = 0;
break;
case hwmon_fan_alarm:
- *val = !!(data->alarm & (1 << (12 + channel)));
+ switch (channel) {
+ case 0:
+ *val = !!(data->alarm & (ADT7470_FAN1_ALARM << 8));
+ break;
+ case 1:
+ *val = !!(data->alarm & (ADT7470_FAN2_ALARM << 8));
+ break;
+ case 2:
+ *val = !!(data->alarm & (ADT7470_FAN3_ALARM << 8));
+ break;
+ case 3:
+ *val = !!(data->alarm & (ADT7470_FAN4_ALARM << 8));
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
break;
default:
return -EOPNOTSUPP;
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values read output
2025-01-05 19:55 ` [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values read output Adrian DC
@ 2025-01-06 18:59 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2025-01-06 18:59 UTC (permalink / raw)
To: Adrian DC, Jean Delvare, linux-hwmon, linux-kernel
On 1/5/25 11:55, Adrian DC wrote:
> Tested with the following script:
The patch description is supposed to explain what is changed, and why.
Test results are appropriate after "---", but not as patch description.
> ---
>
> {
> # Access hwmon
> cd /sys/class/hwmon/hwmon1/
>
> # Check alarms
> alarms() {
> echo -n ' [READ]'
> echo -n " fan[1234]_alarm = $(cat ./fan[1234]_alarm | tr '\n' ' ')"
> echo -n " temp{[123456789],10}_alarm = $(cat ./temp{[123456789],10}_alarm | tr '\n' ' ')"
> echo ''
> }
>
> # Configure hardware
> echo '0' >./alarm_mask
> echo '10' >./num_temp_sensors
>
> # Test fans
> for fan in $(seq 1 4); do
> echo " [TEST] fan${fan}_min : Min FAN speed ${fan} under limit"
> echo '65535' >"./fan${fan}_min"
> sleep 4
> alarms
> echo '0' >"./fan${fan}_min"
> sleep 5
> done
>
> # Test temperatures
> for temp in $(seq 1 10); do
> echo " [TEST] temp${temp}_max : Max temperature ${temp} over limit"
> echo '-126000' >"./temp${temp}_max"
> sleep 5
> alarms
> echo '127000' >"./temp${temp}_max"
> sleep 5
> done
>
> # Test clean
> echo ' [TEST] Final state'
> alarms
> }
> ---
>
> Faulty values:
> [TEST] temp1_max : Max temperature 1 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 1 0 1 0 1 0 1
> [TEST] temp2_max : Max temperature 2 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> ---
>
> Fixed values:
> [TEST] fan1_min : Min FAN speed 1 under limit
> [READ] fan[1234]_alarm = 1 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan2_min : Min FAN speed 2 under limit
> [READ] fan[1234]_alarm = 0 1 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan3_min : Min FAN speed 3 under limit
> [READ] fan[1234]_alarm = 0 0 1 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan4_min : Min FAN speed 4 under limit
> [READ] fan[1234]_alarm = 0 0 0 1 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] temp1_max : Max temperature 1 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 1 0 0 0 0 0 0 0 0 0
> [TEST] temp2_max : Max temperature 2 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 0 0 0 0 0 0 0
> [TEST] temp3_max : Max temperature 3 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 1 0 0 0 0 0 0 0
> ...
> ---
>
> Signed-off-by: Adrian DC <radian.dc@gmail.com>
> ---
> drivers/hwmon/adt7470.c | 52 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 712bc41b4a0d..afb881385dbb 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -551,7 +551,40 @@ static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *va
> *val = 1000 * data->temp_max[channel];
> break;
> case hwmon_temp_alarm:
> - *val = !!(data->alarm & channel);
> + switch (channel) {
> + case 0:
> + *val = !!(data->alarm & ADT7470_R1T_ALARM);
> + break;
> + case 1:
> + *val = !!(data->alarm & ADT7470_R2T_ALARM);
> + break;
> + case 2:
> + *val = !!(data->alarm & ADT7470_R3T_ALARM);
> + break;
> + case 3:
> + *val = !!(data->alarm & ADT7470_R4T_ALARM);
> + break;
> + case 4:
> + *val = !!(data->alarm & ADT7470_R5T_ALARM);
> + break;
> + case 5:
> + *val = !!(data->alarm & ADT7470_R6T_ALARM);
> + break;
> + case 6:
> + *val = !!(data->alarm & ADT7470_R7T_ALARM);
> + break;
> + case 7:
> + *val = !!(data->alarm & (ADT7470_R8T_ALARM << 8));
> + break;
> + case 8:
> + *val = !!(data->alarm & (ADT7470_R9T_ALARM << 8));
> + break;
> + case 9:
> + *val = !!(data->alarm & (ADT7470_R10T_ALARM << 8));
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
This can be simplified as
if (channel < 7)
*val = !!(data->alarm & BIT(channel));
else
*val = !!(data->alarm & BIT(channel + 1);
> break;
> default:
> return -EOPNOTSUPP;
> @@ -648,7 +681,22 @@ static int adt7470_fan_read(struct device *dev, u32 attr, int channel, long *val
> *val = 0;
> break;
> case hwmon_fan_alarm:
> - *val = !!(data->alarm & (1 << (12 + channel)));
> + switch (channel) {
> + case 0:
> + *val = !!(data->alarm & (ADT7470_FAN1_ALARM << 8));
> + break;
> + case 1:
> + *val = !!(data->alarm & (ADT7470_FAN2_ALARM << 8));
> + break;
> + case 2:
> + *val = !!(data->alarm & (ADT7470_FAN3_ALARM << 8));
> + break;
> + case 3:
> + *val = !!(data->alarm & (ADT7470_FAN4_ALARM << 8));
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
I don't immediately see the problem with the current code, nor do I see a functional
difference. Please explain. Either case, the switch statement is unnecessary.
> break;
> default:
> return -EOPNOTSUPP;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] hwmon/adt7470: create 'temp_fan_norm_alarm' attribute for 'NORM' alarm
2025-01-05 19:55 [PATCH 0/3] hwmon/adt7470: fix 'fan*_{min,max}', 'temp*_alarm' and add 'temp_fan_norm_alarm' Adrian DC
2025-01-05 19:55 ` [PATCH 1/3] hwmon/adt7470: allow 'fan*_{min,max}' to be reset to '0' Adrian DC
2025-01-05 19:55 ` [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values read output Adrian DC
@ 2025-01-05 19:55 ` Adrian DC
2025-01-06 19:10 ` Guenter Roeck
2 siblings, 1 reply; 7+ messages in thread
From: Adrian DC @ 2025-01-05 19:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel; +Cc: Adrian DC
Default: 0 in all normal use cases
Test: Raises to 1 if all FANs are in automatic mode and below threshold
---
Signed-off-by: Adrian DC <radian.dc@gmail.com>
---
drivers/hwmon/adt7470.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index afb881385dbb..1ec4c0807053 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -54,6 +54,7 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
#define ADT7470_R8T_ALARM 0x01
#define ADT7470_R9T_ALARM 0x02
#define ADT7470_R10T_ALARM 0x04
+#define ADT7470_NORM_ALARM 0x08
#define ADT7470_FAN1_ALARM 0x10
#define ADT7470_FAN2_ALARM 0x20
#define ADT7470_FAN3_ALARM 0x40
@@ -533,6 +534,18 @@ static ssize_t num_temp_sensors_store(struct device *dev,
return count;
}
+static ssize_t temp_fan_norm_alarm_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct adt7470_data *data = adt7470_update_device(dev);
+
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ return sprintf(buf, "%d\n", !!(data->alarm & (ADT7470_NORM_ALARM << 8)));
+}
+
static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *val)
{
struct adt7470_data *data = adt7470_update_device(dev);
@@ -1080,6 +1093,7 @@ static ssize_t pwm_auto_temp_store(struct device *dev,
static DEVICE_ATTR_RW(alarm_mask);
static DEVICE_ATTR_RW(num_temp_sensors);
static DEVICE_ATTR_RW(auto_update_interval);
+static DEVICE_ATTR_RO(temp_fan_norm_alarm);
static SENSOR_DEVICE_ATTR_RW(force_pwm_max, force_pwm_max, 0);
@@ -1112,6 +1126,7 @@ static struct attribute *adt7470_attrs[] = {
&dev_attr_alarm_mask.attr,
&dev_attr_num_temp_sensors.attr,
&dev_attr_auto_update_interval.attr,
+ &dev_attr_temp_fan_norm_alarm.attr,
&sensor_dev_attr_force_pwm_max.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] hwmon/adt7470: create 'temp_fan_norm_alarm' attribute for 'NORM' alarm
2025-01-05 19:55 ` [PATCH 3/3] hwmon/adt7470: create 'temp_fan_norm_alarm' attribute for 'NORM' alarm Adrian DC
@ 2025-01-06 19:10 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2025-01-06 19:10 UTC (permalink / raw)
To: Adrian DC, Jean Delvare, linux-hwmon, linux-kernel
On 1/5/25 11:55, Adrian DC wrote:
> Default: 0 in all normal use cases
> Test: Raises to 1 if all FANs are in automatic mode and below threshold
As with the other patches, this is not an acceptable patch description.
On top of that, it introduces a non-standard attribute without documenting
it nor providing a rationale explaining the need for it. Also, this is not
an alarm. The bit says "everything is normal". I fail to see the value of
such an attribute, much less as an "alarm" attribute.
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread