[PATCH v2 0/2] LTC4286 and LTC4287 driver support

[PATCH v2 0/2] LTC4286 and LTC4287 driver support

[Delphine CC Chiu]

v2 - Add LTC4286 and LTC4287 binding document
   - Add LTC4286 and LTC4287 driver

Delphine CC Chiu (2):
  dt-bindings: hwmon: Add lltc ltc4286 driver bindings
  hwmon: pmbus: Add ltc4286 driver

 .../bindings/hwmon/lltc,ltc4286.yaml          |  50 ++++++
 Documentation/hwmon/ltc4286.rst               |  79 +++++++++
 MAINTAINERS                                   |  10 ++
 drivers/hwmon/pmbus/Kconfig                   |   9 +
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/ltc4286.c                 | 160 ++++++++++++++++++
 6 files changed, 309 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
 create mode 100644 Documentation/hwmon/ltc4286.rst
 create mode 100644 drivers/hwmon/pmbus/ltc4286.c

-- 
2.25.1

[PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine CC Chiu]

Add a device tree bindings for ltc4286 driver.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>

Changelog:
  v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
     - Add type for adi,vrange-select-25p6
     - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
---
 .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
 MAINTAINERS                                   | 10 ++++
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
new file mode 100644
index 000000000000..17022de657bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LTC4286 power monitors
+
+maintainers:
+  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+
+properties:
+  compatible:
+    enum:
+      - lltc,ltc4286
+      - lltc,ltc4287
+
+  reg:
+    maxItems: 1
+
+  adi,vrange-select-25p6:
+    description:
+      This property is a bool parameter to represent the
+      voltage range is 25.6 or not for this chip.
+    type: boolean
+
+  shunt-resistor-micro-ohms:
+    description:
+      Resistor value in micro-Ohm
+
+required:
+  - compatible
+  - reg
+  - shunt-resistor-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "lltc,ltc4286";
+            reg = <0x40>;
+            adi,vrange-select-25p6;
+            shunt-resistor-micro-ohms = <300>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 668d1e24452d..89e5fff4bba9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12466,6 +12466,16 @@ S:	Maintained
 F:	Documentation/hwmon/ltc4261.rst
 F:	drivers/hwmon/ltc4261.c
 
+LTC4286 HARDWARE MONITOR DRIVER
+M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
+F:	Documentation/devicetree/bindings/hwmon/ltc4286.rst
+F:	drivers/hwmon/pmbus/Kconfig
+F:	drivers/hwmon/pmbus/Makefile
+F:	drivers/hwmon/pmbus/ltc4286.c
+
 LTC4306 I2C MULTIPLEXER DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 L:	linux-i2c@vger.kernel.org
-- 
2.25.1

[PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine CC Chiu]

Add a driver to support ltc4286 chip

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>

Changelog:
  v2 - Revise Linear Technologies LTC4286 to
       Analog Devices LTC4286 in Kconfig
     - Add more description for this driver in Kconfig
     - Add some comments for MBR setting in ltc4286.c
     - Add ltc4286.rst
---
 Documentation/hwmon/ltc4286.rst |  79 ++++++++++++++++
 drivers/hwmon/pmbus/Kconfig     |   9 ++
 drivers/hwmon/pmbus/Makefile    |   1 +
 drivers/hwmon/pmbus/ltc4286.c   | 160 ++++++++++++++++++++++++++++++++
 4 files changed, 249 insertions(+)
 create mode 100644 Documentation/hwmon/ltc4286.rst
 create mode 100644 drivers/hwmon/pmbus/ltc4286.c

diff --git a/Documentation/hwmon/ltc4286.rst b/Documentation/hwmon/ltc4286.rst
new file mode 100644
index 000000000000..9cae50b7478d
--- /dev/null
+++ b/Documentation/hwmon/ltc4286.rst
@@ -0,0 +1,79 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver ltc4286
+=====================
+
+Supported chips:
+
+  * Analog Devices LTC4286
+
+    Prefix: 'ltc4286'
+
+    Addresses scanned: -
+
+    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4286.pdf
+
+  * Analog Devices LTC4287
+
+    Prefix: 'ltc4287'
+
+    Addresses scanned: -
+
+    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4287.pdf
+
+Author: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+
+
+Description
+-----------
+
+This driver supports hardware monitoring for Analog Devices LTC4286
+and LTC4287 Hot-Swap Controller and Digital Power Monitors.
+
+LTC4286 and LTC4287 are hot-swap controllers that allow a circuit board
+to be removed from or inserted into a live backplane. They also feature
+current and voltage readback via an integrated 12 bit analog-to-digital
+converter (ADC), accessed using a PMBus interface.
+
+The driver is a client driver to the core PMBus driver. Please see
+Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
+
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for
+details.
+
+The shunt value in micro-ohms can be set via device tree at compile-time. Please
+refer to the Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml for bindings
+if the device tree is used.
+
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data. Please see
+Documentation/hwmon/pmbus.rst for details.
+
+
+Sysfs entries
+-------------
+
+The following attributes are supported. Limits are read-write, history reset
+attributes are write-only, all other attributes are read-only.
+
+======================= =======================================================
+inX_label               "vin1" or "vout1" depending on chip variant and
+                        configuration.
+inX_input               Measured voltage.
+
+curr1_label             "iout1"
+curr1_input             Measured current.
+
+power1_label            "pin1"
+power1_input            Input power.
+
+temp1_input             Chip temperature.
+======================= =======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b4e93bd5835e..f2b53e8abc3c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -226,6 +226,15 @@ config SENSORS_LTC3815
 
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc3815.
+config SENSORS_LTC4286
+	bool "Analog Devices LTC4286"
+	help
+	  LTC4286 is an integrated solution for hot swap applications that
+	  allows a board to be safely inserted and removed from a live
+	  backplane.
+	  This chip could be used to monitor voltage, current, ...etc.
+	  If you say yes here you get hardware monitoring support for Analog
+	  Devices LTC4286.
 
 config SENSORS_MAX15301
 	tristate "Maxim MAX15301"
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 84ee960a6c2d..94e28f6d6a61 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
 obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
 obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
 obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
+obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
 obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
 obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
 obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
new file mode 100644
index 000000000000..e1d72fe9587c
--- /dev/null
+++ b/drivers/hwmon/pmbus/ltc4286.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+/* LTC4286 register */
+#define LTC4286_MFR_CONFIG1	0xF2
+
+/* LTC4286 configuration */
+#define VRANGE_SELECT_BIT	BIT(1)
+
+#define LTC4286_MFR_ID_SIZE	3
+
+enum chips { ltc4286, ltc4287 };
+
+/*
+ * Initialize the MBR as default settings which is referred to LTC4286 datasheet
+ * (March 22, 2022 version) table 3 page 16
+ */
+static struct pmbus_driver_info ltc4286_info = {
+	.pages = 1,
+	.format[PSC_VOLTAGE_IN] = direct,
+	.format[PSC_VOLTAGE_OUT] = direct,
+	.format[PSC_CURRENT_OUT] = direct,
+	.format[PSC_POWER] = direct,
+	.format[PSC_TEMPERATURE] = direct,
+	.m[PSC_VOLTAGE_IN] = 32,
+	.b[PSC_VOLTAGE_IN] = 0,
+	.R[PSC_VOLTAGE_IN] = 1,
+	.m[PSC_VOLTAGE_OUT] = 32,
+	.b[PSC_VOLTAGE_OUT] = 0,
+	.R[PSC_VOLTAGE_OUT] = 1,
+	.m[PSC_CURRENT_OUT] = 1024,
+	.b[PSC_CURRENT_OUT] = 0,
+	/*
+	 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
+	 * However, the rsense value that user input is mirco ohm.
+	 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
+	 */
+	.R[PSC_CURRENT_OUT] = 3 - 6,
+	.m[PSC_POWER] = 1,
+	.b[PSC_POWER] = 0,
+	/*
+	 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
+	 * However, the rsense value that user input is mirco ohm.
+	 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
+	 */
+	.R[PSC_POWER] = 4 - 6,
+	.m[PSC_TEMPERATURE] = 1,
+	.b[PSC_TEMPERATURE] = 273,
+	.R[PSC_TEMPERATURE] = 0,
+	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
+		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP,
+};
+
+static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
+						   { "ltc4287", ltc4287 },
+						   {} };
+MODULE_DEVICE_TABLE(i2c, ltc4286_id);
+
+static int ltc4286_probe(struct i2c_client *client)
+{
+	int ret;
+	const struct i2c_device_id *mid;
+	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
+	struct pmbus_driver_info *info;
+	u32 rsense;
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer id\n");
+		return ret;
+	}
+
+	/*
+	 * Refer to ltc4286 datasheet page 20
+	 * the manufacturer id is LTC
+	 */
+	if (ret != LTC4286_MFR_ID_SIZE ||
+	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
+		return dev_err_probe(&client->dev, ret,
+				     "failed to read manufacturer id\n");
+	}
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read manufacturer model\n");
+		return ret;
+	}
+
+	for (mid = ltc4286_id; mid->name[0]; mid++) {
+		if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
+			break;
+	}
+
+	ret = of_property_read_u32(client->dev.of_node,
+				   "shunt-resistor-micro-ohms", &rsense);
+	if (ret < 0)
+		return ret;
+
+	if (rsense == 0)
+		return -EINVAL;
+
+	info = &ltc4286_info;
+
+	/* Default of VRANGE_SELECT = 1, 102.4V */
+	if (device_property_read_bool(&client->dev, "adi,vrange-select-25p6")) {
+		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
+		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"failed to read manufacturer configuration one\n");
+			return ret;
+		}
+
+		ret &= ~VRANGE_SELECT_BIT; /* VRANGE_SELECT = 0, 25.6V */
+		ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1,
+						ret);
+		if (ret < 0) {
+			dev_err(&client->dev, "failed to set vrange\n");
+			return ret;
+		}
+
+		info->m[PSC_VOLTAGE_IN] = 128;
+		info->m[PSC_VOLTAGE_OUT] = 128;
+		info->m[PSC_POWER] = 4 * rsense;
+	} else
+		info->m[PSC_POWER] = rsense;
+
+	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
+
+	return pmbus_do_probe(client, info);
+}
+
+static const struct of_device_id ltc4286_of_match[] = {
+	{ .compatible = "lltc,ltc4286" },
+	{ .compatible = "lltc,ltc4287" },
+	{}
+};
+
+static struct i2c_driver ltc4286_driver = {
+	.driver = {
+		.name = "ltc4286",
+		.of_match_table = ltc4286_of_match,
+	},
+	.probe = ltc4286_probe,
+	.id_table = ltc4286_id,
+};
+
+module_i2c_driver(ltc4286_driver);
+
+MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
+MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

