[PATCH v4 0/4] Add support for MAX20830C and MAX20840C step-down DC-DC switching regulator

[PATCH v4 0/4] Add support for MAX20830C and MAX20840C step-down DC-DC switching regulator

[Alexis Czezar Torreno]

This series adds a few features before adding support to MAX20830C and
MAX20840C.

Patches 1 and 2 adds new resistor feedback properties that is needed to
properly scale VOUT

Patches 3 and 4 adds support for devices MAX20830C and MAX20840C.

MAX20830C and MAX20840 are step-down DC-DC switching regulator with PMBus
interface. MAX20830C is a different packaging for MAX20830, and MAX20840C
supports 40A regulation compared to MAX20830 that is only 30A.

Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
---
Changes in v4:
- Patch 1/4:
    - Added missing #include for gpio
    - Removed review tag, patch edited to complete examples
- Patch 2/4:
    - casted feedback resistors to u64 to avoid possible overflow
      during scaling
- Link to v3: https://lore.kernel.org/r/20260720-dev-max20830c-v3-0-9d06d27d6da3@analog.com

Changes in v3:
- Dropped patch that add EN-GPIO property in DTS
    - discussed to be skippable until an actual use-case arise
- Patch 1/4: 
    - Carried over the completed examples from previously dropped patch
    - Kept the review tag as the carried info from the dropped patched
      was also reviewed.
- Patch 2/4: 
    - added clamping to read_vout during scaling
    - improved comments regarding future regulator support 
- Link to v2: https://lore.kernel.org/r/20260706-dev-max20830c-v2-0-37761e89bb5f@analog.com

Changes in v2:
- Patch 1/5: completing example
- Dropped patch adding GPIO enable in driver (no use case)
- Patch 3/5: added notes on READ_VOUT and related VOUT commands
- Patch 4/5: removed comment on fallback
- Patch 5/5: simplified device ID check handling
- Link to v1: https://lore.kernel.org/r/20260630-dev-max20830c-v1-0-a02786bde470@analog.com

---
Alexis Czezar Torreno (4):
      dt-bindings: hwmon: (pmbus/max20830): add VOUT feedback resistor properties and complete examples
      hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
      dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support
      hwmon: (pmbus/max20830): add support for max20830c and max20840c

 .../bindings/hwmon/pmbus/adi,max20830.yaml         | 30 +++++++-
 Documentation/hwmon/max20830.rst                   | 27 +++++--
 drivers/hwmon/pmbus/max20830.c                     | 87 ++++++++++++++++++----
 3 files changed, 124 insertions(+), 20 deletions(-)
---
base-commit: 1a42625604046790ea8ec61c2a749bbf76b28943
change-id: 20260630-dev-max20830c-9d38d8b0a765

Best regards,
-- 
Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

[PATCH v4 1/4] dt-bindings: hwmon: (pmbus/max20830): add VOUT feedback resistor properties and complete examples

[Alexis Czezar Torreno]

Add adi,vout-rfb1-ohms and adi,vout-rfb2-ohms properties to support
external voltage divider configuration for VOUT sensing. When the
desired output voltage is higher than VREF, a resistor divider (RFB1
and RFB2) is required to reach the intended value.

The properties use a dependency constraint to ensure both resistors
are specified together, or neither. This prevents misconfiguration
where only one resistor value is provided.

This patch also added missing entries in the examples.

Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
---
 .../bindings/hwmon/pmbus/adi,max20830.yaml         | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
index 1625dd59417f1b3ca689a9c86ca266da913d1217..01b7275f5b3419cf1b791ed8f9b7ae56cd2961b5 100644
--- a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
@@ -44,15 +44,32 @@ properties:
       GPIO connected to the power-good status output pin.
     maxItems: 1
 
+  adi,vout-rfb1-ohms:
+    description:
+      Top feedback resistor (RFB1) value in ohms for VOUT sensing divider.
+      When the desired output voltage is higher than VREF, a resistor divider
+      is required. VOUT = VREF × (1 + RFB1/RFB2)
+
+  adi,vout-rfb2-ohms:
+    description:
+      Bottom feedback resistor (RFB2) value in ohms for VOUT sensing divider.
+      Datasheet recommends that RFB2 does not exceed 2.5kΩ.
+
 required:
   - compatible
   - reg
   - vddh-supply
 
+dependencies:
+  adi,vout-rfb1-ohms: ['adi,vout-rfb2-ohms']
+  adi,vout-rfb2-ohms: ['adi,vout-rfb1-ohms']
+
 unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -61,6 +78,11 @@ examples:
             compatible = "adi,max20830";
             reg = <0x30>;
             vddh-supply = <&vddh>;
+            avdd-supply = <&avdd>;
+            ldoin-supply = <&ldoin>;
+            pwr-good-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+            adi,vout-rfb1-ohms = <10000>;
+            adi,vout-rfb2-ohms = <2000>;
         };
     };
 ...

-- 
2.34.1

Re: [PATCH v4 1/4] dt-bindings: hwmon: (pmbus/max20830): add VOUT feedback resistor properties and complete examples

[sashiko-bot]

> Add adi,vout-rfb1-ohms and adi,vout-rfb2-ohms properties to support
> external voltage divider configuration for VOUT sensing. When the
> desired output voltage is higher than VREF, a resistor divider (RFB1
> and RFB2) is required to reach the intended value.
> 
> The properties use a dependency constraint to ensure both resistors
> are specified together, or neither. This prevents misconfiguration
> where only one resistor value is provided.
> 
> This patch also added missing entries in the examples.
> 
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-dev-max20830c-v4-0-cec034298535@analog.com?part=1

Re: [PATCH v4 1/4] dt-bindings: hwmon: (pmbus/max20830): add VOUT feedback resistor properties and complete examples

[Krzysztof Kozlowski]

