Hi Krzysztof, On Tue, Aug 27, 2024 at 11:39 AM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > On 27/08/2024 11:33, Krzysztof Kozlowski wrote: > > On 27/08/2024 09:48, Geert Uytterhoeven wrote: > >> 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 First a block with declarations, then a blank line, followed by the actual code (yeah, the pre-C99 style ;-) > > with constructors are expected to be declared - within the code. When it matters. > 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. You're missing reverse Christmas tree order... > >> 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: [...] > > and finally it will reach documentation (although it focuses on Oh, "finally" as in not yet upstream ;-) > > unwinding process to be specific - "When the unwind order ..."): > > https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/ "When the unwind order matters..." So it's perfectly fine to have: static int __init rcar_gen4_sysc_pd_init(void) { struct device_node *np __free(device_node) = NULL; struct rcar_gen4_pm_domains *domains; const struct rcar_gen4_sysc_info *info; const struct of_device_id *match; void __iomem *base; unsigned int i; int error; np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); if (!np) return -ENODEV; ... } But my first suggestion: static int __init rcar_gen4_sysc_pd_init(void) { struct device_node *np __free(device_node) = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); struct rcar_gen4_pm_domains *domains; const struct rcar_gen4_sysc_info *info; const struct of_device_id *match; void __iomem *base; unsigned int i; int error; if (!np) return -ENODEV; ... } is safer w.r.t. to future modification. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
