Re: [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator - sashiko-bot

From: sashiko-bot@kernel.org
To: "Lakshay Piplani" <lakshay.piplani@nxp.com>
Cc: robh@kernel.org, devicetree@vger.kernel.org,
	linux-i3c@lists.infradead.org, Frank.Li@kernel.org,
	conor+dt@kernel.org
Subject: Re: [PATCH v11 5/9] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
Date: Fri, 12 Jun 2026 11:41:01 +0000	[thread overview]
Message-ID: <20260612114102.1697A1F00A3A@smtp.kernel.org> (raw)
In-Reply-To: <20260612111816.3688240-6-lakshay.piplani@nxp.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly binds to any generic I3C hub device on the bus.
- [Low] The driver bypasses the I3C subsystem abstraction by including <linux/i3c/master.h> instead of <linux/i3c/device.h>.
--

commit 3682ca3c66e35b06e88823e8081c57b3ea1d4084
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator

Add core MFD support for the NXP P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841)
family of multiport I3C hub devices. These devices connect to a host via
I3C/I2C/SMBus and expose multiple downstream target ports.

> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..0a80992d6250a
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025-2026 NXP
> + * P3H2X4X i3c hub and regulator device.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i3c/master.h>

[Severity: Low]
Does this include bypass the I3C subsystem abstraction? By including the
master header, the driver has access to the internal definition of
struct i3c_device. Should this include <linux/i3c/device.h> instead?

[ ... ]

> +static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
> +{
> +	struct p3h2x4x_dev *ddata;
> +	int ret;
> +
> +	ddata = devm_kzalloc(&i3cdev->dev, sizeof(*ddata), GFP_KERNEL);

[Severity: Low]
This directly accesses the internal dev field of struct i3c_device. Should
this use i3cdev_to_dev(i3cdev) to obtain the underlying struct device pointer
to maintain API boundaries?

[ ... ]

> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> +	I3C_CLASS(I3C_DCR_HUB, NULL),
> +	{ /* sentinel */ },
> +};

[Severity: High]
Will this driver inappropriately bind to any generic I3C hub device on the
bus?

Using I3C_CLASS(I3C_DCR_HUB, NULL) instructs the subsystem to match any
device that advertises the generic Hub DCR (194), without filtering by
vendor or part ID.

Because the probe function doesn't verify the Manufacturer ID, it seems this
could eagerly bind to third-party hubs and attempt NXP-specific register
operations on them.

Should this use I3C_DEVICE(I3C_VENDOR_NXP, ...) to exclusively match the
supported NXP devices instead of the generic hub class?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=5

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c