| Message ID | 20240628151346.1152838-3-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: > The default value of the maximum fan speed limit register is 0, > essentially translating to an unlimited fan speed. When reading > the limit, a value of 0 is reported in this case. However, writing > a value of 0 results in writing a value of 0xffff into the register, > which is inconsistent. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/amc6821.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > index 3c614a0bd192..e37257ae1a6b 100644 > --- a/drivers/hwmon/amc6821.c > +++ b/drivers/hwmon/amc6821.c > @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, > struct amc6821_data *data = amc6821_update_device(dev); > int ix = to_sensor_dev_attr(devattr)->index; > if (0 == data->fan[ix]) > - return sprintf(buf, "0"); > + return sprintf(buf, "6000000"); > return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); > } > > @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, > int ret = kstrtol(buf, 10, &val); > if (ret) > return ret; > - val = 1 > val ? 0xFFFF : 6000000/val; > + val = val < 1 ? 0xFFFF : 6000000 / val; > > mutex_lock(&data->update_lock); > - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); > + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); This is an unrelated change I believe and I would therefore have this in its own commit with proper documentation in the commit log. Indeed: 1- Change in fan_show handles the default 0x0 register value (which can only currently be achieved via the default value of the registers) 2- Allow (re-)setting unlimited fan speed by allowing the user to pass 6000001+ instead of clamping it to 6000000 RPM. Looking good otherwise, Cheers, Quentin
On 7/1/24 04:05, Quentin Schulz wrote: > Hi Guenter, > > On 6/28/24 5:13 PM, Guenter Roeck wrote: >> The default value of the maximum fan speed limit register is 0, >> essentially translating to an unlimited fan speed. When reading >> the limit, a value of 0 is reported in this case. However, writing >> a value of 0 results in writing a value of 0xffff into the register, >> which is inconsistent. >> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/hwmon/amc6821.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >> index 3c614a0bd192..e37257ae1a6b 100644 >> --- a/drivers/hwmon/amc6821.c >> +++ b/drivers/hwmon/amc6821.c >> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, >> struct amc6821_data *data = amc6821_update_device(dev); >> int ix = to_sensor_dev_attr(devattr)->index; >> if (0 == data->fan[ix]) >> - return sprintf(buf, "0"); >> + return sprintf(buf, "6000000"); >> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >> } >> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, >> int ret = kstrtol(buf, 10, &val); >> if (ret) >> return ret; >> - val = 1 > val ? 0xFFFF : 6000000/val; >> + val = val < 1 ? 0xFFFF : 6000000 / val; >> mutex_lock(&data->update_lock); >> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); > > This is an unrelated change I believe and I would therefore have this in its own commit with proper documentation in the commit log. Indeed: > > 1- Change in fan_show handles the default 0x0 register value (which can only currently be achieved via the default value of the registers) > 2- Allow (re-)setting unlimited fan speed by allowing the user to pass 6000001+ instead of clamping it to 6000000 RPM. > Both changes are related. The whole point of this commit is to report and permit consistent values when the register value is 0. But you do have a point - reading it after my changes returns 6000000, but writing the same value sets the register to 1. So I think the proper change would be to display 6000001 as speed if the register value is 0, and provide a more detailed explanation. Would that address your concerns ? Thanks, Guenter
On 7/1/24 07:11, Guenter Roeck wrote: > On 7/1/24 04:05, Quentin Schulz wrote: >> Hi Guenter, >> >> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>> The default value of the maximum fan speed limit register is 0, >>> essentially translating to an unlimited fan speed. When reading >>> the limit, a value of 0 is reported in this case. However, writing >>> a value of 0 results in writing a value of 0xffff into the register, >>> which is inconsistent. >>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> drivers/hwmon/amc6821.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >>> index 3c614a0bd192..e37257ae1a6b 100644 >>> --- a/drivers/hwmon/amc6821.c >>> +++ b/drivers/hwmon/amc6821.c >>> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, >>> struct amc6821_data *data = amc6821_update_device(dev); >>> int ix = to_sensor_dev_attr(devattr)->index; >>> if (0 == data->fan[ix]) >>> - return sprintf(buf, "0"); >>> + return sprintf(buf, "6000000"); >>> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >>> } >>> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, >>> int ret = kstrtol(buf, 10, &val); >>> if (ret) >>> return ret; >>> - val = 1 > val ? 0xFFFF : 6000000/val; >>> + val = val < 1 ? 0xFFFF : 6000000 / val; >>> mutex_lock(&data->update_lock); >>> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >>> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); >> >> This is an unrelated change I believe and I would therefore have this in its own commit with proper documentation in the commit log. Indeed: >> >> 1- Change in fan_show handles the default 0x0 register value (which can only currently be achieved via the default value of the registers) >> 2- Allow (re-)setting unlimited fan speed by allowing the user to pass 6000001+ instead of clamping it to 6000000 RPM. >> > > Both changes are related. > > The whole point of this commit is to report and permit consistent values when > the register value is 0. But you do have a point - reading it after my changes > returns 6000000, but writing the same value sets the register to 1. So I think > the proper change would be to display 6000001 as speed if the register value is > 0, and provide a more detailed explanation. Would that address your concerns ? > Ah, never mind, I'll do it differently: - If the register value is 0, keep reporting 0. - If the value written is 0, write 0, otherwise limit the range to 1..6000000 and write clamp_val(6000000 / val, 1, 0xffff) This minimizes user visibility of the changes, and also ensures that the reported fan speed is 0 if the register value is 0 when reading the fan speed. Guenter
Hi Guenter, On 7/1/24 4:37 PM, Guenter Roeck wrote: > On 7/1/24 07:11, Guenter Roeck wrote: >> On 7/1/24 04:05, Quentin Schulz wrote: >>> Hi Guenter, >>> >>> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>>> The default value of the maximum fan speed limit register is 0, >>>> essentially translating to an unlimited fan speed. When reading >>>> the limit, a value of 0 is reported in this case. However, writing >>>> a value of 0 results in writing a value of 0xffff into the register, >>>> which is inconsistent. >>>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> drivers/hwmon/amc6821.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >>>> index 3c614a0bd192..e37257ae1a6b 100644 >>>> --- a/drivers/hwmon/amc6821.c >>>> +++ b/drivers/hwmon/amc6821.c >>>> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, >>>> struct device_attribute *devattr, >>>> struct amc6821_data *data = amc6821_update_device(dev); >>>> int ix = to_sensor_dev_attr(devattr)->index; >>>> if (0 == data->fan[ix]) >>>> - return sprintf(buf, "0"); >>>> + return sprintf(buf, "6000000"); >>>> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >>>> } >>>> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, >>>> struct device_attribute *attr, >>>> int ret = kstrtol(buf, 10, &val); >>>> if (ret) >>>> return ret; >>>> - val = 1 > val ? 0xFFFF : 6000000/val; >>>> + val = val < 1 ? 0xFFFF : 6000000 / val; >>>> mutex_lock(&data->update_lock); >>>> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >>>> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); >>> >>> This is an unrelated change I believe and I would therefore have this >>> in its own commit with proper documentation in the commit log. Indeed: >>> >>> 1- Change in fan_show handles the default 0x0 register value (which >>> can only currently be achieved via the default value of the registers) >>> 2- Allow (re-)setting unlimited fan speed by allowing the user to >>> pass 6000001+ instead of clamping it to 6000000 RPM. >>> >> >> Both changes are related. >> >> The whole point of this commit is to report and permit consistent >> values when >> the register value is 0. But you do have a point - reading it after my >> changes >> returns 6000000, but writing the same value sets the register to 1. So >> I think >> the proper change would be to display 6000001 as speed if the register >> value is >> 0, and provide a more detailed explanation. Would that address your >> concerns ? >> > > Ah, never mind, I'll do it differently: > > - If the register value is 0, keep reporting 0. Or...... maybe UINT_MAX? > - If the value written is 0, write 0, otherwise limit the range to > 1..6000000 > and write clamp_val(6000000 / val, 1, 0xffff) > Mmmm... I'm a bit worried about the implication of writing 0 in TACH-Low-Limit, what is actually going to happen in that scenario? I assume **every** possible RPM returned by TACH-DATA will be deemed invalid/below the limit then? Reading `Fan Spin-Up` section, if FSPD bit from register 0x20 (which we don't write to yet I think?) is set to 0, a spin-up is started whenever the fan is detected to be running at too low speed. And we would also be getting an interrupt for that too-low event. Basically, wondering if we shouldn't gate the writing of 0 to only the MAX setting? > This minimizes user visibility of the changes, and also ensures that > the reported fan speed is 0 if the register value is 0 when reading the fan > speed. > But didn't you say this means the fan is running at unknown 60 000 000+ RPMs? Do we really want to return 0 even if the fan is actually running? In which case max < current (possibly) but with no event happening (which I would expect, reading the datasheet). Quentin
On 7/1/24 09:13, Quentin Schulz wrote: > Hi Guenter, > > On 7/1/24 4:37 PM, Guenter Roeck wrote: >> On 7/1/24 07:11, Guenter Roeck wrote: >>> On 7/1/24 04:05, Quentin Schulz wrote: >>>> Hi Guenter, >>>> >>>> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>>>> The default value of the maximum fan speed limit register is 0, >>>>> essentially translating to an unlimited fan speed. When reading >>>>> the limit, a value of 0 is reported in this case. However, writing >>>>> a value of 0 results in writing a value of 0xffff into the register, >>>>> which is inconsistent. >>>>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>>> --- >>>>> drivers/hwmon/amc6821.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >>>>> index 3c614a0bd192..e37257ae1a6b 100644 >>>>> --- a/drivers/hwmon/amc6821.c >>>>> +++ b/drivers/hwmon/amc6821.c >>>>> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, >>>>> struct amc6821_data *data = amc6821_update_device(dev); >>>>> int ix = to_sensor_dev_attr(devattr)->index; >>>>> if (0 == data->fan[ix]) >>>>> - return sprintf(buf, "0"); >>>>> + return sprintf(buf, "6000000"); >>>>> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >>>>> } >>>>> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, >>>>> int ret = kstrtol(buf, 10, &val); >>>>> if (ret) >>>>> return ret; >>>>> - val = 1 > val ? 0xFFFF : 6000000/val; >>>>> + val = val < 1 ? 0xFFFF : 6000000 / val; >>>>> mutex_lock(&data->update_lock); >>>>> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >>>>> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); >>>> >>>> This is an unrelated change I believe and I would therefore have this in its own commit with proper documentation in the commit log. Indeed: >>>> >>>> 1- Change in fan_show handles the default 0x0 register value (which can only currently be achieved via the default value of the registers) >>>> 2- Allow (re-)setting unlimited fan speed by allowing the user to pass 6000001+ instead of clamping it to 6000000 RPM. >>>> >>> >>> Both changes are related. >>> >>> The whole point of this commit is to report and permit consistent values when >>> the register value is 0. But you do have a point - reading it after my changes >>> returns 6000000, but writing the same value sets the register to 1. So I think >>> the proper change would be to display 6000001 as speed if the register value is >>> 0, and provide a more detailed explanation. Would that address your concerns ? >>> >> >> Ah, never mind, I'll do it differently: >> >> - If the register value is 0, keep reporting 0. > > Or...... maybe UINT_MAX? > Problem with that is that disconnected fans would report that value as fan speed. Traditionally drivers report a fan speed of 0 in that case. On the other side I agree that reporting "0" as "maximum fan speed" doesn't make much sense either because the real limit _is_ unlimited. But reporting 4294967295 in that case isn't really any better. >> - If the value written is 0, write 0, otherwise limit the range to 1..6000000 >> and write clamp_val(6000000 / val, 1, 0xffff) >> > > Mmmm... I'm a bit worried about the implication of writing 0 in TACH-Low-Limit, what is actually going to happen in that scenario? I assume **every** possible RPM returned by TACH-DATA will be deemed invalid/below the limit then? Reading `Fan Spin-Up` section, if FSPD bit from register 0x20 (which we don't write to yet I think?) is set to 0, a spin-up is started whenever the fan is detected to be running at too low speed. And we would also be getting an interrupt for that too-low event. > > Basically, wondering if we shouldn't gate the writing of 0 to only the MAX setting? > Hmm, good point, and make sense. >> This minimizes user visibility of the changes, and also ensures that >> the reported fan speed is 0 if the register value is 0 when reading the fan >> speed. >> > > But didn't you say this means the fan is running at unknown 60 000 000+ RPMs? Do we really want to return 0 even if the fan is actually running? In which case max < current (possibly) but with no event happening (which I would expect, reading the datasheet). > Did I say that ? If so, I must have meant something different. The register counts the pulse period, so, yes, it would be 0 if rpm is above 6,000,000. But that is really not realistic. In practice I don't know what the controller reports in the register if no fan is connected - that would require real hardware which obviously I don't have. Overall I think I'll stick with the minimum, at least for now: Permit writing 0 into the high limit register only, and otherwise keep the currently permitted ranges. Thanks, Guenter
On 7/1/24 7:21 PM, Guenter Roeck wrote: > On 7/1/24 09:13, Quentin Schulz wrote: >> Hi Guenter, >> >> On 7/1/24 4:37 PM, Guenter Roeck wrote: >>> On 7/1/24 07:11, Guenter Roeck wrote: >>>> On 7/1/24 04:05, Quentin Schulz wrote: >>>>> Hi Guenter, >>>>> >>>>> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>>>>> The default value of the maximum fan speed limit register is 0, >>>>>> essentially translating to an unlimited fan speed. When reading >>>>>> the limit, a value of 0 is reported in this case. However, writing >>>>>> a value of 0 results in writing a value of 0xffff into the register, >>>>>> which is inconsistent. >>>>>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>>>> --- >>>>>> drivers/hwmon/amc6821.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >>>>>> index 3c614a0bd192..e37257ae1a6b 100644 >>>>>> --- a/drivers/hwmon/amc6821.c >>>>>> +++ b/drivers/hwmon/amc6821.c >>>>>> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, >>>>>> struct device_attribute *devattr, >>>>>> struct amc6821_data *data = amc6821_update_device(dev); >>>>>> int ix = to_sensor_dev_attr(devattr)->index; >>>>>> if (0 == data->fan[ix]) >>>>>> - return sprintf(buf, "0"); >>>>>> + return sprintf(buf, "6000000"); >>>>>> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >>>>>> } >>>>>> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, >>>>>> struct device_attribute *attr, >>>>>> int ret = kstrtol(buf, 10, &val); >>>>>> if (ret) >>>>>> return ret; >>>>>> - val = 1 > val ? 0xFFFF : 6000000/val; >>>>>> + val = val < 1 ? 0xFFFF : 6000000 / val; >>>>>> mutex_lock(&data->update_lock); >>>>>> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >>>>>> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); >>>>> >>>>> This is an unrelated change I believe and I would therefore have >>>>> this in its own commit with proper documentation in the commit log. >>>>> Indeed: >>>>> >>>>> 1- Change in fan_show handles the default 0x0 register value (which >>>>> can only currently be achieved via the default value of the registers) >>>>> 2- Allow (re-)setting unlimited fan speed by allowing the user to >>>>> pass 6000001+ instead of clamping it to 6000000 RPM. >>>>> >>>> >>>> Both changes are related. >>>> >>>> The whole point of this commit is to report and permit consistent >>>> values when >>>> the register value is 0. But you do have a point - reading it after >>>> my changes >>>> returns 6000000, but writing the same value sets the register to 1. >>>> So I think >>>> the proper change would be to display 6000001 as speed if the >>>> register value is >>>> 0, and provide a more detailed explanation. Would that address your >>>> concerns ? >>>> >>> >>> Ah, never mind, I'll do it differently: >>> >>> - If the register value is 0, keep reporting 0. >> >> Or...... maybe UINT_MAX? >> > > Problem with that is that disconnected fans would report that value as > fan speed. > Traditionally drivers report a fan speed of 0 in that case. > OK so the issue is that the current fan speed in RPM could be 0 because it's disconnected, or because it exceeds 6M tach pulses. > On the other side I agree that reporting "0" as "maximum fan speed" doesn't > make much sense either because the real limit _is_ unlimited. But reporting > 4294967295 in that case isn't really any better. > Agreed, but I'm also wondering if there really exist fans at 6M+ RPMs? Maybe we're discussing a scenario that just doesn't exist (yet) and that we don't need to handle? [...] >>> This minimizes user visibility of the changes, and also ensures that >>> the reported fan speed is 0 if the register value is 0 when reading >>> the fan >>> speed. >>> >> >> But didn't you say this means the fan is running at unknown 60 000 >> 000+ RPMs? Do we really want to return 0 even if the fan is actually >> running? In which case max < current (possibly) but with no event >> happening (which I would expect, reading the datasheet). >> > > Did I say that ? If so, I must have meant something different. The > register counts the > pulse period, so, yes, it would be 0 if rpm is above 6,000,000. But that > is really not > realistic. In practice I don't know what the controller reports in the > register if no > fan is connected - that would require real hardware which obviously I > don't have. > I'll forage in our shelves tomorrow if I don't forget, trying to find one... if we have one. > Overall I think I'll stick with the minimum, at least for now: Permit > writing 0 > into the high limit register only, and otherwise keep the currently > permitted ranges. > Works for me, we can always revisit later on if needed/desired. Quentin
On 7/1/24 11:05, Quentin Schulz wrote: > On 7/1/24 7:21 PM, Guenter Roeck wrote: >> On 7/1/24 09:13, Quentin Schulz wrote: >>> Hi Guenter, >>> >>> On 7/1/24 4:37 PM, Guenter Roeck wrote: >>>> On 7/1/24 07:11, Guenter Roeck wrote: >>>>> On 7/1/24 04:05, Quentin Schulz wrote: >>>>>> Hi Guenter, >>>>>> >>>>>> On 6/28/24 5:13 PM, Guenter Roeck wrote: >>>>>>> The default value of the maximum fan speed limit register is 0, >>>>>>> essentially translating to an unlimited fan speed. When reading >>>>>>> the limit, a value of 0 is reported in this case. However, writing >>>>>>> a value of 0 results in writing a value of 0xffff into the register, >>>>>>> which is inconsistent. >>>>>>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>>>>> --- >>>>>>> drivers/hwmon/amc6821.c | 6 +++--- >>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >>>>>>> index 3c614a0bd192..e37257ae1a6b 100644 >>>>>>> --- a/drivers/hwmon/amc6821.c >>>>>>> +++ b/drivers/hwmon/amc6821.c >>>>>>> @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, >>>>>>> struct amc6821_data *data = amc6821_update_device(dev); >>>>>>> int ix = to_sensor_dev_attr(devattr)->index; >>>>>>> if (0 == data->fan[ix]) >>>>>>> - return sprintf(buf, "0"); >>>>>>> + return sprintf(buf, "6000000"); >>>>>>> return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); >>>>>>> } >>>>>>> @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, >>>>>>> int ret = kstrtol(buf, 10, &val); >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> - val = 1 > val ? 0xFFFF : 6000000/val; >>>>>>> + val = val < 1 ? 0xFFFF : 6000000 / val; >>>>>>> mutex_lock(&data->update_lock); >>>>>>> - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); >>>>>>> + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); >>>>>> >>>>>> This is an unrelated change I believe and I would therefore have this in its own commit with proper documentation in the commit log. Indeed: >>>>>> >>>>>> 1- Change in fan_show handles the default 0x0 register value (which can only currently be achieved via the default value of the registers) >>>>>> 2- Allow (re-)setting unlimited fan speed by allowing the user to pass 6000001+ instead of clamping it to 6000000 RPM. >>>>>> >>>>> >>>>> Both changes are related. >>>>> >>>>> The whole point of this commit is to report and permit consistent values when >>>>> the register value is 0. But you do have a point - reading it after my changes >>>>> returns 6000000, but writing the same value sets the register to 1. So I think >>>>> the proper change would be to display 6000001 as speed if the register value is >>>>> 0, and provide a more detailed explanation. Would that address your concerns ? >>>>> >>>> >>>> Ah, never mind, I'll do it differently: >>>> >>>> - If the register value is 0, keep reporting 0. >>> >>> Or...... maybe UINT_MAX? >>> >> >> Problem with that is that disconnected fans would report that value as fan speed. >> Traditionally drivers report a fan speed of 0 in that case. >> > > OK so the issue is that the current fan speed in RPM could be 0 because it's disconnected, or because it exceeds 6M tach pulses. > >> On the other side I agree that reporting "0" as "maximum fan speed" doesn't >> make much sense either because the real limit _is_ unlimited. But reporting >> 4294967295 in that case isn't really any better. >> > > Agreed, but I'm also wondering if there really exist fans at 6M+ RPMs? Maybe we're discussing a scenario that just doesn't exist (yet) and that we don't need to handle? > No, such fans don't exist. There are some industrial fans with high rpm, like in the 30k+ RPM range, but it would not be technically possible to build one much faster than that. The fastest (small) fan I could find is https://www.sanyodenki.com/archive/document/product/cooling/catalog_E_pdf/San_Ace_40HVA28_E.pdf which is rated for up to 38,000 rpm, but that is pretty much as fast as it goes. As an exercise, you could try to calculate the edge speed of a fan running at 6M RPM. That would be several times the speed of sound even for a very small fan. > [...] >>>> This minimizes user visibility of the changes, and also ensures that >>>> the reported fan speed is 0 if the register value is 0 when reading the fan >>>> speed. >>>> >>> >>> But didn't you say this means the fan is running at unknown 60 000 000+ RPMs? Do we really want to return 0 even if the fan is actually running? In which case max < current (possibly) but with no event happening (which I would expect, reading the datasheet). >>> >> >> Did I say that ? If so, I must have meant something different. The register counts the >> pulse period, so, yes, it would be 0 if rpm is above 6,000,000. But that is really not >> realistic. In practice I don't know what the controller reports in the register if no >> fan is connected - that would require real hardware which obviously I don't have. >> > > I'll forage in our shelves tomorrow if I don't forget, trying to find one... if we have one. > >> Overall I think I'll stick with the minimum, at least for now: Permit writing 0 >> into the high limit register only, and otherwise keep the currently permitted ranges. >> > > Works for me, we can always revisit later on if needed/desired. > Agreed. Thanks, Guenter
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index 3c614a0bd192..e37257ae1a6b 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -601,7 +601,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr, struct amc6821_data *data = amc6821_update_device(dev); int ix = to_sensor_dev_attr(devattr)->index; if (0 == data->fan[ix]) - return sprintf(buf, "0"); + return sprintf(buf, "6000000"); return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix])); } @@ -625,10 +625,10 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr, int ret = kstrtol(buf, 10, &val); if (ret) return ret; - val = 1 > val ? 0xFFFF : 6000000/val; + val = val < 1 ? 0xFFFF : 6000000 / val; mutex_lock(&data->update_lock); - data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF); + data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF); if (i2c_smbus_write_byte_data(client, fan_reg_low[ix], data->fan[ix] & 0xFF)) { dev_err(&client->dev, "Register write error, aborting.\n");
The default value of the maximum fan speed limit register is 0, essentially translating to an unlimited fan speed. When reading the limit, a value of 0 is reported in this case. However, writing a value of 0 results in writing a value of 0xffff into the register, which is inconsistent. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/amc6821.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)