[PATCH 00/10] hwmon: (amc6821) Various improvements

[PATCH 00/10] hwmon: (amc6821) Various improvements

[Guenter Roeck]

Cleanup and modernize the amc2821 driver.

Changes introducing or modifying functionality:
- Stop accepting invalid pwm values.
- Improve consistency of reading vs. writing fan speed limits
- Rename fan1_div to fan1_pulses
- Add support for fan1_target and a new pwm1_enable mode 4

Non-functional changes:
- Reorder include files, drop unnecessary ones
- Use tabs for column alignment in defines
- Use BIT() and GENMASK()
- Drop unnecessary enum chips
- Convert to use regmap
- Convert to with_info API

The series was tested with module test scripts at
git@github.com:groeck/module-tests.git.

----------------------------------------------------------------
Guenter Roeck (10):
      hwmon: (amc6821) Stop accepting invalid pwm values
      hwmon: (amc6821) Make reading and writing fan speed limits consistent
      hwmon: (amc6821) Rename fan1_div to fan1_pulses
      hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4
      hwmon: (amc2821) Reorder include files, drop unnecessary ones
      hwmon: (amc6821) Use tabs for column alignment in defines
      hwmon: (amc2821) Use BIT() and GENMASK()
      hwmon: (amc6821) Drop unnecessary enum chips
      hwmon: (amc6821) Convert to use regmap
      hwmon: (amc6821) Convert to with_info API

 Documentation/hwmon/amc6821.rst |    6 +-
 drivers/hwmon/Kconfig           |    1 +
 drivers/hwmon/amc6821.c         | 1274 +++++++++++++++++++--------------------
 3 files changed, 637 insertions(+), 644 deletions(-)

[PATCH 01/10] hwmon: (amc6821) Stop accepting invalid pwm values

[Guenter Roeck]

The pwm value range is well defined from 0..255. Don't accept
any values outside this range.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 9b02b304c2f5..3c614a0bd192 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -360,8 +360,11 @@ static ssize_t pwm1_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (val < 0 || val > 255)
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
-	data->pwm1 = clamp_val(val , 0, 255);
+	data->pwm1 = val;
 	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
 	mutex_unlock(&data->update_lock);
 	return count;
@@ -558,13 +561,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
 	int dpwm;
-	long val;
-	int ret = kstrtol(buf, 10, &val);
+	unsigned long val;
+	int ret = kstrtoul(buf, 10, &val);
 	if (ret)
 		return ret;
 
+	if (val > 255)
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
-	data->pwm1_auto_point_pwm[1] = clamp_val(val, 0, 254);
+	data->pwm1_auto_point_pwm[1] = val;
 	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
 			data->pwm1_auto_point_pwm[1])) {
 		dev_err(&client->dev, "Register write error, aborting.\n");
-- 
2.39.2

Re: [PATCH 01/10] hwmon: (amc6821) Stop accepting invalid pwm values

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> The pwm value range is well defined from 0..255. Don't accept
> any values outside this range.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/hwmon/amc6821.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 9b02b304c2f5..3c614a0bd192 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -360,8 +360,11 @@ static ssize_t pwm1_store(struct device *dev,
>   	if (ret)
>   		return ret;
>   
> +	if (val < 0 || val > 255)
> +		return -EINVAL;
> +

Why not use kstrtoul to avoid having to check for negative val? The same 
way that is done just below in this patch?

Additionally, why not using kstrtou8 so we don't have to do this check 
ourselves in the driver?

>   	mutex_lock(&data->update_lock);
> -	data->pwm1 = clamp_val(val , 0, 255);
> +	data->pwm1 = val;
>   	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
>   	mutex_unlock(&data->update_lock);
>   	return count;
> @@ -558,13 +561,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   	struct amc6821_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
>   	int dpwm;
> -	long val;
> -	int ret = kstrtol(buf, 10, &val);
> +	unsigned long val;
> +	int ret = kstrtoul(buf, 10, &val);

Same remark concerning kstrtou8 use.

>   	if (ret)
>   		return ret;
>   
> +	if (val > 255)
> +		return -EINVAL;
> +
>   	mutex_lock(&data->update_lock);
> -	data->pwm1_auto_point_pwm[1] = clamp_val(val, 0, 254);

We're suddenly allowing 255 as a valid value.

I don't see 255 triggering an obvious divide-by-0 issue in the code, nor 
any limitation from a quick look at the datasheet. 254 was introduced in 
the introducing commit, as well as the other 255... so probably an 
oversight by the original author? In any case, I would make this a 
separate commit or at the very least make this explicit in the commit 
log to show this isn't an oversight **right now** and that this change 
was desired.

Cheers,
Quentin

Re: [PATCH 01/10] hwmon: (amc6821) Stop accepting invalid pwm values

[Guenter Roeck]

On 7/1/24 03:19, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 6/28/24 5:13 PM, Guenter Roeck wrote:
>> The pwm value range is well defined from 0..255. Don't accept
>> any values outside this range.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/hwmon/amc6821.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 9b02b304c2f5..3c614a0bd192 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -360,8 +360,11 @@ static ssize_t pwm1_store(struct device *dev,
>>       if (ret)
>>           return ret;
>> +    if (val < 0 || val > 255)
>> +        return -EINVAL;
>> +
> 
> Why not use kstrtoul to avoid having to check for negative val? The same way that is done just below in this patch?
> 
> Additionally, why not using kstrtou8 so we don't have to do this check ourselves in the driver?
> 

Following my desire to minimize changes, but you have a point. I'll use kstrtou8().

>>       mutex_lock(&data->update_lock);
>> -    data->pwm1 = clamp_val(val , 0, 255);
>> +    data->pwm1 = val;
>>       i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
>>       mutex_unlock(&data->update_lock);
>>       return count;
>> @@ -558,13 +561,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>>       int dpwm;
>> -    long val;
>> -    int ret = kstrtol(buf, 10, &val);
>> +    unsigned long val;
>> +    int ret = kstrtoul(buf, 10, &val);
> 
> Same remark concerning kstrtou8 use.
> 
I'll use kstrtou8().

>>       if (ret)
>>           return ret;
>> +    if (val > 255)
>> +        return -EINVAL;
>> +
>>       mutex_lock(&data->update_lock);
>> -    data->pwm1_auto_point_pwm[1] = clamp_val(val, 0, 254);
> 
> We're suddenly allowing 255 as a valid value.
> 
> I don't see 255 triggering an obvious divide-by-0 issue in the code, nor any limitation from a quick look at the datasheet. 254 was introduced in the introducing commit, as well as the other 255... so probably an oversight by the original author? In any case, I would make this a separate commit or at the very least make this explicit in the commit log to show this isn't an oversight **right now** and that this change was desired.
> 

No, this is on purpose. pwm1_auto_point_pwm[2] is set to a constant
255, and pwm1_auto_point_pwm[1] has to be lower than that. As you had
noticed, I fixed this in a later commit, but I should have fixed it
here.

Thanks,
Guenter

[PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

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);
 	if (i2c_smbus_write_byte_data(client, fan_reg_low[ix],
 			data->fan[ix] & 0xFF)) {
 		dev_err(&client->dev, "Register write error, aborting.\n");
-- 
2.39.2

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz]

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

Re: [PATCH 02/10] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

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

[PATCH 03/10] hwmon: (amc6821) Rename fan1_div to fan1_pulses

[Guenter Roeck]

The chip does not have a fan divisor. What it does have is a configuration
to set either 2 or 4 pulses per fan rotation. Rename the attribute to
reflect its use. Update documentation accordingly.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/hwmon/amc6821.rst |  2 +-
 drivers/hwmon/amc6821.c         | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
index 5ddb2849da90..4ce67c268e52 100644
--- a/Documentation/hwmon/amc6821.rst
+++ b/Documentation/hwmon/amc6821.rst
@@ -47,7 +47,7 @@ fan1_input		ro	tachometer speed
 fan1_min		rw	"
 fan1_max		rw	"
 fan1_fault		ro	"
-fan1_div		rw	Fan divisor can be either 2 or 4.
+fan1_pulses		rw	Pulses per revolution can be either 2 or 4.
 
 pwm1			rw	pwm1
 pwm1_enable		rw	regulator mode, 1=open loop, 2=fan controlled
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index e37257ae1a6b..6eb7edd9aca9 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -148,7 +148,7 @@ struct amc6821_data {
 	int temp[TEMP_IDX_LEN];
 
 	u16 fan[FAN1_IDX_LEN];
-	u8 fan1_div;
+	u8 fan1_pulses;
 
 	u8 pwm1;
 	u8 temp1_auto_point_temp[3];
@@ -193,9 +193,9 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
 					client,
 					fan_reg_hi[i]) << 8;
 		}
-		data->fan1_div = i2c_smbus_read_byte_data(client,
+		data->fan1_pulses = i2c_smbus_read_byte_data(client,
 			AMC6821_REG_CONF4);
-		data->fan1_div = data->fan1_div & AMC6821_CONF4_PSPR ? 4 : 2;
+		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
 
 		data->pwm1_auto_point_pwm[0] = 0;
 		data->pwm1_auto_point_pwm[2] = 255;
@@ -645,16 +645,16 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static ssize_t fan1_div_show(struct device *dev,
-			     struct device_attribute *devattr, char *buf)
+static ssize_t fan1_pulses_show(struct device *dev,
+				struct device_attribute *devattr, char *buf)
 {
 	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->fan1_div);
+	return sprintf(buf, "%d\n", data->fan1_pulses);
 }
 
-static ssize_t fan1_div_store(struct device *dev,
-			      struct device_attribute *attr, const char *buf,
-			      size_t count)
+static ssize_t fan1_pulses_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
@@ -674,11 +674,11 @@ static ssize_t fan1_div_store(struct device *dev,
 	switch (val) {
 	case 2:
 		config &= ~AMC6821_CONF4_PSPR;
-		data->fan1_div = 2;
+		data->fan1_pulses = 2;
 		break;
 	case 4:
 		config |= AMC6821_CONF4_PSPR;
-		data->fan1_div = 4;
+		data->fan1_pulses = 4;
 		break;
 	default:
 		count = -EINVAL;
@@ -713,7 +713,7 @@ 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_RO(fan1_fault, fan1_fault, 0);
-static SENSOR_DEVICE_ATTR_RW(fan1_div, fan1_div, 0);
+static SENSOR_DEVICE_ATTR_RW(fan1_pulses, fan1_pulses, 0);
 
 static SENSOR_DEVICE_ATTR_RW(pwm1, pwm1, 0);
 static SENSOR_DEVICE_ATTR_RW(pwm1_enable, pwm1_enable, 0);
@@ -756,7 +756,7 @@ static struct attribute *amc6821_attrs[] = {
 	&sensor_dev_attr_fan1_min.dev_attr.attr,
 	&sensor_dev_attr_fan1_max.dev_attr.attr,
 	&sensor_dev_attr_fan1_fault.dev_attr.attr,
-	&sensor_dev_attr_fan1_div.dev_attr.attr,
+	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
-- 
2.39.2

Re: [PATCH 03/10] hwmon: (amc6821) Rename fan1_div to fan1_pulses

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> The chip does not have a fan divisor. What it does have is a configuration
> to set either 2 or 4 pulses per fan rotation. Rename the attribute to
> reflect its use. Update documentation accordingly.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Good catch!

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks,
Quentin

[PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Guenter Roeck]

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(-)

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,
-- 
2.39.2

Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Quentin Schulz]

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

Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Guenter Roeck]

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

Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Quentin Schulz]

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

Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Guenter Roeck]

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

[PATCH 05/10] hwmon: (amc2821) Reorder include files, drop unnecessary ones

[Guenter Roeck]

Reorder include files to alphabetic order to simplify maintenance,
and drop the unnecessary kernel.h include.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 8801208430df..0ac073f83c19 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,16 +8,15 @@
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
  */
 
-#include <linux/kernel.h>	/* Needed for KERN_INFO */
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/jiffies.h>
-#include <linux/i2c.h>
+#include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/slab.h>
 
 /*
  * Addresses to scan.
-- 
2.39.2

Re: [PATCH 05/10] hwmon: (amc2821) Reorder include files, drop unnecessary ones

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> Reorder include files to alphabetic order to simplify maintenance,
> and drop the unnecessary kernel.h include.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

[PATCH 06/10] hwmon: (amc6821) Use tabs for column alignment in defines

[Guenter Roeck]

Using tabs for column alignment makes the code easier to read.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 128 ++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 0ac073f83c19..03ce2e3ffd86 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -37,77 +37,77 @@ module_param(init, int, 0444);
 
 enum chips { amc6821 };
 
-#define AMC6821_REG_DEV_ID 0x3D
-#define AMC6821_REG_COMP_ID 0x3E
-#define AMC6821_REG_CONF1 0x00
-#define AMC6821_REG_CONF2 0x01
-#define AMC6821_REG_CONF3 0x3F
-#define AMC6821_REG_CONF4 0x04
-#define AMC6821_REG_STAT1 0x02
-#define AMC6821_REG_STAT2 0x03
-#define AMC6821_REG_TDATA_LOW 0x08
-#define AMC6821_REG_TDATA_HI 0x09
-#define AMC6821_REG_LTEMP_HI 0x0A
-#define AMC6821_REG_RTEMP_HI 0x0B
-#define AMC6821_REG_LTEMP_LIMIT_MIN 0x15
-#define AMC6821_REG_LTEMP_LIMIT_MAX 0x14
-#define AMC6821_REG_RTEMP_LIMIT_MIN 0x19
-#define AMC6821_REG_RTEMP_LIMIT_MAX 0x18
-#define AMC6821_REG_LTEMP_CRIT 0x1B
-#define AMC6821_REG_RTEMP_CRIT 0x1D
-#define AMC6821_REG_PSV_TEMP 0x1C
-#define AMC6821_REG_DCY 0x22
-#define AMC6821_REG_LTEMP_FAN_CTRL 0x24
-#define AMC6821_REG_RTEMP_FAN_CTRL 0x25
-#define AMC6821_REG_DCY_LOW_TEMP 0x21
+#define AMC6821_REG_DEV_ID		0x3D
+#define AMC6821_REG_COMP_ID		0x3E
+#define AMC6821_REG_CONF1		0x00
+#define AMC6821_REG_CONF2		0x01
+#define AMC6821_REG_CONF3		0x3F
+#define AMC6821_REG_CONF4		0x04
+#define AMC6821_REG_STAT1		0x02
+#define AMC6821_REG_STAT2		0x03
+#define AMC6821_REG_TDATA_LOW		0x08
+#define AMC6821_REG_TDATA_HI		0x09
+#define AMC6821_REG_LTEMP_HI		0x0A
+#define AMC6821_REG_RTEMP_HI		0x0B
+#define AMC6821_REG_LTEMP_LIMIT_MIN	0x15
+#define AMC6821_REG_LTEMP_LIMIT_MAX	0x14
+#define AMC6821_REG_RTEMP_LIMIT_MIN	0x19
+#define AMC6821_REG_RTEMP_LIMIT_MAX	0x18
+#define AMC6821_REG_LTEMP_CRIT		0x1B
+#define AMC6821_REG_RTEMP_CRIT		0x1D
+#define AMC6821_REG_PSV_TEMP		0x1C
+#define AMC6821_REG_DCY			0x22
+#define AMC6821_REG_LTEMP_FAN_CTRL	0x24
+#define AMC6821_REG_RTEMP_FAN_CTRL	0x25
+#define AMC6821_REG_DCY_LOW_TEMP	0x21
 
-#define AMC6821_REG_TACH_LLIMITL 0x10
-#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_REG_TACH_LLIMITL	0x10
+#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
-#define AMC6821_CONF1_FANIE 0x04
-#define AMC6821_CONF1_PWMINV 0x08
-#define AMC6821_CONF1_FAN_FAULT_EN 0x10
-#define AMC6821_CONF1_FDRC0 0x20
-#define AMC6821_CONF1_FDRC1 0x40
-#define AMC6821_CONF1_THERMOVIE 0x80
+#define AMC6821_CONF1_START		0x01
+#define AMC6821_CONF1_FAN_INT_EN	0x02
+#define AMC6821_CONF1_FANIE		0x04
+#define AMC6821_CONF1_PWMINV		0x08
+#define AMC6821_CONF1_FAN_FAULT_EN	0x10
+#define AMC6821_CONF1_FDRC0		0x20
+#define AMC6821_CONF1_FDRC1		0x40
+#define AMC6821_CONF1_THERMOVIE		0x80
 
-#define AMC6821_CONF2_PWM_EN 0x01
-#define AMC6821_CONF2_TACH_MODE 0x02
-#define AMC6821_CONF2_TACH_EN 0x04
-#define AMC6821_CONF2_RTFIE 0x08
-#define AMC6821_CONF2_LTOIE 0x10
-#define AMC6821_CONF2_RTOIE 0x20
-#define AMC6821_CONF2_PSVIE 0x40
-#define AMC6821_CONF2_RST 0x80
+#define AMC6821_CONF2_PWM_EN		0x01
+#define AMC6821_CONF2_TACH_MODE		0x02
+#define AMC6821_CONF2_TACH_EN		0x04
+#define AMC6821_CONF2_RTFIE		0x08
+#define AMC6821_CONF2_LTOIE		0x10
+#define AMC6821_CONF2_RTOIE		0x20
+#define AMC6821_CONF2_PSVIE		0x40
+#define AMC6821_CONF2_RST		0x80
 
-#define AMC6821_CONF3_THERM_FAN_EN 0x80
-#define AMC6821_CONF3_REV_MASK 0x0F
+#define AMC6821_CONF3_THERM_FAN_EN	0x80
+#define AMC6821_CONF3_REV_MASK		0x0F
 
-#define AMC6821_CONF4_OVREN 0x10
-#define AMC6821_CONF4_TACH_FAST 0x20
-#define AMC6821_CONF4_PSPR 0x40
-#define AMC6821_CONF4_MODE 0x80
+#define AMC6821_CONF4_OVREN		0x10
+#define AMC6821_CONF4_TACH_FAST		0x20
+#define AMC6821_CONF4_PSPR		0x40
+#define AMC6821_CONF4_MODE		0x80
 
-#define AMC6821_STAT1_RPM_ALARM 0x01
-#define AMC6821_STAT1_FANS 0x02
-#define AMC6821_STAT1_RTH 0x04
-#define AMC6821_STAT1_RTL 0x08
-#define AMC6821_STAT1_R_THERM 0x10
-#define AMC6821_STAT1_RTF 0x20
-#define AMC6821_STAT1_LTH 0x40
-#define AMC6821_STAT1_LTL 0x80
+#define AMC6821_STAT1_RPM_ALARM		0x01
+#define AMC6821_STAT1_FANS		0x02
+#define AMC6821_STAT1_RTH		0x04
+#define AMC6821_STAT1_RTL		0x08
+#define AMC6821_STAT1_R_THERM		0x10
+#define AMC6821_STAT1_RTF		0x20
+#define AMC6821_STAT1_LTH		0x40
+#define AMC6821_STAT1_LTL		0x80
 
-#define AMC6821_STAT2_RTC 0x08
-#define AMC6821_STAT2_LTC 0x10
-#define AMC6821_STAT2_LPSV 0x20
-#define AMC6821_STAT2_L_THERM 0x40
-#define AMC6821_STAT2_THERM_IN 0x80
+#define AMC6821_STAT2_RTC		0x08
+#define AMC6821_STAT2_LTC		0x10
+#define AMC6821_STAT2_LPSV		0x20
+#define AMC6821_STAT2_L_THERM		0x40
+#define AMC6821_STAT2_THERM_IN		0x80
 
 enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
 	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-- 
2.39.2

Re: [PATCH 06/10] hwmon: (amc6821) Use tabs for column alignment in defines

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> Using tabs for column alignment makes the code easier to read.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

[PATCH 07/10] hwmon: (amc2821) Use BIT() and GENMASK()

[Guenter Roeck]

Use BIT() and GENMASK() for bit and mask definitions
to help distinguish bit and mask definitions from other
defines and to make the code easier to read.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 03ce2e3ffd86..042e2044de7b 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
  */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -68,46 +69,46 @@ enum chips { amc6821 };
 #define AMC6821_REG_TACH_SETTINGL	0x1e
 #define AMC6821_REG_TACH_SETTINGH	0x1f
 
-#define AMC6821_CONF1_START		0x01
-#define AMC6821_CONF1_FAN_INT_EN	0x02
-#define AMC6821_CONF1_FANIE		0x04
-#define AMC6821_CONF1_PWMINV		0x08
-#define AMC6821_CONF1_FAN_FAULT_EN	0x10
-#define AMC6821_CONF1_FDRC0		0x20
-#define AMC6821_CONF1_FDRC1		0x40
-#define AMC6821_CONF1_THERMOVIE		0x80
+#define AMC6821_CONF1_START		BIT(0)
+#define AMC6821_CONF1_FAN_INT_EN	BIT(1)
+#define AMC6821_CONF1_FANIE		BIT(2)
+#define AMC6821_CONF1_PWMINV		BIT(3)
+#define AMC6821_CONF1_FAN_FAULT_EN	BIT(4)
+#define AMC6821_CONF1_FDRC0		BIT(5)
+#define AMC6821_CONF1_FDRC1		BIT(7)
+#define AMC6821_CONF1_THERMOVIE		BIT(7)
 
-#define AMC6821_CONF2_PWM_EN		0x01
-#define AMC6821_CONF2_TACH_MODE		0x02
-#define AMC6821_CONF2_TACH_EN		0x04
-#define AMC6821_CONF2_RTFIE		0x08
-#define AMC6821_CONF2_LTOIE		0x10
-#define AMC6821_CONF2_RTOIE		0x20
-#define AMC6821_CONF2_PSVIE		0x40
-#define AMC6821_CONF2_RST		0x80
+#define AMC6821_CONF2_PWM_EN		BIT(0)
+#define AMC6821_CONF2_TACH_MODE		BIT(1)
+#define AMC6821_CONF2_TACH_EN		BIT(2)
+#define AMC6821_CONF2_RTFIE		BIT(3)
+#define AMC6821_CONF2_LTOIE		BIT(4)
+#define AMC6821_CONF2_RTOIE		BIT(5)
+#define AMC6821_CONF2_PSVIE		BIT(6)
+#define AMC6821_CONF2_RST		BIT(7)
 
-#define AMC6821_CONF3_THERM_FAN_EN	0x80
-#define AMC6821_CONF3_REV_MASK		0x0F
+#define AMC6821_CONF3_THERM_FAN_EN	BIT(7)
+#define AMC6821_CONF3_REV_MASK		GENMASK(3, 0)
 
-#define AMC6821_CONF4_OVREN		0x10
-#define AMC6821_CONF4_TACH_FAST		0x20
-#define AMC6821_CONF4_PSPR		0x40
-#define AMC6821_CONF4_MODE		0x80
+#define AMC6821_CONF4_OVREN		BIT(4)
+#define AMC6821_CONF4_TACH_FAST		BIT(5)
+#define AMC6821_CONF4_PSPR		BIT(6)
+#define AMC6821_CONF4_MODE		BIT(7)
 
-#define AMC6821_STAT1_RPM_ALARM		0x01
-#define AMC6821_STAT1_FANS		0x02
-#define AMC6821_STAT1_RTH		0x04
-#define AMC6821_STAT1_RTL		0x08
-#define AMC6821_STAT1_R_THERM		0x10
-#define AMC6821_STAT1_RTF		0x20
-#define AMC6821_STAT1_LTH		0x40
-#define AMC6821_STAT1_LTL		0x80
+#define AMC6821_STAT1_RPM_ALARM		BIT(0)
+#define AMC6821_STAT1_FANS		BIT(1)
+#define AMC6821_STAT1_RTH		BIT(2)
+#define AMC6821_STAT1_RTL		BIT(3)
+#define AMC6821_STAT1_R_THERM		BIT(4)
+#define AMC6821_STAT1_RTF		BIT(5)
+#define AMC6821_STAT1_LTH		BIT(6)
+#define AMC6821_STAT1_LTL		BIT(7)
 
-#define AMC6821_STAT2_RTC		0x08
-#define AMC6821_STAT2_LTC		0x10
-#define AMC6821_STAT2_LPSV		0x20
-#define AMC6821_STAT2_L_THERM		0x40
-#define AMC6821_STAT2_THERM_IN		0x80
+#define AMC6821_STAT2_RTC		BIT(3)
+#define AMC6821_STAT2_LTC		BIT(4)
+#define AMC6821_STAT2_LPSV		BIT(5)
+#define AMC6821_STAT2_L_THERM		BIT(6)
+#define AMC6821_STAT2_THERM_IN		BIT(7)
 
 enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
 	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-- 
2.39.2

Re: [PATCH 07/10] hwmon: (amc2821) Use BIT() and GENMASK()

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> Use BIT() and GENMASK() for bit and mask definitions
> to help distinguish bit and mask definitions from other
> defines and to make the code easier to read.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++--------------------
>   1 file changed, 36 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 03ce2e3ffd86..042e2044de7b 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -8,6 +8,7 @@
>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
>    */
>   
> +#include <linux/bits.h>
>   #include <linux/err.h>
>   #include <linux/hwmon.h>
>   #include <linux/hwmon-sysfs.h>
> @@ -68,46 +69,46 @@ enum chips { amc6821 };
>   #define AMC6821_REG_TACH_SETTINGL	0x1e
>   #define AMC6821_REG_TACH_SETTINGH	0x1f
>   
> -#define AMC6821_CONF1_START		0x01
> -#define AMC6821_CONF1_FAN_INT_EN	0x02
> -#define AMC6821_CONF1_FANIE		0x04
> -#define AMC6821_CONF1_PWMINV		0x08
> -#define AMC6821_CONF1_FAN_FAULT_EN	0x10
> -#define AMC6821_CONF1_FDRC0		0x20
> -#define AMC6821_CONF1_FDRC1		0x40
> -#define AMC6821_CONF1_THERMOVIE		0x80
> +#define AMC6821_CONF1_START		BIT(0)
> +#define AMC6821_CONF1_FAN_INT_EN	BIT(1)
> +#define AMC6821_CONF1_FANIE		BIT(2)
> +#define AMC6821_CONF1_PWMINV		BIT(3)
> +#define AMC6821_CONF1_FAN_FAULT_EN	BIT(4)
> +#define AMC6821_CONF1_FDRC0		BIT(5)
> +#define AMC6821_CONF1_FDRC1		BIT(7)

Should be BIT(6) here if I'm not mistaken.

The rest looks fine to me. We could also start to use FIELD_GET, 
FIELD_PREP, ... instead of manually shifting and masking in-code.

Cheers,
Quentin

Re: [PATCH 07/10] hwmon: (amc2821) Use BIT() and GENMASK()

[Guenter Roeck]

On 7/1/24 04:31, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 6/28/24 5:13 PM, Guenter Roeck wrote:
>> Use BIT() and GENMASK() for bit and mask definitions
>> to help distinguish bit and mask definitions from other
>> defines and to make the code easier to read.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++--------------------
>>   1 file changed, 36 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 03ce2e3ffd86..042e2044de7b 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -8,6 +8,7 @@
>>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
>>    */
>> +#include <linux/bits.h>
>>   #include <linux/err.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/hwmon-sysfs.h>
>> @@ -68,46 +69,46 @@ enum chips { amc6821 };
>>   #define AMC6821_REG_TACH_SETTINGL    0x1e
>>   #define AMC6821_REG_TACH_SETTINGH    0x1f
>> -#define AMC6821_CONF1_START        0x01
>> -#define AMC6821_CONF1_FAN_INT_EN    0x02
>> -#define AMC6821_CONF1_FANIE        0x04
>> -#define AMC6821_CONF1_PWMINV        0x08
>> -#define AMC6821_CONF1_FAN_FAULT_EN    0x10
>> -#define AMC6821_CONF1_FDRC0        0x20
>> -#define AMC6821_CONF1_FDRC1        0x40
>> -#define AMC6821_CONF1_THERMOVIE        0x80
>> +#define AMC6821_CONF1_START        BIT(0)
>> +#define AMC6821_CONF1_FAN_INT_EN    BIT(1)
>> +#define AMC6821_CONF1_FANIE        BIT(2)
>> +#define AMC6821_CONF1_PWMINV        BIT(3)
>> +#define AMC6821_CONF1_FAN_FAULT_EN    BIT(4)
>> +#define AMC6821_CONF1_FDRC0        BIT(5)
>> +#define AMC6821_CONF1_FDRC1        BIT(7)
> 
> Should be BIT(6) here if I'm not mistaken.
> 

Yes, I had fixed that later but accidentally applied the fix to later patch.

Guenter

[PATCH 08/10] hwmon: (amc6821) Drop unnecessary enum chips

[Guenter Roeck]

The driver only supports a single chip, so an enum
to determine the chip type is unnecessary. Drop it.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 042e2044de7b..ebffc9393c3d 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -36,8 +36,6 @@ module_param(pwminv, int, 0444);
 static int init = 1; /*Power-on initialization.*/
 module_param(init, int, 0444);
 
-enum chips { amc6821 };
-
 #define AMC6821_REG_DEV_ID		0x3D
 #define AMC6821_REG_COMP_ID		0x3E
 #define AMC6821_REG_CONF1		0x00
@@ -943,7 +941,7 @@ static int amc6821_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id amc6821_id[] = {
-	{ "amc6821", amc6821 },
+	{ "amc6821", 0 },
 	{ }
 };
 
@@ -952,7 +950,6 @@ MODULE_DEVICE_TABLE(i2c, amc6821_id);
 static const struct of_device_id __maybe_unused amc6821_of_match[] = {
 	{
 		.compatible = "ti,amc6821",
-		.data = (void *)amc6821,
 	},
 	{ }
 };
-- 
2.39.2

Re: [PATCH 08/10] hwmon: (amc6821) Drop unnecessary enum chips

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> The driver only supports a single chip, so an enum
> to determine the chip type is unnecessary. Drop it.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/hwmon/amc6821.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 042e2044de7b..ebffc9393c3d 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -36,8 +36,6 @@ module_param(pwminv, int, 0444);
>   static int init = 1; /*Power-on initialization.*/
>   module_param(init, int, 0444);
>   
> -enum chips { amc6821 };
> -
>   #define AMC6821_REG_DEV_ID		0x3D
>   #define AMC6821_REG_COMP_ID		0x3E
>   #define AMC6821_REG_CONF1		0x00
> @@ -943,7 +941,7 @@ static int amc6821_probe(struct i2c_client *client)
>   }
>   
>   static const struct i2c_device_id amc6821_id[] = {
> -	{ "amc6821", amc6821 },
> +	{ "amc6821", 0 },

amc6821_id being a global variable, its content should be initialized by 
the compiler if omitted, and the default value of kernel_ulong_t (aka 
unsigned long) is 0. So I think we could just remove the second 
parameter there.

Doesn't hurt to keep it though and it seems there's a mix of implicit 
and explicit initialization among the i2c kernel drivers, so with that said:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 08/10] hwmon: (amc6821) Drop unnecessary enum chips

