| 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: <b86e19ed-12af-e488-3c21-002be2ad5914@roeck-us.net>
Date: Thu, 20 Jul 2023 07:00:39 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Marty E. Plummer" <hanetzer@...rtmail.com>,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-doc@...r.kernel.org
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] hwmon: (pmbus) Add driver fpr Infineon IR35201
On 7/20/23 04:58, Marty E. Plummer wrote:
> The IR35201 is a dual-loop digital multi-phase buck controller designed for CPU voltage regulation.
>
> Signed-off-by: Marty E. Plummer <hanetzer@...rtmail.com>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/ir35201.rst | 63 +++++++++++++++++++++++
> drivers/hwmon/pmbus/Kconfig | 9 ++++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/ir35201.c | 89 +++++++++++++++++++++++++++++++++
> 5 files changed, 163 insertions(+)
> create mode 100644 Documentation/hwmon/ir35201.rst
> create mode 100644 drivers/hwmon/pmbus/ir35201.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 042e1cf9501b..5b44a268de0d 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -87,6 +87,7 @@ Hardware Monitoring Kernel Drivers
> ina3221
> inspur-ipsps1
> intel-m10-bmc-hwmon
> + ir35201
> ir35221
> ir38064
> ir36021
> diff --git a/Documentation/hwmon/ir35201.rst b/Documentation/hwmon/ir35201.rst
> new file mode 100644
> index 000000000000..6ca34d4b02a3
> --- /dev/null
> +++ b/Documentation/hwmon/ir35201.rst
> @@ -0,0 +1,63 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ir35201
> +=====================
> +
> +Supported chips:
> +
> + * Infineon IR35201
> +
> + Prefix: ir35201
> + Addresses scanned: -
> +
> + Datasheet: Publicly available at the Infineon website
> + https://www.infineon.com/dgdl/Infineon-IR35201MTRPBF-DS-v01_00-EN.pdf?fileId=5546d462576f347501579c95d19772b5
> +
> +Authors:
> + - Marty E. Plummer <hanetzer@...rtmail.com>
> +
> +Description
> +-----------
> +
> +The IR35201 is a dual-loop digital multi-phase buck controller designed for
> +CPU voltage regulation.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for PMBus devices. You will have to instantiate
> +devices explicitly.
> +
> +Sysfs attributes
> +----------------
> +
> +======================= ===========================
> +curr1_label "iin"
> +curr1_input Measured input current
> +curr1_alarm Input fault alarm
> +
> +curr2_label "iout1"
> +curr2_input Measured output current
> +curr2_alarm Output over-current alarm
> +
> +in1_label "vin"
> +in1_input Measured input voltage
> +in1_alarm Input under-voltage alarm
> +
> +in2_label "vout1"
> +in2_input Measured output voltage
> +in2_alarm Output over-voltage alarm
> +
> +power1_label "pin"
> +power1_input Measured input power
> +power1_alarm Input under-voltage alarm
> +
> +power2_label "pout1"
> +power2_input Measured output power
> +
> +temp1_input Measured temperature
> +temp1_alarm Temperature alarm
> +
> +temp2_input Measured other loop temperature
> +temp2_alarm Temperature alarm
> +======================= ===========================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 270b6336b76d..7180823b15bb 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -123,6 +123,15 @@ config SENSORS_INSPUR_IPSPS
> This driver can also be built as a module. If so, the module will
> be called inspur-ipsps.
>
> +config SENSORS_IR35201
> + tristate "Infineon IR35201"
> + help
> + If you say yes here you get hardware monitoring support for the
> + Infineon IR35201 controller.
> +
> + This driver can also be built as a module. If so, the module will
> + be called ir35201.
> +
> config SENSORS_IR35221
> tristate "Infineon IR35221"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 84ee960a6c2d..40729dd14e7a 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o
> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> +obj-$(CONFIG_SENSORS_IR35201) += ir35201.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> obj-$(CONFIG_SENSORS_IR36021) += ir36021.o
> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
> diff --git a/drivers/hwmon/pmbus/ir35201.c b/drivers/hwmon/pmbus/ir35201.c
> new file mode 100644
> index 000000000000..77f77057175a
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ir35201.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon IR35201
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +static struct pmbus_driver_info ir35201_info = {
> + .pages = 1,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = linear,
> + .format[PSC_CURRENT_IN] = linear,
> + .format[PSC_CURRENT_OUT] = linear,
> + .format[PSC_POWER] = linear,
> + .format[PSC_TEMPERATURE] = linear,
> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT
> + | PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT
> + | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT
> + | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
> + | PMBUS_HAVE_STATUS_TEMP,
Several supported status registers are missing.
> +};
> +
> +static int ir35201_probe(struct i2c_client *client)
> +{
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_BYTE_DATA
> + | I2C_FUNC_SMBUS_READ_WORD_DATA
> + | I2C_FUNC_SMBUS_READ_BLOCK_DATA))
> + return -ENODEV;
> +
> + ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to read PMBUS_MFR_ID\n");
> + return ret;
> + }
> + if (ret != 2 || strncmp(buf, "IR", strlen("IR"))) {
> + dev_err(&client->dev, "MFR_ID unrecognized\n");
> + return -ENODEV;
> + }
> +
Did you actually test this ? Datasheet says it is "ASCII 52 49" which
would make it "RI" like IR35221, not "IR". Problem though is that it
seems like the register is writeable via some USER programming,
making it unreliable.
> + ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to read PMBUS_MFR_MODEL\n");
> + return ret;
> + }
> + if (ret != 1 || buf[0] != 0x50) {
> + dev_err(&client->dev, "MFR_MODEL unrecognized\n");
> + return -ENODEV;
> + }
> +
The datasheet suggests that PMBUS_MFR_ID and PMBUS_MFR_MODEL can differ based
on some USER register programming. I would suggest to read IC_DEVICE_ID instead
and compare against that (which s supposed to be 0x4d).
On a higher level, I don't see anything special in this chip. Would it be possible
to just add it to pmbus.c ? Something like
{"ir35201", (kernel_ulong_t)&pmbus_info_one},
should do.
Thanks,
Guenter
> + return pmbus_do_probe(client, &ir35201_info);
> +}
> +
> +static const struct i2c_device_id ir35201_id[] = {
> + { "ir35201", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, ir35201_id);
> +
> +static const struct of_device_id __maybe_unused ir35201_of_id[] = {
> + { .compatible = "infineon,ir35201" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ir35201_of_id);
> +
> +static struct i2c_driver ir35201_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "ir35201",
> + .of_match_table = of_match_ptr(ir35201_of_id),
> + },
> + .probe_new = ir35201_probe,
> + .id_table = ir35201_id,
> +};
> +
> +module_i2c_driver(ir35201_driver);
> +
> +MODULE_AUTHOR("Marty E. Plummer <hanetzer@...rtmail.com>");
> +MODULE_DESCRIPTION("PMBus driver for Infineon IR35201");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
Powered by blists - more mailing lists