Re: [RFC PATCH 3/5] regulator: drivers: Add TI TPS65219 PMIC regulators support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Mon, Jun 13, 2022 at 11:06:02AM +0200, Jerome NEANNE wrote:

> +static int tps65219_pmic_enable(struct regulator_dev *dev)
> +{
> +	struct tps65219 *tps = rdev_get_drvdata(dev);
> +	int rid = rdev_get_id(dev);
> +	int ret;
> +
> +
> +

Lots of random blank space here.

> +	if (rid < TPS65219_BUCK_1 || rid > TPS65219_LDO_4)
> +		return -EINVAL;
> +

If a regulator doesn't support an operation it shouldn't be providing
it, no need to check here.

> +	ret = tps65219_set_bits(tps, dev->desc->enable_reg,
> +				 dev->desc->enable_mask, dev->desc->enable_mask);

If you provide the regmap to the child devices then this can use the
standard regmap helpers for most of the operations, meaning most of
the operations in the driver can be removed.

> +/* generic regulator_set_bypass_regmap does not match requirements use custom instead */

In what way?  Please also try to keep within 80 columns normally.

> +	if (dev->desc->ops->is_enabled) {
> +		dev_err(tps->dev, "%s LDO%d is enabled, should be shut down to set bypass ",
> +					 __func__, rid);
> +		return -EBUSY;
> +	}

This is testing if there is an enable operation, not if the regulator is
enabled.

> +	if (enable) {
> +		dev_dbg(tps->dev, "%s, LDO%d already in bypass mode", __func__, rid);
> +		return ret;
> +	}

This is not a problem, just silently succeed.

> +	/* Allocate memory for strobes */
> +	tps->strobes = devm_kcalloc(&pdev->dev,
> +				    TPS65219_NUM_REGULATOR, sizeof(u8),
> +				    GFP_KERNEL);
> +	if (!tps->strobes)
> +		return -ENOMEM;

These are never referenced anywhere else in the driver, no need to
allocate them.

> +		ret = regmap_read(tps->regmap, regulators[i].bypass_reg, &val);
> +		if (ret) {
> +			dev_err(tps->dev, "dev_err failed to map register for %s regulator\n",
> +				pdev->name);
> +			return ret;
> +		}

I'm not clear what this check is intended to do, it at least needs to be
better documented.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]