Re: [PATCH v5 09/10] regulator: add mt6357 regulator

[Matti Vaittinen]

Hi Alexandre, All

Please, treat my review more as initiation for discussion than 'hard 
requirements' for this driver. I am in no point or no "confidence level" 
to give you any requirements ;)

On 11/16/22 14:33, Alexandre Mergnat wrote:
> From: Fabien Parent <fparent@xxxxxxxxxxxx>
>
> Add regulator driver for the MT6357 PMIC.
>
> Signed-off-by: Fabien Parent <fparent@xxxxxxxxxxxx>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> Signed-off-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> ---

//snip

> +/*
> + * MT6357 regulators' information
> + *
> + * @desc: standard fields of regulator description.
> + * @da_vsel_reg: Monitor register for query buck's voltage.
> + * @da_vsel_mask: Mask for query buck's voltage.
> + */
> +struct mt6357_regulator_info {
> +	struct regulator_desc desc;
> +	u32 da_vsel_reg;
> +	u32 da_vsel_mask;
> +};
> +

//snip

> +/**
> + * mt6357_get_buck_voltage_sel - get_voltage_sel for regmap users
> + *
> + * @rdev: regulator to operate on
> + *
> + * Regulators that use regmap for their register I/O can set the
> + * da_vsel_reg and da_vsel_mask fields in the info structure and
> + * then use this as their get_voltage_vsel operation.
> + */
> +static int mt6357_get_buck_voltage_sel(struct regulator_dev *rdev)
> +{
> +	int ret, regval;
> +	struct mt6357_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_read(rdev->regmap, info->da_vsel_reg, &regval);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev,
> +			"Failed to get mt6357 Buck %s vsel reg: %d\n",
> +			info->desc.name, ret);
> +		return ret;
> +	}
> +
> +	regval &= info->da_vsel_mask;
> +	regval >>= ffs(info->da_vsel_mask) - 1;
> +
> +	return regval;
> +}

If I read this right, the device has separate register(s) for writing 
and reading the voltage? I wonder if this is a completely unique setup?

If this is not unique, then it might be worth adding another field for 
'vsel_get' register and a flag in regulator desc - and modify the 
generic regmap helpers to handle this in common code if the special 
register? Not sure if this HW design is common enough to warrant the 
added confusion though. You and Mark may have more insight.

> +
> +static const struct linear_range buck_volt_range1[] = {
> +	REGULATOR_LINEAR_RANGE(518750, 0, 0x7f, 6250),
> +};
> +
> +static const struct linear_range buck_volt_range2[] = {
> +	REGULATOR_LINEAR_RANGE(500000, 0, 0x7f, 6250),
> +};
> +
> +static const struct linear_range buck_volt_range3[] = {
> +	REGULATOR_LINEAR_RANGE(500000, 0, 0x3f, 50000),
> +};
> +
> +static const struct linear_range buck_volt_range4[] = {
> +	REGULATOR_LINEAR_RANGE(1200000, 0, 0x7f, 12500),
> +};

I am unsure if we should aim for dropping the REGULATOR_LINEAR_RANGE() 
and using the LINEAR_RANGE(). If yes, then it might simplify things if 
new drivers used LINEAR_RANGE() from the day 1. If we don't, then it 
makes sense to keep consistently using REGULATOR_LINEAR_RANGE() for all 
of the drivers. I am not sure which way is the right way.

> +static int mt6357_regulator_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6357 = dev_get_drvdata(pdev->dev.parent);

I am unsure what data do you need from the parent. If it is just the 
regmap / device-tree node / device, then it does not (in my opinion) 
really warrant using parent's drvdata. One can often get away with the
dev_get_regmap(pdev->dev.parent, NULL).

Anyways, the driver looks good to me.

Yours,
	-- Matti Vaittinen

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~