Re: [PATCH v6 7/7] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality

[Frank Li]

On Tue, Mar 10, 2026 at 12:27:27PM +0530, Lakshay Piplani wrote:
> From: Aman Kumar Pandey <aman.kumarpandey@xxxxxxx>
>
> Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
> These devices support downstream target ports that can be configured
> as I3C, I2C, or SMBus.
>
> This driver enables:
> - I3C/I2C communication between host and hub
> - Transparent communication with downstream devices
> - Target port configuration (I3C/I2C/SMBus)
> - MCTP device support
> - In-band interrupt handling
>
> P3H2440/P3H2441 support 4 target ports.
> P3H2840/P3H2841 support 8 target ports.
>
> Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@xxxxxxx>
> Signed-off-by: Vikash Bansal <vikash.bansal@xxxxxxx>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@xxxxxxx>
>
> ---
...
> +
> +static int find_close(int val, const int *tbl, int size)
> +{
> +	int best = 0, i;
> +
> +	for (i = 1; i < size; i++)
> +		if (abs(tbl[i] - val) < abs(tbl[best] - val))
> +			best = i;
> +
> +	return best;
> +}
> +

There find_closest() in
/ include / linux / util_macros.h

Can you reuse it?

> +static u8 p3h2x4x_pullup_dt_to_reg(int dt_value)
> +{
> +	return find_close(dt_value, p3h2x4x_pullup_tbl,
> +			  ARRAY_SIZE(p3h2x4x_pullup_tbl));
> +}
> +
> +static u8 p3h2x4x_io_strength_dt_to_reg(int dt_value)
> +{
> +	return find_close(dt_value, p3h2x4x_io_strength_tbl,
> +			  ARRAY_SIZE(p3h2x4x_io_strength_tbl));
> +}
> +
...
> +
> +	p3h2x4x_i3c_hub->rp3h2x4x.rcp0 = devm_regulator_get_optional(dev, "vcc1");
> +	if (IS_ERR(p3h2x4x_i3c_hub->rp3h2x4x.rcp0)) {
> +		p3h2x4x_i3c_hub->rp3h2x4x.rcp0 = NULL;
> +		dev_dbg(dev, "vdd1-supply not found\n");
> +	}
> +
> +	p3h2x4x_i3c_hub->rp3h2x4x.rcp1 = devm_regulator_get_optional(dev, "vcc2");
> +	if (IS_ERR(p3h2x4x_i3c_hub->rp3h2x4x.rcp1)) {
> +		p3h2x4x_i3c_hub->rp3h2x4x.rcp1 = NULL;
> +		dev_dbg(dev, "vdd2-supply not found\n");
> +	}
> +
> +	p3h2x4x_i3c_hub->rp3h2x4x.rtp0145 = devm_regulator_get_optional(dev, "vcc3");
> +	if (IS_ERR(p3h2x4x_i3c_hub->rp3h2x4x.rtp0145)) {
> +		p3h2x4x_i3c_hub->rp3h2x4x.rtp0145 = NULL;
> +		dev_dbg(dev, "vdd3-supply not found\n");
> +	}
> +
> +	p3h2x4x_i3c_hub->rp3h2x4x.rtp2367 = devm_regulator_get_optional(dev, "vcc4");
> +	if (IS_ERR(p3h2x4x_i3c_hub->rp3h2x4x.rtp2367)) {
> +		p3h2x4x_i3c_hub->rp3h2x4x.rtp2367 = NULL;
> +		dev_dbg(dev, "vdd4-supply not found\n");
> +	}
> +
> +	/* Enable regulators */
> +	if (p3h2x4x_i3c_hub->rp3h2x4x.rcp0) {
> +		ret = regulator_enable(p3h2x4x_i3c_hub->rp3h2x4x.rcp0);

can you use devm_regulator_get_enable_optional() to combine get and enable
by one call?

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 000000000000..833bc0bebc4e
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> @@ -0,0 +1,119 @@
...
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
> +	struct i3c_master_controller *parent = i3c_dev_get_master(p3h2x4x_hub->i3cdev->desc);
> +	u8 tp, ntwk_mask = 0;
> +	int ret;
> +
> +	p3h2x4x_hub->hub = i3c_hub_init(parent,
> +					&p3h2x4x_hub_ops,
> +					p3h2x4x_hub->i3cdev);
> +
> +	if (IS_ERR(p3h2x4x_hub->hub))
> +		return PTR_ERR(p3h2x4x_hub->hub);
> +
> +	for (tp = 0; tp < P3H2x4x_TP_MAX_COUNT; tp++) {
> +		if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> +		    p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2x4x_TP_MODE_I3C)
> +			continue;
> +
> +		/* Assign DT node for this TP */
> +		p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
> +
> +		struct i3c_hub_controller *hub_controller =
> +				&p3h2x4x_hub->tp_bus[tp].hub_controller;
> +		struct i3c_master_controller *controller = &hub_controller->controller;
> +
> +		hub_controller->parent = parent;
> +		hub_controller->hub = p3h2x4x_hub->hub;
> +
> +		dev_set_drvdata(&controller->dev, hub_controller);
> +
> +		ret = i3c_master_register(controller,
> +					  p3h2x4x_hub->dev,
> +					  i3c_hub_master_ops(),
> +					  false);
> +
> +		if (ret)
> +			return ret;
> +
> +		/* Perform DAA */
> +		ret = i3c_master_do_daa(parent);
> +		if (ret)
> +			return ret;
> +
> +		ntwk_mask |= p3h2x4x_hub->tp_bus[tp].tp_mask;
> +		p3h2x4x_hub->tp_bus[tp].is_registered = true;
> +		p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> +	}
> +
> +	ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> +	if (ret)
> +		return ret;

