Re: [PATCH 3/4] REGULATOR: regulator driver for Palmas series chips

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

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);
> +
> +	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, &reg);
> +
> +	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);
> +
> +	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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]