| 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: <65d6635c-dc16-4462-96f7-9b82c49f5673@roeck-us.net>
Date: Mon, 4 Aug 2025 13:04:37 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: ChiShih Tsai <tomtsai764@...il.com>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Cc: jdelvare@...e.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, corbet@....net, t630619@...il.com
Subject: Re: [PATCH 2/2] hwmon: (pmbus/adm1275) add sq24905c support
On 8/4/25 05:48, ChiShih Tsai wrote:
> Add support for sq24905c which is similar to adm1275 and other chips
> of the series.
>
> Signed-off-by: ChiShih Tsai <tomtsai764@...il.com>
> ---
> Documentation/hwmon/adm1275.rst | 24 ++++++++++++++++--------
> drivers/hwmon/pmbus/Kconfig | 5 +++--
> drivers/hwmon/pmbus/adm1275.c | 27 +++++++++++++++++++--------
> 3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/hwmon/adm1275.rst b/Documentation/hwmon/adm1275.rst
> index 57bd7a850558..914f009f34e0 100644
> --- a/Documentation/hwmon/adm1275.rst
> +++ b/Documentation/hwmon/adm1275.rst
> @@ -67,6 +67,14 @@ Supported chips:
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1293_1294.pdf
>
> + * Silergy SQ24905C
> +
> + Prefix: 'sq24905c'
> +
> + Addresses scanned: -
> +
> + Datasheet: https://www.silergy.com/download/downloadFile?id=5669&type=product&ftype=note
> +
> Author: Guenter Roeck <linux@...ck-us.net>
>
>
> @@ -74,14 +82,14 @@ Description
> -----------
>
> This driver supports hardware monitoring for Analog Devices ADM1075, ADM1272,
> -ADM1273, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and ADM1294 Hot-Swap
> -Controller and Digital Power Monitors.
> +ADM1273, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, ADM1294, and SQ24905C
> +Hot-Swap Controller and Digital Power Monitors.
>
> -ADM1075, ADM1272, ADM1273, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293, and
> -ADM1294 are hot-swap controllers that allow a circuit board to be removed from
> -or inserted into a live backplane. They also feature current and voltage
> -readback via an integrated 12 bit analog-to-digital converter (ADC), accessed
> -using a PMBus interface.
> +ADM1075, ADM1272, ADM1273, ADM1275, ADM1276, ADM1278, ADM1281, ADM1293,
> +ADM1294 and SQ24905C are hot-swap controllers that allow a circuit board to be
> +removed from or inserted into a live backplane. They also feature current and
> +voltage readback via an integrated 12 bit analog-to-digital converter (ADC),
> +accessed using a PMBus interface.
>
> The driver is a client driver to the core PMBus driver. Please see
> Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> @@ -160,5 +168,5 @@ temp1_highest Highest observed temperature.
> temp1_reset_history Write any value to reset history.
>
> Temperature attributes are supported on ADM1272,
> - ADM1273, ADM1278, and ADM1281.
> + ADM1273, ADM1278, ADM1281 and SQ24905C.
> ======================= =======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 55e492452ce8..7485bc6b2e8a 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -51,8 +51,9 @@ config SENSORS_ADM1275
> tristate "Analog Devices ADM1275 and compatibles"
> help
> If you say yes here you get hardware monitoring support for Analog
> - Devices ADM1075, ADM1272, ADM1273, ADM1275, ADM1276, ADM1278, ADM1281,
> - ADM1293, and ADM1294 Hot-Swap Controller and Digital Power Monitors.
> + Devices ADM1075, ADM1272, ADM1273, ADM1275, ADM1276, ADM1278,
> + ADM1281, ADM1293, ADM1294 and SQ24905C Hot-Swap Controller and
> + Digital Power Monitors.
>
> This driver can also be built as a module. If so, the module will
> be called adm1275.
> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> index 7d175baa5de2..1c032aaac379 100644
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
> @@ -18,7 +18,8 @@
> #include <linux/log2.h>
> #include "pmbus.h"
>
> -enum chips { adm1075, adm1272, adm1273, adm1275, adm1276, adm1278, adm1281, adm1293, adm1294 };
> +enum chips { adm1075, adm1272, adm1273, adm1275, adm1276, adm1278, adm1281,
> + adm1293, adm1294, sq24905c };
>
> #define ADM1275_MFR_STATUS_IOUT_WARN2 BIT(0)
> #define ADM1293_MFR_STATUS_VAUX_UV_WARN BIT(5)
> @@ -486,6 +487,7 @@ static const struct i2c_device_id adm1275_id[] = {
> { "adm1281", adm1281 },
> { "adm1293", adm1293 },
> { "adm1294", adm1294 },
> + { "MC09C", sq24905c },
mc09c (lower case). But how did you test this ? This seems to be intended to match
PMBUS_MFR_MODEL, but it is not the ID suggested in devicetree properties
(silergy,sq24905c). It seems to me that the match code in the probe function needs
to be modified to handle situations where PMBUS_MFR_MODEL does not match the
device id.
> { }
> };
> MODULE_DEVICE_TABLE(i2c, adm1275_id);
> @@ -533,8 +535,10 @@ static int adm1275_probe(struct i2c_client *client)
> return ret;
> }
> if (ret != 3 || strncmp(block_buffer, "ADI", 3)) {
> - dev_err(&client->dev, "Unsupported Manufacturer ID\n");
> - return -ENODEV;
> + if (ret != 2 || strncmp(block_buffer, "SY", 2)) {
> + dev_err(&client->dev, "Unsupported Manufacturer ID\n");
> + return -ENODEV;
> + }
Combine to a single if statement.
if ((ret != 3 || strncmp(block_buffer, "ADI", 3)) &&
(ret != 2 || strncmp(block_buffer, "SY", 2))) {
dev_err(&client->dev, "Unsupported Manufacturer ID\n");
return -ENODEV;
}
> }
>
> ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> @@ -558,7 +562,8 @@ static int adm1275_probe(struct i2c_client *client)
>
> if (mid->driver_data == adm1272 || mid->driver_data == adm1273 ||
> mid->driver_data == adm1278 || mid->driver_data == adm1281 ||
> - mid->driver_data == adm1293 || mid->driver_data == adm1294)
> + mid->driver_data == adm1293 || mid->driver_data == adm1294 ||
> + mid->driver_data == sq24905c)
> config_read_fn = i2c_smbus_read_word_data;
> else
> config_read_fn = i2c_smbus_read_byte_data;
> @@ -708,6 +713,7 @@ static int adm1275_probe(struct i2c_client *client)
> break;
> case adm1278:
> case adm1281:
> + case sq24905c:
That suggests compatibility to ADM1278 and ADM1281, not ADM1275.
That should be mentioned in the description.
> data->have_vout = true;
> data->have_pin_max = true;
> data->have_temp_max = true;
> @@ -786,9 +792,12 @@ static int adm1275_probe(struct i2c_client *client)
> return -ENODEV;
> }
>
> - if (data->have_power_sampling &&
> + if ((data->have_power_sampling &&
> of_property_read_u32(client->dev.of_node,
> - "adi,power-sample-average", &avg) == 0) {
> + "adi,power-sample-average", &avg) == 0) ||
> + (data->have_power_sampling &&
> + of_property_read_u32(client->dev.of_node,
> + "silergy,power-sample-average", &avg) == 0)) {
Checking for have_power_sampling twice just makes the code unnecessarily complex.
Also, if it is really even acceptable to have two properties for the same
function, there will need to be a validation to ensure that only the
"correct" one, based on the chip type, is used.
> if (!avg || avg > ADM1275_SAMPLES_AVG_MAX ||
> BIT(__fls(avg)) != avg) {
> dev_err(&client->dev,
> @@ -804,8 +813,10 @@ static int adm1275_probe(struct i2c_client *client)
> }
> }
>
> - if (of_property_read_u32(client->dev.of_node,
> - "adi,volt-curr-sample-average", &avg) == 0) {
> + if ((of_property_read_u32(client->dev.of_node,
> + "adi,volt-curr-sample-average", &avg) == 0) ||
> + (of_property_read_u32(client->dev.of_node,
> + "silergy,volt-curr-sample-average", &avg) == 0)) {
> if (!avg || avg > ADM1275_SAMPLES_AVG_MAX ||
> BIT(__fls(avg)) != avg) {
> dev_err(&client->dev,
Powered by blists - more mailing lists