Add Richtek RTQ6056 support

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +26 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +6 -0

+    description: Shunt IN+/IN- sensing node resistor in microohm.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg

Quoted 47 lines

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +31 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:
+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 36

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +5 -0

+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:

Use standard properties, so "-micro-ohms". Drop the unit from description and drop the ref.

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +10 -0

+    description: Shunt IN+/IN- sensing node resistor in microohm.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - "#io-channel-cells"
+
+additionalProperties: false
+

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +6 -0

+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      rtq6056@40 {

Generic node name, so probably "adc" (or something else appropriate).

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 57

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +5 -0

+        compatible = "richtek,rtq6056";
+        reg = <0x40>;
+        #io-channel-cells = <1>;
+      };
+    };

Best regards,
Krzysztof

Krzysztof Kozlowski <[email protected]> 於 2022年6月18日 週六 上午9:45寫道：

Quoted 49 lines · cy_huang

Quoted 47 lines

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +31 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:
+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 36

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +5 -0

+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:

Use standard properties, so "-micro-ohms". Drop the unit from description and drop the ref.

Use richtek.shunt-resistor-micro-ohms?

Or the prefix 'richtek,' can be removed?

Quoted 59 lines · ChiYuan Huang

Krzysztof Kozlowski <[email protected]> 於 2022年6月18日 週六 上午9:45寫道：

Quoted 58 lines · cy_huang

Quoted 42 lines

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +26 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 36

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +10 -0

+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:  

Use standard properties, so "-micro-ohms". Drop the unit from description and drop the ref.

Quoted 1 line

Use richtek.shunt-resistor-micro-ohms?

Yes.

Quoted 1 line

Or the prefix 'richtek,' can be removed?

No, the vendor prefix must stay.

Best regards,
Krzysztof

Quoted 62 lines · On Sat, 18 Jun 2022 19:00:19 +0200 Krzysztof Kozlowski…

Quoted 60 lines · ChiYuan Huang

Krzysztof Kozlowski <[email protected]> 於 2022年6月18日 週六 上午9:45寫道：

Quoted 49 lines · cy_huang

Quoted 47 lines

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +31 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:
+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 36

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +5 -0

+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:    

Use standard properties, so "-micro-ohms". Drop the unit from description and drop the ref.

Use richtek.shunt-resistor-micro-ohms?

Quoted 5 lines

Yes.

Quoted 1 line

Or the prefix 'richtek,' can be removed?

No, the vendor prefix must stay.

It's a standard generic binding used in a bunch of IIO and hwmon drivers without vendor prefix. Mind you I can't find a definition

in any of the top level binding files. Probably wants to be in adc.yaml for IIO.

Jonathan

Quoted 3 lines

Best regards,
Krzysztof

Quoted 64 lines · On Sat, 18 Jun 2022 19:00:19 +0200 Krzysztof Kozlowski…

Quoted 59 lines · ChiYuan Huang

Krzysztof Kozlowski <[email protected]> 於 2022年6月18日 週六 上午9:45寫道：

Quoted 49 lines · cy_huang

Quoted 47 lines

From: ChiYuan Huang <[email protected]>

Add the documentation for Richtek RTQ6056.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +31 -0

new file mode 100644
index 00000000..6b4e0e0
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <[email protected]>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:
+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml line 36

Quoted 5 lines

Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml new file +5 -0

+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:  

Use standard properties, so "-micro-ohms". Drop the unit from description and drop the ref.

Quoted 1 line

Use richtek.shunt-resistor-micro-ohms?

Yes.

Quoted 3 lines

Quoted 1 line

Or the prefix 'richtek,' can be removed?

No, the vendor prefix must stay.

It's a standard generic binding used in a bunch of IIO and hwmon drivers without vendor prefix. Mind you I can't find a definition in any of the top level binding files. Probably wants to be in adc.yaml for IIO.

From: ChiYuan Huang <[email protected]>

Add Richtek RTQ6056 supporting.

It can be used for the system to monitor load current and power with 16bit

resolution.

+KernelVersion: 5.18.2

+#include <linux/of.h>

+#define RTQ6056_DEFAULT_RSHUNT 2000

+};

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+       struct regmap_field *rm_fields[F_MAX_FIELDS];
+       u32 shunt_resistor_uohm;
+       int vshuntct; /* vshunt conversion time in uS */
+       int vbusct; /* vbus conversion time in uS */
+       int avg_sample;
+};

+ IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)

+};

+}

+

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}

Quoted 6 lines

From: ChiYuan Huang <[email protected]>

Add Richtek RTQ6056 supporting.

It can be used for the system to monitor load current and power with 16bit

16-bit

Quoted 1 line

resolution.

Overall looks good, needs some cosmetic work.

...

Quoted 1 line

+KernelVersion: 5.18.2

Wrong version, this won't be part of a stable kernel.

...

Quoted 1 line

+#include <linux/of.h>

Any users of this?

But for sure you missed

mod_devicetable.h
types.h

...

Quoted 1 line

+#define RTQ6056_DEFAULT_RSHUNT 2000

_mOHMs ?

...

drivers/iio/adc/rtq6056-adc.c line 41

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+enum {
+       F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
+       F_MAX_FIELDS

Hard to read this way. Split to be one emum entry per line.

Quoted 1 line

+};

...

drivers/iio/adc/rtq6056-adc.c line 46

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+struct rtq6056_priv {
+       struct device *dev;
+       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have you run bloat-o-meter?

drivers/iio/adc/rtq6056-adc.c line 52

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+       struct regmap_field *rm_fields[F_MAX_FIELDS];
+       u32 shunt_resistor_uohm;
+       int vshuntct; /* vshunt conversion time in uS */
+       int vbusct; /* vbus conversion time in uS */
+       int avg_sample;
+};