Hey,

On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> Add a device tree bindings for ltc4286 driver.

Bindings are for devices, not for drivers.

> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> 
> Changelog:
>   v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
>      - Add type for adi,vrange-select-25p6
>      - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> ---
>  .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
>  MAINTAINERS                                   | 10 ++++
>  2 files changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> new file mode 100644
> index 000000000000..17022de657bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTC4286 power monitors
> +
> +maintainers:
> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lltc,ltc4286
> +      - lltc,ltc4287

I don't recall seeing an answer to Guenter about this ltc4287 device:
https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.net/

> +
> +  reg:
> +    maxItems: 1
> +
> +  adi,vrange-select-25p6:
> +    description:
> +      This property is a bool parameter to represent the
> +      voltage range is 25.6 or not for this chip.

25.6 what? Volts? microvolts?
What about Guenter's suggestion to name this so that it better matches
the other, similar properties?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On 10/26/23 07:25, Conor Dooley wrote:
> Hey,
> 
> On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
>> Add a device tree bindings for ltc4286 driver.
> 
> Bindings are for devices, not for drivers.
> 
>>
>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>
>> Changelog:
>>    v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
>>       - Add type for adi,vrange-select-25p6
>>       - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
>> ---
>>   .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
>>   MAINTAINERS                                   | 10 ++++
>>   2 files changed, 60 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>> new file mode 100644
>> index 000000000000..17022de657bb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LTC4286 power monitors
>> +
>> +maintainers:
>> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - lltc,ltc4286
>> +      - lltc,ltc4287
> 
> I don't recall seeing an answer to Guenter about this ltc4287 device:
> https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.net/
> 

At least the chip does officially exist now, and a datasheet is available.

https://www.analog.com/en/products/ltc4287.html

It shows that coefficients for the telemetry commands are different,
meaning that indeed both chips need to be explicitly referenced
in the properties description (and handled in the driver, which proves
my point of needing a datasheet before accepting such a driver).

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  adi,vrange-select-25p6:
>> +    description:
>> +      This property is a bool parameter to represent the
>> +      voltage range is 25.6 or not for this chip.
> 
> 25.6 what? Volts? microvolts?
> What about Guenter's suggestion to name this so that it better matches
> the other, similar properties?
> 

I still would prefer one of the more common properties.
I still prefer adi,vrange-high-enable.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

On Thu, Oct 26, 2023 at 08:09:52AM -0700, Guenter Roeck wrote:
> On 10/26/23 07:25, Conor Dooley wrote:
> > Hey,
> > 
> > On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> > > Add a device tree bindings for ltc4286 driver.
> > 
> > Bindings are for devices, not for drivers.
> > 
> > > 
> > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > > 
> > > Changelog:
> > >    v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> > >       - Add type for adi,vrange-select-25p6
> > >       - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> > > ---
> > >   .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
> > >   MAINTAINERS                                   | 10 ++++
> > >   2 files changed, 60 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > new file mode 100644
> > > index 000000000000..17022de657bb
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > @@ -0,0 +1,50 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: LTC4286 power monitors
> > > +
> > > +maintainers:
> > > +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - lltc,ltc4286
> > > +      - lltc,ltc4287
> > 
> > I don't recall seeing an answer to Guenter about this ltc4287 device:
> > https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.net/
> > 
> 
> At least the chip does officially exist now, and a datasheet is available.
> 
> https://www.analog.com/en/products/ltc4287.html
> 
> It shows that coefficients for the telemetry commands are different,
> meaning that indeed both chips need to be explicitly referenced
> in the properties description (and handled in the driver, which proves
> my point of needing a datasheet before accepting such a driver).

Oh neat, would've been good if that'd been mentioned!

> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  adi,vrange-select-25p6:
> > > +    description:
> > > +      This property is a bool parameter to represent the
> > > +      voltage range is 25.6 or not for this chip.
> > 
> > 25.6 what? Volts? microvolts?
> > What about Guenter's suggestion to name this so that it better matches
> > the other, similar properties?
> > 
> 
> I still would prefer one of the more common properties.
> I still prefer adi,vrange-high-enable.

I think, from reading the previous version, that this is actually the
lower voltage range, as there is a 102.x $unit range too that is the
default in the hardware. Obviously, the use of the property could be
inverted to match that naming however.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 10/26/23 01:15, Delphine CC Chiu wrote:
> Add a driver to support ltc4286 chip
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> 
> Changelog:
>    v2 - Revise Linear Technologies LTC4286 to
>         Analog Devices LTC4286 in Kconfig
>       - Add more description for this driver in Kconfig
>       - Add some comments for MBR setting in ltc4286.c
>       - Add ltc4286.rst
> ---
>   Documentation/hwmon/ltc4286.rst |  79 ++++++++++++++++
>   drivers/hwmon/pmbus/Kconfig     |   9 ++
>   drivers/hwmon/pmbus/Makefile    |   1 +
>   drivers/hwmon/pmbus/ltc4286.c   | 160 ++++++++++++++++++++++++++++++++
>   4 files changed, 249 insertions(+)
>   create mode 100644 Documentation/hwmon/ltc4286.rst
>   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> 
> diff --git a/Documentation/hwmon/ltc4286.rst b/Documentation/hwmon/ltc4286.rst
> new file mode 100644
> index 000000000000..9cae50b7478d
> --- /dev/null
> +++ b/Documentation/hwmon/ltc4286.rst
> @@ -0,0 +1,79 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver ltc4286
> +=====================
> +
> +Supported chips:
> +
> +  * Analog Devices LTC4286
> +
> +    Prefix: 'ltc4286'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4286.pdf
> +
> +  * Analog Devices LTC4287
> +
> +    Prefix: 'ltc4287'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4287.pdf
> +
> +Author: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for Analog Devices LTC4286
> +and LTC4287 Hot-Swap Controller and Digital Power Monitors.
> +
> +LTC4286 and LTC4287 are hot-swap controllers that allow a circuit board
> +to be removed from or inserted into a live backplane. They also feature
> +current and voltage readback via an integrated 12 bit analog-to-digital
> +converter (ADC), accessed using a PMBus interface.
> +
> +The driver is a client driver to the core PMBus driver. Please see
> +Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The shunt value in micro-ohms can be set via device tree at compile-time. Please
> +refer to the Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml for bindings
> +if the device tree is used.
> +
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data. Please see
> +Documentation/hwmon/pmbus.rst for details.
> +
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. Limits are read-write, history reset
> +attributes are write-only, all other attributes are read-only.
> +
> +======================= =======================================================
> +inX_label               "vin1" or "vout1" depending on chip variant and
> +                        configuration.

Is that a cut-and-paste ? I don't see it handled in the driver, and I don't immediately
see it in the datasheet. From the datasheet, it seems to me that both are always
present.

