Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator

[Wen Su <Wen.Su@xxxxxxxxxxxx>]

Hi Mark,

On Mon, 2020-01-20 at 19:04 +0000, Mark Brown wrote:
> On Mon, Jan 20, 2020 at 03:47:29PM +0800, Wen Su wrote:
> 
> This seems pretty good, a few comments below but they're fairly small
> and should be easy to address:
> 
> > +static int mt6359_set_voltage_sel(struct regulator_dev *rdev,
> > +				  unsigned int selector)
> > +{
> > +	int idx, ret;
> > +	const u32 *pvol;
> > +	struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> > +
> > +	pvol = info->index_table;
> > +
> > +	idx = pvol[selector];
> > +	ret = regmap_update_bits(rdev->regmap, info->desc.vsel_reg,
> > +				 info->desc.vsel_mask,
> > +				 idx << info->vsel_shift);
> > +
> > +	return ret;
> > +}
> 
> This looks like you should be using regulator_list_voltage_table() and
> associated functions, probably map_voltage_ascend() or _iterate() and
> just a simple set_voltage_sel_regmap().
Thanks for your suggestion.
Currently it's using regulator_list_voltage_table() and
regulator_map_voltage_iterate() as below:

static const struct regulator_ops mt6359_volt_table_ops = {
	.list_voltage = regulator_list_voltage_table,
	.map_voltage = regulator_map_voltage_iterate,
	...
The reason to use mt6359_set_voltage_sel() is to convert selector value
to hardware register index value:
	idx = pvol[selector];

To avoid using mt6359_set_voltage_sel(), the *_voltages array need to be
filled with zeros as below: 
Current:
static const u32 vemc_voltages[] = {
	2900000, 3000000, 3300000,
};
static const u32 vemc_idx[] = {
	10, 11, 13,
};

change to:
static const u32 vxo22_voltages[] = {
	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2900000, 3000000, 0, 3300000,
};
> 
> > +static int mt6359_get_status(struct regulator_dev *rdev)
> > +{
> > +	int ret;
> > +	u32 regval;
> > +	struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> > +
> > +	ret = regmap_read(rdev->regmap, info->status_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;
Thanks for your suggestion.
I will update it in the next patch.
> 
> Please write normal conditionl statements rather than using the ternery
> operator to improve legibility.
> 
> > +	switch (mode) {
> > +	case REGULATOR_MODE_FAST:
> > +		if (curr_mode == REGULATOR_MODE_IDLE) {
> > +			WARN_ON(1);
> > +			dev_notice(&rdev->dev,
> > +				   "BUCK %s is LP mode, can't FPWM\n",
> > +				   rdev->desc->name);
> > +			return -EIO;
> 
> I'd expect the device to go out of low power mode then into force PWM
> mode if it has to do that rather than reject the operation.
The device low power mode may control by hardware pad, so that the
reason to reject the operation is the device low power mode can not go
out by software.
Another scenario is one user set the device to low power mode, we think
it's not suitable to change device mode to _FAST mode by another user.