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

From: Quentin Schulz
Date: Mon Jul 01 2024 - 12:55:12 EST

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