Messages in this thread | | | Subject | Re: [PATCH v2 04/12] mtd: rawnand: stm32_fmc2: manage all errors cases at probe time | From | Christophe Kerello <> | Date | Wed, 29 Apr 2020 10:00:01 +0200 |
| |
On 4/27/20 10:10 PM, Marek Vasut wrote: > On 4/27/20 10:08 PM, Miquel Raynal wrote: >> Hi Marek, >> >> Marek Vasut <marex@denx.de> wrote on Mon, 27 Apr 2020 21:46:44 +0200: >> >>> 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: >> >> My point being, Christophe, you can use err_ as a prefix but I think >> it's better to use: >> >> err_do_this_cleanup >> >> than >> >> err_this_failed > > That's fine either way.
Hi Miquel,
I will rename the label in v3: - err_device_register => err_nand_cleanup - err_dma_setup => err_release_dma - err_clk_disable => will keep this one
Regards, Christophe Kerello.
> >>>> Any way I suppose catching ":\n" is already a good approximation to >>>> find labels? >>> >>> Not very practical with git grep (^err.*: works nicely though) >> >> I suppose ^.*:$ would work the same ;) > > Try and see how much other irrelevant stuff that sucks in ;-) >
| |