linux-kernel - Re: [PATCH 4/4] tps65912: add 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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 May 2011 22:53:21 +0200
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Margarita Olaya <magi@...mlogic.co.uk>
Cc:	linux-kernel@...r.kernel.org, Liam Girdwood <lrg@...mlogic.co.uk>
Subject: Re: [PATCH 4/4] tps65912: add regulator driver

On Tue, May 10, 2011 at 03:25:59PM -0500, Margarita Olaya wrote:

A lot of this driver looks awfully similar to a bunch of the other TI
PMIC drivers posted recently - is it possible to reuse some of driver
code here?

> +/* Supported voltage values for regulators */
> +static const u16 VDCDCx_VSEL0_table[] = {
> +	5000, 5125, 5250, 5375, 5500,
> +	5625, 5750, 5875, 6000, 6125,

All these tables look like they're nice regular voltage steps and could
be replaced with simple calculations in the code.  That's better as it
means we don't have to iterate through the table every time we want to
do anything.

> +static inline int tps65912_read(struct tps65912_reg *pmic, u8 reg)
> +{
> +	u8 val;
> +	int err;
> +
> +	err = pmic->mfd->read(pmic->mfd, reg, 1, &val);
> +	if (err < 0)
> +		return err;
> +
> +	return val;
> +}

Shouldn't the MFD driver just export this function?  Both the IRQ and
GPIO drivers also need it.  Similarly for write() and the reg_ versions
of them.

> +		reg1_val |= DCDCCTRL_DCDC_MODE_MASK;
> +		tps65912_reg_write(pmic, reg1, reg1_val);
> +		reg2_val &= ~DCDC_AVS_ECO_MASK;
> +		tps65912_reg_write(pmic, reg2, reg2_val);

There's set_bits() and clear_bits() ops in the core...

> +	switch (range) {
> +	case 0:
> +		/* 0.5 - 1.2875V in 12.5mV steps */
> +		voltage = tps65912_vsel_to_uv_range0(vsel);
> +		break;

Just implement get_voltage_sel() and let the core do the lookup.

> +static int tps65912_set_voltage_dcdc(struct regulator_dev *dev,
> +						unsigned selector)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	int id = rdev_get_id(dev);
> +	int value;
> +	const u16 *table;
> +	u8 table_len, reg;
> +
> +	table = pmic->info[id]->table;
> +	table_len = pmic->info[id]->table_len;
> +
> +	reg = tps65912_get_dcdc_sel_register(pmic, id);
> +	value = tps65912_reg_read(pmic, reg);
> +	value &= 0xC0;
> +	return tps65912_reg_write(pmic, reg, selector | value);

You're not actually doing anything with table here...

> +static int tps65912_get_voltage_ldo(struct regulator_dev *dev)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	int id = rdev_get_id(dev), voltage = 0;
> +	int vsel = 0;
> +	u8 reg;
> +
> +	reg = tps65912_get_ldo_sel_register(pmic, id);
> +	vsel = tps65912_reg_read(pmic, reg);
> +	vsel &= 0x3F;
> +
> +	/* 0.5 - 1.2875V in 12.5mV steps */
> +	voltage = LDO_VSEL_table[vsel];
> +
> +	return voltage * 100;

Just make this into a get_voltage_sel().

> +static int tps65912_list_voltage_dcdc(struct regulator_dev *dev,
> +					unsigned selector)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	int id = rdev_get_id(dev);
> +	const u16 *table;
> +	u8 table_len;
> +
> +	table = pmic->info[id]->table;
> +	table_len = pmic->info[id]->table_len;
> +	if (selector >= table_len)
> +		return -EINVAL;
> +	else
> +		return table[selector] * 100;
> +}

There were a bunch of vsel_to_uV functions further up the driver which
do the same thing - one of the two implementations should go.
--
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/