Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1231950 > unrolled thread
| Started by | Paul Osmialowski <newchief@king.net.pl> |
|---|---|
| First post | 2015-09-24 10:40 +0200 |
| Last post | 2015-10-01 20:10 +0200 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Paul Osmialowski <newchief@king.net.pl> - 2015-09-24 10:40 +0200
Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Stephen Boyd <sboyd@codeaurora.org> - 2015-09-29 01:10 +0200
Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Paul Osmialowski <pawelo@king.net.pl> - 2015-09-29 06:50 +0200
Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Stephen Boyd <sboyd@codeaurora.org> - 2015-10-01 00:10 +0200
Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Paul Osmialowski <pawelo@king.net.pl> - 2015-10-01 10:00 +0200
Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions Stephen Boyd <sboyd@codeaurora.org> - 2015-10-01 20:10 +0200
| From | Paul Osmialowski <newchief@king.net.pl> |
|---|---|
| Date | 2015-09-24 10:40 +0200 |
| Subject | [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qcael-6bA-13@gated-at.bofh.it> |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-09-29 01:10 +0200 |
| Subject | Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qdPIt-5nN-5@gated-at.bofh.it> |
| In reply to | #1231950 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Paul Osmialowski <pawelo@king.net.pl> |
|---|---|
| Date | 2015-09-29 06:50 +0200 |
| Subject | Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qdV1v-4tL-1@gated-at.bofh.it> |
| In reply to | #1234566 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-10-01 00:10 +0200 |
| Subject | Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qexJw-1cv-15@gated-at.bofh.it> |
| In reply to | #1234716 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Paul Osmialowski <pawelo@king.net.pl> |
|---|---|
| Date | 2015-10-01 10:00 +0200 |
| Subject | Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qeGWu-5Xc-19@gated-at.bofh.it> |
| In reply to | #1236824 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-10-01 20:10 +0200 |
| Subject | Re: [PATCH 0/1] add devm_of_clk_get() and devm_of_clk_get_by_name() functions |
| Message-ID | <qeQsO-3Lc-9@gated-at.bofh.it> |
| In reply to | #1237102 |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web