Re: [PATCH v15 5/8] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] MFD subsystem guide violation: driver-specific header in global include directory and use of bespoke parent context structure.
- [Low] The `p3h2x4x_regulators` array of regulator descriptors is not marked `const`.
--

--- Patch [5]: [PATCH v15 5/8] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub ---
commit 6bd5ced43bd70804c772227fde0c55395341c77d
Author: Aman Kumar Pandey <[email protected]>

regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub

This patch introduces a new regulator driver for the NXP P3H2x4x I3C hub. It
registers the LDOs using the MFD framework and exposes them via the regulator
subsystem.

> diff --git a/drivers/regulator/p3h2840_i3c_hub_regulator.c b/drivers/regulator/p3h2840_i3c_hub_regulator.c
> new file mode 100644
> index 0000000000000..645d46ad7c2f2
> --- /dev/null
> +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025-2026 NXP
> + * NXP P3H2X4X regulator driver file contain functions for enable/disable regulator
> + * and voltage set/get.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/mfd/p3h2840.h>

[Severity: Medium]
Should this header be kept local to drivers/mfd/ instead of placing it in the
global include/linux/mfd/ directory? The MFD subsystem guidelines recommend
against placing driver-specific header files in the global include directory
if they are only used by the parent and its immediate children.

> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>

[ ... ]

> +static const unsigned int p3h2x4x_voltage_table[] = {
> +	1000000,
> +	1100000,
> +	1200000,
> +	1800000,
> +};
> +
> +static struct regulator_desc p3h2x4x_regulators[] = {

[Severity: Low]
Could this array be marked const? Because devm_regulator_register() accepts a
const struct regulator_desc pointer, making this array const would move it to
the read-only .rodata section.

> +	{
> +		.name = "ldo-cp0",
> +		.of_match = of_match_ptr("ldo-cp0"),
> +		.regulators_node = of_match_ptr("regulators"),

[ ... ]

> +	{
> +		.name = "ldo-tpg1",
> +		.of_match = of_match_ptr("ldo-tpg1"),
> +		.regulators_node = of_match_ptr("regulators"),
> +		.volt_table = p3h2x4x_voltage_table,
> +		.n_voltages = ARRAY_SIZE(p3h2x4x_voltage_table),
> +		.ops = &p3h2x4x_ldo_ops,
> +		.type = REGULATOR_VOLTAGE,
> +		.owner = THIS_MODULE,
> +		.enable_reg = P3H2X4X_LDO_AND_PULLUP_CONF,
> +		.enable_mask = P3H2X4X_TP2367_EN_LDO,
> +		.vsel_reg = P3H2X4X_VCCIO_LDO_CONF,
> +		.vsel_mask = P3H2X4X_TP2367_VCCIO_LDO_VOLTAGE_MASK,
> +	},
> +};
> +
> +static int p3h2x4x_regulator_probe(struct platform_device *pdev)
> +{
> +	struct p3h2x4x *p3h2x4x = dev_get_drvdata(pdev->dev.parent);

[Severity: Medium]
Is it possible to use standard APIs to access the required state rather than
exposing the bespoke parent struct p3h2x4x to the child drivers? 

The MFD subsystem guidelines discourage exposing bespoke parent structures for
parent-child data sharing to maintain proper layering.

> +	struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
> +	struct regulator_config rcfg = { };
> +	struct device *dev = &pdev->dev;

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

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