| Message ID | 1444051772-20270-1-git-send-email-jon@ringle.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Oct 05, 2015 at 09:29:31AM -0400, jon@ringle.org wrote: > From: Jon Ringle <jringle@gridpoint.com> > > The only time that it makes sense to call a custom provided reg_update_bits > function, is the register being updated is one that has volatile bits. > Otherwise, the normal read/modify/write cycle (where the read is likely to > be free from the cache) will do just fine. This should keep the reg cache > intact, since volatile registers won't get cached anyway. Dave, to be clear please do *not* apply this patch at least for the time being - I've not reviewed it or the one from Thursday that you applied this morning.
From: Mark Brown <broonie@kernel.org> Date: Mon, 5 Oct 2015 15:57:22 +0100 > On Mon, Oct 05, 2015 at 09:29:31AM -0400, jon@ringle.org wrote: >> From: Jon Ringle <jringle@gridpoint.com> >> >> The only time that it makes sense to call a custom provided reg_update_bits >> function, is the register being updated is one that has volatile bits. >> Otherwise, the normal read/modify/write cycle (where the read is likely to >> be free from the cache) will do just fine. This should keep the reg cache >> intact, since volatile registers won't get cached anyway. > > Dave, to be clear please do *not* apply this patch at least for the time > being - I've not reviewed it or the one from Thursday that you applied > this morning. It's applied, it's pushed out to my tree, and therefore this will need to be fixed up with a relative patch of some sort. What you don't seem to understand is that my GIT tree is never rebased or mangled because many people depend upon it. So once a patch is applied, that commit lives on forever. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 05, 2015 at 11:29:56PM -0700, David Miller wrote: > From: Mark Brown <broonie@kernel.org> > > Dave, to be clear please do *not* apply this patch at least for the time > > being - I've not reviewed it or the one from Thursday that you applied > > this morning. > It's applied, it's pushed out to my tree, and therefore this will need to > be fixed up with a relative patch of some sort. This appears to be an incremental change, not the initial commit which you already applied. I'm asking you to stop applying changes to regmap which have not been reviewed. > What you don't seem to understand is that my GIT tree is never rebased > or mangled because many people depend upon it. So once a patch is > applied, that commit lives on forever. I'm not *so* concerned if the patch lives in history, I'm concerned with having something I can sensibly review and ideally getting the code into my tree.
> On Oct 6, 2015, at 6:13 AM, Mark Brown <broonie@kernel.org> wrote: > >> On Mon, Oct 05, 2015 at 11:29:56PM -0700, David Miller wrote: >> From: Mark Brown <broonie@kernel.org> > >>> Dave, to be clear please do *not* apply this patch at least for the time >>> being - I've not reviewed it or the one from Thursday that you applied >>> this morning. > >> It's applied, it's pushed out to my tree, and therefore this will need to >> be fixed up with a relative patch of some sort. > > This appears to be an incremental change, not the initial commit which > you already applied. I'm asking you to stop applying changes to regmap > which have not been reviewed. > >> What you don't seem to understand is that my GIT tree is never rebased >> or mangled because many people depend upon it. So once a patch is >> applied, that commit lives on forever. > > I'm not *so* concerned if the patch lives in history, I'm concerned with > having something I can sensibly review and ideally getting the code into > my tree. I would suggest the following course of action: 1) David, revert the following from net-next: $ git revert 9886ce2b9d4e5a8bb3d78d0f7eff3c0f1ed58d67 $ git revert 04fbfce7a222327b97ca165294ef19f0faa45960 $ git revert 7741c373cf3ea1f5383fa97fb7a640a429d3dd7c 2) Mark, please use the patch titled "[PATCH net-next v3 1/2] regmap: Allow installing custom". It will apply cleanly to the regmap for-next branch 3) Once Mark has accepted this patch into the regmap tree, I will make any adjustments needed to the net-next patch. 4) David should then merge the regmap for-next branch into net-next 5) I will submit a new patch to net-next for the encx24j600 driver that should build against the regmap changes Sound like a good plan? -Jon -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jon Ringle <jonringle@gmail.com> Date: Tue, 6 Oct 2015 08:50:43 -0400 > 4) David should then merge the regmap for-next branch into net-next Nope, this doesn't work at all. It is my tree which people depend upon, not the other way around. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jon Ringle <jonringle@gmail.com> Date: Tue, 6 Oct 2015 08:50:43 -0400 > 1) David, revert the following from net-next: > $ git revert 9886ce2b9d4e5a8bb3d78d0f7eff3c0f1ed58d67 > $ git revert 04fbfce7a222327b97ca165294ef19f0faa45960 > $ git revert 7741c373cf3ea1f5383fa97fb7a640a429d3dd7c Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 06, 2015 at 08:50:43AM -0400, Jon Ringle wrote: > 4) David should then merge the regmap for-next branch into net-next What I generally do (and what's best practice in general for cross tree work) is apply patches on topic branches and then create a signed tag for anything that's being merged into another tree. That both means we've got a signed tag for the chain of trust in the code and means that only the specific dependency gets pulled into other trees rather than everything that's queued. Minimising what's pulled in makes pull requests look cleaner and avoids noise from in development code appearing in other trees (eg, any bugs in other code that's not quite ready won't get merged). In my specific trees nobody should ever merge my for-FOO branches, they're rebuilt frequently - the topic branches are fast forward only and intended for that (though like I say a signed tag is best). Linus complains about keeping the for-FOO branches fast forward only. > 5) I will submit a new patch to net-next for the encx24j600 driver > that should build against the regmap changes > Sound like a good plan? Makes sense to me modulo the signed tag thing above.
On Tue, Oct 06, 2015 at 06:25:08AM -0700, David Miller wrote: > > 4) David should then merge the regmap for-next branch into net-next > Nope, this doesn't work at all. > It is my tree which people depend upon, not the other way around. Yes, it does work - this is the way we normally handle cross tree issues. There is nothing about pulling code from other trees into your tree which will stop other people depending on your tree, obviously anything you merge in needs to stay fast forward only and ideally not make any resulting pull requests look terrible but that's really the only restriction.
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 4036d7a..628ad7a 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -99,8 +99,7 @@ struct regmap { int (*reg_read)(void *context, unsigned int reg, unsigned int *val); int (*reg_write)(void *context, unsigned int reg, unsigned int val); int (*reg_update_bits)(void *context, unsigned int reg, - unsigned int mask, unsigned int val, - bool *change, bool force_write); + unsigned int mask, unsigned int val); bool defer_caching; diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 70387c9..8cd155a 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2510,44 +2510,26 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg, int ret; unsigned int tmp, orig; - if (map->reg_update_bits) { - ret = map->reg_update_bits(map->bus_context, reg, mask, val, - change, force_write); + if (change) + *change = false; + + if (regmap_volatile(map, reg) && map->reg_update_bits) { + ret = map->reg_update_bits(map->bus_context, reg, mask, val); + if (ret == 0 && change) + *change = true; + } else { + ret = _regmap_read(map, reg, &orig); if (ret != 0) return ret; - /* Fix up the cache by read/modify/write */ - if (!map->cache_bypass && !map->defer_caching) { - ret = regcache_read(map, reg, &orig); - if (ret != 0) - return ret; + tmp = orig & ~mask; + tmp |= val & mask; - tmp = orig & ~mask; - tmp |= val & mask; - - ret = regcache_write(map, reg, tmp); - if (ret != 0) - return ret; - if (map->cache_only) - map->cache_dirty = true; + if (force_write || (tmp != orig)) { + ret = _regmap_write(map, reg, tmp); + if (ret == 0 && change) + *change = true; } - return ret; - } - - ret = _regmap_read(map, reg, &orig); - if (ret != 0) - return ret; - - tmp = orig & ~mask; - tmp |= val & mask; - - if (force_write || (tmp != orig)) { - ret = _regmap_write(map, reg, tmp); - if (change) - *change = true; - } else { - if (change) - *change = false; } return ret; diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 4d3a3b1..b49d413 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -297,8 +297,7 @@ typedef int (*regmap_hw_reg_read)(void *context, unsigned int reg, typedef int (*regmap_hw_reg_write)(void *context, unsigned int reg, unsigned int val); typedef int (*regmap_hw_reg_update_bits)(void *context, unsigned int reg, - unsigned int mask, unsigned int val, - bool *change, bool force_write); + unsigned int mask, unsigned int val); typedef struct regmap_async *(*regmap_hw_async_alloc)(void); typedef void (*regmap_hw_free_context)(void *context);