On Tue, Jul 28, 2026 at 11:32:55AM +0800, Alexis Czezar Torreno wrote:
> Add adi,vout-rfb1-ohms and adi,vout-rfb2-ohms properties to support
> external voltage divider configuration for VOUT sensing. When the
> desired output voltage is higher than VREF, a resistor divider (RFB1
> and RFB2) is required to reach the intended value.
> 
> The properties use a dependency constraint to ensure both resistors
> are specified together, or neither. This prevents misconfiguration
> where only one resistor value is provided.
> 
> This patch also added missing entries in the examples.

"Add..."


> 
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> ---
>  .../bindings/hwmon/pmbus/adi,max20830.yaml         | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

[PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

[Alexis Czezar Torreno]

Implement support for external voltage divider scaling using the
adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties.

When the desired output voltage exceeds VREF, a resistor divider
(RFB1 and RFB2) is used to scale down the feedback voltage. The
driver reads these resistor values from device tree and applies
the scaling formula: VOUT_actual = VOUT_measured × (1 + RFB1/RFB2)

The properties are optional. If not specified, the driver assumes
no voltage divider is present and reports the raw VOUT reading.

Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
---
 drivers/hwmon/pmbus/max20830.c | 60 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
index cb2c23672166d641852199ca07eb716924f4f286..7c1e94c43ac4e5d1b4f69dc77b02e424a674e908 100644
--- a/drivers/hwmon/pmbus/max20830.c
+++ b/drivers/hwmon/pmbus/max20830.c
@@ -7,6 +7,7 @@
 
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/string.h>
@@ -14,6 +15,51 @@
 
 #define MAX20830_IC_DEVICE_ID_LENGTH	9
 
+struct max20830_data {
+	struct pmbus_driver_info info;
+	u32 vout_rfb1;
+	u32 vout_rfb2;
+};
+
+/*
+ * MAX20830 only supports READ_VOUT for VOUT monitoring.
+ *
+ * MAX20830 uses an external resistor divider for voltage sensing:
+ * - VOUT_COMMAND and VOUT_MAX set the reference voltage at the feedback pin
+ * - READ_VOUT reports the feedback voltage, which needs to be scaled for actual
+ *   output voltage
+ *
+ * Scaling formula: vout_actual = vout_fb × (1 + RFB1 / RFB2)
+ *
+ * If regulator support is added in the future, some adjustments are needed to
+ * ensure correct feedback voltages are set.
+ */
+static int max20830_read_word_data(struct i2c_client *client, int page,
+				   int phase, int reg)
+{
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	const struct max20830_data *data = container_of(info, struct max20830_data, info);
+	int ret;
+	u64 temp;
+
+	switch (reg) {
+	case PMBUS_READ_VOUT:
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret < 0)
+			return ret;
+
+		/* Apply voltage divider scaling if resistors are non-zero */
+		if (data->vout_rfb1 && data->vout_rfb2) {
+			temp = (u64)data->vout_rfb1 + (u64)data->vout_rfb2;
+			temp = DIV_ROUND_CLOSEST_ULL((u64)ret * temp, data->vout_rfb2);
+			ret = clamp_val(temp, 0, 0xFFFF);
+		}
+		return ret;
+	default:
+		return -ENODATA;
+	}
+}
+
 static struct pmbus_driver_info max20830_info = {
 	.pages = 1,
 	.format[PSC_VOLTAGE_IN] = linear,
@@ -24,13 +70,25 @@ static struct pmbus_driver_info max20830_info = {
 		PMBUS_HAVE_TEMP |
 		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
 		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
+	.read_word_data = max20830_read_word_data,
 };
 
 static int max20830_probe(struct i2c_client *client)
 {
 	u8 buf[I2C_SMBUS_BLOCK_MAX + 1] = {};
+	struct max20830_data *data;
 	int ret;
 
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->info = max20830_info;
+
+	/* Read optional voltage divider resistor values */
+	device_property_read_u32(&client->dev, "adi,vout-rfb1-ohms", &data->vout_rfb1);
+	device_property_read_u32(&client->dev, "adi,vout-rfb2-ohms", &data->vout_rfb2);
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA) &&
 	    !i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK))
 		return -ENODEV;
@@ -78,7 +136,7 @@ static int max20830_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, -ENODEV,
 				     "Unsupported device: '%s'\n", buf);
 
