Message-ID: <0dd3aa96-13c9-4b2d-902a-21ab8baec170@roeck-us.net>
Date: Thu, 4 Sep 2025 11:34:40 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: gfuchedgi@...il.com
Cc: Robert Marko <robert.marko@...tura.hr>,
	Luka Perkov <luka.perkov@...tura.hr>,
	Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-hwmon@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] hwmon: (tps23861) add class restrictions and
 semi-auto mode support

On Thu, Sep 04, 2025 at 10:33:45AM -0700, Gregory Fuchedgi via B4 Relay wrote:
> From: Gregory Fuchedgi <gfuchedgi@...il.com>
> 
> This commit adds optional individual per-port device tree nodes, in which
> ports can be:
>   - restricted by class. For example, ti,class = <2> for a port would only
>     enable power if class 1 or class 2 were negotiated. Requires interrupt
>     handler to be configured if class != 4 (max supported). This is because
>     hardware cannot be set with acceptable classes in advance. So the
>     driver would enable Semi-Auto mode and in the interrupt handler would
>     check negotiated class against device tree config and enable power only
>     if it is acceptable class.
>   - fully disabled. For boards that do not use all 4 ports. This would put
>     disabled ports in Off state and would hide corresponding hwmon files.
>   - off by default. The port is kept disabled, until enabled via
>     corresponding in_enable in sysfs.
> 
> The driver keeps current behavior of using Auto mode for all ports if no
> per-port device tree nodes given.
> 
> This commit also adds optional reset and ti,ports-shutdown pin support:
>   - if reset pin is configured the chip will be reset in probe.
>   - if both reset and shutdown pins are configured the driver would keep
>     ports shut down while configuring the tps23861 over i2c. Having both
>     shutdown and reset pins ensures no PoE activity happens while i2c
>     configuration is happening.
> 
> Tested with hw poe tester:
>  - Auto mode tested with no per-port DT settings as well as explicit port
>    DT ti,class=4. Tested that no IRQ is required in this case.
>  - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>  - Tested current cut-offs in Semi-Auto mode.
>  - On/off by default setting tested for both Auto and Semi-Auto modes.
>  - Tested fully disabling the ports in DT.
>  - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>    with reset only, as well as with neither reset nor shutdown.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@...il.com>
> ---
>  Documentation/hwmon/tps23861.rst |   6 +-
>  drivers/hwmon/tps23861.c         | 249 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 249 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/hwmon/tps23861.rst b/Documentation/hwmon/tps23861.rst
> index 46d121ff3f316d977ab3d1ab4e0d7b7736a57c60..5d14f683d0de11091f4a845a78a613604cbde4cc 100644
> --- a/Documentation/hwmon/tps23861.rst
> +++ b/Documentation/hwmon/tps23861.rst
> @@ -22,9 +22,13 @@ and monitoring capabilities.
>  
>  TPS23861 offers three modes of operation: Auto, Semi-Auto and Manual.
>  
> -This driver only supports the Auto mode of operation providing monitoring
> +This driver supports Auto and Semi-Auto modes of operation providing monitoring
>  as well as enabling/disabling the four ports.
>  
> +Ports that do not have associated child device tree entry or do not restrict poe
> +class to 3 or lower will operate in Auto mode. Otherwise the port will port will
> +operate in Semi-Auto mode and require an interrupt pin.
> +
>  Sysfs entries
>  -------------
>  
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index 4cb3960d51704f6ad4106adc927f213d090c0f9a..c43c667c5d7e6f7813f8ac6bc5267cb8fe562f9c 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -10,13 +10,26 @@
>  #include <linux/bitfield.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/hwmon.h>
>  #include <linux/i2c.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  
> +#define MAX_SUPPORTED_POE_CLASS		4
> +#define INTERRUPT_STATUS		0x0
> +#define INTERRUPT_ENABLE		0x1
> +#define INTERRUPT_CLASS			BIT(4)
> +#define DETECTION_EVENT			0x5
> +#define POWER_STATUS			0x10
> +#define PORT_1_ICUT			0x2A
> +#define PORT_1_ICUT_MASK		GENMASK(2, 0)
> +#define RESET				0x1a
> +#define RESET_CLRAIN			0x80
>  #define TEMPERATURE			0x2c
>  #define INPUT_VOLTAGE_LSB		0x2e
>  #define INPUT_VOLTAGE_MSB		0x2f
> @@ -102,6 +115,12 @@
>  #define TPS23861_GENERAL_MASK_1		0x17
>  #define TPS23861_CURRENT_SHUNT_MASK	BIT(0)
>  
> +#define TPS23861_TIME_RESET_US		5
> +#define TPS23861_TIME_RESET_US_MAX	1000
> +
> +#define TPS23861_TIME_POWER_ON_RESET_MS 23
> +#define TPS23861_TIME_I2C_MS		20
> +
>  #define TEMPERATURE_LSB			652 /* 0.652 degrees Celsius */
>  #define VOLTAGE_LSB			3662 /* 3.662 mV */
>  #define SHUNT_RESISTOR_DEFAULT		255000 /* 255 mOhm */
> @@ -110,10 +129,28 @@
>  #define RESISTANCE_LSB			110966 /* 11.0966 Ohm*/
>  #define RESISTANCE_LSB_LOW		157216 /* 15.7216 Ohm*/
>  
> +struct tps23861_port_data { // From DT.

C comments or C++ comment, but not both.

> +	const char *label;
> +	/* Maximum class accepted by the port. The hardware cannot be

Standard multi-line comments, please. This is not the networking subsystem.

> +	 * preconfigured to accept certain class only, so classification
> +	 * interrupt handler is required to for power-on decision if class is
> +	 * not MAX_SUPPORTED_POE_CLASS.
> +	 */
> +	u32 class;
> +	/* true if the port is disabled in the DT */
> +	bool fully_disabled;
> +	/* true if the port shouldn't be enabled on driver init */
> +	bool off_by_default;
> +};
> +
>  struct tps23861_data {
>  	struct regmap *regmap;
>  	u32 shunt_resistor;
>  	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *shutdown_gpio;
> +	int irq;
> +	struct tps23861_port_data ports[TPS23861_NUM_PORTS];
>  };
>  
>  static const struct regmap_config tps23861_regmap_config = {
> @@ -205,13 +242,19 @@ static int tps23861_port_enable(struct tps23861_data *data, int channel)
>  	regval |= BIT(channel);
>  	regval |= BIT(channel + 4);
>  	err = regmap_write(data->regmap, DETECT_CLASS_RESTART, regval);
> -
> -	return err;
> +	if (err)
> +		return err;
> +	return regmap_write(data->regmap, RESET, RESET_CLRAIN);
>  }
>  
> -static umode_t tps23861_is_visible(const void *data, enum hwmon_sensor_types type,
> +static umode_t tps23861_is_visible(const void *input_data, enum hwmon_sensor_types type,
>  				   u32 attr, int channel)
>  {
> +	const struct tps23861_data *data = input_data;
> +
> +	/* Channel may be >=TPS23861_NUM_PORTS for common Input voltage */
> +	if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled)
> +		return 0;
>  	switch (type) {
>  	case hwmon_temp:
>  		switch (attr) {
> @@ -325,10 +368,15 @@ static int tps23861_read_string(struct device *dev,
>  				enum hwmon_sensor_types type,
>  				u32 attr, int channel, const char **str)
>  {
> +	struct tps23861_data *data = dev_get_drvdata(dev);
> +
>  	switch (type) {
>  	case hwmon_in:
>  	case hwmon_curr:
> -		*str = tps23861_port_label[channel];
> +		if (channel < TPS23861_NUM_PORTS)
> +			*str = data->ports[channel].label;
> +		else
> +			*str = tps23861_port_label[channel];
>  		break;
>  	case hwmon_temp:
>  		*str = "Die";
> @@ -502,17 +550,155 @@ static int tps23861_port_status_show(struct seq_file *s, void *data)
>  
>  DEFINE_SHOW_ATTRIBUTE(tps23861_port_status);
>  
> +static inline bool tps23861_auto_mode(struct tps23861_port_data *port)
> +{
> +	return port->class == MAX_SUPPORTED_POE_CLASS;
> +}
> +
> +static irqreturn_t tps23861_irq_handler(int irq, void *dev_id)
> +{
> +	struct tps23861_data *data = (struct tps23861_data *)dev_id;
> +	struct tps23861_port_data *ports = data->ports;
> +	struct device *dev = &data->client->dev;
> +
> +	unsigned int intr_status, status, class, i;
> +	unsigned int det_status = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, INTERRUPT_STATUS, &intr_status);
> +	if (ret || intr_status == 0)
> +		return IRQ_NONE;
> +	if (intr_status & INTERRUPT_CLASS) {
> +		regmap_read(data->regmap, DETECTION_EVENT, &det_status);
> +		for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +			if (tps23861_auto_mode(ports + i))
> +				continue;
> +			if (!(det_status & BIT(i + 4)))
> +				continue;
> +			ret = regmap_read(data->regmap, PORT_1_STATUS + i, &status);
> +			if (ret)
> +				continue;
> +			class = FIELD_GET(PORT_STATUS_CLASS_MASK, status);
> +			if (class == PORT_CLASS_0) {
> +				/* class 0 is same power as class 3, change 0 to
> +				 * 3 for easy comparison
> +				 */
> +				class = 3;
> +			}
> +			if (class == PORT_CLASS_UNKNOWN ||
> +			    class > MAX_SUPPORTED_POE_CLASS)
> +				continue;
> +			if (class > ports[i].class) {
> +				dev_info(dev, "%s class mismatch: got %d, want <= %d",
> +					 ports[i].label, class, ports[i].class);
> +				continue;

In an interrupt handler ? Please don't.

> +			}
> +			regmap_read(data->regmap, POWER_STATUS, &status);
> +			if (status & BIT(i))
> +				continue; /* already enabled */
> +			/* Set ICUT and poe+ according to class. Values in ICUT table happen
> +			 * to match class values, so just set class.
> +			 */
> +			regmap_update_bits(data->regmap,
> +					   PORT_1_ICUT + i / 2,
> +					   PORT_1_ICUT_MASK << ((i % 2) * 4),
> +					   class << ((i % 2) * 4));
> +			regmap_update_bits(data->regmap, POE_PLUS,
> +					   BIT(i + 4),
> +					   class > 3 ? BIT(i + 4) : 0);
> +			dev_info(dev, "%s class %d, enabling power",
> +				 ports[i].label, class);

Same here. If you want to have debugging messages, make it debug messages.

> +			regmap_write(data->regmap, POWER_ENABLE, BIT(i));
> +		}
> +	}
> +	regmap_write(data->regmap, RESET, RESET_CLRAIN);
> +	return IRQ_HANDLED;
> +}
> +
> +static int tps23861_reset(struct i2c_client *client)
> +{
> +	struct tps23861_data *data = i2c_get_clientdata(client);
> +	unsigned int val;
> +
> +	if (data->reset_gpio) {
> +		gpiod_direction_output(data->reset_gpio, true);
> +		/* If shutdown pin is defined, use it to keep ports off, while
> +		 * turning the chip on for i2c configuration.
> +		 */
> +		if (data->shutdown_gpio)
> +			gpiod_direction_output(data->shutdown_gpio, true);
> +		usleep_range(TPS23861_TIME_RESET_US, TPS23861_TIME_RESET_US_MAX);
> +		gpiod_set_value_cansleep(data->reset_gpio, false);
> +		msleep(TPS23861_TIME_POWER_ON_RESET_MS);
> +		if (data->shutdown_gpio)
> +			gpiod_set_value_cansleep(data->shutdown_gpio, false);
> +		msleep(TPS23861_TIME_I2C_MS);
> +	}
> +
> +	/* Check the device is responsive */
> +	return regmap_read(data->regmap, OPERATING_MODE, &val);
> +}
> +
> +static void tps23861_init_ports(struct i2c_client *client)
> +{
> +	struct tps23861_data *data = i2c_get_clientdata(client);
> +	struct tps23861_port_data *ports = data->ports;
> +	unsigned int i, mode;
> +
> +	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +		mode = ports[i].fully_disabled	     ? OPERATING_MODE_OFF :
> +		       tps23861_auto_mode(&ports[i]) ? OPERATING_MODE_AUTO :
> +						       OPERATING_MODE_SEMI;
> +		regmap_update_bits(data->regmap, OPERATING_MODE,
> +				   OPERATING_MODE_PORT_1_MASK << (2 * i),
> +				   mode << (2 * i));
> +		if (ports[i].fully_disabled)
> +			continue;
> +		if (ports[i].off_by_default)
> +			tps23861_port_disable(data, i);
> +		else
> +			tps23861_port_enable(data, i);
> +	}
> +}
> +
>  static int tps23861_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct tps23861_data *data;
> +	struct tps23861_port_data *ports;
>  	struct device *hwmon_dev;
> +	struct gpio_desc *gpio;
> +	struct device_node *child;
>  	u32 shunt_resistor;
> +	int ret;
> +	unsigned int i;
> +	bool need_irq = false;
> +	const char *hwmon_name = client->name;
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> +	ports = data->ports;
> +	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +		ports[i].label = tps23861_port_label[i];
> +		ports[i].class = MAX_SUPPORTED_POE_CLASS;
> +	}
> +
> +	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(gpio)) {
> +		devm_kfree(dev, data);
> +		return -EPROBE_DEFER;
> +	}
> +	data->reset_gpio = gpio;
> +
> +	gpio = devm_gpiod_get_optional(dev, "ti,ports-shutdown", GPIOD_ASIS);
> +	if (IS_ERR(gpio)) {
> +		devm_kfree(dev, data);
> +		return -EPROBE_DEFER;
> +	}
> +	data->shutdown_gpio = gpio;
> +
>  	data->client = client;
>  	i2c_set_clientdata(client, data);
>  
> @@ -521,6 +707,55 @@ static int tps23861_probe(struct i2c_client *client)
>  		dev_err(dev, "failed to allocate register map\n");
>  		return PTR_ERR(data->regmap);
>  	}
> +	ret = tps23861_reset(client);
> +	if (ret)
> +		return -ENODEV;
> +	for_each_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &i);
> +		if (ret || i >= TPS23861_NUM_PORTS) {
> +			dev_err(dev, "node %s must define 'reg' < %d\n",
> +				child->name, TPS23861_NUM_PORTS);

If it is an error, return with an error. If it not an error,
dev_err is not acceptable.

> +			continue;
> +		}
> +		if (!of_device_is_available(child)) {
> +			ports[i].fully_disabled = true;
> +			continue;
> +		}
> +		ports[i].off_by_default = of_property_read_bool(child, "ti,off-by-default");
> +		of_property_read_string(child, "label", &ports[i].label);
> +		of_property_read_u32(child, "ti,class", &ports[i].class);
> +		if (ports[i].class > MAX_SUPPORTED_POE_CLASS) {
> +			dev_err(dev, "%s invalid class, disabling\n", ports[i].label);
> +			ports[i].fully_disabled = true;
> +			continue;
> +		}
> +		if (ports[i].class == 0) {
> +			/* class 0 is same power as class 3, change 0 to 3 for
> +			 * easy comparison
> +			 */
> +			ports[i].class = 3;
> +		}
> +		need_irq = need_irq || !tps23861_auto_mode(&ports[i]);
> +		dev_info(dev, "%s: max class: %d, %s by default\n",
> +			 ports[i].label, ports[i].class,
> +			 ports[i].off_by_default ? "off" : "on");

