| Message ID | 1375944991-29182-2-git-send-email-wni@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> The device lm90 can be controlled by the vdd rail. > Adding the power control support to power on/off the vdd rail. > And make sure that power is enabled before accessing the device. > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c [...] > +static void lm90_power_control(struct i2c_client *client, bool is_enable) > +{ > + struct lm90_data *data = i2c_get_clientdata(client); > + int ret; > + > + if (!data->lm90_reg) > + return; > + > + mutex_lock(&data->update_lock); > + > + if (is_enable) > + ret = regulator_enable(data->lm90_reg); > + else > + ret = regulator_disable(data->lm90_reg); > + > + if (ret < 0) > + dev_err(&client->dev, > + "Error in %s rail vdd, error %d\n", > + (is_enable) ? "enabling" : "disabling", ret); > + else > + dev_info(&client->dev, "success in %s rail vdd\n", > + (is_enable) ? "enabling" : "disabling"); dev_dbg() ? > + > + mutex_unlock(&data->update_lock); > +} > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, > i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > > + data->lm90_reg = regulator_get(&client->dev, "vdd"); > + if (IS_ERR_OR_NULL(data->lm90_reg)) { What about deferred probe? if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) return -EPROBE_DEFER; > + if (PTR_ERR(data->lm90_reg) == -ENODEV) > + dev_info(&client->dev, > + "No regulator found for vdd. Assuming vdd is always powered."); On my opinion it is unnecessary message. > + else > + dev_warn(&client->dev, > + "Error [%ld] in getting the regulator handle for vdd.\n", > + PTR_ERR(data->lm90_reg)); Ditto. > + data->lm90_reg = NULL; You can just use !IS_ERR(data->lm90_reg) macro in the future, rather than set this to NULL. [...] ---
On 08/07/2013 11:56 PM, Wei Ni wrote: > The device lm90 can be controlled by the vdd rail. > Adding the power control support to power on/off the vdd rail. > And make sure that power is enabled before accessing the device. > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index cdff742..306a348 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -89,6 +89,7 @@ > #include <linux/err.h> > #include <linux/mutex.h> > #include <linux/sysfs.h> > +#include <linux/regulator/consumer.h> > > /* > * Addresses to scan > @@ -302,6 +303,7 @@ static const struct lm90_params lm90_params[] = { > struct lm90_data { > struct device *hwmon_dev; > struct mutex update_lock; > + struct regulator *lm90_reg; > char valid; /* zero until following fields are valid */ > unsigned long last_updated; /* in jiffies */ > int kind; > @@ -1391,6 +1393,32 @@ static void lm90_init_client(struct i2c_client *client) > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > } > > +static void lm90_power_control(struct i2c_client *client, bool is_enable) > +{ > + struct lm90_data *data = i2c_get_clientdata(client); > + int ret; > + > + if (!data->lm90_reg) > + return; > + > + mutex_lock(&data->update_lock); > + This is only called during probe and remove, so the mutex is unnecessary. > + if (is_enable) > + ret = regulator_enable(data->lm90_reg); > + else > + ret = regulator_disable(data->lm90_reg); > + > + if (ret < 0) > + dev_err(&client->dev, > + "Error in %s rail vdd, error %d\n", > + (is_enable) ? "enabling" : "disabling", ret); > + else > + dev_info(&client->dev, "success in %s rail vdd\n", > + (is_enable) ? "enabling" : "disabling"); > + which reduces the function to (pretty much unnecessary) messages and an if statement which you only need because you have the function. You should just call regulator_enable in probe and regulator_disable in remove. Guenter > + mutex_unlock(&data->update_lock); > +} > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, > i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > > + data->lm90_reg = regulator_get(&client->dev, "vdd"); You should use devm_regulator_get(). Then you also don't need the call to regulator_put(). > + if (IS_ERR_OR_NULL(data->lm90_reg)) { The function never returns NULL except if the regulator subsystem is not configured, so IS_ERR() is more appropriate. If the regulator subsystem is not configured, you especially don't need or want to pollute the log with an error message. > + if (PTR_ERR(data->lm90_reg) == -ENODEV) > + dev_info(&client->dev, > + "No regulator found for vdd. Assuming vdd is always powered."); > + else > + dev_warn(&client->dev, > + "Error [%ld] in getting the regulator handle for vdd.\n", > + PTR_ERR(data->lm90_reg)); I consider the messages unnecessary and confusing. You are polluting the log of pretty much every PC user who has one of the supported chips in the system, and of everyone else not using regulators for this chip. > + data->lm90_reg = NULL; As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly. > + } > + > + lm90_power_control(client, true); > + > /* Set the device type */ > data->kind = id->driver_data; > if (data->kind == adm1032) { > @@ -1473,6 +1515,10 @@ exit_remove_files: > lm90_remove_files(client, data); > exit_restore: > lm90_restore_conf(client, data); > + lm90_power_control(client, false); > + if (data->lm90_reg) > + regulator_put(data->lm90_reg); > + > return err; > } > > @@ -1483,6 +1529,9 @@ static int lm90_remove(struct i2c_client *client) > hwmon_device_unregister(data->hwmon_dev); > lm90_remove_files(client, data); > lm90_restore_conf(client, data); > + lm90_power_control(client, false); > + if (data->lm90_reg) > + regulator_put(data->lm90_reg); > > return 0; > } >
On 08/08/2013 03:13 PM, Alexander Shiyan wrote: >> The device lm90 can be controlled by the vdd rail. >> Adding the power control support to power on/off the vdd rail. >> And make sure that power is enabled before accessing the device. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > [...] >> +static void lm90_power_control(struct i2c_client *client, bool is_enable) >> +{ >> + struct lm90_data *data = i2c_get_clientdata(client); >> + int ret; >> + >> + if (!data->lm90_reg) >> + return; >> + >> + mutex_lock(&data->update_lock); >> + >> + if (is_enable) >> + ret = regulator_enable(data->lm90_reg); >> + else >> + ret = regulator_disable(data->lm90_reg); >> + >> + if (ret < 0) >> + dev_err(&client->dev, >> + "Error in %s rail vdd, error %d\n", >> + (is_enable) ? "enabling" : "disabling", ret); >> + else >> + dev_info(&client->dev, "success in %s rail vdd\n", >> + (is_enable) ? "enabling" : "disabling"); > > dev_dbg() ? As Guenter said, I will remove these messages, and remove this function, the code will be more clean. > >> + >> + mutex_unlock(&data->update_lock); >> +} >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, >> i2c_set_clientdata(client, data); >> mutex_init(&data->update_lock); >> >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > What about deferred probe? > if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) > return -EPROBE_DEFER; Oh, yes, I should add it. > >> + if (PTR_ERR(data->lm90_reg) == -ENODEV) >> + dev_info(&client->dev, >> + "No regulator found for vdd. Assuming vdd is always powered."); > > On my opinion it is unnecessary message. > >> + else >> + dev_warn(&client->dev, >> + "Error [%ld] in getting the regulator handle for vdd.\n", >> + PTR_ERR(data->lm90_reg)); > > Ditto. I will remove these log messages. > >> + data->lm90_reg = NULL; > > You can just use !IS_ERR(data->lm90_reg) macro in the future, > rather than set this to NULL. Yes, it's better, I will do it. > > [...] > > --- >
On 08/08/2013 04:42 PM, Guenter Roeck wrote: > On 08/07/2013 11:56 PM, Wei Ni wrote: >> The device lm90 can be controlled by the vdd rail. >> Adding the power control support to power on/off the vdd rail. >> And make sure that power is enabled before accessing the device. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index cdff742..306a348 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -89,6 +89,7 @@ >> #include <linux/err.h> >> #include <linux/mutex.h> >> #include <linux/sysfs.h> >> +#include <linux/regulator/consumer.h> >> >> /* >> * Addresses to scan >> @@ -302,6 +303,7 @@ static const struct lm90_params lm90_params[] = { >> struct lm90_data { >> struct device *hwmon_dev; >> struct mutex update_lock; >> + struct regulator *lm90_reg; >> char valid; /* zero until following fields are valid */ >> unsigned long last_updated; /* in jiffies */ >> int kind; >> @@ -1391,6 +1393,32 @@ static void lm90_init_client(struct i2c_client *client) >> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); >> } >> >> +static void lm90_power_control(struct i2c_client *client, bool is_enable) >> +{ >> + struct lm90_data *data = i2c_get_clientdata(client); >> + int ret; >> + >> + if (!data->lm90_reg) >> + return; >> + >> + mutex_lock(&data->update_lock); >> + > > This is only called during probe and remove, so the mutex is unnecessary. I considered that we may call this function in suspend/resume routine, so I add this mutex. But as you said, currently we doesn't have these routine yet, the mutex is unnecessary, so I will remove it. > >> + if (is_enable) >> + ret = regulator_enable(data->lm90_reg); >> + else >> + ret = regulator_disable(data->lm90_reg); >> + >> + if (ret < 0) >> + dev_err(&client->dev, >> + "Error in %s rail vdd, error %d\n", >> + (is_enable) ? "enabling" : "disabling", ret); >> + else >> + dev_info(&client->dev, "success in %s rail vdd\n", >> + (is_enable) ? "enabling" : "disabling"); >> + > which reduces the function to (pretty much unnecessary) messages and an if statement > which you only need because you have the function. > > You should just call regulator_enable in probe and regulator_disable in remove. Ok, I will remove these messages and this function. > > Guenter > >> + mutex_unlock(&data->update_lock); >> +} >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, >> i2c_set_clientdata(client, data); >> mutex_init(&data->update_lock); >> >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); > > You should use devm_regulator_get(). Then you also don't need the call to regulator_put(). Oh, yes, you are right, I will do it. > >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > The function never returns NULL except if the regulator subsystem is not configured, > so IS_ERR() is more appropriate. > > If the regulator subsystem is not configured, you especially don't need or want > to pollute the log with an error message. > >> + if (PTR_ERR(data->lm90_reg) == -ENODEV) >> + dev_info(&client->dev, >> + "No regulator found for vdd. Assuming vdd is always powered."); >> + else >> + dev_warn(&client->dev, >> + "Error [%ld] in getting the regulator handle for vdd.\n", >> + PTR_ERR(data->lm90_reg)); > > I consider the messages unnecessary and confusing. You are polluting the log > of pretty much every PC user who has one of the supported chips in the system, > and of everyone else not using regulators for this chip. Ok, I will remove these codes. So I will write something like: if (!IS_ERR(data->lm90_reg)) { ret = regulator_enable(data->lm90_reg); if (ret < 0) { dev_err(); return ret; } } else { if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) return -EPRPBE_DEFER; data->lm90_reg = !!IS_ERR(data->lm90_reg); } > >> + data->lm90_reg = NULL; > > As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly. I think get_regulator() will return error values, not only -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other error values. > >> + } >> + >> + lm90_power_control(client, true); >> + >> /* Set the device type */ >> data->kind = id->driver_data; >> if (data->kind == adm1032) { >> @@ -1473,6 +1515,10 @@ exit_remove_files: >> lm90_remove_files(client, data); >> exit_restore: >> lm90_restore_conf(client, data); >> + lm90_power_control(client, false); >> + if (data->lm90_reg) >> + regulator_put(data->lm90_reg); >> + >> return err; >> } >> >> @@ -1483,6 +1529,9 @@ static int lm90_remove(struct i2c_client *client) >> hwmon_device_unregister(data->hwmon_dev); >> lm90_remove_files(client, data); >> lm90_restore_conf(client, data); >> + lm90_power_control(client, false); >> + if (data->lm90_reg) >> + regulator_put(data->lm90_reg); >> >> return 0; >> } >> >
> On 08/08/2013 04:42 PM, Guenter Roeck wrote: > > On 08/07/2013 11:56 PM, Wei Ni wrote: > >> The device lm90 can be controlled by the vdd rail. > >> Adding the power control support to power on/off the vdd rail. > >> And make sure that power is enabled before accessing the device. > >> > >> Signed-off-by: Wei Ni <wni@nvidia.com> > >> --- > >> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ [...] > > I consider the messages unnecessary and confusing. You are polluting the log > > of pretty much every PC user who has one of the supported chips in the system, > > and of everyone else not using regulators for this chip. > > Ok, I will remove these codes. > So I will write something like: > if (!IS_ERR(data->lm90_reg)) { > ret = regulator_enable(data->lm90_reg); > if (ret < 0) { > dev_err(); > return ret; > } > } else { > if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) > return -EPRPBE_DEFER; > > data->lm90_reg = !!IS_ERR(data->lm90_reg); No. You do not need this line. Just use in remove(): if (!IS_ERR(data->lm90_reg)) regulator_disable(data->lm90_reg); ---
On 08/08/2013 05:57 PM, Alexander Shiyan wrote: >> On 08/08/2013 04:42 PM, Guenter Roeck wrote: >>> On 08/07/2013 11:56 PM, Wei Ni wrote: >>>> The device lm90 can be controlled by the vdd rail. >>>> Adding the power control support to power on/off the vdd rail. >>>> And make sure that power is enabled before accessing the device. >>>> >>>> Signed-off-by: Wei Ni <wni@nvidia.com> >>>> --- >>>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > [...] >>> I consider the messages unnecessary and confusing. You are polluting the log >>> of pretty much every PC user who has one of the supported chips in the system, >>> and of everyone else not using regulators for this chip. >> >> Ok, I will remove these codes. >> So I will write something like: >> if (!IS_ERR(data->lm90_reg)) { >> ret = regulator_enable(data->lm90_reg); >> if (ret < 0) { >> dev_err(); >> return ret; >> } >> } else { >> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) >> return -EPRPBE_DEFER; >> >> data->lm90_reg = !!IS_ERR(data->lm90_reg); > > No. You do not need this line. > > Just use in remove(): > if (!IS_ERR(data->lm90_reg)) > regulator_disable(data->lm90_reg); Oh, I see, thank you very much. > > --- >
On 08/08/2013 05:57 PM, Alexander Shiyan wrote: >> On 08/08/2013 04:42 PM, Guenter Roeck wrote: >>> On 08/07/2013 11:56 PM, Wei Ni wrote: >>>> The device lm90 can be controlled by the vdd rail. >>>> Adding the power control support to power on/off the vdd rail. >>>> And make sure that power is enabled before accessing the device. >>>> >>>> Signed-off-by: Wei Ni <wni@nvidia.com> >>>> --- >>>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > [...] >>> I consider the messages unnecessary and confusing. You are polluting the log >>> of pretty much every PC user who has one of the supported chips in the system, >>> and of everyone else not using regulators for this chip. >> >> Ok, I will remove these codes. >> So I will write something like: >> if (!IS_ERR(data->lm90_reg)) { >> ret = regulator_enable(data->lm90_reg); >> if (ret < 0) { >> dev_err(); >> return ret; >> } >> } else { >> if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) >> return -EPRPBE_DEFER; >> >> data->lm90_reg = !!IS_ERR(data->lm90_reg); BTW, since it may return EPROBE_DEFER, these codes should be put in the beginning of the probe() function, should before allocate lm90_data. > > No. You do not need this line. > > Just use in remove(): > if (!IS_ERR(data->lm90_reg)) > regulator_disable(data->lm90_reg); > > --- >
On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote: > + mutex_lock(&data->update_lock); > + > + if (is_enable) > + ret = regulator_enable(data->lm90_reg); > + else > + ret = regulator_disable(data->lm90_reg); > + > + if (ret < 0) > + dev_err(&client->dev, > + "Error in %s rail vdd, error %d\n", > + (is_enable) ? "enabling" : "disabling", ret); > + else > + dev_info(&client->dev, "success in %s rail vdd\n", > + (is_enable) ? "enabling" : "disabling"); > + > + mutex_unlock(&data->update_lock); Two things here. One is that it's not clear what this lokc is protecting since the only thing in the locked region is the regulator operation and that is thread safe. The other thing is that I'm not seeing anthing that ensures that enables and disables are matched - regulators are reference counted so two enables need two disables. > + data->lm90_reg = regulator_get(&client->dev, "vdd"); > + if (IS_ERR_OR_NULL(data->lm90_reg)) { NULL is a valid regulator, use IS_ERR(). > + if (PTR_ERR(data->lm90_reg) == -ENODEV) > + dev_info(&client->dev, > + "No regulator found for vdd. Assuming vdd is always powered."); > + else > + dev_warn(&client->dev, > + "Error [%ld] in getting the regulator handle for vdd.\n", > + PTR_ERR(data->lm90_reg)); You shouldn't just be ignoring errors here, though there are deployment difficulties with making sure a stub regulator is provided. These should be getting easier after the next merge window, the stubs will be being tweaked slightly to have an "assume it's there" option even when regulators are used. Especially in cases with device tree you should be paying attention to -EPROBE_DEFER, that will accurately reflect if a regulator is present but not loaded yet. That said if you *are* going to do this you should request the regulator using devm_regulator_get_optional(), this is intended to support things that don't need regulators (though that's not the case here).
On 08/08/2013 02:47 AM, Wei Ni wrote: > On 08/08/2013 04:42 PM, Guenter Roeck wrote: >> On 08/07/2013 11:56 PM, Wei Ni wrote: >>> The device lm90 can be controlled by the vdd rail. >>> Adding the power control support to power on/off the vdd rail. >>> And make sure that power is enabled before accessing the device. >>> >>> Signed-off-by: Wei Ni <wni@nvidia.com> >>> --- >>> drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 49 insertions(+) >>> >>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >>> index cdff742..306a348 100644 >>> --- a/drivers/hwmon/lm90.c >>> +++ b/drivers/hwmon/lm90.c >>> @@ -89,6 +89,7 @@ >>> #include <linux/err.h> >>> #include <linux/mutex.h> >>> #include <linux/sysfs.h> >>> +#include <linux/regulator/consumer.h> >>> >>> /* >>> * Addresses to scan >>> @@ -302,6 +303,7 @@ static const struct lm90_params lm90_params[] = { >>> struct lm90_data { >>> struct device *hwmon_dev; >>> struct mutex update_lock; >>> + struct regulator *lm90_reg; >>> char valid; /* zero until following fields are valid */ >>> unsigned long last_updated; /* in jiffies */ >>> int kind; >>> @@ -1391,6 +1393,32 @@ static void lm90_init_client(struct i2c_client *client) >>> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); >>> } >>> >>> +static void lm90_power_control(struct i2c_client *client, bool is_enable) >>> +{ >>> + struct lm90_data *data = i2c_get_clientdata(client); >>> + int ret; >>> + >>> + if (!data->lm90_reg) >>> + return; >>> + >>> + mutex_lock(&data->update_lock); >>> + >> >> This is only called during probe and remove, so the mutex is unnecessary. > > I considered that we may call this function in suspend/resume routine, > so I add this mutex. > But as you said, currently we doesn't have these routine yet, the mutex > is unnecessary, so I will remove it. > In that case, you can call mutex_lock() regulator_enable() / regulator_disable() mutex_unlock() directly in those functions. Again no need for the additional function. >> >>> + if (is_enable) >>> + ret = regulator_enable(data->lm90_reg); >>> + else >>> + ret = regulator_disable(data->lm90_reg); >>> + >>> + if (ret < 0) >>> + dev_err(&client->dev, >>> + "Error in %s rail vdd, error %d\n", >>> + (is_enable) ? "enabling" : "disabling", ret); >>> + else >>> + dev_info(&client->dev, "success in %s rail vdd\n", >>> + (is_enable) ? "enabling" : "disabling"); >>> + >> which reduces the function to (pretty much unnecessary) messages and an if statement >> which you only need because you have the function. >> >> You should just call regulator_enable in probe and regulator_disable in remove. > > Ok, I will remove these messages and this function. > >> >> Guenter >> >>> + mutex_unlock(&data->update_lock); >>> +} >>> + >>> static int lm90_probe(struct i2c_client *client, >>> const struct i2c_device_id *id) >>> { >>> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, >>> i2c_set_clientdata(client, data); >>> mutex_init(&data->update_lock); >>> >>> + data->lm90_reg = regulator_get(&client->dev, "vdd"); >> >> You should use devm_regulator_get(). Then you also don't need the call to regulator_put(). > > Oh, yes, you are right, I will do it. > >> >>> + if (IS_ERR_OR_NULL(data->lm90_reg)) { >> >> The function never returns NULL except if the regulator subsystem is not configured, >> so IS_ERR() is more appropriate. >> >> If the regulator subsystem is not configured, you especially don't need or want >> to pollute the log with an error message. >> >>> + if (PTR_ERR(data->lm90_reg) == -ENODEV) >>> + dev_info(&client->dev, >>> + "No regulator found for vdd. Assuming vdd is always powered."); >>> + else >>> + dev_warn(&client->dev, >>> + "Error [%ld] in getting the regulator handle for vdd.\n", >>> + PTR_ERR(data->lm90_reg)); >> >> I consider the messages unnecessary and confusing. You are polluting the log >> of pretty much every PC user who has one of the supported chips in the system, >> and of everyone else not using regulators for this chip. > > Ok, I will remove these codes. > So I will write something like: > if (!IS_ERR(data->lm90_reg)) { > ret = regulator_enable(data->lm90_reg); > if (ret < 0) { > dev_err(); > return ret; > } > } else { Handle the error in the if case. > if (PTR_ERR(data->lm90_reg) == -EPROBE_DEFER) > return -EPRPBE_DEFER; > > data->lm90_reg = !!IS_ERR(data->lm90_reg); You know that IS_ERR is true here. Unless I am missing something, this would assign "1" to lm90_reg. > } > >> >>> + data->lm90_reg = NULL; >> >> As pointed out, this is unnecessary, and you should handle -EPROBE_DEFER correctly. > > I think get_regulator() will return error values, not only > -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other > error values. > Matter of opinion if you want to check for IS_ERR or NULL later on. >> >>> + } >>> + >>> + lm90_power_control(client, true); >>> + >>> /* Set the device type */ >>> data->kind = id->driver_data; >>> if (data->kind == adm1032) { >>> @@ -1473,6 +1515,10 @@ exit_remove_files: >>> lm90_remove_files(client, data); >>> exit_restore: >>> lm90_restore_conf(client, data); >>> + lm90_power_control(client, false); >>> + if (data->lm90_reg) >>> + regulator_put(data->lm90_reg); >>> + >>> return err; >>> } >>> >>> @@ -1483,6 +1529,9 @@ static int lm90_remove(struct i2c_client *client) >>> hwmon_device_unregister(data->hwmon_dev); >>> lm90_remove_files(client, data); >>> lm90_restore_conf(client, data); >>> + lm90_power_control(client, false); >>> + if (data->lm90_reg) >>> + regulator_put(data->lm90_reg); >>> >>> return 0; >>> } >>> >> > > >
On 08/08/2013 04:01 AM, Mark Brown wrote: > On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote: > >> + mutex_lock(&data->update_lock); >> + >> + if (is_enable) >> + ret = regulator_enable(data->lm90_reg); >> + else >> + ret = regulator_disable(data->lm90_reg); >> + >> + if (ret < 0) >> + dev_err(&client->dev, >> + "Error in %s rail vdd, error %d\n", >> + (is_enable) ? "enabling" : "disabling", ret); >> + else >> + dev_info(&client->dev, "success in %s rail vdd\n", >> + (is_enable) ? "enabling" : "disabling"); >> + >> + mutex_unlock(&data->update_lock); > > Two things here. One is that it's not clear what this lokc is > protecting since the only thing in the locked region is the regulator > operation and that is thread safe. The other thing is that I'm not > seeing anthing that ensures that enables and disables are matched - > regulators are reference counted so two enables need two disables. > >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > NULL is a valid regulator, use IS_ERR(). > >> + if (PTR_ERR(data->lm90_reg) == -ENODEV) >> + dev_info(&client->dev, >> + "No regulator found for vdd. Assuming vdd is always powered."); >> + else >> + dev_warn(&client->dev, >> + "Error [%ld] in getting the regulator handle for vdd.\n", >> + PTR_ERR(data->lm90_reg)); > > You shouldn't just be ignoring errors here, though there are deployment > difficulties with making sure a stub regulator is provided. These > should be getting easier after the next merge window, the stubs will be > being tweaked slightly to have an "assume it's there" option even when > regulators are used. Especially in cases with device tree you should be > paying attention to -EPROBE_DEFER, that will accurately reflect if a > regulator is present but not loaded yet. > > That said if you *are* going to do this you should request the > regulator using devm_regulator_get_optional(), this is intended to > support things that don't need regulators (though that's not the case > here). > The lm90 driver works perfectly fine without regulator. Guenter
On Thu, Aug 08, 2013 at 04:25:41AM -0700, Guenter Roeck wrote: > On 08/08/2013 04:01 AM, Mark Brown wrote: > >That said if you *are* going to do this you should request the > >regulator using devm_regulator_get_optional(), this is intended to > >support things that don't need regulators (though that's not the case > >here). > The lm90 driver works perfectly fine without regulator. I'd be most surprised if the device worked without power, if the driver fails to get and enable a regulator for it then that's not great (especially if it does so silently). Note that the regulator API is written to stub itself out if not enabled, the code should be able to assume that it's there.
On 08/08/2013 06:08 AM, Mark Brown wrote: > On Thu, Aug 08, 2013 at 04:25:41AM -0700, Guenter Roeck wrote: >> On 08/08/2013 04:01 AM, Mark Brown wrote: > >>> That said if you *are* going to do this you should request the >>> regulator using devm_regulator_get_optional(), this is intended to >>> support things that don't need regulators (though that's not the case >>> here). > >> The lm90 driver works perfectly fine without regulator. > > I'd be most surprised if the device worked without power, if the driver > fails to get and enable a regulator for it then that's not great > (especially if it does so silently). > Correct, but it appears that the driver magically worked for a long time without it. Is it guaranteed that the driver keeps working for all cases where regulator support is enabled in the kernel, and where it used to work so far without mandating the existence of this specific regulator ? My main concern is having to deal with complaints that the driver stopped working for no good reason. In this context, is it common practice to name such regulators "vdd" or similar ? What if there are multiple LM90 or compatible chips in a system, connected to different power rails ? Who determines if the regulator is supposed to be named "vdd" or "vcc" or anything else, and to which power rails it is actually connected ? How can and does one guarantee that "vdd" is the correct regulator to use for all systems ? What if some other driver requests "vdd", but the chip it supports happens to be connected to a different power rail ? Sorry for my ignorance in that matter. I did browse through Documentation/power, but did not find a clear answer. > Note that the regulator API is written to stub itself out if not > enabled, the code should be able to assume that it's there. > I am aware of that; this is why the driver should not dump an error message if the function returns NULL and bail out, as it did originally. Guenter
On Thu, Aug 08, 2013 at 08:21:48AM -0700, Guenter Roeck wrote: > On 08/08/2013 06:08 AM, Mark Brown wrote: > >I'd be most surprised if the device worked without power, if the driver > >fails to get and enable a regulator for it then that's not great > >(especially if it does so silently). > Correct, but it appears that the driver magically worked for a long time > without it. Sure, it'll work in systems that have always on regulators. > Is it guaranteed that the driver keeps working for all cases where > regulator support is enabled in the kernel, and where it used to work > so far without mandating the existence of this specific regulator ? > My main concern is having to deal with complaints that the driver stopped > working for no good reason. Sure, that's the transition issues I mentioned - the regulator API does have stubbing facilities which should cover things and it's very easy to define stub regulators if you need to. Like I say I expect this to be a lot easier after the next merge window as another way of doing stubs is being added which should make this even easier by avoiding disrupting drivers that do genuinely want to check for absent supplies and handle that better. > In this context, is it common practice to name such regulators "vdd" > or similar ? What if there are multiple LM90 or compatible chips > in a system, connected to different power rails ? Who determines > if the regulator is supposed to be named "vdd" or "vcc" or anything > else, and to which power rails it is actually connected ? How can > and does one guarantee that "vdd" is the correct regulator to use > for all systems ? What if some other driver requests "vdd", but the chip > it supports happens to be connected to a different power rail ? That's not what the name means - they are nothing to do with the board. The names requested by a driver are defined with regard to the device and should be the names used by the chip itself as defined in the datasheet. A board that uses regulators then maps these onto specific regulators in the system, the driver doesn't need to know anything about this process.
On 08/08/2013 02:42 AM, Guenter Roeck wrote: > On 08/07/2013 11:56 PM, Wei Ni wrote: >> The device lm90 can be controlled by the vdd rail. >> Adding the power control support to power on/off the vdd rail. >> And make sure that power is enabled before accessing the device. >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, >> i2c_set_clientdata(client, data); >> mutex_init(&data->update_lock); >> >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); > > You should use devm_regulator_get(). Then you also don't need the call > to regulator_put(). > >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > The function never returns NULL except if the regulator subsystem is not > configured, > so IS_ERR() is more appropriate. > > If the regulator subsystem is not configured, you especially don't need > or want > to pollute the log with an error message. DT parsing errors should be reported. However, if there's nothing to parse, it's not an error. So, I think this should report an error message *if* there is a DT property that defines the regulator to use. If there's no property, just use no regulator. If there is a property, it had better be possible to parse it.
On 08/08/2013 05:23 AM, Guenter Roeck wrote: > On 08/08/2013 02:47 AM, Wei Ni wrote: ... >> I think get_regulator() will return error values, not only >> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other >> error values. >> > Matter of opinion if you want to check for IS_ERR or NULL later on. No, if regulator returns either: * Something valid * Someting IS_ERR() ... then everywhere has to check the value using IS_ERR(). If regulator returns either: * Something valid * Someting NULL ... then everywhere has to check the value against NULL. checking against both IS_ERR() and NULL shouldn't ever happen, and likewise IS_ERR_OR_NULL() is deprecated.
On Thu, Aug 08, 2013 at 11:33:13AM -0600, Stephen Warren wrote: > On 08/08/2013 05:23 AM, Guenter Roeck wrote: > > On 08/08/2013 02:47 AM, Wei Ni wrote: > ... > >> I think get_regulator() will return error values, not only > >> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other > >> error values. > >> > > Matter of opinion if you want to check for IS_ERR or NULL later on. > > No, if regulator returns either: > > * Something valid > * Someting IS_ERR() > > ... then everywhere has to check the value using IS_ERR(). > > If regulator returns either: > > * Something valid > * Someting NULL > > ... then everywhere has to check the value against NULL. > Other drivers calling get_regulator() don't check against NULL, so it should not be needed here either. Guenter > checking against both IS_ERR() and NULL shouldn't ever happen, and > likewise IS_ERR_OR_NULL() is deprecated. >
On Thu, Aug 08, 2013 at 11:30:30AM -0600, Stephen Warren wrote: > On 08/08/2013 02:42 AM, Guenter Roeck wrote: > > On 08/07/2013 11:56 PM, Wei Ni wrote: > >> The device lm90 can be controlled by the vdd rail. > >> Adding the power control support to power on/off the vdd rail. > >> And make sure that power is enabled before accessing the device. > > >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > > >> static int lm90_probe(struct i2c_client *client, > >> const struct i2c_device_id *id) > >> { > >> @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, > >> i2c_set_clientdata(client, data); > >> mutex_init(&data->update_lock); > >> > >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); > > > > You should use devm_regulator_get(). Then you also don't need the call > > to regulator_put(). > > > >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > > > The function never returns NULL except if the regulator subsystem is not > > configured, > > so IS_ERR() is more appropriate. > > > > If the regulator subsystem is not configured, you especially don't need > > or want > > to pollute the log with an error message. > > DT parsing errors should be reported. However, if there's nothing to > parse, it's not an error. > > So, I think this should report an error message *if* there is a DT > property that defines the regulator to use. If there's no property, just > use no regulator. If there is a property, it had better be possible to > parse it. > Agreed. Guenter
On 08/08/2013 11:59 AM, Guenter Roeck wrote: > On Thu, Aug 08, 2013 at 11:33:13AM -0600, Stephen Warren wrote: >> On 08/08/2013 05:23 AM, Guenter Roeck wrote: >>> On 08/08/2013 02:47 AM, Wei Ni wrote: >> ... >>>> I think get_regulator() will return error values, not only >>>> -EPROBE_DEFER, so we should set data->lm90_reg to NULL to handle other >>>> error values. >>>> >>> Matter of opinion if you want to check for IS_ERR or NULL later on. >> >> No, if regulator returns either: >> >> * Something valid >> * Someting IS_ERR() >> >> ... then everywhere has to check the value using IS_ERR(). >> >> If regulator returns either: >> >> * Something valid >> * Someting NULL >> >> ... then everywhere has to check the value against NULL. >> > Other drivers calling get_regulator() don't check against NULL, > so it should not be needed here either. Right I should have mentioned that I believe regulator falls into the first valid-or-IS_ERR case, and not the second valid-or-NULL case.
On Thu, Aug 08, 2013 at 11:30:30AM -0600, Stephen Warren wrote: > On 08/08/2013 02:42 AM, Guenter Roeck wrote: > > If the regulator subsystem is not configured, you especially don't need > > or want > > to pollute the log with an error message. > DT parsing errors should be reported. However, if there's nothing to > parse, it's not an error. > So, I think this should report an error message *if* there is a DT > property that defines the regulator to use. If there's no property, just > use no regulator. If there is a property, it had better be possible to > parse it. On a DT system you should get this behaviour simply by paying attention to the error code from the regualtor API - the core should complain loudly if the DT is incomprehensible.
On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote: > On Thu, Aug 08, 2013 at 08:21:48AM -0700, Guenter Roeck wrote: > > On 08/08/2013 06:08 AM, Mark Brown wrote: > > > >I'd be most surprised if the device worked without power, if the driver > > >fails to get and enable a regulator for it then that's not great > > >(especially if it does so silently). > > > Correct, but it appears that the driver magically worked for a long time > > without it. > > Sure, it'll work in systems that have always on regulators. > > > Is it guaranteed that the driver keeps working for all cases where > > regulator support is enabled in the kernel, and where it used to work > > so far without mandating the existence of this specific regulator ? > > My main concern is having to deal with complaints that the driver stopped > > working for no good reason. > > Sure, that's the transition issues I mentioned - the regulator API does > have stubbing facilities which should cover things and it's very easy to > define stub regulators if you need to. Like I say I expect this to be a > lot easier after the next merge window as another way of doing stubs is > being added which should make this even easier by avoiding disrupting > drivers that do genuinely want to check for absent supplies and handle > that better. > We will need to make sure that all dts files using any of the compatible chips are updated accordingly. There are several entries in various dts files for adm1032, adt7461, lm90, and nct1008. > > In this context, is it common practice to name such regulators "vdd" > > or similar ? What if there are multiple LM90 or compatible chips > > in a system, connected to different power rails ? Who determines > > if the regulator is supposed to be named "vdd" or "vcc" or anything > > else, and to which power rails it is actually connected ? How can > > and does one guarantee that "vdd" is the correct regulator to use > > for all systems ? What if some other driver requests "vdd", but the chip > > it supports happens to be connected to a different power rail ? > > That's not what the name means - they are nothing to do with the board. Ok, makes sense. > The names requested by a driver are defined with regard to the device > and should be the names used by the chip itself as defined in the 9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available). Guess one is as good as the other ;-). Guenter > datasheet. A board that uses regulators then maps these onto specific > regulators in the system, the driver doesn't need to know anything about > this process.
On Thu, Aug 08, 2013 at 01:00:26PM -0700, Guenter Roeck wrote: > On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote: > > Sure, that's the transition issues I mentioned - the regulator API does > > have stubbing facilities which should cover things and it's very easy to > > define stub regulators if you need to. Like I say I expect this to be a > > lot easier after the next merge window as another way of doing stubs is > > being added which should make this even easier by avoiding disrupting > > drivers that do genuinely want to check for absent supplies and handle > > that better. > We will need to make sure that all dts files using any of the compatible chips > are updated accordingly. There are several entries in various dts files for > adm1032, adt7461, lm90, and nct1008. Yes, and probably also board files as well. Or either just accept bisection trouble for now or wait till the better stubbing is in there - that will mean that for DT systems the core will just assume the supply is really there and not fail requests if it's not in the DT. > > The names requested by a driver are defined with regard to the device > > and should be the names used by the chip itself as defined in the > 9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available). > Guess one is as good as the other ;-). What I've suggested before is to use the name from the part for which the driver is named. Assuming the vendor doesn't randomly change their datasheet (but that causes problems for hardware engineers so tends to be avoided).
On 08/08/2013 02:18 PM, Mark Brown wrote: > On Thu, Aug 08, 2013 at 01:00:26PM -0700, Guenter Roeck wrote: >> On Thu, Aug 08, 2013 at 06:15:54PM +0100, Mark Brown wrote: > >>> Sure, that's the transition issues I mentioned - the regulator API does >>> have stubbing facilities which should cover things and it's very easy to >>> define stub regulators if you need to. Like I say I expect this to be a >>> lot easier after the next merge window as another way of doing stubs is >>> being added which should make this even easier by avoiding disrupting >>> drivers that do genuinely want to check for absent supplies and handle >>> that better. > >> We will need to make sure that all dts files using any of the compatible chips >> are updated accordingly. There are several entries in various dts files for >> adm1032, adt7461, lm90, and nct1008. > > Yes, and probably also board files as well. Or either just accept > bisection trouble for now or wait till the better stubbing is in there - Ah, that is exactly the trouble I wanted to avoid. > that will mean that for DT systems the core will just assume the supply > is really there and not fail requests if it's not in the DT. > >>> The names requested by a driver are defined with regard to the device >>> and should be the names used by the chip itself as defined in the > >> 9 votes for vdd, 11 votes for vcc, one undecided (no datasheet available). >> Guess one is as good as the other ;-). > > What I've suggested before is to use the name from the part for which > the driver is named. Assuming the vendor doesn't randomly change their > datasheet (but that causes problems for hardware engineers so tends to > be avoided). > Makes sense. Guenter
On 08/08/2013 07:01 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote: > >> + mutex_lock(&data->update_lock); >> + >> + if (is_enable) >> + ret = regulator_enable(data->lm90_reg); >> + else >> + ret = regulator_disable(data->lm90_reg); >> + >> + if (ret < 0) >> + dev_err(&client->dev, >> + "Error in %s rail vdd, error %d\n", >> + (is_enable) ? "enabling" : "disabling", ret); >> + else >> + dev_info(&client->dev, "success in %s rail vdd\n", >> + (is_enable) ? "enabling" : "disabling"); >> + >> + mutex_unlock(&data->update_lock); > > Two things here. One is that it's not clear what this lokc is > protecting since the only thing in the locked region is the regulator > operation and that is thread safe. The other thing is that I'm not > seeing anthing that ensures that enables and disables are matched - > regulators are reference counted so two enables need two disables. > >> + data->lm90_reg = regulator_get(&client->dev, "vdd"); >> + if (IS_ERR_OR_NULL(data->lm90_reg)) { > > NULL is a valid regulator, use IS_ERR(). > >> + if (PTR_ERR(data->lm90_reg) == -ENODEV) >> + dev_info(&client->dev, >> + "No regulator found for vdd. Assuming vdd is always powered."); >> + else >> + dev_warn(&client->dev, >> + "Error [%ld] in getting the regulator handle for vdd.\n", >> + PTR_ERR(data->lm90_reg)); > > You shouldn't just be ignoring errors here, though there are deployment > difficulties with making sure a stub regulator is provided. These > should be getting easier after the next merge window, the stubs will be > being tweaked slightly to have an "assume it's there" option even when Oh, really, could you show me the patch, I wish to take a look :) Wei. > regulators are used. Especially in cases with device tree you should be > paying attention to -EPROBE_DEFER, that will accurately reflect if a > regulator is present but not loaded yet. > > That said if you *are* going to do this you should request the > regulator using devm_regulator_get_optional(), this is intended to > support things that don't need regulators (though that's not the case > here). > > * Unknown Key > * 0x7EA229BD >
On Fri, Aug 09, 2013 at 09:57:00AM +0400, Alexander Shiyan wrote: > Instead of adding the support of regulators in each device, let's think about > whether it is possible to create a global regulator for any device on the I2C bus. > I see it like this: > We add an extra field in the i2c_board_info structure "power_name" and handle > it in the i2c_device_{probe/remove} functions. This would need to be an array of supplies, relatively few devices need only a single power supply. This is also not something that should be handled in I2C, power is not something that's uniquely needed by devices on an I2C bus. > The question remains how to maintain such regulator in the PM functions, > but we can discuss it. Well, there's some basic stuff for this for clocks already - a similar pattern should work. You need to use runtime PM to hook in the power management and it's not going to work for everything (especially things that can be wake sources) but there's some value in trying to factor out the basic use case.
> On Fri, Aug 09, 2013 at 09:57:00AM +0400, Alexander Shiyan wrote: > > > Instead of adding the support of regulators in each device, let's think about > > whether it is possible to create a global regulator for any device on the I2C bus. > > > I see it like this: > > We add an extra field in the i2c_board_info structure "power_name" and handle > > it in the i2c_device_{probe/remove} functions. > > This would need to be an array of supplies, relatively few devices need > only a single power supply. This is also not something that should be > handled in I2C, power is not something that's uniquely needed by devices > on an I2C bus. Additional regulators can be handled in the driver, or "parent"-scheme can be used. Is not it? ---
On Fri, Aug 09, 2013 at 03:23:31PM +0800, Wei Ni wrote: > On 08/08/2013 07:01 PM, Mark Brown wrote: > > You shouldn't just be ignoring errors here, though there are deployment > > difficulties with making sure a stub regulator is provided. These > > should be getting easier after the next merge window, the stubs will be > > being tweaked slightly to have an "assume it's there" option even when > Oh, really, could you show me the patch, I wish to take a look :) No patch quite yet - the basic idea is that for plain regulator_get() you'll only ever get -EPROBE_DEFER as an error, not -ENODEV. If the regulator is missing in the DT we assume it's there and provide a dummy if the DT doesn't hook it up. If the consumer genuinely wants to see if a supply might not be wired up it should use regulator_get_optional() which is in -next at the minute.
On Fri, Aug 09, 2013 at 02:50:11PM +0400, Alexander Shiyan wrote: > > This would need to be an array of supplies, relatively few devices need > > only a single power supply. This is also not something that should be > > handled in I2C, power is not something that's uniquely needed by devices > > on an I2C bus. > Additional regulators can be handled in the driver, or "parent"-scheme can > be used. Is not it? Given how common it is to have multiple supplies it seems sensible to just handle that in the core - it's common to have a digital supply and an analogue supply for example. The regulator framework already has APIs for working with multiple regulators in a single call so it's not going to be much more effort on the framework side.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index cdff742..306a348 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -89,6 +89,7 @@ #include <linux/err.h> #include <linux/mutex.h> #include <linux/sysfs.h> +#include <linux/regulator/consumer.h> /* * Addresses to scan @@ -302,6 +303,7 @@ static const struct lm90_params lm90_params[] = { struct lm90_data { struct device *hwmon_dev; struct mutex update_lock; + struct regulator *lm90_reg; char valid; /* zero until following fields are valid */ unsigned long last_updated; /* in jiffies */ int kind; @@ -1391,6 +1393,32 @@ static void lm90_init_client(struct i2c_client *client) i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); } +static void lm90_power_control(struct i2c_client *client, bool is_enable) +{ + struct lm90_data *data = i2c_get_clientdata(client); + int ret; + + if (!data->lm90_reg) + return; + + mutex_lock(&data->update_lock); + + if (is_enable) + ret = regulator_enable(data->lm90_reg); + else + ret = regulator_disable(data->lm90_reg); + + if (ret < 0) + dev_err(&client->dev, + "Error in %s rail vdd, error %d\n", + (is_enable) ? "enabling" : "disabling", ret); + else + dev_info(&client->dev, "success in %s rail vdd\n", + (is_enable) ? "enabling" : "disabling"); + + mutex_unlock(&data->update_lock); +} + static int lm90_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1406,6 +1434,20 @@ static int lm90_probe(struct i2c_client *client, i2c_set_clientdata(client, data); mutex_init(&data->update_lock); + data->lm90_reg = regulator_get(&client->dev, "vdd"); + if (IS_ERR_OR_NULL(data->lm90_reg)) { + if (PTR_ERR(data->lm90_reg) == -ENODEV) + dev_info(&client->dev, + "No regulator found for vdd. Assuming vdd is always powered."); + else + dev_warn(&client->dev, + "Error [%ld] in getting the regulator handle for vdd.\n", + PTR_ERR(data->lm90_reg)); + data->lm90_reg = NULL; + } + + lm90_power_control(client, true); + /* Set the device type */ data->kind = id->driver_data; if (data->kind == adm1032) { @@ -1473,6 +1515,10 @@ exit_remove_files: lm90_remove_files(client, data); exit_restore: lm90_restore_conf(client, data); + lm90_power_control(client, false); + if (data->lm90_reg) + regulator_put(data->lm90_reg); + return err; } @@ -1483,6 +1529,9 @@ static int lm90_remove(struct i2c_client *client) hwmon_device_unregister(data->hwmon_dev); lm90_remove_files(client, data); lm90_restore_conf(client, data); + lm90_power_control(client, false); + if (data->lm90_reg) + regulator_put(data->lm90_reg); return 0; }
The device lm90 can be controlled by the vdd rail. Adding the power control support to power on/off the vdd rail. And make sure that power is enabled before accessing the device. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/hwmon/lm90.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)