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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Driver-specific shared header `p3h2840.h` is improperly placed in the global `include/linux/mfd/` directory instead of being local to `drivers/mfd/`.
- [Low] `struct i2c_device_id` array uses a positional initializer instead of a named initializer.
--

--- Patch [4]: [PATCH v15 4/8] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator ---
commit b12649d8bd1516b7d9cfa69f8a2a7bc8f4a10164
Author: Aman Kumar Pandey <[email protected]>

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. It provides driver support using both I3C and I2C
communication interfaces, allowing sub-drivers to be enabled independently.

> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..6c497d84e2bda
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
[ ... ]
> +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[] = {
> +	{ "nxp-i3c-hub" },

[Severity: Low]
Does this code use a positional initializer instead of a named initializer?

According to I2C subsystem guidelines, initialized arrays of type
struct i2c_device_id must use named initializers (e.g., .name = "nxp-i3c-hub").

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, p3h2x4x_i2c_id_table);
[ ... ]
> 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
[ ... ]
> +struct p3h2x4x_i3c_hub_dev;
> +
> +struct p3h2x4x {
> +	struct i3c_device *i3cdev;
> +	struct regmap *regmap;

[Severity: Low]
Is it necessary to place this driver-specific shared header in the global
include/linux/mfd/ directory?

The MFD subsystem guidelines recommend keeping driver-specific header files
local to drivers/mfd/ if they are only used by the parent and its immediate
children.

> +	/* 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;
> +};
> +#endif /* _LINUX_MFD_P3H2840_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c