On Mon, May 14, 2012 at 10:58:31AM +0900, Graeme Gregory wrote:
Looks good, quite a few of the things below are updates for very
recently introduced APIs.
> +static int palmas_smps_read(struct palmas *palmas, unsigned int reg,
> + unsigned int *dest)
> +{
> + int slave;
> + unsigned int addr;
> +
> + slave = PALMAS_BASE_TO_SLAVE(PALMAS_SMPS_BASE);
> + addr = PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, reg);
> +
> + return regmap_read(palmas->regmap[slave], addr, dest);
> +}
Looks like the LDO and SPMS regulators both use the same regmap?
> + .get_voltage = palmas_get_voltage_smps,
Why not get_voltage_sel?
> + .set_voltage_sel = palmas_set_voltage_smps_sel,
> + .list_voltage = palmas_list_voltage_smps,
Implementing map_voltage() too would be nice.
> +static int palmas_is_enabled_smps10(struct regulator_dev *dev)
> +{
> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
> + unsigned int reg;
> +
> + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_STATUS, ®);
> +
> + reg &= SMPS10_BOOST_EN;
> +
Should be able to use regulator_is_enabled_regmap() and friends.
> +static int palmas_list_voltage_smps10(struct regulator_dev *dev,
> + unsigned selector)
> +{
> + if (selector)
> + return 5000000;
> +
> + return 3750000;
This could be written a little more transparently!
> +static int palmas_get_voltage_smps10(struct regulator_dev *dev)
> +{
> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
> + int selector;
> + unsigned int reg;
> +
> + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_CTRL, ®);
> +
> + selector = (reg & SMPS10_VSEL) >> 3;
> +
> + return palmas_list_voltage_smps10(dev, selector);
Should be get_voltage_sel() really, or beter still should be using
regulator_set_voltage_sel_regmap(). Similarly for the set (which is
already using _sel()).
> +static int palmas_enable_ldo(struct regulator_dev *dev)
> +{
> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
> + int id = rdev_get_id(dev);
> + unsigned int reg;
> +
> + palmas_ldo_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, ®);
> +
> + reg |= LDO1_CTRL_MODE_ACTIVE;
> +
> + palmas_ldo_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg);
Could use the core regmap stuff for the LDOs too.
> + /* Adjust selector to match list_voltage ranges */
> + if (selector > 49)
> + selector = 49;
> +
> + return palmas_list_voltage_ldo(dev, selector);
get_voltage_sel().
> + if (!pdata)
> + return -EINVAL;
> + if (!pdata->reg_data)
> + return -EINVAL;
I'd expect the driver to be able to register the regulators with no
platform data at all.
> + pmic = kzalloc(sizeof(*pmic), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
devm_kzalloc()
> + pmic->desc = kcalloc(PALMAS_NUM_REGS,
> + sizeof(struct regulator_desc), GFP_KERNEL);
> + if (!pmic->desc) {
> + ret = -ENOMEM;
> + goto err_free_pmic;
> + }
Just embed this in the pmic struct? It's always the same size.
> + pmic->rdev = kcalloc(PALMAS_NUM_REGS,
> + sizeof(struct regulator_dev *), GFP_KERNEL);
Similarly heere.
Attachment:
signature.asc
Description: Digital signature
