On 4/27/20 8:08 PM, Miquel Raynal wrote: [...] >>>> /* 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) I find it useful to have a unified way to find those labels, e.g. err_because_foo: err_because_bar: err_last_one: is much nicer than: foo_failed: bar_also_failed: its_total_randomness: > Any way I suppose catching ":\n" is already a good approximation to > find labels? Not very practical with git grep (^err.*: works nicely though) >> 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. Right -- Best regards, Marek Vasut
