Re: [PATCH v2 6/7] ASoC: cs42l43: Refactor to use for_each_set_bit()

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Jan 29, 2024 at 6:27 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 25, 2024 at 09:11:44PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 25, 2024 at 12:31 PM Charles Keepax
> > <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > Adding nslots parameter is a good idea, but I still think the code can
> > be refactored better (have you checked the code generation, btw? I
> > believe my version would be better or not worse).
> >
> > > +       for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) {
> > > +               if (i == nslots) {
> > > +                       dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n",
> > > +                                mask);
> > >                         return;
> > > +               }
> > >
> > > +               slots[i++] = slot;
> > >         }
> >
> >        i = 0;
> >        for_each_set_bit(slot, &mask, CS42L43_ASP_MAX_CHANNELS)
> >                slots[i++] = slot;
> >
> >        if (hweight_long(mask) >= CS42L43_ASP_MAX_CHANNELS)
> >                dev_warn(priv->dev, "Too many channels in TDM mask\n");
> >
> > The code is simpler and behaviour is not changed.
>
> I don't think this works, the limit here is on the number of
> channels not on the position of those channels. The last parameter
> of for_each_set_bits appears to measure against the bit position
> not the number of set bits. So for example 0xFC000000 would be a
> valid 6 channel mask, but would result in no slot positions being
> set in the above code.

Ah, indeed. Then BITS_PER_LONG is the correct approach.

-- 
With Best Regards,
Andy Shevchenko