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