...

Quoted 1 line

+ IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)

Keep a comma.

...

drivers/iio/adc/rtq6056-adc.c line 146

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+       /* Only power and vbus channel is unsigned */
+       if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
+               *val = regval;
+       else
 
+               *val = (s16)regval;

Why casting? At very minimum this requires a comment.

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+       if (val > 8205 || val < 139)
+               return -EINVAL;

This strange range requires a good comment with possible references to the datasheet.

...

drivers/iio/adc/rtq6056-adc.c line 208

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+static const int rtq6056_avg_sample_list[] = {
+       1, 4, 16, 64, 128, 256, 512, 1024

Keep a comma at the end.

Quoted 1 line

+};

...

drivers/iio/adc/rtq6056-adc.c line 298

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
+                                 struct iio_chan_spec const *chan,
+                                 char *label)
+{
+       return snprintf(label, PAGE_SIZE, "%s\n",
+                       rtq6056_channel_labels[chan->channel]);

sysfs_emit()

Quoted 1 line

+}

...

drivers/iio/adc/rtq6056-adc.c line 356

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+static IIO_DEVICE_ATTR(shunt_resistor, 0644,
+                      rtq6056_shunt_resistor_show,
+                      rtq6056_shunt_resistor_store, 0);

IIO_DEVICE_ATTR_RW()

...

drivers/iio/adc/rtq6056-adc.c line 390

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       for_each_set_bit(bit, indio_dev->active_scan_mask,
+                        indio_dev->masklength) {

On one line it's better.

Quoted 1 line

+

Redundant blank line.

Quoted 7 lines

drivers/iio/adc/rtq6056-adc.c new file +7 -0

+               ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
+                                 &raw);
+               if (ret)
+                       goto out;
+
+               data.vals[i++] = raw;
+       }

drivers/iio/adc/rtq6056-adc.c line 488

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+       ret = of_property_read_u32(i2c->dev.of_node,
+                                  "richtek,shunt-resistor-uohm",
+                                  &shunt_resistor_uohm);

device_property_read()

Review

Quoted 2 lines

(quoted diff) +2 -0

+       if (ret)
+               shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;

Can be done without branch

... = DEFAULT; device_property_read_u32(...); // no error checking.

...

drivers/iio/adc/rtq6056-adc.c line 517

