Re: [PATCH v3 6/6] regulator: mt6359: Add proper ldo_vcn33_[12] regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `mt6359_sync_vcn33_setting()` is ignored in `mt6359_regulator_probe()`.
- [Low] The logging messages in `mt6359_sync_vcn33_setting()` print incorrect 0-based regulator indices.
--

commit 6f24659e7932d4cb71b42c3442fde771560f8271
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: regulator: mt6359: Add proper ldo_vcn33_[12] regulators

This commit consolidates the separate Wi-Fi and Bluetooth regulators into
single hardware regulators for ldo_vcn33_1 and ldo_vcn33_2. It syncs the
enable bits and forces the Wi-Fi bits off to simplify handling, while
retaining dummy regulators for device tree backward compatibility.

> diff --git a/drivers/regulator/mt6359-regulator.c b/drivers/regulator/mt6359-regulator.c
> --- a/drivers/regulator/mt6359-regulator.c
> +++ b/drivers/regulator/mt6359-regulator.c

[ ... ]

> @@ -951,27 +949,114 @@ static const struct mt6359_regulator_info mt6359p_regulators[] = {

[ ... ]

> +static int mt6359_sync_vcn33_setting(struct device *dev, unsigned int idx)
> +{

[ ... ]

> +	for (unsigned int i = 0; i < ARRAY_SIZE(vcn33_regs[0]); i++) {
> +		u32 bt_en_mask = vcn33_regs[idx][i].bt_en_mask;
> +		u32 wifi_en_mask = vcn33_regs[idx][i].wifi_en_mask;
> +
> +		ret = regmap_read(mt6397->regmap, vcn33_regs[idx][i].wifi_en_reg, &val);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to read VCN33_%u_WIFI setting\n", i);

This isn't a bug, but since the loop index i starts at 0, will this print
VCN33_0_WIFI instead of VCN33_1_WIFI on the first iteration? Would it make
sense to use i + 1 here and in the subsequent error messages to accurately
match the hardware component names?

[ ... ]

> +		/* Disable VCN33_[12]_WIFI */
> +		ret = regmap_update_bits(mt6397->regmap, vcn33_regs[idx][i].wifi_en_reg,
> +					 wifi_en_mask, 0);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to disable VCN33_%u_WIFI\n", i);
> +	}
> +
> +	return 0;
> +}
> +
>  static int mt6359_regulator_probe(struct platform_device *pdev)
>  {
>  	struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent);

[ ... ]

>  	ret = regmap_read(mt6397->regmap, MT6359P_HWCID, &hw_ver);
>  	if (ret)
>  		return ret;
>  
> -	if (hw_ver >= MT6359P_CHIP_VER)
> +	if (hw_ver >= MT6359P_CHIP_VER) {
>  		mt6359_info = mt6359p_regulators;
> -	else
> +		mt6359_sync_vcn33_setting(&pdev->dev, 1);
> +	} else {
>  		mt6359_info = mt6359_regulators;
> +		mt6359_sync_vcn33_setting(&pdev->dev, 0);
> +	}

Should the return value of mt6359_sync_vcn33_setting() be checked here?

If the regmap operations inside the sync function fail, it returns a negative
error code via dev_err_probe(). By dropping the return value, the driver
silently continues to probe and register regulators on top of an
unsynchronized or inconsistent PMIC hardware state.

Could this return value be captured in the ret variable and propagated up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512085358.1693208-1-wenst@chromium.org?part=6