Re: [PATCH v2 3/3] regulator: axp20x: add support for the AXP318W

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The macro `AXP_DESC_RANGES_DELAY_OPS` ignores its `_ops` parameter, discarding custom regulator operations.
- [Medium] In `axp318_threshold_check`, negative error codes from `is_enabled` are incorrectly treated as false (enabled).
- [Low] Spelling error in the minimum dropout comment block.
- [Low] Unused defined constants for the number of DCDC voltages.
--

--- Patch [3]: [PATCH v2 3/3] regulator: axp20x: add support for the AXP318W ---
commit f7b8096eeee3c9cefed0efe26a8d34e4c6a08d5a
Author: Andre Przywara <andre.przywara@arm.com>

regulator: axp20x: add support for the AXP318W

The patch introduces regulator support for the X-Powers AXP318W PMIC,
describing the chip's voltage settings, switch registers, and how the
voltages are encoded.

Reviewed-by: Mark Brown <broonie@kernel.org>
Co-developed-by: Jerome Brunet <jbrunet@baylibre.com>

> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index da891415efc0b..bc8d3aa2bd672 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
[ ... ]
> @@ -138,6 +138,31 @@
>  #define AXP313A_DCDC_V_OUT_MASK		GENMASK(6, 0)
>  #define AXP313A_LDO_V_OUT_MASK		GENMASK(4, 0)
>  
> +#define AXP318_DCDC1_V_OUT_MASK		GENMASK(4, 0)
> +#define AXP318_DCDC2_V_OUT_MASK		GENMASK(6, 0)
> +#define AXP318_LDO_V_OUT_MASK		GENMASK(4, 0)
> +#define AXP318_ELDO_V_OUT_MASK		GENMASK(5, 0)
> +#define AXP318_DCDC2_NUM_VOLTAGES	88
> +#define AXP318_DCDC6_NUM_VOLTAGES	128
> +#define AXP318_DCDC7_NUM_VOLTAGES	103
> +#define AXP318_DCDC8_NUM_VOLTAGES	119
> +#define AXP318_THRESHOLD_VOLTAGE	1540000
> +/*
> + * FIXME:
> + * Some LDOs of the AXP318 may be fed by different supplies and
> + * the documentation repeatidly warns that output voltage must

[Severity: Low]
This isn't a bug, but there's a typo here ("repeatidly" instead of
"repeatedly").

> + * be less than the supply, which is the case for any LDO really.
[ ... ]
> -#define AXP_DESC_RANGES_DELAY(_family, _id, _match, _supply, _ranges, _n_voltages,	\
> -			_vreg, _vmask, _ereg, _emask, _ramp_delay)	\
> +#define AXP_DESC_RANGES_DELAY_OPS(_family, _id, _match, _supply, _ranges,	\
> +				  _n_voltages, _vreg, _vmask, _ereg, _emask,	\
> +				  _ramp_delay, _ops)				\
>  	[_family##_##_id] = {							\
>  		.name		= (_match),					\
>  		.supply_name	= (_supply),					\
> @@ -436,10 +481,23 @@
>  		.enable_mask	= (_emask),					\
>  		.linear_ranges	= (_ranges),					\
>  		.n_linear_ranges = ARRAY_SIZE(_ranges),				\
> -		.ramp_delay = (_ramp_delay),					\
> +		.ramp_delay	= (_ramp_delay),				\
> -		.ops		= &axp20x_ops_range,				\
> +		.ops		= &axp20x_ops_range,				\

[Severity: High]
Is it intentional that the .ops field is hardcoded to &axp20x_ops_range here,
rather than using the _ops macro parameter?

This appears to silently discard the custom axp318_threshold_ops_range passed
in by AXP_DESC_RANGES_THRESHOLD, which would cause the protective voltage
threshold checks to be ignored when configuring the regulator.

>  	}
[ ... ]
> +static int axp318_threshold_check(struct regulator_dev *rdev,
> +				  unsigned int sel)
> +{
> +	int old_sel, old_uV, new_uV;
> +
> +	/* Only applicable if the regulator is enabled */
> +	if (!rdev->desc->ops->is_enabled(rdev))
> +		return 0;

[Severity: Medium]
Could this check inadvertently ignore I2C or regmap errors?

If regulator_is_enabled_regmap() fails and returns a negative error code like
-EIO, !(-EIO) evaluates to false. This means the function will continue
executing instead of propagating the error or returning early, potentially
leading to incorrect state transitions.

> +
> +	old_sel = rdev->desc->ops->get_voltage_sel(rdev);
[ ... ]
> +static const struct regulator_desc axp318_regulators[] = {
> +	AXP_DESC(AXP318, DCDC1, "dcdc1", "vin19", 1000, 3400, 100,
> +		 AXP318_DCDC1_CONTROL, AXP318_DCDC1_V_OUT_MASK,
> +		 AXP318_DCDC_OUTPUT_CONTROL1, BIT(0)),
> +	AXP_DESC_RANGES(AXP318, DCDC2, "dcdc2", "vin23",
> +			axp318_dcdc2_ranges, 88,

[Severity: Low]
Should these newly defined constants be used here?

The AXP318_DCDC2_NUM_VOLTAGES (and similar) macros were defined earlier in
this patch, but the array initialization seems to use hardcoded integers like
88 and 128 instead.

> +			AXP318_DCDC2_CONTROL, AXP318_DCDC2_V_OUT_MASK,
> +			AXP318_DCDC_OUTPUT_CONTROL1, BIT(1)),
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-axp318-regulator-v2-0-ee5f1c56b49f@baylibre.com?part=3