Re: [PATCH v3 2/2] hwmon: pmbus: Add support for Sony APS-379

[Guenter Roeck]

Hi Chris,

On 4/1/26 12:42, Chris Packham wrote:
> Add pmbus support for Sony APS-379 power supplies. There are a few PMBUS
> commands that return data that is undocumented/invalid so these need to
> be rejected with -ENXIO. The READ_VOUT command returns data in linear11
> format instead of linear16 so we need to workaround this.
>
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ---
Sashiko is still not happy:

https://sashiko.dev/#/patchset/20260401194235.2149796-1-chris.packham%40alliedtelesis.co.nz

I think we can (hopefully) ignore the multi-page concerns, and the MODULE_IMPORT_NS
concern is obviously wrong.

However, there are a number of real possible issues:

- Is the linear11 format returned for PMBUS_READ_VOUT guaranteed to be positive ?
  Because if not, sign-extending and then clamping the result to [0, 0x3ff]
  would indeed be necessary (just masking to 0x3ff would still return a large
  positive value, so that would not work).

- PMBUS_IOUT_OC_FAULT_LIMIT: Good question. Reporting the value in % would
  indeed be wrong. Either the code or the documentation is wrong and must
  be adjusted.

- The concern about the exponent value from PMBUS_READ_VOUT during probe
  is really a good one. What does the command return if the power supply
  is turned off ? If it is not guaranteed to be static, it might be better
  to hard-code the exponent and adjust the actual runtime value if it happens
  to be different.

Thanks,
Guenter

