On Mon, Nov 17, 2014 at 03:40:23PM +0800, Flora Fu wrote:
This looks mostly good but there are a few fairly straightfoward things:
> @@ -725,5 +725,11 @@ config REGULATOR_WM8994
> This driver provides support for the voltage regulators on the
> WM8994 CODEC.
>
> +config REGULATOR_MT6397
> + tristate "MediaTek MT6397 PMIC"
> + depends on MFD_MT6397
> + help
> + This driver provides support for the voltage regulators on the MediaTek MT6397 PMIC.
> +
> endif
Keep this and the Makefile sorted.
> +static int mt6397_buck_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
> +{
> + vosel = info->buck_conf.vosel_reg;
> + voselon = info->buck_conf.voselon_reg;
> + vosel_mask = info->buck_conf.vosel_mask;
Please use the standard way of specifying data even if you can't use the
standard function.
> +
> + ret = regmap_update_bits(rdev->regmap, vosel, vosel_mask, sel);
> + if (ret != 0) {
> + dev_err(&rdev->dev, "Failed to update vosel: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(rdev->regmap, voselon, vosel_mask, sel);
> + if (ret != 0) {
> + dev_err(&rdev->dev, "Failed to update vosel_on: %d\n", ret);
> + return ret;
> + }
> + return 0;
You should add comments here explaining what's going on - it's very
strange to have to write the same value to two different registers and
the names of the registers look suspicously like this is something to do
with a suspend mode...
Missing blank line before the return too.
> +static int mt6397_buck_get_voltage_sel(struct regulator_dev *rdev)
> +{
You could use the regmap based helper for this.
> +static int mt6397_ldo_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
> +{
The LDO operations appear to be identical to the standard regmap
helpers, please use them.
> + if (is_fixed)
> + return 0;
You should use the standard fixed voltage regulator support rather than
> +static int mt6397_regulator_is_enabled(struct regulator_dev *rdev)
> +{
Again this looks like it should be using helpers.
> +#define MT6397_REGULATOR_OF_MATCH(_name, _id) \
> +[MT6397_ID_##_id] = { \
> + .name = #_name, \
> + .driver_data = &mt6397_regulators[MT6397_ID_##_id], \
> +}
Define regulators_node and of_match in the regulator desc and you can
remove both this table and all your DT matching code in the driver, the
core will handle it for you.
> + if ((reg_value & 0xFF) == MT6397_REGULATOR_ID91) {
> + j = MT6397_ID_VCAMIO;
> + mt6397_regulator_matches[j].init_data->constraints.min_uV =
> + 1000000;
> + mt6397_regulators[j].desc.volt_table = ldo_volt_table5_v2;
> + }
Use a switch statement, that way other variants can be added more
easily.
Attachment:
signature.asc
Description: Digital signature
