Hi Marek,
Marek Vasut <marex@xxxxxxx> wrote on Mon, 27 Apr 2020 19:59:34 +0200:
> On 4/27/20 7:47 PM, Miquel Raynal wrote:
> > Hi Christophe,
> >
> > Christophe Kerello <christophe.kerello@xxxxxx> wrote on Wed, 15 Apr
> > 2020 17:57:28 +0200:
> >
> >> This patch defers its probe when the expected reset control is not
> >> yet ready. This patch also handles properly all errors cases at probe
> >> time.
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@xxxxxx>
> >> ---
> >> drivers/mtd/nand/raw/stm32_fmc2_nand.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> index b6d45cd..0a96797 100644
> >> --- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> @@ -1967,7 +1967,11 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >> }
> >>
> >> rstc = devm_reset_control_get(dev, NULL);
> >> - if (!IS_ERR(rstc)) {
> >> + if (IS_ERR(rstc)) {
> >> + ret = PTR_ERR(rstc);
> >> + if (ret == -EPROBE_DEFER)
> >> + goto err_clk_disable;
> >> + } else {
> >> reset_control_assert(rstc);
> >> reset_control_deassert(rstc);
> >> }
> >> @@ -1975,7 +1979,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >> /* DMA setup */
> >> ret = stm32_fmc2_dma_setup(fmc2);
> >> if (ret)
> >> - return ret;
> >> + goto err_dma_setup;
> >>
> >> /* FMC2 init routine */
> >> stm32_fmc2_init(fmc2);
> >> @@ -1997,7 +2001,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >> /* Scan to find existence of the device */
> >> ret = nand_scan(chip, nand->ncs);
> >> if (ret)
> >> - goto err_scan;
> >> + goto err_dma_setup;
> >>
> >> ret = mtd_device_register(mtd, NULL, 0);
> >> if (ret)
> >> @@ -2010,7 +2014,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >> err_device_register:
> >> nand_cleanup(chip);
> >>
> >> -err_scan:
> >> +err_dma_setup:
> >> if (fmc2->dma_ecc_ch)
> >> dma_release_channel(fmc2->dma_ecc_ch);
> >> if (fmc2->dma_tx_ch)
> >> @@ -2021,6 +2025,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >> sg_free_table(&fmc2->dma_data_sg);
> >> sg_free_table(&fmc2->dma_ecc_sg);
> >>
> >> +err_clk_disable:
> >> clk_disable_unprepare(fmc2->clk);
> >>
> >> return ret;
> >
> > I didn't spot it during my earlier reviews but I really prefer using
> > labels explaining what you do than having the same name of the function
> > which failed. This way you don't have to rework the error path when
> > you handle an additional error.
> >
> > So, would you mind doing this in two steps:
> >
> > 1/
> > Replace
> >
> > err_scan:
> >
> > with, eg.
> >
> > release_dma_objs:
>
> The ^err_ prefix in failpath labels is useful, since it's easily
> possible to match on it with regexes ; not so much on arbitrary label name.
I guess so, but is it actually useful to catch labels in a regex? (real
question)
Any way I suppose catching ":\n" is already a good approximation to
find labels?
>
> btw would it make sense to split the first three patches of this series
> into a separate series ? This rawnand part seems more like an unrelated
> cleanup.
As it seems that the MFD discussion can take longer, then I would say
yes, at least for the cleanup/misc changes part.
Thanks,
Miquèl
