| Message ID | 20211223002225.3738385-5-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | BCMA support for brcmnand | expand |
Hi Florian, f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800: > In order to initialize a given chip select object for use by the > brcmnand driver, move all of the Device Tree specific routines outside > of brcmnand_init_cs() in order to make it usable in a platform data > configuration which will be necessary for supporting BCMA chips. TBH I'm note a big fan of the idea. I'm not sure going back to supporting platform data this way really is a good idea... There are so much things that are well described with DT that we now rely upon that I am not entirely convinced by these changes :-/ The move is generally in the other direction: getting rid of the legacy platform data. > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Cheers, Miquèl > --- > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index 35f8d8e02d4a..60a7f375df83 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -2760,7 +2760,7 @@ static const struct nand_controller_ops brcmnand_controller_ops = { > .attach_chip = brcmnand_attach_chip, > }; > > -static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) > +static int brcmnand_init_cs(struct brcmnand_host *host) > { > struct brcmnand_controller *ctrl = host->ctrl; > struct device *dev = ctrl->dev; > @@ -2769,16 +2769,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) > int ret; > u16 cfg_offs; > > - ret = of_property_read_u32(dn, "reg", &host->cs); > - if (ret) { > - dev_err(dev, "can't get chip-select\n"); > - return -ENXIO; > - } > - > mtd = nand_to_mtd(&host->chip); > chip = &host->chip; > > - nand_set_flash_node(chip, dn); > nand_set_controller_data(chip, host); > mtd->name = devm_kasprintf(dev, GFP_KERNEL, "brcmnand.%d", > host->cs); > @@ -3179,7 +3172,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) > host->pdev = pdev; > host->ctrl = ctrl; > > - ret = brcmnand_init_cs(host, child); > + ret = of_property_read_u32(dn, "reg", &host->cs); > + if (ret) { > + dev_err(dev, "can't get chip-select\n"); > + devm_kfree(dev, host); > + continue; > + } > + > + nand_set_flash_node(&host->chip, dn); > + > + ret = brcmnand_init_cs(host); > if (ret) { > devm_kfree(dev, host); > continue; /* Try all chip-selects */
On 1/3/2022 8:56 AM, Miquel Raynal wrote: > Hi Florian, > > f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800: > >> In order to initialize a given chip select object for use by the >> brcmnand driver, move all of the Device Tree specific routines outside >> of brcmnand_init_cs() in order to make it usable in a platform data >> configuration which will be necessary for supporting BCMA chips. > > TBH I'm note a big fan of the idea. I'm not sure going back to > supporting platform data this way really is a good idea... There are so > much things that are well described with DT that we now rely upon that > I am not entirely convinced by these changes :-/ The move is generally > in the other direction: getting rid of the legacy platform data. In the cover letter there is an explanation as to why we need to introduce platform data/device support here: the platforms on which this NAND controller shim is used do not have Device Tree support, and won't have it in the future either. They are old platforms (first SoC supported by bcm47xx is maybe 15 years old now) but they are still in active and wide use by the OpenWrt, dd-wrt communities.
Hi Florian, f.fainelli@gmail.com wrote on Mon, 3 Jan 2022 09:27:28 -0800: > On 1/3/2022 8:56 AM, Miquel Raynal wrote: > > Hi Florian, > > > > f.fainelli@gmail.com wrote on Wed, 22 Dec 2021 16:22:20 -0800: > > > >> In order to initialize a given chip select object for use by the > >> brcmnand driver, move all of the Device Tree specific routines outside > >> of brcmnand_init_cs() in order to make it usable in a platform data > >> configuration which will be necessary for supporting BCMA chips. > > > > TBH I'm note a big fan of the idea. I'm not sure going back to > > supporting platform data this way really is a good idea... There are so > > much things that are well described with DT that we now rely upon that > > I am not entirely convinced by these changes :-/ The move is generally > > in the other direction: getting rid of the legacy platform data. > > In the cover letter there is an explanation as to why we need to introduce platform data/device support here: the platforms on which this NAND controller shim is used do not have Device Tree support, and won't have it in the future either. They are old platforms (first SoC supported by bcm47xx is maybe 15 years old now) but they are still in active and wide use by the OpenWrt, dd-wrt communities. Yeah, I read the cover letter, I understand these platforms won't ever be updated so you're stuck. I'll close my eyes. Thanks, Miquèl
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 35f8d8e02d4a..60a7f375df83 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -2760,7 +2760,7 @@ static const struct nand_controller_ops brcmnand_controller_ops = { .attach_chip = brcmnand_attach_chip, }; -static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) +static int brcmnand_init_cs(struct brcmnand_host *host) { struct brcmnand_controller *ctrl = host->ctrl; struct device *dev = ctrl->dev; @@ -2769,16 +2769,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) int ret; u16 cfg_offs; - ret = of_property_read_u32(dn, "reg", &host->cs); - if (ret) { - dev_err(dev, "can't get chip-select\n"); - return -ENXIO; - } - mtd = nand_to_mtd(&host->chip); chip = &host->chip; - nand_set_flash_node(chip, dn); nand_set_controller_data(chip, host); mtd->name = devm_kasprintf(dev, GFP_KERNEL, "brcmnand.%d", host->cs); @@ -3179,7 +3172,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) host->pdev = pdev; host->ctrl = ctrl; - ret = brcmnand_init_cs(host, child); + ret = of_property_read_u32(dn, "reg", &host->cs); + if (ret) { + dev_err(dev, "can't get chip-select\n"); + devm_kfree(dev, host); + continue; + } + + nand_set_flash_node(&host->chip, dn); + + ret = brcmnand_init_cs(host); if (ret) { devm_kfree(dev, host); continue; /* Try all chip-selects */
In order to initialize a given chip select object for use by the brcmnand driver, move all of the Device Tree specific routines outside of brcmnand_init_cs() in order to make it usable in a platform data configuration which will be necessary for supporting BCMA chips. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)