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

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

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.
Btw, if you know someone who can take it, could you please reference
them here?

> --
> With Best Regards,
> Andy Shevchenko
>
>