[Guenter Roeck]

On 7/1/24 04:36, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 6/28/24 5:13 PM, Guenter Roeck wrote:
>> The driver only supports a single chip, so an enum
>> to determine the chip type is unnecessary. Drop it.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/hwmon/amc6821.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 042e2044de7b..ebffc9393c3d 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -36,8 +36,6 @@ module_param(pwminv, int, 0444);
>>   static int init = 1; /*Power-on initialization.*/
>>   module_param(init, int, 0444);
>> -enum chips { amc6821 };
>> -
>>   #define AMC6821_REG_DEV_ID        0x3D
>>   #define AMC6821_REG_COMP_ID        0x3E
>>   #define AMC6821_REG_CONF1        0x00
>> @@ -943,7 +941,7 @@ static int amc6821_probe(struct i2c_client *client)
>>   }
>>   static const struct i2c_device_id amc6821_id[] = {
>> -    { "amc6821", amc6821 },
>> +    { "amc6821", 0 },
> 
> amc6821_id being a global variable, its content should be initialized by the compiler if omitted, and the default value of kernel_ulong_t (aka unsigned long) is 0. So I think we could just remove the second parameter there.
> 
> Doesn't hurt to keep it though and it seems there's a mix of implicit and explicit initialization among the i2c kernel drivers, so with that said:
> 

I may use that inconsistently myself. I am never sure if I want to say
"keep default" or "explicitly state that the value is 0". I'll keep
it as-is.

> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 

Thanks,
Guenter

[PATCH 09/10] hwmon: (amc6821) Convert to use regmap

[Guenter Roeck]

Use regmap for register accesses and for most caching.

While at it, use sysfs_emit() instead of sprintf() to write sysfs
attribute data.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 711 +++++++++++++++++++---------------------
 1 file changed, 330 insertions(+), 381 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index ebffc9393c3d..6ffab4288134 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -14,9 +14,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/jiffies.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 /*
@@ -44,6 +44,7 @@ module_param(init, int, 0444);
 #define AMC6821_REG_CONF4		0x04
 #define AMC6821_REG_STAT1		0x02
 #define AMC6821_REG_STAT2		0x03
+#define AMC6821_REG_TEMP_LO		0x06
 #define AMC6821_REG_TDATA_LOW		0x08
 #define AMC6821_REG_TDATA_HI		0x09
 #define AMC6821_REG_LTEMP_HI		0x0A
@@ -61,11 +62,8 @@ module_param(init, int, 0444);
 #define AMC6821_REG_DCY_LOW_TEMP	0x21
 
 #define AMC6821_REG_TACH_LLIMITL	0x10
-#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		BIT(0)
 #define AMC6821_CONF1_FAN_INT_EN	BIT(1)
@@ -73,7 +71,7 @@ module_param(init, int, 0444);
 #define AMC6821_CONF1_PWMINV		BIT(3)
 #define AMC6821_CONF1_FAN_FAULT_EN	BIT(4)
 #define AMC6821_CONF1_FDRC0		BIT(5)
-#define AMC6821_CONF1_FDRC1		BIT(7)
+#define AMC6821_CONF1_FDRC1		BIT(6)
 #define AMC6821_CONF1_THERMOVIE		BIT(7)
 
 #define AMC6821_CONF2_PWM_EN		BIT(0)
@@ -130,224 +128,170 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
 			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_SETTINGH, };
-
 /*
  * Client data (each client gets its own)
  */
 
 struct amc6821_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	struct mutex update_lock;
-	bool valid; /* false until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
 
-	/* register values */
-	int temp[TEMP_IDX_LEN];
-
-	u16 fan[FAN1_IDX_LEN];
-	u8 fan1_pulses;
-
-	u8 pwm1;
 	u8 temp1_auto_point_temp[3];
 	u8 temp2_auto_point_temp[3];
-	u8 pwm1_auto_point_pwm[3];
-	u8 pwm1_enable;
-	u8 pwm1_auto_channels_temp;
-
-	u8 stat1;
-	u8 stat2;
 };
 
