Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Wed, 13 Oct 2021 15:08:37 +0200 | Subject | Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node |
| |
Hi Sam,
On Wed, Oct 13, 2021 at 1:36 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 07, 2021 at 09:21:58PM +0300, Sam Protsenko wrote: > > > Useful for testing mux clocks. One can write the index of the parent to > > > be set into clk_parent node, starting from 0. Example > > > > > > # cd /sys/kernel/debug/clk/mout_peri_bus > > > # cat clk_possible_parents > > > dout_shared0_div4 dout_shared1_div4 > > > # cat clk_parent > > > dout_shared0_div4 > > > # echo 1 > clk_parent > > > # cat clk_parent > > > dout_shared1_div4 > > > > > > CLOCK_ALLOW_WRITE_DEBUGFS has to be defined in drivers/clk/clk.c in > > > order to use this feature. > > > > ... > > > > > +#ifdef CLOCK_ALLOW_WRITE_DEBUGFS > > > + if (core->num_parents > 1) > > > + debugfs_create_file("clk_parent", 0644, root, core, > > > + ¤t_parent_rw_fops); > > > + else > > > +#endif > > > > > + { > > > + if (core->num_parents > 0) > > > + debugfs_create_file("clk_parent", 0444, root, core, > > > + ¤t_parent_fops); > > > + } > > > > Currently there is no need to add the {} along with increased indentation > > level. I.o.w. the 'else if' is valid in C. > > Without those {} we have two bad options: > > 1. When putting subsequent 'if' block on the same indentation level > as 'else': looks ok-ish for my taste (though inconsistent with #ifdef > code) and checkpatch swears: > > WARNING: suspect code indent for conditional statements (8, 8) > #82: FILE: drivers/clk/clk.c:3334: > + else > [...] > if (core->num_parents > 0) > > 2. When adding 1 additional indentation level for subsequent 'if' > block: looks plain ugly to me, inconsistent for the case when > CLOCK_ALLOW_WRITE_DEBUGFS is not defined, but checkpatch is happy > > I still think that the way I did that (with curly braces) is better > one: it's consistent for all cases, looking ok, checkpatch is happy > too. But isn't it hairsplitting? This particular case is not described > in kernel coding style doc, so it's about personal preferences. > > If it's still important to you -- please provide exact code snippet > here (with indentations) for what you desire, I'll send v6. But > frankly I'd rather spend my time on something more useful. This is > minor patch, and I don't see any maintainers wishing to pull it yet.
Note that checkpatch is just a tool, providing advice. It is not perfect, and if there is a good reason to ignore it, I'm all for that.
Personally, I would write:
#ifdef CLOCK_ALLOW_WRITE_DEBUGFS if (core->num_parents > 1) debugfs_create_file("clk_parent", 0644, root, core, ¤t_parent_rw_fops); else #endif if (core->num_parents > 0) debugfs_create_file("clk_parent", 0444, root, core, ¤t_parent_fops); }
Then, I'm wondering if it really is worth it to have separate cases for "num_parents> 1" and "num_parents > 0". If there's a single parent, current_parent_write() should still work fine with "0", wouldn't it? Then the only differences are the file mode and the fops. You could handle that with #defines above, like is currently done for clk_rate_mode. And the checkpatch issue is gone ;-)
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
| |