Re: [PATCH v2] hwmon: Add driver for MPS MPQ8785 Synchronous Step-Down Converter

[Guenter Roeck]

On 1/29/24 00:31, Charles Hsu wrote:
> Add support for mpq8785 device from Monolithic Power Systems, Inc.
> (MPS) vendor. This is synchronous step-down controller.
>
> Signed-off-by: Charles Hsu <ythsu0511@xxxxxxxxx>
>
> ---
> Change in v1:
>      Initial patchset.
> Change in v2:
>      1.Add pmbus support status registers.
>      2.Add mpq8785 in trivial-devices.yaml.
>      3.Remove format[PSC_VOLTAGE_OUT]

This is insufficient, sorry. Problem is that pmbus_identify_common()
compares the more reported by PMBUS_VOUT_MODE with the configured mode.
Not setting format[PSC_VOLTAGE_OUT] means that the mode is the default (0),
which is linear mode. If the chip is configured for direct or vid mode
this again will result in probe failure.

I can see two options:

- Add a chip configuration flag such as PMBUS_DETECT_VOUT_MODE (for lack
  of a better idea) and use it in pmbus_identify_common() to actually
  _set_ the mode from the VOUT_MODE command instead of expecting it
  to be already set. This would require providing the coefficients for
  output voltage direct mode in mpq8785_info. This would be my preferred
  solution (but require a separate patch to add the code to pmbus_core.c).
- Select a fixed mode and write code to convert affected register readings
  to that mode if needed. See mp5990.c for an example.

>      4.Fix MODULE_DESCRIPTION
> ---
>   .../devicetree/bindings/trivial-devices.yaml  |  2 +
>   Documentation/hwmon/index.rst                 |  1 +
>   Documentation/hwmon/mpq8785.rst               | 94 +++++++++++++++++++
>   drivers/hwmon/pmbus/Kconfig                   |  9 ++
>   drivers/hwmon/pmbus/Makefile                  |  1 +
>   drivers/hwmon/pmbus/mpq8785.c                 | 62 ++++++++++++
>   6 files changed, 169 insertions(+)
>   create mode 100644 Documentation/hwmon/mpq8785.rst
>   create mode 100644 drivers/hwmon/pmbus/mpq8785.c
>
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 79dcd92c4a43..088b23ed2ae6 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -129,6 +129,8 @@ properties:
>             - mps,mp2975
>               # Monolithic Power Systems Inc. multi-phase hot-swap controller mp5990
>             - mps,mp5990
> +            # Monolithic Power Systems Inc. synchronous step-down converter mpq8785
> +          - mps,mpq8785
>               # Honeywell Humidicon HIH-6130 humidity/temperature sensor
>             - honeywell,hi6130
>               # IBM Common Form Factor Power Supply Versions (all versions)

Sorry, devicetree changes require a separate patch.

Thanks,
Guenter