[PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[Linus Walleij]

After som grep:ing it turns out nothing in the kernel is really calling
clk_[hw_]_register_gpio_[gate|mux](). All existing instances are just
created directly from the device tree probe functions at the bottom of
the clk-gpio.c clock provider file.

This means we can change the signature of the function without any
consequences! Everyone should be using GPIO descriptors now, so let's
just go in and enforce that.

This saves a bit of code since GPIO descriptors know inherently if they
are active low so no need for the code keeping track of that.

We leave it to the caller to come up with the GPIO descriptor. It is
nowadays possible to do that even without a corresponding device, so
no excuse not to pass them around. The one in-kernel user lifecycles
it using devm_gpiod_get() in gpio_clk_driver_probe().

Cc: Sergej Sawazki <ce3a@gmx.de>
Cc: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I don't have GPIO clocks in my setups so this will obviously
need some testing.

I guess the public APIs in clk-provider.h could actually be
removed, but I'm not certain if that is proper. Maybe there
are plans for this driver to be used by other providers than the
device tree.
---
 drivers/clk/clk-gpio.c       | 90 +++++++++++++++++---------------------------
 include/linux/clk-provider.h | 12 +++---
 2 files changed, 41 insertions(+), 61 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 86b245746a6b..9d057073e110 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -15,9 +15,7 @@
 #include <linux/clk-provider.h>
 #include <linux/export.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
-#include <linux/of_gpio.h>
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
@@ -95,14 +93,12 @@ const struct clk_ops clk_gpio_mux_ops = {
 EXPORT_SYMBOL_GPL(clk_gpio_mux_ops);
 
 static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
-		const char * const *parent_names, u8 num_parents, unsigned gpio,
-		bool active_low, unsigned long flags,
-		const struct clk_ops *clk_gpio_ops)
+		const char * const *parent_names, u8 num_parents, struct gpio_desc *gpiod,
+		unsigned long flags, const struct clk_ops *clk_gpio_ops)
 {
 	struct clk_gpio *clk_gpio;
 	struct clk_hw *hw;
 	struct clk_init_data init = {};
-	unsigned long gpio_flags;
 	int err;
 
 	if (dev)
@@ -113,24 +109,13 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
 	if (!clk_gpio)
 		return ERR_PTR(-ENOMEM);
 
-	if (active_low)
-		gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_HIGH;
-	else
-		gpio_flags = GPIOF_OUT_INIT_LOW;
-
-	if (dev)
-		err = devm_gpio_request_one(dev, gpio, gpio_flags, name);
-	else
-		err = gpio_request_one(gpio, gpio_flags, name);
-	if (err) {
-		if (err != -EPROBE_DEFER)
-			pr_err("%s: %s: Error requesting clock control gpio %u\n",
-					__func__, name, gpio);
-		if (!dev)
-			kfree(clk_gpio);
-
-		return ERR_PTR(err);
-	}
+	/*
+	 * Set to disabled no matter what: NOTE if the GPIO line is active low
+	 * the GPIO descriptor knows this and will set it high to deassert the
+	 * line. This assumes the GPIO descriptor has been requested using
+	 * GPIOD_ASIS by the callers so we need to initialize it as disabled here.
+	 */
+	gpiod_set_value(gpiod, 0);
 
 	init.name = name;
 	init.ops = clk_gpio_ops;
@@ -138,7 +123,7 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
 	init.parent_names = parent_names;
 	init.num_parents = num_parents;
 
-	clk_gpio->gpiod = gpio_to_desc(gpio);
+	clk_gpio->gpiod = gpiod;
 	clk_gpio->hw.init = &init;
 
 	hw = &clk_gpio->hw;
@@ -151,7 +136,6 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
 		return hw;
 
 	if (!dev) {
-		gpiod_put(clk_gpio->gpiod);
 		kfree(clk_gpio);
 	}
 
@@ -164,29 +148,27 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
  * @dev: device that is registering this clock
  * @name: name of this clock
  * @parent_name: name of this clock's parent
- * @gpio: gpio number to gate this clock
- * @active_low: true if gpio should be set to 0 to enable clock
+ * @gpiod: gpio descriptor to gate this clock
  * @flags: clock flags
  */
 struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, const char *name,
-		const char *parent_name, unsigned gpio, bool active_low,
+		const char *parent_name, struct gpio_desc *gpiod,
 		unsigned long flags)
 {
 	return clk_register_gpio(dev, name,
 			(parent_name ? &parent_name : NULL),
-			(parent_name ? 1 : 0), gpio, active_low, flags,
+			(parent_name ? 1 : 0), gpiod, flags,
 			&clk_gpio_gate_ops);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_gpio_gate);
 
 struct clk *clk_register_gpio_gate(struct device *dev, const char *name,
-		const char *parent_name, unsigned gpio, bool active_low,
+		const char *parent_name, struct gpio_desc *gpiod,
 		unsigned long flags)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_gpio_gate(dev, name, parent_name, gpio, active_low,
-				       flags);
+	hw = clk_hw_register_gpio_gate(dev, name, parent_name, gpiod, flags);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;
@@ -199,13 +181,12 @@ EXPORT_SYMBOL_GPL(clk_register_gpio_gate);
  * @name: name of this clock
  * @parent_names: names of this clock's parents
  * @num_parents: number of parents listed in @parent_names
