Re: [PATCH] pinctrl: samsung: Allow grouping multiple pinmux/pinconf nodes
On Wed, Nov 20, 2013 at 3:00 PM, Tomasz Figa t.f...@samsung.com wrote: Stephen: Is the lifetime of the string returned by of_property_read_string_index() really so short that you must copy the string? I'd be tempted just to store the pointer, although perhaps you need to get() the node so that's safe. Right. I have done it the copy-less way in other places, but missed this one. Thanks. So I guess I'm waiting for a new version of this patch, right? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pinctrl: samsung: Allow grouping multiple pinmux/pinconf nodes
On Monday 25 of November 2013 15:46:12 Linus Walleij wrote: On Wed, Nov 20, 2013 at 3:00 PM, Tomasz Figa t.f...@samsung.com wrote: Stephen: Is the lifetime of the string returned by of_property_read_string_index() really so short that you must copy the string? I'd be tempted just to store the pointer, although perhaps you need to get() the node so that's safe. Right. I have done it the copy-less way in other places, but missed this one. Thanks. So I guess I'm waiting for a new version of this patch, right? Right. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pinctrl: samsung: Allow grouping multiple pinmux/pinconf nodes
Hi Stephen,
On Tuesday 19 of November 2013 12:10:01 Stephen Warren wrote:
On 11/19/2013 10:10 AM, Tomasz Figa wrote:
One of remaining limitations of current pinctrl-samsung driver was
the inability to parse multiple pinmux/pinconf group nodes grouped
inside a single device tree node. It made defining groups of pins for
single purpose, but with different parameters very inconvenient.
This patch implements Tegra-like support for grouping multiple pinctrl
groups inside one device tree node, by completely changing the way
pin groups and functions are parsed from device tree.
The code creating
pinctrl maps from DT nodes has been borrowed from pinctrl-tegra,
A lot of the Tegra code has been slightly generalized and put into
pinconf-generic.c. Can the Samsung driver be converted to use that core
code rather than adding another copy of it? Perhaps this isn't possible
given the backwards-compatibility requirements that allow either 1- or
2-level nodes though, although I imagine that could be added to the core
code. One thing you'd certainly need to do is enhance the code in
pinconf-generic.c so that you could substitute your own
pinconf_generic_parse_dt_config() function or dt_params[] table, to
allow for the SoC-specific property names, but I doubt that's too hard.
Tegra could be converted then too:-)
I can't say that it's not a good idea, but I would prefer this to be
merged first as an Exynos-specific patch and only then somehow try to
figure out how to remove code duplication. This is due to the fact that
I won't have too much time to work on this in very near future, but this
feature itself is rather convenient and it would be nice to have it
anyway.
while
the initial creation of groups and functions has been completely
rewritten with following assumptions:
- each group consists of just one pin and does not depend on data
from device tree,
- each function is represented by a device tree child node of the
pin controller, which in turn can contain multiple child nodes
for pins that need to have different configuration values.
OK, I think that sounds reasonable.
Device Tree bindings are fully backwards compatible. New functionality
can be used by defining a new pinctrl group consisting of several child
nodes, as on following example:
sd4_bus8: sd4-bus-width8 {
part-1 {
samsung,pins = gpk0-3, gpk0-4,
gpk0-5, gpk0-6;
samsung,pin-function = 3;
samsung,pin-pud = 3;
samsung,pin-drv = 3;
};
part-2 {
samsung,pins = gpk1-3, gpk1-4,
gpk1-5, gpk1-6;
samsung,pin-function = 4;
samsung,pin-pud = 4;
samsung,pin-drv = 3;
};
};
OK, that all looks great!
diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
The DT changes fully, and the code a little briefly,
Reviewed-by: Stephen Warren swar...@nvidia.com
Just a minor comment below,
diff --git a/drivers/pinctrl/pinctrl-samsung.c
b/drivers/pinctrl/pinctrl-samsung.c
+static int samsung_pinctrl_create_function(struct device *dev,
+ struct samsung_pinctrl_drv_data *drvdata,
+ struct device_node *func_np,
+ struct samsung_pmx_func *func)
...
+ for (i = 0; i npins; ++i) {
+ const char *gname;
+ char *gname_copy;
+
+ ret = of_property_read_string_index(func_np, samsung,pins,
+ i, gname);
+ if (ret) {
+ dev_err(dev,
+ failed to read pin name %d from %s node\n,
+ i, func_np-name);
+ return ret;
}
+
+ gname_copy = devm_kzalloc(dev, strlen(gname) + 1, GFP_KERNEL);
+ if (!gname_copy)
+ return -ENOMEM;
+ strcpy(gname_copy, gname);
Is the lifetime of the string returned by
of_property_read_string_index() really so short that you must copy the
string? I'd be tempted just to store the pointer, although perhaps you
need to get() the node so that's safe.
Right. I have done it the copy-less way in other places, but missed this
one. Thanks.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pinctrl: samsung: Allow grouping multiple pinmux/pinconf nodes
One of remaining limitations of current pinctrl-samsung driver was
the inability to parse multiple pinmux/pinconf group nodes grouped
inside a single device tree node. It made defining groups of pins for
single purpose, but with different parameters very inconvenient.
This patch implements Tegra-like support for grouping multiple pinctrl
groups inside one device tree node, by completely changing the way
pin groups and functions are parsed from device tree. The code creating
pinctrl maps from DT nodes has been borrowed from pinctrl-tegra, while
the initial creation of groups and functions has been completely
rewritten with following assumptions:
- each group consists of just one pin and does not depend on data
from device tree,
- each function is represented by a device tree child node of the
pin controller, which in turn can contain multiple child nodes
for pins that need to have different configuration values.
Device Tree bindings are fully backwards compatible. New functionality
can be used by defining a new pinctrl group consisting of several child
nodes, as on following example:
sd4_bus8: sd4-bus-width8 {
part-1 {
samsung,pins = gpk0-3, gpk0-4,
gpk0-5, gpk0-6;
samsung,pin-function = 3;
samsung,pin-pud = 3;
samsung,pin-drv = 3;
};
part-2 {
samsung,pins = gpk1-3, gpk1-4,
gpk1-5, gpk1-6;
samsung,pin-function = 4;
samsung,pin-pud = 4;
samsung,pin-drv = 3;
};
};
Tested on Exynos4210-Trats board and a custom Exynos4212-based one.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../bindings/pinctrl/samsung-pinctrl.txt | 23 +-
drivers/pinctrl/pinctrl-samsung.c | 619 -
drivers/pinctrl/pinctrl-samsung.h | 1 +
3 files changed, 392 insertions(+), 251 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index 257677d..fe34cbb 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -43,7 +43,11 @@ Required Properties:
- Pin mux/config groups as child nodes: The pin mux (selecting pin function
mode) and pin config (pull up/down, driver strength) settings are represented
as child nodes of the pin-controller node. There should be atleast one
- child node and there is no limit on the count of these child nodes.
+ child node and there is no limit on the count of these child nodes. It is
+ also possible for a child node to consist of several further child nodes
+ to allow grouping multiple pinctrl groups into one. The format of second
+ level child nodes is exactly the same as for first level ones and is
+ described below.
The child node should contain a list of pin(s) on which a particular pin
function selection or pin configuration (or both) have to applied. This
@@ -248,6 +252,23 @@ Example 1: A pin-controller node with pin groups.
samsung,pin-pud = 3;
samsung,pin-drv = 0;
};
+
+ sd4_bus8: sd4-bus-width8 {
+ part-1 {
+ samsung,pins = gpk0-3, gpk0-4,
+ gpk0-5, gpk0-6;
+ samsung,pin-function = 3;
+ samsung,pin-pud = 3;
+ samsung,pin-drv = 3;
+ };
+ part-2 {
+ samsung,pins = gpk1-3, gpk1-4,
+ gpk1-5, gpk1-6;
+ samsung,pin-function = 4;
+ samsung,pin-pud = 4;
+ samsung,pin-drv = 3;
+ };
+ };
};
Example 2: A pin-controller node with external wakeup interrupt controller
node.
diff --git a/drivers/pinctrl/pinctrl-samsung.c
b/drivers/pinctrl/pinctrl-samsung.c
index 47ec2e8..c752de4 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -40,9 +40,9 @@
/* list of all possible config options supported */
static struct pin_config {
- char*prop_cfg;
- unsigned intcfg_type;
-} pcfgs[] = {
+ const char *property;
+ enum pincfg_type param;
+} cfg_params[] = {
{ samsung,pin-pud, PINCFG_TYPE_PUD },
{ samsung,pin-drv, PINCFG_TYPE_DRV },
{ samsung,pin-con-pdn, PINCFG_TYPE_CON_PDN },
@@ -59,163 +59,242 @@ static inline struct
Re: [PATCH] pinctrl: samsung: Allow grouping multiple pinmux/pinconf nodes
On 11/19/2013 10:10 AM, Tomasz Figa wrote:
One of remaining limitations of current pinctrl-samsung driver was
the inability to parse multiple pinmux/pinconf group nodes grouped
inside a single device tree node. It made defining groups of pins for
single purpose, but with different parameters very inconvenient.
This patch implements Tegra-like support for grouping multiple pinctrl
groups inside one device tree node, by completely changing the way
pin groups and functions are parsed from device tree.
The code creating
pinctrl maps from DT nodes has been borrowed from pinctrl-tegra,
A lot of the Tegra code has been slightly generalized and put into
pinconf-generic.c. Can the Samsung driver be converted to use that core
code rather than adding another copy of it? Perhaps this isn't possible
given the backwards-compatibility requirements that allow either 1- or
2-level nodes though, although I imagine that could be added to the core
code. One thing you'd certainly need to do is enhance the code in
pinconf-generic.c so that you could substitute your own
pinconf_generic_parse_dt_config() function or dt_params[] table, to
allow for the SoC-specific property names, but I doubt that's too hard.
Tegra could be converted then too:-)
while
the initial creation of groups and functions has been completely
rewritten with following assumptions:
- each group consists of just one pin and does not depend on data
from device tree,
- each function is represented by a device tree child node of the
pin controller, which in turn can contain multiple child nodes
for pins that need to have different configuration values.
OK, I think that sounds reasonable.
Device Tree bindings are fully backwards compatible. New functionality
can be used by defining a new pinctrl group consisting of several child
nodes, as on following example:
sd4_bus8: sd4-bus-width8 {
part-1 {
samsung,pins = gpk0-3, gpk0-4,
gpk0-5, gpk0-6;
samsung,pin-function = 3;
samsung,pin-pud = 3;
samsung,pin-drv = 3;
};
part-2 {
samsung,pins = gpk1-3, gpk1-4,
gpk1-5, gpk1-6;
samsung,pin-function = 4;
samsung,pin-pud = 4;
samsung,pin-drv = 3;
};
};
OK, that all looks great!
diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
The DT changes fully, and the code a little briefly,
Reviewed-by: Stephen Warren swar...@nvidia.com
Just a minor comment below,
diff --git a/drivers/pinctrl/pinctrl-samsung.c
b/drivers/pinctrl/pinctrl-samsung.c
+static int samsung_pinctrl_create_function(struct device *dev,
+ struct samsung_pinctrl_drv_data *drvdata,
+ struct device_node *func_np,
+ struct samsung_pmx_func *func)
...
+ for (i = 0; i npins; ++i) {
+ const char *gname;
+ char *gname_copy;
+
+ ret = of_property_read_string_index(func_np, samsung,pins,
+ i, gname);
+ if (ret) {
+ dev_err(dev,
+ failed to read pin name %d from %s node\n,
+ i, func_np-name);
+ return ret;
}
+
+ gname_copy = devm_kzalloc(dev, strlen(gname) + 1, GFP_KERNEL);
+ if (!gname_copy)
+ return -ENOMEM;
+ strcpy(gname_copy, gname);
Is the lifetime of the string returned by
of_property_read_string_index() really so short that you must copy the
string? I'd be tempted just to store the pointer, although perhaps you
need to get() the node so that's safe.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
