Hello Bas, thank you for your review, please find my comments below.
On Fri, May 28, 2021 at 11:41 AM Bas Mevissen <[email protected]> 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 _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
