| Message ID | 20200928153923.134151-4-alban.bedel@aerq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (lm75) Add regulator support | expand |
On Mon, Sep 28, 2020 at 05:39:23PM +0200, Alban Bedel wrote: > Add regulator support for boards where the sensor first need to be > powered up before it can be used. > > Signed-off-by: Alban Bedel <alban.bedel@aerq.com> > --- > v2: Rely on dummy regulators instead of explicitly handling missing > regulator > --- > drivers/hwmon/lm75.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c > index ba0be48aeadd..e394df648c26 100644 > --- a/drivers/hwmon/lm75.c > +++ b/drivers/hwmon/lm75.c > @@ -17,6 +17,7 @@ > #include <linux/of.h> > #include <linux/regmap.h> > #include <linux/util_macros.h> > +#include <linux/regulator/consumer.h> > #include "lm75.h" > > /* > @@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, 0x4c, > struct lm75_data { > struct i2c_client *client; > struct regmap *regmap; > + struct regulator *vs; > u8 orig_conf; > u8 current_conf; > u8 resolution; /* In bits, 9 to 16 */ > @@ -540,6 +542,7 @@ static void lm75_remove(void *data) > struct i2c_client *client = lm75->client; > > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); > + regulator_disable(lm75->vs); > } > > static int > @@ -567,6 +570,10 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) > data->client = client; > data->kind = kind; > > + data->vs = devm_regulator_get(dev, "vs"); > + if (IS_ERR(data->vs)) > + return PTR_ERR(data->vs); > + > data->regmap = devm_regmap_init_i2c(client, &lm75_regmap_config); > if (IS_ERR(data->regmap)) > return PTR_ERR(data->regmap); > @@ -581,11 +588,19 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) > data->sample_time = data->params->default_sample_time; > data->resolution = data->params->default_resolution; > > + /* Enable the power */ > + err = regulator_enable(data->vs); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > /* Cache original configuration */ > status = i2c_smbus_read_byte_data(client, LM75_REG_CONF); > if (status < 0) { > dev_dbg(dev, "Can't read config? %d\n", status); > - return status; > + err = status; > + goto disable_regulator; The point of using devm_add_action_or_reset() was specifically to avoid having to have the cleanup gotos. On top of that, the lm75_remove() function was specifically intended to clean up configuration data, not to do anything else. While hijacking lm75_remove() to also disable the regulator is technically correct, it makes the code more difficult to understand, and it creates a potential source for subsequently introduced bugs. Right now I am not inclined to accept this code as-is. Please provide arguments for handling the cleanup this way instead of using devm_add_action_or_reset(). Thanks, Guenter
On Mon, 2020-09-28 at 09:39 -0700, Guenter Roeck wrote: > On Mon, Sep 28, 2020 at 05:39:23PM +0200, Alban Bedel wrote: > > Add regulator support for boards where the sensor first need to be > > powered up before it can be used. > > > > Signed-off-by: Alban Bedel <alban.bedel@aerq.com> > > --- > > v2: Rely on dummy regulators instead of explicitly handling missing > > regulator > > --- > > drivers/hwmon/lm75.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c > > index ba0be48aeadd..e394df648c26 100644 > > --- a/drivers/hwmon/lm75.c > > +++ b/drivers/hwmon/lm75.c > > @@ -17,6 +17,7 @@ > > #include <linux/of.h> > > #include <linux/regmap.h> > > #include <linux/util_macros.h> > > +#include <linux/regulator/consumer.h> > > #include "lm75.h" > > > > /* > > @@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { > > 0x48, 0x49, 0x4a, 0x4b, 0x4c, > > struct lm75_data { > > struct i2c_client *client; > > struct regmap *regmap; > > + struct regulator *vs; > > u8 orig_conf; > > u8 current_conf; > > u8 resolution; /* In bits, 9 to 16 > > */ > > @@ -540,6 +542,7 @@ static void lm75_remove(void *data) > > struct i2c_client *client = lm75->client; > > > > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75- > > >orig_conf); > > + regulator_disable(lm75->vs); > > } > > > > static int > > @@ -567,6 +570,10 @@ lm75_probe(struct i2c_client *client, const > > struct i2c_device_id *id) > > data->client = client; > > data->kind = kind; > > > > + data->vs = devm_regulator_get(dev, "vs"); > > + if (IS_ERR(data->vs)) > > + return PTR_ERR(data->vs); > > + > > data->regmap = devm_regmap_init_i2c(client, > > &lm75_regmap_config); > > if (IS_ERR(data->regmap)) > > return PTR_ERR(data->regmap); > > @@ -581,11 +588,19 @@ lm75_probe(struct i2c_client *client, const > > struct i2c_device_id *id) > > data->sample_time = data->params->default_sample_time; > > data->resolution = data->params->default_resolution; > > > > + /* Enable the power */ > > + err = regulator_enable(data->vs); > > + if (err) { > > + dev_err(dev, "failed to enable regulator: %d\n", err); > > + return err; > > + } > > + > > /* Cache original configuration */ > > status = i2c_smbus_read_byte_data(client, LM75_REG_CONF); > > if (status < 0) { > > dev_dbg(dev, "Can't read config? %d\n", status); > > - return status; > > + err = status; > > + goto disable_regulator; > > The point of using devm_add_action_or_reset() was specifically to avoid having > to have the cleanup gotos. On top of that, the lm75_remove() function was > specifically intended to clean up configuration data, not to do anything else. > While hijacking lm75_remove() to also disable the regulator is technically > correct, it makes the code more difficult to understand, and it creates a > potential source for subsequently introduced bugs. Right now I am not inclined > to accept this code as-is. Please provide arguments for handling the cleanup > this way instead of using devm_add_action_or_reset(). I wrote it that way to keep the memory usage lower, but I see your point and will update the patch accordingly. Alban
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c index ba0be48aeadd..e394df648c26 100644 --- a/drivers/hwmon/lm75.c +++ b/drivers/hwmon/lm75.c @@ -17,6 +17,7 @@ #include <linux/of.h> #include <linux/regmap.h> #include <linux/util_macros.h> +#include <linux/regulator/consumer.h> #include "lm75.h" /* @@ -101,6 +102,7 @@ static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, 0x4c, struct lm75_data { struct i2c_client *client; struct regmap *regmap; + struct regulator *vs; u8 orig_conf; u8 current_conf; u8 resolution; /* In bits, 9 to 16 */ @@ -540,6 +542,7 @@ static void lm75_remove(void *data) struct i2c_client *client = lm75->client; i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); + regulator_disable(lm75->vs); } static int @@ -567,6 +570,10 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) data->client = client; data->kind = kind; + data->vs = devm_regulator_get(dev, "vs"); + if (IS_ERR(data->vs)) + return PTR_ERR(data->vs); + data->regmap = devm_regmap_init_i2c(client, &lm75_regmap_config); if (IS_ERR(data->regmap)) return PTR_ERR(data->regmap); @@ -581,11 +588,19 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) data->sample_time = data->params->default_sample_time; data->resolution = data->params->default_resolution; + /* Enable the power */ + err = regulator_enable(data->vs); + if (err) { + dev_err(dev, "failed to enable regulator: %d\n", err); + return err; + } + /* Cache original configuration */ status = i2c_smbus_read_byte_data(client, LM75_REG_CONF); if (status < 0) { dev_dbg(dev, "Can't read config? %d\n", status); - return status; + err = status; + goto disable_regulator; } data->orig_conf = status; data->current_conf = status; @@ -593,7 +608,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) err = lm75_write_config(data, data->params->set_mask, data->params->clr_mask); if (err) - return err; + goto disable_regulator; err = devm_add_action_or_reset(dev, lm75_remove, data); if (err) @@ -608,6 +623,10 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name); return 0; + +disable_regulator: + regulator_disable(data->vs); + return err; } static const struct i2c_device_id lm75_ids[] = {
Add regulator support for boards where the sensor first need to be powered up before it can be used. Signed-off-by: Alban Bedel <alban.bedel@aerq.com> --- v2: Rely on dummy regulators instead of explicitly handling missing regulator --- drivers/hwmon/lm75.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)