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