Re: [PATCH v2 02/10] mtd: rawnand: denali: refactor syndrome layout handling for raw access

[Masahiro Yamada <yamada.masahiro@socionext.com>]

Hi Miquel,

On Mon, Mar 4, 2019 at 6:01 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Masahiro,
>
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Tue, 12 Feb
> 2019 16:12:54 +0900:
>
> > The Denali IP adopts the syndrome page layout (payload and ECC are
> > interleaved). The *_page_raw() and *_oob() callbacks are complicated
> > because they must hide the underlying layout used by the hardware,
> > and always return contiguous in-band and out-of-band data.
> >
> > Currently, similar code is duplicated to reorganize the data layout.
> > For example, denali_read_page_raw() and denali_write_page_raw() look
> > almost the same.
> >
> > The idea for refactoring is to split the code into two parts:
> >   [1] conversion of page layout
> >   [2] what to do at every ECC chunk boundary
> >
> > For [1], I wrote denali_raw_payload_op() and denali_raw_oob_op().
> > They manipulate data for the Denali controller's specific page layout
> > of in-band, out-of-band, respectively.
>
> Could you please comment the purpose of these two functions in the code
> as well?


OK, I will.



> >
> > The difference between write and read is just the operation at
> > ECC chunk boundaries. For example, denali_read_oob() calls
> > nand_change_read_column_op(), whereas denali_write_oob() calls
> > nand_change_write_column_op(). So, I implemented [2] as a callback
> > passed into [1].
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > ---
> >
> > Changes in v2: None
> >
> > drivers/mtd/nand/raw/denali.c | 354 +++++++++++++++++++-----------------------
> > 1 file changed, 163 insertions(+), 191 deletions(-)
>
> Too bad that the size of the driver did not shrink more than that :)

Indeed, less than expected.

But, please do not miss this commit is adding
error check to every operation.

Prior to this commit, the code just ignored the return code
because 97d90da8a886949f simply replaced old hooks
despite the new ones return the error code.


Generally, every error check costs two lines
in the following form:


  ret = (do something)
  if (ret)
           return ret;



> > +
> > +static int denali_memcpy_in(void *buf, unsigned int offset, unsigned int len,
> > +                         void *priv)
> > +{
> > +     memcpy(buf, priv + offset, len);
> > +     return 0;
> >  }
>
> Maybe this "callback" and the (_out cousin) could be part of you
> controller's structure,
> and you could use a read/write flag instead of
> passing the functions' pointer?

This is what the old code does.

There are 4 callbacks for the combination
of raw/oob, and write/read.

I do not know how your suggestion looks like,
and whether it will make the code cleaner.



--
Best Regards
Masahiro Yamada