-	return pmbus_do_probe(client, &max20830_info);
+	return pmbus_do_probe(client, &data->info);
 }
 
 static const struct i2c_device_id max20830_id[] = {

-- 
2.34.1

Re: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete VOUT scaling introduces UAPI inconsistency and dangerous hardware limit writes.
--

--- Patch [2]: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support ---
commit 4d1fa3c4cb8d1f4bfe3ca16a79ad028a0dd9bfee
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

This commit adds support for external voltage divider scaling using the
adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties. It reads
these properties and scales the feedback voltage for PMBUS_READ_VOUT.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index cb2c23672166d..7c1e94c43ac4e 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c
[ ... ]
> @@ -14,6 +15,51 @@
[ ... ]
> +static int max20830_read_word_data(struct i2c_client *client, int page,
> +				   int phase, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	const struct max20830_data *data = container_of(info, struct max20830_data, info);
> +	int ret;
> +	u64 temp;
> +
> +	switch (reg) {
> +	case PMBUS_READ_VOUT:
> +		ret = pmbus_read_word_data(client, page, phase, reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Apply voltage divider scaling if resistors are non-zero */
> +		if (data->vout_rfb1 && data->vout_rfb2) {
> +			temp = (u64)data->vout_rfb1 + (u64)data->vout_rfb2;
> +			temp = DIV_ROUND_CLOSEST_ULL((u64)ret * temp, data->vout_rfb2);
> +			ret = clamp_val(temp, 0, 0xFFFF);
> +		}
> +		return ret;
> +	default:
> +		return -ENODATA;
> +	}
> +}

[Severity: High]
Does this code introduce a UAPI inconsistency by only scaling PMBUS_READ_VOUT?

When in1_input is read, it returns the scaled output voltage.

However, because this switch returns -ENODATA for limits (like
PMBUS_VOUT_OV_WARN_LIMIT or PMBUS_VOUT_OV_FAULT_LIMIT), reading limits like
in1_max will return the unscaled feedback voltage directly from the chip.

> +
>  static struct pmbus_driver_info max20830_info = {
>  	.pages = 1,
>  	.format[PSC_VOLTAGE_IN] = linear,
> @@ -24,13 +70,25 @@ static struct pmbus_driver_info max20830_info = {
>  		PMBUS_HAVE_TEMP |
>  		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
>  		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
> +	.read_word_data = max20830_read_word_data,
>  };

[Severity: High]
Should there be a matching write_word_data callback here to reverse the
scaling for limit registers?

Since there is no write_word_data callback, if a user writes a new limit using
the scaled unit, the core will write it directly to the hardware without
reverse-scaling.

The chip treats this as a feedback limit, which could drastically raise the
physical overvoltage trip point and bypass hardware protection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-dev-max20830c-v4-0-cec034298535@analog.com?part=2

Re: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

[Guenter Roeck]

On 7/27/26 20:45, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incomplete VOUT scaling introduces UAPI inconsistency and dangerous hardware limit writes.
> --
> 
> --- Patch [2]: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support ---
> commit 4d1fa3c4cb8d1f4bfe3ca16a79ad028a0dd9bfee
> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> 
> hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
> 
> This commit adds support for external voltage divider scaling using the
> adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties. It reads
> these properties and scales the feedback voltage for PMBUS_READ_VOUT.
> 
>> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
>> index cb2c23672166d..7c1e94c43ac4e 100644
>> --- a/drivers/hwmon/pmbus/max20830.c
>> +++ b/drivers/hwmon/pmbus/max20830.c
> [ ... ]
>> @@ -14,6 +15,51 @@
> [ ... ]
>> +static int max20830_read_word_data(struct i2c_client *client, int page,
>> +				   int phase, int reg)
>> +{
>> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>> +	const struct max20830_data *data = container_of(info, struct max20830_data, info);
>> +	int ret;
>> +	u64 temp;
>> +
>> +	switch (reg) {
>> +	case PMBUS_READ_VOUT:
>> +		ret = pmbus_read_word_data(client, page, phase, reg);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Apply voltage divider scaling if resistors are non-zero */
>> +		if (data->vout_rfb1 && data->vout_rfb2) {
>> +			temp = (u64)data->vout_rfb1 + (u64)data->vout_rfb2;
>> +			temp = DIV_ROUND_CLOSEST_ULL((u64)ret * temp, data->vout_rfb2);
>> +			ret = clamp_val(temp, 0, 0xFFFF);
>> +		}
>> +		return ret;
>> +	default:
>> +		return -ENODATA;
>> +	}
>> +}
> 
> [Severity: High]
> Does this code introduce a UAPI inconsistency by only scaling PMBUS_READ_VOUT?
> 
> When in1_input is read, it returns the scaled output voltage.
> 
> However, because this switch returns -ENODATA for limits (like
> PMBUS_VOUT_OV_WARN_LIMIT or PMBUS_VOUT_OV_FAULT_LIMIT), reading limits like
> in1_max will return the unscaled feedback voltage directly from the chip.
> 

I would suggest to add a comment explaining that this series does not support
any limit registers. That should "address" the issues raised by Sashiko, and also
help future readers understand why the limit registers are not scaled.

Thanks,
Guenter

>> +
>>   static struct pmbus_driver_info max20830_info = {
>>   	.pages = 1,
>>   	.format[PSC_VOLTAGE_IN] = linear,
>> @@ -24,13 +70,25 @@ static struct pmbus_driver_info max20830_info = {
>>   		PMBUS_HAVE_TEMP |
>>   		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
>>   		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
>> +	.read_word_data = max20830_read_word_data,
>>   };
> 
> [Severity: High]
> Should there be a matching write_word_data callback here to reverse the
> scaling for limit registers?
> 
> Since there is no write_word_data callback, if a user writes a new limit using
> the scaled unit, the core will write it directly to the hardware without
> reverse-scaling.
> 
> The chip treats this as a feedback limit, which could drastically raise the
> physical overvoltage trip point and bypass hardware protection.
> 

RE: [PATCH v4 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support

[Torreno, Alexis Czezar]

> >> +	switch (reg) {
> >> +	case PMBUS_READ_VOUT:
> >> +		ret = pmbus_read_word_data(client, page, phase, reg);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		/* Apply voltage divider scaling if resistors are non-zero */
> >> +		if (data->vout_rfb1 && data->vout_rfb2) {
> >> +			temp = (u64)data->vout_rfb1 + (u64)data->vout_rfb2;
> >> +			temp = DIV_ROUND_CLOSEST_ULL((u64)ret * temp,
> data->vout_rfb2);
> >> +			ret = clamp_val(temp, 0, 0xFFFF);
> >> +		}
> >> +		return ret;
> >> +	default:
> >> +		return -ENODATA;
> >> +	}
> >> +}
> >
> > [Severity: High]
> > Does this code introduce a UAPI inconsistency by only scaling
> PMBUS_READ_VOUT?
> >
> > When in1_input is read, it returns the scaled output voltage.
> >
> > However, because this switch returns -ENODATA for limits (like
> > PMBUS_VOUT_OV_WARN_LIMIT or PMBUS_VOUT_OV_FAULT_LIMIT),
> reading limits
> > like in1_max will return the unscaled feedback voltage directly from the chip.
> >
> 
> I would suggest to add a comment explaining that this series does not support
> any limit registers. That should "address" the issues raised by Sashiko, and also
> help future readers understand why the limit registers are not scaled.
> 

Will try adding this, thanks.
- Alexis

[PATCH v4 3/4] dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support

[Alexis Czezar Torreno]

Add compatible strings for variants of MAX20830 which are MAX20830C
and MAX20840C. These devices have the same register functionality with
MAX20830 but with a longer IC_DEVICE_ID.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
---
 Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
index 01b7275f5b3419cf1b791ed8f9b7ae56cd2961b5..cc50a4ee44a27bd5b14d6b56afe87f807adcb9e2 100644
--- a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
@@ -22,7 +22,13 @@ allOf:
 
 properties:
   compatible:
-    const: adi,max20830
+    oneOf:
+      - const: adi,max20830
+      - items:
+          - enum:
+              - adi,max20830c
+              - adi,max20840c
+          - const: adi,max20830
 
   reg:
     maxItems: 1

-- 
2.34.1

Re: [PATCH v4 3/4] dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The device tree fallback compatible `adi,max20830` for `adi,max20840c` is broken in practice because the `max20830` driver explicitly rejects the MAX20840C hardware ID.
--

--- Patch [3]: [PATCH v4 3/4] dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support ---
commit c98fae7fd69f878845956d0f0458ff6045d86d9d
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support

This commit adds compatible strings for MAX20830C and MAX20840C variants,
using the existing adi,max20830 compatible string as a fallback.

> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> index 01b7275f5b341..cc50a4ee44a27 100644
> --- a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> @@ -22,7 +22,13 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: adi,max20830
> +    oneOf:
> +      - const: adi,max20830
> +      - items:
> +          - enum:
> +              - adi,max20830c
> +              - adi,max20840c
> +          - const: adi,max20830

[Severity: Medium]
Does this fallback compatible actually work for the MAX20840C?

Looking at max20830_probe() in drivers/hwmon/pmbus/max20830.c, the driver
strictly compares the hardware ID to MAX20830:

drivers/hwmon/pmbus/max20830.c:max20830_probe() {
	...
	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
		return dev_err_probe(&client->dev, -ENODEV,
				     "Unsupported device: '%s'\n", buf);
	...
}

Will an unmodified driver successfully probe the adi,max20840c device using
this fallback string, or will it fail the hardware ID check and return -ENODEV?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-dev-max20830c-v4-0-cec034298535@analog.com?part=3

[PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Alexis Czezar Torreno]

Add support for MAX20830C and MAX20840 step-down DC-DC switching
regulator with PMBus interface. MAX20830C is a different packaging
for MAX20830, and MAX20840C supports 40A regulation compared to
MAX20830 that is only 30A.

Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
---
 Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++-----
 drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/Documentation/hwmon/max20830.rst b/Documentation/hwmon/max20830.rst
index 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be40af02265aced59 100644
--- a/Documentation/hwmon/max20830.rst
+++ b/Documentation/hwmon/max20830.rst
@@ -13,6 +13,22 @@ Supported chips:
 
     Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max20830.pdf
 
+  * Analog Devices MAX20830C
+
+    Prefix: 'max20830c'
+
+    Addresses scanned: -
+
+    Datasheet:
+
+  * Analog Devices MAX20840C
+
+    Prefix: 'max20840c'
+
+    Addresses scanned: -
+
+    Datasheet:
+
 Author:
 
   - Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
@@ -21,12 +37,13 @@ Author:
 Description
 -----------
 
-This driver supports hardware monitoring for Analog Devices MAX20830
-Step-Down Switching Regulator with PMBus Interface.
+This driver supports hardware monitoring for Analog Devices MAX20830, MAX20830C
+and MAX20840C. These are Step-Down Switching Regulator with PMBus Interface.
 
-The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC switching
-regulator. Through the PMBus interface, the device can monitor input/output
-voltages, output current and temperature.
+MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated step-down DC-DC
+switching regulators. MAX20840C is similar but can reach 40A. Through the PMBus
+interface, these devices can monitor input/output voltages, output current and
+temperature.
 
 The driver is a client driver to the core PMBus driver. Please see
 Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
index 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841db4a40dd540f1149e 100644
--- a/drivers/hwmon/pmbus/max20830.c
+++ b/drivers/hwmon/pmbus/max20830.c
@@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client *client)
 	 * which do not support SMBus block reads.
 	 */
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
-		/* Reads 9 Data bytes from MAX20830 */
 		ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
 		if (ret < 0)
 			return dev_err_probe(&client->dev, ret,
 					     "Failed to read IC_DEVICE_ID\n");
 	} else {
-		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
+		/* Reads 1 length byte + data bytes */
 		ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
 						    MAX20830_IC_DEVICE_ID_LENGTH + 1,
 						    buf);
@@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client *client)
 		ret = ret - 1;
 	}
 
