diff mbox series

hwmon: (pmbus) Add driver fpr Infineon IR35201

Message ID 20230720115805.1510279-1-hanetzer@startmail.com (mailing list archive)
State Changes Requested
Headers show
Series hwmon: (pmbus) Add driver fpr Infineon IR35201 | expand

Commit Message

Marty E. Plummer July 20, 2023, 11:58 a.m. UTC
The IR35201 is a dual-loop digital multi-phase buck controller designed for CPU voltage regulation.

Signed-off-by: Marty E. Plummer <hanetzer@startmail.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

Comments

Guenter Roeck July 20, 2023, 2 p.m. UTC | #1
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@startmail.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@startmail.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@startmail.com>");
> +MODULE_DESCRIPTION("PMBus driver for Infineon IR35201");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
Marty E. Plummer July 20, 2023, 6:35 p.m. UTC | #2
On Thu, Jul 20, 2023 at 07:00:39AM -0700, Guenter Roeck wrote:
>
Maybe.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@startmail.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@startmail.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.
> 
Maybe. Did the best I could with this and another datasheet (ir36* iirc)
open at the same time and both source files open for comparison, and the
output from sensors with this patch, with allowances for variations
in temps, matches more or less what HWINFO64 outputs on a windows pe
based build of hiren's boot cd.
> > +};
> > +
> > +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.
> 
Yes, I did. And strangely enough, it reads 'backwards' or so, relative
to the 35221. I almost sent this along without removing the debugging
pr_infos I had in this area to check that. drove me bonkers a bit.
> > +	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).
> 
Somehow I missed that, but it was on my 'to-check' list. I think the
issue may have arose from my datasheet comparison, as the ir36* doesn't
have such a register listed.
> 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},
> 
Honestly, I was wondering about folding this and the other very similar
IR3* chips into one driver. Should be doable? But I guess this approach
works as well; in fact, during my investigation phase I stuck the pmbus
driver onto the correct i2c address to get an easy way to read stuff
from the chip (tbh I'm surprised that this far along in linux we don't
have anything other than pmbus_peek to poke for info; maybe i2c-tools
can do it but I can't seem to make it work like I'd expect).
> 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@startmail.com>");
> > +MODULE_DESCRIPTION("PMBus driver for Infineon IR35201");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(PMBUS);
>
Guenter Roeck July 20, 2023, 7:22 p.m. UTC | #3
On 7/20/23 11:35, Marty E. Plummer wrote:
> On Thu, Jul 20, 2023 at 07:00:39AM -0700, Guenter Roeck wrote:
>>
> Maybe.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@startmail.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@startmail.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.
>>
> Maybe. Did the best I could with this and another datasheet (ir36* iirc)
> open at the same time and both source files open for comparison, and the
> output from sensors with this patch, with allowances for variations
> in temps, matches more or less what HWINFO64 outputs on a windows pe
> based build of hiren's boot cd.

STATUS_INPUT, STATUS_IOUT, and STATUS_VOUT are supported according
to the datasheet. Do you have reason to believe that this is incorrect ?
If so, I would want to see a comment in the driver explaining that the
datasheet is wrong and doesn't support those registers.

>>> +};
>>> +
>>> +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.
>>
> Yes, I did. And strangely enough, it reads 'backwards' or so, relative
> to the 35221. I almost sent this along without removing the debugging
> pr_infos I had in this area to check that. drove me bonkers a bit.
>>> +	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).
>>
> Somehow I missed that, but it was on my 'to-check' list. I think the
> issue may have arose from my datasheet comparison, as the ir36* doesn't
> have such a register listed.

Different series, probably different microcontroller and different microcode.

>> 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},
>>
> Honestly, I was wondering about folding this and the other very similar
> IR3* chips into one driver. Should be doable? But I guess this approach
> works as well; in fact, during my investigation phase I stuck the pmbus
> driver onto the correct i2c address to get an easy way to read stuff
> from the chip (tbh I'm surprised that this far along in linux we don't
> have anything other than pmbus_peek to poke for info; maybe i2c-tools
> can do it but I can't seem to make it work like I'd expect).

This isn't about "doable". We don't want to add new drivers just for fun
and/or "because it works as well". A new driver should only be added if
needed. It was done, for example, for IR35221 because that chip has
non-standard registers which we want to have supported.

Unless I am missing something, IR35201 only supports standard commands.
So the question is: Is the new driver really needed ? It appears you are
saying that, no, it isn't needed. Add the chip to pmbus.c as I suggested above.
If and only if that doesn't work we can talk about a new driver.

Thanks,
Guenter
Marty E. Plummer July 23, 2023, 10:07 p.m. UTC | #4
On Thu, Jul 20, 2023 at 12:22:47PM -0700, Guenter Roeck wrote:
> On 7/20/23 11:35, Marty E. Plummer wrote:
> > On Thu, Jul 20, 2023 at 07:00:39AM -0700, Guenter Roeck wrote:
> > > 
> > Maybe.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@startmail.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@startmail.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.
> > > 
> > Maybe. Did the best I could with this and another datasheet (ir36* iirc)
> > open at the same time and both source files open for comparison, and the
> > output from sensors with this patch, with allowances for variations
> > in temps, matches more or less what HWINFO64 outputs on a windows pe
> > based build of hiren's boot cd.
> 
> STATUS_INPUT, STATUS_IOUT, and STATUS_VOUT are supported according
> to the datasheet. Do you have reason to believe that this is incorrect ?
> If so, I would want to see a comment in the driver explaining that the
> datasheet is wrong and doesn't support those registers.
> 
> > > > +};
> > > > +
> > > > +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.
> > > 
> > Yes, I did. And strangely enough, it reads 'backwards' or so, relative
> > to the 35221. I almost sent this along without removing the debugging
> > pr_infos I had in this area to check that. drove me bonkers a bit.
> > > > +	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).
> > > 
> > Somehow I missed that, but it was on my 'to-check' list. I think the
> > issue may have arose from my datasheet comparison, as the ir36* doesn't
> > have such a register listed.
> 
> Different series, probably different microcontroller and different microcode.
> 
> > > 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},
> > > 
> > Honestly, I was wondering about folding this and the other very similar
> > IR3* chips into one driver. Should be doable? But I guess this approach
> > works as well; in fact, during my investigation phase I stuck the pmbus
> > driver onto the correct i2c address to get an easy way to read stuff
> > from the chip (tbh I'm surprised that this far along in linux we don't
> > have anything other than pmbus_peek to poke for info; maybe i2c-tools
> > can do it but I can't seem to make it work like I'd expect).
> 
> This isn't about "doable". We don't want to add new drivers just for fun
> and/or "because it works as well". A new driver should only be added if
> needed. It was done, for example, for IR35221 because that chip has
> non-standard registers which we want to have supported.
> 
> Unless I am missing something, IR35201 only supports standard commands.
> So the question is: Is the new driver really needed ? It appears you are
> saying that, no, it isn't needed. Add the chip to pmbus.c as I suggested above.
> If and only if that doesn't work we can talk about a new driver.
> 
Binding the pmbus subdriver (I guess you could call it that) for the
"adp4000", which shares the same pmbus_device_info struct you suggested
results in sensible output in lm_sensors. I'll whip up a new patch and
send it in.
> Thanks,
> Guenter
>
diff mbox series

Patch

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@startmail.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,
+};
+
+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;
+	}
+
+	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;
+	}
+
+	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@startmail.com>");
+MODULE_DESCRIPTION("PMBus driver for Infineon IR35201");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);