Re: [PATCH 3/3] [regmap] [RESEND] Add "no-bus" option for regmap API

[Andrey Smirnov]

On Sun, Jan 13, 2013 at 3:18 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Jan 12, 2013 at 12:54:14PM -0800, Andrey Smirnov wrote:
>
>> +     bool cache_registers;
>> +
>
> I'm afraid I don't quite understand this...
>
>> -     if (!map->cache_bypass && map->format.format_write) {
>> +     if (!map->cache_bypass && map->cache_registers) {
>>               ret = regcache_write(map, reg, val);
>
> ...I think it's mostly to service this check here, but looking at the
> code I can't quite think why the code is doing what it's doing - I
> suspect we should just remove the check for format_write() here.  I
> think it was there to support potential bulk writes from the cache code
> (which we don't do yet) but we're not actually doing those and it's not
> clear that this should be doing that anyway.
>

>From my understanding of the code it is done because the caching is
handled differently for cases when format_write() and format_reg(),
format_val() are provided.
In case of 'format_write' the regcache_write() is called in
_regmap_write() directly whereas when format_reg(), format_val() are
there _regmap_write() calls _regmap_raw_write() which in turn calls
regcache_write(). If I remove that variable and corresponding check
then regcache_write() would be called twice in the case of
format_reg(), format_val(), when _regmap_write() is called, would it
not? I apologise if I miss something obvious and that is not a case(or
issue).

>>       int ret;
>> +     void *context = (map->bus) ? map : map->bus_context;
>> +
>
> Can you please make this a static inline regmap_map_get_context() or
> something?  The same thing appears in quite a few places and the terery
> operator isn't great at the best of times.
>

Will do in the next version of this patch.


Completely unrelated off-topic question: the following code in
regmap_bulk_write()

...

if (map->use_single_rw) {
    for (i = 0; i < val_count; i++) {
        ret = regmap_raw_write(map,
                                           reg + (i * map->reg_stride),
                                           val + (i * val_bytes),
                                           val_bytes);
        if (ret != 0)
            return ret;
    }
}

....

Would it not deadlock, since both regmap_bulk_write() and
regmap_raw_write() call map->lock(...)?
Once again, sorry for off-topic, I just didn't want to start a new
thread because of what very well may by my poor understanding of the
code(I'll submit a patch to change regmap_raw_write() to
_regmap_raw_write() If I am correct).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/