-	/*
-	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
-	 * Return value should at least be 9 bytes of data.
-	 */
+	/* Verify we read the expected number of bytes */
 	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
 		return dev_err_probe(&client->dev, -ENODEV,
-				     "IC_DEVICE_ID too short: expected at least 9 bytes, got %d\n",
-				     ret);
+				     "IC_DEVICE_ID too short: expected %d bytes, got %d\n",
+				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
+
+	/* Null-terminate the string */
+	buf[ret] = '\0';
 
-	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
-	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
-	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
+	/* Verify the device ID matches what we expect */
+	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
+	    strcmp(buf, "MAX20840C")))
 		return dev_err_probe(&client->dev, -ENODEV,
-				     "Unsupported device: '%s'\n", buf);
+				     "Unsupported device: '%*pE'\n", ret, buf);
 
 	return pmbus_do_probe(client, &data->info);
 }
 
 static const struct i2c_device_id max20830_id[] = {
-	{"max20830"},
+	{ "max20830" },
+	{ "max20830c" },
+	{ "max20840c" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max20830_id);

-- 
2.34.1

Re: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing OF match table entries for new devices breaks module auto-loading.
- [Low] The newly added entries in the i2c_device_id array use positional initializers instead of named initializers, violating I2C subsystem guidelines.
--

--- Patch [4]: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c ---
commit 4a124678c88f942f9f2f9403e79e6632f0000f9f
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830): add support for max20830c and max20840c
    
