[PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Paul Osmialowski]

From: Paul Osmialowski <pawelo@king.net.pl>

While working on my pinctrl driver I've found lack of devres compatible
equivalent for of_clk_get() function. I'd like to use it for the following
(incomplete) piece of device tree configuration:

pinctrl: pinctrl {
	compatible = "fsl,kinetis-pinctrl";
	#address-cells = <1>;
	#size-cells = <1>;
	ranges;

	port_a@40049000 {
		compatible = "fsl,kinetis-pin-bank";
		reg = <0x40049000 0x1000>;
		clocks = <&sim SIM_CLK_SCGC5_PORTA>;
	};

	port_b@4004a000 {
		compatible = "fsl,kinetis-pin-bank";
		reg = <0x4004a000 0x1000>;
		clocks = <&sim SIM_CLK_SCGC5_PORTB>;
	};
...
};

In my pinconf-generic compatible fsl,kinetis-pinctrl driver, I'm iterating
over fsl,kinetis-pin-bank nodes using for_each_child_of_node(dev->of_node,
child) along with of_match_node() in order to grab resources (I/O base
address, clock gate).

Normally, I'd have to use of_clk_get() on each pin bank device_node and
then worry about proper resource release myself.

IMHO using devres infrastructure for this is far better. This patch adds
missing functions needed to do it that way.

Paul Osmialowski (1):
  clk: add devm_of_clk_get() and devm_of_clk_get_by_name() functions

 drivers/clk/clk-devres.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 20 ++++++++++++++++++++
 2 files changed, 66 insertions(+)

-- 
2.4.9

[PATCH 1/1] clk: add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Paul Osmialowski]

From: Paul Osmialowski <pawelo@king.net.pl>

These two functions were added to ease management of clocks obtained
from OF device nodes.

Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
---
 drivers/clk/clk-devres.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 20 ++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..197075a 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,52 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+#ifdef CONFIG_OF
+
+struct clk *devm_of_clk_get(struct device *dev, struct device_node *np,
+			    int index)
+{
+	struct clk **ptr, *clk;
+
+	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	clk = of_clk_get(np, index);
+	if (!IS_ERR(clk)) {
+		*ptr = clk;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return clk;
+}
+EXPORT_SYMBOL(devm_of_clk_get);
+
+struct clk *devm_of_clk_get_by_name(struct device *dev, struct device_node *np,
+				    const char *name)
+{
+	struct clk **ptr, *clk;
+
+	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	clk = of_clk_get_by_name(np, name);
+	if (!IS_ERR(clk)) {
+		*ptr = clk;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return clk;
+}
+EXPORT_SYMBOL(devm_of_clk_get_by_name);
+
+#endif /* CONFIG_OF */
+
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0df4a51..d7763f1 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -504,4 +504,24 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
 }
 #endif
 
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) && defined(CONFIG_HAVE_CLK)
+struct clk *devm_of_clk_get(struct device *dev, struct device_node *np,
+			    int index);
+struct clk *devm_of_clk_get_by_name(struct device *dev, struct device_node *np,
+				    const char *name);
+#else
+static inline struct clk *devm_of_clk_get(struct device *dev,
+					  struct device_node *np,
+					  int index)
+{
+	return ERR_PTR(-ENOENT);
+}
+static inline struct clk *devm_of_clk_get_by_name(struct device *dev,
+						  struct device_node *np,
+						  const char *name)
+{
+	return ERR_PTR(-ENOENT);
+}
+#endif
+
 #endif
-- 
2.4.9

Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Stephen Boyd]

On 09/24, Paul Osmialowski wrote:
> From: Paul Osmialowski <pawelo@king.net.pl>
> 
> While working on my pinctrl driver I've found lack of devres compatible
> equivalent for of_clk_get() function. I'd like to use it for the following
> (incomplete) piece of device tree configuration:
> 
> pinctrl: pinctrl {
> 	compatible = "fsl,kinetis-pinctrl";
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges;
> 
> 	port_a@40049000 {
> 		compatible = "fsl,kinetis-pin-bank";
> 		reg = <0x40049000 0x1000>;
> 		clocks = <&sim SIM_CLK_SCGC5_PORTA>;
> 	};
> 
> 	port_b@4004a000 {
> 		compatible = "fsl,kinetis-pin-bank";
> 		reg = <0x4004a000 0x1000>;
> 		clocks = <&sim SIM_CLK_SCGC5_PORTB>;
> 	};
> ...
> };
> 
> In my pinconf-generic compatible fsl,kinetis-pinctrl driver, I'm iterating
> over fsl,kinetis-pin-bank nodes using for_each_child_of_node(dev->of_node,
> child) along with of_match_node() in order to grab resources (I/O base
> address, clock gate).
> 
> Normally, I'd have to use of_clk_get() on each pin bank device_node and
> then worry about proper resource release myself.
> 