-static struct amc6821_data *amc6821_update_device(struct device *dev)
+static int amc6821_init_auto_point_data(struct amc6821_data *data)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int timeout = HZ;
-	u8 reg;
-	int i;
+	struct regmap *regmap = data->regmap;
+	u32 pwm, regval;
+	int err;
 
-	mutex_lock(&data->update_lock);
+	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+	if (err)
+		return err;
 
-	if (time_after(jiffies, data->last_updated + timeout) ||
-			!data->valid) {
+	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
+	if (err)
+		return err;
+	data->temp1_auto_point_temp[0] = regval;
+	data->temp2_auto_point_temp[0] = data->temp1_auto_point_temp[0];
 
-		for (i = 0; i < TEMP_IDX_LEN; i++)
-			data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
-				client, temp_reg[i]);
+	err = regmap_read(regmap, AMC6821_REG_LTEMP_FAN_CTRL, &regval);
+	if (err)
+		return err;
+	data->temp1_auto_point_temp[1] = (regval & 0xF8) >> 1;
 
-		data->stat1 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_STAT1);
-		data->stat2 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_STAT2);
+	regval &= 0x07;
+	regval = 0x20 >> regval;
+	if (regval)
+		data->temp1_auto_point_temp[2] =
+			data->temp1_auto_point_temp[1] +
+			(255 - pwm) / regval;
+	else
+		data->temp1_auto_point_temp[2] = 255;
 
-		data->pwm1 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_DCY);
-		for (i = 0; i < FAN1_IDX_LEN; i++) {
-			data->fan[i] = i2c_smbus_read_byte_data(
-					client,
-					fan_reg_low[i]);
-			data->fan[i] += i2c_smbus_read_byte_data(
-					client,
-					fan_reg_hi[i]) << 8;
-		}
-		data->fan1_pulses = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_CONF4);
-		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
+	err = regmap_read(regmap, AMC6821_REG_RTEMP_FAN_CTRL, &regval);
+	if (err)
+		return err;
 
-		data->pwm1_auto_point_pwm[0] = 0;
-		data->pwm1_auto_point_pwm[2] = 255;
-		data->pwm1_auto_point_pwm[1] = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_DCY_LOW_TEMP);
+	data->temp2_auto_point_temp[1] = (regval & 0xF8) >> 1;
+	regval &= 0x07;
+	regval = 0x20 >> regval;
 
-		data->temp1_auto_point_temp[0] =
-			i2c_smbus_read_byte_data(client,
-					AMC6821_REG_PSV_TEMP);
-		data->temp2_auto_point_temp[0] =
-				data->temp1_auto_point_temp[0];
-		reg = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_LTEMP_FAN_CTRL);
-		data->temp1_auto_point_temp[1] = (reg & 0xF8) >> 1;
-		reg &= 0x07;
-		reg = 0x20 >> reg;
-		if (reg > 0)
-			data->temp1_auto_point_temp[2] =
-				data->temp1_auto_point_temp[1] +
-				(data->pwm1_auto_point_pwm[2] -
-				data->pwm1_auto_point_pwm[1]) / reg;
-		else
-			data->temp1_auto_point_temp[2] = 255;
+	if (regval)
+		data->temp2_auto_point_temp[2] =
+			data->temp2_auto_point_temp[1] +
+			(255 - pwm) / regval;
+	else
+		data->temp2_auto_point_temp[2] = 255;
 
-		reg = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_RTEMP_FAN_CTRL);
-		data->temp2_auto_point_temp[1] = (reg & 0xF8) >> 1;
-		reg &= 0x07;
-		reg = 0x20 >> reg;
-		if (reg > 0)
-			data->temp2_auto_point_temp[2] =
-				data->temp2_auto_point_temp[1] +
-				(data->pwm1_auto_point_pwm[2] -
-				data->pwm1_auto_point_pwm[1]) / reg;
-		else
-			data->temp2_auto_point_temp[2] = 255;
-
-		reg = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-		reg = (reg >> 5) & 0x3;
-		switch (reg) {
-		case 0: /*open loop: software sets pwm1*/
-			data->pwm1_auto_channels_temp = 0;
-			data->pwm1_enable = 1;
-			break;
-		case 2: /*closed loop: remote T (temp2)*/
-			data->pwm1_auto_channels_temp = 2;
-			data->pwm1_enable = 2;
-			break;
-		case 3: /*closed loop: local and remote T (temp2)*/
-			data->pwm1_auto_channels_temp = 3;
-			data->pwm1_enable = 3;
-			break;
-		case 1: /*
-			 * semi-open loop: software sets rpm, chip controls
-			 * pwm1
-			 */
-			data->pwm1_auto_channels_temp = 0;
-			data->pwm1_enable = 4;
-			break;
-		}
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	mutex_unlock(&data->update_lock);
-	return data;
+	return 0;
 }
 
 static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
+	u32 regval;
+	int err;
 
-	return sprintf(buf, "%d\n", data->temp[ix] * 1000);
+	err = regmap_read(data->regmap, temp_reg[ix], &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
 }
 
 static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int ix = to_sensor_dev_attr(attr)->index;
 	long val;
+	int err;
 
 	int ret = kstrtol(buf, 10, &val);
 	if (ret)
 		return ret;
 	val = clamp_val(val / 1000, -128, 127);
 
-	mutex_lock(&data->update_lock);
-	data->temp[ix] = val;
-	if (i2c_smbus_write_byte_data(client, temp_reg[ix], data->temp[ix])) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-	}
-	mutex_unlock(&data->update_lock);
+	err = regmap_write(data->regmap, temp_reg[ix], val);
+	if (err)
+		return err;
+
 	return count;
 }
 
 static ssize_t temp_alarm_show(struct device *dev,
 			       struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	u8 flag;
+	u32 regval, mask, reg;
+	int err;
 
 	switch (ix) {
 	case IDX_TEMP1_MIN:
-		flag = data->stat1 & AMC6821_STAT1_LTL;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_LTL;
 		break;
 	case IDX_TEMP1_MAX:
-		flag = data->stat1 & AMC6821_STAT1_LTH;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_LTH;
 		break;
 	case IDX_TEMP1_CRIT:
-		flag = data->stat2 & AMC6821_STAT2_LTC;
+		reg = AMC6821_REG_STAT2;
+		mask = AMC6821_STAT2_LTC;
 		break;
 	case IDX_TEMP2_MIN:
-		flag = data->stat1 & AMC6821_STAT1_RTL;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_RTL;
 		break;
 	case IDX_TEMP2_MAX:
-		flag = data->stat1 & AMC6821_STAT1_RTH;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_RTH;
 		break;
 	case IDX_TEMP2_CRIT:
-		flag = data->stat2 & AMC6821_STAT2_RTC;
+		reg = AMC6821_REG_STAT2;
+		mask = AMC6821_STAT2_RTC;
 		break;
 	default:
 		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
 		return -EINVAL;
 	}
-	if (flag)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	err = regmap_read(data->regmap, reg, &regval);
+	if (err)
+		return err;
+	return sysfs_emit(buf, "%d\n", !!(regval & mask));
 }
 
 static ssize_t temp2_fault_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	if (data->stat1 & AMC6821_STAT1_RTF)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
 }
 
 static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", regval);
 }
 
 static ssize_t pwm1_store(struct device *dev,
@@ -355,7 +299,6 @@ static ssize_t pwm1_store(struct device *dev,
 			  size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	long val;
 	int ret = kstrtol(buf, 10, &val);
 	if (ret)
@@ -364,18 +307,38 @@ static ssize_t pwm1_store(struct device *dev,
 	if (val < 0 || val > 255)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->pwm1 = val;
-	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
+	if (ret)
+		return ret;
+
 	return count;
 }
 
 static ssize_t pwm1_enable_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_enable);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	int err;
+	u32 val;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+	if (err)
+		return err;
+	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+	case 0:
+		val = 1;	/* manual */
+		break;
+	case AMC6821_CONF1_FDRC0:
+		val = 4;	/* target rpm (fan1_target) controlled */
+		break;
+	case AMC6821_CONF1_FDRC1:
+		val = 2;	/* remote temp controlled */
+		break;
+	default:
+		val = 3;	/* max(local, remote) temp controlled */
+		break;
+	}
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t pwm1_enable_store(struct device *dev,
@@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
 				 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	long val;
-	int config = kstrtol(buf, 10, &val);
-	if (config)
-		return config;
+	u32 mask;
+	int err;
 
-	mutex_lock(&data->update_lock);
-	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-	if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-			count = config;
-			goto unlock;
-	}
+	err = kstrtol(buf, 10, &val);
+	if (err)
+		return err;
 
 	switch (val) {
 	case 1:
-		config &= ~AMC6821_CONF1_FDRC0;
-		config &= ~AMC6821_CONF1_FDRC1;
+		mask = 0;
 		break;
 	case 2:
-		config &= ~AMC6821_CONF1_FDRC0;
-		config |= AMC6821_CONF1_FDRC1;
+		mask = AMC6821_CONF1_FDRC1;
 		break;
 	case 3:
-		config |= AMC6821_CONF1_FDRC0;
-		config |= AMC6821_CONF1_FDRC1;
+		mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
 		break;
 	case 4:
-		config |= AMC6821_CONF1_FDRC0;
-		config &= ~AMC6821_CONF1_FDRC1;
+		mask = AMC6821_CONF1_FDRC0;
 		break;
 	default:
-		count = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF1, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			count = -EIO;
-	}
-unlock:
-	mutex_unlock(&data->update_lock);
+
+	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
+				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
+				 mask);
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
@@ -433,8 +384,27 @@ static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
 					    struct device_attribute *devattr,
 					    char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_auto_channels_temp);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 val;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+	if (err)
+		return err;
+	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+	case 0:
+	case AMC6821_CONF1_FDRC0:
+		val = 0;	/* manual or target rpm controlled */
+		break;
+	case AMC6821_CONF1_FDRC1:
+		val = 2;	/* remote temp controlled */
+		break;
+	default:
+		val = 3;	/* max(local, remote) temp controlled */
+		break;
+	}
+
+	return sprintf(buf, "%d\n", val);
 }
 
 static ssize_t temp_auto_point_temp_show(struct device *dev,
@@ -443,7 +413,8 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
 {
 	int ix = to_sensor_dev_attr_2(devattr)->index;
 	int nr = to_sensor_dev_attr_2(devattr)->nr;
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+
 	switch (nr) {
 	case 1:
 		return sprintf(buf, "%d\n",
@@ -461,44 +432,59 @@ static ssize_t pwm1_auto_point_pwm_show(struct device *dev,
 					struct device_attribute *devattr,
 					char *buf)
 {
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_auto_point_pwm[ix]);
+	u32 val;
+	int err;
+
+	switch (ix) {
+	case 0:
+		val = 0;
+		break;
+	case 1:
+		err = regmap_read(data->regmap, AMC6821_REG_DCY_LOW_TEMP, &val);
+		if (err)
+			return err;
+		break;
+	default:
+		val = 255;
+		break;
+	}
+	return sysfs_emit(buf, "%d\n", val);
 }
 
-static inline ssize_t set_slope_register(struct i2c_client *client,
-		u8 reg,
-		u8 dpwm,
-		u8 *ptemp)
+static inline int set_slope_register(struct regmap *regmap,
+				     u8 reg, u8 *ptemp)
 {
-	int dt;
-	u8 tmp;
+	u8 tmp, dpwm;
+	int err, dt;
+	u32 pwm;
 
-	dt = ptemp[2]-ptemp[1];
+	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+	if (err)
+		return err;
+
+	dpwm = 255 - pwm;
+
+	dt = ptemp[2] - ptemp[1];
 	for (tmp = 4; tmp > 0; tmp--) {
 		if (dt * (0x20 >> tmp) >= dpwm)
 			break;
 	}
 	tmp |= (ptemp[1] & 0x7C) << 1;
-	if (i2c_smbus_write_byte_data(client,
-			reg, tmp)) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		return -EIO;
-	}
-	return 0;
+	return regmap_write(regmap, reg, tmp);
 }
 
 static ssize_t temp_auto_point_temp_store(struct device *dev,
 					  struct device_attribute *attr,
 					  const char *buf, size_t count)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	struct i2c_client *client = data->client;
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 	int ix = to_sensor_dev_attr_2(attr)->index;
 	int nr = to_sensor_dev_attr_2(attr)->nr;
 	u8 *ptemp;
 	u8 reg;
-	int dpwm;
 	long val;
 	int ret = kstrtol(buf, 10, &val);
 	if (ret)
@@ -519,7 +505,6 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 	}
 
 	mutex_lock(&data->update_lock);
-	data->valid = false;
 
 	switch (ix) {
 	case 0:
@@ -528,13 +513,9 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 		ptemp[0] = clamp_val(ptemp[0], 0,
 				     data->temp2_auto_point_temp[1]);
 		ptemp[0] = clamp_val(ptemp[0], 0, 63);
-		if (i2c_smbus_write_byte_data(
-					client,
-					AMC6821_REG_PSV_TEMP,
-					ptemp[0])) {
-				dev_err(&client->dev,
-					"Register write error, aborting.\n");
-				count = -EIO;
+		if (regmap_write(regmap, AMC6821_REG_PSV_TEMP, ptemp[0])) {
+			dev_err(dev, "Register write error, aborting.\n");
+			count = -EIO;
 		}
 		goto EXIT;
 	case 1:
@@ -550,8 +531,7 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 		count = -EINVAL;
 		goto EXIT;
 	}
-	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
-	if (set_slope_register(client, reg, dpwm, ptemp))
+	if (set_slope_register(regmap, reg, ptemp))
 		count = -EIO;
 
 EXIT:
@@ -564,38 +544,37 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 					 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int dpwm;
-	unsigned long val;
-	int ret = kstrtoul(buf, 10, &val);
+	struct regmap *regmap = data->regmap;
+	long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
 	if (ret)
 		return ret;
 
-	if (val > 255)
+	if (val > 254)
 		return -EINVAL;
 
 	mutex_lock(&data->update_lock);
-	data->pwm1_auto_point_pwm[1] = val;
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
-			data->pwm1_auto_point_pwm[1])) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-		goto EXIT;
+
+	ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
+	if (ret)
+		goto unlock;
+
+	ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
+				 data->temp1_auto_point_temp);
+	if (ret) {
+		count = ret;
+		goto unlock;
 	}
-	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
-	if (set_slope_register(client, AMC6821_REG_LTEMP_FAN_CTRL, dpwm,
-			data->temp1_auto_point_temp)) {
-		count = -EIO;
-		goto EXIT;
-	}
-	if (set_slope_register(client, AMC6821_REG_RTEMP_FAN_CTRL, dpwm,
-			data->temp2_auto_point_temp)) {
-		count = -EIO;
-		goto EXIT;
+	ret = set_slope_register(regmap, AMC6821_REG_RTEMP_FAN_CTRL,
+				 data->temp2_auto_point_temp);
+	if (ret) {
+		count = ret;
+		goto unlock;
 	}
 
-EXIT:
-	data->valid = false;
+unlock:
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
 			char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	if (0 == data->fan[ix])
-		return sprintf(buf, "6000000");
-	return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
+	u32 regval;
+	u8 regs[2];
+	int err;
+
+	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
+	if (err)
+		return err;
+	regval = (regs[1] << 8) | regs[0];
+
+	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
 }
 
 static ssize_t fan1_fault_show(struct device *dev,
 			       struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	if (data->stat1 & AMC6821_STAT1_FANS)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
 }
 
 static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	long val;
 	int ix = to_sensor_dev_attr(attr)->index;
-	int ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-	val = val < 1 ? 0xFFFF : 6000000 / val;
+	u8 regs[2];
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err)
+		return err;
+
+	val = val < 1 ? 0xFFFF : 6000000 / val;
+	val = clamp_val(val, 0, 0xFFFF);
+
+	regs[0] = val & 0xff;
+	regs[1] = val >> 8;
+
+	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
+	if (err)
+		return err;
 
