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.
Attachment:
signature.asc
Description: PGP signature
