| Message ID | 1375858358-15070-2-git-send-email-wni@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/06/2013 11:52 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index cdff742..eeb0115 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -89,6 +89,8 @@ > #include <linux/err.h> > #include <linux/mutex.h> > #include <linux/sysfs.h> > +#include <linux/delay.h> > +#include <linux/regulator/consumer.h> > > /* > * Addresses to scan > @@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, > #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ > > +#define POWER_ON_DELAY 20 /*ms*/ > + > /* > * Driver data (common to all clients) > */ > @@ -302,6 +306,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 +1396,48 @@ static void lm90_init_client(struct i2c_client *client) > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > } > > +static int lm90_power_control(struct i2c_client *client, bool is_enable) > +{ > + struct lm90_data *data = i2c_get_clientdata(client); > + int ret; > + > + mutex_lock(&data->update_lock); > + > + if (!data->lm90_reg) { > + 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; > + mutex_unlock(&data->update_lock); > + return -ENODEV; I don't think it is acceptable to have the driver fail on pretty much all PCs. Also, I dislike that - even if the calling code doesn't fail - the above message would be displayed on unload as well. In general, the 'unload' flag seems unnecessary. You could just call if (data->lm90_reg) regulator_disable(); in the remove function. In addition to that, shouldn't you call regulator_put() on exit ? Also, I am missing error handling in the probe function; if something else fails, the regulator is neither disabled nor released. Guenter > + } > + } > + if (is_enable) { > + ret = regulator_enable(data->lm90_reg); > + msleep(POWER_ON_DELAY); > + } 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); > + > + return ret; > +} > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client, > i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > > + err = lm90_power_control(client, true); > + if (err < 0) > + return err; > + > /* Set the device type */ > data->kind = id->driver_data; > if (data->kind == adm1032) { > @@ -1483,6 +1534,7 @@ 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); > > return 0; > } >
On 08/07/2013 03:03 PM, Guenter Roeck wrote: > On 08/06/2013 11:52 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 52 insertions(+) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index cdff742..eeb0115 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -89,6 +89,8 @@ >> #include <linux/err.h> >> #include <linux/mutex.h> >> #include <linux/sysfs.h> >> +#include <linux/delay.h> >> +#include <linux/regulator/consumer.h> >> >> /* >> * Addresses to scan >> @@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, >> #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ >> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ >> >> +#define POWER_ON_DELAY 20 /*ms*/ >> + >> /* >> * Driver data (common to all clients) >> */ >> @@ -302,6 +306,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 +1396,48 @@ static void lm90_init_client(struct i2c_client *client) >> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); >> } >> >> +static int lm90_power_control(struct i2c_client *client, bool is_enable) >> +{ >> + struct lm90_data *data = i2c_get_clientdata(client); >> + int ret; >> + >> + mutex_lock(&data->update_lock); >> + >> + if (!data->lm90_reg) { >> + 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; >> + mutex_unlock(&data->update_lock); >> + return -ENODEV; > > I don't think it is acceptable to have the driver fail on pretty much all PCs. Yes, you are right, I didn't consider it carefully, I will fix it. I think it's better to move these codes to the probe() directly. > > Also, I dislike that - even if the calling code doesn't fail - the above message would be displayed on unload as well. > > In general, the 'unload' flag seems unnecessary. You could just call > > if (data->lm90_reg) > regulator_disable(); > > in the remove function. In addition to that, shouldn't you call regulator_put() on exit ? Oh, sorry, I miss the regulator_put(), I will fix it. > Also, I am missing error handling in the probe function; if something else fails, > the regulator is neither disabled nor released. It looks this patch have many problems, I will fix them. Thanks for your comments. > > Guenter > >> + } >> + } >> + if (is_enable) { >> + ret = regulator_enable(data->lm90_reg); >> + msleep(POWER_ON_DELAY); >> + } 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); >> + >> + return ret; >> +} >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client, >> i2c_set_clientdata(client, data); >> mutex_init(&data->update_lock); >> >> + err = lm90_power_control(client, true); >> + if (err < 0) >> + return err; >> + >> /* Set the device type */ >> data->kind = id->driver_data; >> if (data->kind == adm1032) { >> @@ -1483,6 +1534,7 @@ 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); >> >> 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ [...] > + if (!data->lm90_reg) { > + 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; > + mutex_unlock(&data->update_lock); > + return -ENODEV; > + } > + } > + if (is_enable) { > + ret = regulator_enable(data->lm90_reg); > + msleep(POWER_ON_DELAY); Can this delay be handled directly from regulator? [...] ---
On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > [...] >> + if (!data->lm90_reg) { >> + 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; >> + mutex_unlock(&data->update_lock); >> + return -ENODEV; >> + } >> + } >> + if (is_enable) { >> + ret = regulator_enable(data->lm90_reg); >> + msleep(POWER_ON_DELAY); > > Can this delay be handled directly from regulator? I think it should be handled in the device driver. Because there have different delay time to wait devices stable. > > [...] > --- >
On 08/07/2013 12:27 AM, 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > [...] >> + if (!data->lm90_reg) { >> + 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; >> + mutex_unlock(&data->update_lock); >> + return -ENODEV; >> + } >> + } >> + if (is_enable) { >> + ret = regulator_enable(data->lm90_reg); >> + msleep(POWER_ON_DELAY); > > Can this delay be handled directly from regulator? > Good question. Browsing through other regulator_enable calls, I don't see a similar delay anywhere else, so one should assume that this is not necessary. Guenter
On 08/07/2013 12:32 AM, Wei Ni wrote: > On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> [...] >>> + if (!data->lm90_reg) { >>> + 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; >>> + mutex_unlock(&data->update_lock); >>> + return -ENODEV; >>> + } >>> + } >>> + if (is_enable) { >>> + ret = regulator_enable(data->lm90_reg); >>> + msleep(POWER_ON_DELAY); >> >> Can this delay be handled directly from regulator? > > I think it should be handled in the device driver. > Because there have different delay time to wait devices stable. > Then why does no other caller of regulator_enable() need this ? I don't think lm90 is so much different to other users of regulator functionality. Besides that, your delay is unconditional, even for static regulators which are always on. Other callers of regulator_enable() don't need all that complexity, which I take as sign that it is not needed here either. Guenter
On 08/07/2013 03:50 PM, Guenter Roeck wrote: > On 08/07/2013 12:32 AM, Wei Ni wrote: >> On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> [...] >>>> + if (!data->lm90_reg) { >>>> + 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; >>>> + mutex_unlock(&data->update_lock); >>>> + return -ENODEV; >>>> + } >>>> + } >>>> + if (is_enable) { >>>> + ret = regulator_enable(data->lm90_reg); >>>> + msleep(POWER_ON_DELAY); >>> >>> Can this delay be handled directly from regulator? >> >> I think it should be handled in the device driver. >> Because there have different delay time to wait devices stable. >> > > Then why does no other caller of regulator_enable() need this ? > I don't think lm90 is so much different to other users of regulator > functionality. May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock Low Time" is 25ms, so I supposed that it may need about 20ms to stable after power on. Anyway, if I remove this delay, the driver also works fine, so I will remove it in my next patch. Thanks. Wei. > > Besides that, your delay is unconditional, even for static regulators > which are always on. Other callers of regulator_enable() don't need > all that complexity, which I take as sign that it is not needed here either. > > Guenter >
> On 08/07/2013 03:50 PM, Guenter Roeck wrote: > > On 08/07/2013 12:32 AM, Wei Ni wrote: > >> On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >>> [...] > >>>> + if (!data->lm90_reg) { > >>>> + 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; > >>>> + mutex_unlock(&data->update_lock); > >>>> + return -ENODEV; > >>>> + } > >>>> + } > >>>> + if (is_enable) { > >>>> + ret = regulator_enable(data->lm90_reg); > >>>> + msleep(POWER_ON_DELAY); > >>> > >>> Can this delay be handled directly from regulator? > >> > >> I think it should be handled in the device driver. > >> Because there have different delay time to wait devices stable. > >> > > > > Then why does no other caller of regulator_enable() need this ? > > I don't think lm90 is so much different to other users of regulator > > functionality. > > May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock > Low Time" is 25ms, so I supposed that it may need about 20ms to stable > after power on. > > Anyway, if I remove this delay, the driver also works fine, so I will > remove it in my next patch. I originally had in mind that regulator API contain own delay option. E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property. ---
On 08/07/2013 04:45 PM, Alexander Shiyan wrote: >> On 08/07/2013 03:50 PM, Guenter Roeck wrote: >>> On 08/07/2013 12:32 AM, Wei Ni wrote: >>>> On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> [...] >>>>>> + if (!data->lm90_reg) { >>>>>> + 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; >>>>>> + mutex_unlock(&data->update_lock); >>>>>> + return -ENODEV; >>>>>> + } >>>>>> + } >>>>>> + if (is_enable) { >>>>>> + ret = regulator_enable(data->lm90_reg); >>>>>> + msleep(POWER_ON_DELAY); >>>>> >>>>> Can this delay be handled directly from regulator? >>>> >>>> I think it should be handled in the device driver. >>>> Because there have different delay time to wait devices stable. >>>> >>> >>> Then why does no other caller of regulator_enable() need this ? >>> I don't think lm90 is so much different to other users of regulator >>> functionality. >> >> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock >> Low Time" is 25ms, so I supposed that it may need about 20ms to stable >> after power on. >> >> Anyway, if I remove this delay, the driver also works fine, so I will >> remove it in my next patch. > > I originally had in mind that regulator API contain own delay option. > E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property. As I know the "startup-delay-us" is used for the regulator device, not the consumer devices. In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable, but it seems it's unnecessary now :) Wei. > > --- >
On 08/07/2013 03:35 AM, Wei Ni wrote: > On 08/07/2013 04:45 PM, Alexander Shiyan wrote: >>> On 08/07/2013 03:50 PM, Guenter Roeck wrote: >>>> On 08/07/2013 12:32 AM, Wei Ni wrote: >>>>> On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> [...] >>>>>>> + if (!data->lm90_reg) { >>>>>>> + 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; >>>>>>> + mutex_unlock(&data->update_lock); >>>>>>> + return -ENODEV; >>>>>>> + } >>>>>>> + } >>>>>>> + if (is_enable) { >>>>>>> + ret = regulator_enable(data->lm90_reg); >>>>>>> + msleep(POWER_ON_DELAY); >>>>>> >>>>>> Can this delay be handled directly from regulator? >>>>> >>>>> I think it should be handled in the device driver. >>>>> Because there have different delay time to wait devices stable. >>>>> >>>> >>>> Then why does no other caller of regulator_enable() need this ? >>>> I don't think lm90 is so much different to other users of regulator >>>> functionality. >>> >>> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock >>> Low Time" is 25ms, so I supposed that it may need about 20ms to stable >>> after power on. >>> >>> Anyway, if I remove this delay, the driver also works fine, so I will >>> remove it in my next patch. >> >> I originally had in mind that regulator API contain own delay option. >> E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property. > > As I know the "startup-delay-us" is used for the regulator device, not > the consumer devices. Yes, the regulator should encoded its own startup delay. Each individual device should handle its own requirements for delay after power is stable. > In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable, > but it seems it's unnecessary now :) No, the driver needs to handle this properly. If the datasheet says a delay is needed, it is. It's probably working because in your tests the supply just happens to be on already.
On Wed, Aug 07, 2013 at 10:06:30AM -0600, Stephen Warren wrote: > On 08/07/2013 03:35 AM, Wei Ni wrote: > > On 08/07/2013 04:45 PM, Alexander Shiyan wrote: > >>> On 08/07/2013 03:50 PM, Guenter Roeck wrote: > >>>> On 08/07/2013 12:32 AM, Wei Ni wrote: > >>>>> On 08/07/2013 03:27 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> [...] > >>>>>>> + if (!data->lm90_reg) { > >>>>>>> + 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; > >>>>>>> + mutex_unlock(&data->update_lock); > >>>>>>> + return -ENODEV; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + if (is_enable) { > >>>>>>> + ret = regulator_enable(data->lm90_reg); > >>>>>>> + msleep(POWER_ON_DELAY); > >>>>>> > >>>>>> Can this delay be handled directly from regulator? > >>>>> > >>>>> I think it should be handled in the device driver. > >>>>> Because there have different delay time to wait devices stable. > >>>>> > >>>> > >>>> Then why does no other caller of regulator_enable() need this ? > >>>> I don't think lm90 is so much different to other users of regulator > >>>> functionality. > >>> > >>> May be I'm wrong. I noticed that in lm90 SPEC, the max of "SMBus Clock > >>> Low Time" is 25ms, so I supposed that it may need about 20ms to stable > >>> after power on. > >>> > >>> Anyway, if I remove this delay, the driver also works fine, so I will > >>> remove it in my next patch. > >> > >> I originally had in mind that regulator API contain own delay option. > >> E.g. reg-fixed-voltage && gpio-regulator contains "startup-delay-us" property. > > > > As I know the "startup-delay-us" is used for the regulator device, not > > the consumer devices. > > Yes, the regulator should encoded its own startup delay. Each individual > device should handle its own requirements for delay after power is stable. > > > In this patch, msleep(POWER_ON_DELAY) was used to wait the lm90 stable, > > but it seems it's unnecessary now :) > > No, the driver needs to handle this properly. If the datasheet says a > delay is needed, it is. > Yes but, if at all, only if it is known that the supply has just been turned on. Imposing the delay on every user of the driver is unacceptable. Guenter
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index cdff742..eeb0115 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -89,6 +89,8 @@ #include <linux/err.h> #include <linux/mutex.h> #include <linux/sysfs.h> +#include <linux/delay.h> +#include <linux/regulator/consumer.h> /* * Addresses to scan @@ -179,6 +181,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ +#define POWER_ON_DELAY 20 /*ms*/ + /* * Driver data (common to all clients) */ @@ -302,6 +306,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 +1396,48 @@ static void lm90_init_client(struct i2c_client *client) i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); } +static int lm90_power_control(struct i2c_client *client, bool is_enable) +{ + struct lm90_data *data = i2c_get_clientdata(client); + int ret; + + mutex_lock(&data->update_lock); + + if (!data->lm90_reg) { + 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; + mutex_unlock(&data->update_lock); + return -ENODEV; + } + } + if (is_enable) { + ret = regulator_enable(data->lm90_reg); + msleep(POWER_ON_DELAY); + } 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); + + return ret; +} + static int lm90_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1406,6 +1453,10 @@ static int lm90_probe(struct i2c_client *client, i2c_set_clientdata(client, data); mutex_init(&data->update_lock); + err = lm90_power_control(client, true); + if (err < 0) + return err; + /* Set the device type */ data->kind = id->driver_data; if (data->kind == adm1032) { @@ -1483,6 +1534,7 @@ 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); 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)