[PATCH] ath79: rb4xx-nand: fix 512 byte pages compatibility

[Sergey Ryazanov]

Hello Bas,

thank you for your review, please find my comments below.

On Fri, May 28, 2021 at 11:41 AM Bas Mevissen <abuse at basmevissen.nl> wrote:
> On 2021-05-28 00:27, Sergey Ryazanov wrote:

[skipped]

>> +static int rb4xx_nand_attach_chip(struct nand_chip *chip)
>> +{
>> +     struct mtd_info *mtd = nand_to_mtd(chip);
>> +
>> +     /*
>> +      * Now we knows flash parameters and can tweak OOB the layout for old
>
> Rephrase to something like:
> Knowing the flash parameters, we can tweak the OOB layout for old
>

Yeah, I am not happy with this comment too, but this is the best I was
able to compose at 01:00 :) I will rephrase it if V2 will be needed.

Here we need a comment that briefly and clearly states that the NAND
params become known at this particular moment of initialization.
Before this moment, we do not know the page size, after this moment it
is too late to reconfigure something. Do you have any thoughts?

>> +      * (usually 64MiB) flashes.
>> +      *
>> +      * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB
>> +      * bootloader will not be able to find the kernel that we load.
>> +      */
>> +     if (mtd->writesize == 512)
>> +             mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops);
>> +
>> +     return 0;
>> +}
>> +
>>  static u8 rb4xx_nand_read_byte(struct nand_chip *chip)
>>  {
>>       struct rb4xx_nand *nand = chip->priv;
>> @@ -135,6 +152,10 @@ static int rb4xx_nand_dev_ready(struct nand_chip
>> *chip)
>>       return gpiod_get_value_cansleep(nand->rdy);
>>  }
>>
>> +static const struct nand_controller_ops rb4xx_nand_controller_ops = {
>> +     .attach_chip = rb4xx_nand_attach_chip,
>
> remove the ,

I intentionally placed the comma here to make text git-friendly. If we
will need to define more callbacks here, then we will just add a new
new line, without modifying the earlier added lines.

E.g. if commit this code without the comma, then a chip detaching
callback patch will looks like this:

 static const struct nand_controller_ops rb4xx_nand_controller_ops = {
-     .attach_chip = rb4xx_nand_attach_chip
+     .attach_chip = rb4xx_nand_attach_chip,
+     .detach_chip = rb4xx_nand_detach_chip,
 };

With this intentionally placed comma, the same theoretical patch will
looks like this:

 static const struct nand_controller_ops rb4xx_nand_controller_ops = {
      .attach_chip = rb4xx_nand_attach_chip,
+     .detach_chip = rb4xx_nand_detach_chip,
 };

So this comma is my investment in the future ;)

>> +};
>> +
>>  static int rb4xx_nand_probe(struct platform_device *pdev)
>>  {
>>       struct device *dev = &pdev->dev;
>> @@ -185,9 +206,6 @@ static int rb4xx_nand_probe(struct platform_device
>> *pdev)
>>       mtd->dev.parent = dev;
>>       mtd_set_of_node(mtd, dev->of_node);
>>
>> -     if (mtd->writesize == 512)
>> -             mtd_set_ooblayout(mtd, &rb4xx_nand_ecclayout_ops);
>> -
>>       nand->chip.ecc.mode     = NAND_ECC_SOFT;
>>       nand->chip.ecc.algo     = NAND_ECC_HAMMING;
>>       nand->chip.options      = NAND_NO_SUBPAGE_WRITE;
>> @@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device
>> *pdev)
>>       nand->chip.legacy.cmd_ctrl      = rb4xx_nand_cmd_ctrl;
>>       nand->chip.legacy.dev_ready     = rb4xx_nand_dev_ready;
>>       nand->chip.legacy.chip_delay    = 25;
>> +     nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops;
>
> Fix indentation for all nand->something assignments to line up the =
> sign

I do not think that this is a good idea for this patch. Here we add
one line that configures the single field deep inside the structure.
The line is placed after the configuration of "surface" fields. So the
overall look should not be harmed dreadfully.

In case we will rework the indentation with this patch, the 57 lines
patch will become a ~70 lines patch. Where at least 12 lines will
rework indentation, so the functional part could be easily lost. Also
the moving text back and forth within an item line, turns the history
to the white noise and makes git-blame(1) usage a pain.

If you prefer, then I could insert an empty line before the ops setup
to make it nice looking. E.g. turn this hunk:

@@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device *pdev)
      nand->chip.legacy.cmd_ctrl      = rb4xx_nand_cmd_ctrl;
      nand->chip.legacy.dev_ready     = rb4xx_nand_dev_ready;
      nand->chip.legacy.chip_delay    = 25;
+     nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops;

into this:

@@ -199,6 +217,7 @@ static int rb4xx_nand_probe(struct platform_device *pdev)
      nand->chip.legacy.cmd_ctrl      = rb4xx_nand_cmd_ctrl;
      nand->chip.legacy.dev_ready     = rb4xx_nand_dev_ready;
      nand->chip.legacy.chip_delay    = 25;
+
+     nand->chip.legacy.dummy_controller.ops = &rb4xx_nand_controller_ops;

But I do not think this rework is worth the V2 on its own.

-- 
Sergey