Message-ID: <CAPLW+4muEHPijg2yZJ-gu--6Sbg9DvTeSz5QcwOR+eAVniczyA@mail.gmail.com>
Date:   Wed, 13 Oct 2021 19:30:30 +0300
From:   Sam Protsenko <semen.protsenko@...aro.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Sylwester Nawrocki <s.nawrocki@...sung.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Mike Tipton <mdtipton@...eaurora.org>,
        Andy Shevchenko <andy@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        linux-clk <linux-clk@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node

On Wed, 13 Oct 2021 at 16:08, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
>   Hi Sam,
>
> On Wed, Oct 13, 2021 at 1:36 PM Sam Protsenko
> <semen.protsenko@...aro.org> wrote:
> > On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> > <andriy.shevchenko@...ux.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);
>             }
>

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@...ux-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

Powered by blists - more mailing lists