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

[Frank Li <Frank.li@nxp.com>]

On Fri, Feb 06, 2026 at 02:01:21PM +0200, Aman Kumar Pandey wrote:
> 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@nxp.com>
> Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
>
> ---
> Changes in v5:
>  - Updated supply names.
>
> Changes in v4:
>  - Split the driver into three separate patches (mfd, regulator and I3C hub)
>  - Added support for NXP P3H2x4x I3C hub functionality
>  - Integrated hub driver with its on-die regulator
>
> Changes in v3:
>  - Added MFD (Multi-Function Device) support for I3C hub and on-die regulator
>
> Changes in v2:
>  - Refined coding style and incorporated review feedback
>  - Updated directory structure
>  - Revised logic for parsing DTS nodes
> ---
> ---
>  MAINTAINERS                              |   1 +
>  drivers/i3c/Kconfig                      |   1 +
>  drivers/i3c/Makefile                     |   1 +
>  drivers/i3c/hub/Kconfig                  |  10 +
>  drivers/i3c/hub/Makefile                 |   4 +
>  drivers/i3c/hub/p3h2840_i3c_hub.h        | 340 ++++++++++++++++++
>  drivers/i3c/hub/p3h2840_i3c_hub_common.c | 405 ++++++++++++++++++++++
>  drivers/i3c/hub/p3h2840_i3c_hub_i3c.c    | 354 +++++++++++++++++++
>  drivers/i3c/hub/p3h2840_i3c_hub_smbus.c  | 418 +++++++++++++++++++++++
>  include/linux/i3c/device.h               |   1 +
>  10 files changed, 1535 insertions(+)
>  create mode 100644 drivers/i3c/hub/Kconfig
>  create mode 100644 drivers/i3c/hub/Makefile
>  create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub.h
>  create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_common.c
>  create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
>  create mode 100644 drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
>
...
> +
> +static struct platform_driver p3h2x4x_i3c_hub_driver = {
> +	.driver = {
> +		.name = "p3h2x4x-i3c-hub",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.probe = p3h2x4x_i3c_hub_probe,
> +	.remove = p3h2x4x_i3c_hub_remove,
> +};
> +module_platform_driver(p3h2x4x_i3c_hub_driver);
> +
> +MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
> +MODULE_AUTHOR("vikash Bansal <vikash.bansal@nxp.com>");

AI: Should be "Vikash Bansal" (capitalize first name)

> +MODULE_DESCRIPTION("P3H2x4x I3C HUB driver");
> +MODULE_LICENSE("GPL");
> 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..cbc6626721fe
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> @@ -0,0 +1,354 @@
...
> +
> +static int p3h2x4x_request_ibi(struct i3c_dev_desc *desc,
> +			       const struct i3c_ibi_setup *req)
> +{
> +	struct i3c_master_controller *parent = get_parent_controller_from_i3c_desc(desc);
> +	struct i3c_master_controller *orig_parent;
> +	int ret;
> +
> +	orig_parent = update_i3c_i2c_desc_parent(&desc->common, parent);
> +	ret = i3c_master_direct_attach_i3c_dev(parent, desc);
> +	if (ret) {
> +		restore_i3c_i2c_desc_parent(&desc->common, orig_parent);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&desc->ibi_lock);

supposed caller hold ibi_lock.

provide i3c_device_request_ibi_lock() version, which have not hold ibi_lock.

unlock and lock is not good enough.

> +	kfree(desc->ibi);
> +	desc->ibi = NULL;
> +	ret = i3c_device_request_ibi(desc->dev, req);
> +	mutex_lock(&desc->ibi_lock);
> +	restore_i3c_i2c_desc_parent(&desc->common, orig_parent);
> +
> +	return ret;
> +}
> +
...
> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	int ret_sum = 0, ret;
> +	u8 msg_count, rw;
> +
> +	struct tp_bus *bus = i2c_get_adapdata(adap);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = bus->p3h2x4x_i3c_hub;
> +
> +	guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
> +	guard(mutex)(&bus->port_mutex);
> +
> +	for (msg_count = 0; msg_count < num; msg_count++) {
> +		if (msgs[msg_count].len > P3H2x4x_SMBUS_PAYLOAD_SIZE) {
> +			dev_err(p3h2x4x_i3c_hub->dev,
> +				"Message nr. %d not sent - length over %d bytes.\n",
> +				msg_count, P3H2x4x_SMBUS_PAYLOAD_SIZE);
> +			continue;
> +		}
> +
> +		rw = msgs[msg_count].flags % 2;
> +		if (!rw) {
> +		/* If a read message is immediately followed by a write message to
> +		 * the same address,  consider combining them into a single transaction.
> +		 */
> +			if (msg_count < num && msgs[msg_count].addr == msgs[msg_count + 1].addr &&
> +			    (msgs[msg_count + 1].flags & I2C_M_RD)) {

AI:  `msg_count < num` should be `msg_count + 1 < num` to prevent out-of-bounds access

Frank
> +				rw = 2;
> +				msg_count += 1;
> +				ret_sum += 1;
> +			}
> +		}
> +
> +		ret = p3h2x4x_tp_i2c_xfer_msg(p3h2x4x_i3c_hub,
> +					      msgs,
> +					      bus->tp_port,
> +					      (rw == 2) ? (msg_count - 1) : msg_count,
> +						  rw);
> +		if (ret)
> +			return ret;
> +
> +		ret_sum++;
> +	}
> +	return ret_sum;
> +}
> +
...
> --
> 2.25.1
>