Please drop to dev_dbg or remove.

> +	}
> +	if (need_irq) {
> +		data->irq = irq_of_parse_and_map(dev->of_node, 0);
> +		if (!data->irq) {
> +			dev_err(dev, "failed to configure irq\n");
> +			return -EINVAL;
> +		}
> +		ret = devm_request_threaded_irq(dev, data->irq, NULL,
> +						tps23861_irq_handler,
> +						IRQF_TRIGGER_FALLING,
> +						"tps23861_irq", data);
> +		if (ret) {
> +			dev_err(dev, "failed to request irq %d\n", data->irq);
> +			return ret;
> +		}
> +		regmap_write(data->regmap, INTERRUPT_ENABLE, INTERRUPT_CLASS);
> +	}
>  
>  	if (!of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &shunt_resistor))
>  		data->shunt_resistor = shunt_resistor;
> @@ -536,7 +771,11 @@ static int tps23861_probe(struct i2c_client *client)
>  				TPS23861_GENERAL_MASK_1,
>  				TPS23861_CURRENT_SHUNT_MASK);
>  
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +	of_property_read_string(dev->of_node, "label", &hwmon_name);
> +
> +	tps23861_init_ports(client);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_name,
>  							 data, &tps23861_chip_info,
>  							 NULL);
>  	if (IS_ERR(hwmon_dev))
> 
> -- 
> 2.43.0
> 
> 

Powered by blists - more mailing lists