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

Frank Li
2026-03-10T18:29:03Z

On Tue, Mar 10, 2026 at 12:27:27PM +0530, Lakshay Piplani wrote:
> From: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> 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>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
>
> ---
...
> +
> +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