Re: [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller

[Philipp Zabel <p.zabel@pengutronix.de>]

On Di, 2026-09-01 at 19:35 +0800, Billy Tsai wrote:
> The AST2700 I3C controller carries an ASPEED vendor extended
> capability describing an in-house control block and a PHY programming
> window. Recognize the ASPEED MIPI vendor ID in the extended capability
> parser and cache both register bases via the generic vendor_data
> pointer, which other vendors (e.g. NXP) also populate; add is_aspeed()
> to identify ASPEED specifically.
> 
[...]
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5

Drop, see 1e035297ffda ("docs: remove Assisted-by references").

> ---
>  drivers/i3c/master/mipi-i3c-hci/Makefile        |   2 +-
>  drivers/i3c/master/mipi-i3c-hci/cmd_v1.c        |  21 +++
>  drivers/i3c/master/mipi-i3c-hci/core.c          | 175 ++++++++++++++++--
>  drivers/i3c/master/mipi-i3c-hci/ext_caps.c      |  15 ++
>  drivers/i3c/master/mipi-i3c-hci/ext_caps.h      |   1 +
>  drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 234 ++++++++++++++++++++++++
>  drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 174 ++++++++++++++++++
>  7 files changed, 606 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 2290a889701cd..e0ee8148aa2f8 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
[...]
> @@ -1146,10 +1235,17 @@ static int i3c_hci_init(struct i3c_hci *hci)
>  	return i3c_hci_reset_and_init(hci);
>  }
>  
> +static void i3c_hci_dma_rst_assert(void *data)
> +{
> +	reset_control_assert(data);
> +}
> +
>  static int i3c_hci_probe(struct platform_device *pdev)
>  {
>  	const struct mipi_i3c_hci_platform_data *pdata = pdev->dev.platform_data;
>  	struct clk_bulk_data *clks;
> +	irq_handler_t irq_handler;
> +	unsigned long irq_flags;
>  	struct i3c_hci *hci;
>  	int irq, ret;
>  
> @@ -1176,8 +1272,12 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  	}
>  
>  	platform_set_drvdata(pdev, hci);
> -	/* temporary for dev_printk's, to be replaced in i3c_master_register */
> +	/*
> +	 * Temporary for dev_printk's and is_aspeed(), both replaced with
> +	 * the real values by i3c_master_register().
> +	 */
>  	hci->master.dev.init_name = dev_name(&pdev->dev);
> +	hci->master.dev.of_node = pdev->dev.of_node;
>  
>  	hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
>  	if (!hci->quirks && platform_get_device_id(pdev))
> @@ -1189,6 +1289,40 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  					     "cannot set DMA mask\n");
>  	}
>  
> +	if (is_aspeed(hci)) {
> +		struct aspeed_i3c_vendor_data *vd;
> +
> +		vd = devm_kzalloc(&pdev->dev, sizeof(*vd), GFP_KERNEL);
> +		if (!vd)
> +			return -ENOMEM;
> +		hci->vendor_data = vd;
> +
> +		vd->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, "core");

vd->rst is only ever used in i3c_hci_probe(), this could be a local
variable.

> +		if (IS_ERR(vd->rst))
> +			return PTR_ERR(vd->rst);
> +		ret = reset_control_deassert(vd->rst);
> +		if (ret)
> +			return ret;

Is the core reset never asserted again on purpose? Otherwise you could
use devm_reset_control_get_optional_exclusive_deasserted() to simplify
this.

> +
> +		vd->dma_rst = devm_reset_control_get_optional_shared(&pdev->dev, "dma");

vd->dma_rst is only ever used in i3c_hci_probe(), this could be a local
variable.

> +		if (IS_ERR(vd->dma_rst))
> +			return PTR_ERR(vd->dma_rst);
> +		if (vd->dma_rst) {
> +			ret = reset_control_deassert(vd->dma_rst);
> +			if (ret)
> +				return ret;
> +			ret = devm_add_action_or_reset(&pdev->dev,
> +						       i3c_hci_dma_rst_assert,
> +						       vd->dma_rst);
> +			if (ret)
> +				return ret;

This looks like it can be simplified with
devm_reset_control_get_optional_shared_deasserted().

> +		}
> +
> +		vd->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +		if (IS_ERR(vd->clk))
> +			return PTR_ERR(vd->clk);
> +	}
> +
>  	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
>  	if (ret < 0)
>  		return dev_err_probe(&pdev->dev, ret,
[...]
> diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
> new file mode 100644
> index 0000000000000..8760384d1c6f5
> --- /dev/null
> +++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
> @@ -0,0 +1,174 @@
[...]
> +struct aspeed_i3c_vendor_data {
> +	void __iomem *inhouse_regs;
> +	void __iomem *phy_regs;
> +	struct reset_control *rst;
> +	struct reset_control *dma_rst;

These two don't seem necessary.

> +	struct clk *clk;
> +};
> +

regards
Philipp