Re: [PATCH v2 3/4] regulator: Add driver for MediaTek MT6328 PMIC regulators

[Mark Brown]

On Sun, May 31, 2026 at 11:10:44AM +0200, Yassine Oudjana via B4 Relay wrote:
> @@ -0,0 +1,500 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek MT6328 regulator driver
> + * Based on MT6323 driver.
> + *
> + * Copyright (c) 2016 MediaTek Inc.
> + * Copyright (c) 2022 Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static const unsigned int ldo_volt_table3[] = {
> +	0, 0, 0, 1800000, 1900000, 2000000, 2100000, 2200000
> +};

Use linear_min_sel for the first three values.

> +static const struct linear_range ldo_volt_range1[] = {
> +	REGULATOR_LINEAR_RANGE(700000, 0, 0x7f, 6250)
> +};

If there's just one range there's no need for a lookup table, use
regulator_map_voltage_linar().

> +static int mt6328_get_status(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u32 regval;
> +	struct mt6328_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_read(rdev->regmap, info->desc.enable_reg, &regval);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev, "Failed to get enable reg: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return (regval & info->qi) ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;
> +}

get_status() should report the actual status of the regulator, not what
was configured.  If the device can't report this just omit the
operation.

> +		if (mt6328_regulators[i].vselctrl_reg) {
> +			if (regmap_read(mt6328->regmap,
> +				mt6328_regulators[i].vselctrl_reg,
> +				&regval) < 0) {
> +				dev_err(&pdev->dev,
> +					"Failed to read buck ctrl\n");
> +				return -EIO;

Better to return the actual error.

> +		rdev = devm_regulator_register(&pdev->dev,
> +				&mt6328_regulators[i].desc, &config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(&pdev->dev, "failed to register %s\n",
> +				mt6328_regulators[i].desc.name);
> +			return PTR_ERR(rdev);

dev_err_probe().

Attachment: signature.asc
Description: PGP signature