Quoted 5 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+static int rtq6056_remove(struct i2c_client *i2c)
+{
+       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+
+       /* Config opmode to 'shutdown' mode to minimize quiescient current */

quiescent

drivers/iio/adc/rtq6056-adc.c line 525

Quoted 8 lines

drivers/iio/adc/rtq6056-adc.c new file +8 -0

+       return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}
+
+static void rtq6056_shutdown(struct i2c_client *i2c)
+{
+       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+
+       /* Config opmode to 'shutdown' mode to minimize quiescient current */

quiescent

drivers/iio/adc/rtq6056-adc.c line 527

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko <[email protected]> 於 2022年6月18日 週六 凌晨1:08寫道：

Quoted 1 line · cy_huang

Quoted 4 lines

Quoted 1 line

+KernelVersion: 5.18.2

Wrong version, this won't be part of a stable kernel.

From kernel.org, currently the stable kernel version is 5.18.5. Change to 5.18.5?

Quoted 4 lines

But for sure you missed

types.h

Ack in next. But for types.h, i2c.h already include device.h. And device.h already include types.h.
Is it still needed to declare explicitly for types.h??

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c line 46

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       struct device *dev;
+       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have you run bloat-o-meter?

I never know about this tool.
I'll check it before I submit the next revision.
Thanks for the reminding.

But from Jonathan's reply, I may remove 'struct regmap *regmap'. If all function need the 'regmap', a local variable 'regmap' need to be declared.
To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.

Quoted 2 lines

Andy Shevchenko <[email protected]> 於 2022年6月18日 週六 凌晨1:08寫道：

Quoted 1 line · cy_huang

...

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

+KernelVersion: 5.18.2

Wrong version, this won't be part of a stable kernel.

From kernel.org, currently the stable kernel version is 5.18.5. Change to 5.18.5?

Nope. You need to use realistic kernel version, and as I said it can't be a stable one.

...

Quoted 7 lines

Quoted 4 lines

But for sure you missed

types.h

Ack in next. But for types.h, i2c.h already include device.h. And device.h already include types.h.
Is it still needed to declare explicitly for types.h??

Yes. You have to include all headers you are a direct user of, except the ones that are guaranteed to be included by others. The types.h is not guaranteed to be included by listed above.

...

Quoted 10 lines

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c line 46

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       struct device *dev;
+       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have you run bloat-o-meter?

I never know about this tool.
I'll check it before I submit the next revision.
Thanks for the reminding.

Quoted 4 lines

But from Jonathan's reply, I may remove 'struct regmap *regmap'. If all function need the 'regmap', a local variable 'regmap' need to be declared.
To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.

It's fine, but you may experiment with bloat-o-meter even in that case out of curiosity.

--
With Best Regards,
Andy Shevchenko

Andy Shevchenko <[email protected]> 於 2022年6月19日 週日 下午6:38寫道：

Quoted 3 lines · ChiYuan Huang

Quoted 2 lines

Andy Shevchenko <[email protected]> 於 2022年6月18日 週六 凌晨1:08寫道：

Quoted 1 line · cy_huang

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c line 46

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+       struct device *dev;
+       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have you run bloat-o-meter?

I never know about this tool.
I'll check it before I submit the next revision.
Thanks for the reminding.

Quoted 8 lines

Quoted 4 lines

But from Jonathan's reply, I may remove 'struct regmap *regmap'. If all function need the 'regmap', a local variable 'regmap' need to be declared.
To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.

It's fine, but you may experiment with bloat-o-meter even in that case out of curiosity.

I tred to only swap these two line for *dev and *regmap. Check the below two cases
1. bloat-o-meter with rtq6056 as the builtin add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Function old new delta
Total: Before=24428680, After=24428680, chg +0.00%
2. size tool with rtq6056 as the kernel build

   text    data     bss     dec     hex filename
   5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko.old
   text    data     bss     dec     hex filename
   5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko

It's weird that there's no difference.

Do I misunderstand something?

From: ChiYuan Huang <[email protected]>

Add Richtek RTQ6056 supporting.

It can be used for the system to monitor load current and power with 16bit resolution.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |  58 +++
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rtq6056-adc.c                      | 548 +++++++++++++++++++++
 4 files changed, 622 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
 create mode 100644 drivers/iio/adc/rtq6056-adc.c

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..0516429

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +29 -0

+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		Shunt IN +/- sensing range from -82mV to +81.9175mV
+		Calculating with scale (2.5 uV)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_raw
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		BUS voltage sensing range from 0V to 36V.
+		Calculating with scale (1.25 mV)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_power2_raw
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		Power loading that equals to bus voltage multiple loading
+		current.
+		Calculating with scale (25 mWatt)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_current3_raw
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		Consuming current from bus voltage.
+		Directly report loading current in mA
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +18 -0

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_oversampling_ratio
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		The number of average sample
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_sampling_frequency
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		Sampling frequency in HZ for power and current report
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_time_available
+KernelVersion:	5.18.2
+Contact:	[email protected]
+Description:
+		Sample conversion time available for BUS and Shunt, unit in second

drivers/iio/adc/Kconfig +15 -0

index 48ace74..0b2d17c 100644
@@ -908,6 +908,21 @@ config ROCKCHIP_SARADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rockchip_saradc.
 
+config RICHTEK_RTQ6056_ADC
+	tristate "Richtek RTQ6056 Current and Power Monitor ADC"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to enable RQT6056 ADC support.
+	  RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+	  compatible interface, and the device provides full information for
+	  system by reading out the load current and power.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called rtq6056-adc.
+
 config RZG2L_ADC
 	tristate "Renesas RZ/G2L ADC driver"
 	depends on ARCH_RZG2L || COMPILE_TEST

drivers/iio/adc/Makefile +0 -0

index 39d806f..e8c6e6e 100644

drivers/iio/adc/Makefile +0 -0

 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o

drivers/iio/adc/rtq6056-adc.c new file +0 -0

new file mode 100644
index 00000000..8374fce

drivers/iio/adc/rtq6056-adc.c new file +9 -0

+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+

drivers/iio/adc/rtq6056-adc.c new file +28 -0

+#define RTQ6056_DEFAULT_RSHUNT	2000
+
+enum {
+	RTQ6056_CH_VSHUNT = 0,
+	RTQ6056_CH_VBUS,
+	RTQ6056_CH_POWER,
+	RTQ6056_CH_CURRENT,
+	RTQ6056_MAX_CHANNEL
+};
+
+enum {
+	F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
+	F_MAX_FIELDS
+};
+
+struct rtq6056_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_field *rm_fields[F_MAX_FIELDS];
+	u32 shunt_resistor_uohm;
+	int vshuntct; /* vshunt conversion time in uS */
+	int vbusct; /* vbus conversion time in uS */
+	int avg_sample;
+};
+
+static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
+	[F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
+	[F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),

drivers/iio/adc/rtq6056-adc.c new file +65 -0

+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = RTQ6056_CH_VBUS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_POWER,
+		.indexed = 1,
+		.channel = RTQ6056_CH_POWER,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_CURRENT,
+		.indexed = 1,
+		.channel = RTQ6056_CH_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
+};
+
+static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, int channel,
+				    int *val)
+{
+	unsigned int reg = RTQ6056_REG_SHUNTVOLT + channel;
+	unsigned int regval;
+	int ret;

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+
+	return IIO_VAL_INT;
+}
+

drivers/iio/adc/rtq6056-adc.c new file +7 -0

+
+static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
+{
+	unsigned int selector;
+	int ret;
+
+	if (val > 1024 || val < 1)

drivers/iio/adc/rtq6056-adc.c new file +9 -0

+	return 0;
+}
+
+static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
+{
+	int sample_time;
+
+	sample_time = priv->vshuntct + priv->vbusct;
+	sample_time *= priv->avg_sample;
 

drivers/iio/adc/rtq6056-adc.c new file +21 -0

+	[RTQ6056_CH_VBUS] = "Vbus(mV)",
+	[RTQ6056_CH_POWER] = "Power(mW)",
+	[RTQ6056_CH_CURRENT] = "Current(mA)",
+};
+
+static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  char *label)
+{
+	return snprintf(label, PAGE_SIZE, "%s\n",
+			rtq6056_channel_labels[chan->channel]);
+}
+
+static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
+				      int resistor_uohm)
+{
+	unsigned int calib_val;
+	int ret;
+
+	if (resistor_uohm <= 0) {
+		dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+	return 0;
+}

drivers/iio/adc/rtq6056-adc.c new file +14 -0

+	&iio_dev_attr_shunt_resistor.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group rtq6056_attribute_group = {
+	.attrs = rtq6056_attributes,
+};
+
+static const struct iio_info rtq6056_info = {
+	.attrs = &rtq6056_attribute_group,
+	.read_raw = rtq6056_adc_read_raw,
+	.write_raw = rtq6056_adc_write_raw,
+	.read_label = rtq6056_adc_read_label,
+};

drivers/iio/adc/rtq6056-adc.c new file +11 -0

+static int rtq6056_probe(struct i2c_client *i2c)
+{
+	struct iio_dev *indio_dev;
+	struct rtq6056_priv *priv;
+	unsigned int vendor_id, shunt_resistor_uohm;
+	int ret;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));

