[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45d1a08c-8a7f-4b6e-8068-4ab0e63abbf3@roeck-us.net>
Date: Fri, 9 May 2025 06:54:55 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Pawel Dembicki <paweldembicki@...il.com>, linux-hwmon@...r.kernel.org
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
Noah Wang <noahwang.wang@...look.com>, Fabio Estevam <festevam@...il.com>,
Naresh Solanki <naresh.solanki@...ements.com>,
Michal Simek <michal.simek@....com>, Grant Peltier
<grantpeltier93@...il.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Peter Zijlstra <peterz@...radead.org>, Shen Lichuan <shenlichuan@...o.com>,
Greg KH <gregkh@...uxfoundation.org>, Charles Hsu <ythsu0511@...il.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 2/5] hwmon: pmbus: mpq8785: Add support for MPM82504
On 5/8/25 23:51, Pawel Dembicki wrote:
> Add support for the Monolithic Power Systems MPM82504 digital voltage
> regulator. MPM82504 uses PMBus direct format for voltage output.
>
> Tested with device tree based matching.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
>
> ---
> v2:
> - fixed signedess for temperatures < 0 deg C
> - remove empty lines
> ---
> Documentation/hwmon/mpq8785.rst | 20 +++++++++++++++-----
> drivers/hwmon/pmbus/mpq8785.c | 30 +++++++++++++++++++++++++++++-
> 2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/hwmon/mpq8785.rst b/Documentation/hwmon/mpq8785.rst
> index bf8176b87086..be228ee58ce2 100644
> --- a/Documentation/hwmon/mpq8785.rst
> +++ b/Documentation/hwmon/mpq8785.rst
> @@ -6,6 +6,7 @@ Kernel driver mpq8785
> Supported chips:
>
> * MPS MPQ8785
> + * MPS MPM82504
>
> Prefix: 'mpq8785'
>
> @@ -20,21 +21,30 @@ buck converter. The MPQ8785 offers a very compact solution that achieves up to
> wide input supply range. The MPQ8785 operates at high efficiency over a wide
> output current load range.
>
> +The MPM82504 is a quad 25A, scalable, fully integrated power module with a PMBus
> +interface. The device offers a complete power solution that achieves up to 25A
> +per output channel. The MPM82504 has four output channels that can be paralleled
> +to provide 50A, 75A, or 100A of output current for flexible configurations.
> +The device can also operate in parallel with the MPM3695-100 and additional
> +MPM82504 devices to provide a higher output current. The MPM82504 operates
> +at high efficiency across a wide load range.
> +
> The PMBus interface provides converter configurations and key parameters
> monitoring.
>
> -The MPQ8785 adopts MPS's proprietary multi-phase digital constant-on-time (MCOT)
> +The devices adopts MPS's proprietary multi-phase digital constant-on-time (MCOT)
> control, which provides fast transient response and eases loop stabilization.
> -The MCOT scheme also allows multiple MPQ8785 devices to be connected in parallel
> -with excellent current sharing and phase interleaving for high-current
> +The MCOT scheme also allows multiple devices or chennels to be connected in
channels
> +parallel with excellent current sharing and phase interleaving for high-current
> applications.
>
> Fully integrated protection features include over-current protection (OCP),
> over-voltage protection (OVP), under-voltage protection (UVP), and
> over-temperature protection (OTP).
>
> -The MPQ8785 requires a minimal number of readily available, standard external
> -components, and is available in a TLGA (5mmx6mm) package.
> +All supported modules require a minimal number of readily available, standard
> +external components. The MPQ8785 is available in a TLGA (5mmx6mm) package
> +and the MPM82504 is available in a BGA (15mmx30mmx5.18mm) package.
>
> Device compliant with:
>
> diff --git a/drivers/hwmon/pmbus/mpq8785.c b/drivers/hwmon/pmbus/mpq8785.c
> index 00ec21b081cb..9a4a211b2aeb 100644
> --- a/drivers/hwmon/pmbus/mpq8785.c
> +++ b/drivers/hwmon/pmbus/mpq8785.c
> @@ -8,7 +8,9 @@
> #include <linux/of_device.h>
> #include "pmbus.h"
>
> -enum chips { mpq8785 };
> +#define PMBUS_READ_TEMPERATURE_1_SIGN BIT(9)
s/PMBUS/MPM82504/
because this is not a standard bit. Needs to include linux/bits.h
or better linux/bitops.h (see below).
> +
> +enum chips { mpq8785, mpm82504 };
Please keep those in alphabetic order.
>
> static int mpq8785_identify(struct i2c_client *client,
> struct pmbus_driver_info *info)
> @@ -36,6 +38,23 @@ static int mpq8785_identify(struct i2c_client *client,
> return 0;
> };
>
> +static int mpm82504_read_word_data(struct i2c_client *client, int page,
> + int phase, int reg)
> +{
> + int ret;
> +
> + ret = pmbus_read_word_data(client, page, phase, reg);
> +
> + if (ret < 0 || reg != PMBUS_READ_TEMPERATURE_1)
> + return ret;
> +
> + /* Fix PMBUS_READ_TEMPERATURE_1 signedness */
> + if (ret & PMBUS_READ_TEMPERATURE_1_SIGN)
> + ret |= GENMASK(15, 10);
Better use sign_extend32(ret, PMBUS_READ_TEMPERATURE_1_SIGN) & 0xffff.
The mask is a bit odd but there is no sign_extend16(), but this makes it
more obvious what is done here.
Also, this needs to include linux/bitops.h.
> +
> + return ret;
> +}
> +
> static struct pmbus_driver_info mpq8785_info = {
> .pages = 1,
> .format[PSC_VOLTAGE_IN] = direct,
> @@ -59,12 +78,14 @@ static struct pmbus_driver_info mpq8785_info = {
>
> static const struct i2c_device_id mpq8785_id[] = {
> { "mpq8785", mpq8785 },
> + { "mpm82504", mpm82504 },
> { },
> };
> MODULE_DEVICE_TABLE(i2c, mpq8785_id);
>
> static const struct of_device_id __maybe_unused mpq8785_of_match[] = {
> { .compatible = "mps,mpq8785", .data = (void *)mpq8785 },
> + { .compatible = "mps,mpm82504", .data = (void *)mpm82504 },
Alphabetic order.
> {}
> };
> MODULE_DEVICE_TABLE(of, mpq8785_of_match);
> @@ -88,6 +109,13 @@ static int mpq8785_probe(struct i2c_client *client)
> case mpq8785:
> info->identify = mpq8785_identify;
> break;
> + case mpm82504:
> + info->format[PSC_VOLTAGE_OUT] = direct;
> + info->m[PSC_VOLTAGE_OUT] = 8;
> + info->b[PSC_VOLTAGE_OUT] = 0;
> + info->R[PSC_VOLTAGE_OUT] = 2;
> + info->read_word_data = mpm82504_read_word_data;
> + break;
Same here, with case statements. Sorry, I should have noticed in v1.
Thanks,
Guenter
> default:
> return -ENODEV;
> }
Powered by blists - more mailing lists