Re: [PATCH 2/2] TPS65217: Add tps65217 regulator driver

Re: [PATCH 2/2] TPS65217: Add tps65217 regulator driver

[Mark Brown]

On Fri, Dec 23, 2011 at 10:51:36AM +0530, AnilKumar Ch wrote:

Again there really ought to be some opportunity for code sharing here.

> +	help
> +	  This driver supports TPS65217 voltage regulator chips. TPS65217 provides
> +	  three step-down converters and four general-purpose LDO voltage regulators.
> +	  It supports software based voltage control for different voltage domains

This needs word wrapping.

> +/* Supported voltage values for regulators (in milliVolts) */
> +static const u16 VDCDC1_VSEL_table[] = {
> +	900, 925, 950, 975,
> +	1000, 1025, 1050, 1075,
> +	1100, 1125, 1150, 1175,
> +	1200, 1225, 1250, 1275,
> +	1300, 1325, 1350, 1375,
> +	1400, 1425, 1450, 1475,
> +	1500, 1550, 1600, 1650,
> +	1700, 1750, 1800,
> +};

You should replace all these vsel tables with calculations in the code,
they're all regular steps and some of the tables are getting a bit
large.

> +static inline int tps65217_pmic_read(struct tps65217_pmic *tps, u8 reg)
> +{
> +	u8 val;
> +	int err;
> +
> +	err = tps->mfd->read_dev(tps->mfd, reg, 1, &val);
> +	if (err)
> +		return err;
> +
> +	return val;
> +}

There's no way stuff like this should be open coded in each driver using
the MFD, basic register I/O stuff should be implemented in a single
place in the MFD core driver.  All this I/O code should be there.

> +static int tps65217_pmic_reg_read(struct tps65217_pmic *tps, u8 reg)
> +{
> +	int data;
> +
> +	mutex_lock(&tps->io_lock);
> +
> +	data = tps65217_pmic_read(tps, reg);
> +	if (data < 0)
> +		dev_err(tps->mfd->dev, "Read from reg 0x%x failed\n", reg);
> +
> +	mutex_unlock(&tps->io_lock);
> +	return data;
> +}

Three levels of read and write functions seems excessive...

