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], ®val);
+ 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, ®val);
+ 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
