| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <80259b75-dfa6-4e49-8d9d-9edd72ae672f@roeck-us.net>
Date: Wed, 19 Mar 2025 21:24:10 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Alexis Czezar Torreno <alexisczezar.torreno@...log.com>,
Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
Cc: linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH 1/2] hwmon: (pmbus/max34440): Fix support for max34451
On 3/19/25 20:55, Alexis Czezar Torreno wrote:
> The max344** family has an issue with some PMBUS address being switched.
> This includes max34451 however version MAX34451-NA6 and later has this
> issue fixed and this commit supports that update.
>
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@...log.com>
> ---
> Documentation/hwmon/max34440.rst | 8 ++-
> drivers/hwmon/pmbus/max34440.c | 102 ++++++++++++++++++++++++++-------------
> 2 files changed, 74 insertions(+), 36 deletions(-)
>
> diff --git a/Documentation/hwmon/max34440.rst b/Documentation/hwmon/max34440.rst
> index 162d289f08140341e8e76ab7033834ba07a8b935..b1f3f75091bb8e233e766c24913194dd62b0cd90 100644
> --- a/Documentation/hwmon/max34440.rst
> +++ b/Documentation/hwmon/max34440.rst
> @@ -35,7 +35,7 @@ Supported chips:
>
> PMBus 16-Channel V/I Monitor and 12-Channel Sequencer/Marginer
>
> - Prefixes: 'max34451'
> + Prefixes: 'max34451', 'max34451_na6'
>
> Addresses scanned: -
>
> @@ -93,6 +93,10 @@ attribute is set to a positive value. Power measurement is only enabled if
> channel 1 (3) is configured for voltage measurement, and channel 2 (4) is
> configured for current measurement.
>
> +For MAX34451, version MAX34451ETNA6+ and later are denoted with prefix
> +'max34451_na6'. The previous versions contains some errors on the PMBUS
> +addresses and these are fixed on the later versions.
> +
>
> Platform data support
> ---------------------
> @@ -192,4 +196,4 @@ temp[1-8]_reset_history Write any value to reset history.
> - MAX34451 supports attribute groups in[1-16] (or curr[1-16] based on
> input pins) and temp[1-5].
> - MAX34460 supports attribute groups in[1-12] and temp[1-5].
> - - MAX34461 supports attribute groups in[1-16] and temp[1-5].
> + - MAX34461 supports attribute groups in[1-16] and temp[1-5].
> \ No newline at end of file
> diff --git a/drivers/hwmon/pmbus/max34440.c b/drivers/hwmon/pmbus/max34440.c
> index c9dda33831ff24e7b5e2fd1956a65e6bd2bfcbb9..d483c01f256c96f048c9da5981f10f52402d981c 100644
> --- a/drivers/hwmon/pmbus/max34440.c
> +++ b/drivers/hwmon/pmbus/max34440.c
> @@ -14,7 +14,15 @@
> #include <linux/i2c.h>
> #include "pmbus.h"
>
> -enum chips { max34440, max34441, max34446, max34451, max34460, max34461 };
> +enum chips {
> + max34440,
> + max34441,
> + max34446,
> + max34451,
> + max34451_na6,
> + max34460,
> + max34461,
> +};
>
> #define MAX34440_MFR_VOUT_PEAK 0xd4
> #define MAX34440_MFR_IOUT_PEAK 0xd5
> @@ -34,6 +42,7 @@ enum chips { max34440, max34441, max34446, max34451, max34460, max34461 };
> /*
> * The whole max344* family have IOUT_OC_WARN_LIMIT and IOUT_OC_FAULT_LIMIT
> * swapped from the standard pmbus spec addresses.
> + * For max34451, version MAX34451ETNA6+ and later has this issue fixed.
> */
> #define MAX34440_IOUT_OC_WARN_LIMIT 0x46
> #define MAX34440_IOUT_OC_FAULT_LIMIT 0x4A
> @@ -59,12 +68,20 @@ static int max34440_read_word_data(struct i2c_client *client, int page,
>
> switch (reg) {
> case PMBUS_IOUT_OC_FAULT_LIMIT:
> - ret = pmbus_read_word_data(client, page, phase,
> - MAX34440_IOUT_OC_FAULT_LIMIT);
> + if (data->id == max34451_na6)
Use a flag instead of a chip ID, or even better store the register addresses
in max34440_data to avoid the runtime checks.
> + ret = pmbus_read_word_data(client, page, phase,
> + PMBUS_IOUT_OC_FAULT_LIMIT);
> + else
> + ret = pmbus_read_word_data(client, page, phase,
> + MAX34440_IOUT_OC_FAULT_LIMIT);
> break;
> case PMBUS_IOUT_OC_WARN_LIMIT:
> - ret = pmbus_read_word_data(client, page, phase,
> - MAX34440_IOUT_OC_WARN_LIMIT);
> + if (data->id == max34451_na6)
> + ret = pmbus_read_word_data(client, page, phase,
> + PMBUS_IOUT_OC_WARN_LIMIT);
> + else
> + ret = pmbus_read_word_data(client, page, phase,
> + MAX34440_IOUT_OC_WARN_LIMIT);
> break;
> case PMBUS_VIRT_READ_VOUT_MIN:
> ret = pmbus_read_word_data(client, page, phase,
> @@ -75,7 +92,8 @@ static int max34440_read_word_data(struct i2c_client *client, int page,
> MAX34440_MFR_VOUT_PEAK);
> break;
> case PMBUS_VIRT_READ_IOUT_AVG:
> - if (data->id != max34446 && data->id != max34451)
> + if (data->id != max34446 && data->id != max34451 &&
> + data->id != max34451_na6)
> return -ENXIO;
> ret = pmbus_read_word_data(client, page, phase,
> MAX34446_MFR_IOUT_AVG);
> @@ -133,12 +151,20 @@ static int max34440_write_word_data(struct i2c_client *client, int page,
>
> switch (reg) {
> case PMBUS_IOUT_OC_FAULT_LIMIT:
> - ret = pmbus_write_word_data(client, page, MAX34440_IOUT_OC_FAULT_LIMIT,
> - word);
> + if (data->id == max34451_na6)
> + ret = pmbus_write_word_data(client, page, PMBUS_IOUT_OC_FAULT_LIMIT,
> + word);
> + else
> + ret = pmbus_write_word_data(client, page, MAX34440_IOUT_OC_FAULT_LIMIT,
> + word);
> break;
> case PMBUS_IOUT_OC_WARN_LIMIT:
> - ret = pmbus_write_word_data(client, page, MAX34440_IOUT_OC_WARN_LIMIT,
> - word);
> + if (data->id == max34451_na6)
> + ret = pmbus_write_word_data(client, page, PMBUS_IOUT_OC_WARN_LIMIT,
> + word);
> + else
> + ret = pmbus_write_word_data(client, page, MAX34440_IOUT_OC_WARN_LIMIT,
> + word);
> break;
> case PMBUS_VIRT_RESET_POUT_HISTORY:
> ret = pmbus_write_word_data(client, page,
> @@ -159,7 +185,8 @@ static int max34440_write_word_data(struct i2c_client *client, int page,
> case PMBUS_VIRT_RESET_IOUT_HISTORY:
> ret = pmbus_write_word_data(client, page,
> MAX34440_MFR_IOUT_PEAK, 0);
> - if (!ret && (data->id == max34446 || data->id == max34451))
> + if (!ret && (data->id == max34446 || data->id == max34451 ||
> + data->id == max34451_na6))
> ret = pmbus_write_word_data(client, page,
> MAX34446_MFR_IOUT_AVG, 0);
>
> @@ -270,6 +297,29 @@ static int max34451_set_supported_funcs(struct i2c_client *client,
> return 0;
> }
>
> +#define MAX34451_COMMON_INFO \
> + .pages = 21, \
> + .format[PSC_VOLTAGE_OUT] = direct, \
> + .format[PSC_TEMPERATURE] = direct, \
> + .format[PSC_CURRENT_OUT] = direct, \
> + .m[PSC_VOLTAGE_OUT] = 1, \
> + .b[PSC_VOLTAGE_OUT] = 0, \
> + .R[PSC_VOLTAGE_OUT] = 3, \
> + .m[PSC_CURRENT_OUT] = 1, \
> + .b[PSC_CURRENT_OUT] = 0, \
> + .R[PSC_CURRENT_OUT] = 2, \
> + .m[PSC_TEMPERATURE] = 1, \
> + .b[PSC_TEMPERATURE] = 0, \
> + .R[PSC_TEMPERATURE] = 2, \
> + /* func 0-15 is set dynamically before probing */ \
> + .func[16] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, \
> + .func[17] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, \
> + .func[18] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, \
> + .func[19] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, \
> + .func[20] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP, \
> + .read_word_data = max34440_read_word_data, \
> + .write_word_data = max34440_write_word_data,
> +
> static struct pmbus_driver_info max34440_info[] = {
> [max34440] = {
> .pages = 14,
> @@ -394,27 +444,10 @@ static struct pmbus_driver_info max34440_info[] = {
> .write_word_data = max34440_write_word_data,
> },
> [max34451] = {
> - .pages = 21,
> - .format[PSC_VOLTAGE_OUT] = direct,
> - .format[PSC_TEMPERATURE] = direct,
> - .format[PSC_CURRENT_OUT] = direct,
> - .m[PSC_VOLTAGE_OUT] = 1,
> - .b[PSC_VOLTAGE_OUT] = 0,
> - .R[PSC_VOLTAGE_OUT] = 3,
> - .m[PSC_CURRENT_OUT] = 1,
> - .b[PSC_CURRENT_OUT] = 0,
> - .R[PSC_CURRENT_OUT] = 2,
> - .m[PSC_TEMPERATURE] = 1,
> - .b[PSC_TEMPERATURE] = 0,
> - .R[PSC_TEMPERATURE] = 2,
> - /* func 0-15 is set dynamically before probing */
> - .func[16] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP,
> - .func[17] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP,
> - .func[18] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP,
> - .func[19] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP,
> - .func[20] = PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP,
> - .read_word_data = max34440_read_word_data,
> - .write_word_data = max34440_write_word_data,
> + MAX34451_COMMON_INFO,
> + },
> + [max34451_na6] = {
> + MAX34451_COMMON_INFO,
> },
This is way too complicated. Use a flag or set the register addresses in struct
max34440_data instead.
> [max34460] = {
> .pages = 18,
> @@ -495,7 +528,7 @@ static int max34440_probe(struct i2c_client *client)
> data->id = i2c_match_id(max34440_id, client)->driver_data;
> data->info = max34440_info[data->id];
>
> - if (data->id == max34451) {
> + if (data->id == max34451 || data->id == max34451_na6) {
> rv = max34451_set_supported_funcs(client, data);
> if (rv)
> return rv;
> @@ -509,6 +542,7 @@ static const struct i2c_device_id max34440_id[] = {
> {"max34441", max34441},
> {"max34446", max34446},
> {"max34451", max34451},
> + {"max34451_na6", max34451_na6},
Relying on this is way too fragile. This must be detectable from the chip;
maybe using MFR_REVISION.
> {"max34460", max34460},
> {"max34461", max34461},
> {}
> @@ -529,4 +563,4 @@ module_i2c_driver(max34440_driver);
> MODULE_AUTHOR("Guenter Roeck");
> MODULE_DESCRIPTION("PMBus driver for Maxim MAX34440/MAX34441");
> MODULE_LICENSE("GPL");
> -MODULE_IMPORT_NS("PMBUS");
> +MODULE_IMPORT_NS(PMBUS);
Looks like your code is based on an older kernel branch. Please
rebase on top of mainline.
Guenter
>
Powered by blists - more mailing lists