All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] clk: Add write operation for clk_parent debugfs node
@ 2021-10-07 18:21 Sam Protsenko
  2021-10-12 18:55 ` Andy Shevchenko
  2021-10-13 13:00 ` Geert Uytterhoeven
  0 siblings, 2 replies; 9+ messages in thread
From: Sam Protsenko @ 2021-10-07 18:21 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Russell King
  Cc: Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski,
	Mike Tipton, Andy Shevchenko, Andy Shevchenko,
	Geert Uytterhoeven, Fabio Estevam, linux-clk, linux-kernel

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 <semen.protsenko@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
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,
-				    &current_parent_fops);
+#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);
+	}
 
 	if (core->num_parents > 1)
 		debugfs_create_file("clk_possible_parents", 0444, root, core,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-07 18:21 [PATCH v5] clk: Add write operation for clk_parent debugfs node Sam Protsenko
@ 2021-10-12 18:55 ` Andy Shevchenko
  2021-10-13 11:35   ` Sam Protsenko
  2021-10-13 13:00 ` Geert Uytterhoeven
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-10-12 18:55 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Michael Turquette, Stephen Boyd, Russell King, Chanwoo Choi,
	Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton,
	Andy Shevchenko, Geert Uytterhoeven, Fabio Estevam, linux-clk,
	linux-kernel

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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-12 18:55 ` Andy Shevchenko
@ 2021-10-13 11:35   ` Sam Protsenko
  2021-10-13 13:07     ` Andy Shevchenko
  2021-10-13 13:08     ` Geert Uytterhoeven
  0 siblings, 2 replies; 9+ messages in thread
From: Sam Protsenko @ 2021-10-13 11:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Michael Turquette, Stephen Boyd, Russell King, Chanwoo Choi,
	Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton,
	Andy Shevchenko, Geert Uytterhoeven, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

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
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-07 18:21 [PATCH v5] clk: Add write operation for clk_parent debugfs node Sam Protsenko
  2021-10-12 18:55 ` Andy Shevchenko
@ 2021-10-13 13:00 ` Geert Uytterhoeven
  1 sibling, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2021-10-13 13:00 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Michael Turquette, Stephen Boyd, Russell King, Chanwoo Choi,
	Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton,
	Andy Shevchenko, Andy Shevchenko, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

On Thu, Oct 7, 2021 at 8:22 PM Sam Protsenko <semen.protsenko@linaro.org> 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 <semen.protsenko@linaro.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-13 11:35   ` Sam Protsenko
@ 2021-10-13 13:07     ` Andy Shevchenko
  2021-10-13 16:15       ` Sam Protsenko
  2021-10-13 13:08     ` Geert Uytterhoeven
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-10-13 13:07 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Michael Turquette, Stephen Boyd, Russell King, Chanwoo Choi,
	Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton,
	Andy Shevchenko, Geert Uytterhoeven, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

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,
> > > +                                 &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.

I meant

#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);

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


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-13 11:35   ` Sam Protsenko
  2021-10-13 13:07     ` Andy Shevchenko
@ 2021-10-13 13:08     ` Geert Uytterhoeven
  2021-10-13 16:30       ` Sam Protsenko
  1 sibling, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2021-10-13 13:08 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King,
	Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski,
	Mike Tipton, Andy Shevchenko, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

  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.

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);
            }

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 -- 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-13 13:07     ` Andy Shevchenko
@ 2021-10-13 16:15       ` Sam Protsenko
  0 siblings, 0 replies; 9+ messages in thread
From: Sam Protsenko @ 2021-10-13 16:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Michael Turquette, Stephen Boyd, Russell King, Chanwoo Choi,
	Sylwester Nawrocki, Krzysztof Kozlowski, Mike Tipton,
	Andy Shevchenko, Geert Uytterhoeven, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

On Wed, 13 Oct 2021 at 16:07, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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,
> > > > +                                 &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.
>
> I meant
>
> #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);
>
> 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,
>                                     &current_parent_rw_fops);
>         } else
> #endif
>         {
>                 if (core->num_parents > 0)
>                         debugfs_create_file("clk_parent", 0444, root, core,
>                                             &current_parent_fops);
>         }
>

No problem, will add those {} in v6.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-13 13:08     ` Geert Uytterhoeven
@ 2021-10-13 16:30       ` Sam Protsenko
  2021-10-13 17:13         ` Sam Protsenko
  0 siblings, 1 reply; 9+ messages in thread
From: Sam Protsenko @ 2021-10-13 16:30 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King,
	Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski,
	Mike Tipton, Andy Shevchenko, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

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);
>             }
>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5] clk: Add write operation for clk_parent debugfs node
  2021-10-13 16:30       ` Sam Protsenko
@ 2021-10-13 17:13         ` Sam Protsenko
  0 siblings, 0 replies; 9+ messages in thread
From: Sam Protsenko @ 2021-10-13 17:13 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King,
	Chanwoo Choi, Sylwester Nawrocki, Krzysztof Kozlowski,
	Mike Tipton, Andy Shevchenko, Fabio Estevam, linux-clk,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-10-13 17:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-07 18:21 [PATCH v5] clk: Add write operation for clk_parent debugfs node Sam Protsenko
2021-10-12 18:55 ` Andy Shevchenko
2021-10-13 11:35   ` Sam Protsenko
2021-10-13 13:07     ` Andy Shevchenko
2021-10-13 16:15       ` Sam Protsenko
2021-10-13 13:08     ` Geert Uytterhoeven
2021-10-13 16:30       ` Sam Protsenko
2021-10-13 17:13         ` Sam Protsenko
2021-10-13 13:00 ` Geert Uytterhoeven

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.