Staging/max20826 support

[nunojsa]

PR Description

Just opening for the llm review. If someone else wants to add some reviewing, also welcome to do so.

PR Type

• Bug fix (a change that fixes an issue)
• New feature (a change that adds new functionality)
• Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

• I have conducted a self-review of my own code changes
• I have compiled my changes, including the documentation
• I have tested the changes on the relevant hardware
• I have updated the documentation outside this repo accordingly
• I have provided links for the relevant upstream lore

[github-actions]

LLM review

This series adds PMBus hwmon driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916 multiphase regulators, along with three PMBus core patches (regulator enable fix, phase count increase, read_block_data hook).

run: 29251533245

0539a3fd667d - hwmon: (pmbus) add support for MAX20826 and similar devices

Short-read / clang analyzer warning (max20826.c:791):

max20826_read_block_data_unsafe() does not verify that i2c_smbus_read_i2c_block_data() returned enough bytes before doing memcpy. When it returns fewer bytes than size + 1, block_data[2] (and beyond) are uninitialized stack data. This is what causes the CI clang_analyzer warning: vout_res[1] is a garbage value at line 791.

// Missing check after ret < 0:
if (ret < size + 1)
    return -EIO;

GPIO NULL handling bug in max20826_regulator_enabled():

The comment "If the gpios are not given, just assume it's on" and int on = 1 are immediately overridden by gpiod_get_value_cansleep(), which returns 0 when passed NULL (optional GPIO not provided). This reports the regulator as off when no GPIO is wired. Fix: check if (gpio) before calling gpiod_get_value_cansleep().

Missing pmbus_lock in max20826_high_speed_en_store():

The sysfs store performs an I2C read-modify-write via max20826_update_byte_data_unsafe() without holding pmbus_lock. The reference pattern in isl68137.c uses pmbus_lock_interruptible() for this purpose. This is a race with concurrent pmbus_core operations.

Debug comment left in submitted code (max20826.c:272):

/* Need to evaluate of the avsbus bool is really needed!!!! */

Should be removed before submission.

Kconfig issues

SENSORS_MAX20826_REGULATOR is listed before SENSORS_MAX20826 in Kconfig. Sub-options should follow their parent. Additionally the help text says "Maxim MAX20826" — should be "Analog Devices MAX20826".

CI warnings

The clang_analyzer warning for max20826.c:791 (UndefinedBinaryOperatorResult) is confirmed as a real bug caused by missing short-read detection. The kernel_smatch failures are all in core kernel files unrelated to this series.

Verification data

• Compiled max20826.o and pmbus_core.o with set_arch llvm_x86 + auto_set_kconfig — clean build
• checkpatch.pl run on all 5 patches — no warnings
• DT binding validated with make dt_binding_check (after installing dtschema)
• gpiod_get_value_cansleep(NULL) return value confirmed at drivers/gpio/gpiolib.c:385-401
• Locking pattern cross-referenced with drivers/hwmon/pmbus/isl68137.c
• Datasheet for MAX20826 is NDA-protected (confirmed in driver documentation); MAX20855A abridged datasheet obtained from doctools mirror but contains no register maps

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29251533245

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[github-actions]

LLM review

This series adds PMBus hwmon/regulator driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916 multiphase regulators, plus three pmbus_core fixes (regulator enable return value, phase count to 16, read_block_data hook).

run: 29264461327

fe3685aa2185 - hwmon: (pmbus) add support for MAX20826 and similar devices

Probe-time regulator enable bug in max20826_detect_phases() (max20826.c:674):

oper_save is initialized to 0, but 0 is also a valid OPERATION register value (regulator fully off, all bits clear). When OPERATION reads as 0x00, the code saves oper_save = 0, enables the regulator for phase detection, then checks if (oper_save) — which evaluates false — and skips the restore write. The output stays enabled after probe.

/* Before: 0 is a valid OPERATION value, not a safe sentinel */
int ret, oper_save = 0;
...
if (oper_save)  /* false when OPERATION was 0x00 — restore skipped! */
    i2c_smbus_write_byte_data(rail, PMBUS_OPERATION, oper_save);

/* After: use -1 as sentinel */
int ret, oper_save = -1;
...
if (oper_save >= 0)
    i2c_smbus_write_byte_data(rail, PMBUS_OPERATION, oper_save);

Kconfig help text drivers/hwmon/pmbus/Kconfig:399:

The SENSORS_MAX20826 entry title says "Analog Devices" but the help paragraph says "Maxim MAX20826". Also missing period at the end of the module name line.

Observations (not blocking)

