[PATCH] clk: add userspace clock consumer

[PATCH] clk: add userspace clock consumer

[Akinobu Mita]

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 and
some classes of devices that are controlled entirely from user space.

Example binding and usages:

clksqw_userspace_consumer {
	compatible = "linux,clk-userspace-consumer";
	clocks = <&clksqw>;
};

	# cd /sys/devices/platform/clksqw_userspace_consumer
	# echo 8192 > rate
	# echo enabled > state

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
---
 .../bindings/clock/clk-userspace-consumer.txt      |  17 ++
 drivers/clk/Kconfig                                |   9 ++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-userspace-consumer.c               | 180 +++++++++++++++++++++
 4 files changed, 207 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt
 create mode 100644 drivers/clk/clk-userspace-consumer.c

diff --git a/Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt b/Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt
new file mode 100644
index 0000000..34b1bd1
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt
@@ -0,0 +1,17 @@
+* Userspace consumer for common clock
+
+Required properties:
+- compatible: Should be "linux,clk-userspace-consumer"
+- clocks: clock phandle to control from userspace
+
+Examples:
+
+clk32k_userspace_consumer {
+	compatible = "linux,clk-userspace-consumer";
+	clocks = <&clk32k>;
+};
+
+sqw_userspace_consumer {
+	compatible = "linux,clk-userspace-consumer";
+	clocks = <&sqw>;
+};
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index eca8e01..c8dc228 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -200,6 +200,15 @@ config COMMON_CLK_CDCE706
 	---help---
 	  This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
 
+config COMMON_CLK_USERSPACE_CONSUMER
+	tristate "Userspace clock consumer support"
+	help
+	  There are some classes of devices that are controlled entirely
+	  from user space. Userspace consumer driver provides ability to
+	  control clock for such devices.
+
+	  If unsure, say no.
+
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
 source "drivers/clk/qcom/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index b038e36..f3b51f1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-gpio.o
 ifeq ($(CONFIG_OF), y)
 obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
 endif
