linux-kernel - RE: [PATCH V3 2/2] regulator: tps65217: Add tps65217 regulator driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 11 Jan 2012 08:22:59 +0000
From:	"AnilKumar, Chimata" <anilkumar@...com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
CC:	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"Girdwood, Liam" <lrg@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"Nori, Sekhar" <nsekhar@...com>
Subject: RE: [PATCH V3 2/2] regulator: tps65217: Add tps65217 regulator
 driver

Hi Mark,

On Mon, Jan 09, 2012 at 12:49:31, Mark Brown wrote:
> On Wed, Jan 04, 2012 at 03:22:27PM +0530, AnilKumar Ch wrote:
> 
> This looks pretty good.  A couple of small issues.
> 
> > +static int tps65217_vsel_to_uv_range1(unsigned int vsel)
> > +{
> > +	int uV = 0;
> > +
> > +	if (vsel > 15)
> > +		return -EINVAL;
> > +
> > +	if (vsel <= 2)
> > +		uV = vsel * 100000 + 1000000;
> > +	else if (vsel <= 6)
> > +		uV = (vsel - 2) * 50000 + 1200000;
> > +	else if (vsel <= 9)
> > +		uV = (vsel - 6) * 100000 + 1400000;
> > +	else if (vsel == 10)
> > +		uV = 2500000;
> > +	else if (vsel == 11)
> > +		uV = 2750000;
> > +	else if (vsel == 12)
> > +		uV = 2800000;
> > +	else if (vsel == 13)
> > +		uV = 3000000;
> > +	else if (vsel == 14)
> > +		uV = 3100000;
> > +	else
> > +		uV = 3300000;
> 
> This looks like it should actually be a table - there's far too many
> irregular steps here.  The other regulators looked to be benefiting from
> the use of calculations.

Changed to a table for this regulator only. 

> 
> > +static int tps65217_pmic_dcdc_get_voltage(struct regulator_dev *dev)
> > +{
> > +	int ret;
> > +	struct tps65217 *tps = rdev_get_drvdata(dev);
> > +	unsigned int data, dcdc = rdev_get_id(dev);
> > +
> > +	if (dcdc < TPS65217_DCDC_1 || dcdc > TPS65217_DCDC_3)
> > +		return -EINVAL;
> > +
> > +	ret = tps65217_reg_read(tps, tps->info[dcdc]->set_vout_reg, &data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data &= tps->info[dcdc]->set_vout_mask;
> > +
> > +	ret = tps->info[dcdc]->tps_range(data);
> > +	if (ret < 0)
> > +		dev_err(&dev->dev, "Failed to get voltage\n");
> > +
> > +	return ret;
> 
> It seems odd to implement this as a vanilla get_voltage()

Agree, changed to get_voltage_sel()

> 
> > +static int tps65217_pmic_dcdc_set_voltage(struct regulator_dev *dev,
> > +						unsigned selector)
> > +{
> 
> but this as set_voltage_sel().  For non table based regulators plain
> set_voltage() usually makes a bit more sense as we don't have to iterate
> through the selectors looking for a match.
> 

I changed the implementation according to your point but the
code size actually increased by 68 lines after fixing these
comments. I will send across the replacement, please see if
you like this one better.

Regards,
AnilKumar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/