- * @gpio: gpio number to gate this clock
- * @active_low: true if gpio should be set to 0 to enable clock
+ * @gpiod: gpio descriptor to gate this clock
  * @flags: clock flags
  */
 struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
-		const char * const *parent_names, u8 num_parents, unsigned gpio,
-		bool active_low, unsigned long flags)
+		const char * const *parent_names, u8 num_parents, struct gpio_desc *gpiod,
+		unsigned long flags)
 {
 	if (num_parents != 2) {
 		pr_err("mux-clock %s must have 2 parents\n", name);
@@ -213,18 +194,18 @@ struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
 	}
 
 	return clk_register_gpio(dev, name, parent_names, num_parents,
-			gpio, active_low, flags, &clk_gpio_mux_ops);
+			gpiod, flags, &clk_gpio_mux_ops);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_gpio_mux);
 
 struct clk *clk_register_gpio_mux(struct device *dev, const char *name,
-		const char * const *parent_names, u8 num_parents, unsigned gpio,
-		bool active_low, unsigned long flags)
+		const char * const *parent_names, u8 num_parents, struct gpio_desc *gpiod,
+		unsigned long flags)
 {
 	struct clk_hw *hw;
 
 	hw = clk_hw_register_gpio_mux(dev, name, parent_names, num_parents,
-			gpio, active_low, flags);
+			gpiod, flags);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;
@@ -236,10 +217,10 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	struct device_node *node = pdev->dev.of_node;
 	const char **parent_names, *gpio_name;
 	unsigned int num_parents;
-	int gpio;
-	enum of_gpio_flags of_flags;
+	struct gpio_desc *gpiod;
 	struct clk *clk;
