Hi Akshay,
On Mon, Feb 25, 2013 at 10:04 AM, Akshay Saraswat akshay.s@samsung.com wrote:
Hi Simon,
Hi Akshay,
On Thu, Feb 21, 2013 at 11:05 AM, Akshay Saraswat akshay.s@samsung.com wrote:
This patch adds fdt nodes for peripherals which require pin muxing and configuration. Device tree bindings for pinctrl are kept same as required for Linux. Existing pinmux code modified to retrieve gpio range and function related info from fdt.
Depends-on: [U-Boot] [PATCH 0/4 V3] EXYNOS5: Add GPIO numbering feature URL: http://lists.denx.de/pipermail/u-boot/2013-February/146151.html
Signed-off-by: Akshay Saraswat akshay.s@samsung.com
Changes since v2: - Rebased as per new version of GPIO numbering patch-set.
This looks pretty reasonable to me. Please find some comment below.
As a general comment, it seems to read from the FDT each time anything is changed. I suppose that is efficient on space, and allows it to work prior to malloc() being available, but isn't it slow? Perhaps the data should be cached?
Malloc is not present initially with us which you have already highlighted, adding to that we dont use same value more than once. Hence, I found it better to stick with FDT reading instead of creating a linked list of all nodes. Please suggest a location where we shall create the list if we need to maintain the cache.
I think this is OK for a start.
arch/arm/cpu/armv7/exynos/pinmux.c | 379 +++++++++++---- arch/arm/dts/exynos5250-pinctrl.dtsi | 675 ++++++++++++++++++++++++++ arch/arm/dts/exynos5250.dtsi | 1 + doc/device-tree-bindings/samsung-pinctrl.txt | 253 ++++++++++ include/fdtdec.h | 1 + lib/fdtdec.c | 1 + 6 files changed, 1204 insertions(+), 106 deletions(-) create mode 100644 arch/arm/dts/exynos5250-pinctrl.dtsi create mode 100644 doc/device-tree-bindings/samsung-pinctrl.txt
diff --git a/arch/arm/cpu/armv7/exynos/pinmux.c b/arch/arm/cpu/armv7/exynos/pinmux.c index a01ce0c..f4dccad 100644 --- a/arch/arm/cpu/armv7/exynos/pinmux.c +++ b/arch/arm/cpu/armv7/exynos/pinmux.c @@ -27,6 +27,18 @@ #include <asm/arch/pinmux.h> #include <asm/arch/sromc.h>
+DECLARE_GLOBAL_DATA_PTR;
+/* Struct for storing pin function and gpio related info */ +struct pin_group {
void *dev_name;What is this for?
This is the device name to search for corresponding node. Since we cannot declare a compatible property for every node, so I used names to search for nodes.
OK I see - the comment help thank you.
int npins;int function;int pull_mode;int drive_strength;Should add comments about the S5P_GPIO... defines to use in each case.
enum exynos5_gpio_pin gpio[];This array has no members?
This is a dynamic array because we do not know exact number of pins in every node.
+};
struct gpio_name_num_table exynos5_gpio_table[] = { { 'a', GPIO_A00 }, { 'b', GPIO_B00 }, @@ -42,87 +54,202 @@ struct gpio_name_num_table exynos5_gpio_table[] = { { 'z', GPIO_Z0 }, };
-static void exynos5_uart_config(int peripheral) +static void get_pins(const struct fdt_property *fprop, struct pin_group *pingrp) {
int i, start, count;
int i;char gpio[5];pingrp->npins = 0;for (i = 0; !(fprop->data[i] == (int)NULL &&fprop->data[i-1] == (int)NULL); i += 7) {This is a bit obtuse - please add a comment
int pin_num = -1;gpio[0] = fprop->data[i];gpio[1] = fprop->data[i + 1];gpio[2] = fprop->data[i + 2];gpio[3] = fprop->data[i + 3];gpio[4] = fprop->data[i + 5];What is this doing? Please add a comment.
pin_num = name_to_gpio(gpio);if (pin_num >= 0) {pingrp->gpio[pingrp->npins] = pin_num;This seems to be assigning to something with no members.
Actually I tried to make a dynamic array because we dont know the exact number of pins for every node. Please tell me if this introduces any bug. I'll try to figure some other solution.
pingrp->npins++;}}+}
+static int get_fdt_values(struct pin_group *pingrp)
Please add a function comment.
+{
int i;int node, subnode;const struct fdt_property *fprop;for (i = 0; i < 4; i++) {Why 4? I think you should just continue until node returns -ve.
/* Get the node from FDT for pinctrl */node = fdtdec_next_compatible(gd->fdt_blob, 0,COMPAT_SAMSUNG_PINCTRL);if (node < 0) {printf("PINCTRL: No node for pinctrl in device tree\n");debug()
Only give this error if you don't find any match
return -1;}subnode = fdt_subnode_offset(gd->fdt_blob,node, pingrp->dev_name);if (subnode < 0)continue;fprop = fdt_get_property(gd->fdt_blob,subnode, "samsung,pins", NULL);Suggest you use fdt_getprop() here.
I tried that earlier. Both use fdt_get_property_namelen underneath. "prop" check in case of fdt_getprop_namelen hinders boot instead of the fact that pointer is returned everytime in case of fdt_get_property. So I used fdt_get_property.
Sorry I don't understand this.
fdt_getprop() returns a pointer to fprop->data, so you can just check for NULL Here.
pingrp->function = fdtdec_get_int(gd->fdt_blob,subnode, "samsung,pin-function", -1);pingrp->pull_mode = fdtdec_get_int(gd->fdt_blob,subnode, "samsung,pin-pud", -1);pingrp->drive_strength = fdtdec_get_int(gd->fdt_blob,subnode, "samsung,pin-drv", -1);Error checking on these? Is -1 a valid value?
get_pins(fprop, pingrp);return 0;}debug("PINCTRL: No subnode for %s\n", (char *)pingrp->dev_name);Can dev_name be const char *?
return -1;+}
+static void pin_config_group_set(struct pin_group *pingrp) +{
int i;for (i = 0; i < pingrp->npins; i++) {gpio_cfg_pin(pingrp->gpio[i], S5P_GPIO_FUNC(pingrp->function));gpio_set_pull(pingrp->gpio[i], pingrp->pull_mode);gpio_set_drv(pingrp->gpio[i], pingrp->drive_strength);}+}
+static int exynos5_uart_config(int peripheral) +{
char *data, *fctl = NULL;struct pin_group pingrp; switch (peripheral) { case PERIPH_ID_UART0:
start = GPIO_A00;count = 4;
data = "uart0-data";fctl = "uart0-fctl"; break; case PERIPH_ID_UART1:
start = GPIO_D00;count = 4;
data = "uart1-data";fctl = "uart1-fctl"; break; case PERIPH_ID_UART2:
start = GPIO_A10;count = 4;
data = "uart2-data";fctl = "uart2-fctl"; break; case PERIPH_ID_UART3:
start = GPIO_A14;count = 2;
data = "uart3-data"; break; }
for (i = start; i < start + count; i++) {gpio_set_pull(i, S5P_GPIO_PULL_NONE);gpio_cfg_pin(i, S5P_GPIO_FUNC(0x2));
pingrp.dev_name = data;/** Retrieve and apply pin config details* from fdt for this UART's data line.*/if (get_fdt_values(&pingrp))return -1;pin_config_group_set(&pingrp);Suggest you add a function which does both of thees things - i.e. get_fdt_values() and pin_config_group_set().
Also probably should have a pinmux_ prefix on the latter, given the name of this file.
We need to separate these functions, because on the basis of flags we need to increment pin numbers at times. Like in SROM we do it for CS1.
I think you can still keep the separate functions, but create a new one in addition which does both things. You mostly call the two together.
Regards, Simon