Messages in this thread | | | From | Sam Protsenko <> | Date | Wed, 13 Oct 2021 20:13:11 +0300 | Subject | Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node |
| |
On Wed, 13 Oct 2021 at 19:30, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, 13 Oct 2021 at 16:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > 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. > > > > Agreed. Actually I did the same grepping as Andy mentioned in previous > mails, and used that style because that's what other people often do. > checkpatch is more like excuse for me in this case :) > > > 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); > > } > > >
Actually... After considering all options and looking at actual diff, I'll go with that option: looks least cluttered, and the delta is really minimal.
> That looks good to me. But I'd keep it as is, if you don't have a > strong opinion about this: looks better with braces, because it's > multi-line blocks (although physically and not semantically). > > > 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 ;-) > > > > I considered such case. But it would be inconsistent with this already > existing code: > > if (core->num_parents > 1) > debugfs_create_file("clk_possible_parents", 0444, root, core, > &possible_parents_fops); > > Because user would probably want to use both 'clk_parent' and > 'clk_possible_parents' together (e.g. see my example in commit > message). From logical point of view, I designed that code for testing > MUX clocks, and I doubt there are any MUXes with only one parent > (input signal). So I'd like to keep this logic as is, if you don't > mind, even though it might appear bulky. > > So for v6 I'm going to go exactly with what Andy suggested, hope it's > fine with you? > > > 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
| |