-	bool active_low, is_mux;
+	bool is_mux;
+	int ret;
 
 	num_parents = of_clk_get_parent_count(node);
 	if (num_parents) {
@@ -255,28 +236,27 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 
 	is_mux = of_device_is_compatible(node, "gpio-mux-clock");
 
-	gpio_name = is_mux ? "select-gpios" : "enable-gpios";
-	gpio = of_get_named_gpio_flags(node, gpio_name, 0, &of_flags);
-	if (gpio < 0) {
-		if (gpio == -EPROBE_DEFER)
+	gpio_name = is_mux ? "select" : "enable";
+	gpiod = devm_gpiod_get(&pdev->dev, gpio_name, GPIOD_ASIS);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		if (ret == -EPROBE_DEFER)
 			pr_debug("%s: %s: GPIOs not yet available, retry later\n",
 					node->name, __func__);
 		else
-			pr_err("%s: %s: Can't get '%s' DT property\n",
+			pr_err("%s: %s: Can't get '%s' named GPIO property\n",
 					node->name, __func__,
 					gpio_name);
-		return gpio;
+		return ret;
 	}
 
-	active_low = of_flags & OF_GPIO_ACTIVE_LOW;
-
 	if (is_mux)
 		clk = clk_register_gpio_mux(&pdev->dev, node->name,
-				parent_names, num_parents, gpio, active_low, 0);
+				parent_names, num_parents, gpiod, 0);
 	else
 		clk = clk_register_gpio_gate(&pdev->dev, node->name,
-				parent_names ?  parent_names[0] : NULL, gpio,
-				active_low, 0);
+				parent_names ?  parent_names[0] : NULL, gpiod,
+				0);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5100ec1b5d55..063d8cb9926f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -682,10 +682,10 @@ struct clk_gpio {
 
 extern const struct clk_ops clk_gpio_gate_ops;
 struct clk *clk_register_gpio_gate(struct device *dev, const char *name,
-		const char *parent_name, unsigned gpio, bool active_low,
+		const char *parent_name, struct gpio_desc *gpiod,
 		unsigned long flags);
 struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, const char *name,
-		const char *parent_name, unsigned gpio, bool active_low,
+		const char *parent_name, struct gpio_desc *gpiod,
 		unsigned long flags);
 void clk_hw_unregister_gpio_gate(struct clk_hw *hw);
 
@@ -701,11 +701,11 @@ void clk_hw_unregister_gpio_gate(struct clk_hw *hw);
 
 extern const struct clk_ops clk_gpio_mux_ops;
 struct clk *clk_register_gpio_mux(struct device *dev, const char *name,
-		const char * const *parent_names, u8 num_parents, unsigned gpio,
-		bool active_low, unsigned long flags);
+		const char * const *parent_names, u8 num_parents, struct gpio_desc *gpiod,
+		unsigned long flags);
 struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
-		const char * const *parent_names, u8 num_parents, unsigned gpio,
-		bool active_low, unsigned long flags);
+		const char * const *parent_names, u8 num_parents, struct gpio_desc *gpiod,
+		unsigned long flags);
 void clk_hw_unregister_gpio_mux(struct clk_hw *hw);
 
 /**
-- 
2.13.5

[PATCH 2/2] clk: clk-gpio: Request GPIO descriptor as LOW

[Linus Walleij]

Requesting the GPIOD_OUT_LOW low will make sure the GPIO is
deasserted when requested. The gpiolib core will make sure that
if the GPIO line is active low, it will be logically driven high
when deasserted, see drivers/gpiolib.c gpiod_configure_flags().

Cc: Sergej Sawazki <ce3a@gmx.de>
Cc: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/clk/clk-gpio.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 9d057073e110..151513c655c3 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -109,14 +109,6 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
 	if (!clk_gpio)
 		return ERR_PTR(-ENOMEM);
 
-	/*
-	 * Set to disabled no matter what: NOTE if the GPIO line is active low
-	 * the GPIO descriptor knows this and will set it high to deassert the
-	 * line. This assumes the GPIO descriptor has been requested using
-	 * GPIOD_ASIS by the callers so we need to initialize it as disabled here.
-	 */
-	gpiod_set_value(gpiod, 0);
-
 	init.name = name;
 	init.ops = clk_gpio_ops;
 	init.flags = flags | CLK_IS_BASIC;
@@ -237,7 +229,7 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	is_mux = of_device_is_compatible(node, "gpio-mux-clock");
 
 	gpio_name = is_mux ? "select" : "enable";
