On Wed, 2019-05-15 at 07:02 +0200, Mario Six wrote:
On Tue, May 14, 2019 at 3:53 PM Jagan Teki jagan@amarulasolutions.com wrote:
On Thu, May 2, 2019 at 2:37 PM Joakim Tjernlund Joakim.Tjernlund@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 jagan@amarulasolutions.com wrote:
- Mario
On Mon, Apr 29, 2019 at 2:48 PM Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Mon, 2019-04-29 at 01:58 +0530, Jagan Teki wrote: > From: Mario Six mario.six@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