-	mutex_lock(&data->update_lock);
-	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");
-		count = -EIO;
-		goto EXIT;
-	}
-	if (i2c_smbus_write_byte_data(client,
-			fan_reg_hi[ix], data->fan[ix] >> 8)) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-	}
-EXIT:
-	mutex_unlock(&data->update_lock);
 	return count;
 }
 
 static ssize_t fan1_pulses_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->fan1_pulses);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
 }
 
 static ssize_t fan1_pulses_store(struct device *dev,
@@ -662,40 +655,22 @@ static ssize_t fan1_pulses_store(struct device *dev,
 				 size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	long val;
-	int config = kstrtol(buf, 10, &val);
-	if (config)
-		return config;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err)
+		return err;
+
+	if (val != 2 && val != 4)
+		return -EINVAL;
+
+	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
+				 AMC6821_CONF4_PSPR,
+				 val == 4 ? AMC6821_CONF4_PSPR : 0);
+	if (err)
+		return err;
 
-	mutex_lock(&data->update_lock);
-	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
-	if (config < 0) {
-		dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-		count = config;
-		goto EXIT;
-	}
-	switch (val) {
-	case 2:
-		config &= ~AMC6821_CONF4_PSPR;
-		data->fan1_pulses = 2;
-		break;
-	case 4:
-		config |= AMC6821_CONF4_PSPR;
-		data->fan1_pulses = 4;
-		break;
-	default:
-		count = -EINVAL;
-		goto EXIT;
-	}
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4, config)) {
-		dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-		count = -EIO;
-	}
-EXIT:
-	mutex_unlock(&data->update_lock);
 	return count;
 }
 
@@ -827,110 +802,84 @@ static int amc6821_detect(
 	return 0;
 }
 
-static int amc6821_init_client(struct i2c_client *client)
+static int amc6821_init_client(struct amc6821_data *data)
 {
-	int config;
-	int err = -EIO;
+	struct regmap *regmap = data->regmap;
+	int err;
+
+	err = amc6821_init_auto_point_data(data);
+	if (err)
+		return err;
 
 	if (init) {
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
-
-		if (config < 0) {
-				dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-				return err;
-		}
-
-		config |= AMC6821_CONF4_MODE;
-
-		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4,
-				config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
+		err = regmap_set_bits(regmap, AMC6821_REG_CONF4, AMC6821_CONF4_MODE);
+		if (err)
 			return err;
-		}
-
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF3);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
+		err = regmap_clear_bits(regmap, AMC6821_REG_CONF3, AMC6821_CONF3_THERM_FAN_EN);
+		if (err)
 			return err;
-		}
-
-		dev_info(&client->dev, "Revision %d\n", config & 0x0f);
-
-		config &= ~AMC6821_CONF3_THERM_FAN_EN;
-
-		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF3,
-				config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
+		err = regmap_clear_bits(regmap, AMC6821_REG_CONF2,
+					AMC6821_CONF2_RTFIE |
+					AMC6821_CONF2_LTOIE |
+					AMC6821_CONF2_RTOIE);
+		if (err)
 			return err;
-		}
 
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF2);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
+		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
+					 AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
+					 AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
+					 AMC6821_CONF1_START |
+					 (pwminv ? AMC6821_CONF1_PWMINV : 0));
+		if (err)
 			return err;
-		}
-
-		config &= ~AMC6821_CONF2_RTFIE;
-		config &= ~AMC6821_CONF2_LTOIE;
-		config &= ~AMC6821_CONF2_RTOIE;
-		if (i2c_smbus_write_byte_data(client,
-				AMC6821_REG_CONF2, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			return err;
-		}
-
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-			return err;
-		}
-
-		config &= ~AMC6821_CONF1_THERMOVIE;
-		config &= ~AMC6821_CONF1_FANIE;
-		config |= AMC6821_CONF1_START;
-		if (pwminv)
-			config |= AMC6821_CONF1_PWMINV;
-		else
-			config &= ~AMC6821_CONF1_PWMINV;
-
-		if (i2c_smbus_write_byte_data(
-				client, AMC6821_REG_CONF1, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			return err;
-		}
 	}
 	return 0;
 }
 
+static bool amc6821_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case AMC6821_REG_STAT1:
+	case AMC6821_REG_STAT2:
+	case AMC6821_REG_TEMP_LO:
+	case AMC6821_REG_TDATA_LOW:
+	case AMC6821_REG_LTEMP_HI:
+	case AMC6821_REG_RTEMP_HI:
+	case AMC6821_REG_TDATA_HI:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config amc6821_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AMC6821_REG_CONF3,
+	.volatile_reg = amc6821_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
 static int amc6821_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct amc6821_data *data;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
 	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
-	mutex_init(&data->update_lock);
 
-	/*
-	 * Initialize the amc6821 chip
-	 */
-	err = amc6821_init_client(client);
+	regmap = devm_regmap_init_i2c(client, &amc6821_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize regmap\n");
+	data->regmap = regmap;
+
+	err = amc6821_init_client(data);
 	if (err)
 		return err;
 
-- 
2.39.2

Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> Use regmap for register accesses and for most caching.
> 
> While at it, use sysfs_emit() instead of sprintf() to write sysfs
> attribute data.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/hwmon/amc6821.c | 711 +++++++++++++++++++---------------------
>   1 file changed, 330 insertions(+), 381 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index ebffc9393c3d..6ffab4288134 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -14,9 +14,9 @@
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/i2c.h>
>   #include <linux/init.h>
> -#include <linux/jiffies.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/regmap.h>
>   #include <linux/slab.h>
>   
>   /*
> @@ -44,6 +44,7 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_CONF4		0x04
>   #define AMC6821_REG_STAT1		0x02
>   #define AMC6821_REG_STAT2		0x03
> +#define AMC6821_REG_TEMP_LO		0x06
>   #define AMC6821_REG_TDATA_LOW		0x08
>   #define AMC6821_REG_TDATA_HI		0x09
>   #define AMC6821_REG_LTEMP_HI		0x0A
> @@ -61,11 +62,8 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_DCY_LOW_TEMP	0x21
>   
>   #define AMC6821_REG_TACH_LLIMITL	0x10
> -#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		BIT(0)
>   #define AMC6821_CONF1_FAN_INT_EN	BIT(1)
> @@ -73,7 +71,7 @@ module_param(init, int, 0444);
>   #define AMC6821_CONF1_PWMINV		BIT(3)
>   #define AMC6821_CONF1_FAN_FAULT_EN	BIT(4)
>   #define AMC6821_CONF1_FDRC0		BIT(5)
> -#define AMC6821_CONF1_FDRC1		BIT(7)
> +#define AMC6821_CONF1_FDRC1		BIT(6)

This should have been squashed with a previous commit.

>   #define AMC6821_CONF1_THERMOVIE		BIT(7)
>   
>   #define AMC6821_CONF2_PWM_EN		BIT(0)
> @@ -130,224 +128,170 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
>   			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_SETTINGH, };
> -
>   /*
>    * Client data (each client gets its own)
>    */
>   
>   struct amc6821_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>   	struct mutex update_lock;
> -	bool valid; /* false until following fields are valid */
> -	unsigned long last_updated; /* in jiffies */
>   
> -	/* register values */
> -	int temp[TEMP_IDX_LEN];
> -
> -	u16 fan[FAN1_IDX_LEN];
> -	u8 fan1_pulses;
> -
> -	u8 pwm1;
>   	u8 temp1_auto_point_temp[3];
>   	u8 temp2_auto_point_temp[3];
> -	u8 pwm1_auto_point_pwm[3];
> -	u8 pwm1_enable;
> -	u8 pwm1_auto_channels_temp;
> -
> -	u8 stat1;
> -	u8 stat2;
>   };
>   
> -static struct amc6821_data *amc6821_update_device(struct device *dev)
> +static int amc6821_init_auto_point_data(struct amc6821_data *data)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int timeout = HZ;
> -	u8 reg;
> -	int i;
> +	struct regmap *regmap = data->regmap;
> +	u32 pwm, regval;
> +	int err;
>   
> -	mutex_lock(&data->update_lock);
> +	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> +	if (err)
> +		return err;
>   
> -	if (time_after(jiffies, data->last_updated + timeout) ||
> -			!data->valid) {
> +	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
> +	if (err)
> +		return err;
> +	data->temp1_auto_point_temp[0] = regval;
> +	data->temp2_auto_point_temp[0] = data->temp1_auto_point_temp[0];
>   
> -		for (i = 0; i < TEMP_IDX_LEN; i++)
> -			data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
> -				client, temp_reg[i]);
> +	err = regmap_read(regmap, AMC6821_REG_LTEMP_FAN_CTRL, &regval);
> +	if (err)
> +		return err;
> +	data->temp1_auto_point_temp[1] = (regval & 0xF8) >> 1;
>   
> -		data->stat1 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT1);
> -		data->stat2 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT2);
> +	regval &= 0x07;
> +	regval = 0x20 >> regval;
> +	if (regval)
> +		data->temp1_auto_point_temp[2] =
> +			data->temp1_auto_point_temp[1] +
> +			(255 - pwm) / regval;
> +	else
> +		data->temp1_auto_point_temp[2] = 255;
>   
> -		data->pwm1 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_DCY);
> -		for (i = 0; i < FAN1_IDX_LEN; i++) {
> -			data->fan[i] = i2c_smbus_read_byte_data(
> -					client,
> -					fan_reg_low[i]);
> -			data->fan[i] += i2c_smbus_read_byte_data(
> -					client,
> -					fan_reg_hi[i]) << 8;
> -		}
> -		data->fan1_pulses = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_CONF4);
> -		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
> +	err = regmap_read(regmap, AMC6821_REG_RTEMP_FAN_CTRL, &regval);
> +	if (err)
> +		return err;
>   
> -		data->pwm1_auto_point_pwm[0] = 0;
> -		data->pwm1_auto_point_pwm[2] = 255;
> -		data->pwm1_auto_point_pwm[1] = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_DCY_LOW_TEMP);
> +	data->temp2_auto_point_temp[1] = (regval & 0xF8) >> 1;
> +	regval &= 0x07;
> +	regval = 0x20 >> regval;
>   
> -		data->temp1_auto_point_temp[0] =
> -			i2c_smbus_read_byte_data(client,
> -					AMC6821_REG_PSV_TEMP);
> -		data->temp2_auto_point_temp[0] =
> -				data->temp1_auto_point_temp[0];
> -		reg = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_LTEMP_FAN_CTRL);
> -		data->temp1_auto_point_temp[1] = (reg & 0xF8) >> 1;
> -		reg &= 0x07;
> -		reg = 0x20 >> reg;
> -		if (reg > 0)
> -			data->temp1_auto_point_temp[2] =
> -				data->temp1_auto_point_temp[1] +
> -				(data->pwm1_auto_point_pwm[2] -
> -				data->pwm1_auto_point_pwm[1]) / reg;
> -		else
> -			data->temp1_auto_point_temp[2] = 255;
> +	if (regval)
> +		data->temp2_auto_point_temp[2] =
> +			data->temp2_auto_point_temp[1] +
> +			(255 - pwm) / regval;
> +	else
> +		data->temp2_auto_point_temp[2] = 255;
>   
> -		reg = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_RTEMP_FAN_CTRL);
> -		data->temp2_auto_point_temp[1] = (reg & 0xF8) >> 1;
> -		reg &= 0x07;
> -		reg = 0x20 >> reg;
> -		if (reg > 0)
> -			data->temp2_auto_point_temp[2] =
> -				data->temp2_auto_point_temp[1] +
> -				(data->pwm1_auto_point_pwm[2] -
> -				data->pwm1_auto_point_pwm[1]) / reg;
> -		else
> -			data->temp2_auto_point_temp[2] = 255;
> -
> -		reg = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
> -		reg = (reg >> 5) & 0x3;
> -		switch (reg) {
> -		case 0: /*open loop: software sets pwm1*/
> -			data->pwm1_auto_channels_temp = 0;
> -			data->pwm1_enable = 1;
> -			break;
> -		case 2: /*closed loop: remote T (temp2)*/
> -			data->pwm1_auto_channels_temp = 2;
> -			data->pwm1_enable = 2;
> -			break;
> -		case 3: /*closed loop: local and remote T (temp2)*/
> -			data->pwm1_auto_channels_temp = 3;
> -			data->pwm1_enable = 3;
> -			break;
> -		case 1: /*
> -			 * semi-open loop: software sets rpm, chip controls
> -			 * pwm1
> -			 */
> -			data->pwm1_auto_channels_temp = 0;
> -			data->pwm1_enable = 4;
> -			break;
> -		}
> -
> -		data->last_updated = jiffies;
> -		data->valid = true;
> -	}
> -	mutex_unlock(&data->update_lock);
> -	return data;
> +	return 0;
>   }
>   
>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>   			 char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> +	u32 regval;

Why not a u8 directly here? We know single reads are going to return a 
u8 so no need to store more?

I'm not too fluent in type conversion, but maybe even an s8 since this 
is actually what's stored in the register?

> +	int err;
>   
> -	return sprintf(buf, "%d\n", data->temp[ix] * 1000);
> +	err = regmap_read(data->regmap, temp_reg[ix], &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);

The type casting choice is odd here. Considering we'll be printing an 
int and that 1000 cannot be stored in an int8_t, maybe just cast it to 
an int directly?

>   }
>   
>   static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
>   			  const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	int ix = to_sensor_dev_attr(attr)->index;
>   	long val;
> +	int err;
>   
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   	val = clamp_val(val / 1000, -128, 127);
>   
> -	mutex_lock(&data->update_lock);
> -	data->temp[ix] = val;
> -	if (i2c_smbus_write_byte_data(client, temp_reg[ix], data->temp[ix])) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		count = -EIO;
> -	}
> -	mutex_unlock(&data->update_lock);
> +	err = regmap_write(data->regmap, temp_reg[ix], val);
> +	if (err)
> +		return err;
> +
>   	return count;
>   }
>   
>   static ssize_t temp_alarm_show(struct device *dev,
>   			       struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> -	u8 flag;
> +	u32 regval, mask, reg;

Why not u8 for regval?

> +	int err;
>   
>   	switch (ix) {
>   	case IDX_TEMP1_MIN:
> -		flag = data->stat1 & AMC6821_STAT1_LTL;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_LTL;
>   		break;
>   	case IDX_TEMP1_MAX:
> -		flag = data->stat1 & AMC6821_STAT1_LTH;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_LTH;
>   		break;
>   	case IDX_TEMP1_CRIT:
> -		flag = data->stat2 & AMC6821_STAT2_LTC;
> +		reg = AMC6821_REG_STAT2;
> +		mask = AMC6821_STAT2_LTC;
>   		break;
>   	case IDX_TEMP2_MIN:
> -		flag = data->stat1 & AMC6821_STAT1_RTL;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_RTL;
>   		break;
>   	case IDX_TEMP2_MAX:
> -		flag = data->stat1 & AMC6821_STAT1_RTH;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_RTH;
>   		break;
>   	case IDX_TEMP2_CRIT:
> -		flag = data->stat2 & AMC6821_STAT2_RTC;
> +		reg = AMC6821_REG_STAT2;
> +		mask = AMC6821_STAT2_RTC;
>   		break;
>   	default:
>   		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
>   		return -EINVAL;
>   	}
> -	if (flag)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	err = regmap_read(data->regmap, reg, &regval);
> +	if (err)
> +		return err;
> +	return sysfs_emit(buf, "%d\n", !!(regval & mask));
>   }
>   
>   static ssize_t temp2_fault_show(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	if (data->stat1 & AMC6821_STAT1_RTF)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;

Ditto.

> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
>   }
>   
>   static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
>   			 char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;

Ditto.

> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", regval);
>   }
>   
>   static ssize_t pwm1_store(struct device *dev,
> @@ -355,7 +299,6 @@ static ssize_t pwm1_store(struct device *dev,
>   			  size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	long val;
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
> @@ -364,18 +307,38 @@ static ssize_t pwm1_store(struct device *dev,
>   	if (val < 0 || val > 255)
>   		return -EINVAL;
>   
> -	mutex_lock(&data->update_lock);
> -	data->pwm1 = val;
> -	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
> -	mutex_unlock(&data->update_lock);
> +	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
> +	if (ret)
> +		return ret;
> +
>   	return count;
>   }
>   
>   static ssize_t pwm1_enable_show(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_enable);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	int err;
> +	u32 val;

Ditto.

> +
> +	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> +	if (err)
> +		return err;
> +	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +	case 0:
> +		val = 1;	/* manual */
> +		break;
> +	case AMC6821_CONF1_FDRC0:
> +		val = 4;	/* target rpm (fan1_target) controlled */
> +		break;
> +	case AMC6821_CONF1_FDRC1:
> +		val = 2;	/* remote temp controlled */
> +		break;
> +	default:
> +		val = 3;	/* max(local, remote) temp controlled */
> +		break;
> +	}
> +	return sysfs_emit(buf, "%d\n", val);
>   }
>   
>   static ssize_t pwm1_enable_store(struct device *dev,
> @@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   				 const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	long val;
> -	int config = kstrtol(buf, 10, &val);
> -	if (config)
> -		return config;
> +	u32 mask;

Please rename to something else, e.g. val, as this is NOT used as a mask 
but rather the value to write in the masked bitfield (which is hardcoded 
to AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1.

> +	int err;
>   
> -	mutex_lock(&data->update_lock);
> -	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
> -	if (config < 0) {
> -			dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> -			count = config;
> -			goto unlock;
> -	}
> +	err = kstrtol(buf, 10, &val);
> +	if (err)
> +		return err;
>   
>   	switch (val) {
>   	case 1:
> -		config &= ~AMC6821_CONF1_FDRC0;
> -		config &= ~AMC6821_CONF1_FDRC1;
> +		mask = 0;
>   		break;
>   	case 2:
> -		config &= ~AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mask = AMC6821_CONF1_FDRC1;
>   		break;
>   	case 3:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
>   		break;
>   	case 4:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config &= ~AMC6821_CONF1_FDRC1;
> +		mask = AMC6821_CONF1_FDRC0;
>   		break;
>   	default:
> -		count = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>   	}
> -	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF1, config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> -			count = -EIO;
> -	}
> -unlock:
> -	mutex_unlock(&data->update_lock);
> +
> +	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
> +				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
> +				 mask);
> +	if (err < 0)

Shouldn't we check for err != 0 (so err) instead to be consistent with 
the rest of the code base in the driver?

> +		return err;
> +
>   	return count;
>   }
>   
> @@ -433,8 +384,27 @@ static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
>   					    struct device_attribute *devattr,
>   					    char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_auto_channels_temp);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 val;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> +	if (err)
> +		return err;
> +	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +	case 0:
> +	case AMC6821_CONF1_FDRC0:
> +		val = 0;	/* manual or target rpm controlled */
> +		break;
> +	case AMC6821_CONF1_FDRC1:
> +		val = 2;	/* remote temp controlled */
> +		break;
> +	default:
> +		val = 3;	/* max(local, remote) temp controlled */
> +		break;
> +	}
> +
> +	return sprintf(buf, "%d\n", val);
>   }
>   
>   static ssize_t temp_auto_point_temp_show(struct device *dev,
> @@ -443,7 +413,8 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
>   {
>   	int ix = to_sensor_dev_attr_2(devattr)->index;
>   	int nr = to_sensor_dev_attr_2(devattr)->nr;
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +
>   	switch (nr) {
>   	case 1:
>   		return sprintf(buf, "%d\n",
> @@ -461,44 +432,59 @@ static ssize_t pwm1_auto_point_pwm_show(struct device *dev,
>   					struct device_attribute *devattr,
>   					char *buf)
>   {
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_auto_point_pwm[ix]);
> +	u32 val;
> +	int err;
> +
> +	switch (ix) {
> +	case 0:
> +		val = 0;
> +		break;
> +	case 1:
> +		err = regmap_read(data->regmap, AMC6821_REG_DCY_LOW_TEMP, &val);
> +		if (err)
> +			return err;
> +		break;
> +	default:
> +		val = 255;
> +		break;
> +	}
> +	return sysfs_emit(buf, "%d\n", val);
>   }
>   
> -static inline ssize_t set_slope_register(struct i2c_client *client,
> -		u8 reg,
> -		u8 dpwm,
> -		u8 *ptemp)
> +static inline int set_slope_register(struct regmap *regmap,
> +				     u8 reg, u8 *ptemp)
>   {
> -	int dt;
> -	u8 tmp;
> +	u8 tmp, dpwm;
> +	int err, dt;
> +	u32 pwm;
>   
> -	dt = ptemp[2]-ptemp[1];
> +	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> +	if (err)
> +		return err;
> +
> +	dpwm = 255 - pwm;
> +
> +	dt = ptemp[2] - ptemp[1];
>   	for (tmp = 4; tmp > 0; tmp--) {
>   		if (dt * (0x20 >> tmp) >= dpwm)
>   			break;
>   	}
>   	tmp |= (ptemp[1] & 0x7C) << 1;
> -	if (i2c_smbus_write_byte_data(client,
> -			reg, tmp)) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		return -EIO;
> -	}
> -	return 0;
> +	return regmap_write(regmap, reg, tmp);
>   }
>   
>   static ssize_t temp_auto_point_temp_store(struct device *dev,
>   					  struct device_attribute *attr,
>   					  const char *buf, size_t count)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	struct i2c_client *client = data->client;
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
>   	int ix = to_sensor_dev_attr_2(attr)->index;
>   	int nr = to_sensor_dev_attr_2(attr)->nr;
>   	u8 *ptemp;
>   	u8 reg;
> -	int dpwm;
>   	long val;
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
> @@ -519,7 +505,6 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   	}
>   
>   	mutex_lock(&data->update_lock);
> -	data->valid = false;
>   
>   	switch (ix) {
>   	case 0:
> @@ -528,13 +513,9 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		ptemp[0] = clamp_val(ptemp[0], 0,
>   				     data->temp2_auto_point_temp[1]);
>   		ptemp[0] = clamp_val(ptemp[0], 0, 63);
> -		if (i2c_smbus_write_byte_data(
> -					client,
> -					AMC6821_REG_PSV_TEMP,
> -					ptemp[0])) {
> -				dev_err(&client->dev,
> -					"Register write error, aborting.\n");
> -				count = -EIO;
> +		if (regmap_write(regmap, AMC6821_REG_PSV_TEMP, ptemp[0])) {
> +			dev_err(dev, "Register write error, aborting.\n");
> +			count = -EIO;
>   		}
>   		goto EXIT;
>   	case 1:
> @@ -550,8 +531,7 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		count = -EINVAL;
>   		goto EXIT;
>   	}
> -	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
> -	if (set_slope_register(client, reg, dpwm, ptemp))
> +	if (set_slope_register(regmap, reg, ptemp))
>   		count = -EIO;
>   
>   EXIT:
> @@ -564,38 +544,37 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   					 const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int dpwm;
> -	unsigned long val;
> -	int ret = kstrtoul(buf, 10, &val);
> +	struct regmap *regmap = data->regmap;
> +	long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   
> -	if (val > 255)
> +	if (val > 254)

I think this should have been squashed with an earlier commit?

>   		return -EINVAL;
>   
>   	mutex_lock(&data->update_lock);
> -	data->pwm1_auto_point_pwm[1] = val;
> -	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
> -			data->pwm1_auto_point_pwm[1])) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		count = -EIO;
> -		goto EXIT;
> +
> +	ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
> +				 data->temp1_auto_point_temp);
> +	if (ret) {
> +		count = ret;
> +		goto unlock;
>   	}
> -	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
> -	if (set_slope_register(client, AMC6821_REG_LTEMP_FAN_CTRL, dpwm,
> -			data->temp1_auto_point_temp)) {
> -		count = -EIO;
> -		goto EXIT;
> -	}
> -	if (set_slope_register(client, AMC6821_REG_RTEMP_FAN_CTRL, dpwm,
> -			data->temp2_auto_point_temp)) {
> -		count = -EIO;
> -		goto EXIT;
> +	ret = set_slope_register(regmap, AMC6821_REG_RTEMP_FAN_CTRL,
> +				 data->temp2_auto_point_temp);
> +	if (ret) {
> +		count = ret;
> +		goto unlock;
>   	}
>   
> -EXIT:
> -	data->valid = false;
> +unlock:
>   	mutex_unlock(&data->update_lock);
>   	return count;
>   }
> @@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
>   			char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> -	if (0 == data->fan[ix])
> -		return sprintf(buf, "6000000");
> -	return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
> +	u32 regval;
> +	u8 regs[2];

Can't this be a u16 directly.......

> +	int err;
> +
> +	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
> +	if (err)
> +		return err;
> +	regval = (regs[1] << 8) | regs[0];
> +


..... to avoid doing this here?

> +	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
>   }
>   
>   static ssize_t fan1_fault_show(struct device *dev,
>   			       struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	if (data->stat1 & AMC6821_STAT1_FANS)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
>   }
>   
>   static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>   			 const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	long val;
>   	int ix = to_sensor_dev_attr(attr)->index;
> -	int ret = kstrtol(buf, 10, &val);
> -	if (ret)
> -		return ret;
> -	val = val < 1 ? 0xFFFF : 6000000 / val;
> +	u8 regs[2];

Ditto.

> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err)
> +		return err;
> +
> +	val = val < 1 ? 0xFFFF : 6000000 / val;
> +	val = clamp_val(val, 0, 0xFFFF);
> +
> +	regs[0] = val & 0xff;
> +	regs[1] = val >> 8;
> +

to avoid this here.

> +	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
> +	if (err)
> +		return err;
>   
> -	mutex_lock(&data->update_lock);
> -	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");
> -		count = -EIO;
> -		goto EXIT;
> -	}
> -	if (i2c_smbus_write_byte_data(client,
> -			fan_reg_hi[ix], data->fan[ix] >> 8)) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		count = -EIO;
> -	}
> -EXIT:
> -	mutex_unlock(&data->update_lock);
>   	return count;
>   }
>   
>   static ssize_t fan1_pulses_show(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->fan1_pulses);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
>   }
>   
>   static ssize_t fan1_pulses_store(struct device *dev,
> @@ -662,40 +655,22 @@ static ssize_t fan1_pulses_store(struct device *dev,
>   				 size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	long val;
> -	int config = kstrtol(buf, 10, &val);
> -	if (config)
> -		return config;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err)
> +		return err;
> +
> +	if (val != 2 && val != 4)
> +		return -EINVAL;
> +
> +	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
> +				 AMC6821_CONF4_PSPR,
> +				 val == 4 ? AMC6821_CONF4_PSPR : 0);
> +	if (err)
> +		return err;
>   
> -	mutex_lock(&data->update_lock);
> -	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
> -	if (config < 0) {
> -		dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> -		count = config;
> -		goto EXIT;
> -	}
> -	switch (val) {
> -	case 2:
> -		config &= ~AMC6821_CONF4_PSPR;
> -		data->fan1_pulses = 2;
> -		break;
> -	case 4:
> -		config |= AMC6821_CONF4_PSPR;
> -		data->fan1_pulses = 4;
> -		break;
> -	default:
> -		count = -EINVAL;
> -		goto EXIT;
> -	}
> -	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4, config)) {
> -		dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> -		count = -EIO;
> -	}
> -EXIT:
> -	mutex_unlock(&data->update_lock);
>   	return count;
>   }
>   
> @@ -827,110 +802,84 @@ static int amc6821_detect(
>   	return 0;
>   }
>   
> -static int amc6821_init_client(struct i2c_client *client)
> +static int amc6821_init_client(struct amc6821_data *data)
>   {
> -	int config;
> -	int err = -EIO;
> +	struct regmap *regmap = data->regmap;
> +	int err;
> +
> +	err = amc6821_init_auto_point_data(data);
> +	if (err)
> +		return err;
>   
>   	if (init) {
> -		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
> -
> -		if (config < 0) {
> -				dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> -				return err;
> -		}
> -
> -		config |= AMC6821_CONF4_MODE;
> -
> -		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4,
> -				config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> +		err = regmap_set_bits(regmap, AMC6821_REG_CONF4, AMC6821_CONF4_MODE);
> +		if (err)
>   			return err;
> -		}
> -
> -		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF3);
> -
> -		if (config < 0) {
> -			dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> +		err = regmap_clear_bits(regmap, AMC6821_REG_CONF3, AMC6821_CONF3_THERM_FAN_EN);
> +		if (err)
>   			return err;
> -		}
> -
> -		dev_info(&client->dev, "Revision %d\n", config & 0x0f);
> -
> -		config &= ~AMC6821_CONF3_THERM_FAN_EN;
> -
> -		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF3,
> -				config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> +		err = regmap_clear_bits(regmap, AMC6821_REG_CONF2,
> +					AMC6821_CONF2_RTFIE |
> +					AMC6821_CONF2_LTOIE |
> +					AMC6821_CONF2_RTOIE);
> +		if (err)
>   			return err;
> -		}
>   
> -		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF2);
> -
> -		if (config < 0) {
> -			dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> +		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
> +					 AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
> +					 AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
> +					 AMC6821_CONF1_START |
> +					 (pwminv ? AMC6821_CONF1_PWMINV : 0));
> +		if (err)
>   			return err;
> -		}
> -
> -		config &= ~AMC6821_CONF2_RTFIE;
> -		config &= ~AMC6821_CONF2_LTOIE;
> -		config &= ~AMC6821_CONF2_RTOIE;
> -		if (i2c_smbus_write_byte_data(client,
> -				AMC6821_REG_CONF2, config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> -			return err;
> -		}
> -
> -		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
> -
> -		if (config < 0) {
> -			dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> -			return err;
> -		}
> -
> -		config &= ~AMC6821_CONF1_THERMOVIE;
> -		config &= ~AMC6821_CONF1_FANIE;
> -		config |= AMC6821_CONF1_START;
> -		if (pwminv)
> -			config |= AMC6821_CONF1_PWMINV;
> -		else
> -			config &= ~AMC6821_CONF1_PWMINV;
> -
> -		if (i2c_smbus_write_byte_data(
> -				client, AMC6821_REG_CONF1, config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> -			return err;
> -		}
>   	}
>   	return 0;
>   }
>   
> +static bool amc6821_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AMC6821_REG_STAT1:
> +	case AMC6821_REG_STAT2:
> +	case AMC6821_REG_TEMP_LO:
> +	case AMC6821_REG_TDATA_LOW: > +	case AMC6821_REG_LTEMP_HI:
> +	case AMC6821_REG_RTEMP_HI:
> +	case AMC6821_REG_TDATA_HI:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config amc6821_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AMC6821_REG_CONF3,
> +	.volatile_reg = amc6821_volatile_reg,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
>   static int amc6821_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct amc6821_data *data;
>   	struct device *hwmon_dev;
> +	struct regmap *regmap;

Save this variable by using data->client directly?

Cheers,
Quentin

Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

[Guenter Roeck]

On 7/1/24 06:01, Quentin Schulz wrote:

>> -#define AMC6821_CONF1_FDRC1        BIT(7)
>> +#define AMC6821_CONF1_FDRC1        BIT(6)
> 
> This should have been squashed with a previous commit.
> 

Yes. I had found the bug but then fixed it in the wrong patch of the series.

...
>>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>>                char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> +    u32 regval;
> 
> Why not a u8 directly here? We know single reads are going to return a u8 so no need to store more?
> 

The parameter of regmap_read is a pointer to unsigned int.

> I'm not too fluent in type conversion, but maybe even an s8 since this is actually what's stored in the register?
> 
>> +    int err;
>> -    return sprintf(buf, "%d\n", data->temp[ix] * 1000);
>> +    err = regmap_read(data->regmap, temp_reg[ix], &regval);
>> +    if (err)
>> +        return err;
>> +
>> +    return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
> 
> The type casting choice is odd here. Considering we'll be printing an int and that 1000 cannot be stored in an int8_t, maybe just cast it to an int directly?
> 

This is a trick which actually originates from the original code, only
there it is hidden in amc6821_update_device(). This is equivalent to
sign_extend32(regval, 7) which is more expensive, so I decided to stick
with it. On the other side, you are correct, it makes the code more
difficult to understand. I'll change it to use sign_extend32().