+obj-$(CONFIG_COMMON_CLK_USERSPACE_CONSUMER)	+= clk-userspace-consumer.o
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
diff --git a/drivers/clk/clk-userspace-consumer.c b/drivers/clk/clk-userspace-consumer.c
new file mode 100644
index 0000000..d7f36c1
--- /dev/null
+++ b/drivers/clk/clk-userspace-consumer.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright (c) 2016 Akinobu Mita <akinobu.mita@gmail.com>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * Inspired from reg-userspace-consumer
+ */
+
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+
+struct clk_userspace_consumer {
+	struct mutex lock;
+	bool enabled;
+	struct clk *clk;
+};
+
+static ssize_t clk_show_state(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct clk_userspace_consumer *consumer = dev_get_drvdata(dev);
+
+	if (consumer->enabled)
+		return sprintf(buf, "enabled\n");
+
+	return sprintf(buf, "disabled\n");
+}
+
+static ssize_t clk_store_state(struct device *dev,
+			struct device_attribute *attr, const char *buf,
+			size_t count)
+{
+	struct clk_userspace_consumer *consumer = dev_get_drvdata(dev);
+	bool enabled;
+
+	/*
+	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
+	 * will be shared with show_state(), above.
+	 */
+	if (sysfs_streq(buf, "enabled\n") || sysfs_streq(buf, "1"))
+		enabled = true;
+	else if (sysfs_streq(buf, "disabled\n") || sysfs_streq(buf, "0"))
+		enabled = false;
+	else {
+		dev_err(dev, "Configuring invalid mode\n");
+		return count;
+	}
+
+	mutex_lock(&consumer->lock);
+
+	if (enabled != consumer->enabled) {
+		int ret = 0;
+
+		if (enabled) {
+			ret = clk_prepare_enable(consumer->clk);
+			if (ret) {
+				dev_err(dev, "Failed to configure state: %d\n",
+					ret);
+			}
+		} else {
+			clk_disable_unprepare(consumer->clk);
+		}
+
+		if (!ret)
+			consumer->enabled = enabled;
+	}
+
+	mutex_unlock(&consumer->lock);
+
+	return count;
+}
+static DEVICE_ATTR(state, 0644, clk_show_state, clk_store_state);
+
+static ssize_t clk_show_rate(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct clk_userspace_consumer *consumer = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%ld\n", clk_get_rate(consumer->clk));
+}
+
+static ssize_t clk_store_rate(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct clk_userspace_consumer *consumer = dev_get_drvdata(dev);
+	unsigned long rate;
+	int err;
+
+	err = kstrtoul(buf, 0, &rate);
+	if (err)
+		return err;
+
+	err = clk_set_rate(consumer->clk, rate);
+	if (err)
+		return err;
+
+	return count;
+}
+static DEVICE_ATTR(rate, 0644, clk_show_rate, clk_store_rate);
+
+static struct attribute *attributes[] = {
+	&dev_attr_state.attr,
+	&dev_attr_rate.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs	= attributes,
+};
+
+static int clk_userspace_consumer_probe(struct platform_device *pdev)
+{
+	struct clk_userspace_consumer *consumer;
+	int ret;
+
+	consumer = devm_kzalloc(&pdev->dev, sizeof(*consumer), GFP_KERNEL);
+	if (!consumer)
+		return -ENOMEM;
+
+	mutex_init(&consumer->lock);
+
+	consumer->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(consumer->clk)) {
+		ret = PTR_ERR(consumer->clk);
+		dev_err(&pdev->dev, "Failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, consumer);
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int clk_userspace_consumer_remove(struct platform_device *pdev)
+{
+	struct clk_userspace_consumer *consumer = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+
+	mutex_lock(&consumer->lock);
+	if (consumer->enabled)
+		clk_disable_unprepare(consumer->clk);
+	mutex_unlock(&consumer->lock);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+
+static const struct of_device_id userspace_consumer_id[] = {
+	{ .compatible = "linux,clk-userspace-consumer" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, userspace_consumer_id);
+
+#endif
+
+static struct platform_driver clk_userspace_consumer_driver = {
+	.probe = clk_userspace_consumer_probe,
+	.remove = clk_userspace_consumer_remove,
+	.driver = {
+		.name = "clk-userspace-consumer",
+		.of_match_table = of_match_ptr(userspace_consumer_id),
+	},
+};
+module_platform_driver(clk_userspace_consumer_driver);
+
+MODULE_AUTHOR("Akinobu Mita <akinobu.mita@gmail.com>");
+MODULE_DESCRIPTION("Userspace consumer for common clock");
+MODULE_LICENSE("GPL");
-- 
2.5.0

Re: [PATCH] clk: add userspace clock consumer

[Michael Turquette]

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 and
> some classes of devices that are controlled entirely from user space.

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.

Can you explain your use case more? If your main concern is testing,
should COMMON_CLK_USERSPACE_CONSUMER be hidden behind CONFIG_DEBUG_FS?

Have you looked at the per-provider DEBUGFS hooks? See commit
fb2b3c9f6857, "clk: define and export clk_debugs_add_file".

Regards,
Mike

> =

> Example binding and usages:
> =

> clksqw_userspace_consumer {
>         compatible =3D "linux,clk-userspace-consumer";
>         clocks =3D <&clksqw>;
> };
> =

>         # cd /sys/devices/platform/clksqw_userspace_consumer
>         # echo 8192 > rate
>         # echo enabled > state
> =

> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  .../bindings/clock/clk-userspace-consumer.txt      |  17 ++
>  drivers/clk/Kconfig                                |   9 ++
>  drivers/clk/Makefile                               |   1 +
>  drivers/clk/clk-userspace-consumer.c               | 180 +++++++++++++++=
++++++
>  4 files changed, 207 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/clk-userspace=
-consumer.txt
>  create mode 100644 drivers/clk/clk-userspace-consumer.c
> =

> diff --git a/Documentation/devicetree/bindings/clock/clk-userspace-consum=
er.txt b/Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt
> new file mode 100644
> index 0000000..34b1bd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clk-userspace-consumer.txt
> @@ -0,0 +1,17 @@
> +* Userspace consumer for common clock
> +
> +Required properties:
> +- compatible: Should be "linux,clk-userspace-consumer"
> +- clocks: clock phandle to control from userspace
> +
> +Examples:
> +
> +clk32k_userspace_consumer {
> +       compatible =3D "linux,clk-userspace-consumer";
> +       clocks =3D <&clk32k>;
> +};
> +
> +sqw_userspace_consumer {
> +       compatible =3D "linux,clk-userspace-consumer";
> +       clocks =3D <&sqw>;
> +};
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index eca8e01..c8dc228 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -200,6 +200,15 @@ config COMMON_CLK_CDCE706
>         ---help---
>           This driver supports TI CDCE706 programmable 3-PLL clock synthe=
sizer.
>  =

> +config COMMON_CLK_USERSPACE_CONSUMER
> +       tristate "Userspace clock consumer support"
> +       help
> +         There are some classes of devices that are controlled entirely
> +         from user space. Userspace consumer driver provides ability to
> +         control clock for such devices.
> +
> +         If unsure, say no.
> +
>  source "drivers/clk/bcm/Kconfig"
>  source "drivers/clk/hisilicon/Kconfig"
>  source "drivers/clk/qcom/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index b038e36..f3b51f1 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_COMMON_CLK)      +=3D clk-gpio.o
>  ifeq ($(CONFIG_OF), y)
>  obj-$(CONFIG_COMMON_CLK)       +=3D clk-conf.o
>  endif
> +obj-$(CONFIG_COMMON_CLK_USERSPACE_CONSUMER)    +=3D clk-userspace-consum=
er.o
>  =

>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file/directory pa=
th name
> diff --git a/drivers/clk/clk-userspace-consumer.c b/drivers/clk/clk-users=
pace-consumer.c
> new file mode 100644
> index 0000000..d7f36c1
> --- /dev/null
> +++ b/drivers/clk/clk-userspace-consumer.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (c) 2016 Akinobu Mita <akinobu.mita@gmail.com>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * Inspired from reg-userspace-consumer
> + */
> +
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +
> +struct clk_userspace_consumer {
> +       struct mutex lock;
> +       bool enabled;
> +       struct clk *clk;
> +};
> +
> +static ssize_t clk_show_state(struct device *dev, struct device_attribut=
e *attr,
> +                       char *buf)
> +{
> +       struct clk_userspace_consumer *consumer =3D dev_get_drvdata(dev);
> +
> +       if (consumer->enabled)
> +               return sprintf(buf, "enabled\n");
> +
> +       return sprintf(buf, "disabled\n");
> +}
> +
> +static ssize_t clk_store_state(struct device *dev,
> +                       struct device_attribute *attr, const char *buf,
> +                       size_t count)
> +{
> +       struct clk_userspace_consumer *consumer =3D dev_get_drvdata(dev);
> +       bool enabled;
> +
> +       /*
> +        * sysfs_streq() doesn't need the \n's, but we add them so the st=
rings
> +        * will be shared with show_state(), above.
> +        */
> +       if (sysfs_streq(buf, "enabled\n") || sysfs_streq(buf, "1"))
> +               enabled =3D true;
> +       else if (sysfs_streq(buf, "disabled\n") || sysfs_streq(buf, "0"))
> +               enabled =3D false;
> +       else {
> +               dev_err(dev, "Configuring invalid mode\n");
> +               return count;
> +       }
> +
> +       mutex_lock(&consumer->lock);
> +
> +       if (enabled !=3D consumer->enabled) {
> +               int ret =3D 0;
> +
> +               if (enabled) {
> +                       ret =3D clk_prepare_enable(consumer->clk);
> +                       if (ret) {
> +                               dev_err(dev, "Failed to configure state: =
%d\n",
> +                                       ret);
> +                       }
> +               } else {
> +                       clk_disable_unprepare(consumer->clk);
> +               }
> +
> +               if (!ret)
> +                       consumer->enabled =3D enabled;
> +       }
> +
> +       mutex_unlock(&consumer->lock);
> +
> +       return count;
> +}
> +static DEVICE_ATTR(state, 0644, clk_show_state, clk_store_state);
> +
> +static ssize_t clk_show_rate(struct device *dev, struct device_attribute=
 *attr,
> +                       char *buf)
> +{
> +       struct clk_userspace_consumer *consumer =3D dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%ld\n", clk_get_rate(consumer->clk));
> +}
> +
> +static ssize_t clk_store_rate(struct device *dev, struct device_attribut=
e *attr,
> +                       const char *buf, size_t count)
> +{
> +       struct clk_userspace_consumer *consumer =3D dev_get_drvdata(dev);
> +       unsigned long rate;
> +       int err;
> +
> +       err =3D kstrtoul(buf, 0, &rate);
> +       if (err)
> +               return err;
> +
> +       err =3D clk_set_rate(consumer->clk, rate);
> +       if (err)
> +               return err;
> +
> +       return count;
> +}
> +static DEVICE_ATTR(rate, 0644, clk_show_rate, clk_store_rate);
> +
> +static struct attribute *attributes[] =3D {
> +       &dev_attr_state.attr,
> +       &dev_attr_rate.attr,
> +       NULL,
> +};
> +
> +static const struct attribute_group attr_group =3D {
> +       .attrs  =3D attributes,
> +};
> +
> +static int clk_userspace_consumer_probe(struct platform_device *pdev)
> +{
> +       struct clk_userspace_consumer *consumer;
> +       int ret;
> +
> +       consumer =3D devm_kzalloc(&pdev->dev, sizeof(*consumer), GFP_KERN=
EL);
> +       if (!consumer)
> +               return -ENOMEM;
> +
> +       mutex_init(&consumer->lock);
> +
> +       consumer->clk =3D devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(consumer->clk)) {
> +               ret =3D PTR_ERR(consumer->clk);
> +               dev_err(&pdev->dev, "Failed to get clock: %d\n", ret);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, consumer);
> +
> +       ret =3D sysfs_create_group(&pdev->dev.kobj, &attr_group);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int clk_userspace_consumer_remove(struct platform_device *pdev)
> +{
> +       struct clk_userspace_consumer *consumer =3D platform_get_drvdata(=
pdev);
> +
> +       sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +
> +       mutex_lock(&consumer->lock);
> +       if (consumer->enabled)
> +               clk_disable_unprepare(consumer->clk);
> +       mutex_unlock(&consumer->lock);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +
> +static const struct of_device_id userspace_consumer_id[] =3D {
> +       { .compatible =3D "linux,clk-userspace-consumer" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, userspace_consumer_id);
> +
> +#endif
> +
> +static struct platform_driver clk_userspace_consumer_driver =3D {
> +       .probe =3D clk_userspace_consumer_probe,
> +       .remove =3D clk_userspace_consumer_remove,
> +       .driver =3D {
> +               .name =3D "clk-userspace-consumer",
> +               .of_match_table =3D of_match_ptr(userspace_consumer_id),
> +       },
> +};
> +module_platform_driver(clk_userspace_consumer_driver);
> +
> +MODULE_AUTHOR("Akinobu Mita <akinobu.mita@gmail.com>");
> +MODULE_DESCRIPTION("Userspace consumer for common clock");
> +MODULE_LICENSE("GPL");
> -- =

> 2.5.0
>=20

Re: [PATCH] clk: add userspace clock consumer

[Akinobu Mita]

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 and
>> some classes of devices that are controlled entirely from user space.
>
> 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_FS?

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)

> Have you looked at the per-provider DEBUGFS hooks? See commit
> fb2b3c9f6857, "clk: define and export clk_debugs_add_file".

I think it is useful for providing device specific debug features.
But this userspace consumer only provides generic feature for most
clock provider and allows controlling the devices specified in
the device tree.

Re: [PATCH] clk: add userspace clock consumer

[Michael Turquette]

Quoting Akinobu Mita (2016-02-15 06:40:51)
> +static ssize_t clk_show_state(struct device *dev, struct device_attribut=
e *attr,
> +                       char *buf)
> +{
> +       struct clk_userspace_consumer *consumer =3D dev_get_drvdata(dev);
> +
> +       if (consumer->enabled)
> +               return sprintf(buf, "enabled\n");
> +
> +       return sprintf(buf, "disabled\n");
> +}
> +
> +static ssize_t clk_store_state(struct device *dev,

s/state/enable/

...
> +static const struct of_device_id userspace_consumer_id[] =3D {
> +       { .compatible =3D "linux,clk-userspace-consumer" },

Let's use "clock" instead of "clk" for the compat string, this is how
the other bindings do it. We tend to use "clk" when talking specifically
about Linux.

Regards,
Mike

Re: [PATCH] clk: add userspace clock consumer

[Michael Turquette]

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 and
> >> some classes of devices that are controlled entirely from user space.
> >
> > 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_FS?
> =

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

Regards,
Mike

> =

> > Have you looked at the per-provider DEBUGFS hooks? See commit
> > fb2b3c9f6857, "clk: define and export clk_debugs_add_file".
> =

> I think it is useful for providing device specific debug features.
> But this userspace consumer only provides generic feature for most
> clock provider and allows controlling the devices specified in
> the device tree.

Re: [PATCH] clk: add userspace clock consumer

[Akinobu Mita]

2016-02-18 6:14 GMT+09:00 Michael Turquette <mturquette@baylibre.com>:
> Quoting Akinobu Mita (2016-02-15 06:40:51)
>> +static ssize_t clk_show_state(struct device *dev, struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +       struct clk_userspace_consumer *consumer = dev_get_drvdata(dev);
>> +
>> +       if (consumer->enabled)
>> +               return sprintf(buf, "enabled\n");
>> +
>> +       return sprintf(buf, "disabled\n");
>> +}
>> +
>> +static ssize_t clk_store_state(struct device *dev,
>
> s/state/enable/

OK.  I'll rename this sysfs file name from 'state' to 'enable' and it
prints '0' or '1' instead of 'disabled' or 'enabled'.

>> +static const struct of_device_id userspace_consumer_id[] = {
>> +       { .compatible = "linux,clk-userspace-consumer" },
>
> Let's use "clock" instead of "clk" for the compat string, this is how
> the other bindings do it. We tend to use "clk" when talking specifically
> about Linux.

OK.

Re: [PATCH] clk: add userspace clock consumer

[Akinobu Mita]

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 and
>> >> some classes of devices that are controlled entirely from user space.
>> >
>> > 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_FS?
>>
>> 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
        ...

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


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

Re: [PATCH] clk: add userspace clock consumer

[Michael Turquette]

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.