Add support for MAX20830C and MAX20840 step-down DC-DC switching
regulator with PMBus interface. MAX20830C is a different packaging
for MAX20830, and MAX20840C supports 40A regulation compared to
MAX20830 that is only 30A.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index 7c1e94c43ac4e..029733a377123 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client *client)
>  		ret = ret - 1;
>  	}
>  
> -	/*
> -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
> -	 * Return value should at least be 9 bytes of data.
> -	 */
> +	/* Verify we read the expected number of bytes */
>  	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
>  		return dev_err_probe(&client->dev, -ENODEV,
> -				     "IC_DEVICE_ID too short: expected at least 9 bytes, got %d\n",
> -				     ret);
> +				     "IC_DEVICE_ID too short: expected %d bytes, got %d\n",
> +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
> +
> +	/* Null-terminate the string */
> +	buf[ret] = '\0';
>  
> -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
> -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
> -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
> +	/* Verify the device ID matches what we expect */
> +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
> +	    strcmp(buf, "MAX20840C")))
>  		return dev_err_probe(&client->dev, -ENODEV,
> -				     "Unsupported device: '%s'\n", buf);
> +				     "Unsupported device: '%*pE'\n", ret, buf);
>  
>  	return pmbus_do_probe(client, &data->info);
>  }
>  
>  static const struct i2c_device_id max20830_id[] = {
> -	{"max20830"},
> +	{ "max20830" },
> +	{ "max20830c" },
> +	{ "max20840c" },

[Severity: Low]
This isn't a bug, but does the initialization of this array violate the I2C
subsystem rules?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must be declared const and use named initializers
instead of positional ones:

	{ .name = "max20830" },
	{ .name = "max20830c" },
	{ .name = "max20840c" },

[Severity: Medium]
Will this driver fail to auto-load via udev for the new devices when
instantiated via Device Tree?

It seems the new compatibles (e.g., "adi,max20830c" and "adi,max20840c")
were not added to max20830_of_match[] lower down in this file:

static const struct of_device_id max20830_of_match[] = {
	{ .compatible = "adi,max20830" },
	{ }
};

Without updating this array to include the new devices, the kernel won't
emit the correct MODALIAS for the new chips, which breaks module
auto-loading.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, max20830_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-dev-max20830c-v4-0-cec034298535@analog.com?part=4

Re: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Guenter Roeck]

On 7/27/26 20:32, Alexis Czezar Torreno wrote:
> Add support for MAX20830C and MAX20840 step-down DC-DC switching
> regulator with PMBus interface. MAX20830C is a different packaging
> for MAX20830, and MAX20840C supports 40A regulation compared to
> MAX20830 that is only 30A.
> 

Are those chips still not published ? I find MAX20840T, but no "C" variants.
And MAX20840T presumably has an I2C device ID of "MAX20840", not "MAX20840C".

I also noticed that MAX20810 and MAX20815 seem to be register compatible.

> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> ---
>   Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++-----
>   drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
>   2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/hwmon/max20830.rst b/Documentation/hwmon/max20830.rst
> index 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be40af02265aced59 100644
> --- a/Documentation/hwmon/max20830.rst
> +++ b/Documentation/hwmon/max20830.rst
> @@ -13,6 +13,22 @@ Supported chips:
>   
>       Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/max20830.pdf
>   
> +  * Analog Devices MAX20830C
> +
> +    Prefix: 'max20830c'
> +
> +    Addresses scanned: -
> +
> +    Datasheet:
> +
> +  * Analog Devices MAX20840C
> +
> +    Prefix: 'max20840c'
> +
> +    Addresses scanned: -
> +
> +    Datasheet:
> +
>   Author:
>   
>     - Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> @@ -21,12 +37,13 @@ Author:
>   Description
>   -----------
>   
> -This driver supports hardware monitoring for Analog Devices MAX20830
> -Step-Down Switching Regulator with PMBus Interface.
> +This driver supports hardware monitoring for Analog Devices MAX20830, MAX20830C
> +and MAX20840C. These are Step-Down Switching Regulator with PMBus Interface.
>   
> -The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC switching
> -regulator. Through the PMBus interface, the device can monitor input/output
> -voltages, output current and temperature.
> +MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated step-down DC-DC
> +switching regulators. MAX20840C is similar but can reach 40A. Through the PMBus
> +interface, these devices can monitor input/output voltages, output current and
> +temperature.
>   
>   The driver is a client driver to the core PMBus driver. Please see
>   Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841db4a40dd540f1149e 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c
> @@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client *client)
>   	 * which do not support SMBus block reads.
>   	 */
>   	if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> -		/* Reads 9 Data bytes from MAX20830 */
>   		ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
>   		if (ret < 0)
>   			return dev_err_probe(&client->dev, ret,
>   					     "Failed to read IC_DEVICE_ID\n");
>   	} else {
> -		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
> +		/* Reads 1 length byte + data bytes */
>   		ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
>   						    MAX20830_IC_DEVICE_ID_LENGTH + 1,
>   						    buf);
> @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client *client)
>   		ret = ret - 1;
>   	}
>   
> -	/*
> -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
> -	 * Return value should at least be 9 bytes of data.
> -	 */
> +	/* Verify we read the expected number of bytes */
>   	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
>   		return dev_err_probe(&client->dev, -ENODEV,
> -				     "IC_DEVICE_ID too short: expected at least 9 bytes, got %d\n",
> -				     ret);
> +				     "IC_DEVICE_ID too short: expected %d bytes, got %d\n",
> +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
> +
> +	/* Null-terminate the string */
> +	buf[ret] = '\0';
>   
> -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
> -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
> -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
> +	/* Verify the device ID matches what we expect */
> +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
> +	    strcmp(buf, "MAX20840C")))
>   		return dev_err_probe(&client->dev, -ENODEV,
> -				     "Unsupported device: '%s'\n", buf);
> +				     "Unsupported device: '%*pE'\n", ret, buf);
>   
>   	return pmbus_do_probe(client, &data->info);
>   }
>   
>   static const struct i2c_device_id max20830_id[] = {
> -	{"max20830"},
> +	{ "max20830" },
> +	{ "max20830c" },
> +	{ "max20840c" },

I don't think that is needed or warranted, given that there is no matching
devicetree node.

Thanks,
Guenter

>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, max20830_id);
> 

RE: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Torreno, Alexis Czezar]