>>   static ssize_t temp_alarm_show(struct device *dev,
>>                      struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> -    u8 flag;
>> +    u32 regval, mask, reg;
> 
> Why not u8 for regval?
> 

Same as above. regmap() expects a pointer to unsigned int.


>>   static ssize_t temp2_fault_show(struct device *dev,
>>                   struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> -    if (data->stat1 & AMC6821_STAT1_RTF)
>> -        return sprintf(buf, "1");
>> -    else
>> -        return sprintf(buf, "0");
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    u32 regval;
> 
> Ditto.
> 

Same here and everywhere else.

>>   static ssize_t pwm1_enable_store(struct device *dev,
>> @@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
>>                    const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>>       long val;
>> -    int config = kstrtol(buf, 10, &val);
>> -    if (config)
>> -        return config;
>> +    u32 mask;
> 
> Please rename to something else, e.g. val, as this is NOT used as a mask but rather the value to write in the masked bitfield (which is hardcoded to AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1.
> 

I'll rename it to mode.

>> +
>> +    err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
>> +                 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
>> +                 mask);
>> +    if (err < 0)
> 
> Shouldn't we check for err != 0 (so err) instead to be consistent with the rest of the code base in the driver?
> 

Ok.

>> @@ -564,38 +544,37 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>                        const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int dpwm;
>> -    unsigned long val;
>> -    int ret = kstrtoul(buf, 10, &val);
>> +    struct regmap *regmap = data->regmap;
>> +    long val;
>> +    int ret;
>> +
>> +    ret = kstrtoul(buf, 10, &val);
>>       if (ret)
>>           return ret;
>> -    if (val > 255)
>> +    if (val > 254)
> 
> I think this should have been squashed with an earlier commit?
> 
Yes, another fix applied to the wrong patch.

>> @@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>   static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
>>               char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> -    if (0 == data->fan[ix])
>> -        return sprintf(buf, "6000000");
>> -    return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
>> +    u32 regval;
>> +    u8 regs[2];
> 
> Can't this be a u16 directly.......
> 
>> +    int err;
>> +
>> +    err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
>> +    if (err)
>> +        return err;
>> +    regval = (regs[1] << 8) | regs[0];
>> +
> 
> 
> ..... to avoid doing this here?
> 

Then it would need an endianness conversion instead. That might save a couple
of cycles, but I think it would make the code more difficult to read.

>> +    return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
>>   }
>>   static ssize_t fan1_fault_show(struct device *dev,
>>                      struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> -    if (data->stat1 & AMC6821_STAT1_FANS)
>> -        return sprintf(buf, "1");
>> -    else
>> -        return sprintf(buf, "0");
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    u32 regval;
>> +    int err;
>> +
>> +    err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
>> +    if (err)
>> +        return err;
>> +
>> +    return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
>>   }
>>   static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>>                const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    long val;
>>       int ix = to_sensor_dev_attr(attr)->index;
>> -    int ret = kstrtol(buf, 10, &val);
>> -    if (ret)
>> -        return ret;
>> -    val = val < 1 ? 0xFFFF : 6000000 / val;
>> +    u8 regs[2];
> 
> Ditto.
> 
>> +    long val;
>> +    int err;
>> +
>> +    err = kstrtol(buf, 10, &val);
>> +    if (err)
>> +        return err;
>> +
>> +    val = val < 1 ? 0xFFFF : 6000000 / val;
>> +    val = clamp_val(val, 0, 0xFFFF);
>> +
>> +    regs[0] = val & 0xff;
>> +    regs[1] = val >> 8;
>> +
> 
> to avoid this here.
> Again, this would require an endianness conversion. Sure, that would save a couple
of cycles, but it would make the code more difficult to read.

>>   static int amc6821_probe(struct i2c_client *client)
>>   {
>>       struct device *dev = &client->dev;
>>       struct amc6821_data *data;
>>       struct device *hwmon_dev;
>> +    struct regmap *regmap;
> 
> Save this variable by using data->client directly?
> 

Then I'd have to dereference it twice to check for error.
In such situations I prefer to use a separate variable.

Thanks,
Guenter

Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

[Quentin Schulz]

Hi Guenter,

On 7/1/24 3:47 PM, Guenter Roeck wrote:
> On 7/1/24 06:01, Quentin Schulz wrote:
> 
>>> -#define AMC6821_CONF1_FDRC1        BIT(7)
>>> +#define AMC6821_CONF1_FDRC1        BIT(6)
>>
>> This should have been squashed with a previous commit.
>>
> 
> Yes. I had found the bug but then fixed it in the wrong patch of the 
> series.
> 
> ...
>>>   static ssize_t temp_show(struct device *dev, struct 
>>> device_attribute *devattr,
>>>                char *buf)
>>>   {
>>> -    struct amc6821_data *data = amc6821_update_device(dev);
>>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>>       int ix = to_sensor_dev_attr(devattr)->index;
>>> +    u32 regval;
>>
>> Why not a u8 directly here? We know single reads are going to return a 
>> u8 so no need to store more?
>>
> 
> The parameter of regmap_read is a pointer to unsigned int.
> 

Eventually through the many indirections, because our regmap_config sets 
the size of values in registers to 8b, it's a u8. But it's not worth our 
time to optimize this.

EDIT: coming back to this after reading the rest... Wouldn't we have a 
possible endianness issue here? Especially with the int8_t cast or the 
sign_extend32 (wouldn't the sign bit be at a different index on LE/BE 
?). Sorry for the question, endianness really isn't my cup of tea.

>> I'm not too fluent in type conversion, but maybe even an s8 since this 
>> is actually what's stored in the register?
>>
>>> +    int err;
>>> -    return sprintf(buf, "%d\n", data->temp[ix] * 1000);
>>> +    err = regmap_read(data->regmap, temp_reg[ix], &regval);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
>>
>> The type casting choice is odd here. Considering we'll be printing an 
>> int and that 1000 cannot be stored in an int8_t, maybe just cast it to 
>> an int directly?
>>
> 
> This is a trick which actually originates from the original code, only
> there it is hidden in amc6821_update_device(). This is equivalent to
> sign_extend32(regval, 7) which is more expensive, so I decided to stick
> with it. On the other side, you are correct, it makes the code more
> difficult to understand. I'll change it to use sign_extend32().
> 

Indeed, I completely missed the implications of having a "real" s8 
stored in a u32, thanks for the explanation. It does make more sense to 
use sign_extend32() indeed.

[...]
>>> @@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct 
>>> device *dev,
>>>   static ssize_t fan_show(struct device *dev, struct device_attribute 
>>> *devattr,
>>>               char *buf)
>>>   {
>>> -    struct amc6821_data *data = amc6821_update_device(dev);
>>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>>       int ix = to_sensor_dev_attr(devattr)->index;
>>> -    if (0 == data->fan[ix])
>>> -        return sprintf(buf, "6000000");
>>> -    return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
>>> +    u32 regval;
>>> +    u8 regs[2];
>>
>> Can't this be a u16 directly.......
>>
>>> +    int err;
>>> +
>>> +    err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
>>> +    if (err)
>>> +        return err;
>>> +    regval = (regs[1] << 8) | regs[0];
>>> +
>>
>>
>> ..... to avoid doing this here?
>>
> 
> Then it would need an endianness conversion instead. That might save a 
> couple
> of cycles, but I think it would make the code more difficult to read.
> 

Ah dang it, I always forget Aarch64 (thus LE) is not the only 
architecture that exists in the world :D Endianness conversion being my 
nemesis, I wholeheartedly agree with you.

[...]
>>>   static int amc6821_probe(struct i2c_client *client)
>>>   {
>>>       struct device *dev = &client->dev;
>>>       struct amc6821_data *data;
>>>       struct device *hwmon_dev;
>>> +    struct regmap *regmap;
>>
>> Save this variable by using data->client directly?
>>
> 
> Then I'd have to dereference it twice to check for error.
> In such situations I prefer to use a separate variable.
> 

Fair enough. One's taste after all :)

Cheers,
Quentin

Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap

[Guenter Roeck]

On 7/1/24 09:54, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 3:47 PM, Guenter Roeck wrote:
>> On 7/1/24 06:01, Quentin Schulz wrote:
>>
>>>> -#define AMC6821_CONF1_FDRC1        BIT(7)
>>>> +#define AMC6821_CONF1_FDRC1        BIT(6)
>>>
>>> This should have been squashed with a previous commit.
>>>
>>
>> Yes. I had found the bug but then fixed it in the wrong patch of the series.
>>
>> ...
>>>>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>>>>                char *buf)
>>>>   {
>>>> -    struct amc6821_data *data = amc6821_update_device(dev);
>>>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>>>       int ix = to_sensor_dev_attr(devattr)->index;
>>>> +    u32 regval;
>>>
>>> Why not a u8 directly here? We know single reads are going to return a u8 so no need to store more?
>>>
>>
>> The parameter of regmap_read is a pointer to unsigned int.
>>
> 
> Eventually through the many indirections, because our regmap_config sets the size of values in registers to 8b, it's a u8. But it's not worth our time to optimize this.
> 
> EDIT: coming back to this after reading the rest... Wouldn't we have a possible endianness issue here? Especially with the int8_t cast or the sign_extend32 (wouldn't the sign bit be at a different index on LE/BE ?). Sorry for the question, endianness really isn't my cup of tea.
> 

The underlying code does an implicit conversion. For example,  see
regmap_smbus_byte_reg_read(). There is no endianness issue because
the returned data is always in host endianness order. Sure, that
could be big endian, but the sign bit is still in bit 7.

Thanks,
Guenter

[PATCH 10/10] hwmon: (amc6821) Convert to with_info API

[Guenter Roeck]

Convert to use with_info API to simplify the code and make it easier
to maintain. This also reduces code size by approximately 20%.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/amc6821.c | 743 +++++++++++++++++++++-------------------
 1 file changed, 386 insertions(+), 357 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 6ffab4288134..14d59aa4254b 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -6,6 +6,9 @@
  *
  * Based on max6650.c:
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
+ *
+ * Conversion to regmap and with_info API:
+ * Copyright (C) 2024 Guenter Roeck <linux@roeck-us.net>
  */
 
 #include <linux/bits.h>
@@ -106,28 +109,6 @@ module_param(init, int, 0444);
 #define AMC6821_STAT2_L_THERM		BIT(6)
 #define AMC6821_STAT2_THERM_IN		BIT(7)
 
-enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
-	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-	IDX_TEMP2_MAX, IDX_TEMP2_CRIT,
-	TEMP_IDX_LEN, };
-
-static const u8 temp_reg[] = {AMC6821_REG_LTEMP_HI,
-			AMC6821_REG_LTEMP_LIMIT_MIN,
-			AMC6821_REG_LTEMP_LIMIT_MAX,
-			AMC6821_REG_LTEMP_CRIT,
-			AMC6821_REG_RTEMP_HI,
-			AMC6821_REG_RTEMP_LIMIT_MIN,
-			AMC6821_REG_RTEMP_LIMIT_MAX,
-			AMC6821_REG_RTEMP_CRIT, };
-
-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_SETTINGL, };
-
 /*
  * Client data (each client gets its own)
  */
@@ -188,232 +169,323 @@ static int amc6821_init_auto_point_data(struct amc6821_data *data)
 	return 0;
 }
 
-static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
+static int amc6821_temp_read_values(struct regmap *regmap, u32 attr, int channel, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
+	int reg, err;
 	u32 regval;
-	int err;
 
-	err = regmap_read(data->regmap, temp_reg[ix], &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
-}
-
-static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(attr)->index;
-	long val;
-	int err;
-
-	int ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-	val = clamp_val(val / 1000, -128, 127);
-
-	err = regmap_write(data->regmap, temp_reg[ix], val);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static ssize_t temp_alarm_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
-	u32 regval, mask, reg;
-	int err;
-
-	switch (ix) {
-	case IDX_TEMP1_MIN:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_LTL;
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? AMC6821_REG_RTEMP_HI : AMC6821_REG_LTEMP_HI;
 		break;
-	case IDX_TEMP1_MAX:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_LTH;
+	case hwmon_temp_min:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
 		break;
-	case IDX_TEMP1_CRIT:
-		reg = AMC6821_REG_STAT2;
-		mask = AMC6821_STAT2_LTC;
+	case hwmon_temp_max:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
 		break;
-	case IDX_TEMP2_MIN:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_RTL;
-		break;
-	case IDX_TEMP2_MAX:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_RTH;
-		break;
-	case IDX_TEMP2_CRIT:
-		reg = AMC6821_REG_STAT2;
-		mask = AMC6821_STAT2_RTC;
+	case hwmon_temp_crit:
+		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
 		break;
 	default:
-		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
-		return -EINVAL;
+		return -EOPNOTSUPP;
 	}
-	err = regmap_read(data->regmap, reg, &regval);
+	err = regmap_read(regmap, reg, &regval);
 	if (err)
 		return err;
-	return sysfs_emit(buf, "%d\n", !!(regval & mask));
+	*val = (int8_t)regval * 1000;
+	return 0;
 }
 
-static ssize_t temp2_fault_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
+static int amc6821_read_alarms(struct regmap *regmap, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
+	int reg, mask, err;
 	u32 regval;
-	int err;
 
-	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
-}
-
-static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", regval);
-}
-
-static ssize_t pwm1_store(struct device *dev,
-			  struct device_attribute *devattr, const char *buf,
-			  size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	long val;
-	int ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-
-	if (val < 0 || val > 255)
-		return -EINVAL;
-
-	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
-	if (ret)
-		return ret;
-
-	return count;
-}
-
-static ssize_t pwm1_enable_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int err;
-	u32 val;
-
-	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
-	if (err)
-		return err;
-	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
-	case 0:
-		val = 1;	/* manual */
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_min_alarm:
+			reg = AMC6821_REG_STAT1;
+			mask = channel ? AMC6821_STAT1_RTL : AMC6821_STAT1_LTL;
+			break;
+		case hwmon_temp_max_alarm:
+			reg = AMC6821_REG_STAT1;
+			mask = channel ? AMC6821_STAT1_RTH : AMC6821_STAT1_LTH;
+			break;
+		case hwmon_temp_crit_alarm:
+			reg = AMC6821_REG_STAT2;
+			mask = channel ? AMC6821_STAT2_RTC : AMC6821_STAT2_LTC;
+			break;
+		case hwmon_temp_fault:
+			reg = AMC6821_REG_STAT1;
+			mask = AMC6821_STAT1_RTF;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
 		break;
-	case AMC6821_CONF1_FDRC0:
-		val = 4;	/* target rpm (fan1_target) controlled */
-		break;
-	case AMC6821_CONF1_FDRC1:
-		val = 2;	/* remote temp controlled */
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_fault:
+			reg = AMC6821_REG_STAT1;
+			mask = AMC6821_STAT1_FANS;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
 		break;
 	default:
-		val = 3;	/* max(local, remote) temp controlled */
-		break;
+		return -EOPNOTSUPP;
 	}
-	return sysfs_emit(buf, "%d\n", val);
+	err = regmap_read(regmap, reg, &regval);
+	if (err)
+		return err;
+	*val = !!(regval & mask);
+	return 0;
 }
 
-static ssize_t pwm1_enable_store(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
+static int amc6821_temp_read(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	long val;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_crit:
+		return amc6821_temp_read_values(data->regmap, attr, channel, val);
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_fault:
+		return amc6821_read_alarms(data->regmap, hwmon_temp, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_temp_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	int reg;
+
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
+
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
+		break;
+	case hwmon_temp_max:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
+		break;
+	case hwmon_temp_crit:
+		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return regmap_write(data->regmap, reg, val);
+}
+
+static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
+		if (err)
+			return err;
+		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+		case 0:
+			*val = 1;	/* manual */
+			break;
+		case AMC6821_CONF1_FDRC0:
+			*val = 4;	/* target rpm (fan1_target) controlled */
+			break;
+		case AMC6821_CONF1_FDRC1:
+			*val = 2;	/* remote temp controlled */
+			break;
+		default:
+			*val = 3;	/* max(local, remote) temp controlled */
+			break;
+		}
+		return 0;
+	case hwmon_pwm_auto_channels_temp:
+		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
+		if (err)
+			return err;
+		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+		case 0:
+		case AMC6821_CONF1_FDRC0:
+			*val = 0;	/* manual or target rpm controlled */
+			break;
+		case AMC6821_CONF1_FDRC1:
+			*val = 2;	/* remote temp controlled */
+			break;
+		default:
+			*val = 3;	/* max(local, remote) temp controlled */
+			break;
+		}
+		return 0;
+	case hwmon_pwm_input:
+		err = regmap_read(regmap, AMC6821_REG_DCY, &regval);
+		if (err)
+			return err;
+		*val = regval;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 	u32 mask;
-	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	switch (val) {
-	case 1:
-		mask = 0;
-		break;
-	case 2:
-		mask = AMC6821_CONF1_FDRC1;
-		break;
-	case 3:
-		mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
-		break;
-	case 4:
-		mask = AMC6821_CONF1_FDRC0;
-		break;
+	switch (attr) {
+	case hwmon_pwm_enable:
+		switch (val) {
+		case 1:
+			mask = 0;
+			break;
+		case 2:
+			mask = AMC6821_CONF1_FDRC1;
+			break;
+		case 3:
+			mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
+			break;
+		case 4:
+			mask = AMC6821_CONF1_FDRC0;
+			break;
+		default:
+			return -EINVAL;
+		}
+		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
+					  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
+					  mask);
+	case hwmon_pwm_input:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+		return regmap_write(regmap, AMC6821_REG_DCY, val);
 	default:
-		return -EINVAL;
+		return -EOPNOTSUPP;
 	}
-
-	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
-				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
-				 mask);
-	if (err < 0)
-		return err;
-
-	return count;
 }
 