I'd say your binding is wrong. Either the container node
"pinctrl" is a software concept that contains the two devices for
port_a and port_b or there's only one pinctrl device that happens
to span some number of 0x1000 size banks. The former would be
written as so

	pinctrl {
		compatible = "fsl,kenetis-pinctrl";
		reg = <0x40049000 0x2000>;
		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;
	};

and the latter would drop the container node and have two nodes
that probed the same driver instance twice.


 	port_a@40049000 {
 		compatible = "fsl,kinetis-pin-bank";
 		reg = <0x40049000 0x1000>;
 		clocks = <&sim SIM_CLK_SCGC5_PORTA>;
 	};
 
 	port_b@4004a000 {
 		compatible = "fsl,kinetis-pin-bank";
 		reg = <0x4004a000 0x1000>;
 		clocks = <&sim SIM_CLK_SCGC5_PORTB>;
 	};

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

Re: [PATCH 1/1] clk: add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Stephen Boyd]

On 09/24, Paul Osmialowski wrote:
> From: Paul Osmialowski <pawelo@king.net.pl>
> 
> These two functions were added to ease management of clocks obtained
> from OF device nodes.
> 

All the words that were in the cover letter for this single patch
should have been here in the commit text instead. If we were to
look back on this commit text a year from now we'd learn that
things got easier for clock management, but not *why* things got
easier.

Cover letters for single patches are practically useless by the
way. That's because they duplicate the diffstat that is already
in the patch and summarize the subject of the patch. Please don't
send cover letters for single patches.

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 0df4a51..d7763f1 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -504,4 +504,24 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
>  }
>  #endif
>  
> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) && defined(CONFIG_HAVE_CLK)

Doesn't CONFIG_COMMON_CLK imply CONFIG_HAVE_CLK? So drop that
config check? Also, devm_*() functions are documented in
Documentation, so please update that file too, if it even makes
sense to have this new API.

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

Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Paul Osmialowski]

Hi Stephen,

Thanks for all of your comments.

On Mon, 28 Sep 2015, Stephen Boyd wrote:

> I'd say your binding is wrong. Either the container node
> "pinctrl" is a software concept that contains the two devices for
> port_a and port_b or there's only one pinctrl device that happens
> to span some number of 0x1000 size banks. The former would be
> written as so
> 
> 	pinctrl {
> 		compatible = "fsl,kenetis-pinctrl";
> 		reg = <0x40049000 0x2000>;
> 		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;
> 	};
>

I tried this and actually this looks similar to my first approach to this 
driver. I wasn't happy with the fact that pin banks are so loosely coupled 
with resources they use.

I thought that making pin controller a container for pin bank devices 
would create better coupling and simply look better in DT file.

With this first approach example pin definition would look like:

fsl,kinetis-pins = <PORT_A 0 1 &pcfg_pull_pin_default> ...

where the binding format is fsl,kinetis-pins = <bank pin function CONFIG>

&pcfg_pull_pin_default is defined as:

pcfg_pull_pin_default: pcfg-pull-pin-default {
	bias-pull-pin-default;
};

...and PORT_A would have to be defined as preprocessor macro in some 
header file:

#define PORT_A   0
#define PORT_B   1
#define PORT_C   2
#define PORT_D   3
#define PORT_E   4
#define PORT_F   5
#define PORT_NUM 6

That's another thing I'd want to avoid.

I wanted a DT file which driver could use to figure out how many pin banks 
there are, what clocks and IO ranges they use and how pins are associated 
with banks.

Now I see I pasted example from some old file (sorry for that), it differs 
in one small detail, so again:

pinctrl: pinctrl {
        compatible = "fsl,kinetis-pinctrl";
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;

        port_a: pin-bank@40049000 {
                compatible = "fsl,kinetis-pin-bank";
                reg = <0x40049000 0x1000>;
                clocks = <&sim SIM_CLK_SCGC5_PORTA>;
        };

        port_b: pin-bank@4004a000 {
                compatible = "fsl,kinetis-pin-bank";
                reg = <0x4004a000 0x1000>;
                clocks = <&sim SIM_CLK_SCGC5_PORTB>;
        };
...
};

Now, assuming use of of_find_node_by_phandle(), example pin definition 
would look like:

fsl,kinetis-pins = <&port_a 0 1 &pcfg_pull_pin_default> ...