drivers/iio/adc/rtq6056-adc.c new file +13 -0

+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&i2c->dev, PTR_ERR(priv->regmap),
+				     "Failed to init regmap\n");
+
+	ret = regmap_read(priv->regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to get manufacturer info\n");
+
+	if (vendor_id != RTQ6056_VENDOR_ID)
+		return dev_err_probe(&i2c->dev, -ENODEV,
+				     "invalid vendor id 0x%04x\n", vendor_id);
+

drivers/iio/adc/rtq6056-adc.c new file +26 -0

+
+	ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
+	if (ret)
+		dev_err_probe(&i2c->dev, ret,
+			      "Failed to init shunt resistor\n");
+
+	indio_dev->name = "rtq6056";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = rtq6056_channels;
+	indio_dev->num_channels = ARRAY_SIZE(rtq6056_channels);
+	indio_dev->info = &rtq6056_info;
+
+	ret = devm_iio_triggered_buffer_setup(&i2c->dev, indio_dev, NULL,
+					      rtq6056_buffer_trigger_handler,
+					      NULL);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to allocate iio trigger buffer\n");
+
+	return devm_iio_device_register(&i2c->dev, indio_dev);
+}
+
+static int rtq6056_remove(struct i2c_client *i2c)
+{
+	struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+

drivers/iio/adc/rtq6056-adc.c new file +22 -0

+}
+
+static const struct of_device_id rtq6056_device_match[] = {
+	{ .compatible = "richtek,rtq6056", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtq6056_device_match);
+
+static struct i2c_driver rtq6056_driver = {
+	.driver = {
+		.name = "rtq6056",
+		.of_match_table = rtq6056_device_match,
+	},
+	.probe_new = rtq6056_probe,
+	.remove = rtq6056_remove,
+	.shutdown = rtq6056_shutdown,
+};
+module_i2c_driver(rtq6056_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <[email protected]>");
+MODULE_DESCRIPTION("Richtek RTQ6056 ADC Driver");
+MODULE_LICENSE("GPL v2");

Quoted 8 lines · On Fri, 17 Jun 2022 17:32:55 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

Add Richtek RTQ6056 supporting.

It can be used for the system to monitor load current and power with 16bit resolution.

Signed-off-by ChiYuan Huang <[email protected]>

Hi ChiYuan,

Comments inline.

It would be nice to consider how to take advantage of the fact we know which channels are of interest in buffered mode, and change the operating mode to suite.

Quoted 9 lines

---
 .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |  58 +++
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rtq6056-adc.c                      | 548 +++++++++++++++++++++
 4 files changed, 622 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
 create mode 100644 drivers/iio/adc/rtq6056-adc.c

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 1

Quoted 7 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +1 -0

new file mode 100644
index 00000000..0516429
@@ -0,0 +1,58 @@
+What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_raw

Documentation can't be duplicated in mulitple files so these standard attributes should rely only on the main docs. If we duplicate it breaks building the html docs unfortunately.

Quoted 30 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +29 -0

+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Shunt IN +/- sensing range from -82mV to +81.9175mV
+             Calculating with scale (2.5 uV)
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_voltage1_raw
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             BUS voltage sensing range from 0V to 36V.
+             Calculating with scale (1.25 mV)
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_power2_raw
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Power loading that equals to bus voltage multiple loading
+             current.
+             Calculating with scale (25 mWatt)
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_current3_raw
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Consuming current from bus voltage.
+             Directly report loading current in mA
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 40

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Shunt voltage conversion time in uS
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             BUS voltage conversion time in uS

integration time is not normally about conversion time. It's normally for things like light sensors where they are charging up a capacitor for a fixed time period.

For ADC channels we tend to use _sampling_frequency (so 1/period). An example of this being per channel is ad7124.

That does mean we tend not to provide the overall 'sampling_frequency' on these devices, but rather let an interested userspace program figure it out - often it depends on which channels are enabled.

Quoted 49 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +18 -0

+
+What:                /sys/bus/iio/devices/iio:deviceX/in_oversampling_ratio
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             The number of average sample
+
+What:                /sys/bus/iio/devices/iio:deviceX/in_sampling_frequency
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Sampling frequency in HZ for power and current report
+
+What:                /sys/bus/iio/devices/iio:deviceX/integration_time_available
+KernelVersion:       5.18.2
+Contact:     [email protected]
+Description:
+             Sample conversion time available for BUS and Shunt, unit in second

drivers/iio/adc/Kconfig +15 -0

index 48ace74..0b2d17c 100644
@@ -908,6 +908,21 @@ config ROCKCHIP_SARADC
        To compile this driver as a module, choose M here: the
        module will be called rockchip_saradc.
 
+config RICHTEK_RTQ6056_ADC
+     tristate "Richtek RTQ6056 Current and Power Monitor ADC"
+     depends on I2C
+     select REGMAP_I2C
+     select IIO_BUFFER
+     select IIO_TRIGGERED_BUFFER
+     help
+       Say yes here to enable RQT6056 ADC support.
+       RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+       compatible interface, and the device provides full information for
+       system by reading out the load current and power.
+
+       This driver can also be built as a module. If so, the module will be
+       called rtq6056-adc.
+
 config RZG2L_ADC
      tristate "Renesas RZ/G2L ADC driver"
      depends on ARCH_RZG2L || COMPILE_TEST

drivers/iio/adc/Makefile +0 -0

index 39d806f..e8c6e6e 100644
@@ -84,6 +84,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o

drivers/iio/adc/Makefile obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o · line 87

Quoted 4 lines

drivers/iio/adc/Makefile +1 -0

 obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_RICHTEK_RTQ6056_ADC) += rtq6056-adc.o

Is the device anything other than ADC? If not, drop the -adc. The other entries you see here are ADCs that form part of a much larger SoC.

Quoted 11 lines

drivers/iio/adc/Makefile +0 -0

 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o

drivers/iio/adc/rtq6056-adc.c new file +2 -0

new file mode 100644
index 00000000..8374fce
@@ -0,0 +1,548 @@
+// SPDX-License-Identifier: GPL-2.0+
+

drivers/iio/adc/rtq6056-adc.c line 7

Quoted 5 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>

property.h and mod_devicetable.h instead of of.h once you have moved to generic firmware properties.

Quoted 14 lines

drivers/iio/adc/rtq6056-adc.c new file +14 -0

+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define RTQ6056_REG_CONFIG   0x00
+#define RTQ6056_REG_SHUNTVOLT        0x01
+#define RTQ6056_REG_CALIBRATION      0x05
+#define RTQ6056_REG_MASKENABLE       0x06
+#define RTQ6056_REG_ALERTLIMIT       0x07

drivers/iio/adc/rtq6056-adc.c line 26

Quoted 5 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+#define RTQ6056_REG_MANUFACTID       0xFE
+#define RTQ6056_REG_DIEID    0xFF
+
+#define RTQ6056_VENDOR_ID    0x1214
+#define RTQ6056_DEFAULT_CONFIG       0x4127

This is not defined in terms of the field provided below so it's not obvious what the initial state is.

drivers/iio/adc/rtq6056-adc.c line 28

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+#define RTQ6056_DEFAULT_CONVT        1037
+#define RTQ6056_DEFAULT_AVG  1

This and the next are real world values. Having them as defines makes the code less readable.

Quoted 28 lines

drivers/iio/adc/rtq6056-adc.c new file +28 -0

+#define RTQ6056_DEFAULT_RSHUNT       2000
+
+enum {
+     RTQ6056_CH_VSHUNT = 0,
+     RTQ6056_CH_VBUS,
+     RTQ6056_CH_POWER,
+     RTQ6056_CH_CURRENT,
+     RTQ6056_MAX_CHANNEL
+};
+
+enum {
+     F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
+     F_MAX_FIELDS
+};
+
+struct rtq6056_priv {
+     struct device *dev;
+     struct regmap *regmap;
+     struct regmap_field *rm_fields[F_MAX_FIELDS];
+     u32 shunt_resistor_uohm;
+     int vshuntct; /* vshunt conversion time in uS */
+     int vbusct; /* vbus conversion time in uS */
+     int avg_sample;
+};
+
+static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
+     [F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
+     [F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),

drivers/iio/adc/rtq6056-adc.c line 66

Quoted 10 lines

drivers/iio/adc/rtq6056-adc.c new file +10 -0

+     [F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 6, 8),
+     [F_AVG] = REG_FIELD(RTQ6056_REG_CONFIG, 9, 11),
+     [F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15)
+};
+
+static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
+     {
+             .type = IIO_VOLTAGE,
+             .indexed = 1,
+             .channel = RTQ6056_CH_VSHUNT,

Where we have a bunch of different types of channel we generally count the channel numbers separately. Where this is only one of a give type, don't bother making it indexed.

Where you need an offset to use for accessing an actual register, the .address field is more appropriate.

drivers/iio/adc/rtq6056-adc.c line 71

Quoted 5 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+                                   BIT(IIO_CHAN_INFO_SCALE) |
+                                   BIT(IIO_CHAN_INFO_INT_TIME),
+             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),

Why _by_dir as opposed to by_all given there are no output channels registered?

Quoted 70 lines

drivers/iio/adc/rtq6056-adc.c new file +69 -0

+             .scan_index = 0,
+             .scan_type = {
+                     .sign = 's',
+                     .realbits = 16,
+                     .storagebits = 16,
+                     .endianness = IIO_CPU,
+             },
+     },
+     {
+             .type = IIO_VOLTAGE,
+             .indexed = 1,
+             .channel = RTQ6056_CH_VBUS,
+             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+                                   BIT(IIO_CHAN_INFO_SCALE) |
+                                   BIT(IIO_CHAN_INFO_INT_TIME),
+             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+             .scan_index = 1,
+             .scan_type = {
+                     .sign = 'u',
+                     .realbits = 16,
+                     .storagebits = 16,
+                     .endianness = IIO_CPU,
+             },
+     },
+     {
+             .type = IIO_POWER,
+             .indexed = 1,
+             .channel = RTQ6056_CH_POWER,
+             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+                                   BIT(IIO_CHAN_INFO_SCALE),
+             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+             .scan_index = 2,
+             .scan_type = {
+                     .sign = 'u',
+                     .realbits = 16,
+                     .storagebits = 16,
+                     .endianness = IIO_CPU,
+             },
+     },
+     {
+             .type = IIO_CURRENT,
+             .indexed = 1,
+             .channel = RTQ6056_CH_CURRENT,
+             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+             .scan_index = 3,
+             .scan_type = {
+                     .sign = 's',
+                     .realbits = 16,
+                     .storagebits = 16,
+                     .endianness = IIO_CPU,
+             },
+     },
+     IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
+};
+
+static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, int channel,
+                                 int *val)
+{
+     unsigned int reg = RTQ6056_REG_SHUNTVOLT + channel;
+     unsigned int regval;
+     int ret;
+
+     ret = regmap_read(priv->regmap, reg, &regval);
+     if (ret)
+             return ret;

drivers/iio/adc/rtq6056-adc.c line 146

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+
+     /* Only power and vbus channel is unsigned */
+     if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
+             *val = regval;
+     else
+             *val = (s16)regval;

Prefer making it clear this is sign extension with sign_extend32()

Quoted 4 lines

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+
+     return IIO_VAL_INT;
+}
+