-static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
-					    struct device_attribute *devattr,
-					    char *buf)
+static int amc6821_fan_read_rpm(struct regmap *regmap, u32 attr, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 val;
-	int err;
+	int reg, err;
+	u8 regs[2];
+	u32 regval;
 
-	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
-	if (err)
-		return err;
-	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
-	case 0:
-	case AMC6821_CONF1_FDRC0:
-		val = 0;	/* manual or target rpm controlled */
+	switch (attr) {
+	case hwmon_fan_input:
+		reg = AMC6821_REG_TDATA_LOW;
 		break;
-	case AMC6821_CONF1_FDRC1:
-		val = 2;	/* remote temp controlled */
+	case hwmon_fan_min:
+		reg = AMC6821_REG_TACH_LLIMITL;
+		break;
+	case hwmon_fan_max:
+		reg = AMC6821_REG_TACH_HLIMITL;
+		break;
+	case hwmon_fan_target:
+		reg = AMC6821_REG_TACH_SETTINGL;
 		break;
 	default:
-		val = 3;	/* max(local, remote) temp controlled */
-		break;
+		return -EOPNOTSUPP;
 	}
 
-	return sprintf(buf, "%d\n", val);
+	err = regmap_bulk_read(regmap, reg, regs, 2);
+	if (err)
+		return err;
+
+	regval = (regs[1] << 8) | regs[0];
+	*val = 6000000 / (regval ? : 1);
+
+	return 0;
+}
+
+static int amc6821_fan_read(struct device *dev, u32 attr, long *val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_fan_input:
+	case hwmon_fan_min:
+	case hwmon_fan_max:
+	case hwmon_fan_target:
+		return amc6821_fan_read_rpm(regmap, attr, val);
+	case hwmon_fan_fault:
+		return amc6821_read_alarms(regmap, hwmon_fan, attr, 0, val);
+	case hwmon_fan_pulses:
+		err = regmap_read(regmap, AMC6821_REG_CONF4, &regval);
+		if (err)
+			return err;
+		*val = (regval & AMC6821_CONF4_PSPR) ? 4 : 2;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_fan_write(struct device *dev, u32 attr, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u8 regs[2];
+	int reg;
+
+	if (attr == hwmon_fan_pulses) {
+		if (val != 2 && val != 4)
+			return -EINVAL;
+		return regmap_update_bits(regmap, AMC6821_REG_CONF4,
+					 AMC6821_CONF4_PSPR,
+					 val == 4 ? AMC6821_CONF4_PSPR : 0);
+	}
+
+	if (val < 0)
+		return -EINVAL;
+
+	val = clamp_val(6000000 / (val ? : 1), 0, 0xffff);
+
+	switch (attr) {
+	case hwmon_fan_min:
+		reg = AMC6821_REG_TACH_LLIMITL;
+		break;
+	case hwmon_fan_max:
+		reg = AMC6821_REG_TACH_HLIMITL;
+		break;
+	case hwmon_fan_target:
+		reg = AMC6821_REG_TACH_SETTINGL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	regs[0] = val & 0xff;
+	regs[1] = val >> 8;
+
+	return regmap_bulk_write(data->regmap, reg, regs, 2);
 }
 
 static ssize_t temp_auto_point_temp_show(struct device *dev,
 					 struct device_attribute *devattr,
 					 char *buf)
 {
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr_2(devattr)->index;
 	int nr = to_sensor_dev_attr_2(devattr)->nr;
-	struct amc6821_data *data = dev_get_drvdata(dev);
 
 	switch (nr) {
 	case 1:
@@ -423,7 +495,6 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
 		return sprintf(buf, "%d\n",
 			data->temp2_auto_point_temp[ix] * 1000);
 	default:
-		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
 		return -EINVAL;
 	}
 }
@@ -579,130 +650,9 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 	return count;
 }
 
-static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
-			char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
-	u32 regval;
-	u8 regs[2];
-	int err;
-
-	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
-	if (err)
-		return err;
-	regval = (regs[1] << 8) | regs[0];
-
-	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
-}
-
-static ssize_t fan1_fault_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
-}
-
-static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(attr)->index;
-	u8 regs[2];
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	val = val < 1 ? 0xFFFF : 6000000 / val;
-	val = clamp_val(val, 0, 0xFFFF);
-
-	regs[0] = val & 0xff;
-	regs[1] = val >> 8;
-
-	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static ssize_t fan1_pulses_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
-}
-
-static ssize_t fan1_pulses_store(struct device *dev,
-				 struct device_attribute *attr, const char *buf,
-				 size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	if (val != 2 && val != 4)
-		return -EINVAL;
-
-	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
-				 AMC6821_CONF4_PSPR,
-				 val == 4 ? AMC6821_CONF4_PSPR : 0);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, IDX_TEMP1_INPUT);
-static SENSOR_DEVICE_ATTR_RW(temp1_min, temp, IDX_TEMP1_MIN);
-static SENSOR_DEVICE_ATTR_RW(temp1_max, temp, IDX_TEMP1_MAX);
-static SENSOR_DEVICE_ATTR_RW(temp1_crit, temp, IDX_TEMP1_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp1_min_alarm, temp_alarm, IDX_TEMP1_MIN);
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, temp_alarm, IDX_TEMP1_MAX);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, temp_alarm, IDX_TEMP1_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, IDX_TEMP2_INPUT);
-static SENSOR_DEVICE_ATTR_RW(temp2_min, temp, IDX_TEMP2_MIN);
-static SENSOR_DEVICE_ATTR_RW(temp2_max, temp, IDX_TEMP2_MAX);
-static SENSOR_DEVICE_ATTR_RW(temp2_crit, temp, IDX_TEMP2_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp2_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_min_alarm, temp_alarm, IDX_TEMP2_MIN);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, temp_alarm, IDX_TEMP2_MAX);
-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);
-
-static SENSOR_DEVICE_ATTR_RW(pwm1, pwm1, 0);
-static SENSOR_DEVICE_ATTR_RW(pwm1_enable, pwm1_enable, 0);
 static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point1_pwm, pwm1_auto_point_pwm, 0);
 static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_pwm, pwm1_auto_point_pwm, 1);
 static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point3_pwm, pwm1_auto_point_pwm, 2);
-static SENSOR_DEVICE_ATTR_RO(pwm1_auto_channels_temp, pwm1_auto_channels_temp,
-			     0);
 static SENSOR_DEVICE_ATTR_2_RO(temp1_auto_point1_temp, temp_auto_point_temp,
 			       1, 0);
 static SENSOR_DEVICE_ATTR_2_RW(temp1_auto_point2_temp, temp_auto_point_temp,
@@ -718,30 +668,6 @@ static SENSOR_DEVICE_ATTR_2_RW(temp2_auto_point3_temp, temp_auto_point_temp,
 			       2, 2);
 
 static struct attribute *amc6821_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_min.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit.dev_attr.attr,
-	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_min.dev_attr.attr,
-	&sensor_dev_attr_temp2_max.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit.dev_attr.attr,
-	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&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,
-	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
@@ -753,13 +679,117 @@ static struct attribute *amc6821_attrs[] = {
 	&sensor_dev_attr_temp2_auto_point3_temp.dev_attr.attr,
 	NULL
 };
-
 ATTRIBUTE_GROUPS(amc6821);
 
+static int amc6821_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return amc6821_temp_read(dev, attr, channel, val);
+	case hwmon_fan:
+		return amc6821_fan_read(dev, attr, val);
+	case hwmon_pwm:
+		return amc6821_pwm_read(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return amc6821_temp_write(dev, attr, channel, val);
+	case hwmon_fan:
+		return amc6821_fan_write(dev, attr, val);
+	case hwmon_pwm:
+		return amc6821_pwm_write(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t amc6821_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+		case hwmon_temp_crit_alarm:
+		case hwmon_temp_fault:
+			return 0444;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+		case hwmon_temp_crit:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_fault:
+			return 0444;
+		case hwmon_fan_pulses:
+		case hwmon_fan_min:
+		case hwmon_fan_max:
+		case hwmon_fan_target:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_enable:
+		case hwmon_pwm_input:
+			return 0644;
+		case hwmon_pwm_auto_channels_temp:
+			return 0444;
+		default:
+			return 0;
+		}
+	default:
+		return 0;
+	}
+}
+
+static const struct hwmon_channel_info * const amc6821_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX |
+			   HWMON_F_TARGET | HWMON_F_PULSES | HWMON_F_FAULT),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE |
+			   HWMON_PWM_AUTO_CHANNELS_TEMP),
+	NULL
+};
+
+static const struct hwmon_ops amc6821_hwmon_ops = {
+	.is_visible = amc6821_is_visible,
+	.read = amc6821_read,
+	.write = amc6821_write,
+};
+
+static const struct hwmon_chip_info amc6821_chip_info = {
+	.ops = &amc6821_hwmon_ops,
+	.info = amc6821_info,
+};
+
 /* Return 0 if detection is successful, -ENODEV otherwise */
-static int amc6821_detect(
-		struct i2c_client *client,
-		struct i2c_board_info *info)
+static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	int address = client->addr;
@@ -872,7 +902,6 @@ static int amc6821_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-
 	regmap = devm_regmap_init_i2c(client, &amc6821_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap),
@@ -883,9 +912,9 @@ static int amc6821_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data,
-							   amc6821_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data, &amc6821_chip_info,
+							 amc6821_groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.2

Re: [PATCH 10/10] hwmon: (amc6821) Convert to with_info API

[Quentin Schulz]

Hi Guenter,

On 6/28/24 5:13 PM, Guenter Roeck wrote:
> Convert to use with_info API to simplify the code and make it easier
> to maintain. This also reduces code size by approximately 20%.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/hwmon/amc6821.c | 743 +++++++++++++++++++++-------------------
>   1 file changed, 386 insertions(+), 357 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 6ffab4288134..14d59aa4254b 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -6,6 +6,9 @@
>    *
>    * Based on max6650.c:
>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
> + *
> + * Conversion to regmap and with_info API:
> + * Copyright (C) 2024 Guenter Roeck <linux@roeck-us.net>
>    */
>   
>   #include <linux/bits.h>
> @@ -106,28 +109,6 @@ module_param(init, int, 0444);
>   #define AMC6821_STAT2_L_THERM		BIT(6)
>   #define AMC6821_STAT2_THERM_IN		BIT(7)
>   
> -enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
> -	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
> -	IDX_TEMP2_MAX, IDX_TEMP2_CRIT,
> -	TEMP_IDX_LEN, };
> -
> -static const u8 temp_reg[] = {AMC6821_REG_LTEMP_HI,
> -			AMC6821_REG_LTEMP_LIMIT_MIN,
> -			AMC6821_REG_LTEMP_LIMIT_MAX,
> -			AMC6821_REG_LTEMP_CRIT,
> -			AMC6821_REG_RTEMP_HI,
> -			AMC6821_REG_RTEMP_LIMIT_MIN,
> -			AMC6821_REG_RTEMP_LIMIT_MAX,
> -			AMC6821_REG_RTEMP_CRIT, };
> -
> -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_SETTINGL, };
> -
>   /*
>    * Client data (each client gets its own)
>    */
> @@ -188,232 +169,323 @@ static int amc6821_init_auto_point_data(struct amc6821_data *data)
>   	return 0;
>   }
>   
> -static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
> -			 char *buf)
> +static int amc6821_temp_read_values(struct regmap *regmap, u32 attr, int channel, long *val)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int ix = to_sensor_dev_attr(devattr)->index;
> +	int reg, err;
>   	u32 regval;
> -	int err;
>   
> -	err = regmap_read(data->regmap, temp_reg[ix], &regval);
> -	if (err)
> -		return err;
> -
> -	return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
> -}
> -
> -static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
> -			  const char *buf, size_t count)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int ix = to_sensor_dev_attr(attr)->index;
> -	long val;
> -	int err;
> -
> -	int ret = kstrtol(buf, 10, &val);
> -	if (ret)
> -		return ret;
> -	val = clamp_val(val / 1000, -128, 127);
> -
> -	err = regmap_write(data->regmap, temp_reg[ix], val);
> -	if (err)
> -		return err;
> -
> -	return count;
> -}
> -
> -static ssize_t temp_alarm_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int ix = to_sensor_dev_attr(devattr)->index;
> -	u32 regval, mask, reg;
> -	int err;
> -
> -	switch (ix) {
> -	case IDX_TEMP1_MIN:
> -		reg = AMC6821_REG_STAT1;
> -		mask = AMC6821_STAT1_LTL;
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = channel ? AMC6821_REG_RTEMP_HI : AMC6821_REG_LTEMP_HI;
>   		break;
> -	case IDX_TEMP1_MAX:
> -		reg = AMC6821_REG_STAT1;
> -		mask = AMC6821_STAT1_LTH;
> +	case hwmon_temp_min:
> +		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
>   		break;
> -	case IDX_TEMP1_CRIT:
> -		reg = AMC6821_REG_STAT2;
> -		mask = AMC6821_STAT2_LTC;
> +	case hwmon_temp_max:
> +		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
>   		break;
> -	case IDX_TEMP2_MIN:
> -		reg = AMC6821_REG_STAT1;
> -		mask = AMC6821_STAT1_RTL;
> -		break;
> -	case IDX_TEMP2_MAX:
> -		reg = AMC6821_REG_STAT1;
> -		mask = AMC6821_STAT1_RTH;
> -		break;
> -	case IDX_TEMP2_CRIT:
> -		reg = AMC6821_REG_STAT2;
> -		mask = AMC6821_STAT2_RTC;
> +	case hwmon_temp_crit:
> +		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
>   	}
> -	err = regmap_read(data->regmap, reg, &regval);
> +	err = regmap_read(regmap, reg, &regval);
>   	if (err)
>   		return err;
> -	return sysfs_emit(buf, "%d\n", !!(regval & mask));
> +	*val = (int8_t)regval * 1000;

The discussed signed_extended32() in another patch would need to make it 
here too.

> +	return 0;
>   }
>   
> -static ssize_t temp2_fault_show(struct device *dev,
> -				struct device_attribute *devattr, char *buf)
> +static int amc6821_read_alarms(struct regmap *regmap, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long *val)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> +	int reg, mask, err;
>   	u32 regval;
> -	int err;
>   
> -	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> -	if (err)
> -		return err;
> -
> -	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
> -}
> -
> -static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
> -			 char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	u32 regval;
> -	int err;
> -
> -	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
> -	if (err)
> -		return err;
> -
> -	return sysfs_emit(buf, "%d\n", regval);
> -}
> -
> -static ssize_t pwm1_store(struct device *dev,
> -			  struct device_attribute *devattr, const char *buf,
> -			  size_t count)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	long val;
> -	int ret = kstrtol(buf, 10, &val);
> -	if (ret)
> -		return ret;
> -
> -	if (val < 0 || val > 255)
> -		return -EINVAL;
> -
> -	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
> -	if (ret)
> -		return ret;
> -
> -	return count;
> -}
> -
> -static ssize_t pwm1_enable_show(struct device *dev,
> -				struct device_attribute *devattr, char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int err;
> -	u32 val;
> -
> -	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> -	if (err)
> -		return err;
> -	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> -	case 0:
> -		val = 1;	/* manual */
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_min_alarm:
> +			reg = AMC6821_REG_STAT1;
> +			mask = channel ? AMC6821_STAT1_RTL : AMC6821_STAT1_LTL;
> +			break;
> +		case hwmon_temp_max_alarm:
> +			reg = AMC6821_REG_STAT1;
> +			mask = channel ? AMC6821_STAT1_RTH : AMC6821_STAT1_LTH;
> +			break;
> +		case hwmon_temp_crit_alarm:
> +			reg = AMC6821_REG_STAT2;
> +			mask = channel ? AMC6821_STAT2_RTC : AMC6821_STAT2_LTC;
> +			break;
> +		case hwmon_temp_fault:
> +			reg = AMC6821_REG_STAT1;
> +			mask = AMC6821_STAT1_RTF;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>   		break;
> -	case AMC6821_CONF1_FDRC0:
> -		val = 4;	/* target rpm (fan1_target) controlled */
> -		break;
> -	case AMC6821_CONF1_FDRC1:
> -		val = 2;	/* remote temp controlled */
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_fault:
> +			reg = AMC6821_REG_STAT1;
> +			mask = AMC6821_STAT1_FANS;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
>   		break;
>   	default:
> -		val = 3;	/* max(local, remote) temp controlled */
> -		break;
> +		return -EOPNOTSUPP;
>   	}
> -	return sysfs_emit(buf, "%d\n", val);
> +	err = regmap_read(regmap, reg, &regval);
> +	if (err)
> +		return err;
> +	*val = !!(regval & mask);
> +	return 0;
>   }
>   
> -static ssize_t pwm1_enable_store(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t count)
> +static int amc6821_temp_read(struct device *dev, u32 attr, int channel, long *val)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	long val;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +	case hwmon_temp_min:
> +	case hwmon_temp_max:
> +	case hwmon_temp_crit:
> +		return amc6821_temp_read_values(data->regmap, attr, channel, val);
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_crit_alarm:
> +	case hwmon_temp_fault:
> +		return amc6821_read_alarms(data->regmap, hwmon_temp, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int amc6821_temp_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	int reg;
> +
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
> +
> +	switch (attr) {
> +	case hwmon_temp_min:
> +		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
> +		break;
> +	case hwmon_temp_max:
> +		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return regmap_write(data->regmap, reg, val);
> +}
> +
> +static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	u32 regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
> +		if (err)
> +			return err;
> +		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +		case 0:
> +			*val = 1;	/* manual */
> +			break;
> +		case AMC6821_CONF1_FDRC0:
> +			*val = 4;	/* target rpm (fan1_target) controlled */
> +			break;
> +		case AMC6821_CONF1_FDRC1:
> +			*val = 2;	/* remote temp controlled */
> +			break;
> +		default:
> +			*val = 3;	/* max(local, remote) temp controlled */
> +			break;
> +		}
> +		return 0;
> +	case hwmon_pwm_auto_channels_temp:
> +		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
> +		if (err)
> +			return err;
> +		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +		case 0:
> +		case AMC6821_CONF1_FDRC0:
> +			*val = 0;	/* manual or target rpm controlled */
> +			break;
> +		case AMC6821_CONF1_FDRC1:
> +			*val = 2;	/* remote temp controlled */
> +			break;
> +		default:
> +			*val = 3;	/* max(local, remote) temp controlled */
> +			break;
> +		}
> +		return 0;
> +	case hwmon_pwm_input:
> +		err = regmap_read(regmap, AMC6821_REG_DCY, &regval);
> +		if (err)
> +			return err;
> +		*val = regval;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
> +{
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
>   	u32 mask;
> -	int err;
>   
> -	err = kstrtol(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	switch (val) {
> -	case 1:
> -		mask = 0;
> -		break;
> -	case 2:
> -		mask = AMC6821_CONF1_FDRC1;
> -		break;
> -	case 3:
> -		mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
> -		break;
> -	case 4:
> -		mask = AMC6821_CONF1_FDRC0;
> -		break;
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		switch (val) {
> +		case 1:
> +			mask = 0;
> +			break;
> +		case 2:
> +			mask = AMC6821_CONF1_FDRC1;
> +			break;
> +		case 3:
> +			mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
> +			break;
> +		case 4:
> +			mask = AMC6821_CONF1_FDRC0;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
> +					  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
> +					  mask);
> +	case hwmon_pwm_input:
> +		if (val < 0 || val > 255)
> +			return -EINVAL;
> +		return regmap_write(regmap, AMC6821_REG_DCY, val);
>   	default:
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
>   	}
> -
> -	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
> -				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
> -				 mask);
> -	if (err < 0)
> -		return err;
> -
> -	return count;
>   }
>   
> -static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
> -					    struct device_attribute *devattr,
> -					    char *buf)
> +static int amc6821_fan_read_rpm(struct regmap *regmap, u32 attr, long *val)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	u32 val;
> -	int err;
> +	int reg, err;
> +	u8 regs[2];
> +	u32 regval;
>   
> -	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> -	if (err)
> -		return err;
> -	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> -	case 0:
> -	case AMC6821_CONF1_FDRC0:
> -		val = 0;	/* manual or target rpm controlled */
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		reg = AMC6821_REG_TDATA_LOW;
>   		break;
> -	case AMC6821_CONF1_FDRC1:
> -		val = 2;	/* remote temp controlled */
> +	case hwmon_fan_min:
> +		reg = AMC6821_REG_TACH_LLIMITL;
> +		break;
> +	case hwmon_fan_max:
> +		reg = AMC6821_REG_TACH_HLIMITL;
> +		break;
> +	case hwmon_fan_target:
> +		reg = AMC6821_REG_TACH_SETTINGL;
>   		break;
>   	default:
> -		val = 3;	/* max(local, remote) temp controlled */
> -		break;
> +		return -EOPNOTSUPP;
>   	}
>   
> -	return sprintf(buf, "%d\n", val);
> +	err = regmap_bulk_read(regmap, reg, regs, 2);
> +	if (err)
> +		return err;
> +
> +	regval = (regs[1] << 8) | regs[0];
> +	*val = 6000000 / (regval ? : 1);
> +

