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
| ||
|
Date: Fri, 22 Aug 2014 10:30:58 -0500 From: atull <atull@...nsource.altera.com> To: Mark Brown <broonie@...nel.org> 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 Fri, 22 Aug 2014, Mark Brown wrote: > 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. Hi Mark, Thanks for the review. I'll fix these two Kconfig issues. > > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > If you need machine.h that's suspicious... why do you need it? It was to define 'struct regulator_init_data' which I had hardwired here as you noted below. Will fix. > > > +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. > Yes, this info should be from board info or device tree. I'll fix this. Thanks! Alan -- 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/
Powered by blists - more mailing lists