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.
Signed-off-by: Sam Protsenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
Changes in v5:
- Used kstrtou8_from_user() API
- Got rid of code duplication
- Fixed typo in commit message
- Added R-b tag by Andy Shevchenko
Changes in v4:
- Fixed the commit title
Changes in v3:
- Removed unwanted changes added by mistake
Changes in v2:
- Merged write() function into existing 'clk_parent' file
- Removed 'if (val >= core->num_parents)' check
- Removed incorrect usage of IS_ERR_OR_NULL()
drivers/clk/clk.c | 50 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 806c55f0991b..6154d5bc2b45 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3224,6 +3224,42 @@ static int current_parent_show(struct seq_file *s, void *data)
}
DEFINE_SHOW_ATTRIBUTE(current_parent);
+#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
+static ssize_t current_parent_write(struct file *file, const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *s = file->private_data;
+ struct clk_core *core = s->private;
+ struct clk_core *parent;
+ u8 idx;
+ int err;
+
+ err = kstrtou8_from_user(ubuf, count, 0, &idx);
+ if (err < 0)
+ return err;
+
+ parent = clk_core_get_parent_by_index(core, idx);
+ if (!parent)
+ return -ENOENT;
+
+ clk_prepare_lock();
+ err = clk_core_set_parent_nolock(core, parent);
+ clk_prepare_unlock();
+ if (err)
+ return err;
+
+ return count;
+}
+
+static const struct file_operations current_parent_rw_fops = {
+ .open = current_parent_open,
+ .write = current_parent_write,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+#endif
+
static int clk_duty_cycle_show(struct seq_file *s, void *data)
{
struct clk_core *core = s->private;
@@ -3291,9 +3327,17 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
&clk_prepare_enable_fops);
#endif
- if (core->num_parents > 0)
- debugfs_create_file("clk_parent", 0444, root, core,
- ¤t_parent_fops);
+#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);
+ }
if (core->num_parents > 1)
debugfs_create_file("clk_possible_parents", 0444, root, core,
--
2.30.2
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.
--
With Best Regards,
Andy Shevchenko
On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
<[email protected]> 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.
Btw, if you know someone who can take it, could you please reference
them here?
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Thu, Oct 7, 2021 at 8:22 PM Sam Protsenko <[email protected]> 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.
>
> Signed-off-by: Sam Protsenko <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, Oct 13, 2021 at 02:35:48PM +0300, Sam Protsenko wrote:
> On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> <[email protected]> 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
Hi Sam,
On Wed, Oct 13, 2021 at 1:36 PM Sam Protsenko
<[email protected]> wrote:
> On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> <[email protected]> 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.
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);
}
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 ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, 13 Oct 2021 at 16:07, Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Oct 13, 2021 at 02:35:48PM +0300, Sam Protsenko wrote:
> > On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> > <[email protected]> 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);
> }
>
No problem, will add those {} in v6.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, 13 Oct 2021 at 16:08, Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Sam,
>
> On Wed, Oct 13, 2021 at 1:36 PM Sam Protsenko
> <[email protected]> wrote:
> > On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> > <[email protected]> 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);
> }
>
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 -- [email protected]
>
> 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
On Wed, 13 Oct 2021 at 19:30, Sam Protsenko <[email protected]> wrote:
>
> On Wed, 13 Oct 2021 at 16:08, Geert Uytterhoeven <[email protected]> wrote:
> >
> > Hi Sam,
> >
> > On Wed, Oct 13, 2021 at 1:36 PM Sam Protsenko
> > <[email protected]> wrote:
> > > On Tue, 12 Oct 2021 at 21:55, Andy Shevchenko
> > > <[email protected]> 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 -- [email protected]
> >
> > 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