| Message ID | 20171130223524.31716-1-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Bartosz Golaszewski |
| Headers | show |
| Series | [1/1] at24: Fix I²C device selection for runtime PM | expand |
Thank you, it fixes the issue on the multi-address eeprom that I have access to.
Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com>
One very minor remark:
+ struct device *dev = &at24->client[0]->dev;
It is sufficiently clear to others (and us a few months down the line)
why we are
using only client[0] for power management? Could it benefit from a separate
function with comments?
struct device *dev = get_pm_device(at24);
static struct device *get_pm_device(struct at24_data *at24)
{
/* explain why we use client[0] and not any of the dummies */
return &at24->client[0]->dev;
}
Hi Sven, On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote: > Thank you, it fixes the issue on the multi-address eeprom that I have access to. > > Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com> > > One very minor remark: > > + struct device *dev = &at24->client[0]->dev; > > It is sufficiently clear to others (and us a few months down the line) > why we are > using only client[0] for power management? Could it benefit from a separate > function with comments? > > struct device *dev = get_pm_device(at24); > > static struct device *get_pm_device(struct at24_data *at24) > { > /* explain why we use client[0] and not any of the dummies */ > return &at24->client[0]->dev; > } There are no comments in assigning at24->client[0] either (or a helper function). I think it should be rather evident when looking at the code when you think about it. I certainly don't object adding a comment if you insist or someone else thinks it's a good idea. Thanks for testing!
2017-12-01 16:35 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>: > Hi Sven, > > On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote: >> Thank you, it fixes the issue on the multi-address eeprom that I have access to. >> >> Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com> >> >> One very minor remark: >> >> + struct device *dev = &at24->client[0]->dev; >> >> It is sufficiently clear to others (and us a few months down the line) >> why we are >> using only client[0] for power management? Could it benefit from a separate >> function with comments? >> >> struct device *dev = get_pm_device(at24); >> >> static struct device *get_pm_device(struct at24_data *at24) >> { >> /* explain why we use client[0] and not any of the dummies */ >> return &at24->client[0]->dev; >> } > > There are no comments in assigning at24->client[0] either (or a helper > function). I think it should be rather evident when looking at the code > when you think about it. I certainly don't object adding a comment if you > insist or someone else thinks it's a good idea. > > Thanks for testing! > > -- > Kind regards, > > Sakari Ailus > sakari.ailus@linux.intel.com Pushed to at24/fixes, thanks! @Saraki: there were some conflicts with the previous fixes queued for 4.15. Could you take a look if my rebase didn't break anything? You can find my tree at git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git Best regards, Bartosz
Bartosz wrote:
> Could you take a look if my rebase didn't break anything?
Unfortunately the rebase broke multi-address eeprom support ;(
I will post a corrected patch.
On Fri, Dec 01, 2017 at 05:29:27PM +0100, Bartosz Golaszewski wrote: > 2017-12-01 16:35 GMT+01:00 Sakari Ailus <sakari.ailus@linux.intel.com>: > > Hi Sven, > > > > On Fri, Dec 01, 2017 at 10:20:41AM -0500, Sven Van Asbroeck wrote: > >> Thank you, it fixes the issue on the multi-address eeprom that I have access to. > >> > >> Tested-by: Sven Van Asbroeck on a 24AA16/24LC16B <svendev@arcx.com> > >> > >> One very minor remark: > >> > >> + struct device *dev = &at24->client[0]->dev; > >> > >> It is sufficiently clear to others (and us a few months down the line) > >> why we are > >> using only client[0] for power management? Could it benefit from a separate > >> function with comments? > >> > >> struct device *dev = get_pm_device(at24); > >> > >> static struct device *get_pm_device(struct at24_data *at24) > >> { > >> /* explain why we use client[0] and not any of the dummies */ > >> return &at24->client[0]->dev; > >> } > > > > There are no comments in assigning at24->client[0] either (or a helper > > function). I think it should be rather evident when looking at the code > > when you think about it. I certainly don't object adding a comment if you > > insist or someone else thinks it's a good idea. > > > > Thanks for testing! > > > > -- > > Kind regards, > > > > Sakari Ailus > > sakari.ailus@linux.intel.com > > Pushed to at24/fixes, thanks! > > @Saraki: there were some conflicts with the previous fixes queued for > 4.15. Could you take a look if my rebase didn't break anything? You > can find my tree at > git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git Seems fine to me. Thanks!!
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index e0b4b36ef010..07b6b61b96ee 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -561,18 +561,16 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf, static int at24_read(void *priv, unsigned int off, void *val, size_t count) { struct at24_data *at24 = priv; - struct i2c_client *client; + struct device *dev = &at24->client[0]->dev; char *buf = val; int ret; if (unlikely(!count)) return count; - client = at24_translate_offset(at24, &off); - - ret = pm_runtime_get_sync(&client->dev); + ret = pm_runtime_get_sync(dev); if (ret < 0) { - pm_runtime_put_noidle(&client->dev); + pm_runtime_put_noidle(dev); return ret; } @@ -588,7 +586,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) status = at24->read_func(at24, buf, off, count); if (status < 0) { mutex_unlock(&at24->lock); - pm_runtime_put(&client->dev); + pm_runtime_put(dev); return status; } buf += status; @@ -598,7 +596,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) mutex_unlock(&at24->lock); - pm_runtime_put(&client->dev); + pm_runtime_put(dev); return 0; } @@ -606,18 +604,16 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) static int at24_write(void *priv, unsigned int off, void *val, size_t count) { struct at24_data *at24 = priv; - struct i2c_client *client; + struct device *dev = &at24->client[0]->dev; char *buf = val; int ret; if (unlikely(!count)) return -EINVAL; - client = at24_translate_offset(at24, &off); - - ret = pm_runtime_get_sync(&client->dev); + ret = pm_runtime_get_sync(dev); if (ret < 0) { - pm_runtime_put_noidle(&client->dev); + pm_runtime_put_noidle(dev); return ret; } @@ -633,7 +629,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) status = at24->write_func(at24, buf, off, count); if (status < 0) { mutex_unlock(&at24->lock); - pm_runtime_put(&client->dev); + pm_runtime_put(dev); return status; } buf += status; @@ -643,7 +639,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) mutex_unlock(&at24->lock); - pm_runtime_put(&client->dev); + pm_runtime_put(dev); return 0; }
The at24 driver creates dummy I²C devices to access offsets in the chip that are outside the area supported using a single I²C address. It is not meaningful to use runtime PM to such devices; the system firmware (ACPI) does not know about these devices nor runtime PM was enabled for them. Always use the real device instead of the dummy ones. Fixes: 98e8201039af ("eeprom: at24: enable runtime pm support") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/misc/eeprom/at24.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)