Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support

From: Guenter Roeck
Date: Mon Mar 18 2024 - 11:07:24 EST


On 3/18/24 04:21, Radu Sabau wrote:
Add support for ADP1050 Digital Controller for Isolated Power Supplies
with PMBus interface Voltage, Current and Temperature Monitor.

The ADP1050 implements several features to enable a robust
system of parallel and redundant operation for customers who
require high availability. The device can measure voltage,
current and temperature that can be used in different
techniques to identify and safely shut down an erroneous
power supply in parallel operation mode.

Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
---
Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++
Documentation/hwmon/index.rst | 1 +
drivers/hwmon/pmbus/Kconfig | 10 +++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++
5 files changed, 192 insertions(+)
create mode 100644 Documentation/hwmon/adp1050.rst
create mode 100644 drivers/hwmon/pmbus/adp1050.c

diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst
new file mode 100644
index 000000000000..e3e5bb650a51
--- /dev/null
+++ b/Documentation/hwmon/adp1050.rst
@@ -0,0 +1,69 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver adp1050
+=====================
+
+Supported chips:
+
+ * Analog Devices ADP1050
+
+ Prefix: 'adp1050'
+
+ Addresses scanned: I2C 0x70 - 0x77
+
+ Datasheet: https://www.analog.com/media/en/technical-documentation/data-
+sheets/ADP1050.pdf
+
+Authors:
+
+ - Radu Sabau <radu.sabau@xxxxxxxxxx>
+
+
+Description
+-----------
+
+This driver supprts hardware monitoring for Analog Devices ADP1050 Digital
+Controller for Isolated Power Supply with PMBus interface.
+
+The ADP1050 is an advanced digital controller with a PMBus™
+interface targeting high density, high efficiency dc-to-dc power
+conversion used to monitor system temperatures, voltages and currents.
+Through the PMBus interface, the device can monitor input/output voltages,
+input current and temperature.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate
+the devices explicitly.
+Please see Documentation/i2c/instantiating-devices.rst for details.
+
+The vin scale monitor value and iin scale monitor value can be
+configured by a device tree property;see
+Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data.
+
+Sysfs Attributes
+----------------
+
+================= ========================================
+in1_label "vin"
+in1_input Measured input voltage
+in1_alarm Input voltage alarm
+in2_label "vout1"
+in2_input Measured output voltage
+in2_crit Critical maximum output voltage
+in2_crit_alarm Output voltage high alarm
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+curr1_label "iin"
+curr1_input Measured input current.
+curr1_alarm Input current alarm
+temp1_input Measured temperature
+temp1_crit Critical high temperature
+temp1_crit_alarm Chip temperature critical high alarm
+================= ========================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 1ca7a4fe1f8f..9a4fd576e6f6 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers
adm1266
adm1275
adm9240
+ adp1050
ads7828
adt7410
adt7411
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 557ae0c414b0..38e794d83cc3 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -57,6 +57,16 @@ config SENSORS_ADM1275
This driver can also be built as a module. If so, the module will
be called adm1275.
+config SENSORS_ADP1050
+ tristate "Analog Devices ADP1050 digital controller for Power Supplies"
+ help
+ If you say yes here you get hardware monitoring support for Analog
+ Devices ADP1050 digital controller for isolated power supply with
+ PMBus interface.
+
+ This driver can also be built as a module. If so, the module will
+ be called adp1050.
+
config SENSORS_BEL_PFE
tristate "Bel PFE Compatible Power Supplies"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index f14ecf03ad77..95a8dea5e5ed 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
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_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/adp1050.c b/drivers/hwmon/pmbus/adp1050.c
new file mode 100644
index 000000000000..53198d858156
--- /dev/null
+++ b/drivers/hwmon/pmbus/adp1050.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Hardware monitoring driver for Analog Devices ADP1050
+ *
+ * Copyright (C) 2024 Analog Devices, Inc.
+ */
+#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/of.h>
+#include "pmbus.h"
+
+#define ADP1050_CHIP_PASSWORD 0xD7
+
+#define ADP1050_VIN_SCALE_MONITOR 0xD8
+#define ADP1050_IIN_SCALE_MONITOR 0xD9
+
+static struct pmbus_driver_info adp1050_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+ .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
+ | PMBUS_HAVE_STATUS_TEMP,
+};
+
+static int adp1050_probe(struct i2c_client *client)
+{
+ u32 vin_scale_monitor, iin_scale_monitor;
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WRITE_WORD_DATA))
+ return -ENODEV;
+
+ /* Unlock CHIP's password in order to be able to read/write to it's
+ * VIN_SCALE and IIN_SCALE registers.
+ */
+ ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
+ if (ret < 0) {
+ dev_err_probe(&client->dev, "Device can't be unlocked.\n");
+ return ret;
+ }
+

+ ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
+ if (ret < 0) {
+ dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
+ return ret;
+ }
+

The datasheet says that the factory default password is 0xffff. What if it isn't ?
Refusing to instantiate the chip because it can't be unlocked seems a bit extreme.
After all, the password _can_ be changed.

+ /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
+ * VIN monitor isn't used, therefore 0 is used as scale in order
+ * for the readings to return 0.
+ */
+ if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
+ &vin_scale_monitor))
+ vin_scale_monitor = 0;
+
+ /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
+ * IIN monitor isn't used, therefore 0 is used as scale in order
+ * for the readings to return 0.
+ */
+ if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
+ &iin_scale_monitor))
+ iin_scale_monitor = 0;
+

I don't think the "-monitor" extensions in those property names
add any value.

+ ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
+ vin_scale_monitor);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
+ iin_scale_monitor);
+ if (ret < 0)
+ return ret;
+

If vin and iin monitoring is disabled on purpose, why still set
PMBUS_HAVE_VIN and PMBUS_HAVE_IIN above ? Reporting input values as 0
if the values are explicitly not monitored does not make sense to me.

I am puzzled about this to start with. Unless I am missing something,
the data sheet has no indication that VIN monitoring and/or IIN monitoring
is actually turned off if iin_scale/vin_scale are set to 0. Why support
the option to disable them ? That doesn't make sense to me. Please explain
how/when this makes sense. Please also explain why chip support is made
to depend on devicetree support. Register values may have been be set by
rommon/bios or (much more likely) by manufacturing, or users could just
want to keep the chip default. Why force-override it or even force-disable
it ?

Thanks,
Guenter