...

Quoted 11 lines

drivers/iio/adc/rtq6056-adc.c new file +11 -0

+
+static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
+{
+     unsigned int selector;
+     int ret;
+
+     if (val > 1024 || val < 1)
+             return -EINVAL;
+
+     selector = find_closest(val, rtq6056_avg_sample_list,
+                             ARRAY_SIZE(rtq6056_avg_sample_list));

drivers/iio/adc/rtq6056-adc.c line 226

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+
+     ret = regmap_field_write(priv->rm_fields[F_AVG], selector);
+     if (ret)
+             return ret;
+
+     priv->avg_sample = rtq6056_avg_sample_list[selector];

Blank line - see below. Same for other similar cases.

Quoted 15 lines

drivers/iio/adc/rtq6056-adc.c new file +14 -0

+     return 0;
+}
+
+static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
+{
+     int sample_time;
+
+     sample_time = priv->vshuntct + priv->vbusct;
+     sample_time *= priv->avg_sample;
 
+
+     *val = DIV_ROUND_UP(1000000, sample_time);
+     return IIO_VAL_INT;
+}
+

drivers/iio/adc/rtq6056-adc.c line 287

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+
+static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
+     [RTQ6056_CH_VSHUNT] = "Vshunt(uV)",

Units must be the standard ones for the IIO channel types. Hence documenting them here is misleading.

