[PATCH v2 1/3] ARM: Introduce atomic MMIO clear/set

[ezequiel.garcia@free-electrons.com (Ezequiel Garcia)]

Russell,

On Tue, Aug 20, 2013 at 10:08:39PM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 20, 2013 at 01:48:25PM -0300, Ezequiel Garcia wrote:
> > Based on a similar approach suggested by Russel King:
> 
> Russell please.
> 
> We Russells get upset when our names are incorrectly spelt, just like
> others get upset if they end up with extra letters in their names, or
> you confuse Steven vs Stephen.  Or even dare call a Deborah "Debs"
> (I did that once and the result was not particularly nice!)
> 

Ouch... sorry about that!

> > +void atomic_io_clear_set(u32 clear, u32 set, void __iomem *reg)
> > +{
> > +	spin_lock(&__io_lock);
> > +	writel((readl(reg) & ~clear) | set, reg);
> > +	/* ensure the write get done before unlocking */
> > +	__iowmb();
> > +	spin_unlock(&__io_lock);
> > +}
> > +EXPORT_SYMBOL(atomic_io_clear_set);
> 
> Some comments - neither of them you _have_ to act on:
> 
> 1. writel((readl(reg) & ~mask) | (set & mask), reg) could be deemed
>    to give better semantics - consider that if you don't look at the
>    implementation, how do you know what the result of setting a bit
>    in both the set & clear masks would be?
> 
> 2. A historical note, that back in the 1980s with things like the BBC
>    micro, this kind of operation was defined:
> 
> 	new_value = (old_value & mask) ^ value
> 
>    which has the flexibility of being able to set, clear or toggle any
>    bit.  I'm not saying that's a good interface, I'm merely pointing
>    out that the problem of being able to set and clear bits is nothing
>    new and other solutions are available. :)
> 
> 3. Would it be better to separate these by having atomic_io_clear() and
>    atomic_io_set() functions?
> 
> Just some things to think about; I have no overall preference here.

Indeed, I don't have any strong opinions on any of the above.
However, I'm a bit inclined to your proposal in (1), which coincides with:

int regmap_update_bits(struct regmap *map, unsigned int reg,
                       unsigned int mask, unsigned int val)

This looks a lot less intuitive to me, but more flexible.

Any other thoughts?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com