[PATCH 0/2] hwmon: Add support for TI TPS389008

[PATCH 0/2] hwmon: Add support for TI TPS389008

[Flaviu Nistor]

This series adds support and documentation for the TI TPS389008,
TPS389006 and TPS389004 voltage monitor IC's.

This I2C device provides 8 bit ADC conversion result for all
input channels.

The proposed driver implements the logic to perform the ADC read.

The features this driver does not support (alarms and thresholds
for the alarms) have been documented in the "Known Issues" section.

The complete supported functionality has been tested with a TPS389008
and TPS389006 voltage monitors connected to a Raspberry Pi 4.
Different device tree node definitions have been tested with success.

Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>

Flaviu Nistor (2):
  hwmon: Add support for TI TPS389008
  dt-bindings: hwmon: Add TI TPS389008

 .../bindings/hwmon/ti,tps389008.yaml          | 140 ++++++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/tps389008.rst             |  56 +++
 MAINTAINERS                                   |   8 +
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/tps389008.c                     | 466 ++++++++++++++++++
 7 files changed, 682 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
 create mode 100644 Documentation/hwmon/tps389008.rst
 create mode 100644 drivers/hwmon/tps389008.c

-- 
2.43.0

[PATCH 1/2] hwmon: Add support for TI TPS389008

[Flaviu Nistor]

TI TPS38900x are voltage monitor IC's.

All device variants offer an I2C interface and depending on the
part number, x monitor inputs.

Support chips by the driver are TPS389008, TPS389006 and TPS389004.

The driver adds support for reading the 8-bit ADC value of any of
the VMON inputs. By default the inputs are disabled and have a scaling
factor of 1x. They need to be enabled in the device tree, or using the
sysfs attribute from user space.

Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
---
 Documentation/hwmon/index.rst     |   1 +
 Documentation/hwmon/tps389008.rst |  56 ++++
 MAINTAINERS                       |   8 +
 drivers/hwmon/Kconfig             |  10 +
 drivers/hwmon/Makefile            |   1 +
 drivers/hwmon/tps389008.c         | 466 ++++++++++++++++++++++++++++++
 6 files changed, 542 insertions(+)
 create mode 100644 Documentation/hwmon/tps389008.rst
 create mode 100644 drivers/hwmon/tps389008.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 874f8fd26325..b04643d14972 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -239,6 +239,7 @@ Hardware Monitoring Kernel Drivers
    tmp513
    tps23861
    tps25990
+   tps389008
    tps40422
    tps53679
    tps546d24
diff --git a/Documentation/hwmon/tps389008.rst b/Documentation/hwmon/tps389008.rst
new file mode 100644
index 000000000000..6e1166165ac4
--- /dev/null
+++ b/Documentation/hwmon/tps389008.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver tps389008
+======================
+
+Supported chips:
+
+  * TI TPS389008, TPS389006, TPS389004
+
+    Prefix: 'tps389008'
+
+    Addresses scanned: -
+
+    Datasheet: https://www.ti.com/lit/ds/symlink/tps389006.pdf?ts=1741000787840&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FTPS389006
+
+Author:
+
+  - Flaviu Nistor <flaviu.nistor@gmail.com>
+
+Description
+-----------
+
+This driver implements support for TI TPS389008, TPS389006 and TPS389004 voltage monitor chips.
+The driver supports only the chips that have default values based on datasheet and not OTP NVM settings.
+Monitored voltages can be read out via an internal ADC with one register per input channel.
+Measured voltage is expressed in mV per LSB.
+The measurement voltage ranges depends on the scaling factor used as following:
+
+  - 1x scaling: 200 to 1475 mV (8-bit resolution)
+  - 4x scaling: 800 to 5900 mV (8-bit resolution)
+
+The scaling factor is 1 by default for all channels.
+
+All input VMON channel are disabled by default, and they can be enabled via the dts (during probe)
+or using the provided sysfs attribute from user space.
+
+The device communicates with the I2C protocol and uses the I2C address 0x30 by default.
+
+
+Known Issues
+------------
+
+The driver does not support usage of alarms and setting of thresholds (for the alarms).
+
+sysfs-Interface
+---------------
+
+The following list includes the sysfs attributes that the driver will provide for each added input channel:
+
+=============================== ======= ========================================
+Name                            Perm    Description
+=============================== ======= ========================================
+in[12345678]_input:             RO      Voltage channel input
+in[12345678]_label:             RO      Voltage channel label
+in[12345678]_enable:            RW      Voltage channel enable controls
+=============================== ======= ========================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 16f51eb6ebe8..fbf07f26d933 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23510,6 +23510,14 @@ F:	Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
 F:	Documentation/hwmon/tps23861.rst
 F:	drivers/hwmon/tps23861.c
 
+TEXAS INSTRUMENTS TPS389008 VMON DRIVER
+M:	Flaviu Nistor <flaviu.nistor@gmail.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
+F:	Documentation/hwmon/tps389008.rst
+F:	drivers/hwmon/tps389008.c
+
 TEXAS INSTRUMENTS' DAC7612 DAC DRIVER
 M:	Ricardo Ribalda <ribalda@kernel.org>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4cbaba15d86e..5562eea4d0bb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1340,6 +1340,16 @@ config SENSORS_TPS23861
 	  This driver can also be built as a module. If so, the module
 	  will be called tps23861.
 
+config SENSORS_TPS389008
+	tristate "TI TPS389008 VMON Driver"
+	depends on I2C
+	help
+	  This driver provides support for voltage monitoring for the Texas
+	  Instruments TPS389008, TPS389006 and TPS389004 chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tps389008.
+
 config SENSORS_MENF21BMC_HWMON
 	tristate "MEN 14F021P00 BMC Hardware Monitoring"
 	depends on MFD_MENF21BMC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b7ef0f0562d3..6eb0b7696239 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -164,6 +164,7 @@ obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
 obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
 obj-$(CONFIG_SENSORS_TPS23861)	+= tps23861.o