Just putting a "bookmark" here since we're having a discussion about 
this logic in another thread for another patch.

Also... missing 0 here between the question mark and the colon?

> +	return 0;
> +}
> +
> +static int amc6821_fan_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	u32 regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +	case hwmon_fan_min:
> +	case hwmon_fan_max:
> +	case hwmon_fan_target:
> +		return amc6821_fan_read_rpm(regmap, attr, val);
> +	case hwmon_fan_fault:
> +		return amc6821_read_alarms(regmap, hwmon_fan, attr, 0, val);
> +	case hwmon_fan_pulses:
> +		err = regmap_read(regmap, AMC6821_REG_CONF4, &regval);
> +		if (err)
> +			return err;
> +		*val = (regval & AMC6821_CONF4_PSPR) ? 4 : 2;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int amc6821_fan_write(struct device *dev, u32 attr, long val)
> +{
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	u8 regs[2];
> +	int reg;
> +
> +	if (attr == hwmon_fan_pulses) {
> +		if (val != 2 && val != 4)
> +			return -EINVAL;
> +		return regmap_update_bits(regmap, AMC6821_REG_CONF4,
> +					 AMC6821_CONF4_PSPR,
> +					 val == 4 ? AMC6821_CONF4_PSPR : 0);
> +	}
> +
> +	if (val < 0)
> +		return -EINVAL;
> +
> +	val = clamp_val(6000000 / (val ? : 1), 0, 0xffff);
> +

And another "bookmark" here.

> +	switch (attr) {
> +	case hwmon_fan_min:
> +		reg = AMC6821_REG_TACH_LLIMITL;
> +		break;
> +	case hwmon_fan_max:
> +		reg = AMC6821_REG_TACH_HLIMITL;
> +		break;
> +	case hwmon_fan_target:
> +		reg = AMC6821_REG_TACH_SETTINGL;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	regs[0] = val & 0xff;
> +	regs[1] = val >> 8;
> +
> +	return regmap_bulk_write(data->regmap, reg, regs, 2);
>   }
>   
>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>   					 struct device_attribute *devattr,
>   					 char *buf)
>   {
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr_2(devattr)->index;
>   	int nr = to_sensor_dev_attr_2(devattr)->nr;
> -	struct amc6821_data *data = dev_get_drvdata(dev);
>   

Should be squashed with the appropriate commit?

>   	switch (nr) {
>   	case 1:
> @@ -423,7 +495,6 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
>   		return sprintf(buf, "%d\n",
>   			data->temp2_auto_point_temp[ix] * 1000);
>   	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);

Not sure this is related? Maybe a separate commit?

>   		return -EINVAL;
>   	}
>   }
> @@ -579,130 +650,9 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   	return count;
>   }
>   
> -static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int ix = to_sensor_dev_attr(devattr)->index;
> -	u32 regval;
> -	u8 regs[2];
> -	int err;
> -
> -	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
> -	if (err)
> -		return err;
> -	regval = (regs[1] << 8) | regs[0];
> -
> -	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
> -}
> -
> -static ssize_t fan1_fault_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	u32 regval;
> -	int err;
> -
> -	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> -	if (err)
> -		return err;
> -
> -	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
> -}
> -
> -static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
> -			 const char *buf, size_t count)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	int ix = to_sensor_dev_attr(attr)->index;
> -	u8 regs[2];
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	val = val < 1 ? 0xFFFF : 6000000 / val;
> -	val = clamp_val(val, 0, 0xFFFF);
> -
> -	regs[0] = val & 0xff;
> -	regs[1] = val >> 8;
> -
> -	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
> -	if (err)
> -		return err;
> -
> -	return count;
> -}
> -
> -static ssize_t fan1_pulses_show(struct device *dev,
> -				struct device_attribute *devattr, char *buf)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	u32 regval;
> -	int err;
> -
> -	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
> -	if (err)
> -		return err;
> -
> -	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
> -}
> -
> -static ssize_t fan1_pulses_store(struct device *dev,
> -				 struct device_attribute *attr, const char *buf,
> -				 size_t count)
> -{
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	if (val != 2 && val != 4)
> -		return -EINVAL;
> -
> -	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
> -				 AMC6821_CONF4_PSPR,
> -				 val == 4 ? AMC6821_CONF4_PSPR : 0);
> -	if (err)
> -		return err;
> -
> -	return count;
> -}
> -
> -static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, IDX_TEMP1_INPUT);
> -static SENSOR_DEVICE_ATTR_RW(temp1_min, temp, IDX_TEMP1_MIN);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max, temp, IDX_TEMP1_MAX);
> -static SENSOR_DEVICE_ATTR_RW(temp1_crit, temp, IDX_TEMP1_CRIT);
> -static SENSOR_DEVICE_ATTR_RO(temp1_min_alarm, temp_alarm, IDX_TEMP1_MIN);
> -static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, temp_alarm, IDX_TEMP1_MAX);
> -static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, temp_alarm, IDX_TEMP1_CRIT);
> -static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, IDX_TEMP2_INPUT);
> -static SENSOR_DEVICE_ATTR_RW(temp2_min, temp, IDX_TEMP2_MIN);
> -static SENSOR_DEVICE_ATTR_RW(temp2_max, temp, IDX_TEMP2_MAX);
> -static SENSOR_DEVICE_ATTR_RW(temp2_crit, temp, IDX_TEMP2_CRIT);
> -static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp2_fault, 0);
> -static SENSOR_DEVICE_ATTR_RO(temp2_min_alarm, temp_alarm, IDX_TEMP2_MIN);
> -static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, temp_alarm, IDX_TEMP2_MAX);
> -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);
> -
> -static SENSOR_DEVICE_ATTR_RW(pwm1, pwm1, 0);
> -static SENSOR_DEVICE_ATTR_RW(pwm1_enable, pwm1_enable, 0);
>   static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point1_pwm, pwm1_auto_point_pwm, 0);
>   static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_pwm, pwm1_auto_point_pwm, 1);
>   static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point3_pwm, pwm1_auto_point_pwm, 2);
> -static SENSOR_DEVICE_ATTR_RO(pwm1_auto_channels_temp, pwm1_auto_channels_temp,
> -			     0);
>   static SENSOR_DEVICE_ATTR_2_RO(temp1_auto_point1_temp, temp_auto_point_temp,
>   			       1, 0);
>   static SENSOR_DEVICE_ATTR_2_RW(temp1_auto_point2_temp, temp_auto_point_temp,
> @@ -718,30 +668,6 @@ static SENSOR_DEVICE_ATTR_2_RW(temp2_auto_point3_temp, temp_auto_point_temp,
>   			       2, 2);
>   
>   static struct attribute *amc6821_attrs[] = {
> -	&sensor_dev_attr_temp1_input.dev_attr.attr,
> -	&sensor_dev_attr_temp1_min.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max.dev_attr.attr,
> -	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> -	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp2_input.dev_attr.attr,
> -	&sensor_dev_attr_temp2_min.dev_attr.attr,
> -	&sensor_dev_attr_temp2_max.dev_attr.attr,
> -	&sensor_dev_attr_temp2_crit.dev_attr.attr,
> -	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp2_fault.dev_attr.attr,
> -	&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,
> -	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
> -	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
>   	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
>   	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
>   	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
> @@ -753,13 +679,117 @@ static struct attribute *amc6821_attrs[] = {
>   	&sensor_dev_attr_temp2_auto_point3_temp.dev_attr.attr,
>   	NULL
>   };
> -
>   ATTRIBUTE_GROUPS(amc6821);
>   
> +static int amc6821_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return amc6821_temp_read(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return amc6821_fan_read(dev, attr, val);
> +	case hwmon_pwm:
> +		return amc6821_pwm_read(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int amc6821_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return amc6821_temp_write(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return amc6821_fan_write(dev, attr, val);
> +	case hwmon_pwm:
> +		return amc6821_pwm_write(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t amc6821_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_min_alarm:
> +		case hwmon_temp_max_alarm:
> +		case hwmon_temp_crit_alarm:
> +		case hwmon_temp_fault:
> +			return 0444;
> +		case hwmon_temp_min:
> +		case hwmon_temp_max:
> +		case hwmon_temp_crit:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_fault:
> +			return 0444;
> +		case hwmon_fan_pulses:
> +		case hwmon_fan_min:
> +		case hwmon_fan_max:
> +		case hwmon_fan_target:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_enable:
> +		case hwmon_pwm_input:
> +			return 0644;
> +		case hwmon_pwm_auto_channels_temp:
> +			return 0444;
> +		default:
> +			return 0;
> +		}
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct hwmon_channel_info * const amc6821_info[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> +			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
> +			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> +			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
> +			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_FAULT),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX |
> +			   HWMON_F_TARGET | HWMON_F_PULSES | HWMON_F_FAULT),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE |
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP),
> +	NULL
> +};
> +
> +static const struct hwmon_ops amc6821_hwmon_ops = {
> +	.is_visible = amc6821_is_visible,
> +	.read = amc6821_read,
> +	.write = amc6821_write,
> +};
> +
> +static const struct hwmon_chip_info amc6821_chip_info = {
> +	.ops = &amc6821_hwmon_ops,
> +	.info = amc6821_info,
> +};
> +
>   /* Return 0 if detection is successful, -ENODEV otherwise */
> -static int amc6821_detect(
> -		struct i2c_client *client,
> -		struct i2c_board_info *info)
> +static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info)
>   {
>   	struct i2c_adapter *adapter = client->adapter;
>   	int address = client->addr;
> @@ -872,7 +902,6 @@ static int amc6821_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> -

Should be squashed with the previous commit.

Nice clean-up albeit very difficult to review in patch form, not sure 
there's anything we can do about it though. Maybe migrating one 
attribute at a time, but I would myself not be very happy if I was asked 
to do it :)

Cheers,
Quentin

Re: [PATCH 10/10] hwmon: (amc6821) Convert to with_info API

[Guenter Roeck]

Hi uentin,

On 7/1/24 10:46, Quentin Schulz wrote:

>>           return err;
>> -    return sysfs_emit(buf, "%d\n", !!(regval & mask));
>> +    *val = (int8_t)regval * 1000;
> 
> The discussed signed_extended32() in another patch would need to make it here too.
> 
Yes. Thanks for the reminder, I had missed that one.

>> -    return sprintf(buf, "%d\n", val);
>> +    err = regmap_bulk_read(regmap, reg, regs, 2);
>> +    if (err)
>> +        return err;
>> +
>> +    regval = (regs[1] << 8) | regs[0];
>> +    *val = 6000000 / (regval ? : 1);
>> +
> 
> Just putting a "bookmark" here since we're having a discussion about this logic in another thread for another patch.
> 
> Also... missing 0 here between the question mark and the colon?
> 
It translates to
	 *val = 6000000 / (regval ? regval : 1);
which is what I had wanted (divide by regval if regval != 0,
otherwise divide by 1 [i.e., return 6000000]).

I have it now as
	*val = regval ? 6000000 / regval : 0;

>> +static int amc6821_fan_write(struct device *dev, u32 attr, long val)
>> +{
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    struct regmap *regmap = data->regmap;
>> +    u8 regs[2];
>> +    int reg;
>> +
>> +    if (attr == hwmon_fan_pulses) {
>> +        if (val != 2 && val != 4)
>> +            return -EINVAL;
>> +        return regmap_update_bits(regmap, AMC6821_REG_CONF4,
>> +                     AMC6821_CONF4_PSPR,
>> +                     val == 4 ? AMC6821_CONF4_PSPR : 0);
>> +    }
>> +
>> +    if (val < 0)
>> +        return -EINVAL;
>> +
>> +    val = clamp_val(6000000 / (val ? : 1), 0, 0xffff);
>> +
> 
> And another "bookmark" here.
> 

That is now
	val = val ? 6000000 / clamp_val(val, 1, 6000000) : 0;
         val = clamp_val(val, 0, 0xffff);

'val' is checked earlier and must be > 0 for the minimum
and target fan speed.
	
>> +    switch (attr) {
>> +    case hwmon_fan_min:
>> +        reg = AMC6821_REG_TACH_LLIMITL;
>> +        break;
>> +    case hwmon_fan_max:
>> +        reg = AMC6821_REG_TACH_HLIMITL;
>> +        break;
>> +    case hwmon_fan_target:
>> +        reg = AMC6821_REG_TACH_SETTINGL;
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    regs[0] = val & 0xff;
>> +    regs[1] = val >> 8;
>> +
>> +    return regmap_bulk_write(data->regmap, reg, regs, 2);
>>   }
>>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>>                        struct device_attribute *devattr,
>>                        char *buf)
>>   {
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr_2(devattr)->index;
>>       int nr = to_sensor_dev_attr_2(devattr)->nr;
>> -    struct amc6821_data *data = dev_get_drvdata(dev);
> 
> Should be squashed with the appropriate commit?
> 

Yes, the previous one.

>>       switch (nr) {
>>       case 1:
>> @@ -423,7 +495,6 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
>>           return sprintf(buf, "%d\n",
>>               data->temp2_auto_point_temp[ix] * 1000);
>>       default:
>> -        dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
> 
> Not sure this is related? Maybe a separate commit?
> 

It should have been part of the previous commit, where it is explained ("remove
spurious debug messages which would only be seen as result of a bug in the code")

>> @@ -872,7 +902,6 @@ static int amc6821_probe(struct i2c_client *client)
>>       if (!data)
>>           return -ENOMEM;
>> -
> 
> Should be squashed with the previous commit.
> 
Ok.

> Nice clean-up albeit very difficult to review in patch form, not sure there's anything we can do about it though. Maybe migrating one attribute at a time, but I would myself not be very happy if I was asked to do it :)
> 

Yes, that is always the problem with the conversion to the with_info API.
I don't think it would make much sense to try to split it further.

Thanks a lot for the detailed review!

Guenter