Messages in this thread | | | Date | Wed, 13 Mar 2019 15:01:32 +0000 | From | Mark Brown <> | Subject | Re: [PATCH v2 6/9] regulator: mt6358: Add support for MT6358 regulator |
| |
On Mon, Mar 11, 2019 at 11:46:28AM +0800, Hsin-Hsiung Wang wrote:
> +static const u32 vefuse_voltages[] = { > + 1700000, 1800000, 1900000, > +};
This and a bunch of the other regulators look like they're just linear ranges and could use the linear range helpers.
> +static const u32 vcn33_bt_voltages[] = { > + 3300000, 3400000, 3500000, > +}; > + > +static const u32 vcn33_wifi_voltages[] = { > + 3300000, 3400000, 3500000, > +};
These also have the same range so no matter which helpers are used I'd expect the data to be shared, I think some of the other regulators might have shared tables but didn't confirm.
> +static inline unsigned int mt6358_map_mode(unsigned int mode) > +{ > + return mode == MT6358_BUCK_MODE_FORCE_PWM ? > + REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL; > +}
Please write normal if statements, it makes the code more readable.
> +static int mt6358_set_voltage_sel(struct regulator_dev *rdev, > + unsigned int selector) > +{ > + int idx, ret; > + const u32 *pvol; > + struct mt6358_regulator_info *info = rdev_get_drvdata(rdev); > + > + pvol = (const u32 *)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 something other devices might do - rather than implement it in the driver it would be a bit better to add it to helpers.c as a reusable function.
> +static int mt6358_get_buck_voltage_sel(struct regulator_dev *rdev) > +{ > + int ret, regval; > + struct mt6358_regulator_info *info = rdev_get_drvdata(rdev); > + > + ret = regmap_read(rdev->regmap, info->da_vsel_reg, ®val); > + if (ret != 0) { > + dev_info(&rdev->dev, > + "Failed to get mt6358 Buck %s vsel reg: %d\n", > + info->desc.name, ret); > + return ret; > + } > + > + ret = (regval >> info->da_vsel_shift) & info->da_vsel_mask; > + > + return ret; > +}
This is just the generic get_voltage_sel() as far as I can see?
> + /* Read PMIC chip revision to update constraints and voltage table */ > + if (regmap_read(mt6397->regmap, MT6358_SWCID, ®_value) < 0) { > + dev_err(&pdev->dev, "Failed to read Chip ID\n"); > + return -EIO; > + }
We never use the value we read here?
[unhandled content-type:application/pgp-signature] | |