> +inX_input               Measured voltage.
> +
> +curr1_label             "iout1"
> +curr1_input             Measured current.
> +
> +power1_label            "pin1"
> +power1_input            Input power.
> +
> +temp1_input             Chip temperature.
> +======================= =======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index b4e93bd5835e..f2b53e8abc3c 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -226,6 +226,15 @@ config SENSORS_LTC3815
>   
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc3815.
> +config SENSORS_LTC4286
> +	bool "Analog Devices LTC4286"
> +	help
> +	  LTC4286 is an integrated solution for hot swap applications that
> +	  allows a board to be safely inserted and removed from a live
> +	  backplane.
> +	  This chip could be used to monitor voltage, current, ...etc.
> +	  If you say yes here you get hardware monitoring support for Analog
> +	  Devices LTC4286.
>   
>   config SENSORS_MAX15301
>   	tristate "Maxim MAX15301"
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 84ee960a6c2d..94e28f6d6a61 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>   obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
>   obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>   obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
> +obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
>   obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
>   obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
>   obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
> diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
> new file mode 100644
> index 000000000000..e1d72fe9587c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ltc4286.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +/* LTC4286 register */
> +#define LTC4286_MFR_CONFIG1	0xF2
> +
> +/* LTC4286 configuration */
> +#define VRANGE_SELECT_BIT	BIT(1)
> +
> +#define LTC4286_MFR_ID_SIZE	3
> +
> +enum chips { ltc4286, ltc4287 };
> +
> +/*
> + * Initialize the MBR as default settings which is referred to LTC4286 datasheet
> + * (March 22, 2022 version) table 3 page 16
> + */
> +static struct pmbus_driver_info ltc4286_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.format[PSC_TEMPERATURE] = direct,
> +	.m[PSC_VOLTAGE_IN] = 32,
> +	.b[PSC_VOLTAGE_IN] = 0,
> +	.R[PSC_VOLTAGE_IN] = 1,
> +	.m[PSC_VOLTAGE_OUT] = 32,
> +	.b[PSC_VOLTAGE_OUT] = 0,
> +	.R[PSC_VOLTAGE_OUT] = 1,
> +	.m[PSC_CURRENT_OUT] = 1024,
> +	.b[PSC_CURRENT_OUT] = 0,
> +	/*
> +	 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
> +	 * However, the rsense value that user input is mirco ohm.
> +	 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
> +	 */
> +	.R[PSC_CURRENT_OUT] = 3 - 6,
> +	.m[PSC_POWER] = 1,
> +	.b[PSC_POWER] = 0,
> +	/*
> +	 * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
> +	 * However, the rsense value that user input is mirco ohm.
> +	 * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
> +	 */
> +	.R[PSC_POWER] = 4 - 6,
> +	.m[PSC_TEMPERATURE] = 1,
> +	.b[PSC_TEMPERATURE] = 273,
> +	.R[PSC_TEMPERATURE] = 0,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> +		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
> +		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP,
> +};
> +

The datasheet for LTC4286  (in the PMBus description talks about LSB 21.3 mV/RSENSE
for IOUT_OC_WARN_LIMIT and 2.8/RSENSE for PIN_OP_WARN_LIMIT. This contradicts
data elsewhere in the datasheet which uses above coefficients for both LTC4286
and LTC4287. I don't know if the datasheet is wrong, but this needs to be clarified.
If the datasheet is wrong, that needs to be mentioned above. If the limit registers
use different coefficients, local code will be needed to adjust values when reading /
writing the registers to have it match coefficients.

> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> +						   { "ltc4287", ltc4287 },
> +						   {} };
> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> +
> +static int ltc4286_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	const struct i2c_device_id *mid;
> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct pmbus_driver_info *info;
> +	u32 rsense;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer id\n");

Why not use dev_err_probe() here ?

> +		return ret;
> +	}
> +
> +	/*
> +	 * Refer to ltc4286 datasheet page 20
> +	 * the manufacturer id is LTC
> +	 */
> +	if (ret != LTC4286_MFR_ID_SIZE ||
> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to read manufacturer id\n");

This is misleading. It didn't _fail_ to read the manufacturer ID.

> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer model\n");

Why not use dev_err_probe() here ?

> +		return ret;
> +	}
> +
> +	for (mid = ltc4286_id; mid->name[0]; mid++) {
> +		if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> +			break;
> +	}

This is pointless code. If the ID is not found, mid will point after
the end of the array, and then what ?

The purpose of such code is to validate if the chip is actually the one
referenced in the match function and at least warn if that is not the case.
It should never accept a chip which is _known_ to not be supported.

> +
> +	ret = of_property_read_u32(client->dev.of_node,
> +				   "shunt-resistor-micro-ohms", &rsense);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (rsense == 0)
> +		return -EINVAL;
> +
> +	info = &ltc4286_info;
> +
> +	/* Default of VRANGE_SELECT = 1, 102.4V */
> +	if (device_property_read_bool(&client->dev, "adi,vrange-select-25p6")) {

What if the adi,vrange-select-25p6 property is not provided, but the chip
is programmed for this range ?

> +		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> +		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"failed to read manufacturer configuration one\n"); > +			return ret;
> +		}
> +
> +		ret &= ~VRANGE_SELECT_BIT; /* VRANGE_SELECT = 0, 25.6V */
> +		ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1,
> +						ret);

This should only be written if it actually changed.

> +		if (ret < 0) {
> +			dev_err(&client->dev, "failed to set vrange\n");
> +			return ret;
> +		}
> +
> +		info->m[PSC_VOLTAGE_IN] = 128;
> +		info->m[PSC_VOLTAGE_OUT] = 128;
> +		info->m[PSC_POWER] = 4 * rsense;

You can not overwrite ltc4286_info because there might be several chips
in the system with different sense resistor values and range
configurations.

Also, the above (and the calculation in the code below) will overflow
with too-large sense register values.

> +	} else
> +		info->m[PSC_POWER] = rsense;

Please run checkpatch --strict on your patches.

> +
> +	info->m[PSC_CURRENT_OUT] = 1024 * rsense;

Any rsense value larger than MAXINT / 1024 will overflow.

> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct of_device_id ltc4286_of_match[] = {
> +	{ .compatible = "lltc,ltc4286" },
> +	{ .compatible = "lltc,ltc4287" },
> +	{}
> +};
> +
> +static struct i2c_driver ltc4286_driver = {
> +	.driver = {
> +		.name = "ltc4286",
> +		.of_match_table = ltc4286_of_match,
> +	},
> +	.probe = ltc4286_probe,
> +	.id_table = ltc4286_id,
> +};
> +
> +module_i2c_driver(ltc4286_driver);
> +
> +MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>");
> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On 10/26/23 08:26, Conor Dooley wrote:
> On Thu, Oct 26, 2023 at 08:09:52AM -0700, Guenter Roeck wrote:
>> On 10/26/23 07:25, Conor Dooley wrote:
>>> Hey,
>>>
>>> On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
>>>> Add a device tree bindings for ltc4286 driver.
>>>
>>> Bindings are for devices, not for drivers.
>>>
>>>>
>>>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>>>
>>>> Changelog:
>>>>     v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
>>>>        - Add type for adi,vrange-select-25p6
>>>>        - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
>>>> ---
>>>>    .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
>>>>    MAINTAINERS                                   | 10 ++++
>>>>    2 files changed, 60 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>> new file mode 100644
>>>> index 000000000000..17022de657bb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>> @@ -0,0 +1,50 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: LTC4286 power monitors
>>>> +
>>>> +maintainers:
>>>> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - lltc,ltc4286
>>>> +      - lltc,ltc4287
>>>
>>> I don't recall seeing an answer to Guenter about this ltc4287 device:
>>> https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.net/
>>>
>>
>> At least the chip does officially exist now, and a datasheet is available.
>>
>> https://www.analog.com/en/products/ltc4287.html
>>
>> It shows that coefficients for the telemetry commands are different,
>> meaning that indeed both chips need to be explicitly referenced
>> in the properties description (and handled in the driver, which proves
>> my point of needing a datasheet before accepting such a driver).
> 
> Oh neat, would've been good if that'd been mentioned!
> 