Things are getting connected together, no preprocessor definitions, no 
extra header file. What can be wrong with this design?

I'll prepare second iteration with updated documentation (and other 
smaller cleanups) soon.

Thanks,
Paul

Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Stephen Boyd]

On 09/29, Paul Osmialowski wrote:
> Hi Stephen,
> 
> Thanks for all of your comments.
> 
> On Mon, 28 Sep 2015, Stephen Boyd wrote:
> 
> > I'd say your binding is wrong. Either the container node
> > "pinctrl" is a software concept that contains the two devices for
> > port_a and port_b or there's only one pinctrl device that happens
> > to span some number of 0x1000 size banks. The former would be
> > written as so
> > 
> > 	pinctrl {
> > 		compatible = "fsl,kenetis-pinctrl";
> > 		reg = <0x40049000 0x2000>;
> > 		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;
> > 	};
> >
> 
> I tried this and actually this looks similar to my first approach to this 
> driver. I wasn't happy with the fact that pin banks are so loosely coupled 
> with resources they use.
> 
> I thought that making pin controller a container for pin bank devices 
> would create better coupling and simply look better in DT file.
> 
> With this first approach example pin definition would look like:
> 
> fsl,kinetis-pins = <PORT_A 0 1 &pcfg_pull_pin_default> ...
> 
> where the binding format is fsl,kinetis-pins = <bank pin function CONFIG>
> 
> &pcfg_pull_pin_default is defined as:
> 
> pcfg_pull_pin_default: pcfg-pull-pin-default {
> 	bias-pull-pin-default;
> };

The function and pin number should be part of the configuration
node as well. Say we need to configure the uart pins for
bias-pull-pin-default and the rx pin is in port at pin 3 and the
tx pin is in port b at pin 2.

In the pinctrl node we would have

 	pinctrl {
 		compatible = "fsl,kenetis70-pinctrl";
 		reg = <0x40049000 0x2000>;
 		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;

		uart_default: uart_default {
			mux {
				pins = "porta_3", "portb_2";
				function = "uart";
			};

			rx {
				bias-pull-pin-default;
			};
		};
 	};

And then in the uart node we would have

	uart@f00000 {
		compatible = "vendor,uart";
		reg = <0xf00000 0x100>;
		pinctrl-names = "default";
		pinctrl-0 = <&uart_default>;
	};

> 
> ...and PORT_A would have to be defined as preprocessor macro in some 
> header file:
> 
> #define PORT_A   0
> #define PORT_B   1
> #define PORT_C   2
> #define PORT_D   3
> #define PORT_E   4
> #define PORT_F   5
> #define PORT_NUM 6
> 
> That's another thing I'd want to avoid.

Ok. We avoided it in the example above by putting the port in the
pin name.

> 
> I wanted a DT file which driver could use to figure out how many pin banks 
> there are, what clocks and IO ranges they use and how pins are associated 
> with banks.
> 
> Now I see I pasted example from some old file (sorry for that), it differs 
> in one small detail, so again:
> 
> pinctrl: pinctrl {
>         compatible = "fsl,kinetis-pinctrl";
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges;
> 
>         port_a: pin-bank@40049000 {
>                 compatible = "fsl,kinetis-pin-bank";
>                 reg = <0x40049000 0x1000>;
>                 clocks = <&sim SIM_CLK_SCGC5_PORTA>;
>         };
> 
>         port_b: pin-bank@4004a000 {
>                 compatible = "fsl,kinetis-pin-bank";
>                 reg = <0x4004a000 0x1000>;
>                 clocks = <&sim SIM_CLK_SCGC5_PORTB>;
>         };
> ...
> };
> 
> Now, assuming use of of_find_node_by_phandle(), example pin definition 
> would look like:
> 
> fsl,kinetis-pins = <&port_a 0 1 &pcfg_pull_pin_default> ...
> 
> Things are getting connected together, no preprocessor definitions, no 
> extra header file. What can be wrong with this design?

Having fsl,kinetis-pins is already a problem because it doesn't
use the generic pinctrl bindings to pick the function for a pin.
Once you use the generic bindings, the need for a header file and
preprocessor definitions will go away too.

I looked at the reference manual for this k70 SoC. It seems that
the hardware designers had a macro with a maximum of 32 pins, but
this SoC needed more than that, so they copy/pasted the macro a
few times to get the number of pins they needed. That's fine. It
can be treated either as one big pinctrl device and driver or as
multiple devices (one for each port) where the same driver probes
multiple times.

