Re: [PATCH v9 6/7] i3c: hub: Add support for the I3C interface in the I3C hub

[Frank Li]

On Mon, Apr 20, 2026 at 04:22:21PM +0530, Lakshay Piplani wrote:
> Add virtual I3C bus support for the hub and provide interface to enable
> or disable downstream 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>
>
> ---
> Changes in v9:
>  - No change
>
> Changes in v8:
>  - No change
>
> Changes in v7:
>  - Convert Kconfig option to tristate
>  - Fix signedness issue in return value
>  - Fix kernel-doc warnings
>
> Changes in v6:
>  - Add support for the generic I3C interface in the I3C Hub
> ---
> ---
>  MAINTAINERS             |   2 +
>  drivers/i3c/Kconfig     |  15 ++
>  drivers/i3c/Makefile    |   1 +
>  drivers/i3c/hub.c       | 460 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/i3c/hub.h | 107 ++++++++++
>  5 files changed, 585 insertions(+)
>  create mode 100644 drivers/i3c/hub.c
>  create mode 100644 include/linux/i3c/hub.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2119fadef7b..bb3e8e9674c4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19274,8 +19274,10 @@ L:	linux-kernel@xxxxxxxxxxxxxxx
>  L:	linux-i3c-owner@xxxxxxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> +F:	drivers/i3c/hub.c
>  F:	drivers/mfd/p3h2840.c
>  F:	drivers/regulator/p3h2840_i3c_hub_regulator.c
> +F:	include/linux/i3c/hub.h
>  F:	include/linux/mfd/p3h2840.h
>
>  NXP PF5300/PF5301/PF5302 PMIC REGULATOR DEVICE DRIVER
> diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
> index 626c54b386d5..65304b416bb4 100644
> --- a/drivers/i3c/Kconfig
> +++ b/drivers/i3c/Kconfig
> @@ -21,6 +21,21 @@ menuconfig I3C
>
>  if I3C
>  source "drivers/i3c/master/Kconfig"
> +
> +config I3C_HUB
> +	tristate "I3C Hub Support"
> +	depends on I3C
> +	help
> +	  Enable support for the I3C interface in hub devices.
> +
> +	  This option adds virtual I3C bus support for hubs by creating
> +	  virtual master controllers for downstream ports and forwarding
> +	  bus operations through the hub device. It also provides an
> +	  interface used by hub drivers to enable or disable downstream
> +	  ports during bus transactions.
> +
> +	  Say Y here if your platform includes an I3C hub device
> +
>  endif # I3C
>
>  config I3C_OR_I2C
> diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> index 11982efbc6d9..9ddee56a6338 100644
> --- a/drivers/i3c/Makefile
> +++ b/drivers/i3c/Makefile
> @@ -2,3 +2,4 @@
>  i3c-y				:= device.o master.o
>  obj-$(CONFIG_I3C)		+= i3c.o
>  obj-$(CONFIG_I3C)		+= master/
> +obj-$(CONFIG_I3C_HUB)		+= hub.o
> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 000000000000..67c5d2695a61
> --- /dev/null
> +++ b/drivers/i3c/hub.c
> @@ -0,0 +1,460 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2026 NXP
> + * Generic I3C Hub core implementing virtual controller operations.
> + */
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/hub.h>
> +
> +#include "internals.h"
> +
> +/**
> + * i3c_hub_master_bus_init() - Bind controller to hub device
> + * @controller: Virtual controller for a hub port
> + *
> + * Associates the virtual controller with the hub device descriptor so that
> + * transfers are executed through the hub on the parent bus.
> + */
> +static int i3c_hub_master_bus_init(struct i3c_master_controller *controller)
> +{
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_hub *hub;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return -ENODEV;
> +
> +	hub = hub_controller->hub;
> +
> +	if (!hub->hub_dev)
> +		return -ENODEV;
> +
> +	controller->this = hub->hub_dev->desc;
> +	return 0;
> +}
> +
> +static void i3c_hub_master_bus_cleanup(struct i3c_master_controller *controller)
> +{
> +	controller->this = NULL;
> +}
> +
> +static int i3c_hub_attach_i3c_dev(struct i3c_dev_desc *dev)
> +{
> +	return 0;
> +}
> +
> +static int i3c_hub_reattach_i3c_dev(struct i3c_dev_desc *dev, u8 old_dyn_addr)
> +{
> +	return 0;
> +}
> +
> +static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
> +{
> +}
> +
> +/**
> + * i3c_hub_do_daa() - Perform DAA via hub port
> + * @hub: Hub instance
> + * @controller: Virtual controller for a hub port
> + *
> + * Enables the port connection, performs DAA on the parent controller,
> + * then disables the connection.
> + */
> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> +			  struct i3c_master_controller *controller)
> +{
> +	int ret;
> +
> +	if (!hub || !hub->parent)
> +		return -ENODEV;
> +
> +	i3c_hub_enable_port(controller);
> +	ret = i3c_master_do_daa(hub->parent);
> +	i3c_hub_disable_port(controller);
> +
> +	return ret;
> +}
> +
> +static bool i3c_hub_supports_ccc_cmd(struct i3c_hub *hub,
> +				     const struct i3c_ccc_cmd *cmd)
> +{
> +	return i3c_master_supports_ccc_cmd(hub->parent, cmd);
> +}
> +
> +/**
> + * i3c_hub_send_ccc_cmd() - Send CCC through hub port
> + * @hub: Hub instance
> + * @controller: Virtual controller
> + * @cmd: CCC command
> + *
> + * Enables the port connection while issuing CCC on the parent controller.
> + */
> +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> +				struct i3c_master_controller *controller,
> +				struct i3c_ccc_cmd *cmd)
> +{
> +	int ret;
> +
> +	if (!hub || !hub->parent)
> +		return -ENODEV;
> +
> +	i3c_hub_enable_port(controller);
> +	ret = i3c_master_send_ccc_cmd(hub->parent, cmd);
> +	i3c_hub_disable_port(controller);
> +
> +	return ret;
> +}
> +
> +/**
> + * i3c_hub_master_priv_xfers() - Execute private transfers via hub
> + * @dev: Target device descriptor
> + * @xfers: Transfer array
> + * @nxfers: Number of transfers
> + * @mode: transfer mode (SDR, HDR, etc.)
> + *
> + * Handles address adjustment and forwards private transfers through the hub
> + * device.
> + */
> +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> +				     struct i3c_xfer *xfers,
> +				     int nxfers,
> +				     enum i3c_xfer_mode mode)
> +{
> +	struct i3c_master_controller *controller = i3c_dev_get_master(dev);
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_dev_desc *hub_dev;
> +	u8 hub_addr, target_addr;
> +	struct i3c_hub *hub;
> +	int ret;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return -ENODEV;
> +
> +	hub = hub_controller->hub;
> +
> +	if (!hub->hub_dev)
> +		return -ENODEV;
> +
> +	hub_dev = hub->hub_dev->desc;
> +
> +	i3c_hub_enable_port(controller);
> +
> +	hub_addr = hub_dev->info.dyn_addr ?
> +		   hub_dev->info.dyn_addr : hub_dev->info.static_addr;
> +
> +	target_addr = dev->info.dyn_addr ?
> +		      dev->info.dyn_addr : dev->info.static_addr;
> +
> +	if (hub_addr != target_addr) {
> +		hub_dev->info.dyn_addr = target_addr;
> +		ret = i3c_master_reattach_i3c_dev(hub_dev, target_addr);
> +		if (ret)
> +			goto disable;
> +	}
> +
> +	ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
> +
> +	if (hub_addr != target_addr) {
> +		hub_dev->info.dyn_addr = hub_addr;
> +		ret |= i3c_master_reattach_i3c_dev(hub_dev, hub_addr);
> +	}
> +
> +disable:
> +	i3c_hub_disable_port(controller);
> +	return ret;
> +}
> +
> +static int i3c_hub_attach_i2c_dev(struct i2c_dev_desc *dev)
> +{
> +	return 0;
> +}
> +
> +static void i3c_hub_detach_i2c_dev(struct i2c_dev_desc *dev)
> +{
> +}
> +
> +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> +			     struct i2c_msg *xfers, int nxfers)
> +{
> +	return 0;
> +}
> +
> +static int i3c_hub_master_do_daa(struct i3c_master_controller *controller)
> +{
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_hub *hub;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return -ENODEV;
> +
> +	hub = hub_controller->hub;
> +
> +	return i3c_hub_do_daa(hub, controller);
> +}
> +
> +static int i3c_hub_master_send_ccc_cmd(struct i3c_master_controller *controller,
> +				       struct i3c_ccc_cmd *cmd)
> +{
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_hub *hub;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return -ENODEV;
> +
> +	hub = hub_controller->hub;
> +
> +	if (!hub->parent)
> +		return -ENODEV;
> +
> +	if (cmd->id == I3C_CCC_RSTDAA(true))
> +		return 0;
> +
> +	return i3c_hub_send_ccc_cmd(hub, controller, cmd);
> +}
> +
> +static bool i3c_hub_master_supports_ccc_cmd(struct i3c_master_controller *controller,
> +					    const struct i3c_ccc_cmd *cmd)
> +{
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_hub *hub;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return false;
> +
> +	hub = hub_controller->hub;
> +
> +	return i3c_hub_supports_ccc_cmd(hub, cmd);
> +}
> +
> +/**
> + * i3c_hub_request_ibi() - Request IBI through parent controller
> + * @desc: Target device descriptor
> + * @req: IBI setup
> + *
> + * Temporarily updates parent controller context to request IBI for a device
> + * connected through the hub.
> + */
> +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> +			       const struct i3c_ibi_setup *req)
> +{
> +	struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> +	struct i3c_hub_controller *hub_controller;
> +	struct i3c_master_controller *orig_parent;
> +	struct i3c_master_controller *parent;
> +	struct i3c_hub *hub;
> +	int ret;
> +
> +	hub_controller = dev_get_drvdata(&controller->dev);
> +	if (!hub_controller || !hub_controller->hub)
> +		return -ENODEV;
> +
> +	hub = hub_controller->hub;
> +
> +	if (!hub->parent)
> +		return -ENODEV;
> +
> +	parent = hub->parent;
> +
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +
> +	ret = i3c_master_direct_attach_i3c_dev(parent, desc);
> +	if (ret) {
> +		i3c_hub_restore_desc_parent(&desc->common, orig_parent);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&desc->ibi_lock);

why need unlock desc->ibi_lock firstly? If have to do that, need comments
because it is uncommon and try to avoid this kinds of situation.

Frank