> 
> [External]
> 
> On 7/27/26 20:32, Alexis Czezar Torreno wrote:
> > Add support for MAX20830C and MAX20840 step-down DC-DC switching
> > regulator with PMBus interface. MAX20830C is a different packaging for
> > MAX20830, and MAX20840C supports 40A regulation compared to
> > MAX20830 that is only 30A.
> >
> 
> Are those chips still not published ? I find MAX20840T, but no "C" variants.
> And MAX20840T presumably has an I2C device ID of "MAX20840", not
> "MAX20840C".
> 
> I also noticed that MAX20810 and MAX20815 seem to be register compatible.
> 

I believe so yes, they aren't yet.

Actually MAX20810/815 and a few more are next in line after this. A different
person is handling it, but they're waiting on how this patches go.

> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> > ---
> >   Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++-----
> >   drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
> >   2 files changed, 36 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max20830.rst
> > b/Documentation/hwmon/max20830.rst
> > index
> >
> 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be
> 40a
> > f02265aced59 100644
> > --- a/Documentation/hwmon/max20830.rst
> > +++ b/Documentation/hwmon/max20830.rst
> > @@ -13,6 +13,22 @@ Supported chips:
> >
> >       Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ma
> > x20830.pdf
> >
> > +  * Analog Devices MAX20830C
> > +
> > +    Prefix: 'max20830c'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > +
> > +  * Analog Devices MAX20840C
> > +
> > +    Prefix: 'max20840c'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > +
> >   Author:
> >
> >     - Alexis Czezar Torreno <alexisczezar.torreno@analog.com> @@
> > -21,12 +37,13 @@ Author:
> >   Description
> >   -----------
> >
> > -This driver supports hardware monitoring for Analog Devices MAX20830
> > -Step-Down Switching Regulator with PMBus Interface.
> > +This driver supports hardware monitoring for Analog Devices MAX20830,
> > +MAX20830C and MAX20840C. These are Step-Down Switching Regulator
> with PMBus Interface.
> >
> > -The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC
> > switching -regulator. Through the PMBus interface, the device can
> > monitor input/output -voltages, output current and temperature.
> > +MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated
> > +step-down DC-DC switching regulators. MAX20840C is similar but can
> > +reach 40A. Through the PMBus interface, these devices can monitor
> > +input/output voltages, output current and temperature.
> >
> >   The driver is a client driver to the core PMBus driver. Please see
> >   Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> > diff --git a/drivers/hwmon/pmbus/max20830.c
> > b/drivers/hwmon/pmbus/max20830.c index
> >
> 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841d
> b4a4
> > 0dd540f1149e 100644
> > --- a/drivers/hwmon/pmbus/max20830.c
> > +++ b/drivers/hwmon/pmbus/max20830.c
> > @@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client *client)
> >   	 * which do not support SMBus block reads.
> >   	 */
> >   	if (i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> > -		/* Reads 9 Data bytes from MAX20830 */
> >   		ret = i2c_smbus_read_block_data(client,
> PMBUS_IC_DEVICE_ID, buf);
> >   		if (ret < 0)
> >   			return dev_err_probe(&client->dev, ret,
> >   					     "Failed to read IC_DEVICE_ID\n");
> >   	} else {
> > -		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
> > +		/* Reads 1 length byte + data bytes */
> >   		ret = i2c_smbus_read_i2c_block_data(client,
> PMBUS_IC_DEVICE_ID,
> >
> MAX20830_IC_DEVICE_ID_LENGTH + 1,
> >   						    buf);
> > @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client
> *client)
> >   		ret = ret - 1;
> >   	}
> >
> > -	/*
> > -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
> > -	 * Return value should at least be 9 bytes of data.
> > -	 */
> > +	/* Verify we read the expected number of bytes */
> >   	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
> >   		return dev_err_probe(&client->dev, -ENODEV,
> > -				     "IC_DEVICE_ID too short: expected at least
> 9 bytes, got %d\n",
> > -				     ret);
> > +				     "IC_DEVICE_ID too short: expected %d
> bytes, got %d\n",
> > +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
> > +
> > +	/* Null-terminate the string */
> > +	buf[ret] = '\0';
> >
> > -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
> > -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
> > -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
> > +	/* Verify the device ID matches what we expect */
> > +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
> > +	    strcmp(buf, "MAX20840C")))
> >   		return dev_err_probe(&client->dev, -ENODEV,
> > -				     "Unsupported device: '%s'\n", buf);
> > +				     "Unsupported device: '%*pE'\n", ret, buf);
> >
> >   	return pmbus_do_probe(client, &data->info);
> >   }
> >
> >   static const struct i2c_device_id max20830_id[] = {
> > -	{"max20830"},
> > +	{ "max20830" },
> > +	{ "max20830c" },
> > +	{ "max20840c" },
> 
> I don't think that is needed or warranted, given that there is no matching
> devicetree node.
> 

I see, will remove.

Re: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Guenter Roeck]

On 7/27/26 22:05, Torreno, Alexis Czezar wrote:
>>
>> [External]
>>
>> On 7/27/26 20:32, Alexis Czezar Torreno wrote:
>>> Add support for MAX20830C and MAX20840 step-down DC-DC switching
>>> regulator with PMBus interface. MAX20830C is a different packaging for
>>> MAX20830, and MAX20840C supports 40A regulation compared to
>>> MAX20830 that is only 30A.
>>>
>>
>> Are those chips still not published ? I find MAX20840T, but no "C" variants.
>> And MAX20840T presumably has an I2C device ID of "MAX20840", not
>> "MAX20840C".
>>
>> I also noticed that MAX20810 and MAX20815 seem to be register compatible.
>>
> 
> I believe so yes, they aren't yet.
> 

I just hope they are really compatible, and that the device ID strings really
include the "C". The "T" variants seem to have no T in the device ID string,
making it a bit odd that it was (or will be) added for the C variants.

> Actually MAX20810/815 and a few more are next in line after this. A different
> person is handling it, but they're waiting on how this patches go.
> 

You are making yourself more work than necessary. Knowing that there are more
chips coming, the sequence of strcmp() is not really that desirable anymore.
It might make sense to create an array with all chips supported by the driver
instead of adding up strcmp sequences. That would make it much easier to add
support for new variants.

