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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jan 25, 2024 at 12:42:13AM +0200, Andy Shevchenko wrote:
> On Wed, Jan 24, 2024 at 6:56 PM Charles Keepax
> <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Refactor the code in cs42l43_mask_to_slots to use for_each_set_bit.
> 
> ..._bit()
> 
> ...
> 
> >  #include <linux/bitops.h>
> 
> > +#include <linux/bits.h>
> > +#include <linux/find.h>
> 
> No need, it's included by bitops.h (and there is kinda guarantee for these).
> 

We just moved a bunch of includes out of the cs42l43 header files
to be directly included in the C files. It makes sense to be
consistent if each file is going to directly include each header
it uses it should do so. The header guards will weed out what is
already included.

> > +               if (i == CS42L43_ASP_MAX_CHANNELS) {
> > +                       dev_warn(priv->dev, "Too many channels in TDM mask: %lx\n",
> > +                                mask);
> 
> This is invariant to the loop, you may check even before it (I'm
> writing by memory, might have made mistake(s) in the snippet):
> 
>   nslots = hweight_long(mask);
>   if (nslots >= ..._MAX_CHANNELS)
>     dev_warn(...);
> 
> >                         return;
> > +               }
> 

This would result in a change of behaviour, as one would need to
return before the loop to ensure we didn't overflow the slots
array. We could possible do something with masking the slots to
ensure it only has the right number of bits set, but it is really
starting to get a little micro-optimisation for something that is
likely only going to run once whilst the kernel boots.

Thanks,
Charles