Quoted 25 lines

drivers/iio/adc/rtq6056-adc.c new file +25 -0

+     [RTQ6056_CH_VBUS] = "Vbus(mV)",
+     [RTQ6056_CH_POWER] = "Power(mW)",
+     [RTQ6056_CH_CURRENT] = "Current(mA)",
+};
+
+static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
+                               struct iio_chan_spec const *chan,
+                               char *label)
+{
+     return snprintf(label, PAGE_SIZE, "%s\n",
+                     rtq6056_channel_labels[chan->channel]);
+}
+
+static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
+                                   int resistor_uohm)
+{
+     unsigned int calib_val;
+     int ret;
+
+     if (resistor_uohm <= 0) {
+             dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);
+             return -EINVAL;
+     }
+
+     /* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */

drivers/iio/adc/rtq6056-adc.c line 318

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+     calib_val = 5120000 / resistor_uohm;
+     ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
+     if (ret)
+             return ret;
+
+     priv->shunt_resistor_uohm = resistor_uohm;

Blank line before a simple return like this. It slightly helps readability.

Quoted 2 lines

drivers/iio/adc/rtq6056-adc.c new file +2 -0

+     return 0;
+}

...

drivers/iio/adc/rtq6056-adc.c line 359

Quoted 10 lines

drivers/iio/adc/rtq6056-adc.c new file +10 -0

+static IIO_CONST_ATTR_NAMED(rtq6056_conv_time_available,
+                         integration_time_available,
+                         "0.000139 0.000203 0.000269 0.000525 0.001037 0.002061 0.004109 0.008205");
+
+static IIO_DEVICE_ATTR(shunt_resistor, 0644,
+                    rtq6056_shunt_resistor_show,
+                    rtq6056_shunt_resistor_store, 0);
+
+static struct attribute *rtq6056_attributes[] = {
+     &iio_const_attr_rtq6056_conv_time_available.dev_attr.attr,

Whilst I think this particular interface will change anyway, you should use the read_avail callback for available attributes for standard ABI.

Quoted 14 lines

drivers/iio/adc/rtq6056-adc.c new file +14 -0

+     &iio_dev_attr_shunt_resistor.dev_attr.attr,
+     NULL
+};
+
+static const struct attribute_group rtq6056_attribute_group = {
+     .attrs = rtq6056_attributes,
+};
+
+static const struct iio_info rtq6056_info = {
+     .attrs = &rtq6056_attribute_group,
+     .read_raw = rtq6056_adc_read_raw,
+     .write_raw = rtq6056_adc_write_raw,
+     .read_label = rtq6056_adc_read_label,
+};

...

Quoted 17 lines

drivers/iio/adc/rtq6056-adc.c new file +17 -0

