Messages in this thread | | | Date | Wed, 13 Oct 2021 16:07:42 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node |
| |
On Wed, Oct 13, 2021 at 02:35:48PM +0300, Sam Protsenko 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:
...
> > > +#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.
I meant
#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);
But after looking at the present code, this variant is occurred 5x-10x times less. So, only nit-picks then (note additional {} along with no blank line):
#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); }
-- With Best Regards, Andy Shevchenko
| |