| Message ID | 20240628151346.1152838-5-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (amc6821) Various improvements | expand |
Hi Guenter, On 6/28/24 5:13 PM, Guenter Roeck wrote: > After setting fan1_target and setting pwm1_enable to 4, > the fan controller tries to achieve the requested fan speed. > There's something in the docs (section `Software-RPM Control Mode (Fan Speed Regulator`) that rubs me the wrong way though. """ When the TACH-MODE bit (bit 1 of 0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% when the calculated desired value of duty cycle is less than 30%. Therefore, the TACH setting must be not greater than the value corresponding to the RPM for 30% duty cycle. """ TACH-MODE is never modified in the driver, so its default value prevails: 0. I'm wondering if there isn't something we need to do to make sure we're not under those 30% for TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 if current values are <30% duty cycle, or update them to be >=30%? Otherwise, looking good to me, Cheers, Quentin
Quentin, On 7/1/24 04:23, Quentin Schulz wrote: > Hi Guenter, > > On 6/28/24 5:13 PM, Guenter Roeck wrote: >> After setting fan1_target and setting pwm1_enable to 4, >> the fan controller tries to achieve the requested fan speed. >> > > There's something in the docs (section `Software-RPM Control Mode (Fan Speed Regulator`) that rubs me the wrong way though. > > """ > When the TACH-MODE bit (bit 1 of > 0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% when the calculated desired value of duty > cycle is less than 30%. Therefore, the TACH setting must be not greater than the value corresponding to the > RPM for 30% duty cycle. > """ > It turns out that the tach-mode bit is in reality the DC vs. pwm selector, and defaults to DC. For pwm fans (4-bit fans), the bit should be set to 1. That means that pwm1_mode should be supported to set the mode. I'll add a patch for that. > TACH-MODE is never modified in the driver, so its default value prevails: 0. > > I'm wondering if there isn't something we need to do to make sure we're not under those 30% for TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 if current values are <30% duty cycle, or update them to be >=30%? > It also says that the "the selected target speed must not be too low to operate the fan", which makes sense. It also says that the requested fan speed should not be below the speed translating to 30% duty cycle. However, that is not a fixed value; it depends on the fan. Some fans may operate at 500 rpm with a duty cycle of 30%, others at 3,000 rpm. Looking at Figure 26, I don't think the value written into the pwm register makes any difference in Software-RPM control mode. With that in mind, the only thing we could do is to ensure that the requested fan speed is within the configured low and high limits, or in other words require the user to set the limits before writing the target fan speed. That is a bit circular, though - the user could still write the target speed and _then_ update the limits to a value outside the requested limit. The best we could do would be to sanitize settings when the mode is set to 4 and any of the limits is changed, and return an error if an obviously wrong limit or target speed is requested (target speed outside limit, or low limit >= high limit). Do you think that would be worth the effort ? Thanks, Guenter
Guenter, On 7/1/24 5:26 PM, Guenter Roeck wrote: > Quentin, > > On 7/1/24 04:23, Quentin Schulz wrote: >> Hi Guenter, >> >> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>> After setting fan1_target and setting pwm1_enable to 4, >>> the fan controller tries to achieve the requested fan speed. >>> >> >> There's something in the docs (section `Software-RPM Control Mode (Fan >> Speed Regulator`) that rubs me the wrong way though. >> >> """ >> When the TACH-MODE bit (bit 1 of >> 0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% >> when the calculated desired value of duty >> cycle is less than 30%. Therefore, the TACH setting must be not >> greater than the value corresponding to the >> RPM for 30% duty cycle. >> """ >> > > It turns out that the tach-mode bit is in reality the DC vs. pwm selector, > and defaults to DC. For pwm fans (4-bit fans), the bit should be set to 1. > That means that pwm1_mode should be supported to set the mode. I'll add > a patch > for that. > >> TACH-MODE is never modified in the driver, so its default value >> prevails: 0. >> >> I'm wondering if there isn't something we need to do to make sure >> we're not under those 30% for >> TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write >> (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 >> if current values are <30% duty cycle, or update them to be >=30%? >> > > It also says that the "the selected target speed must not be too low > to operate the fan", which makes sense. It also says that the requested > fan speed should not be below the speed translating to 30% duty cycle. > However, that is not a fixed value; it depends on the fan. Some fans may > operate at 500 rpm with a duty cycle of 30%, others at 3,000 rpm. > Looking at Figure 26, I don't think the value written into the pwm > register makes any difference in Software-RPM control mode. > > With that in mind, the only thing we could do is to ensure that the > requested fan speed is within the configured low and high limits, > or in other words require the user to set the limits before writing > the target fan speed. That is a bit circular, though - the user > could still write the target speed and _then_ update the limits > to a value outside the requested limit. The best we could do would be > to sanitize settings when the mode is set to 4 and any of the limits > is changed, and return an error if an obviously wrong limit or target > speed is requested (target speed outside limit, or low limit >= high > limit). Do you think that would be worth the effort ? > It depends how far we want to go to prevent the user shooting themself in the foot. I think the kernel's stance on that is "let them"? The "benefit" of forcing the user to enter a value in a user-modifiable range is that they wouldn't unknowingly trigger a too-low or too-high logic within the IC. As an example, my bank has a limit on how much I can pay by card per day. However, I can instantly change the value through an app and retry the payment again right after if it's been refused. Would that be something interesting for this speed limit.... who knows. Another thing we could do is modify the min and max values if they are higher and lower than the requested speed. But this is trying to be smart, which I think isn't something the kernel is aiming for (as little logic/algorithm as possible)? So... I guess, the answer is "no, not worth the effort"? Cheers, Quentin
On 7/1/24 09:29, Quentin Schulz wrote: > Guenter, > > On 7/1/24 5:26 PM, Guenter Roeck wrote: >> Quentin, >> >> On 7/1/24 04:23, Quentin Schulz wrote: >>> Hi Guenter, >>> >>> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>>> After setting fan1_target and setting pwm1_enable to 4, >>>> the fan controller tries to achieve the requested fan speed. >>>> >>> >>> There's something in the docs (section `Software-RPM Control Mode (Fan Speed Regulator`) that rubs me the wrong way though. >>> >>> """ >>> When the TACH-MODE bit (bit 1 of >>> 0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% when the calculated desired value of duty >>> cycle is less than 30%. Therefore, the TACH setting must be not greater than the value corresponding to the >>> RPM for 30% duty cycle. >>> """ >>> >> >> It turns out that the tach-mode bit is in reality the DC vs. pwm selector, >> and defaults to DC. For pwm fans (4-bit fans), the bit should be set to 1. >> That means that pwm1_mode should be supported to set the mode. I'll add a patch >> for that. >> >>> TACH-MODE is never modified in the driver, so its default value prevails: 0. >>> >>> I'm wondering if there isn't something we need to do to make sure we're not under those 30% for TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 if current values are <30% duty cycle, or update them to be >=30%? >>> >> >> It also says that the "the selected target speed must not be too low >> to operate the fan", which makes sense. It also says that the requested >> fan speed should not be below the speed translating to 30% duty cycle. >> However, that is not a fixed value; it depends on the fan. Some fans may >> operate at 500 rpm with a duty cycle of 30%, others at 3,000 rpm. >> Looking at Figure 26, I don't think the value written into the pwm >> register makes any difference in Software-RPM control mode. >> >> With that in mind, the only thing we could do is to ensure that the >> requested fan speed is within the configured low and high limits, >> or in other words require the user to set the limits before writing >> the target fan speed. That is a bit circular, though - the user >> could still write the target speed and _then_ update the limits >> to a value outside the requested limit. The best we could do would be >> to sanitize settings when the mode is set to 4 and any of the limits >> is changed, and return an error if an obviously wrong limit or target >> speed is requested (target speed outside limit, or low limit >= high >> limit). Do you think that would be worth the effort ? >> > > It depends how far we want to go to prevent the user shooting themself in the foot. I think the kernel's stance on that is "let them"? > > The "benefit" of forcing the user to enter a value in a user-modifiable range is that they wouldn't unknowingly trigger a too-low or too-high logic within the IC. > > As an example, my bank has a limit on how much I can pay by card per day. However, I can instantly change the value through an app and retry the payment again right after if it's been refused. > > Would that be something interesting for this speed limit.... who knows. > > Another thing we could do is modify the min and max values if they are higher and lower than the requested speed. But this is trying to be smart, which I think isn't something the kernel is aiming for (as little logic/algorithm as possible)? > > So... I guess, the answer is "no, not worth the effort"? > I'll go with "not worth the effort". Thanks a lot for the feedback! Guenter
diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst index 4ce67c268e52..96e604c5ea8e 100644 --- a/Documentation/hwmon/amc6821.rst +++ b/Documentation/hwmon/amc6821.rst @@ -48,12 +48,16 @@ fan1_min rw " fan1_max rw " fan1_fault ro " fan1_pulses rw Pulses per revolution can be either 2 or 4. +fan1_target rw Target fan speed, to be used with pwm1_enable + mode 4. pwm1 rw pwm1 pwm1_enable rw regulator mode, 1=open loop, 2=fan controlled by remote temperature, 3=fan controlled by combination of the on-chip temperature and remote-sensor temperature, + 4=fan controlled by target rpm set with + fan1_target attribute. pwm1_auto_channels_temp ro 1 if pwm_enable==2, 3 if pwm_enable==3 pwm1_auto_point1_pwm ro Hardwired to 0, shared for both temperature channels. diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index 6eb7edd9aca9..8801208430df 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -66,6 +66,8 @@ enum chips { amc6821 }; #define AMC6821_REG_TACH_LLIMITH 0x11 #define AMC6821_REG_TACH_HLIMITL 0x12 #define AMC6821_REG_TACH_HLIMITH 0x13 +#define AMC6821_REG_TACH_SETTINGL 0x1e +#define AMC6821_REG_TACH_SETTINGH 0x1f #define AMC6821_CONF1_START 0x01 #define AMC6821_CONF1_FAN_INT_EN 0x02 @@ -122,17 +124,18 @@ static const u8 temp_reg[] = {AMC6821_REG_LTEMP_HI, AMC6821_REG_RTEMP_LIMIT_MAX, AMC6821_REG_RTEMP_CRIT, }; -enum {IDX_FAN1_INPUT = 0, IDX_FAN1_MIN, IDX_FAN1_MAX, +enum {IDX_FAN1_INPUT = 0, IDX_FAN1_MIN, IDX_FAN1_MAX, IDX_FAN1_TARGET, FAN1_IDX_LEN, }; static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW, AMC6821_REG_TACH_LLIMITL, - AMC6821_REG_TACH_HLIMITL, }; - + AMC6821_REG_TACH_HLIMITL, + AMC6821_REG_TACH_SETTINGL, }; static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI, AMC6821_REG_TACH_LLIMITH, - AMC6821_REG_TACH_HLIMITH, }; + AMC6821_REG_TACH_HLIMITH, + AMC6821_REG_TACH_SETTINGH, }; /* * Client data (each client gets its own) @@ -250,10 +253,10 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) break; case 1: /* * semi-open loop: software sets rpm, chip controls - * pwm1, currently not implemented + * pwm1 */ data->pwm1_auto_channels_temp = 0; - data->pwm1_enable = 0; + data->pwm1_enable = 4; break; } @@ -410,6 +413,10 @@ static ssize_t pwm1_enable_store(struct device *dev, config |= AMC6821_CONF1_FDRC0; config |= AMC6821_CONF1_FDRC1; break; + case 4: + config |= AMC6821_CONF1_FDRC0; + config &= ~AMC6821_CONF1_FDRC1; + break; default: count = -EINVAL; goto unlock; @@ -712,6 +719,7 @@ static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, temp_alarm, IDX_TEMP2_CRIT); static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, IDX_FAN1_INPUT); static SENSOR_DEVICE_ATTR_RW(fan1_min, fan, IDX_FAN1_MIN); static SENSOR_DEVICE_ATTR_RW(fan1_max, fan, IDX_FAN1_MAX); +static SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET); static SENSOR_DEVICE_ATTR_RO(fan1_fault, fan1_fault, 0); static SENSOR_DEVICE_ATTR_RW(fan1_pulses, fan1_pulses, 0); @@ -755,6 +763,7 @@ static struct attribute *amc6821_attrs[] = { &sensor_dev_attr_fan1_input.dev_attr.attr, &sensor_dev_attr_fan1_min.dev_attr.attr, &sensor_dev_attr_fan1_max.dev_attr.attr, + &sensor_dev_attr_fan1_target.dev_attr.attr, &sensor_dev_attr_fan1_fault.dev_attr.attr, &sensor_dev_attr_fan1_pulses.dev_attr.attr, &sensor_dev_attr_pwm1.dev_attr.attr,
After setting fan1_target and setting pwm1_enable to 4, the fan controller tries to achieve the requested fan speed. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Documentation/hwmon/amc6821.rst | 4 ++++ drivers/hwmon/amc6821.c | 21 +++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-)