[RFC 0/3] Adding support for NI Ettus Research USRP E3XX pinconf

[RFC 0/3] Adding support for NI Ettus Research USRP E3XX pinconf

[Moritz Fischer]

Hi all,

I've been hacking on this pinctrl driver to control pins on
our FPGA idle image to use different daughterboards using
different pin configurations.

I send this as an RFC because it's still pretty early stage,
and while it seems to work fine I'm unsure about some of the
semantics (as always, there will probably other changes required).

I'm planning to use this pinctrl driver to set pins to either
input (do nothing, default), or output with a value of (1 or 0).

Can I use the 'output-low', 'output-high' bindings to achieve this,
or am I supposed to implement a gpio controller to do this kind of stuff?

I'm not sure if I'm using the pinctrl framework correctly to achieve this,
any suggestions on how to change a pin from output to input, as the bindings
documentation explicitly states 'input-enable' does *not* affect output.

Thanks for your help & feedback,

Moritz

Moritz Fischer (3):
  Documentation: dt: Add devicetree bindings for NI USRP E3xx pinconf
  pinctrl: e3xx: Adding support for NI Ettus Research USRP E3xx pinconf
  ARM: e3xx: Add header file for pinctrl constants

 .../devicetree/bindings/pinctrl/pinctrl-e3xx.txt   |  27 ++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-e3xx.c                     | 403 +++++++++++++++++++++
 include/dt-bindings/pinctrl/pinctrl-e3xx.h         | 142 ++++++++
 5 files changed, 584 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-e3xx.txt
 create mode 100644 drivers/pinctrl/pinctrl-e3xx.c
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-e3xx.h

-- 
2.4.3

[RFC 1/3] Documentation: dt: Add devicetree bindings for NI USRP E3xx pinconf

[Moritz Fischer]

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-e3xx.txt   | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-e3xx.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-e3xx.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-e3xx.txt
new file mode 100644
index 0000000..2bfbd21
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-e3xx.txt
@@ -0,0 +1,27 @@
+USRP E3xx Pincontrol bindings
+
+The pins of the NI Ettus Research USRP E3xx idle image can be configured for different
+daughterboard configurations. This pinmux is implemented in an FPGA as soft core.
+
+Required properties:
+- compatible: Must be one of the following:
+  - "ettus,pinctrl-e3xx-1.0"
+- clocks: The clock driving the pinmux
+
+Example:
+
+	e3xx_pinctrl: e3xx-pinctrl@40200a00 {
+		compatible = "ettus,e3xx-pinctrl-1.0";
+		reg = <0x40200a00 0x1000>;
+		clocks = <&clkc 15>;
+
+		foo_state: pinconf {
+			conf {
+				pins = E31X_LED_RX1_RX;
+				output-low;
+			};
+		};
+	};
+
+Note: Constants that facilitate creation of devicetree files are available in
+      include/dt-bindings/pinctrl/pinctrl-e3xx.h
-- 
2.4.3

Re: [RFC 1/3] Documentation: dt: Add devicetree bindings for NI USRP E3xx pinconf

[Arnd Bergmann]

On Thursday 05 November 2015 15:41:21 Moritz Fischer wrote:
> +       e3xx_pinctrl: e3xx-pinctrl@40200a00 {

Just "pinctrl@40200a00"

> +               compatible = "ettus,e3xx-pinctrl-1.0";

no wildcards in the name, use exact chip revisions here. If two chips
use the same one, add a fallback to the older version in the dts file
of the newer one.

	Arnd

[RFC 2/3] pinctrl: e3xx: Adding support for NI Ettus Research USRP E3xx pinconf

[Moritz Fischer]

The USRP E3XX series requires pinctrl to configure the idle state
FPGA image for minimizing power consumption.
This is required since different daughtercards have different uses
for pins on a common connector.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/pinctrl/Kconfig        |  11 ++
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-e3xx.c | 403 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 415 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-e3xx.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index b422e4e..fb2e195 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -101,6 +101,17 @@ config PINCTRL_DIGICOLOR
 	select PINMUX
 	select GENERIC_PINCONF
 
+config PINCTRL_E3XX
+	bool "NI Ettus Research USRP E3xx pinctrl driver"
+	depends on OF
+	select PINCONF
+	select GENERIC_PINCONF
+	help
+	  Say Y here to support Ettus Research E3xx pinctrl driver, which is used for
+	  the NI Ettus Research USRP E3xx family of embedded SDRs, including E310
+	  and E330.
+	  This driver requires the pinctrl framework.
+
 config PINCTRL_LANTIQ
 	bool
 	depends on LANTIQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 738cb49..799f3ac 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PINCTRL_AT91)	+= pinctrl-at91.o
 obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
 obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
 obj-$(CONFIG_PINCTRL_DIGICOLOR)	+= pinctrl-digicolor.o
