Re: [PATCH v3 3/6] mtd: set the ecc step size for master/slave mtd_info

[Brian Norris <computersforpeace@gmail.com>]

On Mon, Aug 12, 2013 at 05:44:57PM +0800, Huang Shijie wrote:
> 于 2013年08月12日 17:24, Gupta, Pekon 写道:
> >[Pekon]: Following Artem's recommendations.. Can this be named as
> >chip->ecc.size = ecc_step_size.
> >then chip->ecc.bytes  = ecc_syndrome_size (or ecc_code_size)
> yes, we can rename these fields in the future with another patch set.

Internal naming can be changed in the future if needed, but we really
need to get a good sysfs name now, since that becomes ABI. And it would
also be good to get this to be clearly consistent internally too.

So the main decision to be made here: what to name the sysfs field? It
currently stands as ecc_step, but for clarity to users (and for avoiding
internal confusion with nand_chip.ecc.steps), I think ecc_step_size is
just as good or better.

Then, we might as well have the mtd_info field be
mtd_info.ecc_step_size. So we would have the following names.

For ECC step size:

  /sys/class/mtd/mtdX/ecc_step_size
  mtd_info.ecc_step_size
  nand_chip.ecc.size    <--- candidate for renaming to ...ecc.step_size

For ECC strength:

  /sys/class/mtd/mtdX/ecc_strength
  mtd_info.ecc_strength
  nand_chip.ecc.strength

Other fields to consider for internal naming:

  # Number of ECC steps per page
  nand_chip.ecc.steps

  # Number of ECC bytes per ECC step
  nand_chip.ecc.bytes

Please, let's get an agreement on one of these, so we hopefully only
need at most a v4 patch series:

(1) Huang's current patch set (v3)
(2) My proposed naming above
(3) A third option? (AFAICT, (1) or (2) should be satisfactory)

If we go with (2), the v4 patch series only needs to take care of the
new sysfs naming (plus documentation) and the new mtd_info field naming.
Any internal consistency patches (e.g., for nand_ecc_ctrl) can come as
follow-ups.

> >In addition, chip->ecc.bytes should also be helpful for userspace
> >utility to determine how much bytes to reserve in spare-area for ECC.
> >So exposing that as sysfs entry is also good.
> >
> I do not need the chip->ecc.bytes. :)
> For me, export the chip->ecc.size is enough.

A better argument against this is that ecc.bytes does not necessarily
have utility across many types of NAND drivers by itself, since ECC
layouts differ. And at that point, we're trying to duplicate the
behavior of ioctl(ECCGETLAYOUT) (which notably has run out of room and
isn't 100% informative anymore).

> So you can submit a patch if you need this field.

Yes, if you have good reason for exporting it, send a patch and a good
argument. And it would need to explain why we can't just use
ECCGETLAYOUT.

Brian