Re: [PATCH v16 4/8] 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(s) to consider:
- [Low] MFD Architecture Violation: Global header placement and bespoke intermediate parent structure.
--

commit f74680421e4c370330ed051f68115c488cc48933
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
> index 0000000000000..f8cef54ce7062
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
> @@ -0,0 +1,168 @@
[ ... ]
> +/* Read port count from the device capability register (4- or 8-port variant). */
> +static int p3h2x4x_read_num_target_ports(struct device *dev,
> +					 struct p3h2x4x *ddata)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(ddata->regmap, P3H2X4X_DEV_CAPAB, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to read device capability\n");
> +
> +	ddata->num_target_ports = (val & P3H2X4X_TARGET_PORT_COUNT) ?
> +		P3H2X4X_TARGET_PORTS_8 : P3H2X4X_TARGET_PORTS_4;

[Severity: Low]
Does p3h2x4x_read_num_target_ports() violate the MFD subsystem guidelines by
initializing child-specific resources in the parent driver? Private resources
should be initialized directly in the child driver that consumes them.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/include/linux/mfd/p3h2840.h b/include/linux/mfd/p3h2840.h
> new file mode 100644
> index 0000000000000..eb4792de5440e
> --- /dev/null
> +++ b/include/linux/mfd/p3h2840.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2025-2026 NXP
> + */
> +
> +#ifndef _LINUX_MFD_P3H2840_H
> +#define _LINUX_MFD_P3H2840_H

[Severity: Low]
Is this driver-specific header file inappropriately placed in the global
MFD include path? Driver-specific headers only used by the parent and its
immediate children should remain local to the drivers/mfd/ directory.

> +
> +#include <linux/bits.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
[ ... ]
> +struct p3h2x4x_i3c_hub_dev;
> +
> +struct p3h2x4x {
> +	struct i3c_device *i3cdev;
> +	struct regmap *regmap;
> +	/* Number of downstream target ports (4 or 8). */
> +	u8 num_target_ports;
> +	/* Serializes protected register unlock/lock sequences across MFD children. */
> +	struct mutex protected_reg_lock;
> +	/* Hub context for the IBI handler to reach hub state via the parent i3cdev. */
> +	struct p3h2x4x_i3c_hub_dev *i3c_hub_priv;
> +};

[Severity: Low]
Does struct p3h2x4x aggregate child-specific data and create a bespoke
intermediate parent structure? This creates bidirectional coupling and bypasses
MFD isolation boundaries. Child drivers should directly access parent data using
standard APIs rather than relying on custom parent-initialized private
resources.

> +#endif /* _LINUX_MFD_P3H2840_H */

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