+obj-$(CONFIG_PINCTRL_E3XX)	+= pinctrl-e3xx.o
 obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
 obj-$(CONFIG_PINCTRL_MESON)	+= meson/
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
diff --git a/drivers/pinctrl/pinctrl-e3xx.c b/drivers/pinctrl/pinctrl-e3xx.c
new file mode 100644
index 0000000..55576fd
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-e3xx.c
@@ -0,0 +1,403 @@
+/*
+ * Copyright (c) 2015 National Instruments Corp.
+ *
+ * Pinctrl Driver for Ettus Research E3XX series daughterboards
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include "pinctrl-utils.h"
+#include "core.h"
+
+#define E3XX_NUM_DB_PINS 120
+#define E3XX_PINS_PER_REG 32
+
+#define E3XX_DDR_OFFSET 0x00
+#define E3XX_OUT_OFFSET 0x20
+
+static const struct pinctrl_pin_desc e3xx_pins[] = {
+	/* pin0 doesn't exist */
+	PINCTRL_PIN(1, "DB_1"),
+	PINCTRL_PIN(2, "DB_2"),
+	PINCTRL_PIN(3, "DB_3"),
+	PINCTRL_PIN(4, "DB_4"),
+	PINCTRL_PIN(5, "DB_5"),
+	PINCTRL_PIN(6, "DB_6"),
+	PINCTRL_PIN(7, "DB_7"),
+	PINCTRL_PIN(8, "DB_8"),
+	PINCTRL_PIN(9, "DB_9"),
+	PINCTRL_PIN(10, "DB_10"),
+	PINCTRL_PIN(11, "DB_11"),
+	PINCTRL_PIN(12, "DB_12"),
+	PINCTRL_PIN(13, "DB_13"),
+	PINCTRL_PIN(14, "DB_14"),
+	PINCTRL_PIN(15, "DB_15"),
+	PINCTRL_PIN(16, "DB_16"),
+	PINCTRL_PIN(17, "DB_17"),
+	PINCTRL_PIN(18, "DB_18"),
+	PINCTRL_PIN(19, "DB_19"),
+	PINCTRL_PIN(20, "DB_20"),
+	PINCTRL_PIN(21, "DB_21"),
+	PINCTRL_PIN(22, "DB_22"),
+	PINCTRL_PIN(23, "DB_23"),
+	PINCTRL_PIN(24, "DB_24"),
+	PINCTRL_PIN(25, "DB_25"),
+	PINCTRL_PIN(26, "DB_26"),
+	PINCTRL_PIN(27, "DB_27"),
+	PINCTRL_PIN(28, "DB_28"),
+	PINCTRL_PIN(29, "DB_29"),
+	PINCTRL_PIN(30, "DB_30"),
+	PINCTRL_PIN(31, "DB_31"),
+	PINCTRL_PIN(32, "DB_32"),
+	PINCTRL_PIN(33, "DB_33"),
+	PINCTRL_PIN(34, "DB_34"),
+	PINCTRL_PIN(35, "DB_35"),
+	PINCTRL_PIN(36, "DB_36"),
+	PINCTRL_PIN(37, "DB_37"),
+	PINCTRL_PIN(38, "DB_38"),
+	PINCTRL_PIN(39, "DB_39"),
+	PINCTRL_PIN(40, "DB_40"),
+	PINCTRL_PIN(41, "DB_41"),
+	PINCTRL_PIN(42, "DB_42"),
+	PINCTRL_PIN(43, "DB_43"),
+	PINCTRL_PIN(44, "DB_44"),
+	PINCTRL_PIN(45, "DB_45"),
+	PINCTRL_PIN(46, "DB_46"),
+	PINCTRL_PIN(47, "DB_47"),
+	PINCTRL_PIN(48, "DB_48"),
+	PINCTRL_PIN(49, "DB_49"),
+	PINCTRL_PIN(50, "DB_50"),
+	PINCTRL_PIN(52, "DB_52"),
+	PINCTRL_PIN(53, "DB_53"),
+	PINCTRL_PIN(54, "DB_54"),
+	PINCTRL_PIN(55, "DB_55"),
+	PINCTRL_PIN(56, "DB_56"),
+	PINCTRL_PIN(57, "DB_57"),
+	PINCTRL_PIN(58, "DB_58"),
+	PINCTRL_PIN(59, "DB_59"),
+	PINCTRL_PIN(60, "DB_60"),
+	PINCTRL_PIN(61, "DB_61"),
+	PINCTRL_PIN(62, "DB_62"),
+	PINCTRL_PIN(63, "DB_63"),
+	PINCTRL_PIN(64, "DB_64"),
+	PINCTRL_PIN(65, "DB_65"),
+	PINCTRL_PIN(66, "DB_66"),
+	PINCTRL_PIN(67, "DB_67"),
+	PINCTRL_PIN(68, "DB_68"),
+	PINCTRL_PIN(69, "DB_69"),
+	PINCTRL_PIN(70, "DB_70"),
+	PINCTRL_PIN(71, "DB_71"),
+	PINCTRL_PIN(72, "DB_72"),
+	PINCTRL_PIN(73, "DB_73"),
+	PINCTRL_PIN(74, "DB_74"),
+	PINCTRL_PIN(75, "DB_75"),
+	PINCTRL_PIN(76, "DB_76"),
+	PINCTRL_PIN(77, "DB_77"),
+	PINCTRL_PIN(78, "DB_78"),
+	PINCTRL_PIN(79, "DB_79"),
+	PINCTRL_PIN(80, "DB_80"),
+	PINCTRL_PIN(81, "DB_81"),
+	PINCTRL_PIN(82, "DB_82"),
+	PINCTRL_PIN(83, "DB_83"),
+	PINCTRL_PIN(84, "DB_84"),
+	PINCTRL_PIN(85, "DB_85"),
+	PINCTRL_PIN(86, "DB_86"),
+	PINCTRL_PIN(87, "DB_87"),
+	PINCTRL_PIN(88, "DB_88"),
+	PINCTRL_PIN(89, "DB_89"),
+	PINCTRL_PIN(90, "DB_90"),
+	PINCTRL_PIN(91, "DB_91"),
+	PINCTRL_PIN(92, "DB_92"),
+	PINCTRL_PIN(93, "DB_93"),
+	PINCTRL_PIN(94, "DB_94"),
+	PINCTRL_PIN(95, "DB_95"),
+	PINCTRL_PIN(96, "DB_96"),
+	PINCTRL_PIN(97, "DB_97"),
+	PINCTRL_PIN(98, "DB_98"),
+	PINCTRL_PIN(99, "DB_99"),
+	PINCTRL_PIN(100, "DB_100"),
+	PINCTRL_PIN(101, "DB_101"),
+	PINCTRL_PIN(102, "DB_102"),
+	PINCTRL_PIN(103, "DB_103"),
+	PINCTRL_PIN(104, "DB_104"),
+	PINCTRL_PIN(105, "DB_105"),
+	PINCTRL_PIN(106, "DB_106"),
+	PINCTRL_PIN(107, "DB_107"),
+	PINCTRL_PIN(108, "DB_108"),
+	PINCTRL_PIN(109, "DB_109"),
+	PINCTRL_PIN(110, "DB_110"),
+	PINCTRL_PIN(111, "DB_111"),
+	PINCTRL_PIN(112, "DB_112"),
+	PINCTRL_PIN(113, "DB_113"),
+	PINCTRL_PIN(114, "DB_114"),
+	PINCTRL_PIN(115, "DB_115"),
+	PINCTRL_PIN(116, "DB_116"),
+	PINCTRL_PIN(117, "DB_117"),
+	PINCTRL_PIN(118, "DB_118"),
+	PINCTRL_PIN(119, "DB_119"),
+	PINCTRL_PIN(120, "DB_120"),
+};
+
+struct e3xx_pinctrl {
+	struct clk *clk;
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+
+	void __iomem *io_base;
+
+	const struct e3xx_pinctrl_group *groups;
+};
+
+static inline void e3xx_pinctrl_write(struct e3xx_pinctrl *pctl, u32 offset,
+				      u32 val)
+{
+	writel_relaxed(val, pctl->io_base + offset);
+}
+
+static inline u32 e3xx_pinctrl_read(struct e3xx_pinctrl *pctl, u32 offset)
+{
+	return readl_relaxed(pctl->io_base + offset);
+}
+
+static int e3xx_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *e3xx_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+					       unsigned selector)
+{
+	return NULL;
+}
+
+static int e3xx_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+				       unsigned selector,
+				       const unsigned **pins,
+				       unsigned *num_pins)
+{
+	return -ENOTSUPP;
+}
+
+static const struct pinctrl_ops e3xx_pctrl_ops = {
+	.get_groups_count = e3xx_pinctrl_get_groups_count,
+	.get_group_name = e3xx_pinctrl_get_group_name,
+	.get_group_pins = e3xx_pinctrl_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinctrl_utils_dt_free_map,
+};
+
+static int e3xx_pinconf_cfg_get(struct pinctrl_dev *pctldev,
+				unsigned pin,
+				unsigned long *config)
+{
+	u32 reg, mask;
+	int arg;
+	struct e3xx_pinctrl *pctrl;
+	unsigned int param;
+
+	param = pinconf_to_config_param(*config);
+	pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (pin >= E3XX_NUM_DB_PINS)
+		return -ENOTSUPP;
+
+	mask = BIT(pin % E3XX_PINS_PER_REG);
+
+	switch (param) {
+	case PIN_CONFIG_OUTPUT:
+		clk_enable(pctrl->clk);
+		reg = e3xx_pinctrl_read(pctrl, E3XX_DDR_OFFSET +
+					(pin / E3XX_PINS_PER_REG) * 4);
+
+		clk_disable(pctrl->clk);
+		arg = !!(reg & mask);
+		break;
+	default:
+		dev_err(pctrl->dev, "requested illegal configuration");
+		return -ENOTSUPP;
+	};
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int e3xx_pinconf_cfg_set(struct pinctrl_dev *pctldev,
+				unsigned pin,
+				unsigned long *configs,
+				unsigned num_configs)
+{
+	u32 reg, mask;
+	int i;
+	struct e3xx_pinctrl *pctrl;
+	unsigned int param, arg;
+
+	if (pin >= E3XX_NUM_DB_PINS)
+		return -ENOTSUPP;
+	mask = BIT(pin % E3XX_PINS_PER_REG);
+
+	pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	clk_enable(pctrl->clk);
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_OUTPUT:
+			/* deal with value, set out bit if arg is 1 */
+			reg = e3xx_pinctrl_read(pctrl, E3XX_OUT_OFFSET +
+						((pin / E3XX_PINS_PER_REG) * 4))
+				;
+			reg &= ~mask;
+			if (arg)
+				reg |= mask;
+
+			/* addresses need to be 4 byte aligned */
+			e3xx_pinctrl_write(pctrl, E3XX_OUT_OFFSET +
+					   ((pin / E3XX_PINS_PER_REG) * 4), reg)
+				;
+
+			/* set ddr bit to high for output */
+			reg = e3xx_pinctrl_read(pctrl, E3XX_DDR_OFFSET +
+						((pin / E3XX_PINS_PER_REG) * 4))
+				;
+			reg |= mask;
+
+			/* addresses need to be 4 byte aligned */
+			e3xx_pinctrl_write(pctrl, E3XX_DDR_OFFSET +
+					  ((pin / E3XX_PINS_PER_REG) * 4), reg);
+			break;
+
+		default:
+			clk_disable(pctrl->clk);
+			return -ENOTSUPP;
+		};
+	}
+
+	clk_disable(pctrl->clk);
+
+	return 0;
+}
+
+static int e3xx_pinconf_group_set(struct pinctrl_dev *pctldev,
+				  unsigned selector,
+				  unsigned long *configs,
+				  unsigned num_configs)
+{
+	return -EAGAIN;
+}
+
+static const struct pinconf_ops e3xx_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = e3xx_pinconf_cfg_get,
+	.pin_config_set = e3xx_pinconf_cfg_set,
+	.pin_config_group_set = e3xx_pinconf_group_set,
+};
+
+static struct pinctrl_desc e3xx_desc = {
+	.name = "e3xx_pinctrl",
+	.pins = e3xx_pins,
+	.npins = ARRAY_SIZE(e3xx_pins),
+	.pctlops = &e3xx_pctrl_ops,
+	.confops = &e3xx_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static int e3xx_pinctrl_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct e3xx_pinctrl *pctrl;
+	int err;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+	pctrl->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "missing IO resource\n");
+		return -ENODEV;
+	}
+
+	pctrl->io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pctrl->io_base))
+		return PTR_ERR(pctrl->io_base);
+
+	pctrl->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pctrl->clk)) {
+		dev_err(&pdev->dev, "input clock not found");
+		return PTR_ERR(pctrl->clk);
+	}
+
+	err = clk_prepare(pctrl->clk);
+	if (err) {
+		dev_err(&pdev->dev, "unable to prepare clock");
+		return err;
+	}
+
+	pctrl->pctl = pinctrl_register(&e3xx_desc, &pdev->dev, pctrl);
+	if (!pctrl->pctl)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pctrl);
+
+	dev_info(&pdev->dev, "NI Ettus Research E3xx pinctrl initialized\n");
+
+	return 0;
+}
+
+static int e3xx_pinctrl_remove(struct platform_device *pdev)
+{
+	struct e3xx_pinctrl *pctrl = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(pctrl->pctl);
+
+	clk_unprepare(pctrl->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id e3xx_pinctrl_of_match[] = {
+	{ .compatible = "ettus,e3xx-pinctrl-1.0", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, e3xx_pinctrl_of_match);
+#endif
+
+static struct platform_driver e3xx_pinctrl_driver = {
+	.probe = e3xx_pinctrl_probe,
+	.remove = e3xx_pinctrl_remove,
+	.driver = {
+		.name = "e3xx_pinctrl",
+		.of_match_table = of_match_ptr(e3xx_pinctrl_of_match),
+	},
+};
+
+module_platform_driver(e3xx_pinctrl_driver);
+
+MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
+MODULE_DESCRIPTION("Ettus Research pinctrl driver");
+MODULE_LICENSE("GPL v2");
-- 
2.4.3

Re: [RFC 2/3] pinctrl: e3xx: Adding support for NI Ettus Research USRP E3xx pinconf

[Andy Shevchenko]

On Fri, Nov 6, 2015 at 1:41 AM, Moritz Fischer <moritz.fischer@ettus.com> wrote:
> The USRP E3XX series requires pinctrl to configure the idle state
> FPGA image for minimizing power consumption.
> This is required since different daughtercards have different uses
> for pins on a common connector.

> +#include <linux/pinctrl/pinconf-generic.h>

+ empty line?

> +#include "pinctrl-utils.h"
> +#include "core.h"

> +static int e3xx_pinconf_cfg_get(struct pinctrl_dev *pctldev,
> +                               unsigned pin,
> +                               unsigned long *config)
> +{
> +       u32 reg, mask;
> +       int arg;
> +       struct e3xx_pinctrl *pctrl;
> +       unsigned int param;
> +
> +       param = pinconf_to_config_param(*config);
> +       pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       if (pin >= E3XX_NUM_DB_PINS)
> +               return -ENOTSUPP;
> +
> +       mask = BIT(pin % E3XX_PINS_PER_REG);
> +
> +       switch (param) {
> +       case PIN_CONFIG_OUTPUT:
> +               clk_enable(pctrl->clk);
> +               reg = e3xx_pinctrl_read(pctrl, E3XX_DDR_OFFSET +
> +                                       (pin / E3XX_PINS_PER_REG) * 4);
> +
> +               clk_disable(pctrl->clk);
> +               arg = !!(reg & mask);

So, for now arg variable seems redundant, you may use expression
inside call below.

> +               break;
> +       default:
> +               dev_err(pctrl->dev, "requested illegal configuration");
> +               return -ENOTSUPP;
> +       };
> +
> +       *config = pinconf_to_config_packed(param, arg);
> +
> +       return 0;
> +}
> +
> +static int e3xx_pinconf_cfg_set(struct pinctrl_dev *pctldev,
> +                               unsigned pin,
> +                               unsigned long *configs,
> +                               unsigned num_configs)
> +{
> +       u32 reg, mask;
> +       int i;

unsigned

> +       struct e3xx_pinctrl *pctrl;
> +       unsigned int param, arg;
> +
> +       if (pin >= E3XX_NUM_DB_PINS)
> +               return -ENOTSUPP;
> +       mask = BIT(pin % E3XX_PINS_PER_REG);
> +
> +       pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       clk_enable(pctrl->clk);
> +
> +       for (i = 0; i < num_configs; i++) {
> +               param = pinconf_to_config_param(configs[i]);
> +               arg = pinconf_to_config_argument(configs[i]);
> +
> +               switch (param) {
> +               case PIN_CONFIG_OUTPUT:
> +                       /* deal with value, set out bit if arg is 1 */
> +                       reg = e3xx_pinctrl_read(pctrl, E3XX_OUT_OFFSET +
> +                                               ((pin / E3XX_PINS_PER_REG) * 4))
> +                               ;

Why not one the previous line?

> +                       reg &= ~mask;
> +                       if (arg)
> +                               reg |= mask;



> +
> +                       /* addresses need to be 4 byte aligned */
> +                       e3xx_pinctrl_write(pctrl, E3XX_OUT_OFFSET +
> +                                          ((pin / E3XX_PINS_PER_REG) * 4), reg)
> +                               ;
> +
> +                       /* set ddr bit to high for output */
> +                       reg = e3xx_pinctrl_read(pctrl, E3XX_DDR_OFFSET +
> +                                               ((pin / E3XX_PINS_PER_REG) * 4))
> +                               ;

Ditto.

> +                       reg |= mask;
> +
> +                       /* addresses need to be 4 byte aligned */
> +                       e3xx_pinctrl_write(pctrl, E3XX_DDR_OFFSET +
> +                                         ((pin / E3XX_PINS_PER_REG) * 4), reg);
> +                       break;
> +
> +               default:
> +                       clk_disable(pctrl->clk);
> +                       return -ENOTSUPP;
> +               };
> +       }
> +
> +       clk_disable(pctrl->clk);
> +
> +       return 0;
> +}
> +
> +static int e3xx_pinconf_group_set(struct pinctrl_dev *pctldev,
> +                                 unsigned selector,
> +                                 unsigned long *configs,
> +                                 unsigned num_configs)
> +{
> +       return -EAGAIN;
> +}
> +
> +static const struct pinconf_ops e3xx_pinconf_ops = {
> +       .is_generic = true,
> +       .pin_config_get = e3xx_pinconf_cfg_get,
> +       .pin_config_set = e3xx_pinconf_cfg_set,
> +       .pin_config_group_set = e3xx_pinconf_group_set,
> +};
> +
> +static struct pinctrl_desc e3xx_desc = {
> +       .name = "e3xx_pinctrl",
> +       .pins = e3xx_pins,
> +       .npins = ARRAY_SIZE(e3xx_pins),
> +       .pctlops = &e3xx_pctrl_ops,
> +       .confops = &e3xx_pinconf_ops,
> +       .owner = THIS_MODULE,
> +};
> +
> +static int e3xx_pinctrl_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct e3xx_pinctrl *pctrl;
> +       int err;
> +
> +       pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> +       if (!pctrl)
> +               return -ENOMEM;
> +       pctrl->dev = &pdev->dev;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +       if (!res) {
> +               dev_err(&pdev->dev, "missing IO resource\n");
> +               return -ENODEV;
> +       }

Whole condition is redundant.

> +
> +       pctrl->io_base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(pctrl->io_base))
> +               return PTR_ERR(pctrl->io_base);
> +
> +       pctrl->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(pctrl->clk)) {
> +               dev_err(&pdev->dev, "input clock not found");
> +               return PTR_ERR(pctrl->clk);
> +       }
> +
> +       err = clk_prepare(pctrl->clk);
> +       if (err) {
> +               dev_err(&pdev->dev, "unable to prepare clock");
> +               return err;
> +       }
> +
> +       pctrl->pctl = pinctrl_register(&e3xx_desc, &pdev->dev, pctrl);
> +       if (!pctrl->pctl)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, pctrl);
> +
> +       dev_info(&pdev->dev, "NI Ettus Research E3xx pinctrl initialized\n");
> +
> +       return 0;
> +}
> +
> +static int e3xx_pinctrl_remove(struct platform_device *pdev)
> +{
> +       struct e3xx_pinctrl *pctrl = platform_get_drvdata(pdev);
> +
> +       pinctrl_unregister(pctrl->pctl);
> +
> +       clk_unprepare(pctrl->clk);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id e3xx_pinctrl_of_match[] = {
> +       { .compatible = "ettus,e3xx-pinctrl-1.0", },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, e3xx_pinctrl_of_match);
> +#endif
> +
> +static struct platform_driver e3xx_pinctrl_driver = {
> +       .probe = e3xx_pinctrl_probe,
> +       .remove = e3xx_pinctrl_remove,
> +       .driver = {
> +               .name = "e3xx_pinctrl",
> +               .of_match_table = of_match_ptr(e3xx_pinctrl_of_match),
> +       },
> +};
> +
> +module_platform_driver(e3xx_pinctrl_driver);
> +
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Ettus Research pinctrl driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.4.3
>
> --
> 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/



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 2/3] pinctrl: e3xx: Adding support for NI Ettus Research USRP E3xx pinconf