> +	if (dcdc < TPS65217_DCDC_1 || dcdc > TPS65217_DCDC_3)
> +		return -EINVAL;
> +
> +	switch (dcdc) {
> +	case TPS65217_DCDC_1:
> +		mask = TPS65217_ENABLE_DC1_EN;
> +		break;
> +	case TPS65217_DCDC_2:
> +		mask = TPS65217_ENABLE_DC2_EN;
> +		break;
> +	case TPS65217_DCDC_3:
> +		mask = TPS65217_ENABLE_DC3_EN;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

You may as well just use the switch statements to check if the
regulators are supported given the way this is coded.  Though it would
be even better to just have an array with the per regulator data which
you look up (or use shifting to work out the bit if that's possible).

> +	/* password protected register write level 1 setting */
> +	tps->wp_level = TPS65217_PROTECT_L1;
> +
> +	return tps65217_pmic_set_bits(tps, TPS65217_REG_ENABLE, mask);

This looks like there's an abstraction problem somewhere along the line
- wp_level is a member of the struct but it's basically being used as a
parameter.  Indeed perhaps the write function ought to just do this all
by itself.

> +	/* password protected register write level 1 setting */
> +	tps->wp_level = TPS65217_PROTECT_L1;

You should use these constants consistently through the driver, in other
places you're using magic numbers directly.

> +	for (vsel = 0; vsel < tps->info[dcdc]->table_len; vsel++) {
> +		int mV = tps->info[dcdc]->table[vsel];
> +		int uV = mV * 1000;
> +
> +		/* Break at the first in-range value */
> +		if (min_uV <= uV && uV <= max_uV)
> +			break;
> +	}

If you are going to use a table you should implement set_voltage_sel()
which will do the table walk for you, but like I say you should just be
using a calculation.

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

Use devm_kzalloc().

RE: [PATCH 2/2] TPS65217: Add tps65217 regulator driver

[AnilKumar, Chimata]

Hi Mark,

On Fri, Dec 23, 2011 at 16:45:39, Mark Brown wrote:
> On Fri, Dec 23, 2011 at 10:51:36AM +0530, AnilKumar Ch wrote:
> 
> Again there really ought to be some opportunity for code sharing here.
> 
> > +	help
> > +	  This driver supports TPS65217 voltage regulator chips. TPS65217 provides
> > +	  three step-down converters and four general-purpose LDO voltage regulators.
> > +	  It supports software based voltage control for different voltage domains
> 
> This needs word wrapping.

Modified

> 
> > +/* Supported voltage values for regulators (in milliVolts) */
> > +static const u16 VDCDC1_VSEL_table[] = {
> > +	900, 925, 950, 975,
> > +	1000, 1025, 1050, 1075,
> > +	1100, 1125, 1150, 1175,
> > +	1200, 1225, 1250, 1275,
> > +	1300, 1325, 1350, 1375,
> > +	1400, 1425, 1450, 1475,
> > +	1500, 1550, 1600, 1650,
> > +	1700, 1750, 1800,
> > +};
> 
> You should replace all these vsel tables with calculations in the code,
> they're all regular steps and some of the tables are getting a bit
> large.

TPS65217 don't have any formula for computing the next voltage value. In
entire voltage scale range there is an inconsistency in the step size.

> 
> > +static inline int tps65217_pmic_read(struct tps65217_pmic *tps, u8 reg)
> > +{
> > +	u8 val;
> > +	int err;
> > +
> > +	err = tps->mfd->read_dev(tps->mfd, reg, 1, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	return val;
> > +}
> 
> There's no way stuff like this should be open coded in each driver using
> the MFD, basic register I/O stuff should be implemented in a single
> place in the MFD core driver.  All this I/O code should be there.

Moved to MFD driver

> 
> > +static int tps65217_pmic_reg_read(struct tps65217_pmic *tps, u8 reg)
> > +{
> > +	int data;
> > +
> > +	mutex_lock(&tps->io_lock);
> > +
> > +	data = tps65217_pmic_read(tps, reg);
> > +	if (data < 0)
> > +		dev_err(tps->mfd->dev, "Read from reg 0x%x failed\n", reg);
> > +
> > +	mutex_unlock(&tps->io_lock);
> > +	return data;
> > +}
> 
> Three levels of read and write functions seems excessive...

Took care of this as well

> 
> > +	if (dcdc < TPS65217_DCDC_1 || dcdc > TPS65217_DCDC_3)
> > +		return -EINVAL;
> > +
> > +	switch (dcdc) {
> > +	case TPS65217_DCDC_1:
> > +		mask = TPS65217_ENABLE_DC1_EN;
> > +		break;
> > +	case TPS65217_DCDC_2:
> > +		mask = TPS65217_ENABLE_DC2_EN;
> > +		break;
> > +	case TPS65217_DCDC_3:
> > +		mask = TPS65217_ENABLE_DC3_EN;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> You may as well just use the switch statements to check if the
> regulators are supported given the way this is coded.  Though it would
> be even better to just have an array with the per regulator data which
> you look up (or use shifting to work out the bit if that's possible).

Changed to an array which maintaining the mask for different regulators.

> 
> > +	/* password protected register write level 1 setting */
> > +	tps->wp_level = TPS65217_PROTECT_L1;
> > +
> > +	return tps65217_pmic_set_bits(tps, TPS65217_REG_ENABLE, mask);
> 
> This looks like there's an abstraction problem somewhere along the line
> - wp_level is a member of the struct but it's basically being used as a
> parameter.  Indeed perhaps the write function ought to just do this all
> by itself.

Changed by passing the level flag as a parameter instead of a structure
member

> 
> > +	/* password protected register write level 1 setting */
> > +	tps->wp_level = TPS65217_PROTECT_L1;
> 
> You should use these constants consistently through the driver, in other
> places you're using magic numbers directly.
> 
> > +	for (vsel = 0; vsel < tps->info[dcdc]->table_len; vsel++) {
> > +		int mV = tps->info[dcdc]->table[vsel];
> > +		int uV = mV * 1000;
> > +
> > +		/* Break at the first in-range value */
> > +		if (min_uV <= uV && uV <= max_uV)
> > +			break;
> > +	}
> 
> If you are going to use a table you should implement set_voltage_sel()
> which will do the table walk for you, but like I say you should just be
> using a calculation.

As per my understanding set_voltage_sel() is used to set a voltage if we
know the selector. But here it's different case, based on the min and
max voltages we have to identify the selector. For identifying the selector
we have to walk through the table. I did not find any helper functions
for walking through the table.

> 
> > +	tps = kzalloc(sizeof(*tps), GFP_KERNEL);
> > +	if (!tps)
> > +		return -ENOMEM;
> 
> Use devm_kzalloc().
> 

Changed

Regards
AnilKumar

Re: [PATCH 2/2] TPS65217: Add tps65217 regulator driver

[Mark Brown]

On Wed, Dec 28, 2011 at 09:14:31AM +0000, AnilKumar, Chimata wrote:
> On Fri, Dec 23, 2011 at 16:45:39, Mark Brown wrote:
> > On Fri, Dec 23, 2011 at 10:51:36AM +0530, AnilKumar Ch wrote:

> > > +/* Supported voltage values for regulators (in milliVolts) */
> > > +static const u16 VDCDC1_VSEL_table[] = {
> > > +	900, 925, 950, 975,
> > > +	1000, 1025, 1050, 1075,
> > > +	1100, 1125, 1150, 1175,
> > > +	1200, 1225, 1250, 1275,
> > > +	1300, 1325, 1350, 1375,
> > > +	1400, 1425, 1450, 1475,
> > > +	1500, 1550, 1600, 1650,
> > > +	1700, 1750, 1800,
> > > +};

> > You should replace all these vsel tables with calculations in the code,
> > they're all regular steps and some of the tables are getting a bit
> > large.

> TPS65217 don't have any formula for computing the next voltage value. In
> entire voltage scale range there is an inconsistency in the step size.

if (voltage < 1500)
	...
else
	...

> > > +	for (vsel = 0; vsel < tps->info[dcdc]->table_len; vsel++) {
> > > +		int mV = tps->info[dcdc]->table[vsel];
> > > +		int uV = mV * 1000;
> > > +
> > > +		/* Break at the first in-range value */
> > > +		if (min_uV <= uV && uV <= max_uV)
> > > +			break;
> > > +	}

> > If you are going to use a table you should implement set_voltage_sel()
> > which will do the table walk for you, but like I say you should just be
> > using a calculation.

> As per my understanding set_voltage_sel() is used to set a voltage if we
> know the selector. But here it's different case, based on the min and
> max voltages we have to identify the selector. For identifying the selector
> we have to walk through the table. I did not find any helper functions
> for walking through the table.

No, that would be totally unusable by anything.  If you implement _sel
the core will do the mapping into a selector for you; if you take the
time to look at the code you'll see that it's implementing exactly the
above loop.

RE: [PATCH 2/2] TPS65217: Add tps65217 regulator driver

[AnilKumar, Chimata]

Hi Mark,

On Wed, Dec 28, 2011 at 17:06:02, Mark Brown wrote:
> On Wed, Dec 28, 2011 at 09:14:31AM +0000, AnilKumar, Chimata wrote:
> > On Fri, Dec 23, 2011 at 16:45:39, Mark Brown wrote:
> > > On Fri, Dec 23, 2011 at 10:51:36AM +0530, AnilKumar Ch wrote:
> 
> > > > +/* Supported voltage values for regulators (in milliVolts) */
> > > > +static const u16 VDCDC1_VSEL_table[] = {
> > > > +	900, 925, 950, 975,
> > > > +	1000, 1025, 1050, 1075,
> > > > +	1100, 1125, 1150, 1175,
> > > > +	1200, 1225, 1250, 1275,
> > > > +	1300, 1325, 1350, 1375,
> > > > +	1400, 1425, 1450, 1475,
> > > > +	1500, 1550, 1600, 1650,
> > > > +	1700, 1750, 1800,
> > > > +};
> 
> > > You should replace all these vsel tables with calculations in the code,
> > > they're all regular steps and some of the tables are getting a bit
> > > large.
> 
> > TPS65217 don't have any formula for computing the next voltage value. In
> > entire voltage scale range there is an inconsistency in the step size.
> 
> if (voltage < 1500)
> 	...
> else
> 	...

Changed to functions instead of tables

> 
> > > > +	for (vsel = 0; vsel < tps->info[dcdc]->table_len; vsel++) {
> > > > +		int mV = tps->info[dcdc]->table[vsel];
> > > > +		int uV = mV * 1000;
> > > > +
> > > > +		/* Break at the first in-range value */
> > > > +		if (min_uV <= uV && uV <= max_uV)
> > > > +			break;
> > > > +	}
> 
> > > If you are going to use a table you should implement set_voltage_sel()
> > > which will do the table walk for you, but like I say you should just be
> > > using a calculation.
> 
> > As per my understanding set_voltage_sel() is used to set a voltage if we
> > know the selector. But here it's different case, based on the min and
> > max voltages we have to identify the selector. For identifying the selector
> > we have to walk through the table. I did not find any helper functions
> > for walking through the table.
> 
> No, that would be totally unusable by anything.  If you implement _sel
> the core will do the mapping into a selector for you; if you take the
> time to look at the code you'll see that it's implementing exactly the
> above loop.
>

I got it, core part I missed and now I changed the set_voltage to
set_voltage_sel().

Thanks
AnilKumar