+static int rtq6056_probe(struct i2c_client *i2c)
+{
+     struct iio_dev *indio_dev;
+     struct rtq6056_priv *priv;
+     unsigned int vendor_id, shunt_resistor_uohm;
+     int ret;
+
+     if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+             return -EOPNOTSUPP;
+
+     indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
+     if (!indio_dev)
+             return -ENOMEM;
+
+     priv = iio_priv(indio_dev);
+     priv->dev = &i2c->dev;
+     priv->vshuntct = priv->vbusct = RTQ6056_DEFAULT_CONVT;

drivers/iio/adc/rtq6056-adc.c line 458

Quoted 4 lines

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+     priv->avg_sample = RTQ6056_DEFAULT_AVG;
+     i2c_set_clientdata(i2c, priv);
+
+     priv->regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);

You make a lot of use of regmap in this function. I would suggest first allocating into a local variable, then using that to set priv->regmap. The local variable can then be used avoid having priv->regmap everywhere.

Similar for dev. once a function has multiple accesses to i2c->dev, things are cleaner if you just have

struct device *dev = &i2c->dev; and use dev after that.

Quoted 13 lines

drivers/iio/adc/rtq6056-adc.c new file +13 -0

+     if (IS_ERR(priv->regmap))
+             return dev_err_probe(&i2c->dev, PTR_ERR(priv->regmap),
+                                  "Failed to init regmap\n");
+
+     ret = regmap_read(priv->regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
+     if (ret)
+             return dev_err_probe(&i2c->dev, ret,
+                                  "Failed to get manufacturer info\n");
+
+     if (vendor_id != RTQ6056_VENDOR_ID)
+             return dev_err_probe(&i2c->dev, -ENODEV,
+                                  "invalid vendor id 0x%04x\n", vendor_id);
+

drivers/iio/adc/rtq6056-adc.c line 481

Quoted 10 lines

drivers/iio/adc/rtq6056-adc.c new file +10 -0

+     ret = devm_regmap_field_bulk_alloc(&i2c->dev, priv->regmap,
+                                        priv->rm_fields, rtq6056_reg_fields,
+                                        F_MAX_FIELDS);
+     if (ret)
+             return dev_err_probe(&i2c->dev, ret,
+                                  "Failed to init regmap field\n");
+
+     /* Write the default config value */
+     ret = regmap_write(priv->regmap, RTQ6056_REG_CONFIG,
+                        RTQ6056_DEFAULT_CONFIG);

This write effectively turns the device on so it would be good to register a devm_add_action_or_reset() callback here to do what you currently have in remove. That will mean you don't need a separate remove at all and that you will put the device in a low power state if you get an error after this point.

At some stage it would be nice to do runtime pm with autosuspend so that we enter a low power state if no one is looking at the readings.

drivers/iio/adc/rtq6056-adc.c line 486

Quoted 5 lines

drivers/iio/adc/rtq6056-adc.c new file +5 -0

+     if (ret)
+             return dev_err_probe(&i2c->dev, ret,
+                                  "Failed to write default config\n");
+
+     ret = of_property_read_u32(i2c->dev.of_node,

Please use generic firmware properties not device tree specific ones. That lets people use other firmware interfaces such as ACPI using the PRP0001 HID.

drivers/iio/adc/rtq6056-adc.c line 490

Quoted 4 lines

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+                                "richtek,shunt-resistor-uohm",
+                                &shunt_resistor_uohm);
+     if (ret)
+             shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;

Flip this around

        shunt_resistor_uohm = 2000;
        device_property_read_u32(dev, "rictek,shunt-resistor-uohm",
                                 &shunt_resistor_uohm);

if the property read fails, it won't change the value stored.

Quoted 33 lines

drivers/iio/adc/rtq6056-adc.c new file +33 -0

+
+     ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
+     if (ret)
+             dev_err_probe(&i2c->dev, ret,
+                           "Failed to init shunt resistor\n");
+
+     indio_dev->name = "rtq6056";
+     indio_dev->modes = INDIO_DIRECT_MODE;
+     indio_dev->channels = rtq6056_channels;
+     indio_dev->num_channels = ARRAY_SIZE(rtq6056_channels);
+     indio_dev->info = &rtq6056_info;
+
+     ret = devm_iio_triggered_buffer_setup(&i2c->dev, indio_dev, NULL,
+                                           rtq6056_buffer_trigger_handler,
+                                           NULL);
+     if (ret)
+             return dev_err_probe(&i2c->dev, ret,
+                                  "Failed to allocate iio trigger buffer\n");
+
+     return devm_iio_device_register(&i2c->dev, indio_dev);
+}
+
+static int rtq6056_remove(struct i2c_client *i2c)
+{
+     struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+
+     /* Config opmode to 'shutdown' mode to minimize quiescient current */
+     return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}
+
+static void rtq6056_shutdown(struct i2c_client *i2c)
+{
+     struct rtq6056_priv *priv = i2c_get_clientdata(i2c);

drivers/iio/adc/rtq6056-adc.c line 526

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+
+     /* Config opmode to 'shutdown' mode to minimize quiescient current */
+     regmap_field_write(priv->rm_fields[F_OPMODE], 0);

Unusual to provide a shutdown for a device as simple as an ADC. I'd expect the overall system to power down if we hit this and hence also cover the ADC. If that's not the case, then perhaps add a comment explaining that.

Quoted 22 lines

drivers/iio/adc/rtq6056-adc.c new file +22 -0

+}
+
+static const struct of_device_id rtq6056_device_match[] = {
+     { .compatible = "richtek,rtq6056", },
+     {}
+};
+MODULE_DEVICE_TABLE(of, rtq6056_device_match);
+
+static struct i2c_driver rtq6056_driver = {
+     .driver = {
+             .name = "rtq6056",
+             .of_match_table = rtq6056_device_match,
+     },
+     .probe_new = rtq6056_probe,
+     .remove = rtq6056_remove,
+     .shutdown = rtq6056_shutdown,
+};
+module_i2c_driver(rtq6056_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <[email protected]>");
+MODULE_DESCRIPTION("Richtek RTQ6056 ADC Driver");
+MODULE_LICENSE("GPL v2");

Jonathan Cameron <[email protected]> 於 2022年6月18日 週六 凌晨1:10寫道：

Quoted 13 lines

Quoted 8 lines · On Fri, 17 Jun 2022 17:32:55 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

Add Richtek RTQ6056 supporting.

It can be used for the system to monitor load current and power with 16bit resolution.

Signed-off-by ChiYuan Huang <[email protected]>

Quoted 8 lines

Hi ChiYuan,

Comments inline.

It would be nice to consider how to take advantage of the fact we know which channels are of interest in buffered mode, and change the operating mode to suite.

I implement the buffered mode is for the debugging or continuous tracking purpose.

BUS channel is for input power source voltage
Shunt channel is for the cross voltage difference for Rshunt. Current channel is calculated by Vshunt/Rshunt and the value converted by HW. Power channel is calculated by BUS voltage multiple Current channel, and also the value converted by HW.

For the opmode question, this IC already designed for low quiescent. If IC's in continuous mode, the typical quiescent current is still around 550uA. 'Shutdown' mode only use typical 3.5uA.

Like as you said, I may consider to use pm_runtime plus autosuspend to minimize the wait time for the first new sample ready.

Quoted 10 lines

Quoted 10 lines

---
 .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |  58 +++
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rtq6056-adc.c                      | 548 +++++++++++++++++++++
 4 files changed, 622 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
 create mode 100644 drivers/iio/adc/rtq6056-adc.c

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 1

Quoted 7 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +1 -0

new file mode 100644
index 00000000..0516429
@@ -0,0 +1,58 @@
+What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_raw  

Documentation can't be duplicated in mulitple files so these standard attributes should rely only on the main docs. If we duplicate it breaks building the html docs unfortunately.

Does it mean there's no need to explain it here and covert this voltage to millivolt?

Quoted 9 lines

Quoted 8 lines

drivers/iio/adc/Kconfig +0 -0

index 48ace74..0b2d17c 100644

drivers/iio/adc/rtq6056-adc.c new file +0 -0

new file mode 100644
index 00000000..8374fce
@@ -0,0 +1,548 @@

Quoted 7 lines

drivers/iio/adc/rtq6056-adc.c line 26

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+
+#define RTQ6056_VENDOR_ID    0x1214
+#define RTQ6056_DEFAULT_CONFIG       0x4127  

This is not defined in terms of the field provided below so it's not obvious what the initial state is.

To leave a comment about the default state?
Or use the macro to define the all field?

Quoted 7 lines

drivers/iio/adc/rtq6056-adc.c line 287

Quoted 3 lines

drivers/iio/adc/rtq6056-adc.c new file +3 -0

+
+static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
+     [RTQ6056_CH_VSHUNT] = "Vshunt(uV)",  

Units must be the standard ones for the IIO channel types. Hence documenting them here is misleading.

Can I use the "extend_name' and remove all the 'read_label' and channel_label variable?

And also , the unit will be removed. Thx.

Quoted 22 lines

Quoted 4 lines

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+     [RTQ6056_CH_VBUS] = "Vbus(mV)",
+     [RTQ6056_CH_POWER] = "Power(mW)",
+     [RTQ6056_CH_CURRENT] = "Current(mA)",
+};

