| Message ID | 1429915008-22015-2-git-send-email-cernekee@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 24, 2015 at 3:36 PM, Kevin Cernekee <cernekee@chromium.org> wrote: > regcache_sync() and regcache_sync_region() currently assume that the > hardware has just emerged from a clean reset, and that all registers are > in their default states. But that isn't the only possibility; the device > may have been in a different state in which the registers were > inaccessible but have retained their contents, e.g. clock gating. > > So we will extend the more versatile of the two functions, > regcache_sync_region(), to let the caller decide what assumptions should > be made. > > One driver that can benefit from this is adau1977, which has hacks to > overwrite the registers that regcache_sync() might have missed. Also, > the powerdown pin on tas571x does not reset the register contents either, > so a similar feature will be required by that driver. > > This commit just adds the new argument by changing the function > declarations and call sites, but doesn't wire it up yet. The last two lines of the changelog were inadvertently carried over from an older version of the patch, and should be deleted. Will fix in V3.
On 04/25/2015 12:36 AM, Kevin Cernekee wrote: > regcache_sync() and regcache_sync_region() currently assume that the > hardware has just emerged from a clean reset, and that all registers are > in their default states. But that isn't the only possibility; the device > may have been in a different state in which the registers were > inaccessible but have retained their contents, e.g. clock gating. > > So we will extend the more versatile of the two functions, > regcache_sync_region(), to let the caller decide what assumptions should > be made. > > One driver that can benefit from this is adau1977, which has hacks to > overwrite the registers that regcache_sync() might have missed. The issue with the adau1977 is slightly different, there is only one single register which is not affected, so we need special handling for that register. For all other registers the default behavior of regcache_sync() is correct. > Also, the powerdown pin on tas571x does not reset the register contents > either, so a similar feature will be required by that driver. > > This commit just adds the new argument by changing the function > declarations and call sites, but doesn't wire it up yet. I think a better approach is to introduce a new flag internally, similar to the cache_dirty flag. Set both this new flag and cache_dirty when regcache_mark_dirty() is called. But only cache_dirty when a write is performed when the regmap is marked as cache only. Now in regcache_sync() and friends check this new flag and only when it is set skip registers which match the default. At the end of regcache_sync() clear both flags. [...] > @@ -600,7 +605,8 @@ static bool regcache_reg_present(unsigned long *cache_present, unsigned int idx) > static int regcache_sync_block_single(struct regmap *map, void *block, > unsigned long *cache_present, > unsigned int block_base, > - unsigned int start, unsigned int end) > + unsigned int start, unsigned int end, > + bool was_reset) > { > unsigned int i, regtmp, val; > int ret; > @@ -614,10 +620,12 @@ static int regcache_sync_block_single(struct regmap *map, void *block, > > val = regcache_get_val(map, block, i); > > - /* Is this the hardware default? If so skip. */ > - ret = regcache_lookup_reg(map, regtmp); > - if (ret >= 0 && val == map->reg_defaults[ret].def) > - continue; > + if (was_reset) { > + /* Is this the hardware default? If so skip. */ > + ret = regcache_lookup_reg(map, regtmp); > + if (ret >= 0 && val == map->reg_defaults[ret].def) > + continue; > + } Probably factor the check whether wee need to sync or not into a helper function, that can be used here and below in sync_block_raw(). [...] > @@ -689,14 +697,17 @@ static int regcache_sync_block_raw(struct regmap *map, void *block, > > val = regcache_get_val(map, block, i); > > - /* Is this the hardware default? If so skip. */ > - ret = regcache_lookup_reg(map, regtmp); > - if (ret >= 0 && val == map->reg_defaults[ret].def) { > - ret = regcache_sync_block_raw_flush(map, &data, > - base, regtmp); > - if (ret != 0) > - return ret; > - continue; > + if (was_reset) { > + /* Is this the hardware default? If so skip. */ > + ret = regcache_lookup_reg(map, regtmp); > + if (ret >= 0 && val == map->reg_defaults[ret].def) { > + ret = regcache_sync_block_raw_flush(map, &data, > + base, > + regtmp); > + if (ret != 0) > + return ret; > + continue; > + } > } > [...]
On Fri, Apr 24, 2015 at 03:36:45PM -0700, Kevin Cernekee wrote: > index 116655d92269..ece122a6fdeb 100644 > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map); > > int regcache_sync(struct regmap *map); > int regcache_sync_region(struct regmap *map, unsigned int min, > - unsigned int max); > + unsigned int max, bool was_reset); This seems pretty ugly - both the fact that we're changing the signature of the function and the naming of the argument feel inelegant. The point isn't if the device has been reset, the point is if the device currently has the default register values or not, and this means that the user is responsible for tracking that state until the next time it does the sync. That may be immediately like in your case but there's no reason that has to be the case. The fact that we're passing in something called "is_reset" which sounds like a state value for the register map is a bit of a warning sign here. What we should be doing here is providing a way for users to tell regmap if they've reset the register map and actually we already have that interface, it's just not got the best name - regcache_mark_dirty() is effectively it since there's really not a lot of other reasons why a driver would need to mark the cache as dirty. We're just not handling it properly. What we should do instead is to keep the interface as it is for now and make it behave in a more expected fashion so that if the cache is explicitly marked dirty we assume that the hardware is in the default state and otherwise we don't. Ideally what we'd do is both improve the naming of mark_dirty() (though that's API churn which is nasty) and arrange for rbtree to cache the default values lazily, that way the only things in the cache will be things that have been explicitly changed (we will still want default checking but it makes life easier and means we don't end up having to do a full writeout for cases where things have been put into cache mode without a reset).
On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Apr 24, 2015 at 03:36:45PM -0700, Kevin Cernekee wrote: > >> index 116655d92269..ece122a6fdeb 100644 >> --- a/include/linux/regmap.h >> +++ b/include/linux/regmap.h >> @@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map); >> >> int regcache_sync(struct regmap *map); >> int regcache_sync_region(struct regmap *map, unsigned int min, >> - unsigned int max); >> + unsigned int max, bool was_reset); > > This seems pretty ugly - both the fact that we're changing the signature > of the function and the naming of the argument feel inelegant. The > point isn't if the device has been reset, the point is if the device > currently has the default register values or not, and this means that > the user is responsible for tracking that state until the next time it > does the sync. That may be immediately like in your case but there's no > reason that has to be the case. The fact that we're passing in > something called "is_reset" which sounds like a state value for the > register map is a bit of a warning sign here. > > What we should be doing here is providing a way for users to tell regmap > if they've reset the register map and actually we already have that > interface, it's just not got the best name - regcache_mark_dirty() is > effectively it since there's really not a lot of other reasons why a > driver would need to mark the cache as dirty. We're just not handling > it properly. What we should do instead is to keep the interface as it > is for now and make it behave in a more expected fashion so that if the > cache is explicitly marked dirty we assume that the hardware is in the > default state and otherwise we don't. > > Ideally what we'd do is both improve the naming of mark_dirty() (though > that's API churn which is nasty) and arrange for rbtree to cache the > default values lazily, that way the only things in the cache will be > things that have been explicitly changed (we will still want default > checking but it makes life easier and means we don't end up having to do > a full writeout for cases where things have been put into cache mode > without a reset). Hi Mark, I started prototyping this, but ran into a couple of issues: 1) How do we tell the difference between "regcache contains a non-default value that correctly reflects the hardware register contents" versus "regcache contains a non-default value that is waiting to be written when we exit cache_only mode"? 2) Does that also mean that we should store default values in the rbtree if they are part of a deferred cache_only write, but not store them if the write went through to the hardware? 3) If we're caching the default values lazily, does that mean that every regcache read would incur both an rbtree lookup and a bsearch of the reg_defaults array? 4) If "the only things in the cache will be things that have been explicitly changed," that could impact the semantics of regcache_drop_region(). Which fortunately has no users. Seems like it would be more straightforward just to add an rbnode->dirty bitmask alongside rbnode->cache_present, rather than trying to infer the hardware state from the presence/absence of the cache entry. Knowing whether each individual register is out of sync with the hardware lets us avoid unnecessary writes in both situations: full reset, and temporary loss of register access. What do you think?
On Tue, Apr 28, 2015 at 09:58:48PM -0700, Kevin Cernekee wrote: > On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <broonie@kernel.org> wrote: > > What we should be doing here is providing a way for users to tell regmap > > if they've reset the register map and actually we already have that > > interface, it's just not got the best name - regcache_mark_dirty() is > > effectively it since there's really not a lot of other reasons why a > > driver would need to mark the cache as dirty. We're just not handling > 1) How do we tell the difference between "regcache contains a > non-default value that correctly reflects the hardware register > contents" versus "regcache contains a non-default value that is > waiting to be written when we exit cache_only mode"? Like I said above we can tell if the hardware was reset because mark_dirty() is called. > 2) Does that also mean that we should store default values in the > rbtree if they are part of a deferred cache_only write, but not store > them if the write went through to the hardware? Well, remember that it's very expensive to remove a value from the cache so actively trying to prune the cache would be bad. > 3) If we're caching the default values lazily, does that mean that > every regcache read would incur both an rbtree lookup and a bsearch of > the reg_defaults array? That'd happen on first read, yes. > 4) If "the only things in the cache will be things that have been > explicitly changed," that could impact the semantics of > regcache_drop_region(). Which fortunately has no users. Could you articulate what changes you believe would be seen? > Seems like it would be more straightforward just to add an > rbnode->dirty bitmask alongside rbnode->cache_present, rather than > trying to infer the hardware state from the presence/absence of the > cache entry. Knowing whether each individual register is out of sync > with the hardware lets us avoid unnecessary writes in both situations: > full reset, and temporary loss of register access. I'm not suggesting that we do anything based on the presence of a cache entry, I'm suggesting that we could avoid having to ever cache values that never get referenced on a system (which can be a lot of them for common use cases) saving us memory. Maintaining a dirty bitmask would work too, but it does push the memory consumption up further which might be a concern.
On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Apr 28, 2015 at 09:58:48PM -0700, Kevin Cernekee wrote: >> On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <broonie@kernel.org> wrote: > >> > What we should be doing here is providing a way for users to tell regmap >> > if they've reset the register map and actually we already have that >> > interface, it's just not got the best name - regcache_mark_dirty() is >> > effectively it since there's really not a lot of other reasons why a >> > driver would need to mark the cache as dirty. We're just not handling > >> 1) How do we tell the difference between "regcache contains a >> non-default value that correctly reflects the hardware register >> contents" versus "regcache contains a non-default value that is >> waiting to be written when we exit cache_only mode"? > > Like I said above we can tell if the hardware was reset because > mark_dirty() is called. That covers the public API, but I do not understand how you intended for this data to be stored in the rbtree if the use of a dirty bitmask is discouraged. i.e. regcache_sync() finds a register value marked "present". How do we know whether we need to write it back to the hardware? For the special case of "cached non default register values immediately after a HW reset" you can mostly figure this out, but if there was no HW reset how do we know which entries changed while the HW was inaccessible? > I'm not suggesting that we do anything based on the presence of a cache > entry, I'm suggesting that we could avoid having to ever cache values > that never get referenced on a system (which can be a lot of them for > common use cases) saving us memory. This seems to be solving a different problem. It sounds like you are more worried about regcache_sync() writing back lots of default values for registers that were never touched, than performing unnecessary writes to a few (actively used) registers that weren't changed while we were in cache_only mode. Is that accurate? FWIW, in the current iteration of the tas571x driver, there are few if any registers that meet this criteria.
On Wed, Apr 29, 2015 at 07:13:27AM -0700, Kevin Cernekee wrote: > On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <broonie@kernel.org> wrote: > > Like I said above we can tell if the hardware was reset because > > mark_dirty() is called. > That covers the public API, but I do not understand how you intended > for this data to be stored in the rbtree if the use of a dirty bitmask > is discouraged. We just need a single boolean? > i.e. regcache_sync() finds a register value marked "present". How do > we know whether we need to write it back to the hardware? For the > special case of "cached non default register values immediately after > a HW reset" you can mostly figure this out, but if there was no HW > reset how do we know which entries changed while the HW was > inaccessible? In the first instance do we care? > > I'm not suggesting that we do anything based on the presence of a cache > > entry, I'm suggesting that we could avoid having to ever cache values > > that never get referenced on a system (which can be a lot of them for > > common use cases) saving us memory. > This seems to be solving a different problem. It sounds like you are > more worried about regcache_sync() writing back lots of default values > for registers that were never touched, than performing unnecessary > writes to a few (actively used) registers that weren't changed while > we were in cache_only mode. Is that accurate? No. This is nothing to do with sync, it's just something that might be nice.
On Wed, Apr 29, 2015 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 29, 2015 at 07:13:27AM -0700, Kevin Cernekee wrote: >> On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <broonie@kernel.org> wrote: > >> > Like I said above we can tell if the hardware was reset because >> > mark_dirty() is called. > >> That covers the public API, but I do not understand how you intended >> for this data to be stored in the rbtree if the use of a dirty bitmask >> is discouraged. > > We just need a single boolean? Right, so if we add a per-regmap bool that tells us whether the device has been reset, then in the case of "not reset" we will have to write every regcache entry out to the device. Even the ones that weren't touched while in cache_only mode. This makes the "not reset" case much less efficient than the "reset" case. Maybe that's good enough for most purposes. It's no worse than what my original patch submission did, anyway. BTW, any preferences on naming for the bool or for the renamed mark_dirty function? >> i.e. regcache_sync() finds a register value marked "present". How do >> we know whether we need to write it back to the hardware? For the >> special case of "cached non default register values immediately after >> a HW reset" you can mostly figure this out, but if there was no HW >> reset how do we know which entries changed while the HW was >> inaccessible? > > In the first instance do we care? I'm not sure I understand the question. >> > I'm not suggesting that we do anything based on the presence of a cache >> > entry, I'm suggesting that we could avoid having to ever cache values >> > that never get referenced on a system (which can be a lot of them for >> > common use cases) saving us memory. > >> This seems to be solving a different problem. It sounds like you are >> more worried about regcache_sync() writing back lots of default values >> for registers that were never touched, than performing unnecessary >> writes to a few (actively used) registers that weren't changed while >> we were in cache_only mode. Is that accurate? > > No. This is nothing to do with sync, it's just something that might be > nice. Thanks, that clarifies things. I was not aware that other drivers even had an issue with excessively large regcache rbtrees, as my reg_defaults list is short.
On Wed, Apr 29, 2015 at 10:02:03AM -0700, Kevin Cernekee wrote: > On Wed, Apr 29, 2015 at 9:46 AM, Mark Brown <broonie@kernel.org> wrote: > > We just need a single boolean? > Right, so if we add a per-regmap bool that tells us whether the device > has been reset, then in the case of "not reset" we will have to write > every regcache entry out to the device. Even the ones that weren't > touched while in cache_only mode. This makes the "not reset" case > much less efficient than the "reset" case. The immediate point is to provide a useful external interface, the internal optimisation is much less urgent (and it's less clear to me how often we're going to need to do that at all - generally cache only and power loss go together. > BTW, any preferences on naming for the bool or for the renamed > mark_dirty function? Not urgently. Note that I'm *not* suggesting an immediate rename, that's definitely a separate change (which will be a lot more painful to merge due to cross tree issues). > >> i.e. regcache_sync() finds a register value marked "present". How do > >> we know whether we need to write it back to the hardware? For the > >> special case of "cached non default register values immediately after > >> a HW reset" you can mostly figure this out, but if there was no HW > >> reset how do we know which entries changed while the HW was > >> inaccessible? > > In the first instance do we care? > I'm not sure I understand the question. Do we actually care about getting a list of the changed registers that much?
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index a13587b5c2be..89dfefeb168e 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -155,7 +155,8 @@ struct regcache_ops { #endif int (*read)(struct regmap *map, unsigned int reg, unsigned int *value); int (*write)(struct regmap *map, unsigned int reg, unsigned int value); - int (*sync)(struct regmap *map, unsigned int min, unsigned int max); + int (*sync)(struct regmap *map, unsigned int min, unsigned int max, + bool was_reset); int (*drop)(struct regmap *map, unsigned int min, unsigned int max); }; @@ -215,7 +216,7 @@ int regcache_sync(struct regmap *map); int regcache_sync_block(struct regmap *map, void *block, unsigned long *cache_present, unsigned int block_base, unsigned int start, - unsigned int end); + unsigned int end, bool was_reset); static inline const void *regcache_get_val_addr(struct regmap *map, const void *base, diff --git a/drivers/base/regmap/regcache-lzo.c b/drivers/base/regmap/regcache-lzo.c index 2d53f6f138e1..52ed0d03ce69 100644 --- a/drivers/base/regmap/regcache-lzo.c +++ b/drivers/base/regmap/regcache-lzo.c @@ -332,7 +332,7 @@ out: } static int regcache_lzo_sync(struct regmap *map, unsigned int min, - unsigned int max) + unsigned int max, bool was_reset) { struct regcache_lzo_ctx **lzo_blocks; unsigned int val; diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c index 81751a49d8bf..8fc1727e635c 100644 --- a/drivers/base/regmap/regcache-rbtree.c +++ b/drivers/base/regmap/regcache-rbtree.c @@ -445,7 +445,7 @@ static int regcache_rbtree_write(struct regmap *map, unsigned int reg, } static int regcache_rbtree_sync(struct regmap *map, unsigned int min, - unsigned int max) + unsigned int max, bool was_reset) { struct regcache_rbtree_ctx *rbtree_ctx; struct rb_node *node; @@ -477,7 +477,8 @@ static int regcache_rbtree_sync(struct regmap *map, unsigned int min, ret = regcache_sync_block(map, rbnode->block, rbnode->cache_present, - rbnode->base_reg, start, end); + rbnode->base_reg, start, end, + was_reset); if (ret != 0) return ret; } diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index 7eb7b3b98794..d27b45f50497 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -250,7 +250,7 @@ int regcache_write(struct regmap *map, } static int regcache_default_sync(struct regmap *map, unsigned int min, - unsigned int max) + unsigned int max, bool was_reset) { unsigned int reg; @@ -266,10 +266,12 @@ static int regcache_default_sync(struct regmap *map, unsigned int min, if (ret) return ret; - /* Is this the hardware default? If so skip. */ - ret = regcache_lookup_reg(map, reg); - if (ret >= 0 && val == map->reg_defaults[ret].def) - continue; + if (was_reset) { + /* Is this the hardware default? If so skip. */ + ret = regcache_lookup_reg(map, reg); + if (ret >= 0 && val == map->reg_defaults[ret].def) + continue; + } map->cache_bypass = 1; ret = _regmap_write(map, reg, val); @@ -294,6 +296,10 @@ static int regcache_default_sync(struct regmap *map, unsigned int min, * volatile. In general drivers can choose not to use the provided * syncing functionality if they so require. * + * This assumes that the hardware registers contain their default values. + * If the cached value matches the default value for a register, the write + * operation will be skipped. + * * Return a negative value on failure, 0 on success. */ int regcache_sync(struct regmap *map) @@ -331,9 +337,9 @@ int regcache_sync(struct regmap *map) map->cache_bypass = 0; if (map->cache_ops->sync) - ret = map->cache_ops->sync(map, 0, map->max_register); + ret = map->cache_ops->sync(map, 0, map->max_register, true); else - ret = regcache_default_sync(map, 0, map->max_register); + ret = regcache_default_sync(map, 0, map->max_register, true); if (ret == 0) map->cache_dirty = false; @@ -353,19 +359,18 @@ out: EXPORT_SYMBOL_GPL(regcache_sync); /** - * regcache_sync_region: Sync part of the register cache with the hardware. + * regcache_sync_region: Sync part of the register cache with the hardware. * * @map: map to sync. * @min: first register to sync * @max: last register to sync - * - * Write all non-default register values in the specified region to - * the hardware. + * @was_reset: true if the hardware is known to contain default register values + * in the given range; false otherwise * * Return a negative value on failure, 0 on success. */ int regcache_sync_region(struct regmap *map, unsigned int min, - unsigned int max) + unsigned int max, bool was_reset) { int ret = 0; const char *name; @@ -389,9 +394,9 @@ int regcache_sync_region(struct regmap *map, unsigned int min, map->async = true; if (map->cache_ops->sync) - ret = map->cache_ops->sync(map, min, max); + ret = map->cache_ops->sync(map, min, max, was_reset); else - ret = regcache_default_sync(map, min, max); + ret = regcache_default_sync(map, min, max, was_reset); out: /* Restore the bypass state */ @@ -600,7 +605,8 @@ static bool regcache_reg_present(unsigned long *cache_present, unsigned int idx) static int regcache_sync_block_single(struct regmap *map, void *block, unsigned long *cache_present, unsigned int block_base, - unsigned int start, unsigned int end) + unsigned int start, unsigned int end, + bool was_reset) { unsigned int i, regtmp, val; int ret; @@ -614,10 +620,12 @@ static int regcache_sync_block_single(struct regmap *map, void *block, val = regcache_get_val(map, block, i); - /* Is this the hardware default? If so skip. */ - ret = regcache_lookup_reg(map, regtmp); - if (ret >= 0 && val == map->reg_defaults[ret].def) - continue; + if (was_reset) { + /* Is this the hardware default? If so skip. */ + ret = regcache_lookup_reg(map, regtmp); + if (ret >= 0 && val == map->reg_defaults[ret].def) + continue; + } map->cache_bypass = 1; @@ -667,7 +675,7 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data, static int regcache_sync_block_raw(struct regmap *map, void *block, unsigned long *cache_present, unsigned int block_base, unsigned int start, - unsigned int end) + unsigned int end, bool was_reset) { unsigned int i, val; unsigned int regtmp = 0; @@ -689,14 +697,17 @@ static int regcache_sync_block_raw(struct regmap *map, void *block, val = regcache_get_val(map, block, i); - /* Is this the hardware default? If so skip. */ - ret = regcache_lookup_reg(map, regtmp); - if (ret >= 0 && val == map->reg_defaults[ret].def) { - ret = regcache_sync_block_raw_flush(map, &data, - base, regtmp); - if (ret != 0) - return ret; - continue; + if (was_reset) { + /* Is this the hardware default? If so skip. */ + ret = regcache_lookup_reg(map, regtmp); + if (ret >= 0 && val == map->reg_defaults[ret].def) { + ret = regcache_sync_block_raw_flush(map, &data, + base, + regtmp); + if (ret != 0) + return ret; + continue; + } } if (!data) { @@ -712,12 +723,14 @@ static int regcache_sync_block_raw(struct regmap *map, void *block, int regcache_sync_block(struct regmap *map, void *block, unsigned long *cache_present, unsigned int block_base, unsigned int start, - unsigned int end) + unsigned int end, bool was_reset) { if (regmap_can_raw_write(map) && !map->use_single_rw) return regcache_sync_block_raw(map, block, cache_present, - block_base, start, end); + block_base, start, end, + was_reset); else return regcache_sync_block_single(map, block, cache_present, - block_base, start, end); + block_base, start, end, + was_reset); } diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c index dccf58691650..ff4785f0416d 100644 --- a/drivers/media/radio/radio-si476x.c +++ b/drivers/media/radio/radio-si476x.c @@ -567,19 +567,22 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio, /* regcache_mark_dirty(radio->core->regmap); */ err = regcache_sync_region(radio->core->regmap, SI476X_PROP_DIGITAL_IO_INPUT_SAMPLE_RATE, - SI476X_PROP_DIGITAL_IO_OUTPUT_FORMAT); + SI476X_PROP_DIGITAL_IO_OUTPUT_FORMAT, + true); if (err < 0) return err; err = regcache_sync_region(radio->core->regmap, SI476X_PROP_AUDIO_DEEMPHASIS, - SI476X_PROP_AUDIO_PWR_LINE_FILTER); + SI476X_PROP_AUDIO_PWR_LINE_FILTER, + true); if (err < 0) return err; err = regcache_sync_region(radio->core->regmap, SI476X_PROP_INT_CTL_ENABLE, - SI476X_PROP_INT_CTL_ENABLE); + SI476X_PROP_INT_CTL_ENABLE, + true); if (err < 0) return err; @@ -589,13 +592,15 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio, */ err = regcache_sync_region(radio->core->regmap, SI476X_PROP_VALID_MAX_TUNE_ERROR, - SI476X_PROP_VALID_MAX_TUNE_ERROR); + SI476X_PROP_VALID_MAX_TUNE_ERROR, + true); if (err < 0) return err; err = regcache_sync_region(radio->core->regmap, SI476X_PROP_VALID_SNR_THRESHOLD, - SI476X_PROP_VALID_RSSI_THRESHOLD); + SI476X_PROP_VALID_RSSI_THRESHOLD, + true); if (err < 0) return err; @@ -609,7 +614,8 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio, err = regcache_sync_region(radio->core->regmap, SI476X_PROP_FM_RDS_INTERRUPT_SOURCE, - SI476X_PROP_FM_RDS_CONFIG); + SI476X_PROP_FM_RDS_CONFIG, + true); if (err < 0) return err; } diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 53ae5af5d6e4..d5632634a362 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -158,14 +158,15 @@ static int wm8994_suspend(struct device *dev) * pin configurations. */ ret = regcache_sync_region(wm8994->regmap, WM8994_GPIO_1, - WM8994_GPIO_11); + WM8994_GPIO_11, true); if (ret != 0) dev_err(dev, "Failed to restore GPIO registers: %d\n", ret); /* In case one of the GPIOs is used as a wake input. */ ret = regcache_sync_region(wm8994->regmap, WM8994_INTERRUPT_STATUS_1_MASK, - WM8994_INTERRUPT_STATUS_1_MASK); + WM8994_INTERRUPT_STATUS_1_MASK, + true); if (ret != 0) dev_err(dev, "Failed to restore interrupt mask: %d\n", ret); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 116655d92269..ece122a6fdeb 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map); int regcache_sync(struct regmap *map); int regcache_sync_region(struct regmap *map, unsigned int min, - unsigned int max); + unsigned int max, bool was_reset); int regcache_drop_region(struct regmap *map, unsigned int min, unsigned int max); void regcache_cache_only(struct regmap *map, bool enable); @@ -683,7 +683,7 @@ static inline int regcache_sync(struct regmap *map) } static inline int regcache_sync_region(struct regmap *map, unsigned int min, - unsigned int max) + unsigned int max, bool was_reset) { WARN_ONCE(1, "regmap API is disabled"); return -EINVAL; diff --git a/include/sound/hda_regmap.h b/include/sound/hda_regmap.h index 53a18b3635e2..f6bb68137ca4 100644 --- a/include/sound/hda_regmap.h +++ b/include/sound/hda_regmap.h @@ -211,7 +211,8 @@ static inline void snd_hdac_regmap_sync_node(struct hdac_device *codec, hda_nid_t nid) { regcache_mark_dirty(codec->regmap); - regcache_sync_region(codec->regmap, nid << 20, ((nid + 1) << 20) - 1); + regcache_sync_region(codec->regmap, nid << 20, + ((nid + 1) << 20) - 1, true); } #endif /* __SOUND_HDA_REGMAP_H */ diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index 118b0034ba23..3a25d2d93705 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -1483,7 +1483,8 @@ static int wm8962_dsp2_write_config(struct snd_soc_codec *codec) struct wm8962_priv *wm8962 = snd_soc_codec_get_drvdata(codec); return regcache_sync_region(wm8962->regmap, - WM8962_HDBASS_AI_1, WM8962_MAX_REGISTER); + WM8962_HDBASS_AI_1, WM8962_MAX_REGISTER, + true); } static int wm8962_dsp2_set_enable(struct snd_soc_codec *codec, u16 val)
regcache_sync() and regcache_sync_region() currently assume that the hardware has just emerged from a clean reset, and that all registers are in their default states. But that isn't the only possibility; the device may have been in a different state in which the registers were inaccessible but have retained their contents, e.g. clock gating. So we will extend the more versatile of the two functions, regcache_sync_region(), to let the caller decide what assumptions should be made. One driver that can benefit from this is adau1977, which has hacks to overwrite the registers that regcache_sync() might have missed. Also, the powerdown pin on tas571x does not reset the register contents either, so a similar feature will be required by that driver. This commit just adds the new argument by changing the function declarations and call sites, but doesn't wire it up yet. Signed-off-by: Kevin Cernekee <cernekee@chromium.org> --- drivers/base/regmap/internal.h | 5 ++- drivers/base/regmap/regcache-lzo.c | 2 +- drivers/base/regmap/regcache-rbtree.c | 5 ++- drivers/base/regmap/regcache.c | 75 ++++++++++++++++++++--------------- drivers/media/radio/radio-si476x.c | 18 ++++++--- drivers/mfd/wm8994-core.c | 5 ++- include/linux/regmap.h | 4 +- include/sound/hda_regmap.h | 3 +- sound/soc/codecs/wm8962.c | 3 +- 9 files changed, 72 insertions(+), 48 deletions(-)