Re: [PATCH 3/3] regulator: add regulator driver for the Nexperia NEX10000UB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] regulator: nex10000ub: missing enable_reg and enable_mask breaks state tracking
- [High] regulator: nex10000ub: failing of_parse_cb on missing optional GPIO drops DT constraints
- [High] regulator: nex10000ub: GPIOD_ASIS fails to configure enable GPIO as output
--

commit b4dbdcd03d0963edf5545b4f355cad556309c6b2
Author: Neil Armstrong <neil.armstrong@linaro.org>

regulator: add regulator driver for the Nexperia NEX10000UB

Add regulator support for the Nexperia NEX10000UB dual output LCD
bias power supply which provides programmable positive and negative
output voltages mainly for display panels applications.

> diff --git a/drivers/regulator/nex10000ub-regulator.c b/drivers/regulator/nex10000ub-regulator.c
> new file mode 100644
> index 0000000000000..b45a805d5601d
> --- /dev/null
> +++ b/drivers/regulator/nex10000ub-regulator.c
[ ... ]
> +static const struct regulator_ops nex10000ub_regulator_ops = {
> +	.enable = regulator_enable_regmap,
> +	.disable = regulator_disable_regmap,
> +	.is_enabled = regulator_is_enabled_regmap,

[Severity: High]
Does this break the regulator core's state tracking?

The core regmap helpers regulator_enable_regmap, regulator_disable_regmap,
and regulator_is_enabled_regmap are registered here in
nex10000ub_regulator_ops, but NEX10000UB_REGULATOR_DESC below leaves
.enable_reg and .enable_mask zero-initialized.

When is_enabled is called, won't it bitwise AND the register value with a 0
mask, unconditionally returning 0? Similarly, enable and disable might
perform no-op writes with a 0 mask instead of properly utilizing the GPIOs.

> +	.list_voltage = regulator_list_voltage_linear,
> +	.map_voltage = regulator_map_voltage_linear,
> +	.get_voltage_sel = regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel = regulator_set_voltage_sel_regmap,
> +};
> +
> +static int nex10000ub_of_parse_cb(struct device_node *np,
> +				  const struct regulator_desc *desc,
> +				  struct regulator_config *config)
> +{
> +	struct gpio_desc *ena_gpiod;
> +
> +	ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np), "enable", 0,
> +					   GPIOD_ASIS, desc->name);

[Severity: High]
Does using GPIOD_ASIS fail to configure the enable GPIO as an output?

If the pin is configured as an input by the bootloader, it will remain an
input. The regulator core uses gpiod_set_value_cansleep(), which only sets
the output value and does not implicitly switch the pin's direction to
output. Could this prevent the device from actually turning on or off?

> +	if (IS_ERR(ena_gpiod))
> +		return PTR_ERR(ena_gpiod);

[Severity: High]
Does returning the error directly here drop device tree constraints if the
optional enable GPIO is missing?

If the optional enable-gpios is omitted from the device tree,
fwnode_gpiod_get_index() returns -ENOENT, which is then propagated back to
the core. The core treats this as a fatal DT parse error, frees the
init_data, and registers the regulator without its DT configuration,
discarding all voltage ranges and always-on properties.

> +
> +	config->ena_gpiod = ena_gpiod;
> +
> +	return 0;
> +}
> +
> +#define NEX10000UB_REGULATOR_DESC(_id, _name)			\
> +	[NEX10000UB_REGULATOR_ID_##_id] = {			\
> +		.name = "nex10000ub-"#_name,			\
> +		.supply_name = "vin",				\
> +		.id = NEX10000UB_REGULATOR_ID_##_id,		\
> +		.of_match = of_match_ptr(#_name),		\
> +		.of_parse_cb = nex10000ub_of_parse_cb,		\
> +		.ops = &nex10000ub_regulator_ops,		\
> +		.n_voltages = NEX10000UB_VOUT_N_VOLTAGE,	\
> +		.min_uV = NEX10000UB_VOUT_VMIN,			\
> +		.uV_step = NEX10000UB_VOUT_STEP,		\
> +		.enable_time = 2000,				\
> +		.vsel_mask = NEX10000UB_VOUT_MASK,		\
> +		.vsel_reg = NEX10000UB_REG_##_id,		\
> +		.type = REGULATOR_VOLTAGE,			\
> +		.owner = THIS_MODULE,				\
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-topic-sm8x50-nex10000ub-v1-0-5afe06bace03@linaro.org?part=3