• max20826_read_block_data() returns -EOPNOTSUPP for unhandled block registers (intentional — prevents the core's pmbus_read_block_data() fallthrough which would use pmbus_set_page() + generic I2C, breaking direct-address mode).
• Previous review's concern about gpiod_get_value_cansleep(NULL): the current code already guards with if (gpio) before calling gpiod_get_value_cansleep(). Not a bug in this version.
• Previous review's concern about clang_analyzer vout_res[1]: the if (ret < size + 1) return -EIO short-read check is present at max20826.c:201. Not a bug in this version.
• pmbus_update_byte_data() returning positive on success is correctly handled by the core patch 3218bbe52233.

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29264461327

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[github-actions]

LLM review

This series adds PMBus hwmon driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916
multiphase regulators, plus three pmbus_core fixes (regulator enable return value, phase count
to 16, read_block_data hook). Most issues from previous reviews are resolved.

run: 29316844925

cae742bd7875 - hwmon: (pmbus) add support for MAX20826 and similar devices

Bug: device_get_match_data() instead of i2c_get_match_data() (max20826.c:936):

/* Current — fails when no Device Tree node is present */
st->chip_info = device_get_match_data(&client->dev);

/* Fix */
st->chip_info = i2c_get_match_data(client);

device_get_match_data() only resolves match data from OF/ACPI firmware nodes.
When the device is instantiated via I2C board info (no DT node), dev_fwnode()
returns NULL, device_get_match_data() returns NULL, and probe fails with -EINVAL.
i2c_get_match_data() falls back to i2c_device_id.driver_data for non-DT
instantiation. Every other pmbus driver with both of_match_table and
i2c_device_id uses i2c_get_match_data() (adp1050.c:84, pmbus.c:175,
mp2856.c:407). Confirmed in drivers/i2c/i2c-core-base.c:121-137.

Minor: double space in assignment (max20826.c:708):

-		info->pfunc[phase] =  PMBUS_HAVE_IOUT | PMBUS_HAVE_IIN;
+		info->pfunc[phase] = PMBUS_HAVE_IOUT | PMBUS_HAVE_IIN;

Observations (non-blocking)

• oper_save = -1 sentinel fix: present and correct.
• Short-read check (ret < size + 1): present at line 201.
• if (gpio) guard in regulator_enabled: present at line 503.
• guard(pmbus_lock) in high_speed_en_store: present at line 282.
• gpiod_set_value_cansleep() return value is not checked in
  max20826_regulator_enable() — minor; common pattern, not blocking.
• expected_phases is u8 but count_phases() returns int; negative
  values would wrap, but the n_phases != expected check in detect_phases()
  prevents any mismatch from passing through.
• Commit message has a double space: "similar provide".

CI warnings

checkpatch: clean (0 errors, 0 warnings) on all five commits.
dt_binding_check: clean.
Build (x86/llvm-19, arm/gcc-13): clean.

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29316844925

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[github-actions]

LLM review

This series adds PMBus hwmon driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916
multiphase regulators, along with three pmbus_core patches (regulator enable fix, phase count
increase to 16, read_block_data hook). All issues raised in the previous two reviews are resolved.

run: 29320436295

0e86a1fef019 - hwmon: (pmbus) add support for MAX20826 and similar devices

Bug: max20826_detect_phases() leaves OPERATION register in modified state on error (max20826.c:695-704):

When max20826_detect_phases() enables the regulator (sets PB_OPERATION_CONTROL_ON) to read
the phase count, two error paths return without restoring the saved register value:

1. max20826_read_block_data_unsafe() fails (line 695-698)
2. Phase count mismatch (line 700-704)

In both cases oper_save >= 0 but PMBUS_OPERATION is left with PB_OPERATION_CONTROL_ON set,
inadvertently enabling a rail that was previously off.

Minor: double space in max20826_default_info (max20826.c:591):

-	.read_byte_data =  max20826_read_byte_data,
+	.read_byte_data = max20826_read_byte_data,

Minor: Kconfig help text (drivers/hwmon/pmbus/Kconfig:400,403):

• "Analog devices" should be "Analog Devices" (lowercase 'd')
• Missing period after module name "max20826"

7f51d7ce8182 - hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

Missing API contract documentation (pmbus.h:462):

The new read_block_data hook is placed after the comment block that describes the -ENODATA
fallthrough convention, but its own comment only says "/* size of data_buf is I2C_SMBUS_BLOCK_MAX + 2 */".
The contract (return -ENODATA to fall through to the generic implementation; any other
negative value prevents fallthrough) is not stated. max20826_read_block_data() intentionally
returns -EOPNOTSUPP for unsupported registers to prevent pmbus_set_page() from being called
in direct-address mode — this is a valid use of the non-fallthrough path, but it needs to be
documented in the hook's comment.

Observations (non-blocking)

• i2c_get_match_data() is correctly used (not device_get_match_data()); confirmed in drivers/i2c/i2c-core-base.c:121-137.
• oper_save = -1 sentinel is present and correct.
• if (gpio) guard in max20826_regulator_enabled() is present at line 502.
• guard(pmbus_lock) in max20826_high_speed_en_store() is present at line 282.
• gpiod_set_value_cansleep(NULL, ...) is safe per drivers/gpio/gpiolib.c:385-401 (validate_desc(NULL) returns 0 and the VALIDATE_DESC_VOID macro returns silently).
• pfunc[] in max20826_detect_phases() is indexed 0-based for both RAIL_A and RAIL_B. The RAIL_B call overwrites RAIL_A entries in pfunc[]. Since both rails assign the same mask (PMBUS_HAVE_IOUT | PMBUS_HAVE_IIN), this is functionally harmless but architecturally inconsistent with how mp2975.c handles multi-rail phase indexing (which uses non-overlapping indices). Not blocking for this series.

CI notes

Target upstream mirror: mirror_ci/groeck/linux-staging/hwmon-next, strict mode enabled.

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29320436295

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[github-actions]

LLM review

This series adds PMBus hwmon driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916
multiphase regulators, along with three pmbus_core patches (regulator enable fix, phase count
increase to 16, read_block_data hook). Most issues from the four previous review iterations are
resolved.

run: 29338694846

affb4325c1d4 - hwmon: (pmbus) add support for MAX20826 and similar devices

Bug: ret2 used uninitialized in max20826_detect_phases() (max20826.c:717)

ret2 is declared but only assigned inside if (oper_save >= 0). When the regulator is
already ON at probe time (PB_OPERATION_CONTROL_ON already set), oper_save stays -1
(the sentinel value, never entered the save branch), so ret2 is never written. On the
success path (ret >= 0) the return statement return ret < 0 ? ret : ret2 yields
uninitialized data — undefined behaviour.

/* Before */
int ret, ret2, oper_save = -1;
...
out_restore_oper:
    if (oper_save >= 0)
        ret2 = i2c_smbus_write_byte_data(...);  /* ret2 only set here */

    return ret < 0 ? ret : ret2;   /* ret2 uninitialized when oper_save < 0 */

/* After */
int ret, ret2 = 0, oper_save = -1;

GCC 13 -Wmaybe-uninitialized confirms this:

drivers/hwmon/pmbus/max20826.c:717:30: warning: 'ret2' may be used uninitialized

GCC false-positive: val_buf[2] in max20826_detect_addr_mode() (max20826.c:653)

val_buf[3] is uninitialized on declaration. val_buf[2] is only read when
is_reg_addr_mode_block is true, the same condition that caused the block read to populate
the array. GCC cannot track this correlation across two separate if branches and reports
-Wmaybe-uninitialized. Fix: u8 val_buf[3] = {}.

7d949941369f - hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

Missing API contract documentation (pmbus.h:462-464)

The existing read_byte_data and read_word_data hooks are covered by the comment block
describing -ENODATA as the "delegate to generic" sentinel. The new read_block_data hook
only carries /* size of data_buf is I2C_SMBUS_BLOCK_MAX + 2 */. The -ENODATA delegation
contract used by _pmbus_read_block_data() is not stated, even though max20826_read_block_data()
deliberately returns -EOPNOTSUPP (not -ENODATA) for unhandled registers to prevent
pmbus_set_page() from being called in direct-address mode.

CI warnings

Compilation with set_arch gcc_arm (GCC 13) and KCFLAGS="-Wmaybe-uninitialized" reproduces
both warnings. After applying the fixup patches below, the build is clean.

Verification data

• Compiled max20826.o with set_arch gcc_arm + auto_set_kconfig — warnings reproduced
• Applied ret2 = 0 and val_buf[3] = {} fixes, recompiled — clean build, zero warnings
• checkpatch.pl --strict on all five original commits — clean
• checkpatch.pl --strict on both fixup patches — clean

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29338694846

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[sipraga]

as a total stranger to pmbus, I wanted to find where this is defined. Now that I found it, feel free to use the citation in your commit message if you want:

12.1.3. OPERATION Command Bits [5:4]
https://pmbus.org/wp-content/uploads/2022/01/PMBus-Specification-Rev-1-3-1-Part-II-20150313.pdf

[github-actions]

LLM review

This series adds PMBus hwmon/regulator driver support for MAX20826/MAX20855B/MAX20908/MAX20912/MAX20916
multiphase regulators, along with three pmbus_core patches (regulator enable return-value fix, phase
count increase to 16, read_block_data hook). Most issues from previous review iterations are resolved.

run: 29344289845

837b8ebee3cfe - hwmon: (pmbus) add support for MAX20826 and similar devices

GCC -Wmaybe-uninitialized: val_buf[2] (max20826.c:653)

val_buf[3] is declared without initialization in max20826_detect_addr_mode(). GCC 13 cannot
track that max20826_read_block_data_unsafe() always populates the buffer on success and warns:

max20826.c:653:21: warning: 'val_buf[2]' may be used uninitialized [-Wmaybe-uninitialized]
max20826.c:611:12: note: 'val_buf[2]' was declared here

Reproduced with set_arch gcc_arm + KCFLAGS="-Wmaybe-uninitialized".

Clang static analyzer: vout_res[1] (max20826.c:813) — CI warning

The CI clang_analyzer warning (core.UndefinedBinaryOperatorResult) at line 813 is a false
positive: the ret < 0 guard prevents the uninitialized access path. Initializing u8 vout_res[2] = {}
silences both the analyzer and is safer.

060a8049dad7f - hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

Missing -ENODATA delegation contract (pmbus.h:462)

The new read_block_data hook comment only states the buffer size. The existing comment block above
covers read_byte_data and read_word_data with the -ENODATA delegation convention. The hook
itself lacks this documentation even though _pmbus_read_block_data() uses -ENODATA as a
delegation sentinel, and max20826_read_block_data() deliberately returns -EOPNOTSUPP (not
-ENODATA) for unhandled registers.

Observations (non-blocking)

• pfunc[0..n-1] is filled for both RAIL_A and RAIL_B starting at index 0. With an 8+8
  configuration RAIL_B's call overwrites RAIL_A's entries. Since both rails use the same mask
  (PMBUS_HAVE_IOUT | PMBUS_HAVE_IIN) this is functionally harmless, but architecturally
  inconsistent with mp2975.c which uses non-overlapping indices.
• oper_save = -1 sentinel and ret2 = 0 initialization: present and correct.
• i2c_get_match_data() (not device_get_match_data()): correct.
• guard(pmbus_lock) in max20826_high_speed_en_store(): present.
• Regulator enable return-value fix in pmbus_core.c:3110-3117: correct and confirmed
  against drivers/regulator/core.c:3280 gate check.

CI warnings

The clang_analyzer warning for max20826.c:813 (UndefinedBinaryOperatorResult) is a false
positive but is silenced by the proposed fix. The kernel_smatch failures are all in core kernel
files unrelated to this series.

Verification data

• Compiled max20826.o with set_arch llvm_x86 + auto_set_kconfig — clean build.
• Compiled max20826.o with set_arch gcc_arm + KCFLAGS="-Wmaybe-uninitialized" —
  val_buf[2] warning reproduced at max20826.c:653.
• Applied val_buf[3] = {} and vout_res[2] = {} fixes, rebuilt GCC arm — zero warnings.
• checkpatch.pl --strict --no-signoff on all five commits — no errors; whitespace/trailer
  warnings are false positives per Documentation/process/submitting-patches.rst.
• gpiod_set_value_cansleep(NULL, ...) safety confirmed at drivers/gpio/gpiolib.c:385-401.
• Datasheets are NDA-protected (noted in Documentation/hwmon/max20826.rst).

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 29344289845

Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

[sipraga]

quite a weak review from me as I'm not at all familiar with pmbus, but having said that, it looks very good to me

[actorreno]

this line changes page to RAIL B

if no bvren declared, driver can still probe, but it's on rail B rather than rail A

around line 920 or before 'ret = max20826_setup_vout_format(st);'

should we reselect RAIL A?

[actorreno]

should we revert to the older code style

• check bvren gpio
• if(bvren)
  select rail b
  do rail B code stuff

[nunojsa]

Hmm yeah seems right! Will fix this

[nunojsa]

So, in the end I ignored this because of two things:

• bvren is an optional GPIO. RAIL_B enable pin can be something always on with no control from Linux;
• max20826_setup_vout_format() already selects the proper RAIL it needs so being left in RAIL_B should not be an issue

[actorreno]

I guess that's what it means to be gpio_optional, ok I get it. :)

[actorreno]

missing defintion of this small letters 'field_get'
innersource version has:

#define field_get(_mask, _reg) (_reg & _mask) >> __builtin_ctz(_mask);

because the actual FIELD_GET wanted a constant for the mask field

(not sure if there's a cleaner way to do this, and am surprised this built with no error)

[nunojsa]

not an issue! upstream kernel already defines field_get() for non constant masks

[actorreno]

max209xx devices are VR12,

[nunojsa]

Will double check this one

[nunojsa]

Changes in v2:
* hwmon: (pmbus) max20826: set VR12 (not VR13) VID encoding for the AMD SVI3 parts (max20908/912/916) (@actorreno)
* hwmon: (pmbus) max20826: drop stray blank line in the OPL comment block (@sipraga)