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.
Thanks,
Charles