There also seems to be a MAX20830T. Does it actually make sense to list the
variants (C/T) in the first place ?

>>> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>>> ---
>>>    Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++-----
>>>    drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
>>>    2 files changed, 36 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/hwmon/max20830.rst
>>> b/Documentation/hwmon/max20830.rst
>>> index
>>>
>> 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be
>> 40a
>>> f02265aced59 100644
>>> --- a/Documentation/hwmon/max20830.rst
>>> +++ b/Documentation/hwmon/max20830.rst
>>> @@ -13,6 +13,22 @@ Supported chips:
>>>
>>>        Datasheet:
>>> https://www.analog.com/media/en/technical-documentation/data-sheets/ma
>>> x20830.pdf
>>>
>>> +  * Analog Devices MAX20830C
>>> +
>>> +    Prefix: 'max20830c'
>>> +
>>> +    Addresses scanned: -
>>> +
>>> +    Datasheet:
>>> +
>>> +  * Analog Devices MAX20840C
>>> +
>>> +    Prefix: 'max20840c'
>>> +
>>> +    Addresses scanned: -
>>> +
>>> +    Datasheet:
>>> +
>>>    Author:
>>>
>>>      - Alexis Czezar Torreno <alexisczezar.torreno@analog.com> @@
>>> -21,12 +37,13 @@ Author:
>>>    Description
>>>    -----------
>>>
>>> -This driver supports hardware monitoring for Analog Devices MAX20830
>>> -Step-Down Switching Regulator with PMBus Interface.
>>> +This driver supports hardware monitoring for Analog Devices MAX20830,
>>> +MAX20830C and MAX20840C. These are Step-Down Switching Regulator
>> with PMBus Interface.
>>>
>>> -The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC
>>> switching -regulator. Through the PMBus interface, the device can
>>> monitor input/output -voltages, output current and temperature.
>>> +MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated
>>> +step-down DC-DC switching regulators. MAX20840C is similar but can
>>> +reach 40A. Through the PMBus interface, these devices can monitor
>>> +input/output voltages, output current and temperature.
>>>
>>>    The driver is a client driver to the core PMBus driver. Please see
>>>    Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
>>> diff --git a/drivers/hwmon/pmbus/max20830.c
>>> b/drivers/hwmon/pmbus/max20830.c index
>>>
>> 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841d
>> b4a4
>>> 0dd540f1149e 100644
>>> --- a/drivers/hwmon/pmbus/max20830.c
>>> +++ b/drivers/hwmon/pmbus/max20830.c
>>> @@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client *client)
>>>    	 * which do not support SMBus block reads.
>>>    	 */
>>>    	if (i2c_check_functionality(client->adapter,
>> I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
>>> -		/* Reads 9 Data bytes from MAX20830 */
>>>    		ret = i2c_smbus_read_block_data(client,
>> PMBUS_IC_DEVICE_ID, buf);
>>>    		if (ret < 0)
>>>    			return dev_err_probe(&client->dev, ret,
>>>    					     "Failed to read IC_DEVICE_ID\n");
>>>    	} else {
>>> -		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
>>> +		/* Reads 1 length byte + data bytes */
>>>    		ret = i2c_smbus_read_i2c_block_data(client,
>> PMBUS_IC_DEVICE_ID,
>>>
>> MAX20830_IC_DEVICE_ID_LENGTH + 1,
>>>    						    buf);
>>> @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client
>> *client)
>>>    		ret = ret - 1;
>>>    	}
>>>
>>> -	/*
>>> -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
>>> -	 * Return value should at least be 9 bytes of data.
>>> -	 */
>>> +	/* Verify we read the expected number of bytes */
>>>    	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
>>>    		return dev_err_probe(&client->dev, -ENODEV,
>>> -				     "IC_DEVICE_ID too short: expected at least
>> 9 bytes, got %d\n",
>>> -				     ret);
>>> +				     "IC_DEVICE_ID too short: expected %d
>> bytes, got %d\n",
>>> +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
>>> +
>>> +	/* Null-terminate the string */
>>> +	buf[ret] = '\0';
>>>
>>> -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
>>> -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
>>> -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
>>> +	/* Verify the device ID matches what we expect */
>>> +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
>>> +	    strcmp(buf, "MAX20840C")))
>>>    		return dev_err_probe(&client->dev, -ENODEV,
>>> -				     "Unsupported device: '%s'\n", buf);
>>> +				     "Unsupported device: '%*pE'\n", ret, buf);
>>>
>>>    	return pmbus_do_probe(client, &data->info);
>>>    }
>>>
>>>    static const struct i2c_device_id max20830_id[] = {
>>> -	{"max20830"},
>>> +	{ "max20830" },
>>> +	{ "max20830c" },
>>> +	{ "max20840c" },
>>
>> I don't think that is needed or warranted, given that there is no matching
>> devicetree node.
>>
> 
> I see, will remove.

Same for the prefix string in the documentation. You might want to mention the
supported chips, though, in both the configuration and Kconfig. In Kconfig,
that could be a generic such as "MAX20830 and compatible chips", or list the
base variants, such as "MAX20830, MAX20840 and variants".

Thanks,
Guenter

RE: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Torreno, Alexis Czezar]

> 
> >>
> >> Are those chips still not published ? I find MAX20840T, but no "C" variants.
> >> And MAX20840T presumably has an I2C device ID of "MAX20840", not
> >> "MAX20840C".
> >>
> >> I also noticed that MAX20810 and MAX20815 seem to be register
> compatible.
> >>
> >
> > I believe so yes, they aren't yet.
> >
> 
> I just hope they are really compatible, and that the device ID strings really
> include the "C". The "T" variants seem to have no T in the device ID string,
> making it a bit odd that it was (or will be) added for the C variants.
> 

Yeah the T is weird, but I did test the C variants and they do reply the 'c' char.

