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

[Marek Behún <kabel@xxxxxxxxxx>]

On Fri, May 03, 2024 at 08:33:25PM +0300, Andy Shevchenko wrote:
> 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.

I will try something, altough I fear it might need 64-bit operations,
which may be even more suboptimal since the driver is supposed to run on
armv7.

Also, isn't this premature optimization? Since the computed length is then
passed as a parameter to I2C reading, and the I2C bus is orders of
magnitudes slower and the handling of I2C requests in Marvell I2C driver
will kill any optimization I do here...