-	gpiod = devm_gpiod_get(&pdev->dev, gpio_name, GPIOD_ASIS);
+	gpiod = devm_gpiod_get(&pdev->dev, gpio_name, GPIOD_OUT_LOW);
 	if (IS_ERR(gpiod)) {
 		ret = PTR_ERR(gpiod);
 		if (ret == -EPROBE_DEFER)
-- 
2.13.5

Re: [PATCH 2/2] clk: clk-gpio: Request GPIO descriptor as LOW

[Stephen Boyd]

On 09/24, Linus Walleij wrote:
> Requesting the GPIOD_OUT_LOW low will make sure the GPIO is
> deasserted when requested. The gpiolib core will make sure that
> if the GPIO line is active low, it will be logically driven high
> when deasserted, see drivers/gpiolib.c gpiod_configure_flags().
> 
> Cc: Sergej Sawazki <ce3a@gmx.de>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/clk/clk-gpio.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 9d057073e110..151513c655c3 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -109,14 +109,6 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
>  	if (!clk_gpio)
>  		return ERR_PTR(-ENOMEM);
>  
> -	/*
> -	 * Set to disabled no matter what: NOTE if the GPIO line is active low
> -	 * the GPIO descriptor knows this and will set it high to deassert the
> -	 * line. This assumes the GPIO descriptor has been requested using
> -	 * GPIOD_ASIS by the callers so we need to initialize it as disabled here.
> -	 */
> -	gpiod_set_value(gpiod, 0);
> -

Is there a reason why we split this patch out and then deleted
the comment that was introduced in the previous patch? Why not
combine these two patches into one?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] clk: clk-gpio: Request GPIO descriptor as LOW

[Linus Walleij]

On Sat, Sep 30, 2017 at 12:53 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/24, Linus Walleij wrote:
>> Requesting the GPIOD_OUT_LOW low will make sure the GPIO is
>> deasserted when requested. The gpiolib core will make sure that
>> if the GPIO line is active low, it will be logically driven high
>> when deasserted, see drivers/gpiolib.c gpiod_configure_flags().
>>
>> Cc: Sergej Sawazki <ce3a@gmx.de>
>> Cc: Jyri Sarha <jsarha@ti.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/clk/clk-gpio.c | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
>> index 9d057073e110..151513c655c3 100644
>> --- a/drivers/clk/clk-gpio.c
>> +++ b/drivers/clk/clk-gpio.c
>> @@ -109,14 +109,6 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
>>       if (!clk_gpio)
>>               return ERR_PTR(-ENOMEM);
>>
>> -     /*
>> -      * Set to disabled no matter what: NOTE if the GPIO line is active low
>> -      * the GPIO descriptor knows this and will set it high to deassert the
>> -      * line. This assumes the GPIO descriptor has been requested using
>> -      * GPIOD_ASIS by the callers so we need to initialize it as disabled here.
>> -      */
>> -     gpiod_set_value(gpiod, 0);
>> -
>
> Is there a reason why we split this patch out and then deleted
> the comment that was introduced in the previous patch? Why not
> combine these two patches into one?

I was worried about the semantic ordering here (in the old code this pulling
low happens at a different point in the sequence) so bisect would find that.

But maybe it's just me being overly cautious.

If you prefer, feel free to squash them when applying.

Yours,
Linus Walleij

Re: [PATCH 2/2] clk: clk-gpio: Request GPIO descriptor as LOW

[Stephen Boyd]

On 09/24, Linus Walleij wrote:
> Requesting the GPIOD_OUT_LOW low will make sure the GPIO is
> deasserted when requested. The gpiolib core will make sure that
> if the GPIO line is active low, it will be logically driven high
> when deasserted, see drivers/gpiolib.c gpiod_configure_flags().
> 
> Cc: Sergej Sawazki <ce3a@gmx.de>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[Stephen Boyd]

