Re: [PATCH] clk: add userspace clock consumer

[Michael Turquette <mturquette@baylibre.com>]

Quoting Akinobu Mita (2016-02-18 06:09:46)
> 2016-02-18 6:24 GMT+09:00 Michael Turquette <mturquette@baylibre.com>:
> > Quoting Akinobu Mita (2016-02-17 05:18:41)
> >> 2016-02-16 8:02 GMT+09:00 Michael Turquette <mturquette@baylibre.com>:
> >> > Hello Akinobu Mita,
> >> >
> >> > Quoting Akinobu Mita (2016-02-15 06:40:51)
> >> >> This adds userspace consumer for common clock.
> >> >>
> >> >> This driver is inspired from Userspace regulator consumer
> >> >> (REGULATOR_USERSPACE_CONSUMER) and it is useful for test purposes a=
nd
> >> >> some classes of devices that are controlled entirely from user spac=
e.
> >> >
> >> > Thanks for submitting the patch. Your implementation looks OK, but I
> >> > generally do not like to expose clock hardware controls to userspace
> >> > (and have a NAK'd a lot of patches trying to do this in the past). It
> >> > can be quite dangerous for the system to allow userspace to control
> >> > clocks.
> >>
> >> I understand your concern.  I'll happily add the features and
> >> documentation for this module to avoid misuses and abuses.
> >>
> >> > Can you explain your use case more? If your main concern is testing,
> >>
> >> I can use this for my own electrical circuit.  But mainly it's
> >> useful for testing.  When I tried to add clock provider for DS3231
> >> clkout, I wrote very ad hoc kernel code for testing it.  I think
> >> other people also have been trying something similar.
> >>
> >> > should COMMON_CLK_USERSPACE_CONSUMER be hidden behind CONFIG_DEBUG_F=
S?
> >>
> >> But this driver does not use debugfs.  Instead of that, I have no
> >> problem moving this driver to lib/ and add config option to
> >> lib/Kconfig.debug.  (under "Kernel hacking" in menuconfig)
> >
> > Hiding it behind some sort of debug option sounds good to me. Taking a
> > quick look through lib/Kconfig.debug, it seems that there are no other
> > examples of debug options from drivers/* in there.
> >
> > I'm wondering what is the best way to do it? Just create a
> > CONFIG_COMMON_CLK_DEBUG symbol and source drivers/clk/Kconfig.debug
> > conditionally?
> =

> How about just add that to drivers/clk/Kconfig like below?
> =

> config COMMON_CLK_DEBUG
>         bool "Clock driver debugging support"
>         depends on DEBUG_KERNEL
>         ...

Looks good. If this symbol is selected then we will source
drivers/clk/Kconfig.debug.

> =

> config COMMON_CLK_USERSPACE_CONSUMER
>         tristate "Userspace clock consumer support"
>         depends on COMMON_CLK_DEBUG
>         ...

This should go into drivers/clk/Kconfig.debug. There are some clk tests
that I've been slowly working on and they will end up in the file as
well at some point in the future.

Regards,
Mike

> =

> =

> We can add COMMON_CLK_DEBUG_FS for /sys/kernel/debug/clk/ if preferred.