Re: [PATCH v2 1/2] ASoC: tegra210_adx: simplify byte map get/put logic

[Piyush Patle]

> > +static int tegra210_adx_write_map_ram(struct tegra210_adx *adx)
> >  {
> > +     const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->map) * BITS_PER_BYTE;
>
> Why are we multiplying by BITS_PER_BYTE here?  We've got a number of
> bits already from BITS_PER_TYPE().

Okay correct, that's a bug BITS_PER_TYPE() already returns bits,
so the extra * BITS_PER_BYTE is a unit error. It also references
the wrong type: the bitmap word is unsigned int (which is what the
original code's hard-coded 32 came from), not the map element type.

For v3 I'll change it to:
    const unsigned int bits_per_mask = BITS_PER_TYPE(*adx->byte_mask);

>
> > +     for (i = 0; i < adx->soc_data->ram_depth; i++) {
> > +             u32 word = 0;
> > +             int b;
> > +
> > +             for (b = 0; b < TEGRA_ADX_SLOTS_PER_WORD; b++) {
> > +                     unsigned int slot = i * TEGRA_ADX_SLOTS_PER_WORD + b;
> > +                     u16 val = adx->map[slot];
> > +
> > +                     if (val >= 256)
> > +                             continue;
> > +
> > +                     word |= (u32)val << (b * BITS_PER_BYTE);
> > +                     byte_mask[slot / bits_per_mask] |= 1U << (slot % bits_per_mask);
>
> How big can bits_per_mask get?

With the fix above, bits_per_mask == BITS_PER_TYPE(unsigned int) ==
32,
matching the original "slot / 32" / "slot % 32" expressions exactly.

>
> > @@ -118,9 +144,7 @@ static int tegra210_adx_runtime_resume(struct device *dev)
> >       regcache_cache_only(adx->regmap, false);
> >       regcache_sync(adx->regmap);
> >
> > -     tegra210_adx_write_map_ram(adx);
> > -
> > -     return 0;
> > +     return tegra210_adx_write_map_ram(adx);
>
> We need to unwind at least the regcache_cache_only() above if resume
> fails.

As per Jon's comment on the same patch, I'm moving the
byte_mask buffer back to a probe-time devm_kcalloc() in v3, so
write_map_ram() no longer has a failure path. runtime_resume() will
go back to returning 0 unconditionally and the regcache_cache_only()
unwind won't be needed. If write_map_ram() ever grows another failure
mode in the future, the unwind would have to be added then.

Thanks
Piyush Patle