Messages in this thread | | | Date | Tue, 31 Jan 2017 21:12:56 +0000 | From | Mark Brown <> | Subject | Re: [PATCH] regulator: cpcap: Add basic regulator support |
| |
On Mon, Jan 30, 2017 at 04:12:41PM -0800, Tony Lindgren wrote:
> +static int cpcap_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + unsigned int value = 0; > + > + regmap_read(rdev->regmap, rdev->desc->enable_reg, &value); > + > + return ((value & rdev->desc->enable_mask) == rdev->desc->enable_reg); > +}
Use the core support for this, reulator_is_enabled_regmap().
> + error = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > + rdev->desc->enable_mask, > + rdev->desc->enable_val); > + if (error) > + return error; > + > + if (rdev->desc->enable_val & CPCAP_REG_OFF_MODE_SEC) { > + error = regmap_update_bits(rdev->regmap, regulator->assign_reg, > + regulator->assign_mask, > + regulator->assign_mask); > + if (error) > + return error; > + }
What does this second register bit do?
> +static int cpcap_regulator_get_voltage(struct regulator_dev *rdev) > +{ > + struct cpcap_regulator *regulator = rdev_get_drvdata(rdev); > + unsigned int value = 0; > + int voltage;
regulator_get_voltage_sel_regmap() (the driver should just use selectors and let the mapping happen in the core).
> + regmap_read(rdev->regmap, rdev->desc->enable_reg, &value); > + > + if (!(value & rdev->desc->enable_mask)) > + return 0;
This is buggy, the driver should report the voltage the regulator will have when powered.
> +static int cpcap_regulator_set_voltage(struct regulator_dev *rdev, > + int min_uV, int max_uV, > + unsigned int *selector)
regulator_map_voltage_iterate() and regulator_set_voltage_regmap()
> + switch (mode) { > + case REGULATOR_MODE_NORMAL: > + value = CPCAP_BIT_AUDIO_LOW_PWR; > + break; > + case REGULATOR_MODE_IDLE: > + case REGULATOR_MODE_STANDBY: > + value = 0; > + break;
There should be a 1:1 mapping between modes and register values, if the device only supports two modes so should the driver.
> + init_data.constraints.valid_modes_mask = > + REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY; > + init_data.constraints.valid_ops_mask = > + REGULATOR_CHANGE_MODE | REGULATOR_CHANGE_STATUS; > + config.init_data = &init_data;
Drivers should not configure constraints, they're part of the system integration and come from the firmware or board file.
> + dev_info(&pdev->dev, "registering regulator %s\n", > + regulator->rdesc.name);
This is needless spam, please remove it. [unhandled content-type:application/pgp-signature] | |