Hi Boris,
On lun., mars 12 2018, Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:
> On Mon, 12 Mar 2018 17:55:26 +0100
> Gregory CLEMENT <gregory.clement@xxxxxxxxxxx> wrote:
>
>> >
>> >> struct completion complete;
>> >> unsigned long assigned_cs;
>> >> struct list_head chips;
>> >> @@ -2747,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
>> >> if (ret)
>> >> return ret;
>> >>
>> >> + nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
>> >> + if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
>> >> + if (!IS_ERR(nfc->reg_clk)) {
>> >> + ret = clk_prepare_enable(nfc->reg_clk);
>> >> + if (ret)
>> >> + goto unprepare_clk;
>> >
>> > I already suggested to move the devm_clk_get(&pdev->dev, "reg") before
>> > the clk_prepare_enable(nfc->ecc_clk) one to simplify the error path.
>> >
>>
>> Actually I started to implement your suggestion but unlike what you
>> though it made the code less simpler. Indeed by having the mandatory
>> clock first than in case of failure we can directly exit the function.
>>
>> If the reg clock was initialized first, then if the core/ecc clock fail
>> in soem case we woudl need to daisbel the reg clock and in other case we
>> could directly exit.
>
> Well, it's pretty much the same problem if you do it in the order you
> propose here: if the core clk enable fails, you'll have to disable the
So if it is the same no need to change! :)
> reg clk. Plus, I'm not a big fan of if/else block imbrications when we
> can avoid them.
Your solution to avoid if/else block is to add extra code and extra test
which do not bring anything except removing an if/else bloc.
For the record it was
nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
if (PTR_ERR(nfc->reg_clk) == -ENOENT)
nfc->reg_clk = NULL;
--> here you set to NULL whereas it is useless
if (IS_ERR(nfc->reg_clk))
return PTR_ERR(nfc->reg_clk);
--> here you test again the return value even if it was previously set
to -ENOENT
...
ret = clk_prepare_enable(nfc->reg_clk);
--> here if reg_clk was NULL due to the beginning of the block you do a
useless call to clk_prepare_enable
if (ret)
goto unprepare_ecc_clk;
Gregory
>
>>
>>
>> >> + } else {
>> >> + ret = PTR_ERR(nfc->reg_clk);
>> >> + goto unprepare_clk;
>> >> + }
>> >> + }
--
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