On 09/24, Linus Walleij wrote:
> After som grep:ing it turns out nothing in the kernel is really calling
> clk_[hw_]_register_gpio_[gate|mux](). All existing instances are just
> created directly from the device tree probe functions at the bottom of
> the clk-gpio.c clock provider file.
> 
> This means we can change the signature of the function without any
> consequences! Everyone should be using GPIO descriptors now, so let's
> just go in and enforce that.
> 
> This saves a bit of code since GPIO descriptors know inherently if they
> are active low so no need for the code keeping track of that.
> 
> We leave it to the caller to come up with the GPIO descriptor. It is
> nowadays possible to do that even without a corresponding device, so
> no excuse not to pass them around. The one in-kernel user lifecycles
> it using devm_gpiod_get() in gpio_clk_driver_probe().
> 
> Cc: Sergej Sawazki <ce3a@gmx.de>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I don't have GPIO clocks in my setups so this will obviously
> need some testing.
> 
> I guess the public APIs in clk-provider.h could actually be
> removed, but I'm not certain if that is proper. Maybe there
> are plans for this driver to be used by other providers than the
> device tree.

If nobody is using them it's fine to remove them. This falls into
another "basic clk type" that Mike and I don't really like to
keep around.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[Linus Walleij]

On Sat, Sep 30, 2017 at 12:57 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/24, Linus Walleij wrote:

>> I don't have GPIO clocks in my setups so this will obviously
>> need some testing.
>>
>> I guess the public APIs in clk-provider.h could actually be
>> removed, but I'm not certain if that is proper. Maybe there
>> are plans for this driver to be used by other providers than the
>> device tree.
>
> If nobody is using them it's fine to remove them. This falls into
> another "basic clk type" that Mike and I don't really like to
> keep around.

I started to make a patch for it, removing the non-clk_hw* portions
so that at least only the *hw* infix versions would be around.
(I guess that is what you want to optimally keep around.)

It became a bit complex since this driver itself is using the
non-hw calls, and with not being able to test it I chickened out.

But if you like the idea, I can make a second attempt (on top of
these two patches), especially if someone volunteers to test it.

Yours,
Linus Walleij

Re: [PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[Stephen Boyd]

On 10/01, Linus Walleij wrote:
> On Sat, Sep 30, 2017 at 12:57 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 09/24, Linus Walleij wrote:
> 
> >> I don't have GPIO clocks in my setups so this will obviously
> >> need some testing.
> >>
> >> I guess the public APIs in clk-provider.h could actually be
> >> removed, but I'm not certain if that is proper. Maybe there
> >> are plans for this driver to be used by other providers than the
> >> device tree.
> >
> > If nobody is using them it's fine to remove them. This falls into
> > another "basic clk type" that Mike and I don't really like to
> > keep around.
> 
> I started to make a patch for it, removing the non-clk_hw* portions
> so that at least only the *hw* infix versions would be around.
> (I guess that is what you want to optimally keep around.)
> 
> It became a bit complex since this driver itself is using the
> non-hw calls, and with not being able to test it I chickened out.
> 
> But if you like the idea, I can make a second attempt (on top of
> these two patches), especially if someone volunteers to test it.

I don't know if anyone is going to test it. We have only a
handful of users in mainline. I'll just merge these patches and
you can remove the non-clk_hw portions later if you're
interested.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: clk-gpio: Make GPIO clock provider use descriptors only

[Stephen Boyd]

On 09/24, Linus Walleij wrote:
> After som grep:ing it turns out nothing in the kernel is really calling
> clk_[hw_]_register_gpio_[gate|mux](). All existing instances are just
> created directly from the device tree probe functions at the bottom of
> the clk-gpio.c clock provider file.
> 
> This means we can change the signature of the function without any
> consequences! Everyone should be using GPIO descriptors now, so let's
> just go in and enforce that.
> 
> This saves a bit of code since GPIO descriptors know inherently if they
> are active low so no need for the code keeping track of that.
> 
> We leave it to the caller to come up with the GPIO descriptor. It is
> nowadays possible to do that even without a corresponding device, so
> no excuse not to pass them around. The one in-kernel user lifecycles
> it using devm_gpiod_get() in gpio_clk_driver_probe().
> 
> Cc: Sergej Sawazki <ce3a@gmx.de>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project