On 27/08/2024 11:33, Krzysztof Kozlowski wrote: > On 27/08/2024 09:48, Geert Uytterhoeven wrote: >> Hi Krzysztof, >> >> On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski >> <krzysztof.kozlowski@xxxxxxxxxx> wrote: >>> Obtain the device node reference with scoped/cleanup.h to reduce error >>> handling and make the code a bit simpler. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> >> >> Thanks for your patch! >> >>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c >>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c >>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >>> const struct rcar_gen4_sysc_info *info; >>> const struct of_device_id *match; >>> struct rcar_gen4_pm_domains *domains; >>> - struct device_node *np; >>> void __iomem *base; >>> unsigned int i; >>> int error; >>> >>> - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); >>> + struct device_node *np __free(device_node) = >>> + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); >> >> This breaks the declarations/blank-line/code structure, so please move >> this up. > > What do you mean "declaration structure"? That's the way how variables > with constructors are expected to be declared - within the code. Continuing thoughts, so you prefer: struct rcar_gen4_pm_domains *domains; void __iomem *base; struct device_node *np __free(device_node) = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); (assuming I will put it at the end of declarations). Are you sure this is more readable? It's really long line so it obfuscates a bit the declarations. The point of the scoped assignment is that you declare it at point of need/first use. > >> >> If you insist on keeping assignment to and validation of np together, >> the line should be split in declaration and assignment. > > No, that would be inconsistent with cleanup/constructor coding style. > Maybe this is something new, so let me bring previous discussions: > > https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@xxxxxxxxxxxxxx/ > > https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@xxxxxxxxxxxxxx/ > > https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@xxxxxxxxxxxxxx/ > > and finally it will reach documentation (although it focuses on > unwinding process to be specific - "When the unwind order ..."): > https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ > >> >>> if (!np) >>> return -ENODEV; >>> >> >>> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >>> if (error) { >>> pr_warn("Failed to add PM subdomain %s to parent %u\n", >>> area->name, area->parent); >>> - goto out_put; >>> + return error; >>> } >>> } >>> >>> error = of_genpd_add_provider_onecell(np, &domains->onecell_data); >>> >>> -out_put: >>> - of_node_put(np); >>> return error; >> >> return of_genpd_add_provider_onecell(...); > > Ack. > > Best regards, > Krzysztof > Best regards, Krzysztof
