| Message ID | 20200928153923.134151-1-alban.bedel@aerq.com |
|---|---|
| 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
Hi everybody, this small series add regulator support to the lm75 driver for boards that don't always power such a sensor. While at it also convert the DT bindings to yaml. v2: - Fixed the DT example while converting to YAML - Removed the unneeded maxItems from the binding documentation - Use dummy regulator insted of explicitly handling missing regulator Alban Bedel (3): dt-bindings: hwmon: Convert lm75 bindings to yaml dt-bindings: hwmon: Add the +vs supply to the lm75 bindings hwmon: (lm75) Add regulator support .../devicetree/bindings/hwmon/lm75.txt | 39 ----------- .../devicetree/bindings/hwmon/lm75.yaml | 64 +++++++++++++++++++ drivers/hwmon/lm75.c | 23 ++++++- 3 files changed, 85 insertions(+), 41 deletions(-) delete mode 100644 Documentation/devicetree/bindings/hwmon/lm75.txt create mode 100644 Documentation/devicetree/bindings/hwmon/lm75.yaml