linux-kernel - Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

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:	Thu, 21 Aug 2014 19:36:50 -0500
From:	Mark Brown <broonie@...nel.org>
To:	atull@...nsource.altera.com
Cc:	linux@...ck-us.net, jdelvare@...e.de, lm-sensors@...sensors.org,
	lgirdwood@...il.com, linux-kernel@...r.kernel.org,
	delicious.quinoa@...il.com, dinguyen@...nsource.altera.com,
	yvanderv@...nsource.altera.com
Subject: Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@...nsource.altera.com wrote:

> +config SENSORS_LTC2978_REGULATOR
> +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> +	default n

No need to say default n here, it's the default default.

> +	depends on SENSORS_LTC2978
> +	select REGULATOR

I'd expect a depends here.

> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>

If you need machine.h that's suspicious...  why do you need it?

> +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +
> +	ret = pmbus_set_page(client, 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> +}

This all looks very much like pmbus could use regmap and then the regmap
helpers.  I'd not insist on it though.  What I would however suggest is
that these functions should all be helpers which read the specific
page, addresses and bits to write from the driver structure - I bet the
code is going to be identical for most pmbus using regulators and so it
makes sense to share it like we do with the generic regmap functions.

That means that any good practice can be deployed more easily and any
API updates only need to update the helpers.

> +static struct regulator_init_data ltc2978_regulator_init = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};

You should not be forcing this on, you don't know what's safe on any
given board.  Allow the board to specify constraints then it has
control.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)