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

[Graeme Gregory <gg@slimlogic.co.uk>]

On 14/05/12 17:52, Mark Brown wrote:
> 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?
Yes have tidied this up a little in latest version.
>> +	.get_voltage		= palmas_get_voltage_smps,
> Why not get_voltage_sel?
Mild amnesia is my excuse, is fixed now!

>> +	.set_voltage_sel	= palmas_set_voltage_smps_sel,
>> +	.list_voltage		= palmas_list_voltage_smps,
> Implementing map_voltage() too would be nice.
I have implemented it for smps.
>> +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.
Has been converted thanks!

>> +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!
Have re-written it hopefully clearer.

>> +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()).
I have implemented it using the regmap functions thanks for tip!

>> +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.
Enable/Disable now use regmap but is_enabled cant as it reads from a
different bitmask.

>> +	/* Adjust selector to match list_voltage ranges */
>> +	if (selector > 49)
>> +		selector = 49;
>> +
>> +	return palmas_list_voltage_ldo(dev, selector);
> get_voltage_sel().
Has been fixed!

>> +	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.
Ok, I misunderstood how the pdata was used, hopefully I have got it
correct in the new version.

>> +	pmic = kzalloc(sizeof(*pmic), GFP_KERNEL);
>> +	if (!pmic)
>> +		return -ENOMEM;
> devm_kzalloc()
Fixed!

>> +	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.
Have embedded, this was a left over from when I was originally planning
the muxing handling.

>> +	pmic->rdev = kcalloc(PALMAS_NUM_REGS,
>> +			sizeof(struct regulator_dev *), GFP_KERNEL);
> Similarly heere.
Same as above.

Thanks for taking time to review. Am sending v2 patches soon.

Graeme