linux-kernel - Re: [PATCH v8 4/9] regulator: Add support for MediaTek MT6363 SPMI PMIC Regulators

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ac809179-7192-4218-8b13-86b9252652f4@collabora.com>
Date: Tue, 14 Oct 2025 13:35:37 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
 linux-mediatek@...ts.infradead.org
Cc: lee@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 matthias.bgg@...il.com, lgirdwood@...il.com, broonie@...nel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, kernel@...labora.com,
 wenst@...omium.org, igor.belwon@...tallysanemainliners.org
Subject: Re: [PATCH v8 4/9] regulator: Add support for MediaTek MT6363 SPMI
 PMIC Regulators

Il 09/10/25 16:41, Nicolas Frattaroli ha scritto:
> On Friday, 3 October 2025 11:11:53 Central European Summer Time AngeloGioacchino Del Regno wrote:
>> Add a driver for the regulators found on the MT6363 PMIC, fully
>> controlled by SPMI interface.
>> This PMIC regulates voltage with an input range of 2.6-5.0V, and
>> features 10 buck converters and 26 LDOs.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>>   drivers/regulator/Kconfig                  |  10 +
>>   drivers/regulator/Makefile                 |   1 +
>>   drivers/regulator/mt6363-regulator.c       | 935 +++++++++++++++++++++
>>   include/linux/regulator/mt6363-regulator.h | 330 ++++++++
>>   4 files changed, 1276 insertions(+)
>>   create mode 100644 drivers/regulator/mt6363-regulator.c
>>   create mode 100644 include/linux/regulator/mt6363-regulator.h
>>
>> [...]
>> diff --git a/drivers/regulator/mt6363-regulator.c b/drivers/regulator/mt6363-regulator.c
>> new file mode 100644
>> index 000000000000..812775072eb5
>> --- /dev/null
>> +++ b/drivers/regulator/mt6363-regulator.c
>> [...]
>> +
>> +static int mt6363_regulator_set_mode(struct regulator_dev *rdev,
>> +				     unsigned int mode)
>> +{
>> +	struct mt6363_regulator_info *info = rdev_get_drvdata(rdev);
>> +	struct regmap *regmap = rdev->regmap;
>> +	int cur_mode, ret;
>> +
>> +	if (!info->modeset_reg && mode == REGULATOR_MODE_FAST)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (mode) {
>> +	case REGULATOR_MODE_FAST:
>> +		ret = mt6363_buck_unlock(regmap, true);
>> +		if (ret)
>> +			break;
>> +
>> +		ret = regmap_set_bits(regmap, info->modeset_reg, info->modeset_mask);
>> +
>> +		mt6363_buck_unlock(regmap, false);
>> +		break;
>> +	case REGULATOR_MODE_NORMAL:
>> +		cur_mode = mt6363_regulator_get_mode(rdev);
>> +		if (cur_mode < 0) {
>> +			ret = cur_mode;
>> +			break;
>> +		}
>> +
>> +		if (cur_mode == REGULATOR_MODE_FAST) {
>> +			ret = mt6363_buck_unlock(regmap, true);
>> +			if (ret)
>> +				break;
>> +
>> +			ret = regmap_clear_bits(regmap, info->modeset_reg, info->modeset_mask);
>> +
>> +			mt6363_buck_unlock(regmap, false);
>> +			break;
>> +		} else if (cur_mode == REGULATOR_MODE_IDLE) {
>> +			ret = regmap_clear_bits(regmap, info->lp_mode_reg, info->lp_mode_mask);
>> +			if (ret == 0)
>> +				usleep_range(100, 200);
>> +		} else {
>> +			ret = 0;
> 
> Just initialise ret to 0 at the start of the function scope when
> you declare it. You've already missed an uninitialised use once,
> and playing these branch games is just asking for more trouble
> in the future. There's no micro-optimisation you're doing here,
> clang produces the same assembly for both zero initialised and
> the else branch version you're doing here.
> 

It's not about micro-optimization. Double initialization is a bad coding practice.

Cheers,
Angelo

>> +		}
>> +		break;
>> +	case REGULATOR_MODE_IDLE:
>> +		ret = regmap_set_bits(regmap, info->lp_mode_reg, info->lp_mode_mask);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	if (ret) {
>> +		dev_err(&rdev->dev, "Failed to set mode %u: %d\n", mode, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> [...]
> 
> Kind regards,
> Nicolas Frattaroli
> 
>