Actually, turns out there is some contradiction in the LTC4286 datasheet.
It mentions different coefficients in different places. It is all but
impossible to determine if the datasheet is wrong or if the chip uses
a variety of coefficients unless one has a real chip available. Sigh :-(.

>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  adi,vrange-select-25p6:
>>>> +    description:
>>>> +      This property is a bool parameter to represent the
>>>> +      voltage range is 25.6 or not for this chip.
>>>
>>> 25.6 what? Volts? microvolts?
>>> What about Guenter's suggestion to name this so that it better matches
>>> the other, similar properties?
>>>
>>
>> I still would prefer one of the more common properties.
>> I still prefer adi,vrange-high-enable.
> 
> I think, from reading the previous version, that this is actually the
> lower voltage range, as there is a 102.x $unit range too that is the
> default in the hardware. Obviously, the use of the property could be
> inverted to match that naming however.
> 

It needs to be programmed either way because it is unknown how the chip
has been programmed before. That means some action is needed no matter
if the property is provided or not.

Sure, we could add something like adi,vrange-low-enable. Not sure if
that would be any better.

Actually, after looking at the datasheets, I'd be more interested
in the impact of other configuration buts such as VPWR_SELECT
which selects the voltage used for power calculations, or
all the settings in MFR_ADC_CONFIG. The datasheet doesn't really
explain (or I can't figure out how it does) how the different
configurations affect the reported telemetry values. But that is
a question for the driver, not for the property description.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.6-rc7 next-20231026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20231026-161739
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20231026081514.3610343-2-Delphine_CC_Chiu%40Wiwynn.com
patch subject: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings
reproduce: (https://download.01.org/0day-ci/archive/20231027/202310271540.4uI1Fgxe-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310271540.4uI1Fgxe-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/ltc4286.rst
>> MAINTAINERS:27681: WARNING: unknown document: ../devicetree/bindings/hwmon/ltc4286

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[kernel test robot]

Hi Delphine,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/dt-bindings-hwmon-Add-lltc-ltc4286-driver-bindings/20231026-161739
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20231026081514.3610343-3-Delphine_CC_Chiu%40Wiwynn.com
patch subject: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
reproduce: (https://download.01.org/0day-ci/archive/20231028/202310281159.Y11xKbmu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310281159.Y11xKbmu-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Documentation/hwmon/ltc4286.rst: WARNING: document isn't included in any toctree

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Thursday, October 26, 2023 10:26 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Guenter Roeck
> <linux@roeck-us.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Jonathan Corbet <corbet@lwn.net>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
> Hey,
> 
> On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> > Add a device tree bindings for ltc4286 driver.
> 
> Bindings are for devices, not for drivers.
> 
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >
> > Changelog:
> >   v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> >      - Add type for adi,vrange-select-25p6
> >      - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> > ---
> >  .../bindings/hwmon/lltc,ltc4286.yaml          | 50
> +++++++++++++++++++
> >  MAINTAINERS                                   | 10 ++++
> >  2 files changed, 60 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > new file mode 100644
> > index 000000000000..17022de657bb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LTC4286 power monitors
> > +
> > +maintainers:
> > +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - lltc,ltc4286
> > +      - lltc,ltc4287
> 
> I don't recall seeing an answer to Guenter about this ltc4287 device:
> https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.
> net/
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  adi,vrange-select-25p6:
> > +    description:
> > +      This property is a bool parameter to represent the
> > +      voltage range is 25.6 or not for this chip.
> 
> 25.6 what? Volts? microvolts?
25.6 volts

> What about Guenter's suggestion to name this so that it better matches the
> other, similar properties?
> 
> Cheers,
> Conor.

RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, October 26, 2023 11:10 PM
> To: Conor Dooley <conor@kernel.org>; Delphine_CC_Chiu/WYHQ/Wiwynn
> <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Jonathan Corbet <corbet@lwn.net>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
>
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
>
> On 10/26/23 07:25, Conor Dooley wrote:
> > Hey,
> >
> > On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> >> Add a device tree bindings for ltc4286 driver.
> >
> > Bindings are for devices, not for drivers.
> >
> >>
> >> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >>
> >> Changelog:
> >>    v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> >>       - Add type for adi,vrange-select-25p6
> >>       - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> >> ---
> >>   .../bindings/hwmon/lltc,ltc4286.yaml          | 50
> +++++++++++++++++++
> >>   MAINTAINERS                                   | 10 ++++
> >>   2 files changed, 60 insertions(+)
> >>   create mode 100644
> >> Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >> b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >> new file mode 100644
> >> index 000000000000..17022de657bb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >> @@ -0,0 +1,50 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> >> +---
> >> +$id:
> >> +http://dev/
> >>
> +icetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%7C
> 01%
> >>
> +7CWayne_SC_Liu%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51
> %7Cda6
> >>
> +e0628fc834caf9dd273061cbab167%7C0%7C0%7C638339298041650948%7CU
> nknown
> >>
> +%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiL
> >>
> +CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gGQEDl33zRIJbfbinu2%2Bi2cC
> Ay6y0o
> >> +DSLzBpLL7hA%2F8%3D&reserved=0
> >> +$schema:
> >> +http://dev/
> >>
> +icetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne_S
> C_Li
> >>
> +u%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51%7Cda6e0628fc8
> 34caf
> >>
> +9dd273061cbab167%7C0%7C0%7C638339298041650948%7CUnknown%7CT
> WFpbGZsb3
> >>
> +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D
> >>
> +%7C3000%7C%7C%7C&sdata=nl1IM1HpYptsJOHfiuXtKFmD%2FVlGMCW1IkoK
> HYco0sk
> >> +%3D&reserved=0
> >> +
> >> +title: LTC4286 power monitors
> >> +
> >> +maintainers:
> >> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - lltc,ltc4286
> >> +      - lltc,ltc4287
> >
> > I don't recall seeing an answer to Guenter about this ltc4287 device:
> > https://lore/
> > .kernel.org%2Fall%2F22f6364c-611c-ffb6-451c-9ddc20418d0a%40roeck-us.n
> e
> >
> t%2F&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb250e206c9ef48f
> bc1c108
> >
> dbd6359e51%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6383392
> 9804165
> >
> 0948%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
> IiLCJBT
> >
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OpwfD3rBS0vQBF
> jUhszrMg
> > 4mq581jU7gx54Ln8V3gUA%3D&reserved=0
> >
>
> At least the chip does officially exist now, and a datasheet is available.
>
> https://www.ana/
> log.com%2Fen%2Fproducts%2Fltc4287.html&data=05%7C01%7CWayne_SC_Li
> u%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51%7Cda6e0628fc83
> 4caf9dd273061cbab167%7C0%7C0%7C638339298041650948%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CwT16Uipl8RymnFZ1WuBJzUJv
> fLCKdK3VlTcTBN0xxk%3D&reserved=0
>
> It shows that coefficients for the telemetry commands are different, meaning
> that indeed both chips need to be explicitly referenced in the properties
> description (and handled in the driver, which proves my point of needing a
> datasheet before accepting such a driver).
We will check the difference of coefficients for the telemetry commands with vendor.

>
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  adi,vrange-select-25p6:
> >> +    description:
> >> +      This property is a bool parameter to represent the
> >> +      voltage range is 25.6 or not for this chip.
> >
> > 25.6 what? Volts? microvolts?
> > What about Guenter's suggestion to name this so that it better matches
> > the other, similar properties?
> >
>
> I still would prefer one of the more common properties.
> I still prefer adi,vrange-high-enable.
>
> Guenter

RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Thursday, October 26, 2023 11:26 PM
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Jonathan Corbet <corbet@lwn.net>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
> On Thu, Oct 26, 2023 at 08:09:52AM -0700, Guenter Roeck wrote:
> > On 10/26/23 07:25, Conor Dooley wrote:
> > > Hey,
> > >
> > > On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> > > > Add a device tree bindings for ltc4286 driver.
> > >
> > > Bindings are for devices, not for drivers.
> > >
> > > >
> > > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > > >
> > > > Changelog:
> > > >    v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> > > >       - Add type for adi,vrange-select-25p6
> > > >       - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> > > > ---
> > > >   .../bindings/hwmon/lltc,ltc4286.yaml          | 50
> +++++++++++++++++++
> > > >   MAINTAINERS                                   | 10 ++++
> > > >   2 files changed, 60 insertions(+)
> > > >   create mode 100644
> > > > Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > > b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > > new file mode 100644
> > > > index 000000000000..17022de657bb
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> > > > @@ -0,0 +1,50 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> > > > +1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: LTC4286 power monitors
> > > > +
> > > > +maintainers:
> > > > +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - lltc,ltc4286
> > > > +      - lltc,ltc4287
> > >
> > > I don't recall seeing an answer to Guenter about this ltc4287 device:
> > > https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roe
> > > ck-us.net/
> > >
> >
> > At least the chip does officially exist now, and a datasheet is available.
> >
> > https://www.analog.com/en/products/ltc4287.html
> >
> > It shows that coefficients for the telemetry commands are different,
> > meaning that indeed both chips need to be explicitly referenced in the
> > properties description (and handled in the driver, which proves my
> > point of needing a datasheet before accepting such a driver).
> 
> Oh neat, would've been good if that'd been mentioned!
> 
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  adi,vrange-select-25p6:
> > > > +    description:
> > > > +      This property is a bool parameter to represent the
> > > > +      voltage range is 25.6 or not for this chip.
> > >
> > > 25.6 what? Volts? microvolts?
> > > What about Guenter's suggestion to name this so that it better
> > > matches the other, similar properties?
> > >
> >
> > I still would prefer one of the more common properties.
> > I still prefer adi,vrange-high-enable.
> 
> I think, from reading the previous version, that this is actually the lower voltage
> range, as there is a 102.x $unit range too that is the default in the hardware.
> Obviously, the use of the property could be inverted to match that naming
> however.
We will use adi,vrange-low-enable instead of adi,vrange-select-25p6

> 
> Cheers,
> Conor.

RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 12:49 AM
> To: Conor Dooley <conor@kernel.org>
> Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Jonathan Corbet <corbet@lwn.net>; linux-i2c@vger.kernel.org;
> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 10/26/23 08:26, Conor Dooley wrote:
> > On Thu, Oct 26, 2023 at 08:09:52AM -0700, Guenter Roeck wrote:
> >> On 10/26/23 07:25, Conor Dooley wrote:
> >>> Hey,
> >>>
> >>> On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
> >>>> Add a device tree bindings for ltc4286 driver.
> >>>
> >>> Bindings are for devices, not for drivers.
> >>>
> >>>>
> >>>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >>>>
> >>>> Changelog:
> >>>>     v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
> >>>>        - Add type for adi,vrange-select-25p6
> >>>>        - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
> >>>> ---
> >>>>    .../bindings/hwmon/lltc,ltc4286.yaml          | 50
> +++++++++++++++++++
> >>>>    MAINTAINERS                                   | 10 ++++
> >>>>    2 files changed, 60 insertions(+)
> >>>>    create mode 100644
> >>>> Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >>>> b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..17022de657bb
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
> >>>> @@ -0,0 +1,50 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> >>>> +1.2
> >>>> +---
> >>>> +$id:
> >>>> +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
> >>>>
> +evicetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%
> 7
> >>>>
> +C01%7CWayne_SC_Liu%40wiwynn.com%7Cf555db9618454d17e0b508dbd64
> 36721
> >>>>
> +%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6383393572109674
> 95%7
> >>>>
> +CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> TiI
> >>>>
> +6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cPOOqJ5y3K%2B
> D%2Frsp
> >>>> +gVhpKDIC0gC5nKBbRp7Up0OxDpM%3D&reserved=0
> >>>> +$schema:
> >>>> +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
> >>>>
> +evicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne
> _S
> >>>>
> +C_Liu%40wiwynn.com%7Cf555db9618454d17e0b508dbd6436721%7Cda6e06
> 28fc
> >>>>
> +834caf9dd273061cbab167%7C0%7C0%7C638339357210967495%7CUnknown
> %7CTW
> >>>>
> +FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> >>>>
> +VCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HjOhDm8bwPaNpWgumCSlak%2
> FiqoagwZc
> >>>> +0eb95J1wnKUE%3D&reserved=0
> >>>> +
> >>>> +title: LTC4286 power monitors
> >>>> +
> >>>> +maintainers:
> >>>> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    enum:
> >>>> +      - lltc,ltc4286
> >>>> +      - lltc,ltc4287
> >>>
> >>> I don't recall seeing an answer to Guenter about this ltc4287 device:
> >>> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> >>>
> re.kernel.org%2Fall%2F22f6364c-611c-ffb6-451c-9ddc20418d0a%40roeck-u
> >>>
> s.net%2F&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cf555db96184
> 54d17
> >>>
> e0b508dbd6436721%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6
> 38339
> >>>
> 357210967495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2l
> >>>
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oekIX
> %2FlP
> >>> %2B%2F4KhrSlK8Xp1zR0fdX1Emmr0Ox5GPS9Dz0%3D&reserved=0
> >>>
> >>
> >> At least the chip does officially exist now, and a datasheet is available.
> >>
> >> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> >> .analog.com%2Fen%2Fproducts%2Fltc4287.html&data=05%7C01%7CWayne
> _SC_Li
> >>
> u%40wiwynn.com%7Cf555db9618454d17e0b508dbd6436721%7Cda6e0628fc8
> 34caf9
> >>
> dd273061cbab167%7C0%7C0%7C638339357210967495%7CUnknown%7CTWF
> pbGZsb3d8
> >>
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C
> >>
> 3000%7C%7C%7C&sdata=FqRheGXFi%2FmSH3cnRZ7eSbwD3JfZj8powcGBCJcP
> wWQ%3D&
> >> reserved=0
> >>
> >> It shows that coefficients for the telemetry commands are different,
> >> meaning that indeed both chips need to be explicitly referenced in
> >> the properties description (and handled in the driver, which proves
> >> my point of needing a datasheet before accepting such a driver).
> >
> > Oh neat, would've been good if that'd been mentioned!
> >
> 
> Actually, turns out there is some contradiction in the LTC4286 datasheet.
> It mentions different coefficients in different places. It is all but impossible to
> determine if the datasheet is wrong or if the chip uses a variety of coefficients
> unless one has a real chip available. Sigh :-(.
We are not the chip vendor, but we could forward your question to vendor.
Could you point out the exact places (which pages) where are the contradiction in LTC4286 datasheet?

> 
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  adi,vrange-select-25p6:
> >>>> +    description:
> >>>> +      This property is a bool parameter to represent the
> >>>> +      voltage range is 25.6 or not for this chip.
> >>>
> >>> 25.6 what? Volts? microvolts?
> >>> What about Guenter's suggestion to name this so that it better
> >>> matches the other, similar properties?
> >>>
> >>
> >> I still would prefer one of the more common properties.
> >> I still prefer adi,vrange-high-enable.
> >
> > I think, from reading the previous version, that this is actually the
> > lower voltage range, as there is a 102.x $unit range too that is the
> > default in the hardware. Obviously, the use of the property could be
> > inverted to match that naming however.
> >
> 
> It needs to be programmed either way because it is unknown how the chip has
> been programmed before. That means some action is needed no matter if the
> property is provided or not.
> 
> Sure, we could add something like adi,vrange-low-enable. Not sure if that
> would be any better.
> 
> Actually, after looking at the datasheets, I'd be more interested in the impact
> of other configuration buts such as VPWR_SELECT which selects the voltage
> used for power calculations, or all the settings in MFR_ADC_CONFIG. The
> datasheet doesn't really explain (or I can't figure out how it does) how the
> different configurations affect the reported telemetry values. But that is a
> question for the driver, not for the property description.
We have sent an e-mail about this question.

> 
> Guenter

RE: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 12:28 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> <corbet@lwn.net>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 10/26/23 01:15, Delphine CC Chiu wrote:
> > Add a driver to support ltc4286 chip
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> >
> > Changelog:
> >    v2 - Revise Linear Technologies LTC4286 to
> >         Analog Devices LTC4286 in Kconfig
> >       - Add more description for this driver in Kconfig
> >       - Add some comments for MBR setting in ltc4286.c
> >       - Add ltc4286.rst
> > ---
> >   Documentation/hwmon/ltc4286.rst |  79 ++++++++++++++++
> >   drivers/hwmon/pmbus/Kconfig     |   9 ++
> >   drivers/hwmon/pmbus/Makefile    |   1 +
> >   drivers/hwmon/pmbus/ltc4286.c   | 160
> ++++++++++++++++++++++++++++++++
> >   4 files changed, 249 insertions(+)
> >   create mode 100644 Documentation/hwmon/ltc4286.rst
> >   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> >
> > diff --git a/Documentation/hwmon/ltc4286.rst
> > b/Documentation/hwmon/ltc4286.rst new file mode 100644 index
> > 000000000000..9cae50b7478d
> > --- /dev/null
> > +++ b/Documentation/hwmon/ltc4286.rst
> > @@ -0,0 +1,79 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver ltc4286
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Analog Devices LTC4286
> > +
> > +    Prefix: 'ltc4286'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > + https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > +
> w.analog.com%2Fmedia%2Fen%2Ftechnical-documentation%2Fdata-sheets%2
> F
> > +
> ltc4286.pdf&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb749e252b
> fb84
> > +
> 24531ac08dbd640774e%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%
> 7C638
> > +
> 339344747629221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoi
> > +
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7s
> HwFye
> > + so39ED13H3KaxosVOMJT1Kswhwj38arystWQ%3D&reserved=0
> > +
> > +  * Analog Devices LTC4287
> > +
> > +    Prefix: 'ltc4287'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > + https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > +
> w.analog.com%2Fmedia%2Fen%2Ftechnical-documentation%2Fdata-sheets%2
> F
> > +
> ltc4287.pdf&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb749e252b
> fb84
> > +
> 24531ac08dbd640774e%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%
> 7C638
> > +
> 339344747629221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoi
> > +
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=31
> 402MZ
> > + 9ONvkV8BsRRPxmTZNMU1yj1u%2F3NFVEpThFPI%3D&reserved=0
> > +
> > +Author: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +
> > +
> > +Description
> > +-----------
> > +
> > +This driver supports hardware monitoring for Analog Devices LTC4286
> > +and LTC4287 Hot-Swap Controller and Digital Power Monitors.
> > +
> > +LTC4286 and LTC4287 are hot-swap controllers that allow a circuit
> > +board to be removed from or inserted into a live backplane. They also
> > +feature current and voltage readback via an integrated 12 bit
> > +analog-to-digital converter (ADC), accessed using a PMBus interface.
> > +
> > +The driver is a client driver to the core PMBus driver. Please see
> > +Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> > +
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not auto-detect devices. You will have to
> > +instantiate the devices explicitly. Please see
> > +Documentation/i2c/instantiating-devices.rst for details.
> > +
> > +The shunt value in micro-ohms can be set via device tree at
> > +compile-time. Please refer to the
> > +Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml for bindings
> if the device tree is used.
> > +
> > +
> > +Platform data support
> > +---------------------
> > +
> > +The driver supports standard PMBus driver platform data. Please see
> > +Documentation/hwmon/pmbus.rst for details.
> > +
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +The following attributes are supported. Limits are read-write,
> > +history reset attributes are write-only, all other attributes are read-only.
> > +
> > +=======================
> =======================================================
> > +inX_label               "vin1" or "vout1" depending on chip variant and
> > +                        configuration.
> 
> Is that a cut-and-paste ? I don't see it handled in the driver, and I don't
> immediately see it in the datasheet. From the datasheet, it seems to me that
> both are always present.
We will revise to the correct description

> 
> > +inX_input               Measured voltage.
> > +
> > +curr1_label             "iout1"
> > +curr1_input             Measured current.
> > +
> > +power1_label            "pin1"
> > +power1_input            Input power.
> > +
> > +temp1_input             Chip temperature.
> > +=======================
> > +=======================================================
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index b4e93bd5835e..f2b53e8abc3c 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -226,6 +226,15 @@ config SENSORS_LTC3815
> >
> >         This driver can also be built as a module. If so, the module will
> >         be called ltc3815.
> > +config SENSORS_LTC4286
> > +     bool "Analog Devices LTC4286"
> > +     help
> > +       LTC4286 is an integrated solution for hot swap applications that
> > +       allows a board to be safely inserted and removed from a live
> > +       backplane.
> > +       This chip could be used to monitor voltage, current, ...etc.
> > +       If you say yes here you get hardware monitoring support for Analog
> > +       Devices LTC4286.
> >
> >   config SENSORS_MAX15301
> >       tristate "Maxim MAX15301"
> > diff --git a/drivers/hwmon/pmbus/Makefile
> > b/drivers/hwmon/pmbus/Makefile index 84ee960a6c2d..94e28f6d6a61
> 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_LM25066)       +=
> lm25066.o
> >   obj-$(CONFIG_SENSORS_LT7182S)       += lt7182s.o
> >   obj-$(CONFIG_SENSORS_LTC2978)       += ltc2978.o
> >   obj-$(CONFIG_SENSORS_LTC3815)       += ltc3815.o
> > +obj-$(CONFIG_SENSORS_LTC4286)        += ltc4286.o
> >   obj-$(CONFIG_SENSORS_MAX15301)      += max15301.o
> >   obj-$(CONFIG_SENSORS_MAX16064)      += max16064.o
> >   obj-$(CONFIG_SENSORS_MAX16601)      += max16601.o
> > diff --git a/drivers/hwmon/pmbus/ltc4286.c
> > b/drivers/hwmon/pmbus/ltc4286.c new file mode 100644 index
> > 000000000000..e1d72fe9587c
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/ltc4286.c
> > @@ -0,0 +1,160 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pmbus.h>
> > +#include "pmbus.h"
> > +
> > +/* LTC4286 register */
> > +#define LTC4286_MFR_CONFIG1  0xF2
> > +
> > +/* LTC4286 configuration */
> > +#define VRANGE_SELECT_BIT    BIT(1)
> > +
> > +#define LTC4286_MFR_ID_SIZE  3
> > +
> > +enum chips { ltc4286, ltc4287 };
> > +
> > +/*
> > + * Initialize the MBR as default settings which is referred to
> > +LTC4286 datasheet
> > + * (March 22, 2022 version) table 3 page 16  */ static struct
> > +pmbus_driver_info ltc4286_info = {
> > +     .pages = 1,
> > +     .format[PSC_VOLTAGE_IN] = direct,
> > +     .format[PSC_VOLTAGE_OUT] = direct,
> > +     .format[PSC_CURRENT_OUT] = direct,
> > +     .format[PSC_POWER] = direct,
> > +     .format[PSC_TEMPERATURE] = direct,
> > +     .m[PSC_VOLTAGE_IN] = 32,
> > +     .b[PSC_VOLTAGE_IN] = 0,
> > +     .R[PSC_VOLTAGE_IN] = 1,
> > +     .m[PSC_VOLTAGE_OUT] = 32,
> > +     .b[PSC_VOLTAGE_OUT] = 0,
> > +     .R[PSC_VOLTAGE_OUT] = 1,
> > +     .m[PSC_CURRENT_OUT] = 1024,
> > +     .b[PSC_CURRENT_OUT] = 0,
> > +     /*
> > +      * The rsense value used in MBR formula in LTC4286 datasheet
> should be ohm unit.
> > +      * However, the rsense value that user input is mirco ohm.
> > +      * Thus, the MBR setting which involves rsense should be shifted by 6
> digits.
> > +      */
> > +     .R[PSC_CURRENT_OUT] = 3 - 6,
> > +     .m[PSC_POWER] = 1,
> > +     .b[PSC_POWER] = 0,
> > +     /*
> > +      * The rsense value used in MBR formula in LTC4286 datasheet
> should be ohm unit.
> > +      * However, the rsense value that user input is mirco ohm.
> > +      * Thus, the MBR setting which involves rsense should be shifted by 6
> digits.
> > +      */
> > +     .R[PSC_POWER] = 4 - 6,
> > +     .m[PSC_TEMPERATURE] = 1,
> > +     .b[PSC_TEMPERATURE] = 273,
> > +     .R[PSC_TEMPERATURE] = 0,
> > +     .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT |
> PMBUS_HAVE_IOUT |
> > +                PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP |
> PMBUS_HAVE_STATUS_VOUT |
> > +                PMBUS_HAVE_STATUS_IOUT |
> PMBUS_HAVE_STATUS_TEMP, };
> > +
> 
> The datasheet for LTC4286  (in the PMBus description talks about LSB 21.3
> mV/RSENSE
> for IOUT_OC_WARN_LIMIT and 2.8/RSENSE for PIN_OP_WARN_LIMIT. This
> contradicts
> data elsewhere in the datasheet which uses above coefficients for both
> LTC4286
> and LTC4287. I don't know if the datasheet is wrong, but this needs to be
> clarified.
> If the datasheet is wrong, that needs to be mentioned above. If the limit
> registers
> use different coefficients, local code will be needed to adjust values when
> reading /
> writing the registers to have it match coefficients.
We have sent an e-mail about this question.

> 
> > +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> > +                                                { "ltc4287",
> ltc4287 },
> > +                                                {} };
> > +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> > +
> > +static int ltc4286_probe(struct i2c_client *client)
> > +{
> > +     int ret;
> > +     const struct i2c_device_id *mid;
> > +     u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> > +     struct pmbus_driver_info *info;
> > +     u32 rsense;
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer id\n");
> 
> Why not use dev_err_probe() here ?
We will use dev_err_probe() instead of dev_err()

> 
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * Refer to ltc4286 datasheet page 20
> > +      * the manufacturer id is LTC
> > +      */
> > +     if (ret != LTC4286_MFR_ID_SIZE ||
> > +         strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> > +             return dev_err_probe(&client->dev, ret,
> > +                                  "failed to read manufacturer
> id\n");
> 
> This is misleading. It didn't _fail_ to read the manufacturer ID.
We will revise to "Manufacturer id mismatch"

> 
> > +     }
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL,
> block_buffer);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to read manufacturer
> model\n");
> 
> Why not use dev_err_probe() here ?
We will use dev_err_probe() instead of dev_err()

> 
> > +             return ret;
> > +     }
> > +
> > +     for (mid = ltc4286_id; mid->name[0]; mid++) {
> > +             if (!strncasecmp(mid->name, block_buffer,
> strlen(mid->name)))
> > +                     break;
> > +     }
> 
> This is pointless code. If the ID is not found, mid will point after
> the end of the array, and then what ?
> 
> The purpose of such code is to validate if the chip is actually the one
> referenced in the match function and at least warn if that is not the case.
> It should never accept a chip which is _known_ to not be supported.
We will revise as below
for (mid = ltc4286_id; mid->name[0]; mid++) {
	if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
		break;
}
if (!mid->name[0])
	return dev_err_probe(&client->dev, -ENODEV,
                      "Unsupported device\n");

> 
> > +
> > +     ret = of_property_read_u32(client->dev.of_node,
> > +                                "shunt-resistor-micro-ohms",
> &rsense);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (rsense == 0)
> > +             return -EINVAL;
> > +
> > +     info = &ltc4286_info;
> > +
> > +     /* Default of VRANGE_SELECT = 1, 102.4V */
> > +     if (device_property_read_bool(&client->dev,
> "adi,vrange-select-25p6")) {
> 
> What if the adi,vrange-select-25p6 property is not provided, but the chip
> is programmed for this range ?
The binding document tells programmers how to fill the dts.
Thus, programmers must fill this property if their system is 25.6 volts voltage range.

> 
> > +             /* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> > +             ret = i2c_smbus_read_word_data(client,
> LTC4286_MFR_CONFIG1);
> > +             if (ret < 0) {
> > +                     dev_err(&client->dev,
> > +                             "failed to read manufacturer
> configuration one\n"); > +                 return ret;
> > +             }
> > +
> > +             ret &= ~VRANGE_SELECT_BIT; /* VRANGE_SELECT = 0,
> 25.6V */
> > +             ret = i2c_smbus_write_word_data(client,
> LTC4286_MFR_CONFIG1,
> > +                                             ret);
> 
> This should only be written if it actually changed.
We will revise as below
if ((ret & VRANGE_SELECT_BIT) != VRANGE_25P6) {
	ret &= ~VRANGE_SELECT_BIT; /* VRANGE_SELECT = 0, 25.6V */
	ret = i2c_smbus_write_word_data(
            client, LTC4286_MFR_CONFIG1, ret);
    if (ret < 0)
		return dev_err_probe(&client->dev, ret,
                          "Failed to set vrange\n");
}
Moreover, we will check the behavior about this register with vendor.

> 
> > +             if (ret < 0) {
> > +                     dev_err(&client->dev, "failed to set vrange\n");
> > +                     return ret;
> > +             }
> > +
> > +             info->m[PSC_VOLTAGE_IN] = 128;
> > +             info->m[PSC_VOLTAGE_OUT] = 128;
> > +             info->m[PSC_POWER] = 4 * rsense;
> 
> You can not overwrite ltc4286_info because there might be several chips
> in the system with different sense resistor values and range
> configurations.
We will add below to replace the line info = &ltc4286_info;
info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
if (!info)
	return -ENOMEM;
memcpy(info, &ltc4286_info, sizeof(*info));

> 
> Also, the above (and the calculation in the code below) will overflow
> with too-large sense register values.
We will add below to check if the 4*rsense overflow or not.
if (info->m[PSC_POWER] > INT_MAX)
	return dev_err_probe(&client->dev, -ERANGE,
                      "Power coefficient overflow\n");

> 
> > +     } else
> > +             info->m[PSC_POWER] = rsense;
> 
> Please run checkpatch --strict on your patches.
We will run checkpatch --strict to check our patch.

> 
> > +
> > +     info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> 
> Any rsense value larger than MAXINT / 1024 will overflow.
if (info->m[PSC_CURRENT_OUT] > INT_MAX)
	return dev_err_probe(&client->dev, -ERANGE,
                      "Current coefficient overflow\n");

> 
> > +
> > +     return pmbus_do_probe(client, info);
> > +}
> > +
> > +static const struct of_device_id ltc4286_of_match[] = {
> > +     { .compatible = "lltc,ltc4286" },
> > +     { .compatible = "lltc,ltc4287" },
> > +     {}
> > +};
> > +
> > +static struct i2c_driver ltc4286_driver = {
> > +     .driver = {
> > +             .name = "ltc4286",
> > +             .of_match_table = ltc4286_of_match,
> > +     },
> > +     .probe = ltc4286_probe,
> > +     .id_table = ltc4286_id,
> > +};
> > +
> > +module_i2c_driver(ltc4286_driver);
> > +
> > +MODULE_AUTHOR("Delphine CC Chiu
> <Delphine_CC_Chiu@wiwynn.com>");
> > +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> > +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
[ ... ]
>>
>>> +
>>> +     ret = of_property_read_u32(client->dev.of_node,
>>> +                                "shunt-resistor-micro-ohms",
>> &rsense);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     if (rsense == 0)
>>> +             return -EINVAL;
>>> +
>>> +     info = &ltc4286_info;
>>> +
>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
>>> +     if (device_property_read_bool(&client->dev,
>> "adi,vrange-select-25p6")) {
>>
>> What if the adi,vrange-select-25p6 property is not provided, but the chip
>> is programmed for this range ?
> The binding document tells programmers how to fill the dts.
> Thus, programmers must fill this property if their system is 25.6 volts voltage range.
> 

Sure, but there is no else case, meaning VRANGE_SELECT is
unmodified in that case. There is no guarantee that the chip
is in its power-on state.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

[Guenter Roeck]

On 10/30/23 23:25, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
[ ... ]
>>
>> Actually, turns out there is some contradiction in the LTC4286 datasheet.
>> It mentions different coefficients in different places. It is all but impossible to
>> determine if the datasheet is wrong or if the chip uses a variety of coefficients
>> unless one has a real chip available. Sigh :-(.
> We are not the chip vendor, but we could forward your question to vendor.
> Could you point out the exact places (which pages) where are the contradiction in LTC4286 datasheet?
> 

See "PMBUS COMMAND SUMMARY", default values:

"IOUT_OC_WARN_LIMIT" says "21.3 mV/RSENSE"

"PIN_OP_WARN_LIMIT" says "2.8/RSENSE"

This seems to contradict "Table 8. PMBus M, B, and R Parameters". But then,
reading it again (and again), I think it is just an odd and confusing way
of trying to describe the 0x7fff default register values. Sorry for the noise.

Guenter

RE: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, October 31, 2023 9:47 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> <corbet@lwn.net>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> [ ... ]
> >>
> >>> +
> >>> +     ret = of_property_read_u32(client->dev.of_node,
> >>> +                                "shunt-resistor-micro-ohms",
> >> &rsense);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +
> >>> +     if (rsense == 0)
> >>> +             return -EINVAL;
> >>> +
> >>> +     info = &ltc4286_info;
> >>> +
> >>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
> >>> +     if (device_property_read_bool(&client->dev,
> >> "adi,vrange-select-25p6")) {
> >>
> >> What if the adi,vrange-select-25p6 property is not provided, but the
> >> chip is programmed for this range ?
> > The binding document tells programmers how to fill the dts.
> > Thus, programmers must fill this property if their system is 25.6 volts voltage
> range.
> >
> 
> Sure, but there is no else case, meaning VRANGE_SELECT is unmodified in that
> case. There is no guarantee that the chip is in its power-on state.

The else case is in v2 ltc4286.c line 133
It means that the voltage range for programmer is 102.4 volts which is default value,
so driver doesn't need to do any change for VRANGE_SELECT bit.
Additionally, we have checked the behavior of VRANGE_SELECT bit with vendor.
Below is the reply from vendor:
[Our question]
If we change the VRANGE_SELECT bit value to 0, the value would return to default after we restart this device?
Or VRANGE_SELECT bit value would never change util someone change its value?
[ADI reply]
chip reset will go back to default. Thanks.

Thus, we must overwrite this bit if the user fill adi,vrange-select-25p6 here whenever driver probes

> 
> Guenter

Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On 11/6/23 19:08, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Tuesday, October 31, 2023 9:47 PM
>> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
>> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
>> <corbet@lwn.net>
>> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
>> linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> linux-doc@vger.kernel.org
>> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
>>
>>    Security Reminder: Please be aware that this email is sent by an external
>> sender.
>>
>> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
>> [ ... ]
>>>>
>>>>> +
>>>>> +     ret = of_property_read_u32(client->dev.of_node,
>>>>> +                                "shunt-resistor-micro-ohms",
>>>> &rsense);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     if (rsense == 0)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     info = &ltc4286_info;
>>>>> +
>>>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
>>>>> +     if (device_property_read_bool(&client->dev,
>>>> "adi,vrange-select-25p6")) {
>>>>
>>>> What if the adi,vrange-select-25p6 property is not provided, but the
>>>> chip is programmed for this range ?
>>> The binding document tells programmers how to fill the dts.
>>> Thus, programmers must fill this property if their system is 25.6 volts voltage
>> range.
>>>
>>
>> Sure, but there is no else case, meaning VRANGE_SELECT is unmodified in that
>> case. There is no guarantee that the chip is in its power-on state.
> 
> The else case is in v2 ltc4286.c line 133
> It means that the voltage range for programmer is 102.4 volts which is default value,
> so driver doesn't need to do any change for VRANGE_SELECT bit.

There is no guarantee that the value wasn't changed before the driver was loaded.

Guenter

> Additionally, we have checked the behavior of VRANGE_SELECT bit with vendor.
> Below is the reply from vendor:
> [Our question]
> If we change the VRANGE_SELECT bit value to 0, the value would return to default after we restart this device?
> Or VRANGE_SELECT bit value would never change util someone change its value?
> [ADI reply]
> chip reset will go back to default. Thanks.
> 
> Thus, we must overwrite this bit if the user fill adi,vrange-select-25p6 here whenever driver probes
> 
>>
>> Guenter
> 

RE: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, November 7, 2023 11:30 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> <corbet@lwn.net>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 11/6/23 19:08, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> >> -----Original Message-----
> >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> >> Sent: Tuesday, October 31, 2023 9:47 PM
> >> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> >> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> >> <corbet@lwn.net>
> >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> >> <conor+dt@kernel.org>; linux-i2c@vger.kernel.org;
> >> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> >> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> >>
> >>    Security Reminder: Please be aware that this email is sent by an
> >> external sender.
> >>
> >> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> >> [ ... ]
> >>>>
> >>>>> +
> >>>>> +     ret = of_property_read_u32(client->dev.of_node,
> >>>>> +                                "shunt-resistor-micro-ohms",
> >>>> &rsense);
> >>>>> +     if (ret < 0)
> >>>>> +             return ret;
> >>>>> +
> >>>>> +     if (rsense == 0)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     info = &ltc4286_info;
> >>>>> +
> >>>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
> >>>>> +     if (device_property_read_bool(&client->dev,
> >>>> "adi,vrange-select-25p6")) {
> >>>>
> >>>> What if the adi,vrange-select-25p6 property is not provided, but
> >>>> the chip is programmed for this range ?
> >>> The binding document tells programmers how to fill the dts.
> >>> Thus, programmers must fill this property if their system is 25.6
> >>> volts voltage
> >> range.
> >>>
> >>
> >> Sure, but there is no else case, meaning VRANGE_SELECT is unmodified
> >> in that case. There is no guarantee that the chip is in its power-on state.
> >
> > The else case is in v2 ltc4286.c line 133 It means that the voltage
> > range for programmer is 102.4 volts which is default value, so driver
> > doesn't need to do any change for VRANGE_SELECT bit.
> 
> There is no guarantee that the value wasn't changed before the driver was
> loaded.

We still can’t get your point.
Could you tell us about your concern here?
Thanks.

> 
> Guenter
> 
> > Additionally, we have checked the behavior of VRANGE_SELECT bit with
> vendor.
> > Below is the reply from vendor:
> > [Our question]
> > If we change the VRANGE_SELECT bit value to 0, the value would return to
> default after we restart this device?
> > Or VRANGE_SELECT bit value would never change util someone change its
> value?
> > [ADI reply]
> > chip reset will go back to default. Thanks.
> >
> > Thus, we must overwrite this bit if the user fill
> > adi,vrange-select-25p6 here whenever driver probes
> >
> >>
> >> Guenter
> >

Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On Wed, Nov 15, 2023 at 08:42:22AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Tuesday, November 7, 2023 11:30 AM
> > To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> > patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> > <corbet@lwn.net>
> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> > linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-doc@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > 
> >   Security Reminder: Please be aware that this email is sent by an external
> > sender.
> > 
> > On 11/6/23 19:08, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > >> -----Original Message-----
> > >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > >> Sent: Tuesday, October 31, 2023 9:47 PM
> > >> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> > >> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> > >> <corbet@lwn.net>
> > >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > >> <conor+dt@kernel.org>; linux-i2c@vger.kernel.org;
> > >> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > >> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> > >> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > >>
> > >>    Security Reminder: Please be aware that this email is sent by an
> > >> external sender.
> > >>
> > >> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > >> [ ... ]
> > >>>>
> > >>>>> +
> > >>>>> +     ret = of_property_read_u32(client->dev.of_node,
> > >>>>> +                                "shunt-resistor-micro-ohms",
> > >>>> &rsense);
> > >>>>> +     if (ret < 0)
> > >>>>> +             return ret;
> > >>>>> +
> > >>>>> +     if (rsense == 0)
> > >>>>> +             return -EINVAL;
> > >>>>> +
> > >>>>> +     info = &ltc4286_info;
> > >>>>> +
> > >>>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
> > >>>>> +     if (device_property_read_bool(&client->dev,
> > >>>> "adi,vrange-select-25p6")) {
> > >>>>
> > >>>> What if the adi,vrange-select-25p6 property is not provided, but
> > >>>> the chip is programmed for this range ?
> > >>> The binding document tells programmers how to fill the dts.
> > >>> Thus, programmers must fill this property if their system is 25.6
> > >>> volts voltage
> > >> range.
> > >>>
> > >>
> > >> Sure, but there is no else case, meaning VRANGE_SELECT is unmodified
> > >> in that case. There is no guarantee that the chip is in its power-on state.
> > >
> > > The else case is in v2 ltc4286.c line 133 It means that the voltage
> > > range for programmer is 102.4 volts which is default value, so driver
> > > doesn't need to do any change for VRANGE_SELECT bit.
> > 
> > There is no guarantee that the value wasn't changed before the driver was
> > loaded.
> 
> We still can’t get your point.
> Could you tell us about your concern here?

I have repeated it several times. You are making assumptions about
register values when the driver is loaded. Those asumptions
are wrong since the state of the chip is unknown when the driver
is loaded. Any entty (BIOS, ROMMON, i2cset, some operating system
loaded earlier, or even some other driver or platform code) may
have changed those values.

On top of that, as I also have pointed out, LTC4287 supports
saving its configuration data in eeprom. That means that any chip
configuration set during production or anytime later will be
retained, meaning any assumption about chip configuration
when the driver is loaded is even more wrong.

Guenter

RE: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, November 16, 2023 6:46 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> <corbet@lwn.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> linux-i2c@vger.kernel.org; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On Wed, Nov 15, 2023 at 08:42:22AM +0000,
> Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > > Sent: Tuesday, November 7, 2023 11:30 AM
> > > To: Delphine_CC_Chiu/WYHQ/Wiwynn
> <Delphine_CC_Chiu@wiwynn.com>;
> > > patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan Corbet
> > > <corbet@lwn.net>
> > > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > > <conor+dt@kernel.org>; linux-i2c@vger.kernel.org;
> > > linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> > > Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > >
> > >   Security Reminder: Please be aware that this email is sent by an
> > > external sender.
> > >
> > > On 11/6/23 19:08, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > > >> -----Original Message-----
> > > >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> > > >> Roeck
> > > >> Sent: Tuesday, October 31, 2023 9:47 PM
> > > >> To: Delphine_CC_Chiu/WYHQ/Wiwynn
> <Delphine_CC_Chiu@wiwynn.com>;
> > > >> patrick@stwcx.xyz; Jean Delvare <jdelvare@suse.com>; Jonathan
> > > >> Corbet <corbet@lwn.net>
> > > >> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > > >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > > >> <conor+dt@kernel.org>; linux-i2c@vger.kernel.org;
> > > >> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > > >> linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> > > >> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> > > >>
> > > >>    Security Reminder: Please be aware that this email is sent by
> > > >> an external sender.
> > > >>
> > > >> On 10/30/23 23:46, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> > > >> [ ... ]
> > > >>>>
> > > >>>>> +
> > > >>>>> +     ret = of_property_read_u32(client->dev.of_node,
> > > >>>>> +
> "shunt-resistor-micro-ohms",
> > > >>>> &rsense);
> > > >>>>> +     if (ret < 0)
> > > >>>>> +             return ret;
> > > >>>>> +
> > > >>>>> +     if (rsense == 0)
> > > >>>>> +             return -EINVAL;
> > > >>>>> +
> > > >>>>> +     info = &ltc4286_info;
> > > >>>>> +
> > > >>>>> +     /* Default of VRANGE_SELECT = 1, 102.4V */
> > > >>>>> +     if (device_property_read_bool(&client->dev,
> > > >>>> "adi,vrange-select-25p6")) {
> > > >>>>
> > > >>>> What if the adi,vrange-select-25p6 property is not provided,
> > > >>>> but the chip is programmed for this range ?
> > > >>> The binding document tells programmers how to fill the dts.
> > > >>> Thus, programmers must fill this property if their system is
> > > >>> 25.6 volts voltage
> > > >> range.
> > > >>>
> > > >>
> > > >> Sure, but there is no else case, meaning VRANGE_SELECT is
> > > >> unmodified in that case. There is no guarantee that the chip is in its
> power-on state.
> > > >
> > > > The else case is in v2 ltc4286.c line 133 It means that the
> > > > voltage range for programmer is 102.4 volts which is default
> > > > value, so driver doesn't need to do any change for VRANGE_SELECT bit.
> > >
> > > There is no guarantee that the value wasn't changed before the
> > > driver was loaded.
> >
> > We still can’t get your point.
> > Could you tell us about your concern here?
> 
> I have repeated it several times. You are making assumptions about register
> values when the driver is loaded. Those asumptions are wrong since the state
> of the chip is unknown when the driver is loaded. Any entty (BIOS, ROMMON,
> i2cset, some operating system loaded earlier, or even some other driver or
> platform code) may have changed those values.
> 
> On top of that, as I also have pointed out, LTC4287 supports saving its
> configuration data in eeprom. That means that any chip configuration set
> during production or anytime later will be retained, meaning any assumption
> about chip configuration when the driver is loaded is even more wrong.

OK
We will check the register value first.
Then, we will check the property in dts.

> 
> Guenter