lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 04/12] mtd: rawnand: stm32_fmc2: manage all errors cases at probe time
    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

    >
    > > 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 ;)

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

    Cheers,
    Miquèl

    \
     
     \ /
      Last update: 2020-04-27 22:08    [W:4.509 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site