[Linus Walleij]

On Fri, Nov 6, 2015 at 12:41 AM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:

> The USRP E3XX series requires pinctrl to configure the idle state
> FPGA image for minimizing power consumption.
> This is required since different daughtercards have different uses
> for pins on a common connector.
>
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
(...)

> +static const struct pinctrl_pin_desc e3xx_pins[] = {
> +       /* pin0 doesn't exist */
> +       PINCTRL_PIN(1, "DB_1"),
> +       PINCTRL_PIN(2, "DB_2"),
(...)
> +       PINCTRL_PIN(119, "DB_119"),
> +       PINCTRL_PIN(120, "DB_120"),
> +};

These should be the names on the data sheet for the balls/pins on the ASIC.
Is this the case? I saw some discussion with Arnd that indicated it was
rather rail names for a certain board and that is not OK.

> +static int e3xx_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +       return 0;
> +}
> +
> +static const char *e3xx_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
> +                                              unsigned selector)
> +{
> +       return NULL;
> +}
> +
> +static int e3xx_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
> +                                      unsigned selector,
> +                                      const unsigned **pins,
> +                                      unsigned *num_pins)
> +{
> +       return -ENOTSUPP;
> +}

Hm maybe we should make these group callbacks optional
in the pinctrl core?

> +static int e3xx_pinconf_cfg_set(struct pinctrl_dev *pctldev,
> +                               unsigned pin,
> +                               unsigned long *configs,
> +                               unsigned num_configs)
> +{
> +       u32 reg, mask;
> +       int i;
> +       struct e3xx_pinctrl *pctrl;
> +       unsigned int param, arg;
> +
> +       if (pin >= E3XX_NUM_DB_PINS)
> +               return -ENOTSUPP;
> +       mask = BIT(pin % E3XX_PINS_PER_REG);
> +
> +       pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       clk_enable(pctrl->clk);

Have you considered using pm_runtime_get_sync() etc with
callbacks instead of hammering clk_enable/disable all the
time? See
drivers/hwtracing/coresight/coresight-replicator.c
for an example of how to do it in the runtime PM ops
callbacks. It requires some tweaks (look closely at all setup
there) but it pays off.

> +static int e3xx_pinconf_group_set(struct pinctrl_dev *pctldev,
> +                                 unsigned selector,
> +                                 unsigned long *configs,
> +                                 unsigned num_configs)
> +{
> +       return -EAGAIN;
> +}

Maybe you should group this with the other group callbacks.

Apart from these remarks it looks pretty nice.

Yours,
Linus Walleij

Re: [RFC 2/3] pinctrl: e3xx: Adding support for NI Ettus Research USRP E3xx pinconf

[Moritz Fischer]

Linus,

Thanks for your review!

On Mon, Nov 9, 2015 at 2:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Nov 6, 2015 at 12:41 AM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>
>> The USRP E3XX series requires pinctrl to configure the idle state
>> FPGA image for minimizing power consumption.
>> This is required since different daughtercards have different uses
>> for pins on a common connector.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> (...)
>
>> +static const struct pinctrl_pin_desc e3xx_pins[] = {
>> +       /* pin0 doesn't exist */
>> +       PINCTRL_PIN(1, "DB_1"),
>> +       PINCTRL_PIN(2, "DB_2"),
> (...)
>> +       PINCTRL_PIN(119, "DB_119"),
>> +       PINCTRL_PIN(120, "DB_120"),
>> +};
>
> These should be the names on the data sheet for the balls/pins on the ASIC.
> Is this the case? I saw some discussion with Arnd that indicated it was
> rather rail names for a certain board and that is not OK.

Well so the chip is an FPGA, so in theory you can connect any logic
inside to (almost)
any ball on the outside (at design time). In our case we have a
daughter-board slot that gives
different meanings to what I call DB_1-DB_120. The pinctrl chip is a
soft-core in that FPGA.
Are you suggesting to use the balls of the FPGA as pin names instead?

>> +static int e3xx_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static const char *e3xx_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
>> +                                              unsigned selector)
>> +{
>> +       return NULL;
>> +}
>> +
>> +static int e3xx_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
>> +                                      unsigned selector,
>> +                                      const unsigned **pins,
>> +                                      unsigned *num_pins)
>> +{
>> +       return -ENOTSUPP;
>> +}
>
> Hm maybe we should make these group callbacks optional
> in the pinctrl core?

