Re: [PATCH] hwmon: (pmbus) add ir35215 driver

[Guenter Roeck <linux@roeck-us.net>]

On 4/27/23 12:35, Scott Smith wrote:
> IR35215 is a digital multi-phase controller.
> 
> Signed-off-by: Scott Smith <scott8440@gmail.com>

Does this chip really need its own driver ? Please check if it can be added
to pmbus.c instead. If not, please provide a rationale explaining why
that does not work. Also, if you have access to the datasheet, please
make sure that the chip is sufficiently different from IR35221 to warrant
a separate driver.

Additional comments inline.

Thanks,
Guenter

> ---
>   drivers/hwmon/pmbus/Kconfig   |  9 +++++
>   drivers/hwmon/pmbus/Makefile  |  1 +
>   drivers/hwmon/pmbus/ir35215.c | 65 +++++++++++++++++++++++++++++++++++

Documentation/hwmon/ir35215.rst needed (if this driver is needed).

>   3 files changed, 75 insertions(+)
>   create mode 100644 drivers/hwmon/pmbus/ir35215.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 270b6336b76d..5c089f7e4423 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig1 
> @@ -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_IR35215
> +       tristate "Infineon IR35215"
> +       help
> +         If you say yes here you get hardware monitoring support for the
> +         Infineon IR35215 controller.
> +
> +         This driver can also be built as a module. If so, the module will
> +         be called ir35215.
> +
>   config SENSORS_IR35221
>   	tristate "Infineon IR35221"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 84ee960a6c2d..fbb9cf048326 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_IR35215)   += ir35215.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/ir35215.c b/drivers/hwmon/pmbus/ir35215.c
> new file mode 100644
> index 000000000000..92d59e78bfd0
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ir35215.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon IR35215
> + *
> + * Copyright (c) Meta Platforms, Inc. and affiliates.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon-sysfs.h>

Not needed.

> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +static const u32 functionality = PMBUS_HAVE_TEMP
> +	| PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT
> +	| PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT
> +	| PMBUS_HAVE_PIN | PMBUS_HAVE_POUT
> +	| PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT
> +	| PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP;
> +
> +static struct pmbus_driver_info ir35215_info = {
> +	.pages = 2,
> +	.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] = functionality,
> +	.func[1] = functionality,
> +};
> +
> +static int ir35215_probe(struct i2c_client *client)
> +{
> +	/*
> +	 * IR35215 devices may not stay in page 0 during device
> +	 * probe which leads to probe failure (read status word failed).
> +	 * So let's set the device to page 0 at the beginning.
> +	 */
> +	i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);

That should probably be added to the pmbus core code since
it potentially affects all PMBus chips.

> +	return pmbus_do_probe(client, &ir35215_info);
> +}
> +
> +static const struct i2c_device_id ir35215_id[] = {
> +	{ "ir35215", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ir35215_id);
> +
> +static struct i2c_driver ir35215_driver = {
> +	.driver = {
> +		   .name = "ir35215",
> +	},
> +	.probe_new = ir35215_probe,
> +	.id_table = ir35215_id,
> +};
> +
> +module_i2c_driver(ir35215_driver);
> +
> +MODULE_AUTHOR("Tao Ren <rentao.bupt@gmail.com>");
> +MODULE_DESCRIPTION("PMBus driver for Infineon IR35215");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);