Need unregister_i3c at error path, or add API devm_i3c_master_register().

> +
> +	ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(p3h2x4x_hub->regmap, P3H2x4x_TP_NET_CON_CONF, ntwk_mask);
> +}
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 000000000000..75803a59fd6c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 NXP
> + * This P3H2x4x driver file contain functions for SMBus/I2C virtual Bus creation and read/write.
> + */
> +#include <linux/mfd/p3h2840.h>
> +#include <linux/regmap.h>
> +
> +#include "p3h2840_i3c_hub.h"
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

why I3C_SLAVE here?

"	This enables Linux to act as an I2C slave device. Note that your I2C
        bus master driver also needs to support this functionality. Please
        read Documentation/i2c/slave-interface.rst for further details.
"

I2C_SLAVE is for slave mode of i2c

> +/**
> + * p3h2x4x_ibi_handler - IBI handler.
> + * @i3cdev: i3c device.
> + * @payload: two byte IBI payload data.
> + *
> + */
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> +			 const struct i3c_ibi_payload *payload)
> +{
> +	u32 payload_byte_one = (*(int *)payload->data);
> +
> +	if (!(payload_byte_one & P3H2x4x_SMBUS_AGENT_EVENT_FLAG_STATUS))
> +		return;
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

The same here

> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(&i3cdev->dev);
> +	u32 target_port_status, payload_byte_two;
> +	u32 ret, i;
> +
> +	if (!p3h2x4x_i3c_hub || !p3h2x4x_i3c_hub->regmap)
> +		return;
> +
...
> +/**
> + * p3h2x4x_tp_i2c_xfer_msg() - This starts a SMBus write transaction by writing a descriptor
> + * and a message to the p3h2x4x registers. Controller buffer page is determined by multiplying the
> + * target port index by four and adding the base page number to it.
> + */
> +static int p3h2x4x_tp_i2c_xfer_msg(struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub,
> +				   struct i2c_msg *xfers,
> +				   u8 target_port,
> +				   u8 nxfers_i, u8 rw)
> +{
> +	u8 controller_buffer_page = P3H2x4x_CONTROLLER_BUFFER_PAGE + 4 * target_port;
> +	u8 target_port_status = P3H2x4x_TP0_SMBUS_AGNT_STS + target_port;
> +	u8 desc[P3H2x4x_SMBUS_DESCRIPTOR_SIZE] = { 0 };
> +	u8 transaction_type = P3H2x4x_SMBUS_400kHz;
> +	int write_length, read_length;
> +	u8 addr = xfers[nxfers_i].addr;
> +	u8 rw_address = 2 * addr;
> +	int ret;
> +
> +	if (rw == 2) { /* write and read */
> +		write_length = xfers[nxfers_i].len;
> +		read_length =  xfers[nxfers_i + 1].len;
> +	} else if (rw == 1) {
> +		rw_address |= P3H2x4x_SET_BIT(0);
> +		write_length = 0;
> +		read_length =  xfers[nxfers_i].len;
> +	} else {
> +		write_length = xfers[nxfers_i].len;
> +		read_length = 0;
> +	}
> +
> +	desc[0] = rw_address;
> +	if (rw == 2)
> +		desc[1] = transaction_type | P3H2x4x_SET_BIT(0);
> +	else
> +		desc[1] = transaction_type;
> +	desc[2] = write_length;
> +	desc[3] = read_length;

can you define index 0, 1, 2, 3 as enum

Frank
> +
> +	ret = regmap_write(p3h2x4x_i3c_hub->regmap, target_port_status,
> +			   P3H2x4x_TP_BUFFER_STATUS_MASK);
> +	if (ret)
> +		goto out;
> +

> +static struct i2c_algorithm p3h2x4x_tp_i2c_algorithm = {
> +	.master_xfer    = p3h2x4x_tp_i2c_xfer,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	.reg_slave = p3h2x4x_tp_i2c_reg_slave,
> +	.unreg_slave = p3h2x4x_tp_i2c_unreg_slave,
> +#endif
> +	.functionality  = p3h2x4x_tp_smbus_funcs,
> +};
> +
> +/**
> --
> 2.25.1
>