> Notes:
>      Changes in v3:
>      - Add missing MODULE_DEVICE_TABLE(i2c, ...) and move aps_379_id to just
>        above the probe.
>      - Remove unnecessary sign_extend32
>      - Zero initialise array on stack
>      Changes in v2:
>      - Simplify code per recommendations from Guenter
>      - Add driver documentation
>
>   Documentation/hwmon/aps-379.rst |  58 +++++++++++
>   Documentation/hwmon/index.rst   |   1 +
>   drivers/hwmon/pmbus/Kconfig     |   6 ++
>   drivers/hwmon/pmbus/Makefile    |   1 +
>   drivers/hwmon/pmbus/aps-379.c   | 169 ++++++++++++++++++++++++++++++++
>   5 files changed, 235 insertions(+)
>   create mode 100644 Documentation/hwmon/aps-379.rst
>   create mode 100644 drivers/hwmon/pmbus/aps-379.c
>
> diff --git a/Documentation/hwmon/aps-379.rst b/Documentation/hwmon/aps-379.rst
> new file mode 100644
> index 000000000000..468ec5a98fd6
> --- /dev/null
> +++ b/Documentation/hwmon/aps-379.rst
> @@ -0,0 +1,58 @@
> +Kernel driver aps-379
> +=====================
> +
> +Supported chips:
> +
> +  * Sony APS-379
> +
> +    Prefix: 'aps-379'
> +
> +    Addresses scanned: -
> +
> + Authors:
> +        - Chris Packham
> +
> +Description
> +-----------
> +
> +This driver implements support for the PMBus monitor on the Sony APS-379
> +modular power supply. The APS-379 deviates from the PMBus standard for the
> +READ_VOUT command by using the linear11 format instead of linear16.
> +
> +The known supported PMBus commands are:
> +
> +=== ============================= ========= ======= =====
> +Cmd Function                      Protocol  Scaling Bytes
> +=== ============================= ========= ======= =====
> +01  On / Off Command (OPERATION)  Byte R/W  --      1
> +10  WRITE_PROTECT                 Byte R/W  --      1
> +3B  FAN_COMMAND_1                 Word R/W  --      2
> +46  Current Limit (in percent)    Word R/W  2^0     2
> +47  Current Limit Fault Response  Byte R/W  --      1
> +79  Alarm Data Bits (STATUS_WORD) Word Rd   --      2
> +8B  Output Voltage (READ_VOUT)    Word Rd   2^-4    2
> +8C  Output Current (READ_IOUT)    Word Rd   2^-2    2
> +8D  Power Supply Ambient Temp     Word Rd   2^0     2
> +90  READ_FAN_SPEED_1              Word Rd   2^6     2
> +91  READ_FAN_SPEED_2              Word Rd   2^6     2
> +96  Output Wattage (READ_POUT)    Word Rd   2^1     2
> +97  Input Wattage (READ_PIN)      Word Rd   2^1     2
> +9A  Unit Model Number (MFR_MODEL) Block R/W --      10
> +9B  Unit Revision Number          Block R/W --      10
> +9E  Unit Serial Number            Block R/W --      8
> +99  Unit Manufacturer ID (MFR_ID) Block R/W --      8
> +D0  Unit Run Time Information     Block Rd  --      4
> +D5  Firmware Version              Rd cust   --      8
> +B0  User Data 1 (USER_DATA_00)    Block R/W --      4
> +B1  User Data 2 (USER_DATA_01)    Block R/W --      4
> +B2  User Data 3 (USER_DATA_02)    Block R/W --      4
> +B3  User Data 4 (USER_DATA_03)    Block R/W --      4
> +B4  User Data 5 (USER_DATA_04)    Block R/W --      4
> +B5  User Data 6 (USER_DATA_05)    Block R/W --      4
> +B6  User Data 7 (USER_DATA_06)    Block R/W --      4
> +B7  User Data 8 (USER_DATA_07)    Block R/W --      4
> +F0  Calibration command           Byte R/W  --      1
> +F1  Calibration data              Word Wr   2^9     2
> +F2  Unlock Calibration            Byte Wr   --      1
> +=== ============================= ========= ======= =====
> +
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index b2ca8513cfcd..2bc8d88b5724 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -41,6 +41,7 @@ Hardware Monitoring Kernel Drivers
>      adt7475
>      aht10
>      amc6821
> +   aps-379
>      aquacomputer_d5next
>      asb100
>      asc7621
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index fc1273abe357..29076921e330 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -77,6 +77,12 @@ config SENSORS_ADP1050_REGULATOR
>   	  µModule regulators that can provide microprocessor power from 54V
>   	  power distribution architecture.
>   
> +config SENSORS_APS_379
> +	tristate "Sony APS-379 Power Supplies"
> +	help
> +	  If you say yes here you get hardware monitoring support for Sony
> +	  APS-379 Power Supplies.
> +
>   config SENSORS_BEL_PFE
>   	tristate "Bel PFE Compatible Power Supplies"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index d6c86924f887..94f36c7069ec 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o
>   obj-$(CONFIG_SENSORS_ADM1266)	+= adm1266.o
>   obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
>   obj-$(CONFIG_SENSORS_ADP1050)	+= adp1050.o
> +obj-$(CONFIG_SENSORS_APS_379)	+= aps-379.o
>   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
> diff --git a/drivers/hwmon/pmbus/aps-379.c b/drivers/hwmon/pmbus/aps-379.c
> new file mode 100644
> index 000000000000..5446f663d0b5
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/aps-379.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for Sony APS-379 Power Supplies
> + *
> + * Copyright 2026 Allied Telesis Labs
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +struct aps_379_data {
> +	struct pmbus_driver_info info;
> +	u8 vout_linear_exponent;
> +};
> +
> +#define to_aps_379_data(x) container_of(x, struct aps_379_data, info)
> +
> +static int aps_379_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct aps_379_data *data = to_aps_379_data(info);
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_MODE:
> +		/*
> +		 * The VOUT format used by the chip is linear11,
> +		 * not linear16. Report that VOUT is in linear mode
> +		 * and return exponent value extracted while probing
> +		 * the chip.
> +		 */
> +		return data->vout_linear_exponent;
> +	default:
> +		return -ENODATA;
> +	}
> +}
> +
> +/*
> + * The APS-379 uses linear11 format instead of linear16. We've reported the exponent
> + * via the PMBUS_VOUT_MODE so we just return the mantissa here.
> + */
> +static int aps_379_read_vout(struct i2c_client *client)
> +{
> +	int ret;
> +
> +	ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_READ_VOUT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret & 0x7ff;
> +}
> +
> +static int aps_379_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> +	switch (reg) {
> +	case PMBUS_VOUT_UV_WARN_LIMIT:
> +	case PMBUS_VOUT_OV_WARN_LIMIT:
> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> +	case PMBUS_PIN_OP_WARN_LIMIT:
> +	case PMBUS_POUT_OP_WARN_LIMIT:
> +	case PMBUS_MFR_IIN_MAX:
> +	case PMBUS_MFR_PIN_MAX:
> +	case PMBUS_MFR_VOUT_MIN:
> +	case PMBUS_MFR_VOUT_MAX:
> +	case PMBUS_MFR_IOUT_MAX:
> +	case PMBUS_MFR_POUT_MAX:
> +	case PMBUS_MFR_MAX_TEMP_1:
> +		/* These commands return data but it is invalid/un-documented */
> +		return -ENXIO;
> +	case PMBUS_READ_VOUT:
> +		return aps_379_read_vout(client);
> +	default:
> +		if (reg >= PMBUS_VIRT_BASE)
> +			return -ENXIO;
> +		else
> +			return -ENODATA;
> +	}
> +}
> +
> +static struct pmbus_driver_info aps_379_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_OUT] = linear,
> +	.format[PSC_CURRENT_OUT] = linear,
> +	.format[PSC_POWER] = linear,
> +	.format[PSC_TEMPERATURE] = linear,
> +	.format[PSC_FAN] = linear,
> +	.func[0] = PMBUS_HAVE_VOUT |
> +		PMBUS_HAVE_IOUT |
> +		PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +		PMBUS_HAVE_TEMP |
> +		PMBUS_HAVE_FAN12,
> +	.read_byte_data = aps_379_read_byte_data,
> +	.read_word_data = aps_379_read_word_data,
> +};
> +
> +static const struct i2c_device_id aps_379_id[] = {
> +	{ "aps-379", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, aps_379_id);
> +
> +static int aps_379_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct pmbus_driver_info *info;
> +	struct aps_379_data *data;
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1] = { 0 };
> +	int ret;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	memcpy(&data->info, &aps_379_info, sizeof(*info));
> +	info = &data->info;
> +
> +	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_MODEL, buf);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read Manufacturer Model\n");
> +		return ret;
> +	}
> +
> +	if (strncasecmp(buf, aps_379_id[0].name, strlen(aps_379_id[0].name)) != 0) {
> +		buf[ret] = '\0';
> +		dev_err(dev, "Unsupported Manufacturer Model '%s'\n", buf);
> +		return -ENODEV;
> +	}
> +
> +	ret = i2c_smbus_read_word_data(client, PMBUS_READ_VOUT);
> +	if (ret < 0) {
> +		dev_err(dev, "Can't get vout exponent.\n");
> +		return ret;
> +	}
> +	data->vout_linear_exponent = (u8)((ret >> 11) & 0x1f);
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct of_device_id __maybe_unused aps_379_of_match[] = {
> +	{ .compatible = "sony,aps-379" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, aps_379_of_match);
> +
> +static struct i2c_driver aps_379_driver = {
> +	.driver = {
> +		.name = "aps-379",
> +		.of_match_table = of_match_ptr(aps_379_of_match),
> +	},
> +	.probe = aps_379_probe,
> +	.id_table = aps_379_id,
> +};
> +
> +module_i2c_driver(aps_379_driver);
> +
> +MODULE_AUTHOR("Chris Packham");
> +MODULE_DESCRIPTION("PMBus driver for Sony APS-379");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");