[PATCH 0/2] Add support for LTC2971 power manager

[PATCH 0/2] Add support for LTC2971 power manager

[Patryk Biel]

This series introduces support for LTC2971 power manager.
The LTC2971 is similiar to already supported LTC2972 in terms of the
number of channels and the register set, it differs however in
the supported voltage range.

Signed-off-by: Patryk Biel <pbiel7@gmail.com>
---
Patryk Biel (2):
      hwmon: pmbus: Add support for ltc2971
      dt-bindings: hwmon: Add ltc2971 bindings

 .../devicetree/bindings/hwmon/lltc,ltc2978.yaml    |  2 ++
 drivers/hwmon/pmbus/ltc2978.c                      | 34 ++++++++++++++++++++--
 2 files changed, 33 insertions(+), 3 deletions(-)
---
base-commit: c2ee9f594da826bea183ed14f2cc029c719bf4da
change-id: 20240930-add-ltc2971-1b493fd21b64

Best regards,
-- 
Patryk Biel <pbiel7@gmail.com>

[PATCH 1/2] hwmon: pmbus: Add support for ltc2971

[Patryk Biel]

LTC2971 is a power manager similar to already supported LTC2972,
it uses the same register set but supports a different voltage range.

Signed-off-by: Patryk Biel <pbiel7@gmail.com>
---
 drivers/hwmon/pmbus/ltc2978.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 73a86f4d647288be97615f19a5c6314e4036e6a3..681e7b811dbcc263fe8f5e4f9da30fcbc7e019ad 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -21,7 +21,8 @@
 
 enum chips {
 	/* Managers */
-	ltc2972, ltc2974, ltc2975, ltc2977, ltc2978, ltc2979, ltc2980,
+	ltc2971, ltc2972, ltc2974, ltc2975, ltc2977, ltc2978,
+	ltc2979, ltc2980,
 	/* Controllers */
 	ltc3880, ltc3882, ltc3883, ltc3884, ltc3886, ltc3887, ltc3889, ltc7132, ltc7880,
 	/* Modules */
@@ -61,6 +62,7 @@ enum chips {
 
 #define LTC2978_ID_MASK			0xfff0
 
+#define LTC2971_ID			0x0320
 #define LTC2972_ID			0x0310
 #define LTC2974_ID			0x0210
 #define LTC2975_ID			0x0220
@@ -533,6 +535,7 @@ static int ltc2978_write_word_data(struct i2c_client *client, int page,
 }
 
 static const struct i2c_device_id ltc2978_id[] = {
+	{"ltc2971", ltc2971},
 	{"ltc2972", ltc2972},
 	{"ltc2974", ltc2974},
 	{"ltc2975", ltc2975},
@@ -564,11 +567,19 @@ MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 
 #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
 #define LTC2978_ADC_RES	0xFFFF
+#define LTC2978_UV_STEP	1000
+
+/* Common for most chips */
 #define LTC2978_N_ADC	122
 #define LTC2978_MAX_UV	(LTC2978_ADC_RES * LTC2978_N_ADC)
-#define LTC2978_UV_STEP	1000
 #define LTC2978_N_VOLTAGES	((LTC2978_MAX_UV / LTC2978_UV_STEP) + 1)
 
+/* LTC2971 */
+#define LTC2971_N_ADC	4500
+#define LTC2971_MAX_UV	(LTC2978_ADC_RES * LTC2971_N_ADC)
+#define LTC2971_N_VOLTAGES	((LTC2971_MAX_UV / LTC2978_UV_STEP) + 1)
+
+/* Common for most chips */
 static const struct regulator_desc ltc2978_reg_desc[] = {
 	PMBUS_REGULATOR_STEP("vout", 0, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
 	PMBUS_REGULATOR_STEP("vout", 1, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
@@ -580,6 +591,12 @@ static const struct regulator_desc ltc2978_reg_desc[] = {
 	PMBUS_REGULATOR_STEP("vout", 7, LTC2978_N_VOLTAGES, LTC2978_UV_STEP, 0),
 };
 
+/* LTC2971 */
+static const struct regulator_desc ltc2971_reg_desc[] = {
+	PMBUS_REGULATOR_STEP("vout", 0, LTC2971_N_VOLTAGES, LTC2978_UV_STEP, 0),
+	PMBUS_REGULATOR_STEP("vout", 1, LTC2971_N_VOLTAGES, LTC2978_UV_STEP, 0),
+};
+
 static const struct regulator_desc ltc2978_reg_desc_default[] = {
 	PMBUS_REGULATOR("vout", 0),
 	PMBUS_REGULATOR("vout", 1),
@@ -624,7 +641,9 @@ static int ltc2978_get_id(struct i2c_client *client)
 
 	chip_id &= LTC2978_ID_MASK;
 
-	if (chip_id == LTC2972_ID)
+	if (chip_id == LTC2971_ID)
+		return ltc2971;
+	else if (chip_id == LTC2972_ID)
 		return ltc2972;
 	else if (chip_id == LTC2974_ID)
 		return ltc2974;
@@ -731,6 +750,7 @@ static int ltc2978_probe(struct i2c_client *client)
 	data->temp2_max = 0x7c00;
 
 	switch (data->id) {
+	case ltc2971:
 	case ltc2972:
 		info->read_word_data = ltc2975_read_word_data;
 		info->pages = LTC2972_NUM_PAGES;
@@ -861,6 +881,13 @@ static int ltc2978_probe(struct i2c_client *client)
 #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
 	info->num_regulators = info->pages;
 	switch (data->id) {
+	case ltc2971:
+		info->reg_desc = ltc2971_reg_desc;
+		if (info->num_regulators > ARRAY_SIZE(ltc2971_reg_desc)) {
+			dev_warn(&client->dev, "num_regulators too large!");
+			info->num_regulators = ARRAY_SIZE(ltc2971_reg_desc);
+		}
+		break;
 	case ltc2972:
 	case ltc2974:
 	case ltc2975:
@@ -892,6 +919,7 @@ static int ltc2978_probe(struct i2c_client *client)
 
 #ifdef CONFIG_OF
 static const struct of_device_id ltc2978_of_match[] = {
+	{ .compatible = "lltc,ltc2971" },
 	{ .compatible = "lltc,ltc2972" },
 	{ .compatible = "lltc,ltc2974" },
 	{ .compatible = "lltc,ltc2975" },

-- 
2.43.0

[PATCH 2/2] dt-bindings: hwmon: Add ltc2971 bindings

[Patryk Biel]

Add device-tree bindings and documentation for LTC2971.

Signed-off-by: Patryk Biel <pbiel7@gmail.com>
---
 Documentation/devicetree/bindings/hwmon/lltc,ltc2978.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc2978.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc2978.yaml
index 1f98da32f3feb9899ccdcc15e70ce275a74c63c3..dc3d19a383e8098516d6d2e6ff50f52883c4b8cf 100644
--- a/Documentation/devicetree/bindings/hwmon/lltc,ltc2978.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc2978.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     enum:
+      - lltc,ltc2971
       - lltc,ltc2972
       - lltc,ltc2974
       - lltc,ltc2975
@@ -45,6 +46,7 @@ properties:
     description: |
       list of regulators provided by this controller.
       Valid names of regulators depend on number of supplies supported per device:
+      * ltc2971 vout0 - vout1
       * ltc2972 vout0 - vout1
       * ltc2974, ltc2975 : vout0 - vout3
       * ltc2977, ltc2979, ltc2980, ltm2987 : vout0 - vout7

-- 
2.43.0

Re: [PATCH 1/2] hwmon: pmbus: Add support for ltc2971

[Guenter Roeck]

On 10/26/24 02:08, Patryk Biel wrote:
> LTC2971 is a power manager similar to already supported LTC2972,
> it uses the same register set but supports a different voltage range.
> 
> Signed-off-by: Patryk Biel <pbiel7@gmail.com>
> ---
>   drivers/hwmon/pmbus/ltc2978.c | 34 +++++++++++++++++++++++++++++++---

Please also update Documentation/hwmon/ltc2978.rst and drivers/hwmon/pmbus/Kconfig.

>   1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index 73a86f4d647288be97615f19a5c6314e4036e6a3..681e7b811dbcc263fe8f5e4f9da30fcbc7e019ad 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -21,7 +21,8 @@
>   
>   enum chips {
>   	/* Managers */
> -	ltc2972, ltc2974, ltc2975, ltc2977, ltc2978, ltc2979, ltc2980,
> +	ltc2971, ltc2972, ltc2974, ltc2975, ltc2977, ltc2978,
> +	ltc2979, ltc2980,
>   	/* Controllers */
>   	ltc3880, ltc3882, ltc3883, ltc3884, ltc3886, ltc3887, ltc3889, ltc7132, ltc7880,
>   	/* Modules */
> @@ -61,6 +62,7 @@ enum chips {
>   
>   #define LTC2978_ID_MASK			0xfff0
>   
> +#define LTC2971_ID			0x0320
>   #define LTC2972_ID			0x0310
>   #define LTC2974_ID			0x0210
>   #define LTC2975_ID			0x0220
> @@ -533,6 +535,7 @@ static int ltc2978_write_word_data(struct i2c_client *client, int page,
>   }
>   
>   static const struct i2c_device_id ltc2978_id[] = {
> +	{"ltc2971", ltc2971},
>   	{"ltc2972", ltc2972},
>   	{"ltc2974", ltc2974},
>   	{"ltc2975", ltc2975},
> @@ -564,11 +567,19 @@ MODULE_DEVICE_TABLE(i2c, ltc2978_id);
>   
>   #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
>   #define LTC2978_ADC_RES	0xFFFF
> +#define LTC2978_UV_STEP	1000
> +
> +/* Common for most chips */
>   #define LTC2978_N_ADC	122
>   #define LTC2978_MAX_UV	(LTC2978_ADC_RES * LTC2978_N_ADC)
> -#define LTC2978_UV_STEP	1000
>   #define LTC2978_N_VOLTAGES	((LTC2978_MAX_UV / LTC2978_UV_STEP) + 1)
>   

Please just add the chip specific defines as you do below, and don't move
the defines above around. It happens a lot that there are common definitions
followed by chip specific ones. Spelling that out explicitly doesn't really
add much if any value and just results in rearranged code whenever a new
chip with other parameters is added.

> +/* LTC2971 */
> +#define LTC2971_N_ADC	4500
> +#define LTC2971_MAX_UV	(LTC2978_ADC_RES * LTC2971_N_ADC)

I think something is wrong here.

4500 * 65535 uV ~= 295V.

However, the datasheet says that the output voltage is calculated by
(register value / 1024), where the register value is a 16-bit value.
65535 / 1024 ~= 64V. Where does the 4,500 come from ?

> +#define LTC2971_N_VOLTAGES	((LTC2971_MAX_UV / LTC2978_UV_STEP) + 1)

Does the chip really support 294,908 voltages ? That seems impossible since
the VOUT_COMMAND register is only 16 bit wide.

Unless I misunderstand the datasheet, the voltage resolution should be 1/1024V
or 976 uV, and there should be 65,536 voltages. This is from the chip datasheet,
PMBus Command Description, Note 1: Data Formats, for L16 data.

If my understanding is wrong, please explain and document your numbers.

Thanks,
Guenter

Re: [PATCH 2/2] dt-bindings: hwmon: Add ltc2971 bindings

[Krzysztof Kozlowski]

On 26/10/2024 11:08, Patryk Biel wrote:
> Add device-tree bindings and documentation for LTC2971.
> 
> Signed-off-by: Patryk Biel <pbiel7@gmail.com>

ABI documentation goes before its user, so please re-order the patches.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: pmbus: Add support for ltc2971

[Patryk]

Hi,
> Unless I misunderstand the datasheet, the voltage resolution should be 1/1024V
> or 976 uV, and there should be 65,536 voltages. This is from the chip datasheet,
> PMBus Command Description, Note 1: Data Formats, for L16 data.
I really screwed up, I took 4500 (N_ADC 4.5 mV) from datasheet from
ADC characteristics section and mistakenly interpreted it as the
voltage resolution. Should've spent more time on this, please
disregard this patch series and I apologize for the hassle.

Best regards,
Patryk