Either way, to handle the functions you're going to need to have
an SoC specific driver that knows what functions are supported on
that particular SoC. So it's probably easier to do the big device
and driver, and then you would know how many banks there are
because the SoC specific compatible string would convey that
information already.

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

Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Paul Osmialowski]

Hi Stephen,

On Wed, 30 Sep 2015, Stephen Boyd wrote:

> In the pinctrl node we would have
> 
>  	pinctrl {
>  		compatible = "fsl,kenetis70-pinctrl";
>  		reg = <0x40049000 0x2000>;
>  		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;
> 
> 		uart_default: uart_default {
> 			mux {
> 				pins = "porta_3", "portb_2";
> 				function = "uart";
> 			};
> 
> 			rx {
> 				bias-pull-pin-default;
> 			};
> 		};
>  	};
> 
> And then in the uart node we would have
> 
> 	uart@f00000 {
> 		compatible = "vendor,uart";
> 		reg = <0xf00000 0x100>;
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&uart_default>;
> 	};
> 

Seems like there's another thing I wanted to avoid. The correctness of 
these pin strings will not be checked until the runtime. They need to 
properly encode pin bank and pin number within the bank. No chances it can 
be validated at .dtb build time. But I guess this is proper way for 
generic pinctrl bindings. I mostly (but not completely) based my approach 
on rockchip examples (e.g. rk3288) but it looks like they are not entirely 
sane.

Thanks,
Paul

Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Stephen Boyd]

On 10/01, Paul Osmialowski wrote:
> Hi Stephen,
> 
> On Wed, 30 Sep 2015, Stephen Boyd wrote:
> 
> > In the pinctrl node we would have
> > 
> >  	pinctrl {
> >  		compatible = "fsl,kenetis70-pinctrl";
> >  		reg = <0x40049000 0x2000>;
> >  		clocks = <&sim SIM_CLK_SCGC5_PORTA>, <&sim SIM_CLK_SCGC5_PORTB>;
> > 
> > 		uart_default: uart_default {
> > 			mux {
> > 				pins = "porta_3", "portb_2";
> > 				function = "uart";
> > 			};
> > 
> > 			rx {
> > 				bias-pull-pin-default;
> > 			};
> > 		};
> >  	};
> > 
> > And then in the uart node we would have
> > 
> > 	uart@f00000 {
> > 		compatible = "vendor,uart";
> > 		reg = <0xf00000 0x100>;
> > 		pinctrl-names = "default";
> > 		pinctrl-0 = <&uart_default>;
> > 	};
> > 
> 
> Seems like there's another thing I wanted to avoid. The correctness of 
> these pin strings will not be checked until the runtime. They need to 
> properly encode pin bank and pin number within the bank. No chances it can 
> be validated at .dtb build time. But I guess this is proper way for 
> generic pinctrl bindings. I mostly (but not completely) based my approach 
> on rockchip examples (e.g. rk3288) but it looks like they are not entirely 
> sane.

I don't see how it could be validated with the <&port pin
function config> binding either. Let's hope that people test
their code, including whatever dts files they produce.

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

[PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions

[Paul Osmialowski]

From: Paul Osmialowski <pawelo@king.net.pl>

While working on my pinctrl driver I've found lack of devres compatible
equivalent for of_clk_get() function. I'd like to use it for the following
(incomplete) piece of device tree configuration:

pinctrl: pinctrl {
	compatible = "fsl,kinetis-pinctrl";
	#address-cells = <1>;
	#size-cells = <1>;
	ranges;

	port_a@40049000 {
		compatible = "fsl,kinetis-pin-bank";
		reg = <0x40049000 0x1000>;
		clocks = <&sim SIM_CLK_SCGC5_PORTA>;
	};

	port_b@4004a000 {
		compatible = "fsl,kinetis-pin-bank";
		reg = <0x4004a000 0x1000>;
		clocks = <&sim SIM_CLK_SCGC5_PORTB>;
	};
...
};

In my pinconf-generic compatible fsl,kinetis-pinctrl driver, I'm iterating
over fsl,kinetis-pin-bank nodes using for_each_child_of_node(dev->of_node,
child) along with of_match_node() in order to grab resources (I/O base
address, clock gate).

Normally, I'd have to use of_clk_get() on each pin bank device_node and
then worry about proper resource release myself.

IMHO using devres infrastructure for this is far better. This patch adds
missing functions needed to do it that way.

Paul Osmialowski (1):
  clk: add devm_of_clk_get() and devm_of_clk_get_by_name() functions

 drivers/clk/clk-devres.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 20 ++++++++++++++++++++
 2 files changed, 66 insertions(+)

-- 
2.4.9