| Message ID | 20200917101819.32045-4-alban.bedel@aerq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (lm75) Add regulator support | expand |
On Thu, Sep 17, 2020 at 12:18:19PM +0200, Alban Bedel wrote: > + data->vs = devm_regulator_get_optional(dev, "vs"); > + if (IS_ERR(data->vs)) { Unless the device can work without power you should not be using regulator_get_optional().
On 9/17/20 4:00 AM, Mark Brown wrote: > On Thu, Sep 17, 2020 at 12:18:19PM +0200, Alban Bedel wrote: > >> + data->vs = devm_regulator_get_optional(dev, "vs"); >> + if (IS_ERR(data->vs)) { > > Unless the device can work without power you should not be using > regulator_get_optional(). > The driver works today without regulator, and needs to continue doing so. Guenter
On 9/17/20 3:18 AM, 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> > --- > drivers/hwmon/lm75.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c > index ba0be48aeadd..b673f8d2ef20 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,8 @@ static void lm75_remove(void *data) > struct i2c_client *client = lm75->client; > > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); > + if (lm75->vs) > + regulator_disable(lm75->vs); > } > > static int > @@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) > data->client = client; > data->kind = kind; > > + data->vs = devm_regulator_get_optional(dev, "vs"); Looking into the regulator API, it may be better if you use devm_regulator_get(). AFAICS it returns a dummy regulator if there is none, and NULL if the regulator subsystem is disabled. So data->vs = devm_regulator_get(dev, "vs"); if (IS_ERR(data->vs)) return PTR_ERR(data->vs); should work and would be less messy. > + if (IS_ERR(data->vs)) { > + if (PTR_ERR(data->vs) == -ENODEV) > + data->vs = NULL; > + else > + 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 +593,21 @@ 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 */ > + if (data->vs) { > + err = regulator_enable(data->vs); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + } > + How about device removal ? Don't you have to call regulator_disable() there as well ? If so, it might be best to use devm_add_action_or_reset() to register a disable function. Thanks, Guenter > /* 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 +615,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 +630,11 @@ 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: > + if (data->vs) > + regulator_disable(data->vs); > + return err; > } > > static const struct i2c_device_id lm75_ids[] = { >
On Thu, Sep 17, 2020 at 07:04:51PM -0700, Guenter Roeck wrote: > On 9/17/20 4:00 AM, Mark Brown wrote: > > On Thu, Sep 17, 2020 at 12:18:19PM +0200, Alban Bedel wrote: > >> + data->vs = devm_regulator_get_optional(dev, "vs"); > >> + if (IS_ERR(data->vs)) { > > Unless the device can work without power you should not be using > > regulator_get_optional(). > The driver works today without regulator, and needs to continue > doing so. And it will continue to do so if it uses the normal regulator API, it will as ever ensure that if no regulator is provided by the firmware a dummy is provided.
On Thu, Sep 17, 2020 at 10:33:37PM -0700, Guenter Roeck wrote: > On 9/17/20 3:18 AM, Alban Bedel wrote: > > + err = regulator_enable(data->vs); > How about device removal ? Don't you have to call regulator_disable() > there as well ? If so, it might be best to use devm_add_action_or_reset() > to register a disable function. Yes, disables should be balanced (and any attempt to unregister the regulator with references still held should result in a warning).
On Thu, 2020-09-17 at 22:33 -0700, Guenter Roeck wrote: > On 9/17/20 3:18 AM, 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> > > --- > > drivers/hwmon/lm75.c | 31 +++++++++++++++++++++++++++++-- > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c > > index ba0be48aeadd..b673f8d2ef20 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,8 @@ static void lm75_remove(void *data) > > struct i2c_client *client = lm75->client; > > > > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); > > + if (lm75->vs) > > + regulator_disable(lm75->vs); > > } > > > > static int > > @@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const > > struct i2c_device_id *id) > > data->client = client; > > data->kind = kind; > > > > + data->vs = devm_regulator_get_optional(dev, "vs"); > > Looking into the regulator API, it may be better if you use > devm_regulator_get(). > AFAICS it returns a dummy regulator if there is none, and NULL if the > regulator subsystem is disabled. So > data->vs = devm_regulator_get(dev, "vs"); > if (IS_ERR(data->vs)) > return PTR_ERR(data->vs); > should work and would be less messy. Ok, I'll change that in the next version. > > + if (IS_ERR(data->vs)) { > > + if (PTR_ERR(data->vs) == -ENODEV) > > + data->vs = NULL; > > + else > > + 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 +593,21 @@ 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 */ > > + if (data->vs) { > > + err = regulator_enable(data->vs); > > + if (err) { > > + dev_err(dev, "failed to enable regulator: > > %d\n", err); > > + return err; > > + } > > + } > > + > > How about device removal ? Don't you have to call regulator_disable() > there as well ? If so, it might be best to use > devm_add_action_or_reset() to register a disable function. This is handled in lm75_remove() where I added the regulator_disable() call. Alban
On Mon, Sep 28, 2020 at 03:13:53PM +0000, Bedel, Alban wrote: > On Thu, 2020-09-17 at 22:33 -0700, Guenter Roeck wrote: > > On 9/17/20 3:18 AM, 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> > > > --- > > > drivers/hwmon/lm75.c | 31 +++++++++++++++++++++++++++++-- > > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c > > > index ba0be48aeadd..b673f8d2ef20 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,8 @@ static void lm75_remove(void *data) > > > struct i2c_client *client = lm75->client; > > > > > > i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); > > > + if (lm75->vs) > > > + regulator_disable(lm75->vs); > > > } > > > > > > static int > > > @@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const > > > struct i2c_device_id *id) > > > data->client = client; > > > data->kind = kind; > > > > > > + data->vs = devm_regulator_get_optional(dev, "vs"); > > > > Looking into the regulator API, it may be better if you use > > devm_regulator_get(). > > AFAICS it returns a dummy regulator if there is none, and NULL if the > > regulator subsystem is disabled. So > > data->vs = devm_regulator_get(dev, "vs"); > > if (IS_ERR(data->vs)) > > return PTR_ERR(data->vs); > > should work and would be less messy. > > Ok, I'll change that in the next version. > > > > + if (IS_ERR(data->vs)) { > > > + if (PTR_ERR(data->vs) == -ENODEV) > > > + data->vs = NULL; > > > + else > > > + 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 +593,21 @@ 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 */ > > > + if (data->vs) { > > > + err = regulator_enable(data->vs); > > > + if (err) { > > > + dev_err(dev, "failed to enable regulator: > > > %d\n", err); > > > + return err; > > > + } > > > + } > > > + > > > > How about device removal ? Don't you have to call regulator_disable() > > there as well ? If so, it might be best to use > > devm_add_action_or_reset() to register a disable function. > > This is handled in lm75_remove() where I added the regulator_disable() > call. lm75_remove() won't be called if the probe function fails. Guenter
On Mon, Sep 28, 2020 at 09:29:49AM -0700, Guenter Roeck wrote: > > > > This is handled in lm75_remove() where I added the regulator_disable() > > call. > > lm75_remove() won't be called if the probe function fails. > Sorry, I am confused; please ignore this noise. Guenter
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c index ba0be48aeadd..b673f8d2ef20 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,8 @@ static void lm75_remove(void *data) struct i2c_client *client = lm75->client; i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf); + if (lm75->vs) + regulator_disable(lm75->vs); } static int @@ -567,6 +571,14 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) data->client = client; data->kind = kind; + data->vs = devm_regulator_get_optional(dev, "vs"); + if (IS_ERR(data->vs)) { + if (PTR_ERR(data->vs) == -ENODEV) + data->vs = NULL; + else + 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 +593,21 @@ 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 */ + if (data->vs) { + 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 +615,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 +630,11 @@ 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: + if (data->vs) + 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> --- drivers/hwmon/lm75.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)