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

Mark Brown
2020-01-20T19:04:57Z

[-- Attachment #1.1: Type: text/plain, Size: 1654 bytes --]

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().

> +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;

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.

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

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]