On Wed, 2019-05-15 at 07:02 +0200, Mario Six wrote:
> On Tue, May 14, 2019 at 3:53 PM Jagan Teki <ja...@amarulasolutions.com> wrote:
> > On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund
> > <joakim.tjernl...@infinera.com> wrote:
> > > On Thu, 2019-05-02 at 07:31 +0200, Mario Six wrote:
> > > > CAUTION: This email originated from outside of the organization. Do not
> > > > click links or open attachments unless you recognize the sender and
> > > > know the content is safe.
> > > >
> > > >
> > > > Hi Jagan and Jocke,
> > > >
> > > > I'm back from vacation, so here's my answer:
> > > >
> > > > On Mon, Apr 29, 2019 at 12:41 PM Jagan Teki
> > > > <ja...@amarulasolutions.com> wrote:
> > > > > + Mario
> > > > >
> > > > > On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund
> > > > > <joakim.tjernl...@infinera.com> wrote:
> > > > > > On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote:
> > > > > > > From: Mario Six <mario....@gdsys.cc>
> > > > > > >
> > > > > > > We do nothing in the loop if the "not empty" event was not
> > > > > > > detected. To
> > > > > > > simplify the logic, check if this is the case, and skip the
> > > > > > > execution of
> > > > > > > the loop early to reduce the nesting level and flag checking.
> > > > > >
> > > > > > Looked at the driver to refresh memory and noticed:
> > > > > > if (charSize == 32) {
> > > > > > /* Advance output buffer by 32 bits */
> > > > > > din += 4;
> > > > > > }
> > > > > > which suggests that only 32 bit char will increase the din ptr so
> > > > > > does other bitlens
> > > > > > work for reading?
> > > > Yes, It will work. When charSize < 32 in a loop execution, we
> > > > necessarily also
> > > > have numBlks == 0, since numBlks = DIV_ROUND_UP(bitlen, 32) and
> > > > charSize =
> > > > (bitlen >= 32 ? 32 : bitlen); in other words: we're at the very end of
> > > > the data
> > > > to be sent/received and also the final loop execution, so we don't have
> > > > to
> > > > advance the pointer anymore.
> > >
> > > Ahh, I see now.
> > >
> > > But this over use of always 32 bits cause complexity/subtile bugs. The
> > > driver should use
> > > the requested charsize(wordlen) throughout. As is now you cannot use the
> > > NF/NE flag as intended or
> > > toggle LSB_FIRST
> >
> > Look like Joakim has a point here. better we can check the required
> > charsize instead of looking for magic 32 number. What do you say
> > Mario?
>
> I agree, the driver code could be made more readable and cleaned up a bit, but
> I don't know if it's worth postponing this series for a code readability issue
> that's not a functional bug; I'd say that's an optimization that could be made
> later on.
I would not call it just an optimization but I am fine with applying this patch.
Jocke
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
