: pinctrl-single support DT

drivers/pinctrl/pinctrl-single.c +1 -0

@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 

drivers/pinctrl/pinctrl-single.c +12 -0

+	if (!num_configs)
+		return 0;
+	config = devm_kzalloc(pcs->dev, sizeof(*config) * num_configs,
+			      GFP_KERNEL);
+	if (!config) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	index = 0;
+
+	if (!of_property_read_u32(np, PCS_SCHMITT_NAME, &value))
+		config[index++] =

drivers/pinctrl/pinctrl-single.c +23 -0

+static int pcs_dt_check_maps(struct device_node *np, unsigned *num_maps,
+			     unsigned *num_configs)
+{
+	unsigned size;
+
+	*num_maps = 0;
+	*num_configs = 0;
+	if (of_get_property(np, PCS_MUX_PINS_NAME, &size)
+		|| of_get_property(np, PCS_MUX_BITS_NAME, &size))
+		(*num_maps)++;
+	if (of_get_property(np, PCS_SCHMITT_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_BIAS_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_POWER_SOURCE_NAME, &size))
+		(*num_configs)++;
+	if (*num_configs)
+		(*num_maps)++;
+	if (!(*num_maps))
+		return -EINVAL;
+	return 0;
+}
+
 /**
  * pcs_dt_node_to_map() - allocates and parses pinctrl maps
  * @pctldev: pinctrl instance

Quoted 12 lines

drivers/pinctrl/pinctrl-single.c +1 -0

@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 

drivers/pinctrl/pinctrl-single.c line 37

Quoted 9 lines

drivers/pinctrl/pinctrl-single.c +3 -0

@@ -28,6 +29,9 @@
 #define DRIVER_NAME                  "pinctrl-single"
 #define PCS_MUX_PINS_NAME            "pinctrl-single,pins"
 #define PCS_MUX_BITS_NAME            "pinctrl-single,bits"
+#define PCS_BIAS_NAME                        "pinctrl-single,bias"
+#define PCS_POWER_SOURCE_NAME                "pinctrl-single,power-source"
+#define PCS_SCHMITT_NAME             "pinctrl-single,input-schmitt"
 #define PCS_REG_NAME_LEN             ((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED             ~0U
 #define PCS_MAX_GPIO_VALUES          3

Here too you can remove the new defines.

Quoted 6 lines

static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
struct device_node *np, struct pinctrl_map **map,
+ unsigned num_configs,
const char **pgnames)
{

...

Then I suggest you add a generic pinconf-generic property to indicate the controller supports pinconf. At least for omaps, only some register ranges support pinconf. And by adding pinconf-generic, you can parse it once during the probe of pinctrl-single.c, and set pcs->pinconf flag.

Then you can use here and avoid calling of_property_read_u32 for about 600 times unnecessarily for omap4:

Add comments with pinconf & gpio range in the document of pinctrl-single.

Quoted 2 lines · Haojian Zhuang

Add comments with pinconf & gpio range in the document of pinctrl-single.

I'd tend to suggest separating the series to add GPIO range support and pinconf support, especially since didn't Tony suggest a separate driver for pinconf? Perhaps that was just for non-generic properties.

Quoted 2 lines · Haojian Zhuang

Add comments with pinconf & gpio range in the document of pinctrl-single.

I'd tend to suggest separating the series to add GPIO range support and pinconf support, especially since didn't Tony suggest a separate driver for pinconf? Perhaps that was just for non-generic properties.

Quoted 1 line

+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 30

Quoted 11 lines

+- pinctrl-single,gpio-ranges : gpio range list of phandles.

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +10 -0

+  Must be present if gpio range phandle is specified.
+  This property should be existing in .dtsi files for those silicons.
+
+- pinctrl-single,gpio : array with gpio range start, size & register
+  offset. Must be present if gpio range phandle is specified.
+  This property should be existing in .dts files for those boards.
+
+- pinctrl-single,gpio-func : gpio function value in the pinmux register.
+  Must be present if gpio range phandle is specified.
+  This property should be existing in .dts files for those boards.

I don't see any reason why pinctrl-single,gpio or pinctrl-single,gpio-func are board-specific rather than SoC-specific.

Surely it's the Soc HW construction that defines how the pinctrl and GPIO modules relate to each-other? I would simply remove the last sentence from each of the three descriptions above.

Also, isn't this list a list of properties for the main pinctrl-single node itself? I think you should split the list up as follows:

1) A description of the main node
2) A list of required and optional properties for the main node
3) A description of what a GPIO range node is
4) A list of required and optional properties for a GPIO range node.

I think that would explain the whole structure a lot more clearly.

Oh, and why not just get rid of the pinctrl-single,gpio-ranges property entirely, and simply make the GPIO range definitions be child nodes of the pinctrl-single node? That would require them to either be named according to some scheme or have some compatible property to differentiate them from any other nodes, but I think that's workable.

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Where 0xdc is the offset from the pinctrl register base address for the · line 85

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6 -0

@@ -42,6 +77,15 @@ Where 0xdc is the offset from the pinctrl register base address for the
 device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to
 be used when applying this change to the register.
 
+In case pinctrl device supports gpio function, it needs to define gpio range.
+All the phandles of gpio range list should be set in below:
+
+     pinctrl-single,gpio-ranges = <[phandle of gpio range]>;
+
+     [phandle of gpio range]: {

That's a label, not a phandle. The reference to the label gets compiled to a phandle in the DTB. The node name is missing. you should probably just use an example label and re-write the last 3 lines as:

        pinctrl-single,gpio-ranges = <&range0>;

        range0: range0 {

Dump pinmux register value, not only function part in the pinmux register.

Also fix the issue on caluclating pin offset. The last parameter should be pin number, not register offset.

Signed-off-by Haojian Zhuang <[email protected]>

---
 drivers/pinctrl/pinctrl-single.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pinctrl/pinctrl-single.c +4 -4

index 3b97b65..72017e7 100644
@@ -283,15 +283,15 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
 
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
-					unsigned offset)
+					unsigned pin)
 {
 	struct pcs_device *pcs;
-	unsigned val;
+	unsigned val, mux_bytes;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	val = pcs->read(pcs->base + offset);
-	val &= pcs->fmask;
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	val = pcs->read(pcs->base + pin * mux_bytes);
 
 	seq_printf(s, "%08x %s " , val, DRIVER_NAME);
 }

Signature · 1 line

1.7.10.4

Quoted 5 lines · Haojian Zhuang

Dump pinmux register value, not only function part in the pinmux register.

Also fix the issue on caluclating pin offset. The last parameter should be pin number, not register offset.

You have a minor typo ^^^^^^^^^^^ should be calculating
instead.

Is there a bug that should be fixed for the -rc cycle here? That's the impression I get from the description. Or is it just a cosmetic fix to rename offset to pin?

Regards,

Tony

Quoted 27 lines

Signed-off-by Haojian Zhuang <[email protected]>

---
 drivers/pinctrl/pinctrl-single.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pinctrl/pinctrl-single.c +4 -4

index 3b97b65..72017e7 100644
@@ -283,15 +283,15 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
 
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
                                      struct seq_file *s,
-                                     unsigned offset)
+                                     unsigned pin)
 {
      struct pcs_device *pcs;
-     unsigned val;
+     unsigned val, mux_bytes;
 
      pcs = pinctrl_dev_get_drvdata(pctldev);
 
-     val = pcs->read(pcs->base + offset);
-     val &= pcs->fmask;
+     mux_bytes = pcs->width / BITS_PER_BYTE;
+     val = pcs->read(pcs->base + pin * mux_bytes);
 

Quoted 5 lines

drivers/pinctrl/pinctrl-single.c +0 -1

      seq_printf(s, "%08x %s " , val, DRIVER_NAME);
 }
--

1.7.10.4

Quoted 16 lines · Tony Lindgren

Quoted 16 lines

static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
struct seq_file *s,

drivers/pinctrl/pinctrl-single.c +4 -4

-                                     unsigned offset)
+                                     unsigned pin)
 {
      struct pcs_device *pcs;
-     unsigned val;
+     unsigned val, mux_bytes;
 
      pcs = pinctrl_dev_get_drvdata(pctldev);
 
-     val = pcs->read(pcs->base + offset);
-     val &= pcs->fmask;
+     mux_bytes = pcs->width / BITS_PER_BYTE;
+     val = pcs->read(pcs->base + pin * mux_bytes);

Quoted 5 lines

Quoted 5 lines

      seq_printf(s, "%08x %s " , val, DRIVER_NAME);

}
--
1.7.10.4

There's also include a bug fix. I'm OK that this patch could be included in -rc cycle.