Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node

[Sam Protsenko <semen.protsenko@linaro.org>]

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,
> > > > > +                                 &current_parent_rw_fops);
> > > > > +     else
> > > > > +#endif
> > > >
> > > > > +     {
> > > > > +             if (core->num_parents > 0)
> > > > > +                     debugfs_create_file("clk_parent", 0444, root, core,
> > > > > +                                         &current_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,
> >                                         &current_parent_rw_fops);
> >             else
> >     #endif
> >             if (core->num_parents > 0)
> >                     debugfs_create_file("clk_parent", 0444, root, core,
> >                                         &current_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