> > Actually MAX20810/815 and a few more are next in line after this. A
> > different person is handling it, but they're waiting on how this patches go.
> >
> 
> You are making yourself more work than necessary. Knowing that there are
> more chips coming, the sequence of strcmp() is not really that desirable
> anymore.
> It might make sense to create an array with all chips supported by the driver
> instead of adding up strcmp sequences. That would make it much easier to add
> support for new variants.
> 
> There also seems to be a MAX20830T. Does it actually make sense to list the
> variants (C/T) in the first place ?
> 

Will think about how to make to scale the code when adding newer variants.

I may not know enough how important variants are when supporting them, the
C variant seems to be a package difference and the ID string.

> >>> Signed-off-by: Alexis Czezar Torreno
> >>> <alexisczezar.torreno@analog.com>
> >>> ---
> >>>    Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++----
> -
> >>>    drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
> >>>    2 files changed, 36 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/Documentation/hwmon/max20830.rst
> >>> b/Documentation/hwmon/max20830.rst
> >>> index
> >>>
> >>
> 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be
> >> 40a
> >>> f02265aced59 100644
> >>> --- a/Documentation/hwmon/max20830.rst
> >>> +++ b/Documentation/hwmon/max20830.rst
> >>> @@ -13,6 +13,22 @@ Supported chips:
> >>>
> >>>        Datasheet:
> >>> https://www.analog.com/media/en/technical-documentation/data-sheets/
> >>> ma
> >>> x20830.pdf
> >>>
> >>> +  * Analog Devices MAX20830C
> >>> +
> >>> +    Prefix: 'max20830c'
> >>> +
> >>> +    Addresses scanned: -
> >>> +
> >>> +    Datasheet:
> >>> +
> >>> +  * Analog Devices MAX20840C
> >>> +
> >>> +    Prefix: 'max20840c'
> >>> +
> >>> +    Addresses scanned: -
> >>> +
> >>> +    Datasheet:
> >>> +
> >>>    Author:
> >>>
> >>>      - Alexis Czezar Torreno <alexisczezar.torreno@analog.com> @@
> >>> -21,12 +37,13 @@ Author:
> >>>    Description
> >>>    -----------
> >>>
> >>> -This driver supports hardware monitoring for Analog Devices
> >>> MAX20830 -Step-Down Switching Regulator with PMBus Interface.
> >>> +This driver supports hardware monitoring for Analog Devices
> >>> +MAX20830, MAX20830C and MAX20840C. These are Step-Down Switching
> >>> +Regulator
> >> with PMBus Interface.
> >>>
> >>> -The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC
> >>> switching -regulator. Through the PMBus interface, the device can
> >>> monitor input/output -voltages, output current and temperature.
> >>> +MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated
> >>> +step-down DC-DC switching regulators. MAX20840C is similar but can
> >>> +reach 40A. Through the PMBus interface, these devices can monitor
> >>> +input/output voltages, output current and temperature.
> >>>
> >>>    The driver is a client driver to the core PMBus driver. Please see
> >>>    Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> >>> diff --git a/drivers/hwmon/pmbus/max20830.c
> >>> b/drivers/hwmon/pmbus/max20830.c index
> >>>
> >>
> 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841d
> >> b4a4
> >>> 0dd540f1149e 100644
> >>> --- a/drivers/hwmon/pmbus/max20830.c
> >>> +++ b/drivers/hwmon/pmbus/max20830.c
> >>> @@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client
> *client)
> >>>    	 * which do not support SMBus block reads.
> >>>    	 */
> >>>    	if (i2c_check_functionality(client->adapter,
> >> I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> >>> -		/* Reads 9 Data bytes from MAX20830 */
> >>>    		ret = i2c_smbus_read_block_data(client,
> >> PMBUS_IC_DEVICE_ID, buf);
> >>>    		if (ret < 0)
> >>>    			return dev_err_probe(&client->dev, ret,
> >>>    					     "Failed to read IC_DEVICE_ID\n");
> >>>    	} else {
> >>> -		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
> >>> +		/* Reads 1 length byte + data bytes */
> >>>    		ret = i2c_smbus_read_i2c_block_data(client,
> >> PMBUS_IC_DEVICE_ID,
> >>>
> >> MAX20830_IC_DEVICE_ID_LENGTH + 1,
> >>>    						    buf);
> >>> @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client
> >> *client)
> >>>    		ret = ret - 1;
> >>>    	}
> >>>
> >>> -	/*
> >>> -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
> >>> -	 * Return value should at least be 9 bytes of data.
> >>> -	 */
> >>> +	/* Verify we read the expected number of bytes */
> >>>    	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
> >>>    		return dev_err_probe(&client->dev, -ENODEV,
> >>> -				     "IC_DEVICE_ID too short: expected at least
> >> 9 bytes, got %d\n",
> >>> -				     ret);
> >>> +				     "IC_DEVICE_ID too short: expected %d
> >> bytes, got %d\n",
> >>> +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
> >>> +
> >>> +	/* Null-terminate the string */
> >>> +	buf[ret] = '\0';
> >>>
> >>> -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
> >>> -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
> >>> -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
> >>> +	/* Verify the device ID matches what we expect */
> >>> +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
> >>> +	    strcmp(buf, "MAX20840C")))
> >>>    		return dev_err_probe(&client->dev, -ENODEV,
> >>> -				     "Unsupported device: '%s'\n", buf);
> >>> +				     "Unsupported device: '%*pE'\n", ret, buf);
> >>>
> >>>    	return pmbus_do_probe(client, &data->info);
> >>>    }
> >>>
> >>>    static const struct i2c_device_id max20830_id[] = {
> >>> -	{"max20830"},
> >>> +	{ "max20830" },
> >>> +	{ "max20830c" },
> >>> +	{ "max20840c" },
> >>
> >> I don't think that is needed or warranted, given that there is no
> >> matching devicetree node.
> >>
> >
> > I see, will remove.
> 
> Same for the prefix string in the documentation. You might want to mention the
> supported chips, though, in both the configuration and Kconfig. In Kconfig, that
> could be a generic such as "MAX20830 and compatible chips", or list the base
> variants, such as "MAX20830, MAX20840 and variants".
> 

Ok will double check this.