...

Quoted 17 lines

drivers/iio/adc/rtq6056-adc.c new file +17 -0

+static int rtq6056_probe(struct i2c_client *i2c)
+{
+     struct iio_dev *indio_dev;
+     struct rtq6056_priv *priv;
+     unsigned int vendor_id, shunt_resistor_uohm;
+     int ret;
+
+     if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+             return -EOPNOTSUPP;
+
+     indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
+     if (!indio_dev)
+             return -ENOMEM;
+
+     priv = iio_priv(indio_dev);
+     priv->dev = &i2c->dev;
+     priv->vshuntct = priv->vbusct = RTQ6056_DEFAULT_CONVT;

Quoted 9 lines

drivers/iio/adc/rtq6056-adc.c line 458

Quoted 4 lines

drivers/iio/adc/rtq6056-adc.c new file +4 -0

+     priv->avg_sample = RTQ6056_DEFAULT_AVG;
+     i2c_set_clientdata(i2c, priv);
+
+     priv->regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);  

You make a lot of use of regmap in this function. I would suggest first allocating into a local variable, then using that to set priv->regmap. The local variable can then be used avoid having priv->regmap everywhere.

dev_get_regmap(), instead?

Quoted 12 lines

drivers/iio/adc/rtq6056-adc.c line 526

Quoted 6 lines

drivers/iio/adc/rtq6056-adc.c new file +6 -0

+static void rtq6056_shutdown(struct i2c_client *i2c)
+{
+     struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+
+     /* Config opmode to 'shutdown' mode to minimize quiescient current */
+     regmap_field_write(priv->rm_fields[F_OPMODE], 0);  

Unusual to provide a shutdown for a device as simple as an ADC. I'd expect the overall system to power down if we hit this and hence also cover the ADC. If that's not the case, then perhaps add a comment explaining that.

That's because some application use VBAT as the VDD source. Not all applcation use the PMIC buck or ldo as the supply for RTQ6056. If use VBAT, then the shutdown quiescent current is important.