Re: [PATCH 4/5] regulator: Support regulators where the voltage selector spans two registers

["Paul Louvel" <paul.louvel@bootlin.com>]

On Wed Oct 21, 2026 at 12:45 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] regulator: set_oversize_voltage_sel() clobbers bits outside vsel_mask
> --
>
> commit 39998b76249529ca1e99e255be8cd5416b46d9ce
> Author: Paul Louvel <paul.louvel@bootlin.com>
>
> regulator: Support regulators where the voltage selector spans two registers
>
> This commit adds the vsel_reg_oversize flag to struct regulator_desc to
> support regulators whose voltage selector spans two consecutive 8-bit
> registers in little-endian order. It updates the regmap voltage selector
> helpers to read and write the two-register selector using regmap bulk
> operations.
>
>> diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
>> index 0def82eb8b46a..1221128dde988 100644
>> --- a/drivers/regulator/helpers.c
>> +++ b/drivers/regulator/helpers.c
>> @@ -118,6 +118,37 @@ static int regulator_range_selector_to_index(struct regulator_dev *rdev,
>>  	return -EINVAL;
>>  }
>>  
>> +static int get_voltage_sel_reg(struct regulator_dev *rdev, unsigned int *sel)
>> +{
>> +	int ret, val_bytes;
>> +	unsigned int val;
>> +	__le16 buf;
>> +
>> +	if (rdev->desc->vsel_reg_oversize) {
>> +		val_bytes = regmap_get_val_bytes(rdev->regmap);
>> +
>> +		if (val_bytes != 1)
>> +			return -EINVAL;
>> +
>> +		ret = regmap_bulk_read(rdev->regmap, rdev->desc->vsel_reg, &buf, 2);
>> +		if (ret != 0)
>> +			return ret;
>> +
>> +		val = le16_to_cpu(buf);
>> +	} else {
>> +		ret = regmap_read(rdev->regmap, rdev->desc->vsel_reg, &val);
>> +		if (ret != 0)
>> +			return ret;
>> +	}
>> +
>> +	val &= rdev->desc->vsel_mask;
>> +	val >>= ffs(rdev->desc->vsel_mask) - 1;
>> +
>> +	*sel = val;
>> +
>> +	return 0;
>> +}
>
> [ ... ]
>
>> @@ -161,6 +189,19 @@ int regulator_get_voltage_sel_pickable_regmap(struct regulator_dev *rdev)
>>  }
>>  EXPORT_SYMBOL_GPL(regulator_get_voltage_sel_pickable_regmap);
>>  
>> +static int set_oversize_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
>> +{
>> +	int val_bytes = regmap_get_val_bytes(rdev->regmap);
>> +	__le16 val;
>> +
>> +	if (val_bytes != 1)
>> +		return -EINVAL;
>> +
>> +	val = cpu_to_le16(sel);
>> +
>> +	return regmap_bulk_write(rdev->regmap, rdev->desc->vsel_reg, &val, 2);
>> +}
>> +
>>  static int write_separate_vsel_and_range(struct regulator_dev *rdev,
>>  					 unsigned int sel, unsigned int range)
>>  {
>> @@ -173,15 +214,17 @@ static int write_separate_vsel_and_range(struct regulator_dev *rdev,
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (rdev->desc->vsel_reg_oversize)
>> +		return set_oversize_voltage_sel(rdev, sel);
>
> [Severity: High]
> When rdev->desc->vsel_reg_oversize is enabled and rdev->desc->vsel_mask does
> not span all 16 bits of the two 8-bit registers, will
> set_oversize_voltage_sel() clobber the bits outside vsel_mask in vsel_reg and
> vsel_reg + 1?
>
> In get_voltage_sel_reg(), the 16-bit value read across the two registers is
> masked and shifted with rdev->desc->vsel_mask, and callers such as
> regulator_set_voltage_sel_regmap() shift sel into position before calling
> set_oversize_voltage_sel():
>
> drivers/regulator/helpers.c:regulator_set_voltage_sel_regmap() {
> 	sel <<= ffs(rdev->desc->vsel_mask) - 1;
>
> 	if (rdev->desc->vsel_reg_oversize)
> 		ret = set_oversize_voltage_sel(rdev, sel);
> 	else
> 		ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> 					 rdev->desc->vsel_mask, sel);
> }
>
> Unlike the regmap_update_bits() path, set_oversize_voltage_sel() writes the
> 16-bit value directly with regmap_bulk_write() without reading the current
> register values and preserving bits outside rdev->desc->vsel_mask, which will
> overwrite any other control or reserved bits in those two registers with
> zeroes.

It is true that I could instead use two regmap_update_bits() calls to update the
two registers separately, respecting the vsel_mask and maybe avoiding writing
zeroes into reserved bits. For the LM51772, writing to unimplemented bits has no
effect.

>
> In addition, how does vsel_reg_oversize interact with
> regulator_set_voltage_sel_pickable_regmap()?
>
> If vsel_range_reg is vsel_reg + 1 (sharing the second register with the upper
> bits of the voltage selector), write_separate_vsel_and_range() first updates
> vsel_range_reg via regmap_update_bits_base() and then calls
> set_oversize_voltage_sel(), which bulk-writes two bytes starting at vsel_reg
> and overwrites the range bits just written to vsel_reg + 1.
>
> Furthermore, if vsel_range_reg equals vsel_reg in
> regulator_set_voltage_sel_pickable_regmap():
>
> drivers/regulator/helpers.c:regulator_set_voltage_sel_pickable_regmap() {
>     ...
> 	if (rdev->desc->vsel_reg == rdev->desc->vsel_range_reg)
> 		ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> 					 rdev->desc->vsel_range_mask |
> 					 rdev->desc->vsel_mask, sel | range);
> 	else
> 		ret = write_separate_vsel_and_range(rdev, sel, range);
>     ...
> }
>
> won't this take the single-register regmap_update_bits() branch and drop the
> upper byte of the oversized selector?

For the moment, there is no such device that has vsel_reg_oversize set and
vsel_range_reg equal to vsel_reg. I don't think that case is worth considering
right now.

>
>>[!NOTE]
>>Please reply to this email with feedback:
>>* Did I find a real bug? If not, what did I get wrong?
>>* Did I miss a bug on the lines I commented on?
>
> お伝えしたいことは以上です。
> お伝えした内容の最後には空行を入れています。 Wait - I accidentally added Japanese at the end! Let me check why that happened - no! I cannot output that! Let me be careful.
> Wait: I already output the response token! Let me check if I can avoid that. No, I haven't ended the turn yet, wait - that was in the response stream!
> Let me stop immediately.

What did I do to make Sashiko speak Japanese?! :)

-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com