Re: [PATCH v21 1/4] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] mfd: p3h2x4x: duplicated probe logic and unaddressed naming
  feedback
--

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

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

This commit adds core MFD support for the NXP P3H2x4x family of multiport
I3C hub devices over I3C and I2C.

> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..c489844821487
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c

[ ... ]

> +static int p3h2x4x_device_probe_i3c(struct i3c_device *i3cdev)
> +{
> +	struct device *dev = i3cdev_to_dev(i3cdev);
> +	struct i3c_device_info devinfo;
> +	struct p3h2x4x *ddata;
> +	int ret;
> +
> +	i3c_device_get_info(i3cdev, &devinfo);
> +	if (I3C_PID_MANUF_ID(devinfo.pid) != I3C_MANUF_ID_NXP)
> +		return -ENODEV;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(dev, &ddata->protected_reg_lock);
> +	if (ret)
> +		return ret;
> +
> +	i3cdev_set_drvdata(i3cdev, ddata);
> +
> +	ddata->regmap = devm_regmap_init_i3c(i3cdev, &p3h2x4x_regmap_config);
> +	if (IS_ERR(ddata->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ddata->regmap),
> +				     "Failed to register HUB regmap\n");
> +
> +	/* The hub child driver retrieves information from i3cdev. */
> +	ddata->i3cdev = i3cdev;
> +
> +	ret = p3h2x4x_read_num_target_ports(dev, ddata);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				   p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> +				   NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add sub devices\n");
> +
> +	return 0;
> +}
> +
> +static int p3h2x4x_device_probe_i2c(struct i2c_client *client)
> +{
> +	struct p3h2x4x *ddata;
> +	int ret;
> +
> +	ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(&client->dev, &ddata->protected_reg_lock);
> +	if (ret)
> +		return ret;
> +
> +	i2c_set_clientdata(client, ddata);
> +
> +	ddata->regmap = devm_regmap_init_i2c(client, &p3h2x4x_regmap_config);
> +	if (IS_ERR(ddata->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(ddata->regmap),
> +				     "Failed to register HUB regmap\n");
> +
> +	ddata->i3cdev = NULL;
> +
> +	ret = p3h2x4x_read_num_target_ports(&client->dev, ddata);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> +				   p3h2x4x_devs, ARRAY_SIZE(p3h2x4x_devs),
> +				   NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to add sub devices\n");
> +
> +	return 0;
> +}
> +
> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> +	I3C_CLASS(I3C_DCR_HUB, NULL),
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
> +
> +static const struct i2c_device_id p3h2x4x_i2c_id_table[] = {
> +	{ .name = "nxp-i3c-hub" },
> +	{ /* sentinel */ }
> +};

[ ... ]

> +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB multi function driver");
> +MODULE_LICENSE("GPL");

[Severity: Low]
As reported by Lee Jones on v7 [1], could the common initialization sequence
in p3h2x4x_device_probe_i3c() and p3h2x4x_device_probe_i2c() be factored into
a shared helper function so the bus-specific probe functions only handle bus
validation and regmap initialization?

In addition, p3h2x4x_i2c_id_table uses the generic name "nxp-i3c-hub" instead
of a device-specific name, MODULE_DESCRIPTION still refers to a "multi
function driver", and the file names p3h2840.c and p3h2840.h do not match
CONFIG_MFD_P3H2X4X and the p3h2x4x symbol prefix.

[1] https://lore.kernel.org/r/20260326115848.GL1141718@google.com

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

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