Re: [RFC PATCH 2/3] pinctrl: at91: add support for generic pinconf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 20:45 Mon 26 Aug     , boris brezillon wrote:
> Hello Jean-Christophe,
> 
> Le 26/08/2013 19:53, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> >On 23:37 Sat 24 Aug     , Boris BREZILLON wrote:
> >>Add support for generic pin configuration to pinctrl-at91 driver.
> >>
> >>Signed-off-by: Boris BREZILLON <b.brezillon@xxxxxxxxxxx>
> >>---
> >>  .../bindings/pinctrl/atmel,at91-pinctrl.txt        |   43 +++-
> >>  drivers/pinctrl/Kconfig                            |    2 +-
> >>  drivers/pinctrl/pinctrl-at91.c                     |  265 ++++++++++++++++++--
> >>  3 files changed, 289 insertions(+), 21 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >>index 7ccae49..7a7c0c4 100644
> >>--- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >>+++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> >>@@ -18,7 +18,9 @@ mode) this pin can work on and the 'config' configures various pad settings
> >>  such as pull-up, multi drive, etc.
> >>  Required properties for iomux controller:
> >>-- compatible: "atmel,at91rm9200-pinctrl"
> >>+- compatible: "atmel,at91rm9200-pinctrl" or "atmel,at91sam9x5-pinctrl".
> >>+  Add "generic-pinconf" to the compatible string list to use the generic pin
> >>+  configuration syntax.
> >>  - atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be
> >>    configured in this periph mode. All the periph and bank need to be describe.
> >>@@ -83,6 +85,11 @@ Required properties for pin configuration node:
> >>    setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
> >>    The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
> >>    PIN_BANK 0 is pioA, PIN_BANK 1 is pioB...
> >>+  Dependending on the presence of the "generic-pinconf" string in the
> >>+  compatible property the 4th cell is:
> >>+   * a phandle referencing a generic pin config node (refer to
> >>+     pinctrl-bindings.txt)
> >>+   * an integer defining the pin config (see the following description)
> >>  Bits used for CONFIG:
> >>  PULL_UP		(1 << 0): indicate this pin need a pull up.
> >>@@ -132,6 +139,40 @@ pinctrl@fffff400 {
> >>  	};
> >>  };
> >>+or
> >>+
> >>+pinctrl@fffff400 {
> >>+	#address-cells = <1>;
> >>+	#size-cells = <1>;
> >>+	ranges;
> >>+	compatible = "atmel,at91rm9200-pinctrl", "generic-pinconf", "simple-bus";
> >nack your break the backword compatibility
> >
> >if we use a old kernel with this new dt nothing will work
> >as the old kernel will never known the the "generic-pinconf" means anything
> 
> Your're right, I didn't think of this case (old kernel with new dt).
> 
> >if we want to use generic-pinconf support you *CAN NOT* use atmel,at91rm9200-pinctrl
> >in the compatible
> 
> What about using "atmel,at91xx-pinconf" instead of
> "atmel,at91xx-pinctrl" to notify
> the generic pinconf compatibility (as done by single pinctrl driver) ?
no as the rm9200 IP and sam9x5 IP are only partially compatible
you MUST distinguish them

> 
> >>+	reg = <0xfffff400 0x600>;
> >>+
> >>+	atmel,mux-mask = <
> >>+	      /*    A         B     */
> >>+	       0xffffffff 0xffc00c3b  /* pioA */
> >>+	       0xffffffff 0x7fff3ccf  /* pioB */
> >>+	       0xffffffff 0x007fffff  /* pioC */
> >>+	      >;
> >>+
> >>+	pcfg_none: pcfg_none {
> >>+		bias-disable;
> >>+	};
> >>+
> >>+	pcfg_pull_up: pcfg_pull_up {
> >>+		bias-pullup;
> >>+	};
> >>+
> >>+	/* shared pinctrl settings */
> >>+	dbgu {
> >>+		pinctrl_dbgu: dbgu-0 {
> >>+			atmel,pins =
> >>+				<1 14 0x1 &pcfg_none		/* PB14 periph A */
> >>+				 1 15 0x1 &pcfg_pull_up>;	/* PB15 periph A with pullup */
> >>+		};
> >>+	};
> >>+};
> >>+
> >>  dbgu: serial@fffff200 {
> >>  	compatible = "atmel,at91sam9260-usart";
> >>  	reg = <0xfffff200 0x200>;
> >>diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> >>index bdb1a87..55a4f2c 100644
> >>--- a/drivers/pinctrl/Kconfig
> >>+++ b/drivers/pinctrl/Kconfig
> >>@@ -54,7 +54,7 @@ config PINCTRL_AT91
> >>  	depends on OF
> >>  	depends on ARCH_AT91
> >>  	select PINMUX
> >>-	select PINCONF
> >>+	select GENERIC_PINCONF
> >>  	help
> >>  	  Say Y here to enable the at91 pinctrl driver
> >>diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> >>index 7cce066..1994dd2 100644
> >>--- a/drivers/pinctrl/pinctrl-at91.c
> >>+++ b/drivers/pinctrl/pinctrl-at91.c
> >>@@ -23,6 +23,7 @@
> >>  #include <linux/gpio.h>
> >>  #include <linux/pinctrl/machine.h>
> >>  #include <linux/pinctrl/pinconf.h>
> >>+#include <linux/pinctrl/pinconf-generic.h>
> >>  #include <linux/pinctrl/pinctrl.h>
> >>  #include <linux/pinctrl/pinmux.h>
> >>  /* Since we request GPIOs from ourself */
> >>@@ -32,6 +33,7 @@
> >>  #include <mach/at91_pio.h>
> >>  #include "core.h"
> >>+#include "pinconf.h"
> >>  #define MAX_NB_GPIO_PER_BANK	32
> >>@@ -85,6 +87,21 @@ enum at91_mux {
> >>  	AT91_MUX_PERIPH_D = 4,
> >>  };
> >>+struct at91_generic_pinconf {
> >>+	unsigned long	*configs;
> >>+	unsigned int	nconfigs;
> >>+};
> >>+
> >>+enum at91_pinconf_type {
> >>+	AT91_PINCONF_NATIVE,
> >>+	AT91_PINCONF_GENERIC,
> >>+};
> >>+
> >>+union at91_pinconf {
> >>+	unsigned long			native;
> >>+	struct at91_generic_pinconf	generic;
> >>+};
> >>+
> >>  /**
> >>   * struct at91_pmx_pin - describes an At91 pin mux
> >>   * @bank: the bank of the pin
> >>@@ -93,10 +110,11 @@ enum at91_mux {
> >>   * @conf: the configuration of the pin: PULL_UP, MULTIDRIVE etc...
> >>   */
> >>  struct at91_pmx_pin {
> >>-	uint32_t	bank;
> >>-	uint32_t	pin;
> >>-	enum at91_mux	mux;
> >>-	unsigned long	conf;
> >>+	uint32_t		bank;
> >>+	uint32_t		pin;
> >>+	enum at91_mux		mux;
> >>+	enum at91_pinconf_type	conftype;
> >>+	union at91_pinconf	conf;
> >>  };
> >>  /**
> >>@@ -278,8 +296,16 @@ static int at91_dt_node_to_map(struct pinctrl_dev *pctldev,
> >>  		new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> >>  		new_map[i].data.configs.group_or_pin =
> >>  				pin_get_name(pctldev, grp->pins[i]);
> >>-		new_map[i].data.configs.configs = &grp->pins_conf[i].conf;
> >>-		new_map[i].data.configs.num_configs = 1;
> >>+		if (!pctldev->desc->confops->is_generic) {
> >>+			new_map[i].data.configs.configs =
> >>+				&grp->pins_conf[i].conf.native;
> >>+			new_map[i].data.configs.num_configs = 1;
> >>+		} else {
> >>+			new_map[i].data.configs.configs =
> >>+				grp->pins_conf[i].conf.generic.configs;
> >>+			new_map[i].data.configs.num_configs =
> >>+				grp->pins_conf[i].conf.generic.nconfigs;
> >>+		}
> >>  	}
> >>  	dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
> >>@@ -325,7 +351,7 @@ static void at91_mux_disable_interrupt(void __iomem *pio, unsigned mask)
> >>  static unsigned at91_mux_get_pullup(void __iomem *pio, unsigned pin)
> >>  {
> >>-	return (readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1;
> >>+	return !((readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1);
> >>  }
> >>  static void at91_mux_set_pullup(void __iomem *pio, unsigned mask, bool on)
> >>@@ -407,6 +433,16 @@ static enum at91_mux at91_mux_get_periph(void __iomem *pio, unsigned mask)
> >>  	return select + 1;
> >>  }
> >>+static bool at91_mux_get_output(void __iomem *pio, unsigned pin)
> >>+{
> >>+	return (readl_relaxed(pio + PIO_OSR) >> pin) & 0x1;
> >>+}
> >>+
> >>+static void at91_mux_set_output(void __iomem *pio, unsigned mask, bool is_on)
> >>+{
> >>+	__raw_writel(mask, pio + (is_on ? PIO_OER : PIO_ODR));
> >>+}
> >>+
> >>  static bool at91_mux_get_deglitch(void __iomem *pio, unsigned pin)
> >>  {
> >>  	return (__raw_readl(pio + PIO_IFSR) >> pin) & 0x1;
> >>@@ -445,7 +481,7 @@ static void at91_mux_pio3_set_debounce(void __iomem *pio, unsigned mask,
> >>  static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned pin)
> >>  {
> >>-	return (__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1;
> >>+	return !((__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1);
> >>  }
> >>  static void at91_mux_pio3_set_pulldown(void __iomem *pio, unsigned mask, bool is_on)
> >>@@ -492,12 +528,17 @@ static struct at91_pinctrl_mux_ops at91sam9x5_ops = {
> >>  static void at91_pin_dbg(const struct device *dev, const struct at91_pmx_pin *pin)
> >>  {
> >>  	if (pin->mux) {
> >>-		dev_dbg(dev, "pio%c%d configured as periph%c with conf = 0x%lu\n",
> >>-			pin->bank + 'A', pin->pin, pin->mux - 1 + 'A', pin->conf);
> >>+		dev_dbg(dev, "pio%c%d configured as periph%c",
> >>+			pin->bank + 'A', pin->pin, pin->mux - 1 + 'A');
> >>  	} else {
> >>-		dev_dbg(dev, "pio%c%d configured as gpio with conf = 0x%lu\n",
> >>-			pin->bank + 'A', pin->pin, pin->conf);
> >>+		dev_dbg(dev, "pio%c%d configured as gpio",
> >>+			pin->bank + 'A', pin->pin);
> >>  	}
> >>+
> >>+	if (pin->conftype == AT91_PINCONF_NATIVE)
> >>+		dev_dbg(dev, " with conf = 0x%lu\n", pin->conf.native);
> >>+	else
> >>+		dev_dbg(dev, "\n");
> >do not change debug output
> 
> I do not change the debug output for the native pinconf binding, but
> I cannot print the config as
> a single interger in hex format if the generic pinconf is used.
> I must translate each config entry to a "property=value" string, or
> translate the generic config
> array to a single native config integer.
> 
> Do you have something easier in mind ?

no but I do not want to brake current automatic test tools

propose something with this in mind

Best Regards,
J.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux