Re: [PATCH v12 4/7] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

On Wed, Jun 17, 2026 at 11:17:43AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Concurrent regulator operations race on the shared device-level protection register, leading to silent hardware configuration failures.
> - [Medium] The driver module will not auto-load because it lacks a MODULE_ALIAS or platform_device_id table.
> - [Medium] If regmap_read or the unlock sequence fails in the guard constructor, the driver proceeds with the regulator operation on locked registers.
> - [Low] Typo in regulator config assignment assigns dev->of_node to itself instead of rcfg.of_node.
> --
>
> commit e68081b58005a2422d441a5611dac29f8cffe2f4
> Author: Aman Kumar Pandey <[email protected]>
>
> regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
>
> The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
> functionality. This driver registers the regulators using the MFD
> framework and exposes them via the regulator subsystem.
>
> > --- /dev/null
> > +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
>
> [ ... ]
>
> > +static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
> > +				    struct p3h2x4x_reg_state *state)
> > +{
> > +	state->restore = false;
> > +
> > +	if (regmap_read(rdev->regmap,
> > +			P3H2X4X_DEV_REG_PROTECTION_CODE,
> > +			&state->orig))
> > +		return;
>
> [Severity: Medium]
> If regmap_read() fails here, the function returns early and swallows the
> error since the guard constructor returns void. Will this cause the caller
> to proceed with regulator operations on locked hardware, potentially
> resulting in silent hardware configuration failures?
>
> If the device ACKs the transaction despite ignoring the write, wouldn't
> the driver incorrectly report success to the regulator core?
>
> > +
> > +	if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> > +		regmap_write(rdev->regmap,
> > +			     P3H2X4X_DEV_REG_PROTECTION_CODE,
> > +			     P3H2X4X_REGISTERS_UNLOCK_CODE);
> > +		state->restore = true;
> > +	}
> > +}
> > +
> > +static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
> > +				   struct p3h2x4x_reg_state *state)
> > +{
> > +	if (state->restore)
> > +		regmap_write(rdev->regmap,
> > +			     P3H2X4X_DEV_REG_PROTECTION_CODE,
> > +			     state->orig);
> > +}
> > +
> > +DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
> > +		    p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
> > +		    p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
> > +		    struct p3h2x4x_reg_state state);
>
> [Severity: High]
> Does this guard provide mutual exclusion across the four different regulators
> on the same device?
>
> The regmap lock protects individual I/O operations, but since there is no
> device-level lock held across the unlock/modify/lock sequence, can concurrent
> operations on different regulators interleave?

Look like make sense, need use mutex to protect this sequency.

>
> For example, could one thread re-lock the device in p3h2x4x_reg_guard_exit()
> while another thread is just about to write its configuration, causing the
> hardware write to be silently ignored? Is a shared device-level mutex needed
> to serialize these sequences?
>
> [ ... ]
>
> > +static int p3h2x4x_regulator_probe(struct platform_device *pdev)
> > +{
> > +	struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
> > +	struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
> > +	struct regulator_config rcfg = { };
> > +	struct device *dev = &pdev->dev;
> > +	struct regulator_dev *rdev;
> > +	int i;
> > +
> > +	p3h2x4x_regulator = devm_kzalloc(dev, sizeof(*p3h2x4x_regulator), GFP_KERNEL);
> > +	if (!p3h2x4x_regulator)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, p3h2x4x_regulator);
> > +
> > +	p3h2x4x_regulator->regmap = p3h2x4x->regmap;
> > +	device_set_of_node_from_dev(dev, dev->parent);
> > +
> > +	rcfg.dev = dev;
> > +	rcfg.dev->of_node = dev->of_node;
>
> [Severity: Low]
> Should this be rcfg.of_node = dev->of_node? Since rcfg.dev is assigned to dev
> on the previous line, this just assigns dev->of_node to itself.

Although it is small problem, look likes not necesary to set of_node here.
It'd better to fix it at next version.

Frank

>
> > +	rcfg.regmap = p3h2x4x_regulator->regmap;
> > +	rcfg.driver_data = p3h2x4x_regulator;
>
> [ ... ]
>
> > +static struct platform_driver p3h2x4x_regulator_driver = {
> > +	.driver = {
> > +		.name = "p3h2x4x-regulator",
> > +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > +	},
> > +	.probe = p3h2x4x_regulator_probe,
> > +};
> > +module_platform_driver(p3h2x4x_regulator_driver);
> > +
> > +MODULE_AUTHOR("Aman Kumar Pandey <[email protected]>");
> > +MODULE_AUTHOR("Vikash Bansal <[email protected]>");
> > +MODULE_AUTHOR("Lakshay Piplani <[email protected]>");
> > +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB Regulator driver");
> > +MODULE_LICENSE("GPL");
>
> [Severity: Medium]
> Since the driver is registered as a platform driver with the name
> "p3h2x4x-regulator" but the module is compiled as
> "p3h2840_i3c_hub_regulator.ko", will this module fail to auto-load?
>
> Without a MODULE_ALIAS("platform:p3h2x4x-regulator") or an exported
> platform_device_id table, udev may not be able to load the module when the
> MFD core creates the child platform device.
>
> --
> 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