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

[Bas Mevissen]

On 2021-05-28 13:55, Sergey Ryazanov wrote:
> 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?
> 

The original comment with some spelling/grammar corrections looked fine. 
Having some hint that something is deliberately done at that stage is 
sufficient IMHO.

>>> +      * (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.
> 

Agreed. It actually sounds like a good practice. Learned something today 
:-)

> 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.

Agreed. For that reason, I stopped trying to align such lists. It 
usually gets messy anyway:

if you start with something like:

short_var.x   = foo;
short_var.val = 100;

and add next week:
long_long_long_name.y                  = blabla;
long_long_long_name.long_other_name    = blabla;

the file as a whole still looks messy.

I would say, just fix up the comment a bit and send a V2.

Bas.