+obj-$(CONFIG_SENSORS_TPS389008) += tps389008.o
 obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
 obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
 obj-$(CONFIG_SENSORS_MR75203)	+= mr75203.o
diff --git a/drivers/hwmon/tps389008.c b/drivers/hwmon/tps389008.c
new file mode 100644
index 000000000000..6ee6c3b58747
--- /dev/null
+++ b/drivers/hwmon/tps389008.c
@@ -0,0 +1,466 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * tps389008.c Support for the TI TPS389008 Voltage Monitor
+ *
+ * Part numbers supported:
+ * TPS389006, TPS389008
+ *
+ * Author: Flaviu Nistor <flaviu.nistor@gmail.com>
+ *
+ * Datasheet and application notes:
+ * https://www.ti.com/
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+
+#define TPS389008_NUM_CHANNELS      8
+#define TPS389008_1LSB              5
+#define RANGE_OFFSET                200
+#define BANK0                       0
+#define BANK1                       1
+/*BANK0 REGISTERS*/
+#define MON_LVL			            0x40
+#define BANK_SEL                    0xF0
+/*BANK1 REGISTERS
+ * use 0x100 to signal that address is part of BANK1
+ */
+#define MON_CH_EN                   0x11E
+
+struct tps389008_input {
+	const char *label;
+	int vrange_mult_mv;
+	bool enabled;
+	bool disconnected;
+};
+
+struct tps389008_data {
+	struct device *hwmon;
+	struct i2c_client *client;
+	struct tps389008_input input[TPS389008_NUM_CHANNELS];
+	struct mutex dev_access_lock; /* device access lock */
+	const char *name;
+	int current_bank;
+};
+
+enum tps_chan_addr {
+	TPS_CHANNEL_0 = 0,
+	TPS_CHANNEL_1,
+	TPS_CHANNEL_2,
+	TPS_CHANNEL_3,
+	TPS_CHANNEL_4,
+	TPS_CHANNEL_5,
+	TPS_CHANNEL_6,
+	TPS_CHANNEL_7,
+	TPS_CHANNEL_8
+};
+
+static int tps389008_read_string(struct device *dev, enum hwmon_sensor_types type,
+	u32 attr, int channel, const char **str)
+{
+	struct tps389008_data *tps = dev_get_drvdata(dev);
+	int index;
+
+	index = channel - 1;
+	*str = tps->input[index].label;
+
+	return 0;
+}
+
+static umode_t tps389008_is_visible(const void *data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	const struct tps389008_data *tps = data;
+	const struct tps389008_input *input = NULL;
+	int index;
+
+	/* channel numbering starts from 1, but index from 0*/
+	index = channel - 1;
+
+	/* in0_ or disconnected channels should be ignored*/
+	if (channel == 0 ||
+		(tps->input[index].disconnected ||
+		 tps->input[index].vrange_mult_mv == 0)) {
+		return 0;
+	}
+	switch (attr) {
+	case hwmon_in_input:
+		return 0444;
+	case hwmon_in_label:
+		input = &tps->input[index];
+		/* Hide label node if label is not provided */
+		return (input && input->label) ? 0444 : 0;
+	case hwmon_in_enable:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+static int change_bank(struct tps389008_data *data, u8 bank)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, BANK_SEL, bank);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"change to bank%d failed with error code: %d\n", bank, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int tps389008_write_reg(struct tps389008_data *data, u16 reg, u8 val)
+{
+	int ret;
+	u8 bank;
+
+	bank  = (reg & 0x100) >> 8;
+
+	if (bank != (u8)data->current_bank) {
+		change_bank(data, bank);
+		data->current_bank = bank;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, reg, val);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+				"I2C write failed at address: 0x%X with error code: %d\n",
+				reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tps389008_read_reg(struct tps389008_data *data, u16 reg, u16 *val)
+{
+	int read_byte;
+	u8 bank;
+
+	bank  = (reg & 0x100) >> 8;
+
+	if (bank != (u8)data->current_bank) {
+		change_bank(data, bank);
+		data->current_bank = bank;
+	}
+
+	read_byte = i2c_smbus_read_byte_data(data->client, reg);
+	if (read_byte < 0) {
+		dev_err(&data->client->dev,
+			"I2C read failed at address: 0x%X with error code: %d\n", reg, read_byte);
+		return read_byte;
+	}
+
+	*val = read_byte;
+
+	return 0;
+}
+
+static int tps389008_get_in_val(struct tps389008_data *data, u16 reg, int channel, long *val)
+{
+	u16 reg_val;
+	int ret;
+	int index;
+
+	/* channel numbering starts from 1, but index from 0*/
+	index = channel - 1;
+
+	ret = tps389008_read_reg(data, reg, &reg_val);
+	if (ret)
+		return ret;
+
+	*val = ((reg_val * TPS389008_1LSB + RANGE_OFFSET) * data->input[index].vrange_mult_mv);
+
+	return 0;
+}
+
+static int disable_input(struct tps389008_data *data, u8 channel)
+{
+	u16 reg_val;
+	int ret;
+	int index;
+
+	index = channel - 1;
+
+	ret = tps389008_read_reg(data, MON_CH_EN, &reg_val);
+	if (ret)
+		return ret;
+
+	reg_val = reg_val & ~(1 << index);
+
+	ret = tps389008_write_reg(data, MON_CH_EN, reg_val);
+
+	return ret;
+}
+
+static int enable_input(struct tps389008_data *data, u8 channel)
+{
+	u16 reg_val;
+	int ret;
+	int index;
+
+	index = channel - 1;
+
+	ret = tps389008_read_reg(data, MON_CH_EN, &reg_val);
+	if (ret)
+		return ret;
+
+	reg_val = reg_val | (1 << index);
+
+	ret = tps389008_write_reg(data, MON_CH_EN, reg_val);
+
+	return ret;
+}
+
+static int tps389008_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			int channel, long val)
+{
+	struct tps389008_data *data = dev_get_drvdata(dev);
+	int index;
+	int ret;
+
+	/* channel numbering starts from 1, but index from 0*/
+	index = channel - 1;
+
+	guard(mutex)(&data->dev_access_lock);
+
+	if (attr == hwmon_in_input || attr == hwmon_in_label) {
+		dev_warn(dev, "Write to READ ONLY resource\n");
+		return -EOPNOTSUPP;
+	}
+	if (attr == hwmon_in_enable) {
+		if (val == 0) {
+			data->input[index].enabled = val;
+			ret = disable_input(data, channel);
+			return ret;
+		} else if (val == 1) {
+			data->input[index].enabled = val;
+			ret = enable_input(data, channel);
+			return ret;
+		}
+		dev_err(dev, "invalid value %ld\n", val);
+		return -EINVAL;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int tps389008_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			int channel, long *val)
+{
+
+	struct tps389008_data *data = dev_get_drvdata(dev);
+	int ret;
+	int index;
+
+	/* channel numbering starts from 1, but index from 0*/
+	index = channel - 1;
+
+	guard(mutex)(&data->dev_access_lock);
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = tps389008_get_in_val(data, MON_LVL + index, channel, val);
+		if (ret)
+			dev_err(dev,
+				"Reading the ADC value for channel %d failed with error code: %d\n",
+				channel, ret);
+		break;
+	case hwmon_in_enable:
+		*val = data->input[index].enabled;
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct hwmon_channel_info *tps389008_info[] = {
+	HWMON_CHANNEL_INFO(in,
+					/* 0: dummy, skipped in is_visible */
+					HWMON_I_INPUT,
+					/* 1-8: input voltage Channels */
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops tps389008_hwmon_ops = {
+	.is_visible = tps389008_is_visible,
+	.read_string = tps389008_read_string,
+	.read = tps389008_read,
+	.write = tps389008_write,
+};
+
+static const struct hwmon_chip_info tps389008_chip_info = {
+	.ops = &tps389008_hwmon_ops,
+	.info = tps389008_info,
+};
+
+
+static int tps389008_probe_child_from_dt(struct device *dev,
+	struct device_node *child,
+	struct tps389008_data *tps)
+{
+	struct tps389008_input *input;
+	u32 val;
+	int ret;
+
+	ret = of_property_read_u32(child, "reg", &val);
+	if (ret) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return ret;
+	}
+	if (val < 1 || val > TPS_CHANNEL_8) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", val, child);
+		return -EINVAL;
+	}
+
+	/* remember that children nodes starts from 1, but we have the input start index 0.*/
+	input = &tps->input[val-1];
+
+	ret = (int)of_property_read_bool(child, "ti,vrange-mult-4x");
+	if (ret)
+		input->vrange_mult_mv = 4;
+	else
+		input->vrange_mult_mv = 1;
+
+	ret = (int)of_property_read_bool(child, "ti,vmon-enable");
+	/* missing optional property. Default enable the channel*/
+	if (ret)
+		input->enabled = true;
+
+	/* Log the disconnected channel input */
+	if (!of_device_is_available(child)) {
+		input->disconnected = true;
+		return 0;
+	}
+
+	/* Save the connected input label if available */
+	of_property_read_string(child, "label", &input->label);
+
+	return 0;
+}
+
+static int tps389008_probe_from_dt(struct device *dev, struct tps389008_data *tps)
+{
+	const struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int ret;
+
+	for_each_child_of_node(np, child) {
+		ret = tps389008_probe_child_from_dt(dev, child, tps);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int tps389008_probe(struct i2c_client *client)
+{
+	struct tps389008_data *data;
+	struct device *dev = &client->dev;
+	int ret, i;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, data);
+	mutex_init(&data->dev_access_lock);
+	data->client = client;
+
+	ret = tps389008_probe_from_dt(dev, data);
+	if (ret) {
+		dev_err(dev, "Unable to probe from device tree\n");
+		return ret;
+	}
+
+	data->current_bank = 0;
+
+	/* Enable channels if they are enabled */
+	for (i = 0; i < TPS389008_NUM_CHANNELS; i++) {
+		if (data->input[i].enabled) {
+			enable_input(data, i+1);
+			dev_dbg(dev, "VMON input %d is enabled\n", (i+1));
+		}
+	}
+
+	data->hwmon = devm_hwmon_device_register_with_info(dev, client->name,
+							   data, &tps389008_chip_info,
+							   NULL);
+
+	if (IS_ERR(data->hwmon))
+		return dev_err_probe(dev, PTR_ERR(data->hwmon),
+					 "Failed to register hwmon device tps389008\n");
+
+	dev_info(dev, "hwmon device tps389008 probed successfully\n");
+
+	return 0;
+}
+
+static void tps389008_remove(struct i2c_client *client)
+{
+	struct tps389008_data *tps = dev_get_drvdata(&client->dev);
+
+	hwmon_device_unregister(tps->hwmon);
+
+	mutex_destroy(&tps->dev_access_lock);
+}
+
+static const struct i2c_device_id tps389008_ids[] = {
+	{ "tps389008" },
+	{ "tps389006" },
+	{ "tps389004" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tps389008_ids);
+
+static const struct of_device_id tps389008_of_match[] = {
+	{ .compatible = "ti,tps389008", },
+	{ .compatible = "ti,tps389006", },
+	{ .compatible = "ti,tps389004", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tps389008_of_match);
+
+static struct i2c_driver tps389008_i2c_driver = {
+	.driver = {
+		.name = "tps389008",
+		.of_match_table = tps389008_of_match,
+	},
+	.probe = tps389008_probe,
+	.remove = tps389008_remove,
+	.id_table = tps389008_ids,
+};
+
+module_i2c_driver(tps389008_i2c_driver);
+
+MODULE_AUTHOR("Flaviu Nistor <flaviu.nistor@gmail.com>");
+MODULE_DESCRIPTION("TI TPS389008 voltage monitor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 1/2] hwmon: Add support for TI TPS389008

[Guenter Roeck]

On 3/19/25 10:02, Flaviu Nistor wrote:
> TI TPS38900x are voltage monitor IC's.
> 
> All device variants offer an I2C interface and depending on the
> part number, x monitor inputs.
> 
> Support chips by the driver are TPS389008, TPS389006 and TPS389004.
> 
> The driver adds support for reading the 8-bit ADC value of any of

Use imperative mood.

> the VMON inputs. By default the inputs are disabled and have a scaling
> factor of 1x. They need to be enabled in the device tree, or using the
> sysfs attribute from user space.
> 
Default must be the current chip configuration.

> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>

You'll have to decide if you want to use flaviu.nistor@googlemail.com or
flaviu.nistor@gmail.com.

This is not a complete review.

General comments:

- The driver must support non-devicetree configurations and
   in that case just use defaults (i.e., use what the chip provides).

- Drop VMON. This is a driver for the chip, not just for its voltage monitoring
   capabilities.

- Voltage channel numbers start with 0. Yes, there are one or two drivers
   starting with 1 for historic reasons, but that is not an argument.
   Besides, just looking at the code should make it obvious that the 1->0 mapping
   is risky and error prone.

- checkpatch --strict says:

   total: 0 errors, 4 warnings, 13 checks, 566 lines checked

   I expect that to be addressed in the next version.

> ---
>   Documentation/hwmon/index.rst     |   1 +
>   Documentation/hwmon/tps389008.rst |  56 ++++
>   MAINTAINERS                       |   8 +
>   drivers/hwmon/Kconfig             |  10 +
>   drivers/hwmon/Makefile            |   1 +
>   drivers/hwmon/tps389008.c         | 466 ++++++++++++++++++++++++++++++
>   6 files changed, 542 insertions(+)
>   create mode 100644 Documentation/hwmon/tps389008.rst
>   create mode 100644 drivers/hwmon/tps389008.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 874f8fd26325..b04643d14972 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -239,6 +239,7 @@ Hardware Monitoring Kernel Drivers
>      tmp513
>      tps23861
>      tps25990
> +   tps389008
>      tps40422
>      tps53679
>      tps546d24
> diff --git a/Documentation/hwmon/tps389008.rst b/Documentation/hwmon/tps389008.rst
> new file mode 100644
> index 000000000000..6e1166165ac4
> --- /dev/null
> +++ b/Documentation/hwmon/tps389008.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver tps389008
> +======================

Does that pass documentation builds ?

> +
> +Supported chips:
> +
> +  * TI TPS389008, TPS389006, TPS389004
> +
> +    Prefix: 'tps389008'
> +
> +    Addresses scanned: -
> + 
> +    Datasheet: https://www.ti.com/lit/ds/symlink/tps389006.pdf?ts=1741000787840&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FTPS389006
> +
> +Author:
> +
> +  - Flaviu Nistor <flaviu.nistor@gmail.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for TI TPS389008, TPS389006 and TPS389004 voltage monitor chips.

Those are not voltage monitor chips, they are voltage supersisor and monitor chips.

> +The driver supports only the chips that have default values based on datasheet and not OTP NVM settings.

Why ? Given the normal use case for those chips (as voltage sequencer)
that doesn't make any sense. If anything, it should be the opposite.

> +Monitored voltages can be read out via an internal ADC with one register per input channel.
> +Measured voltage is expressed in mV per LSB.
> +The measurement voltage ranges depends on the scaling factor used as following:
> +
> +  - 1x scaling: 200 to 1475 mV (8-bit resolution)
> +  - 4x scaling: 800 to 5900 mV (8-bit resolution)
> +
> +The scaling factor is 1 by default for all channels.
> +
> +All input VMON channel are disabled by default, and they can be enabled via the dts (during probe)
> +or using the provided sysfs attribute from user space.

This is unacceptable. Default has to be what the chip is programmed to do
for a chip like this.

> +
> +The device communicates with the I2C protocol and uses the I2C address 0x30 by default.
> +
> +
> +Known Issues
> +------------
> +
> +The driver does not support usage of alarms and setting of thresholds (for the alarms).
> +

Curious: Why ? That would be the primary use case for a hardware monitoring driver.

> +sysfs-Interface
> +---------------
> +
> +The following list includes the sysfs attributes that the driver will provide for each added input channel:
> +
> +=============================== ======= ========================================
> +Name                            Perm    Description
> +=============================== ======= ========================================
> +in[12345678]_input:             RO      Voltage channel input
> +in[12345678]_label:             RO      Voltage channel label
> +in[12345678]_enable:            RW      Voltage channel enable controls

Make that [0-7]. Also indicate number of channels per chip.

> +=============================== ======= ========================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16f51eb6ebe8..fbf07f26d933 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23510,6 +23510,14 @@ F:	Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
>   F:	Documentation/hwmon/tps23861.rst
>   F:	drivers/hwmon/tps23861.c
>   
> +TEXAS INSTRUMENTS TPS389008 VMON DRIVER

Drop VMON here and elsewhere.

> +M:	Flaviu Nistor <flaviu.nistor@gmail.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
> +F:	Documentation/hwmon/tps389008.rst
> +F:	drivers/hwmon/tps389008.c
> +
>   TEXAS INSTRUMENTS' DAC7612 DAC DRIVER
>   M:	Ricardo Ribalda <ribalda@kernel.org>
>   L:	linux-iio@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4cbaba15d86e..5562eea4d0bb 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1340,6 +1340,16 @@ config SENSORS_TPS23861
>   	  This driver can also be built as a module. If so, the module
>   	  will be called tps23861.
>   
> +config SENSORS_TPS389008
> +	tristate "TI TPS389008 VMON Driver"
> +	depends on I2C
> +	help
> +	  This driver provides support for voltage monitoring for the Texas
> +	  Instruments TPS389008, TPS389006 and TPS389004 chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called tps389008.
> +
>   config SENSORS_MENF21BMC_HWMON
>   	tristate "MEN 14F021P00 BMC Hardware Monitoring"
>   	depends on MFD_MENF21BMC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b7ef0f0562d3..6eb0b7696239 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -164,6 +164,7 @@ obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
>   obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
>   obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>   obj-$(CONFIG_SENSORS_TPS23861)	+= tps23861.o
> +obj-$(CONFIG_SENSORS_TPS389008) += tps389008.o
>   obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
>   obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>   obj-$(CONFIG_SENSORS_MR75203)	+= mr75203.o
> diff --git a/drivers/hwmon/tps389008.c b/drivers/hwmon/tps389008.c
> new file mode 100644
> index 000000000000..6ee6c3b58747
> --- /dev/null
> +++ b/drivers/hwmon/tps389008.c
> @@ -0,0 +1,466 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * tps389008.c Support for the TI TPS389008 Voltage Monitor
> + *
> + * Part numbers supported:
> + * TPS389006, TPS389008
> + *

This does not match the rest of the code.

> + * Author: Flaviu Nistor <flaviu.nistor@gmail.com>
> + *
> + * Datasheet and application notes:
> + * https://www.ti.com/

Pretty pointless reference.

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>

I do not se where completion.h and delay.h would be needed.

> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>

I do not see any interrupt support.

> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>

I do not see any regulator code.

Please only include files which are actually used.

Overall it seems like the template for this driver was either copied,
or the entire driver ws copied and scaled down. That is fine, but there
should be a reference to the origin.

> +
> +#define TPS389008_NUM_CHANNELS      8
> +#define TPS389008_1LSB              5
> +#define RANGE_OFFSET                200
> +#define BANK0                       0
> +#define BANK1                       1
> +/*BANK0 REGISTERS*/
> +#define MON_LVL			            0x40
> +#define BANK_SEL                    0xF0
> +/*BANK1 REGISTERS
> + * use 0x100 to signal that address is part of BANK1
> + */

/*
  * Please use proper multi-line comments, and add spaces after the beginning and before
  * the end of a comment.
  */

> +#define MON_CH_EN                   0x11E
> +
> +struct tps389008_input {
> +	const char *label;
> +	int vrange_mult_mv;
> +	bool enabled;
> +	bool disconnected;

There should only be one flag, for disconnected channels.
'enabled' is a local cache. Either use regmap for caching,
or get it from the chip.

> +};
> +
> +struct tps389008_data {
> +	struct device *hwmon;
> +	struct i2c_client *client;
> +	struct tps389008_input input[TPS389008_NUM_CHANNELS];
> +	struct mutex dev_access_lock; /* device access lock */
> +	const char *name;
> +	int current_bank;
> +};
> +
> +enum tps_chan_addr {
> +	TPS_CHANNEL_0 = 0,
> +	TPS_CHANNEL_1,
> +	TPS_CHANNEL_2,
> +	TPS_CHANNEL_3,
> +	TPS_CHANNEL_4,
> +	TPS_CHANNEL_5,
> +	TPS_CHANNEL_6,
> +	TPS_CHANNEL_7,
> +	TPS_CHANNEL_8

I do not see the point of these defines.

> +};
> +
> +static int tps389008_read_string(struct device *dev, enum hwmon_sensor_types type,
> +	u32 attr, int channel, const char **str)
> +{
> +	struct tps389008_data *tps = dev_get_drvdata(dev);
> +	int index;
> +
> +	index = channel - 1;

This is an absolute no-go. Channels start with 0. There must not be a
dummy channel.

> +	*str = tps->input[index].label;
> +
> +	return 0;
> +}
> +
> +static umode_t tps389008_is_visible(const void *data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	const struct tps389008_data *tps = data;
> +	const struct tps389008_input *input = NULL;

Unecessary assignment.

> +	int index;
> +
> +	/* channel numbering starts from 1, but index from 0*/
> +	index = channel - 1;
> +
> +	/* in0_ or disconnected channels should be ignored*/
> +	if (channel == 0 ||
> +		(tps->input[index].disconnected ||
> +		 tps->input[index].vrange_mult_mv == 0)) {

vrange_mult_mv should have a default and never be 0.

> +		return 0;
> +	}
> +	switch (attr) {
> +	case hwmon_in_input:
> +		return 0444;
> +	case hwmon_in_label:
> +		input = &tps->input[index];
> +		/* Hide label node if label is not provided */
> +		return (input && input->label) ? 0444 : 0;

"input" is never NULL.

> +	case hwmon_in_enable:
> +		return 0644;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int change_bank(struct tps389008_data *data, u8 bank)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, BANK_SEL, bank);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"change to bank%d failed with error code: %d\n", bank, ret);

Drop all those runtime error messages.

> +		return ret;
> +	}
> +
> +	return ret;
> +}

This function is only used to display an error message, which I am not going
to accept. All callers check if changing the bank is necessary, and save
the new bank number. This doesn't make sense. Please move the common code into
this function.

> +
> +static int tps389008_write_reg(struct tps389008_data *data, u16 reg, u8 val)
> +{
> +	int ret;
> +	u8 bank;
> +
> +	bank  = (reg & 0x100) >> 8;
> +
> +	if (bank != (u8)data->current_bank) {

Why the typecast and not just define current_bank as u8 ?

> +		change_bank(data, bank);

Add error checks here and everywhere else.

> +		data->current_bank = bank;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, reg, val);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +				"I2C write failed at address: 0x%X with error code: %d\n",
> +				reg, ret);

Drop all those log messages.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps389008_read_reg(struct tps389008_data *data, u16 reg, u16 *val)
> +{
> +	int read_byte;
> +	u8 bank;
> +
> +	bank  = (reg & 0x100) >> 8;
> +
> +	if (bank != (u8)data->current_bank) {
> +		change_bank(data, bank);
> +		data->current_bank = bank;
> +	}
> +
> +	read_byte = i2c_smbus_read_byte_data(data->client, reg);
> +	if (read_byte < 0) {
> +		dev_err(&data->client->dev,
> +			"I2C read failed at address: 0x%X with error code: %d\n", reg, read_byte);
> +		return read_byte;
> +	}
> +
> +	*val = read_byte;

What is the point of splitting return value and error code ?
It just adds a lot of unnecessary complexity to the code.

> +
> +	return 0;
> +}
> +
> +static int tps389008_get_in_val(struct tps389008_data *data, u16 reg, int channel, long *val)
> +{
> +	u16 reg_val;
> +	int ret;
> +	int index;
> +
> +	/* channel numbering starts from 1, but index from 0*/
> +	index = channel - 1;
> +
> +	ret = tps389008_read_reg(data, reg, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*val = ((reg_val * TPS389008_1LSB + RANGE_OFFSET) * data->input[index].vrange_mult_mv);

Drop all unnecessary ( ).

> +
> +	return 0;
> +}
> +
> +static int disable_input(struct tps389008_data *data, u8 channel)
> +{
> +	u16 reg_val;
> +	int ret;
> +	int index;
> +
> +	index = channel - 1;
> +
> +	ret = tps389008_read_reg(data, MON_CH_EN, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val = reg_val & ~(1 << index);
> +
> +	ret = tps389008_write_reg(data, MON_CH_EN, reg_val);
> +
> +	return ret;
> +}
> +
> +static int enable_input(struct tps389008_data *data, u8 channel)
> +{
> +	u16 reg_val;
> +	int ret;
> +	int index;
> +
> +	index = channel - 1;
> +
> +	ret = tps389008_read_reg(data, MON_CH_EN, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val = reg_val | (1 << index);

This is what the BIT() macro is for. Same here and elsewhere.

> +
> +	ret = tps389008_write_reg(data, MON_CH_EN, reg_val);
> +
> +	return ret;
> +}
> +
> +static int tps389008_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			int channel, long val)
> +{
> +	struct tps389008_data *data = dev_get_drvdata(dev);
> +	int index;
> +	int ret;
> +
> +	/* channel numbering starts from 1, but index from 0*/
> +	index = channel - 1;
> +
> +	guard(mutex)(&data->dev_access_lock);
> +
> +	if (attr == hwmon_in_input || attr == hwmon_in_label) {
> +		dev_warn(dev, "Write to READ ONLY resource\n");
> +		return -EOPNOTSUPP;
> +	}

The checked attributes are read-only. This won't happen.
Even if it was possible, the error message would be unacceptable.
Please drop this code.

> +	if (attr == hwmon_in_enable) {
> +		if (val == 0) {
> +			data->input[index].enabled = val;
> +			ret = disable_input(data, channel);
> +			return ret;
> +		} else if (val == 1) {
> +			data->input[index].enabled = val;
> +			ret = enable_input(data, channel);
> +			return ret;
> +		}
> +		dev_err(dev, "invalid value %ld\n", val);

This would create a lot of noise if someone keeps writing "2".
It is both unnecessary and unacceptable.

> +		return -EINVAL;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int tps389008_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			int channel, long *val)
> +{
> +
> +	struct tps389008_data *data = dev_get_drvdata(dev);
> +	int ret;
> +	int index;
> +
> +	/* channel numbering starts from 1, but index from 0*/
> +	index = channel - 1;
> +
> +	guard(mutex)(&data->dev_access_lock);
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		ret = tps389008_get_in_val(data, MON_LVL + index, channel, val);
> +		if (ret)
> +			dev_err(dev,
> +				"Reading the ADC value for channel %d failed with error code: %d\n",
> +				channel, ret);
> +		break;
> +	case hwmon_in_enable:
> +		*val = data->input[index].enabled;

ret is uninitialized. No idea why the compiler doesn't bail out here.

> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct hwmon_channel_info *tps389008_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +					/* 0: dummy, skipped in is_visible */
> +					HWMON_I_INPUT,
> +					/* 1-8: input voltage Channels */
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
> +					HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL),

Indentation is way to deep.

> +	NULL
> +};
> +
> +static const struct hwmon_ops tps389008_hwmon_ops = {
> +	.is_visible = tps389008_is_visible,
> +	.read_string = tps389008_read_string,
> +	.read = tps389008_read,
> +	.write = tps389008_write,
> +};
> +
> +static const struct hwmon_chip_info tps389008_chip_info = {
> +	.ops = &tps389008_hwmon_ops,
> +	.info = tps389008_info,
> +};
> +
> +

No double empty lines. checkpatch would tell you.

> +static int tps389008_probe_child_from_dt(struct device *dev,
> +	struct device_node *child,
> +	struct tps389008_data *tps)

Please no unnecessary continuation lines. On top of that,
continuation line assignment is wrong. Again, checkpatch would tell.

> +{
> +	struct tps389008_input *input;
> +	u32 val;
> +	int ret;
> +
> +	ret = of_property_read_u32(child, "reg", &val);
> +	if (ret) {
> +		dev_err(dev, "missing reg property of %pOFn\n", child);
> +		return ret;
> +	}
> +	if (val < 1 || val > TPS_CHANNEL_8) {

This should compare against the actual number of channels supported by the chip.

> +		dev_err(dev, "invalid reg %d of %pOFn\n", val, child);
> +		return -EINVAL;
> +	}
> +
> +	/* remember that children nodes starts from 1, but we have the input start index 0.*/
> +	input = &tps->input[val-1];
> +
> +	ret = (int)of_property_read_bool(child, "ti,vrange-mult-4x");

Unnecessary type cast and, for that matter, variable assignment.

	if (of_property_read_bool(child, "ti,vrange-mult-4x"))

would be just as good.

> +	if (ret)
> +		input->vrange_mult_mv = 4;
> +	else
> +		input->vrange_mult_mv = 1;
> +
> +	ret = (int)of_property_read_bool(child, "ti,vmon-enable");
> +	/* missing optional property. Default enable the channel*/
> +	if (ret)
> +		input->enabled = true;

	input->enabled = of_property_read_bool(child, "ti,vmon-enable");

> +
> +	/* Log the disconnected channel input */
> +	if (!of_device_is_available(child)) {
> +		input->disconnected = true;
> +		return 0;
> +	}
> +
> +	/* Save the connected input label if available */
> +	of_property_read_string(child, "label", &input->label);
> +
> +	return 0;
> +}
> +
> +static int tps389008_probe_from_dt(struct device *dev, struct tps389008_data *tps)
> +{
> +	const struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int ret;
> +
> +	for_each_child_of_node(np, child) {

Use for_each_child_of_node_scoped().

> +		ret = tps389008_probe_child_from_dt(dev, child, tps);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps389008_probe(struct i2c_client *client)
> +{
> +	struct tps389008_data *data;
> +	struct device *dev = &client->dev;
> +	int ret, i;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -EOPNOTSUPP;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->dev_access_lock);

Use devm_mutex_init() and drop the remove function.

> +	data->client = client;
> +
> +	ret = tps389008_probe_from_dt(dev, data);
> +	if (ret) {
> +		dev_err(dev, "Unable to probe from device tree\n");
> +		return ret;
> +	}
> +
> +	data->current_bank = 0;

This is just an assumption. Either read the current channel from
the chip, or set it to an nivalid value such as 0xff to ensure
that the channel is selected with the first access.

> +
> +	/* Enable channels if they are enabled */
> +	for (i = 0; i < TPS389008_NUM_CHANNELS; i++) {
> +		if (data->input[i].enabled) {
> +			enable_input(data, i+1);

Error check missing.

> +			dev_dbg(dev, "VMON input %d is enabled\n", (i+1));
> +		}
> +	}
> +
> +	data->hwmon = devm_hwmon_device_register_with_info(dev, client->name,
> +							   data, &tps389008_chip_info,
> +							   NULL);
> +
> +	if (IS_ERR(data->hwmon))
> +		return dev_err_probe(dev, PTR_ERR(data->hwmon),
> +					 "Failed to register hwmon device tps389008\n");
> +
> +	dev_info(dev, "hwmon device tps389008 probed successfully\n");
> +
Noise; please drop.

> +	return 0;
> +}
> +
> +static void tps389008_remove(struct i2c_client *client)
> +{
> +	struct tps389008_data *tps = dev_get_drvdata(&client->dev);
> +
> +	hwmon_device_unregister(tps->hwmon);
> +
> +	mutex_destroy(&tps->dev_access_lock);
> +}
> +
> +static const struct i2c_device_id tps389008_ids[] = {
> +	{ "tps389008" },
> +	{ "tps389006" },
> +	{ "tps389004" },

The number of channels should be listed here for each chip to support defaults.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tps389008_ids);
> +
> +static const struct of_device_id tps389008_of_match[] = {
> +	{ .compatible = "ti,tps389008", },
> +	{ .compatible = "ti,tps389006", },
> +	{ .compatible = "ti,tps389004", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tps389008_of_match);
> +
> +static struct i2c_driver tps389008_i2c_driver = {
> +	.driver = {
> +		.name = "tps389008",
> +		.of_match_table = tps389008_of_match,
> +	},
> +	.probe = tps389008_probe,
> +	.remove = tps389008_remove,
> +	.id_table = tps389008_ids,
> +};
> +
> +module_i2c_driver(tps389008_i2c_driver);
> +
> +MODULE_AUTHOR("Flaviu Nistor <flaviu.nistor@gmail.com>");
> +MODULE_DESCRIPTION("TI TPS389008 voltage monitor driver");
> +MODULE_LICENSE("GPL");

[PATCH 2/2] dt-bindings: hwmon: Add TI TPS389008

[Flaviu Nistor]

Add device tree bindings and an example for the
TI TPS389008 voltage monitor.

Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
---
 .../bindings/hwmon/ti,tps389008.yaml          | 140 ++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
new file mode 100644
index 000000000000..5a32359894cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,tps389008.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TPS389008 voltage monitor
+
+maintainers:
+  - Flaviu Nistor <flaviu.nistor@gmail.com>
+
+description: |
+  Voltage monitor from TI (TPS389008, TPS389006, TPS389004) on I2C bus.
+
+  Datasheets:
+    https://www.ti.com/lit/ds/symlink/tps389006.pdf?ts=1741000787840&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FTPS389006
+
+properties:
+  compatible:
+    oneOf:
+      - const: ti,tps38908
+      - items:
+          - enum:
+              - ti,tps38908
+              - ti,tps38906
+              - ti,tps38904
+          - const: ti,tps38908
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    description: Required only if a child node is present.
+    const: 1
+
+  "#size-cells":
+    description: Required only if a child node is present.
+    const: 0
+
+patternProperties:
+  "^vmon@[1-8]$":
+    description: |
+      The node contains optional child nodes for up to x channels.
+      There are 8 channels for TPS389008, 6 channels for TPS389006,
+      and 4 channels for TPS389004. Each child node describes the information
+      of the input source. Input channels default to disabled in the chip.
+      Unless channels are explicitly enabled in device-tree,
+      input channels will be disabled.
+    type: object
+    additionalProperties: false
+    properties:
+      reg:
+        description: |
+          Must be 1 to 8, corresponding to the VMONx
+          ports of the TPS389008, TPS389006, or TPS389004, respectively.
+        enum: [ 1, 2, 3, 4, 5, 6, 7, 8 ]
+
+      label:
+        description: Name of the input source.
+
+      ti,vrange-mult-4x:
+        description: |
+          Must have values 5 or 20 in order for the analog
+          conversion to be done correctly.
+          Value 5 indicates multiply by 1 (x1), and value 20 indicates
+          multiply by 4 (x4).
+        type: boolean
+
+      ti,vmon-enable:
+        description: |
+          Sets each channel status. By default, all input channels are disabled.
+            - 0 = channel disabled
+            - 1 = channel active
+        type: boolean
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        vmon@37 {
+            compatible = "ti,tps389008", "ti,tps389006", "ti,tps389004";
+            reg = <0x37>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            vmon1: vmon1@1 {
+                reg = <0x1>;
+                label = "input1";
+                ti,vrange-mult-4x;
+                ti,vmon-enable;
+            };
+            vmon2: vmon2@2 {
+                reg = <0x2>;
+                label = "input2";
+                ti,vrange-mult-4x;
+                ti,vmon-enable;
+            };
+            vmon3: vmon3@3 {
+                reg = <0x3>;
+                label = "input3";
+                ti,vmon-enable;
+            };
+            vmon4: vmon4@4 {
+                reg = <0x4>;
+                label = "input4";
+                ti,vmon-enable;
+            };
+            vmon5: vmon5@5 {
+                reg = <0x5>;
+                label = "input5";
+                status = "disabled";
+            };
+            vmon6: vmon6@6 {
+                reg = <0x6>;
+                label = "input6";
+                ti,vmon-enable;
+            };
+            vmon7: vmon7@7 {
+                reg = <0x7>;
+                label = "input7";
+                ti,vmon-enable;
+            };
+            vmon8: vmon8@8 {
+                reg = <0x8>;
+                label = "input8";
+                ti,vmon-enable;
+            };
+        };
+    };
-- 
2.43.0

Re: [PATCH 2/2] dt-bindings: hwmon: Add TI TPS389008

[Rob Herring (Arm)]

On Wed, 19 Mar 2025 19:02:29 +0200, Flaviu Nistor wrote:
> Add device tree bindings and an example for the
> TI TPS389008 voltage monitor.
> 
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
> ---
>  .../bindings/hwmon/ti,tps389008.yaml          | 140 ++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tps389008.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/hwmon/ti,tps389008.example.dtb: /example-0/i2c/vmon@37: failed to match any schema with compatible: ['ti,tps389008', 'ti,tps389006', 'ti,tps389004']
Documentation/devicetree/bindings/hwmon/ti,tps389008.example.dtb: /example-0/i2c/vmon@37: failed to match any schema with compatible: ['ti,tps389008', 'ti,tps389006', 'ti,tps389004']
Documentation/devicetree/bindings/hwmon/ti,tps389008.example.dtb: /example-0/i2c/vmon@37: failed to match any schema with compatible: ['ti,tps389008', 'ti,tps389006', 'ti,tps389004']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250319170234.63723-3-flaviu.nistor@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/2] dt-bindings: hwmon: Add TI TPS389008

[Krzysztof Kozlowski]

On 19/03/2025 18:02, Flaviu Nistor wrote:
> Add device tree bindings and an example for the
> TI TPS389008 voltage monitor.
> 
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
> ---

This wasn't ever tested, so limited review.

BTW, bindings are before the user.

> +properties:
> +  compatible:
> +    oneOf:
> +      - const: ti,tps38908
> +      - items:
> +          - enum:
> +              - ti,tps38908
> +              - ti,tps38906
> +              - ti,tps38904
> +          - const: ti,tps38908
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    description: Required only if a child node is present.

Drop description

> +    const: 1
> +
> +  "#size-cells":
> +    description: Required only if a child node is present.

Drop description


> +    const: 0
> +
> +patternProperties:
> +  "^vmon@[1-8]$":

s/vmon/channel/

> +    description: |
> +      The node contains optional child nodes for up to x channels.
> +      There are 8 channels for TPS389008, 6 channels for TPS389006,
> +      and 4 channels for TPS389004. Each child node describes the information

Then you need allOf:if:then: disallowing these for specific variants.

> +      of the input source. Input channels default to disabled in the chip.
> +      Unless channels are explicitly enabled in device-tree,
> +      input channels will be disabled.
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      reg:
> +        description: |
> +          Must be 1 to 8,

Don't repeat constraints in free form text.

> corresponding to the VMONx
> +          ports of the TPS389008, TPS389006, or TPS389004, respectively.

Drop entire description

> +        enum: [ 1, 2, 3, 4, 5, 6, 7, 8 ]
> +
> +      label:
> +        description: Name of the input source.
> +
> +      ti,vrange-mult-4x:
> +        description: |
> +          Must have values 5 or 20 in order for the analog
> +          conversion to be done correctly.
> +          Value 5 indicates multiply by 1 (x1), and value 20 indicates
> +          multiply by 4 (x4).
> +        type: boolean
> +
> +      ti,vmon-enable:

Drop. Status tells it, no?

> +        description: |
> +          Sets each channel status. By default, all input channels are disabled.
> +            - 0 = channel disabled
> +            - 1 = channel active
> +        type: boolean
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        vmon@37 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "ti,tps389008", "ti,tps389006", "ti,tps389004";

Test your code before sending, not after.



Best regards,
Krzysztof