I'll submit a patchset separately, seems like a good idea.
>
>> +static int e3xx_pinconf_cfg_set(struct pinctrl_dev *pctldev,
>> +                               unsigned pin,daughterboard
>> +                               unsigned long *configs,
>> +                               unsigned num_configs)
>> +{
>> +       u32 reg, mask;
>> +       int i;
>> +       struct e3xx_pinctrl *pctrl;
>> +       unsigned int param, arg;
>> +
>> +       if (pin >= E3XX_NUM_DB_PINS)
>> +               return -ENOTSUPP;
>> +       mask = BIT(pin % E3XX_PINS_PER_REG);
>> +
>> +       pctrl = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +       clk_enable(pctrl->clk);
>
> Have you considered using pm_runtime_get_sync() etc with
> callbacks instead of hammering clk_enable/disable all the
> time? See
> drivers/hwtracing/coresight/coresight-replicator.c
> for an example of how to do it in the runtime PM ops
> callbacks. It requires some tweaks (look closely at all setup
> there) but it pays off.

Thanks for the pointer, I'll take a look.

>> +static int e3xx_pinconf_group_set(struct pinctrl_dev *pctldev,
>> +                                 unsigned selector,
>> +                                 unsigned long *configs,
>> +                                 unsigned num_configs)
>> +{
>> +       return -EAGAIN;
>> +}
>
> Maybe you should group this with the other group callbacks.

Will do.
>
> Apart from these remarks it looks pretty nice.

While testing I found some more (minor) issues, that I'll need to iron out
before I submit a real patch. In my cover letter I had a question about
how to deal with a situation where I'm exclusively either input or output,
Any suggestions on the correct semantics for expressing that?
The documentation seems to indicate that 'input-enable' *must not* influence
the output of a pin. How do I reconfigure a output to no longer be an output?

Cheers,

Moritz

[RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Moritz Fischer]

This new header file defines pincontrol constants for USRP E3xx to
use from E3xx's DTS file for pincontrol properties option.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 include/dt-bindings/pinctrl/pinctrl-e3xx.h | 142 +++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-e3xx.h

diff --git a/include/dt-bindings/pinctrl/pinctrl-e3xx.h b/include/dt-bindings/pinctrl/pinctrl-e3xx.h
new file mode 100644
index 0000000..7b35ad2
--- /dev/null
+++ b/include/dt-bindings/pinctrl/pinctrl-e3xx.h
@@ -0,0 +1,142 @@
+/*
+ * Copyright (C) 2015 National Instruments Corp
+ *
+ * This header provides constants for USRP E3xx pinctrl bindings
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+
+#ifndef _DT_BINDINGS_PINCTRL_E3XX_H
+#define _DT_BINDINGS_PINCTRL_E3XX_H
+
+/* Pin names for the E31x usecase */
+#define E31X_TX_BANDSEL_2	"DB_1"
+#define E31X_RX1B_BANDSEL_0	"DB_3"
+#define E31X_RX1B_BANDSEL_1	"DB_5"
+#define E31X_VCTXRX2_V2		"DB_7"
+#define E31X_TX_ENABLE1A	"DB_9"
+#define E31X_TX_ENABLE2A	"DB_11"
+#define E31X_TX_BANDSEL_0	"DB_12"
+#define E31X_TX_ENABLE1B	"DB_13"
+#define E31X_TX_ENABLE2B	"DB_15"
+#define E31X_RX1C_BANDSEL_0	"DB_17"
+#define E31X_RX1C_BANDSEL_1	"DB_19"
+#define E31X_VCTXRX2_V1		"DB_21"
+#define E31X_VCTXRX1_V2		"DB_23"
+#define E31X_VCTXRX1_V1		"DB_25"
+#define E31X_VCRX1_V1		"DB_27"
+#define E31X_VCRX1_V2		"DB_29"
+#define E31X_VCRX2_V1		"DB_31"
+#define E31X_VCRX2_V2		"DB_33"
+#define E31X_CAT_CTRL_IN2	"DB_35"
+#define E31X_CAT_CTRL_IN3	"DB_37"
+#define E31X_CAT_CTRL_OUT0	"DB_39"
+#define E31X_CAT_CTRL_OUT1	"DB_41"
+#define E31X_CAT_CTRL_OUT2	"DB_43"
+#define E31X_CAT_CTRL_OUT3	"DB_45"
+/* pin 47 is nc */
+/* pin 49 is nc */
+/* pin 51 is nc */
+/* pin 53 is nc */
+/* pin 55 is nc */
+/* pin 57 is nc */
+/* pin 59 is nc */
+/* pin 61 is nc */
+/* pin 63 is nc */
+/* pin 65 is nc */
+#define E31X_CAT_TXNRX		"DB_67"
+#define E31X_CAT_ENABLE		"DB_69"
+#define E31X_CAT_ENAGC		"DB_71"
+/* pin 73 is nc */
+#define E31X_CAT_P0_D5		"DB_75"
+#define E31X_CAT_P0_D7		"DB_77"
+#define E31X_CAT_P0_D3		"DB_79"
+#define E31X_CAT_P0_D9		"DB_81"
+#define E31X_CAT_P0_D1		"DB_83"
+#define E31X_CAT_P0_D6		"DB_85"
+#define E31X_CAT_P0_D0		"DB_87"
+#define E31X_CAT_P0_D2		"DB_89"
+#define E31X_CAT_P0_D4		"DB_91"
+#define E31X_CAT_P0_D11		"DB_93"
+#define E31X_CAT_P0_D10		"DB_95"
+#define E31X_CAT_P0_D8		"DB_97"
+#define E31X_CAT_RX_FRAME	"DB_99"
+#define E31X_CAT_DATA_CLK	"DB_101"
+/* pin 103 is nc */
+/* pin 105 is nc */
+#define E31X_RX2_BANDSEL_2	"DB_107"
+#define E31X_RX2_BANDSEL_1	"DB_109"
+#define E31X_RX2_BANDSEL_0	"DB_111"
+#define E31X_RX2C_BANDSEL_1	"DB_113"
+#define E31X_RX2C_BANDSEL_0	"DB_115"
+#define E31X_RX2B_BANDSEL_0	"DB_117"
+#define E31X_RX2B_BANDSEL_1	"DB_119"
+
+/* pin 2 is nc */
+/* pin 4 is nc */
+#define E31X_RX1_BANDSEL_0	"DB_6"
+#define E31X_RX1_BANDSEL_1	"DB_8"
+#define E31X_RX1_BANDSEL_2	"DB_10"
+#define E31X_TX_BANDSEL_1	"DB_14"
+#define E31X_DB_SCL		"DB_16"
+#define E31X_DB_SCA		"DB_18"
+#define E31X_TUNE_DAC_SYNC_N	"DB_20"
+#define E31X_TUNE_DAC_SCLK	"DB_22"
+#define E31X_TUNE_DAC_SDIN	"DB_24"
+/* pin 26 is nc */
+/* pin 28 is nc */
+#define E31X_VCTCXO_TO_MB	"DB_30"
+/* pin 32 is nc */
+/* pin 34 is nc */
+/* pin 36 is nc */
+/* pin 38 is nc */
+#define E31X_CAT_CTRL_OUT4	"DB_40"
+#define E31X_CAT_CTRL_OUT5	"DB_42"
+#define E31X_CAT_CTRL_OUT6	"DB_44"
+#define E31X_CAT_CTRL_OUT7	"DB_46"
+#define E31X_CAT_RESET		"DB_48"
+#define E31X_CAT_CS		"DB_50"
+#define E31X_CAT_SCLK		"DB_52"
+#define E31X_CAT_MOSI		"DB_54"
+#define E31X_CAT_MISO		"DB_56"
+#define E31X_CAT_CTRL_IN0	"DB_58"
+#define E31X_CAT_CTRL_IN1	"DB_60"
+/* pin 62 is nc */
+/* pin 64 is nc */
+/* pin 66 is nc */
+/* pin 68 is nc */
+#define E31X_CAT_BBCLK_OUT	"DB_70"
+/* pin 72 is nc */
+#define E31X_CAT_SYNC		"DB_74"
+#define E31X_CAT_P1_D11		"DB_78"
+#define E31X_CAT_P1_D1		"DB_80"
+#define E31X_CAT_P1_D3		"DB_82"
+#define E31X_CAT_P1_D0		"DB_84"
+#define E31X_CAT_P1_D5		"DB_86"
+#define E31X_CAT_P1_D2		"DB_88"
+#define E31X_CAT_P1_D4		"DB_90"
+#define E31X_CAT_P1_D7		"DB_92"
+#define E31X_CAT_P1_D6		"DB_94"
+#define E31X_CAT_P1_D9		"DB_96"
+#define E31X_CAT_P1_D8		"DB_98"
+#define E31X_CAT_P1_D10		"DB_100"
+#define E31X_CAT_TX_FRAME	"DB_102"
+#define E31X_CAT_FB_CLK		"DB_104"
+/* pin 106 is nc */
+#define E31X_LED_TXRX1_TX	"DB_108"
+#define E31X_LED_TXRX1_RX	"DB_110"
+#define E31X_LED_RX1_RX		"DB_112"
+#define E31X_LED_TXRX2_TX	"DB_114"
+#define E31X_LED_TXRX2_RX	"DB_116"
+#define E31X_LED_RX2_RX		"DB_118"
+/* pin 120 is nc */
+
+#endif /* _DT_BINDINGS_PINCTRL_E3XX_H */
-- 
2.4.3

Re: [RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Arnd Bergmann]

On Thursday 05 November 2015 15:41:23 Moritz Fischer wrote:
> +/* Pin names for the E31x usecase */
> +#define E31X_TX_BANDSEL_2      "DB_1"
> +#define E31X_RX1B_BANDSEL_0    "DB_3"
> +#define E31X_RX1B_BANDSEL_1    "DB_5"
> +#define E31X_VCTXRX2_V2                "DB_7"
> +#define E31X_TX_ENABLE1A       "DB_9"
> +#define E31X_TX_ENABLE2A       "DB_11"
> +#define E31X_TX_BANDSEL_0      "DB_12"

Why not put the strings directly into the .dts files and change the
lookup table in the driver accordingly:

+static const struct pinctrl_pin_desc e3xx_pins[] = {
+       /* pin0 doesn't exist */
+       PINCTRL_PIN(1, "TX_BANDSEL_2"),
+       PINCTRL_PIN(3, "RX1B_BANDSEL_0"),
+       PINCTRL_PIN(5, "RX1B_BANDSEL_1"),
+       PINCTRL_PIN(7, "VCTXRX2_V2"),

That would save you the hassle of the three-way dependency between the
dts file, the driver and the header when you want to change something
going through three different maintainer trees.

	arnd

Re: [RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Moritz Fischer]

Hi Arnd,

On Fri, Nov 6, 2015 at 12:54 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 05 November 2015 15:41:23 Moritz Fischer wrote:
>> +/* Pin names for the E31x usecase */
>> +#define E31X_TX_BANDSEL_2      "DB_1"
>> +#define E31X_RX1B_BANDSEL_0    "DB_3"
>> +#define E31X_RX1B_BANDSEL_1    "DB_5"
>> +#define E31X_VCTXRX2_V2                "DB_7"
>> +#define E31X_TX_ENABLE1A       "DB_9"
>> +#define E31X_TX_ENABLE2A       "DB_11"
>> +#define E31X_TX_BANDSEL_0      "DB_12"
>
> Why not put the strings directly into the .dts files and change the
> lookup table in the driver accordingly:
>
> +static const struct pinctrl_pin_desc e3xx_pins[] = {
> +       /* pin0 doesn't exist */
> +       PINCTRL_PIN(1, "TX_BANDSEL_2"),
> +       PINCTRL_PIN(3, "RX1B_BANDSEL_0"),
> +       PINCTRL_PIN(5, "RX1B_BANDSEL_1"),
> +       PINCTRL_PIN(7, "VCTXRX2_V2"),

That's actually the way I initially had it, however the pin names
literally changed
in the schematic depending on which daughter-board you stick into the slot.
So my plan was to add the #defines for the second daughter-board in a
follow up patch
once the pin assignment is final. This would allow me to have something like:

pins = E31X_TX_BANDSEL_2;
output-low;

in one .dts, while having something like

pins = E33X_TX_BANDSEL_2
output low;

in the second dts

The other option would have been to stick a e31x_pins and a e33x_pins
into the driver,
and set them according to a compatible string or "ettus,daughterboard"
property on probe.

Does that make sense, or do you think there's a cleaner / better way
to achieve this sort of behavior?

Thanks for your feedback!

Cheers,
Moritz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Arnd Bergmann]

On Friday 06 November 2015 08:01:25 Moritz Fischer wrote:
> Hi Arnd,
> 
> On Fri, Nov 6, 2015 at 12:54 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 05 November 2015 15:41:23 Moritz Fischer wrote:
> >> +/* Pin names for the E31x usecase */
> >> +#define E31X_TX_BANDSEL_2      "DB_1"
> >> +#define E31X_RX1B_BANDSEL_0    "DB_3"
> >> +#define E31X_RX1B_BANDSEL_1    "DB_5"
> >> +#define E31X_VCTXRX2_V2                "DB_7"
> >> +#define E31X_TX_ENABLE1A       "DB_9"
> >> +#define E31X_TX_ENABLE2A       "DB_11"
> >> +#define E31X_TX_BANDSEL_0      "DB_12"
> >
> > Why not put the strings directly into the .dts files and change the
> > lookup table in the driver accordingly:
> >
> > +static const struct pinctrl_pin_desc e3xx_pins[] = {
> > +       /* pin0 doesn't exist */
> > +       PINCTRL_PIN(1, "TX_BANDSEL_2"),
> > +       PINCTRL_PIN(3, "RX1B_BANDSEL_0"),
> > +       PINCTRL_PIN(5, "RX1B_BANDSEL_1"),
> > +       PINCTRL_PIN(7, "VCTXRX2_V2"),
> 
> That's actually the way I initially had it, however the pin names
> literally changed
> in the schematic depending on which daughter-board you stick into the slot.
> So my plan was to add the #defines for the second daughter-board in a
> follow up patch
> once the pin assignment is final. This would allow me to have something like:
> 
> pins = E31X_TX_BANDSEL_2;
> output-low;
> 
> in one .dts, while having something like
> 
> pins = E33X_TX_BANDSEL_2
> output low;
> 
> in the second dts

That doesn't seem helpful though, because the assignment of the
pins is not hidden from the source file that should be documenting
it: if someone wants to know how the pins are muxed, they now have
to cross-reference the dts file with the header. If you just put the
pin number directly into the dts, it becomes completely clear
what the setting is.

> The other option would have been to stick a e31x_pins and a e33x_pins
> into the driver,
> and set them according to a compatible string or "ettus,daughterboard"
> property on probe.
> 
> Does that make sense, or do you think there's a cleaner / better way
> to achieve this sort of behavior?

The driver should not be board specific.

	Arnd

Re: [RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Moritz Fischer]

On Fri, Nov 6, 2015 at 8:42 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 06 November 2015 08:01:25 Moritz Fischer wrote:
>> Hi Arnd,
>>
>> On Fri, Nov 6, 2015 at 12:54 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Thursday 05 November 2015 15:41:23 Moritz Fischer wrote:
>> >> +/* Pin names for the E31x usecase */
>> >> +#define E31X_TX_BANDSEL_2      "DB_1"
>> >> +#define E31X_RX1B_BANDSEL_0    "DB_3"
>> >> +#define E31X_RX1B_BANDSEL_1    "DB_5"
>> >> +#define E31X_VCTXRX2_V2                "DB_7"
>> >> +#define E31X_TX_ENABLE1A       "DB_9"
>> >> +#define E31X_TX_ENABLE2A       "DB_11"
>> >> +#define E31X_TX_BANDSEL_0      "DB_12"
>> >
>> > Why not put the strings directly into the .dts files and change the
>> > lookup table in the driver accordingly:
>> >
>> > +static const struct pinctrl_pin_desc e3xx_pins[] = {
>> > +       /* pin0 doesn't exist */
>> > +       PINCTRL_PIN(1, "TX_BANDSEL_2"),
>> > +       PINCTRL_PIN(3, "RX1B_BANDSEL_0"),
>> > +       PINCTRL_PIN(5, "RX1B_BANDSEL_1"),
>> > +       PINCTRL_PIN(7, "VCTXRX2_V2"),
>>
>> That's actually the way I initially had it, however the pin names
>> literally changed
>> in the schematic depending on which daughter-board you stick into the slot.
>> So my plan was to add the #defines for the second daughter-board in a
>> follow up patch
>> once the pin assignment is final. This would allow me to have something like:
>>
>> pins = E31X_TX_BANDSEL_2;
>> output-low;
>>
>> in one .dts, while having something like
>>
>> pins = E33X_TX_BANDSEL_2
>> output low;
>>
>> in the second dts
>
> That doesn't seem helpful though, because the assignment of the
> pins is not hidden from the source file that should be documenting
> it: if someone wants to know how the pins are muxed, they now have
> to cross-reference the dts file with the header. If you just put the
> pin number directly into the dts, it becomes completely clear
> what the setting is.

Just to clarify, you're suggesting using "DB_0" etc in the dts file?
At that point
the name becomes kinda useless. I found the PINCTRL_PIN_ANON macro,
but haven't found any use throughout the tree, is using it discouraged?

>> The other option would have been to stick a e31x_pins and a e33x_pins
>> into the driver,
>> and set them according to a compatible string or "ettus,daughterboard"
>> property on probe.
>>
>> Does that make sense, or do you think there's a cleaner / better way
>> to achieve this sort of behavior?
>
> The driver should not be board specific.

Agreed, thank you!

Cheers,

Moritz

Re: [RFC 3/3] ARM: e3xx: Add header file for pinctrl constants

[Arnd Bergmann]

On Friday 06 November 2015 09:55:28 Moritz Fischer wrote:
> On Fri, Nov 6, 2015 at 8:42 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 06 November 2015 08:01:25 Moritz Fischer wrote:
> >> pins = E31X_TX_BANDSEL_2;
> >> output-low;
> >>
> >> in one .dts, while having something like
> >>
> >> pins = E33X_TX_BANDSEL_2
> >> output low;
> >>
> >> in the second dts
> >
> > That doesn't seem helpful though, because the assignment of the
> > pins is not hidden from the source file that should be documenting
> > it: if someone wants to know how the pins are muxed, they now have
> > to cross-reference the dts file with the header. If you just put the
> > pin number directly into the dts, it becomes completely clear
> > what the setting is.
> 
> Just to clarify, you're suggesting using "DB_0" etc in the dts file?
> At that point
> the name becomes kinda useless. I found the PINCTRL_PIN_ANON macro,
> but haven't found any use throughout the tree, is using it discouraged?

I don't know if that is the best approach, but it's better than using
macros here. You should only add a header file with macros if you
absolutely have to define a common namespace between the driver and the
dts files, but you don't do that here: the header file is only used
in one place.

	Arnd

Re: [RFC 0/3] Adding support for NI Ettus Research USRP E3XX pinconf

[Linus Walleij]

On Fri, Nov 6, 2015 at 12:41 AM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:

> I'm planning to use this pinctrl driver to set pins to either
> input (do nothing, default), or output with a value of (1 or 0).
>
> Can I use the 'output-low', 'output-high' bindings to achieve this,
> or am I supposed to implement a gpio controller to do this kind of stuff?

I'm pretty sure you should implement a GPIO chip for this.
output-low and output-high are for things like lines going to
a RAM memory that need to be set up as part of a pin control
state.

> I'm not sure if I'm using the pinctrl framework correctly to achieve this,
> any suggestions on how to change a pin from output to input, as the bindings
> documentation explicitly states 'input-enable' does *not* affect output.

Look at pin controllers also implementing GPIO chips.

Yours,
Linus Walleij