[v1] Add support for HiTRON HAC300S PSU

[PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU

Posted by Vasileios Amoiridis 1 month ago

From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>

Add Support for HiTRON HAC300S PSU. This is a AC/DC hot-swappable
CompactPCI Serial Dual output active current sharing switching power
supply with a 312W rating.

Signed-off-by: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
---
 Documentation/hwmon/hac300s.rst |  37 ++++++++
 MAINTAINERS                     |   7 ++
 drivers/hwmon/pmbus/Kconfig     |   9 ++
 drivers/hwmon/pmbus/Makefile    |   1 +
 drivers/hwmon/pmbus/hac300s.c   | 152 ++++++++++++++++++++++++++++++++
 5 files changed, 206 insertions(+)
 create mode 100644 Documentation/hwmon/hac300s.rst
 create mode 100644 drivers/hwmon/pmbus/hac300s.c

diff --git a/Documentation/hwmon/hac300s.rst b/Documentation/hwmon/hac300s.rst
new file mode 100644
index 000000000000..573269fc81f8
--- /dev/null
+++ b/Documentation/hwmon/hac300s.rst
@@ -0,0 +1,37 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver hac300s
+=====================
+
+Supported chips:
+
+   * HiTRON HAC300S
+
+     Prefix: 'hac300s'
+
+     Datasheet: Publicly available at HiTRON website.
+
+Author:
+
+  - Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
+
+Description
+-----------
+
+This driver implements support for the HiTRON HAC300S PSU. It is a Universal
+AC input harmonic correction AC-DC hot-swappable CompactPCI Serial Dual output
+(with 5V standby) 312 Watts active current sharing switching power supply.
+
+The device has an input of 90-264VAC and 2 nominal output voltaged at 12V and
+5V which they can supplu up to 25A and 2.5A respectively.
+
+Sysfs entries
+-------------
+
+======= ==========================================
+curr1   Output current
+in1     Output voltage
+power1  Output power
+temp1   Ambient temperature inside the module
+temp2   Internal secondary component's temperature
+======= ==========================================
diff --git a/MAINTAINERS b/MAINTAINERS
index a0dd762f5648..feb8ec4d9b17 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11254,6 +11254,13 @@ F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
 F:	tools/testing/selftests/timers/
 
+HITRON HAC300S PSU DRIVER
+M:	Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/hac300s.rst
+F:	drivers/hwmon/pmbus/hac300s.c
+
 DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
 M:	Andreas Hindborg <a.hindborg@kernel.org>
 R:	Boqun Feng <boqun.feng@gmail.com>
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index f3fb94cebf1a..4c2cb51dbe3f 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -124,6 +124,15 @@ config SENSORS_FSP_3Y
 	  This driver can also be built as a module. If so, the module will
 	  be called fsp-3y.
 
+config SENSORS_HAC300S
+	tristate "Hitron HAC300S-D120E PSU"
+	help
+	  If you say yes here you get hardware monitoring support for the
+	  Hitron HAC300S-D120E Power Supply.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called hac300s.
+
 config SENSORS_IBM_CFFPS
 	tristate "IBM Common Form Factor Power Supply"
 	depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 349a89b6d92e..b92309019d35 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
 obj-$(CONFIG_SENSORS_BPA_RS600)	+= bpa-rs600.o
 obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
 obj-$(CONFIG_SENSORS_FSP_3Y)	+= fsp-3y.o
+obj-$(CONFIG_SENSORS_HAC300S)	+= hac300s.o
 obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
 obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
 obj-$(CONFIG_SENSORS_INA233)	+= ina233.o
diff --git a/drivers/hwmon/pmbus/hac300s.c b/drivers/hwmon/pmbus/hac300s.c
new file mode 100644
index 000000000000..a1640449e5f5
--- /dev/null
+++ b/drivers/hwmon/pmbus/hac300s.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2024 CERN (home.cern)
+/*
+ * Hardware monitoring driver for Hi-Tron HAC300S PSU.
+ *
+ * NOTE: The HAC300S device does not support the PMBUS_VOUT_MODE register.
+ * On top of that, it returns the Voltage output values in Linear11 which is
+ * not adhering to the PMBus specifications. (PMBus Specification Part II,
+ * Section 7.1-7.3). For that reason the PMBUS_VOUT_MODE register is being faked
+ * and returns the exponent value of the READ_VOUT register. The exponent part
+ * of the VOUT_* registers is being cleared in order to return the mantissa to
+ * the pmbus core.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+
+#include "pmbus.h"
+
+#define LINEAR11_EXPONENT_MASK GENMASK(15, 11)
+#define LINEAR11_MANTISSA_MASK GENMASK(10, 0)
+
+#define to_hac300s_data(x) container_of(x, struct hac300s_data, info)
+
+struct hac300s_data {
+	struct pmbus_driver_info info;
+	bool vout_linear11;
+	s8 exponent;
+};
+
+static int hac300s_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	struct hac300s_data *data = to_hac300s_data(info);
+
+	if (reg == PMBUS_VOUT_MODE && data->vout_linear11)
+		return data->exponent;
+
+	return pmbus_read_byte_data(client, page, reg);
+}
+
+static int hac300s_read_word_data(struct i2c_client *client, int page,
+				   int phase, int reg)
+{
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	struct hac300s_data *data = to_hac300s_data(info);
+	int rv;
+
+	rv = pmbus_read_word_data(client, page, phase, reg);
+	if (rv < 0)
+		return rv;
+
+	switch (reg) {
+	case PMBUS_VIRT_READ_IOUT_AVG:
+	case PMBUS_VIRT_READ_POUT_AVG:
+	case PMBUS_VIRT_READ_TEMP_AVG:
+		return -ENXIO;
+	case PMBUS_VOUT_OV_WARN_LIMIT:
+	case PMBUS_VOUT_UV_WARN_LIMIT:
+	case PMBUS_VOUT_OV_FAULT_LIMIT:
+	case PMBUS_VOUT_UV_FAULT_LIMIT:
+	case PMBUS_MFR_VOUT_MAX:
+	case PMBUS_MFR_VOUT_MIN:
+	case PMBUS_READ_VOUT:
+		if (data->vout_linear11)
+			return FIELD_GET(LINEAR11_MANTISSA_MASK, rv);
+		fallthrough;
+	default:
+		return rv;
+	}
+}
+
+#define HAC300S_SW_FUNC (PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | \
+			 PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
+			 PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
+			 PMBUS_HAVE_POUT | PMBUS_HAVE_STATUS_TEMP)
+
+static struct pmbus_driver_info hac300s_info = {
+	.pages = 1,
+	.func[0] = HAC300S_SW_FUNC,
+	.read_byte_data = hac300s_read_byte_data,
+	.read_word_data = hac300s_read_word_data,
+	.format[PSC_VOLTAGE_OUT] = linear,
+};
+
+static struct pmbus_platform_data hac300s_pdata = {
+	.flags = PMBUS_NO_CAPABILITY,
+};
+
+static int hac300s_probe(struct i2c_client *client)
+{
+	struct hac300s_data *data;
+	int rv;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct hac300s_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
+				     I2C_FUNC_SMBUS_READ_WORD_DATA))
+		return -ENODEV;
+
+	rv = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
+	if (rv < 0) {
+		data->vout_linear11 = true;
+		/* LINEAR11 format, use exponent from READ_VOUT register */
+		rv = i2c_smbus_read_word_data(client, PMBUS_READ_VOUT);
+		if (rv < 0)
+			return dev_err_probe(&client->dev, rv, "Failed to read vout_mode\n");
+
+		data->exponent = FIELD_GET(LINEAR11_EXPONENT_MASK, rv);
+	}
+
+	data->info = hac300s_info;
+	client->dev.platform_data = &hac300s_pdata;
+	return pmbus_do_probe(client, &data->info);
+}
+
+static const struct of_device_id hac300s_of_match[] = {
+	{ .compatible = "hitron,hac300s" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, hac300s_of_match);
+
+static const struct i2c_device_id hac300s_id[] = {
+	{"hac300s", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, hac300s_id);
+
+static struct i2c_driver hac300s_driver = {
+	.driver = {
+		   .name = "hac300s",
+		   .of_match_table = hac300s_of_match,
+	},
+	.probe = hac300s_probe,
+	.id_table = hac300s_id,
+
+};
+module_i2c_driver(hac300s_driver);
+
+MODULE_AUTHOR("Vasileios Amoiridis");
+MODULE_DESCRIPTION("PMBus driver for Hi-Tron HAC300S PSU");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");
-- 
2.47.3

Re: [PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU

Posted by Guenter Roeck 3 weeks, 6 days ago

On 1/6/26 08:03, Vasileios Amoiridis wrote:
> From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> 
> Add Support for HiTRON HAC300S PSU. This is a AC/DC hot-swappable
> CompactPCI Serial Dual output active current sharing switching power
> supply with a 312W rating.
> 
> Signed-off-by: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> ---
>   Documentation/hwmon/hac300s.rst |  37 ++++++++

Needs to be added to index.rst.

>   MAINTAINERS                     |   7 ++
>   drivers/hwmon/pmbus/Kconfig     |   9 ++
>   drivers/hwmon/pmbus/Makefile    |   1 +
>   drivers/hwmon/pmbus/hac300s.c   | 152 ++++++++++++++++++++++++++++++++
>   5 files changed, 206 insertions(+)
>   create mode 100644 Documentation/hwmon/hac300s.rst
>   create mode 100644 drivers/hwmon/pmbus/hac300s.c
> 
> diff --git a/Documentation/hwmon/hac300s.rst b/Documentation/hwmon/hac300s.rst
> new file mode 100644
> index 000000000000..573269fc81f8
> --- /dev/null
> +++ b/Documentation/hwmon/hac300s.rst
> @@ -0,0 +1,37 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver hac300s
> +=====================
> +
> +Supported chips:
> +
> +   * HiTRON HAC300S
> +
> +     Prefix: 'hac300s'
> +
> +     Datasheet: Publicly available at HiTRON website.
> +
> +Author:
> +
> +  - Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> +
> +Description
> +-----------
> +
> +This driver implements support for the HiTRON HAC300S PSU. It is a Universal

s/implements support/supports/

> +AC input harmonic correction AC-DC hot-swappable CompactPCI Serial Dual output
> +(with 5V standby) 312 Watts active current sharing switching power supply.
> +
> +The device has an input of 90-264VAC and 2 nominal output voltaged at 12V and
> +5V which they can supplu up to 25A and 2.5A respectively.
> +
> +Sysfs entries
> +-------------
> +
> +======= ==========================================
> +curr1   Output current
> +in1     Output voltage
> +power1  Output power
> +temp1   Ambient temperature inside the module
> +temp2   Internal secondary component's temperature
> +======= ==========================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a0dd762f5648..feb8ec4d9b17 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11254,6 +11254,13 @@ F:	kernel/time/timer_list.c
>   F:	kernel/time/timer_migration.*
>   F:	tools/testing/selftests/timers/
>   
> +HITRON HAC300S PSU DRIVER
> +M:	Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/hac300s.rst
> +F:	drivers/hwmon/pmbus/hac300s.c
> +
>   DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
>   M:	Andreas Hindborg <a.hindborg@kernel.org>
>   R:	Boqun Feng <boqun.feng@gmail.com>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index f3fb94cebf1a..4c2cb51dbe3f 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -124,6 +124,15 @@ config SENSORS_FSP_3Y
>   	  This driver can also be built as a module. If so, the module will
>   	  be called fsp-3y.
>   
> +config SENSORS_HAC300S
> +	tristate "Hitron HAC300S-D120E PSU"
> +	help
> +	  If you say yes here you get hardware monitoring support for the
> +	  Hitron HAC300S-D120E Power Supply.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called hac300s.
> +
>   config SENSORS_IBM_CFFPS
>   	tristate "IBM Common Form Factor Power Supply"
>   	depends on LEDS_CLASS
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 349a89b6d92e..b92309019d35 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
>   obj-$(CONFIG_SENSORS_BPA_RS600)	+= bpa-rs600.o
>   obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
>   obj-$(CONFIG_SENSORS_FSP_3Y)	+= fsp-3y.o
> +obj-$(CONFIG_SENSORS_HAC300S)	+= hac300s.o
>   obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
>   obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
>   obj-$(CONFIG_SENSORS_INA233)	+= ina233.o
> diff --git a/drivers/hwmon/pmbus/hac300s.c b/drivers/hwmon/pmbus/hac300s.c
> new file mode 100644
> index 000000000000..a1640449e5f5
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/hac300s.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// SPDX-FileCopyrightText: 2024 CERN (home.cern)
> +/*
> + * Hardware monitoring driver for Hi-Tron HAC300S PSU.
> + *
> + * NOTE: The HAC300S device does not support the PMBUS_VOUT_MODE register.
> + * On top of that, it returns the Voltage output values in Linear11 which is
> + * not adhering to the PMBus specifications. (PMBus Specification Part II,
> + * Section 7.1-7.3). For that reason the PMBUS_VOUT_MODE register is being faked
> + * and returns the exponent value of the READ_VOUT register. The exponent part
> + * of the VOUT_* registers is being cleared in order to return the mantissa to
> + * the pmbus core.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +
> +#include "pmbus.h"
> +
> +#define LINEAR11_EXPONENT_MASK GENMASK(15, 11)
> +#define LINEAR11_MANTISSA_MASK GENMASK(10, 0)
> +
> +#define to_hac300s_data(x) container_of(x, struct hac300s_data, info)
> +
> +struct hac300s_data {
> +	struct pmbus_driver_info info;
> +	bool vout_linear11;
> +	s8 exponent;
> +};
> +
> +static int hac300s_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct hac300s_data *data = to_hac300s_data(info);
> +
> +	if (reg == PMBUS_VOUT_MODE && data->vout_linear11)
> +		return data->exponent;
> +
> +	return pmbus_read_byte_data(client, page, reg);
> +}
> +
> +static int hac300s_read_word_data(struct i2c_client *client, int page,
> +				   int phase, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct hac300s_data *data = to_hac300s_data(info);
> +	int rv;
> +
> +	rv = pmbus_read_word_data(client, page, phase, reg);
> +	if (rv < 0)
> +		return rv;
> +
> +	switch (reg) {
> +	case PMBUS_VIRT_READ_IOUT_AVG:
> +	case PMBUS_VIRT_READ_POUT_AVG:
> +	case PMBUS_VIRT_READ_TEMP_AVG:
> +		return -ENXIO;
> +	case PMBUS_VOUT_OV_WARN_LIMIT:
> +	case PMBUS_VOUT_UV_WARN_LIMIT:
> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> +	case PMBUS_MFR_VOUT_MAX:
> +	case PMBUS_MFR_VOUT_MIN:
> +	case PMBUS_READ_VOUT:
> +		if (data->vout_linear11)
> +			return FIELD_GET(LINEAR11_MANTISSA_MASK, rv);

Is it guaranteed that the exponent is always the same ? Because if not the
conversion will have to be explicit.

> +		fallthrough;
> +	default:
> +		return rv;

This is wrong. The register should only be read by affected commands, and
the function should return -ENODATA for the others.

> +	}
> +}
> +
> +#define HAC300S_SW_FUNC (PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | \
> +			 PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
> +			 PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
> +			 PMBUS_HAVE_POUT | PMBUS_HAVE_STATUS_TEMP)
> +
Unnecessary define since it is only used once. Please fold into the declaration
below.

> +static struct pmbus_driver_info hac300s_info = {
> +	.pages = 1,
> +	.func[0] = HAC300S_SW_FUNC,
> +	.read_byte_data = hac300s_read_byte_data,
> +	.read_word_data = hac300s_read_word_data,
> +	.format[PSC_VOLTAGE_OUT] = linear,
> +};
> +
> +static struct pmbus_platform_data hac300s_pdata = {
> +	.flags = PMBUS_NO_CAPABILITY,
> +};
> +
> +static int hac300s_probe(struct i2c_client *client)
> +{
> +	struct hac300s_data *data;
> +	int rv;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct hac300s_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA))
> +		return -ENODEV;
> +
> +	rv = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
> +	if (rv < 0) {

This needs explanation. Why try to read PMBUS_VOUT_MODE if the failure is
expected ? Are there variants of the PSU which return something useful here ?
The note above does not explain the reason for this conditional.

If this is supposed to check if this is really the expected device,
read and verify PMBUS_MFR_ID instead.

Thanks,
Guenter

> +		data->vout_linear11 = true;
> +		/* LINEAR11 format, use exponent from READ_VOUT register */
> +		rv = i2c_smbus_read_word_data(client, PMBUS_READ_VOUT);
> +		if (rv < 0)
> +			return dev_err_probe(&client->dev, rv, "Failed to read vout_mode\n");
> +
> +		data->exponent = FIELD_GET(LINEAR11_EXPONENT_MASK, rv);
> +	}
> +
> +	data->info = hac300s_info;
> +	client->dev.platform_data = &hac300s_pdata;
> +	return pmbus_do_probe(client, &data->info);
> +}
> +
> +static const struct of_device_id hac300s_of_match[] = {
> +	{ .compatible = "hitron,hac300s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, hac300s_of_match);
> +
> +static const struct i2c_device_id hac300s_id[] = {
> +	{"hac300s", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, hac300s_id);
> +
> +static struct i2c_driver hac300s_driver = {
> +	.driver = {
> +		   .name = "hac300s",
> +		   .of_match_table = hac300s_of_match,
> +	},
> +	.probe = hac300s_probe,
> +	.id_table = hac300s_id,
> +
> +};
> +module_i2c_driver(hac300s_driver);
> +
> +MODULE_AUTHOR("Vasileios Amoiridis");
> +MODULE_DESCRIPTION("PMBus driver for Hi-Tron HAC300S PSU");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");

Re: [PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU

Posted by Vasileios Amoiridis 3 weeks, 3 days ago

On Mon, Jan 12, 2026 at 07:26:49AM -0800, Guenter Roeck wrote:
> On 1/6/26 08:03, Vasileios Amoiridis wrote:
> > From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > 
> > Add Support for HiTRON HAC300S PSU. This is a AC/DC hot-swappable
> > CompactPCI Serial Dual output active current sharing switching power
> > supply with a 312W rating.
> > 
> > Signed-off-by: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > ---
> >   Documentation/hwmon/hac300s.rst |  37 ++++++++
> 
> Needs to be added to index.rst.
> 

ACK

> >   MAINTAINERS                     |   7 ++
> >   drivers/hwmon/pmbus/Kconfig     |   9 ++
> >   drivers/hwmon/pmbus/Makefile    |   1 +
> >   drivers/hwmon/pmbus/hac300s.c   | 152 ++++++++++++++++++++++++++++++++
> >   5 files changed, 206 insertions(+)
> >   create mode 100644 Documentation/hwmon/hac300s.rst
> >   create mode 100644 drivers/hwmon/pmbus/hac300s.c
> > 
> > diff --git a/Documentation/hwmon/hac300s.rst b/Documentation/hwmon/hac300s.rst
> > new file mode 100644
> > index 000000000000..573269fc81f8
> > --- /dev/null
> > +++ b/Documentation/hwmon/hac300s.rst
> > @@ -0,0 +1,37 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver hac300s
> > +=====================
> > +
> > +Supported chips:
> > +
> > +   * HiTRON HAC300S
> > +
> > +     Prefix: 'hac300s'
> > +
> > +     Datasheet: Publicly available at HiTRON website.
> > +
> > +Author:
> > +
> > +  - Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for the HiTRON HAC300S PSU. It is a Universal
> 
> s/implements support/supports/
> 

ACK

> > +AC input harmonic correction AC-DC hot-swappable CompactPCI Serial Dual output
> > +(with 5V standby) 312 Watts active current sharing switching power supply.
> > +
> > +The device has an input of 90-264VAC and 2 nominal output voltaged at 12V and
> > +5V which they can supplu up to 25A and 2.5A respectively.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +======= ==========================================
> > +curr1   Output current
> > +in1     Output voltage
> > +power1  Output power
> > +temp1   Ambient temperature inside the module
> > +temp2   Internal secondary component's temperature
> > +======= ==========================================
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0dd762f5648..feb8ec4d9b17 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11254,6 +11254,13 @@ F:	kernel/time/timer_list.c
> >   F:	kernel/time/timer_migration.*
> >   F:	tools/testing/selftests/timers/
> > +HITRON HAC300S PSU DRIVER
> > +M:	Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/hwmon/hac300s.rst
> > +F:	drivers/hwmon/pmbus/hac300s.c
> > +
> >   DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
> >   M:	Andreas Hindborg <a.hindborg@kernel.org>
> >   R:	Boqun Feng <boqun.feng@gmail.com>
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index f3fb94cebf1a..4c2cb51dbe3f 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -124,6 +124,15 @@ config SENSORS_FSP_3Y
> >   	  This driver can also be built as a module. If so, the module will
> >   	  be called fsp-3y.
> > +config SENSORS_HAC300S
> > +	tristate "Hitron HAC300S-D120E PSU"
> > +	help
> > +	  If you say yes here you get hardware monitoring support for the
> > +	  Hitron HAC300S-D120E Power Supply.
> > +
> > +	  This driver can also be built as a module. If so, the module will
> > +	  be called hac300s.
> > +
> >   config SENSORS_IBM_CFFPS
> >   	tristate "IBM Common Form Factor Power Supply"
> >   	depends on LEDS_CLASS
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index 349a89b6d92e..b92309019d35 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
> >   obj-$(CONFIG_SENSORS_BPA_RS600)	+= bpa-rs600.o
> >   obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
> >   obj-$(CONFIG_SENSORS_FSP_3Y)	+= fsp-3y.o
> > +obj-$(CONFIG_SENSORS_HAC300S)	+= hac300s.o
> >   obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
> >   obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
> >   obj-$(CONFIG_SENSORS_INA233)	+= ina233.o
> > diff --git a/drivers/hwmon/pmbus/hac300s.c b/drivers/hwmon/pmbus/hac300s.c
> > new file mode 100644
> > index 000000000000..a1640449e5f5
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/hac300s.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// SPDX-FileCopyrightText: 2024 CERN (home.cern)
> > +/*
> > + * Hardware monitoring driver for Hi-Tron HAC300S PSU.
> > + *
> > + * NOTE: The HAC300S device does not support the PMBUS_VOUT_MODE register.
> > + * On top of that, it returns the Voltage output values in Linear11 which is
> > + * not adhering to the PMBus specifications. (PMBus Specification Part II,
> > + * Section 7.1-7.3). For that reason the PMBUS_VOUT_MODE register is being faked
> > + * and returns the exponent value of the READ_VOUT register. The exponent part
> > + * of the VOUT_* registers is being cleared in order to return the mantissa to
> > + * the pmbus core.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pmbus.h>
> > +
> > +#include "pmbus.h"
> > +
> > +#define LINEAR11_EXPONENT_MASK GENMASK(15, 11)
> > +#define LINEAR11_MANTISSA_MASK GENMASK(10, 0)
> > +
> > +#define to_hac300s_data(x) container_of(x, struct hac300s_data, info)
> > +
> > +struct hac300s_data {
> > +	struct pmbus_driver_info info;
> > +	bool vout_linear11;
> > +	s8 exponent;
> > +};
> > +
> > +static int hac300s_read_byte_data(struct i2c_client *client, int page, int reg)
> > +{
> > +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> > +	struct hac300s_data *data = to_hac300s_data(info);
> > +
> > +	if (reg == PMBUS_VOUT_MODE && data->vout_linear11)
> > +		return data->exponent;
> > +
> > +	return pmbus_read_byte_data(client, page, reg);
> > +}
> > +
> > +static int hac300s_read_word_data(struct i2c_client *client, int page,
> > +				   int phase, int reg)
> > +{
> > +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> > +	struct hac300s_data *data = to_hac300s_data(info);
> > +	int rv;
> > +
> > +	rv = pmbus_read_word_data(client, page, phase, reg);
> > +	if (rv < 0)
> > +		return rv;
> > +
> > +	switch (reg) {
> > +	case PMBUS_VIRT_READ_IOUT_AVG:
> > +	case PMBUS_VIRT_READ_POUT_AVG:
> > +	case PMBUS_VIRT_READ_TEMP_AVG:
> > +		return -ENXIO;
> > +	case PMBUS_VOUT_OV_WARN_LIMIT:
> > +	case PMBUS_VOUT_UV_WARN_LIMIT:
> > +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> > +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> > +	case PMBUS_MFR_VOUT_MAX:
> > +	case PMBUS_MFR_VOUT_MIN:
> > +	case PMBUS_READ_VOUT:
> > +		if (data->vout_linear11)
> > +			return FIELD_GET(LINEAR11_MANTISSA_MASK, rv);
> 
> Is it guaranteed that the exponent is always the same ? Because if not the
> conversion will have to be explicit.
> 

Yes, after a lot of testing with different values, it remained the same.

> > +		fallthrough;
> > +	default:
> > +		return rv;
> 
> This is wrong. The register should only be read by affected commands, and
> the function should return -ENODATA for the others.
> 

ACK. I hadn't understood that this was the usecase of this function.

> > +	}
> > +}
> > +
> > +#define HAC300S_SW_FUNC (PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | \
> > +			 PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
> > +			 PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
> > +			 PMBUS_HAVE_POUT | PMBUS_HAVE_STATUS_TEMP)
> > +
> Unnecessary define since it is only used once. Please fold into the declaration
> below.
> 

ACK.

> > +static struct pmbus_driver_info hac300s_info = {
> > +	.pages = 1,
> > +	.func[0] = HAC300S_SW_FUNC,
> > +	.read_byte_data = hac300s_read_byte_data,
> > +	.read_word_data = hac300s_read_word_data,
> > +	.format[PSC_VOLTAGE_OUT] = linear,
> > +};
> > +
> > +static struct pmbus_platform_data hac300s_pdata = {
> > +	.flags = PMBUS_NO_CAPABILITY,
> > +};
> > +
> > +static int hac300s_probe(struct i2c_client *client)
> > +{
> > +	struct hac300s_data *data;
> > +	int rv;
> > +
> > +	data = devm_kzalloc(&client->dev, sizeof(struct hac300s_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > +				     I2C_FUNC_SMBUS_READ_WORD_DATA))
> > +		return -ENODEV;
> > +
> > +	rv = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
> > +	if (rv < 0) {
> 
> This needs explanation. Why try to read PMBUS_VOUT_MODE if the failure is
> expected ? Are there variants of the PSU which return something useful here ?
> The note above does not explain the reason for this conditional.
> 
> If this is supposed to check if this is really the expected device,
> read and verify PMBUS_MFR_ID instead.
> 
> Thanks,
> Guenter
> 

That was just some sanity check, it can indeed be removed. 

Cheers,
Vasilis

> > +		data->vout_linear11 = true;
> > +		/* LINEAR11 format, use exponent from READ_VOUT register */
> > +		rv = i2c_smbus_read_word_data(client, PMBUS_READ_VOUT);
> > +		if (rv < 0)
> > +			return dev_err_probe(&client->dev, rv, "Failed to read vout_mode\n");
> > +
> > +		data->exponent = FIELD_GET(LINEAR11_EXPONENT_MASK, rv);
> > +	}
> > +
> > +	data->info = hac300s_info;
> > +	client->dev.platform_data = &hac300s_pdata;
> > +	return pmbus_do_probe(client, &data->info);
> > +}
> > +
> > +static const struct of_device_id hac300s_of_match[] = {
> > +	{ .compatible = "hitron,hac300s" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, hac300s_of_match);
> > +
> > +static const struct i2c_device_id hac300s_id[] = {
> > +	{"hac300s", 0},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, hac300s_id);
> > +
> > +static struct i2c_driver hac300s_driver = {
> > +	.driver = {
> > +		   .name = "hac300s",
> > +		   .of_match_table = hac300s_of_match,
> > +	},
> > +	.probe = hac300s_probe,
> > +	.id_table = hac300s_id,
> > +
> > +};
> > +module_i2c_driver(hac300s_driver);
> > +
> > +MODULE_AUTHOR("Vasileios Amoiridis");
> > +MODULE_DESCRIPTION("PMBus driver for Hi-Tron HAC300S PSU");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("PMBUS");
>

Re: [PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU

Posted by kernel test robot 1 month ago

Hi Vasileios,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on groeck-staging/hwmon-next linus/master v6.19-rc4 next-20260107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Vasileios-Amoiridis/dt-bindings-trivial-devices-Add-hitron-hac300s/20260107-000618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20260106160353.14023-3-vassilisamir%40gmail.com
patch subject: [PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU
reproduce: (https://download.01.org/0day-ci/archive/20260107/202601071730.jz3sGdba-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601071730.jz3sGdba-lkp@intel.com/

All warnings (new ones prefixed by >>):

   ERROR: Cannot find file ./include/linux/mutex.h
   ERROR: Cannot find file ./include/linux/mutex.h
   WARNING: No kernel-doc for file ./include/linux/mutex.h
   ERROR: Cannot find file ./include/linux/fwctl.h
   WARNING: No kernel-doc for file ./include/linux/fwctl.h
>> Documentation/hwmon/hac300s.rst: WARNING: document isn't included in any toctree [toc.not_included]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v1 1/2] dt-bindings: trivial-devices: Add hitron,hac300s
[PATCH v1 2/2] hwmon: Add support for HiTRON HAC300S PSU