linux-kernel - Re: [PATCHv2 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] [day] [month] [year] [list]

Message-ID: <20110514162705.GG2791@opensource.wolfsonmicro.com>
Date:	Sat, 14 May 2011 09:27:06 -0700
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: [PATCHv2 4/4] tps65912: add regulator driver

On Thu, May 12, 2011 at 01:44:05PM -0500, Margarita Olaya wrote:
> The tps65912 consist of 4 DCDCs and 10 LDOs. The output voltages can be
> configured by the SPI or I2C interface, they are meant to supply power
> to the main processor and other components.
> 
> Signed-off-by: Margarita Olaya Cabrera <magi@...mlogic.co.uk>

A few fairly minor issues, and one thing that I just want you to
confirm...

> +static int tps65912_set_mode(struct regulator_dev *dev, unsigned int mode)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	struct tps65912 *mfd = pmic->mfd;
> +	int pwm_mode, eco, reg1, reg2, reg1_val, reg2_val;
> +	int id = rdev_get_id(dev);
> +
> +	switch (id) {
> +	case TPS65912_REG_DCDC1:
> +		reg1 = TPS65912_DCDC1_CTRL;
> +		reg2 = TPS65912_DCDC1_AVS;
> +		break;

A helper function for this switch statement might be useful, it's used
by both get and set mode calls.

> +static int tps65912_set_voltage_ldo(struct regulator_dev *dev,
> +						unsigned selector)
> +{
> +	struct tps65912_reg *pmic = rdev_get_drvdata(dev);
> +	struct tps65912 *mfd = pmic->mfd;
> +	int id = rdev_get_id(dev), reg, value;
> +
> +	reg = tps65912_get_ldo_sel_register(pmic, id);
> +	value = tps65912_reg_read(mfd, reg);
> +	value &= 0xC0;
> +	return tps65912_reg_write(mfd, reg, selector | value);
> +}

I *think* the register only contains things for this regulator so you
don't need locking beyond the lock the regulator API guarantees you but
if that's not the case you'd need to ensure nothing could jump into the
middle of the read/modify/write cycle.

A comment would help.
--
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/