Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, May 03, 2024 at 10:43:06AM +0200, Marek Behún wrote:
> On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:

...

> > > +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> > 
> > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
> > in a few places.
> 
> It is used 3 times:
>   rlen = ((__fls(rising) >> 3) << 1) + 1;
>   flen = ((__fls(falling) >> 3) << 1) + 2;
>   err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> 
> The last one is not compatible with the first two (because of the "<< 1").
> 
> The first two instances are contained within a function that is dedicated
> to "computing needed reply length".
> 
> I could probably do something like
> 
>   static inline unsigned int
>   omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset)
>   {
>           return ((__fls(mask) >> 3) << interleaved) + 1 + offset;
>   }
> 
> Then the 3 instances would become
> 
>   rlen = omnia_compute_reply_len(rising, true, 0);
>   flen = omnia_compute_reply_len(falling, true, 1);
>   err = omnia_cmd_read(client, cmd, &reply,
>                        omnia_compute_reply_len(bits, false, 0));
> 
> What do you think?

Fine, but think about these bit operations, I believe it can be optimised.

-- 
With Best Regards,
Andy Shevchenko