Re: [PATCH v4 3/3] hwmon: pmbus: add MPQ8646 driver

[Guenter Roeck]

On 7/23/26 14:38, Vincent Jardin via B4 Relay wrote:
> From: Vincent Jardin <vjardin@xxxxxxx>
>
> Add a new single-chip driver for the MPS MPQ8646 that is
> a PMBus device.
>
> Beyond basic PMBus telemetry, the driver adds:
>    - MFR_CFG_EXT gate-close retry after CLEAR_LAST_FAULT.
>    - alarm acknowledge via inX_reset_history.
>    - STATUS_WORD MPS-extended bit decode + post-mortem clear.
>    - In-driver alarm-poll fallback work item (thanks lm90) for
>      boards without SMBALERT
>    - on_off_config/vout_margin/mfr_pmbus_lock sysfs.
>
> Signed-off-by: Vincent Jardin <vjardin@xxxxxxx>
> ---
>   Documentation/hwmon/mpq8646.rst |  314 ++++++++++
>   MAINTAINERS                     |    8 +
>   drivers/hwmon/pmbus/Kconfig     |   11 +
>   drivers/hwmon/pmbus/Makefile    |    1 +
>   drivers/hwmon/pmbus/mpq8646.c   | 1237 +++++++++++++++++++++++++++++++++++++++
>   5 files changed, 1571 insertions(+)
>
> diff --git a/Documentation/hwmon/mpq8646.rst b/Documentation/hwmon/mpq8646.rst
...
> +Diagnostics and introspection
> +-----------------------------
> +
> +When ``CONFIG_DEBUG_FS=y`` is set, the driver exposes the following
> +entries under ``/sys/kernel/debug/mpq8646/<bus>-<addr>/``. Most are diagnostic
> +probes; the read/write entries (marked R/W) are direct accessors to
> +the underlying chip command.
> +
> +Identity / observability (read-only):
> +
> +==========================  ==================  =================================
> +File                        PMBus / MFR cmd     Description
> +==========================  ==================  =================================
> +``mfr_config_id``           0xC0 (word)         board / SKU NVM identifier
> +``mfr_config_code_rev``     0xC1 (word)         NVM image revision (PART_RECOG +
> +                                                config code rev fields)
> +``mfr_silicon_rev``         0xC3 (byte)         die revision
> +``mfr_retry_times``         0xF4 (word)         per-fault-class recovery-mode
> +                                                configuration (NOT a retry
> +                                                count). Four 4-bit fields
> +                                                [15:12]=OTP, [11:8]=VOUT_OV,
> +                                                [7:4]=VOUT_UV, [3:0]=OCP.
> +                                                Each field: 0x0=latch-off,
> +                                                0x1..0xE=retry N times then
> +                                                latch off, 0xF=hiccup
> +                                                (retry indefinitely). The
> +                                                chip exposes no in-NVM
> +                                                retry-event counter; track
> +                                                transitions externally if
> +                                                needed (poll
> +                                                ``protection_last`` or watch
> +                                                ``inX_alarm`` / ``temp1_alarm``
> +                                                ``poll(POLLPRI)`` wakes).
> +``mfr_vboot_cfg``           0xFC (word)         ADDR/VBOOT latched at POR
> +==========================  ==================  =================================
> +
> +Timing / UVLO knobs (read-only):
> +
> +==========================  ==================  =================================
> +File                        PMBus cmd           Description
> +==========================  ==================  =================================
> +``vin_on``                  0x35 (word)         UVLO turn-on threshold
> +``vin_off``                 0x36 (word)         UVLO turn-off threshold
> +``ton_delay``               0x60 (word)         soft-start delay
> +``ton_rise``                0x61 (word)         soft-start ramp time
> +``toff_delay``              0x64 (word)         soft-stop delay
> +``toff_fall``               0x65 (word)         soft-stop ramp time
> +==========================  ==================  =================================
> +
> +Configuration (read/write):
> +
> +==========================  ==================  =================================
> +File                        PMBus / MFR cmd     Description
> +==========================  ==================  =================================
> +``on_off_config``           0x02 (byte)         PMBus vs CTRL-pin on/off source +
> +                                                active polarity
> +``vout_margin_high``        0x25 (word)         production margin-high VOUT setpoint
> +``vout_margin_low``         0x26 (word)         production margin-low VOUT setpoint
> +``mfr_pmbus_lock``          0xEE (word)         programmable PMBus write-lock
> +                                                (independent of WRITE_PROTECT)
> +``mfr_product_rev_user``    0xC2 (word)         user-programmable product revision
> +``alarm_poll_interval_ms``  --                  alarm-poll worker cadence (driver-
> +                                                local, not a chip register)
> +==========================  ==================  =================================
> +
> +NVM commit / revert (write-only):
> +
> +==========================  ==================  =================================
> +File                        PMBus cmd           Description
> +==========================  ==================  =================================
> +``store_all``               0x15                STORE_USER_ALL Send-Byte (commit RAM
> +                                                config to chip NVM)
> +``restore_all``             0x16                RESTORE_USER_ALL Send-Byte (revert
> +                                                RAM to last-NVM image)
> +==========================  ==================  =================================
> +
> +Bring-up probes (write-only triggers, results in ``last_probe``):
> +
> +==========================  ==================================================
> +File                        Action
> +==========================  ==================================================
> +``probe_smbus_rword <reg>``  i2c_smbus_read_word_data on <reg>
> +``probe_smbus_rbyte <reg>``  i2c_smbus_read_byte_data on <reg>
> +``probe_raw_xfer <reg>``     raw i2c_transfer read-word on <reg>
> +``probe_page_write <pg>``    write PMBUS_PAGE = <pg>
> +``probe_clear_faults``       send CLEAR_FAULTS (0x03) Send-Byte
> +``force_raw_xfer``           if 1, the read_word_data hook uses raw i2c_transfer
> +``delay_us``                 udelay before each chip-driver hook call
> +``stats``                    per-hook call / error counters
> +``last_probe``               result of the most recent probe_* operation
> +``reset_stats``              zero the stats counters

Many if not all of those commands should really not be part of this driver,
and commands such as probe_page_write would be extremely dangerous and
mess up the driver if ever used.

Are you really sure you want to expose all those to the user, instead
of (say) force them to use i2c_dev if they really want to risk turning
the affected board(s) into a brick ?

I won't prevent you from doing all this if you insist. I used to,
but it does seem that people insist in shooting themselves into the
foot, so I gave up on it. I am, however, telling you that exposing all
those commands is extremely risky.

Guenter