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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] regulator: nex10000ub: fatal error on missing optional enable-gpios and unsafe flags
- [High] regulator: nex10000ub: bogus regmap enable operations due to missing descriptor fields
--

commit 0697e8f972779073ad47133056d9851d3d08dfc2
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.

[ ... ]

> --- /dev/null
> +++ b/drivers/regulator/nex10000ub-regulator.c

[ ... ]

> +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_OUT_LOW, desc->name);
> +	if (IS_ERR(ena_gpiod))
> +		return PTR_ERR(ena_gpiod);

[Severity: High]
If the enable-gpios property is omitted in the device tree, does this cause
fwnode_gpiod_get_index() to return -ENOENT and cause DT initialization to fail
entirely?

Also, does requesting the GPIO with GPIOD_OUT_LOW instead of GPIOD_ASIS force
the output low during boot, potentially disabling the display abruptly?

Finally, if the VPOS and VNEG regulators share the same physical enable pin
(which is common for dual-output PMICs), will omitting
GPIOD_FLAGS_BIT_NONEXCLUSIVE cause a failure with -EBUSY?

> +
> +	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,				\
> +	}

[Severity: High]
Since nex10000ub_regulator_ops uses regulator_enable_regmap and
regulator_is_enabled_regmap, will omitting enable_reg and enable_mask here
cause them to default to 0?

If so, won't regulator_is_enabled_regmap() erroneously read the VPOS voltage
setting register (0x00), mask it with 0, and incorrectly return disabled? It
also seems regulator_enable_regmap() would generate useless I2C traffic
to register 0x00.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-topic-sm8x50-nex10000ub-v2-0-eeeaf9b0c913@linaro.org?part=3