[PATCH v4 0/2] pinctrl: Add RZ/A2 pin and gpio driver

[PATCH v4 0/2] pinctrl: Add RZ/A2 pin and gpio driver

[Chris Brandt]

The pin controller in the RZ/A2 is nothing like the pin controller in
the RZ/A1. That's a good thing! This pin controller is much more simple
and easier to configure.

So, this driver is faily simple (I hope).


  V3 -> V4: Sorry...I forgot Rob's Reviewed-by !!


Chris Brandt (2):
  pinctrl: Add RZ/A2 pin and gpio controller
  dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      |  88 ++++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-rza2.c                     | 530 +++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      |  47 ++
 5 files changed, 677 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rza2.c
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

-- 
2.16.1

[PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v3:
 - Changed names from Px to PORTx because "PC" is already defined
v2:
 - fixed SOC part number in comments
 - sorted #includes
 - removed spaces in pfc_pin_port_name enum
 - put RZA2_NPORTS at the end of pfc_pin_port_name enum
 - added RZA2_ to the beginning of all #define macros
 - put ( ) around all passed arguments in #define macros
 - made helper macros to get register address easier
 - use defines for pin direction bit settings
---
 drivers/pinctrl/Kconfig        |  11 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-rza2.c | 530 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 542 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rza2.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 4d8c00eac742..3e4d890d4bd9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,17 @@ config PINCTRL_RZA1
 	help
 	  This selects pinctrl driver for Renesas RZ/A1 platforms.
 
+config PINCTRL_RZA2
+	bool "Renesas RZ/A2 gpio and pinctrl driver"
+	depends on OF
+	depends on ARCH_R7S9210 || COMPILE_TEST
+	select GPIOLIB
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas RZ/A2 platforms.
+
 config PINCTRL_RZN1
 	bool "Renesas RZ/N1 pinctrl driver"
 	depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 18a13c1e2c21..712184b74a5c 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
+obj-$(CONFIG_PINCTRL_RZA2)	+= pinctrl-rza2.o
 obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
diff --git a/drivers/pinctrl/pinctrl-rza2.c b/drivers/pinctrl/pinctrl-rza2.c
new file mode 100644
index 000000000000..3781c3f693e8
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rza2.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC
+ *
+ * Copyright (C) 2018 Chris Brandt
+ */
+
+/*
+ * This pin controller/gpio combined driver supports Renesas devices of RZ/A2
+ * family.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+#define DRIVER_NAME		"pinctrl-rza2"
+
+#define RZA2_PINS_PER_PORT	8
+#define RZA2_NPINS		(RZA2_PINS_PER_PORT * RZA2_NPORTS)
+#define RZA2_PIN_ID_TO_PORT(id)	((id) / RZA2_PINS_PER_PORT)
+#define RZA2_PIN_ID_TO_PIN(id)	((id) % RZA2_PINS_PER_PORT)
+
+/*
+ * Use 16 lower bits [15:0] for pin identifier
+ * Use 16 higher bits [31:16] for pin mux function
+ */
+#define MUX_PIN_ID_MASK		GENMASK(15, 0)
+#define MUX_FUNC_MASK		GENMASK(31, 16)
+#define MUX_FUNC_OFFS		16
+#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
+
+enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7,
+			PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF,
+			PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS};
+static const char port_names[] = "0123456789ABCDEFGHJKLM";
+
+struct rza2_pinctrl_priv {
+	struct device *dev;
+	void __iomem *base;
+
+	struct pinctrl_pin_desc *pins;
+	struct pinctrl_desc desc;
+	struct pinctrl_dev *pctl;
+};
+
+#define RZA2_PDR_BASE(_b)	((_b) + 0x0000)	/* 16-bit, 2 bytes apart */
+#define RZA2_PODR_BASE(_b)	((_b) + 0x0040)	/* 8-bit, 1 byte apart */
+#define RZA2_PIDR_BASE(_b)	((_b) + 0x0060)	/* 8-bit, 1 byte apart */
+#define RZA2_PMR_BASE(_b)	((_b) + 0x0080)	/* 8-bit, 1 byte apart */
+#define RZA2_DSCR_BASE(_b)	((_b) + 0x0140)	/* 16-bit, 2 bytes apart */
+#define RZA2_PFS_BASE(_b)	((_b) + 0x0200)	/* 8-bit, 8 bytes apart */
+#define RZA2_PWPR(_b)		((_b) + 0x02FF)	/* 8-bit */
+#define RZA2_PFENET(_b)		((_b) + 0x0820)	/* 8-bit */
+#define RZA2_PPOC(_b)		((_b) + 0x0900)	/* 32-bit */
+#define RZA2_PHMOMO(_b)		((_b) + 0x0980)	/* 32-bit */
+#define RZA2_PCKIO(_b)		((_b) + 0x09D0)	/* 8-bit */
+
+#define RZA2_PDR(_b, _port)		(RZA2_PDR_BASE(_b) + ((_port) * 2))
+#define RZA2_PODR(_b, _port)		(RZA2_PODR_BASE(_b) + (_port))
+#define RZA2_PIDR(_b, _port)		(RZA2_PIDR_BASE(_b) + (_port))
+#define RZA2_PMR(_b, _port)		(RZA2_PMR_BASE(_b) + (_port))
+#define RZA2_DSCR(_b, _port)		(RZA2_DSCR_BASE(_b) + ((_port) * 2))
+#define RZA2_PFS(_b, _port, _pin)	(RZA2_PFS_BASE(_b) + ((_port) * 8) \
+					+ (_pin))
+#define RZA2_PDR_HIGHZ		0x00
+#define RZA2_PDR_INPUT		0x02
+#define RZA2_PDR_OUTPUT		0x03
+#define RZA2_PDR_MASK		0x03
+
+static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin,
+				  u8 func)
+{
+	u8 reg8;
+	u16 reg16;
+	u16 mask16;
+
+	/* Set pin to 'Non-use (Hi-z input protection)'  */
+	reg16 = readw(RZA2_PDR(pfc_base, port));
+	mask16 = 0x03 << (pin * 2);
+	reg16 &= ~mask16;
+	writew(reg16, RZA2_PDR(pfc_base, port));
+
+	/* Temporarily switch to GPIO */
+	reg8 = readb(RZA2_PMR(pfc_base, port));
+	reg8 &= ~BIT(pin);
+	writeb(reg8, RZA2_PMR(pfc_base, port));
+
+	/* PFS Register Write Protect : OFF */
+	writeb(0x00, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
+	writeb(0x40, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=1 */
+
+	/* Set Pin function (interrupt disabled, ISEL=0) */
+	writeb(func, RZA2_PFS(pfc_base, port, pin));
+
+	/* PFS Register Write Protect : ON */
+	writeb(0x00, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
+	writeb(0x80, RZA2_PWPR(pfc_base));	/* B0WI=1, PFSWE=0 */
+
+	/* Port Mode  : Peripheral module pin functions */
+	reg8 = readb(RZA2_PMR(pfc_base, port));
+	reg8 |= BIT(pin);
+	writeb(reg8, RZA2_PMR(pfc_base, port));
+}
+
+static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir)
+{
+	u16 reg16;
+	u16 mask16;
+
+	reg16 = readw(RZA2_PDR(pfc_base, port));
+	mask16 = RZA2_PDR_MASK << (pin * 2);
+	reg16 &= ~mask16;
+
+	if (dir == GPIOF_DIR_IN)
+		reg16 |= RZA2_PDR_INPUT << (pin * 2);	/* pin as input */
+	else
+		reg16 |= RZA2_PDR_OUTPUT << (pin * 2);	/* pin as output */
+
+	writew(reg16, RZA2_PDR(pfc_base, port));
+}
+
+static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+	u16 reg16;
+
+	reg16 = readw(RZA2_PDR(priv->base, port));
+	reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK;
+
+	if (reg16 == RZA2_PDR_OUTPUT)
+		return GPIOF_DIR_OUT;
+
+	if (reg16 == RZA2_PDR_INPUT)
+		return GPIOF_DIR_IN;
+
+	/*
+	 * This GPIO controller has a default Hi-Z state that is not input or
+	 * output, so force the pin to input now.
+	 */
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
+
+	return GPIOF_DIR_IN;
+}
+
+static int rza2_chip_direction_input(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
+
+	return 0;
+}
+
+static int rza2_chip_direction_output(struct gpio_chip *chip,
+				      unsigned int offset, int val)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT);
+
+	return 0;
+}
+
+static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1;
+}
+
+static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset,
+			  int value)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+	u8 new_value = readb(RZA2_PODR(priv->base, port));
+
+	if (value)
+		new_value |= (1 << pin);
+	else
+		new_value &= ~(1 << pin);
+
+	writeb(new_value, RZA2_PODR(priv->base, port));
+}
+
+static const char * const rza2_gpio_names[] = {
+	"P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
+	"P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
+	"P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
+	"P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
+	"P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
+	"P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
+	"P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
+	"P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
+	"P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
+	"P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
+	"PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
+	"PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
+	"PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
+	"PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
+	"PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
+	"PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
+	"PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
+	"PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
+	/* port I does not exist */
+	"PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
+	"PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
+	"PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
+	"PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
+};
+
+static struct gpio_chip chip = {
+	.names = rza2_gpio_names,
+	.base = -1,
+	.ngpio = RZA2_NPINS,
+	.get_direction = rza2_chip_get_direction,
+	.direction_input = rza2_chip_direction_input,
+	.direction_output = rza2_chip_direction_output,
+	.get = rza2_chip_get,
+	.set = rza2_chip_set,
+};
+
+struct pinctrl_gpio_range gpio_range;
+
+static int rza2_gpio_register(struct rza2_pinctrl_priv *priv)
+{
+	struct device_node *np = priv->dev->of_node;
+	struct of_phandle_args of_args;
+	int ret;
+
+	if (!of_property_read_bool(np, "gpio-controller")) {
+		dev_info(priv->dev, "No gpio controller registered\n");
+		return 0;
+	}
+
+	chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np);
+	chip.of_node = np;
+	chip.parent = priv->dev;
+
+	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
+					       &of_args);
+	if (ret) {
+		dev_err(priv->dev, "Unable to parse gpio-ranges\n");
+		return ret;
+	}
+
+	gpio_range.id = of_args.args[0];
+	gpio_range.name = chip.label;
+	gpio_range.pin_base = gpio_range.base = of_args.args[1];
+	gpio_range.npins = of_args.args[2];
+	gpio_range.gc = &chip;
+
+	/* Register our gpio chip with gpiolib */
+	ret = devm_gpiochip_add_data(priv->dev, &chip, priv);
+	if (ret)
+		return ret;
+
+	/* Register pin range with pinctrl core */
+	pinctrl_add_gpio_range(priv->pctl, &gpio_range);
+
+	dev_dbg(priv->dev, "Registered gpio controller\n");
+
+	return 0;
+}
+
+static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv)
+{
+	struct pinctrl_pin_desc *pins;
+	unsigned int i;
+	int ret;
+
+	pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	priv->pins = pins;
+	priv->desc.pins = pins;
+	priv->desc.npins = RZA2_NPINS;
+
+	for (i = 0; i < RZA2_NPINS; i++) {
+		unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
+		unsigned int port = RZA2_PIN_ID_TO_PORT(i);
+
+		pins[i].number = i;
+		pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u",
+					      port_names[port], pin);
+	}
+
+	ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv,
+					     &priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "pinctrl registration failed\n");
+		return ret;
+	}
+
+	ret = pinctrl_enable(priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "pinctrl enable failed\n");
+		return ret;
+	}
+
+	ret = rza2_gpio_register(priv);
+	if (ret) {
+		dev_err(priv->dev, "GPIO registration failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * For each DT node, create a single pin mapping. That pin mapping will only
+ * contain a single group of pins, and that group of pins will only have a
+ * single function that can be selected.
+ */
+static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct property *of_pins;
+	int i;
+	unsigned int *pins;
+	unsigned int *psel_val;
+	const char **pin_fn;
+	int ret, npins;
+	int gsel, fsel;
+
+	/* Find out how many pins to map */
+	of_pins = of_find_property(np, "pinmux", NULL);
+	if (!of_pins) {
+		dev_info(priv->dev, "Missing pinmux property\n");
+		return -ENOENT;
+	}
+	npins = of_pins->length / sizeof(u32);
+
+	pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL);
+	psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val),
+				GFP_KERNEL);
+	pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL);
+	if (!pins || !psel_val || !pin_fn)
+		return -ENOMEM;
+
+	/* Collect pin locations and mux settings from DT properties */
+	for (i = 0; i < npins; ++i) {
+		u32 value;
+
+		ret = of_property_read_u32_index(np, "pinmux", i, &value);
+		if (ret)
+			return ret;
+		pins[i] = value & MUX_PIN_ID_MASK;
+		psel_val[i] = MUX_FUNC(value);
+	}
+
+	/* Register a single pin group listing all the pins we read from DT */
+	gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL);
+	if (gsel < 0)
+		return gsel;
+
+	/*
+	 * Register a single group function where the 'data' is an array PSEL
+	 * register values read from DT.
+	 */
+	pin_fn[0] = np->name;
+	fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1,
+					   psel_val);
+	if (fsel < 0) {
+		ret = fsel;
+		goto remove_group;
+	}
+
+	dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
+
+	/* Create map where to retrieve function and mux settings from */
+	*num_maps = 0;
+	*map = kzalloc(sizeof(**map), GFP_KERNEL);
+	if (!*map) {
+		ret = -ENOMEM;
+		goto remove_function;
+	}
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = np->name;
+	(*map)->data.mux.function = np->name;
+	*num_maps = 1;
+
+	return 0;
+
+remove_function:
+	pinmux_generic_remove_function(pctldev, fsel);
+
+remove_group:
+	pinctrl_generic_remove_group(pctldev, gsel);
+
+	dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
+
+	return ret;
+}
+
+static void rza2_dt_free_map(struct pinctrl_dev *pctldev,
+			     struct pinctrl_map *map, unsigned int num_maps)
+{
+	kfree(map);
+}
+
+static const struct pinctrl_ops rza2_pinctrl_ops = {
+	.get_groups_count	= pinctrl_generic_get_group_count,
+	.get_group_name		= pinctrl_generic_get_group_name,
+	.get_group_pins		= pinctrl_generic_get_group_pins,
+	.dt_node_to_map		= rza2_dt_node_to_map,
+	.dt_free_map		= rza2_dt_free_map,
+};
+
+static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			   unsigned int group)
+{
+	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct function_desc *func;
+	struct group_desc *grp;
+	int i;
+	unsigned int *psel_val;
+
+	grp = pinctrl_generic_get_group(pctldev, group);
+	if (!grp)
+		return -EINVAL;
+
+	func = pinmux_generic_get_function(pctldev, selector);
+	if (!func)
+		return -EINVAL;
+
+	psel_val = func->data;
+
+	for (i = 0; i < grp->num_pins; ++i) {
+		dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n",
+			port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])],
+			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+			psel_val[i]);
+		rza2_set_pin_function(
+			priv->base,
+			RZA2_PIN_ID_TO_PORT(grp->pins[i]),
+			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+			psel_val[i]);
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops rza2_pinmux_ops = {
+	.get_functions_count	= pinmux_generic_get_function_count,
+	.get_function_name	= pinmux_generic_get_function_name,
+	.get_function_groups	= pinmux_generic_get_function_groups,
+	.set_mux		= rza2_set_mux,
+	.strict			= true,
+};
+
+static int rza2_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rza2_pinctrl_priv *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->desc.name		= DRIVER_NAME;
+	priv->desc.pctlops	= &rza2_pinctrl_ops;
+	priv->desc.pmxops	= &rza2_pinmux_ops;
+	priv->desc.owner	= THIS_MODULE;
+
+	ret = rza2_pinctrl_register(priv);
+	if (ret)
+		return ret;
+
+	pr_info("RZ/A2 pin controller registered\n");
+
+	return 0;
+}
+
+static const struct of_device_id rza2_pinctrl_of_match[] = {
+	{
+		.compatible	= "renesas,r7s9210-pinctrl",
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver rza2_pinctrl_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = rza2_pinctrl_of_match,
+	},
+	.probe = rza2_pinctrl_probe,
+};
+
+static int __init rza2_pinctrl_init(void)
+{
+	return platform_driver_register(&rza2_pinctrl_driver);
+}
+core_initcall(rza2_pinctrl_init);
+
+
+MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
+MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC");
+MODULE_LICENSE("GPL v2");
-- 
2.16.1

Re: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Geert Uytterhoeven]

Hi Chris,

On Wed, Nov 7, 2018 at 7:27 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,17 @@ config PINCTRL_RZA1
>         help
>           This selects pinctrl driver for Renesas RZ/A1 platforms.
>
> +config PINCTRL_RZA2
> +       bool "Renesas RZ/A2 gpio and pinctrl driver"
> +       depends on OF
> +       depends on ARCH_R7S9210 || COMPILE_TEST
> +       select GPIOLIB
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS
> +       select GENERIC_PINCONF

Given the PFS_ISEL bit, I think you can select GPIOLIB_IRQCHIP, and
implement interrupt support on GPIOs.
Of course that can be added later...

> +       help
> +         This selects pinctrl driver for Renesas RZ/A2 platforms.

GPIO and pinctrl?

> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rza2.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC
> + *
> + * Copyright (C) 2018 Chris Brandt
> + */
> +
> +/*
> + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2
> + * family.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "core.h"
> +#include "pinmux.h"
> +
> +#define DRIVER_NAME            "pinctrl-rza2"
> +
> +#define RZA2_PINS_PER_PORT     8
> +#define RZA2_NPINS             (RZA2_PINS_PER_PORT * RZA2_NPORTS)
> +#define RZA2_PIN_ID_TO_PORT(id)        ((id) / RZA2_PINS_PER_PORT)
> +#define RZA2_PIN_ID_TO_PIN(id) ((id) % RZA2_PINS_PER_PORT)
> +
> +/*
> + * Use 16 lower bits [15:0] for pin identifier
> + * Use 16 higher bits [31:16] for pin mux function
> + */
> +#define MUX_PIN_ID_MASK                GENMASK(15, 0)
> +#define MUX_FUNC_MASK          GENMASK(31, 16)
> +#define MUX_FUNC_OFFS          16
> +#define MUX_FUNC(pinconf)      ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> +
> +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7,
> +                       PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF,
> +                       PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS};

The only reason for this enum is to fix the value of RZA2_NPORTS,  right?

> +#define RZA2_PDR_BASE(_b)      ((_b) + 0x0000) /* 16-bit, 2 bytes apart */
> +#define RZA2_PODR_BASE(_b)     ((_b) + 0x0040) /* 8-bit, 1 byte apart */
> +#define RZA2_PIDR_BASE(_b)     ((_b) + 0x0060) /* 8-bit, 1 byte apart */
> +#define RZA2_PMR_BASE(_b)      ((_b) + 0x0080) /* 8-bit, 1 byte apart */
> +#define RZA2_DSCR_BASE(_b)     ((_b) + 0x0140) /* 16-bit, 2 bytes apart */
> +#define RZA2_PFS_BASE(_b)      ((_b) + 0x0200) /* 8-bit, 8 bytes apart */
> +#define RZA2_PWPR(_b)          ((_b) + 0x02FF) /* 8-bit */
> +#define RZA2_PFENET(_b)                ((_b) + 0x0820) /* 8-bit */
> +#define RZA2_PPOC(_b)          ((_b) + 0x0900) /* 32-bit */
> +#define RZA2_PHMOMO(_b)                ((_b) + 0x0980) /* 32-bit */
> +#define RZA2_PCKIO(_b)         ((_b) + 0x09D0) /* 8-bit */
> +
> +#define RZA2_PDR(_b, _port)            (RZA2_PDR_BASE(_b) + ((_port) * 2))
> +#define RZA2_PODR(_b, _port)           (RZA2_PODR_BASE(_b) + (_port))
> +#define RZA2_PIDR(_b, _port)           (RZA2_PIDR_BASE(_b) + (_port))
> +#define RZA2_PMR(_b, _port)            (RZA2_PMR_BASE(_b) + (_port))
> +#define RZA2_DSCR(_b, _port)           (RZA2_DSCR_BASE(_b) + ((_port) * 2))
> +#define RZA2_PFS(_b, _port, _pin)      (RZA2_PFS_BASE(_b) + ((_port) * 8) \
> +                                       + (_pin))

The above two sets of macros split information in two locations, and partially
duplicates it. I think it becomes easier to read if you combine them, and factor
out the addition of the base address. E.g.

        #define RZA2_PDR(port)        (0x0000 + (port) * 2) /* Port
Direction, 16-bit */

Then you can write:

        reg16 = readw(pfc_base + RZA2_PDR(port));
        mask16 = RZA2_PDR_MASK << (pin * 2);
        reg16 &= ~mask16;
        writew(reg16, pfc_base + RZA2_PDR(port));

> +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin,
> +                                 u8 func)
> +{
> +       u8 reg8;
> +       u16 reg16;
> +       u16 mask16;
> +
> +       /* Set pin to 'Non-use (Hi-z input protection)'  */
> +       reg16 = readw(RZA2_PDR(pfc_base, port));
> +       mask16 = 0x03 << (pin * 2);

mask16 = RZA2_PDR_MASK << (pin * 2);

> +       reg16 &= ~mask16;
> +       writew(reg16, RZA2_PDR(pfc_base, port));
> +
> +       /* Temporarily switch to GPIO */
> +       reg8 = readb(RZA2_PMR(pfc_base, port));
> +       reg8 &= ~BIT(pin);
> +       writeb(reg8, RZA2_PMR(pfc_base, port));
> +
> +       /* PFS Register Write Protect : OFF */
> +       writeb(0x00, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=0 */
> +       writeb(0x40, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=1 */

#define PWPR_B0WI        BIT(7)        /* Bit Write Disable */
#define PWPR_PFSWE       BIT(6)        /* PFS Register Write Enable */

> +
> +       /* Set Pin function (interrupt disabled, ISEL=0) */
> +       writeb(func, RZA2_PFS(pfc_base, port, pin));

#define PFS_ISEL        BIT(6)        /* Interrupt Select */

> +
> +       /* PFS Register Write Protect : ON */
> +       writeb(0x00, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=0 */
> +       writeb(0x80, RZA2_PWPR(pfc_base));      /* B0WI=1, PFSWE=0 */
> +
> +       /* Port Mode  : Peripheral module pin functions */
> +       reg8 = readb(RZA2_PMR(pfc_base, port));
> +       reg8 |= BIT(pin);
> +       writeb(reg8, RZA2_PMR(pfc_base, port));
> +}
> +
> +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir)
> +{
> +       u16 reg16;
> +       u16 mask16;
> +
> +       reg16 = readw(RZA2_PDR(pfc_base, port));
> +       mask16 = RZA2_PDR_MASK << (pin * 2);
> +       reg16 &= ~mask16;
> +
> +       if (dir == GPIOF_DIR_IN)
> +               reg16 |= RZA2_PDR_INPUT << (pin * 2);   /* pin as input */
> +       else
> +               reg16 |= RZA2_PDR_OUTPUT << (pin * 2);  /* pin as output */
> +
> +       writew(reg16, RZA2_PDR(pfc_base, port));
> +}
> +
> +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +       u8 port = offset / 8;
> +       u8 pin = offset % 8;

GPIO offset numbers are identical to PIN numbers, right?
So you can use RZA2_PIN_ID_TO_PORT(), RZA2_PIN_ID_TO_PIN()
for consistency.

> +       u16 reg16;
> +
> +       reg16 = readw(RZA2_PDR(priv->base, port));
> +       reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK;
> +
> +       if (reg16 == RZA2_PDR_OUTPUT)
> +               return GPIOF_DIR_OUT;
> +
> +       if (reg16 == RZA2_PDR_INPUT)
> +               return GPIOF_DIR_IN;
> +
> +       /*
> +        * This GPIO controller has a default Hi-Z state that is not input or
> +        * output, so force the pin to input now.
> +        */
> +       rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> +
> +       return GPIOF_DIR_IN;
> +}
> +
> +static int rza2_chip_direction_input(struct gpio_chip *chip,
> +                                    unsigned int offset)
> +{
> +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +       u8 port = offset / 8;
> +       u8 pin = offset % 8;
> +
> +       rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);

Perhaps pass offset to rza2_pin_to_gpio() directly?

> +
> +       return 0;
> +}
> +
> +static int rza2_chip_direction_output(struct gpio_chip *chip,
> +                                     unsigned int offset, int val)
> +{
> +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +       u8 port = offset / 8;
> +       u8 pin = offset % 8;
> +
> +       rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT);
> +
> +       return 0;
> +}
> +
> +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +       u8 port = offset / 8;
> +       u8 pin = offset % 8;
> +
> +       return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1;

Might be easier to read if you maintain symmetry with below:

    return !!(readb(RZA2_PIDR(priv->base, port) & BIT(pin));

> +}
> +
> +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset,
> +                         int value)
> +{
> +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +       u8 port = offset / 8;
> +       u8 pin = offset % 8;
> +       u8 new_value = readb(RZA2_PODR(priv->base, port));
> +
> +       if (value)
> +               new_value |= (1 << pin);

new_value |= BIT(pin);

> +       else
> +               new_value &= ~(1 << pin);

new_value &= ~BIT(pin);

> +
> +       writeb(new_value, RZA2_PODR(priv->base, port));
> +}
> +
> +static const char * const rza2_gpio_names[] = {
> +       "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> +       "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> +       "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> +       "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> +       "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> +       "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> +       "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> +       "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> +       "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> +       "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> +       "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> +       "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> +       "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> +       "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> +       "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> +       "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> +       "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> +       "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> +       /* port I does not exist */

Hence shouldn't you have 8 NULL entries here?

 * @names: if set, must be an array of strings to use as alternative
 *      names for the GPIOs in this chip. Any entry in the array
 *      may be NULL if there is no alias for the GPIO, however the
 *      array must be @ngpio entries long.  A name can include a single printk
 *      format specifier for an unsigned int.  It is substituted by the actual
 *      number of the gpio.

> +       "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
> +       "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
> +       "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
> +       "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
> +};
> +
> +static struct gpio_chip chip = {
> +       .names = rza2_gpio_names,

BTW, is their much value in filling gpio_chip.names[]?
I had never seen that before.

> +       .base = -1,
> +       .ngpio = RZA2_NPINS,
> +       .get_direction = rza2_chip_get_direction,
> +       .direction_input = rza2_chip_direction_input,
> +       .direction_output = rza2_chip_direction_output,
> +       .get = rza2_chip_get,
> +       .set = rza2_chip_set,

You may want to implement .[gs]et_multiple(), too.

> +};
> +
> +struct pinctrl_gpio_range gpio_range;

Please make this static.
Or even better, store it in rza2_pinctrl_priv?

> +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv)
> +{
> +       struct pinctrl_pin_desc *pins;
> +       unsigned int i;
> +       int ret;
> +
> +       pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL);
> +       if (!pins)
> +               return -ENOMEM;
> +
> +       priv->pins = pins;
> +       priv->desc.pins = pins;
> +       priv->desc.npins = RZA2_NPINS;
> +
> +       for (i = 0; i < RZA2_NPINS; i++) {
> +               unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
> +               unsigned int port = RZA2_PIN_ID_TO_PORT(i);
> +
> +               pins[i].number = i;
> +               pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u",
> +                                             port_names[port], pin);

These are identical to rza2_gpio_names[], so can't you use those?

> +/*
> + * For each DT node, create a single pin mapping. That pin mapping will only
> + * contain a single group of pins, and that group of pins will only have a
> + * single function that can be selected.
> + */
> +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev,
> +                              struct device_node *np,
> +                              struct pinctrl_map **map,
> +                              unsigned int *num_maps)
> +{
> +       struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +       struct property *of_pins;
> +       int i;
> +       unsigned int *pins;
> +       unsigned int *psel_val;
> +       const char **pin_fn;
> +       int ret, npins;
> +       int gsel, fsel;

Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing
declaration length.

[...]

> +       dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);

%pOF ... np

> +
> +       /* Create map where to retrieve function and mux settings from */
> +       *num_maps = 0;
> +       *map = kzalloc(sizeof(**map), GFP_KERNEL);
> +       if (!*map) {
> +               ret = -ENOMEM;
> +               goto remove_function;
> +       }
> +
> +       (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> +       (*map)->data.mux.group = np->name;
> +       (*map)->data.mux.function = np->name;
> +       *num_maps = 1;
> +
> +       return 0;
> +
> +remove_function:
> +       pinmux_generic_remove_function(pctldev, fsel);
> +
> +remove_group:
> +       pinctrl_generic_remove_group(pctldev, gsel);
> +
> +       dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);

dev_err?

%pOF ... np


> +
> +       return ret;
> +}

> +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +                          unsigned int group)
> +{
> +       struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +       struct function_desc *func;
> +       struct group_desc *grp;
> +       int i;
> +       unsigned int *psel_val;

Reverse Xmas tree ordering?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Geert,

As always, thank you for your review!


On Monday, November 12, 2018, Geert Uytterhoeven wrote:
> > +config PINCTRL_RZA2
> > +       bool "Renesas RZ/A2 gpio and pinctrl driver"
> > +       depends on OF
> > +       depends on ARCH_R7S9210 || COMPILE_TEST
> > +       select GPIOLIB
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> > +       select GENERIC_PINCONF
> 
> Given the PFS_ISEL bit, I think you can select GPIOLIB_IRQCHIP, and
> implement interrupt support on GPIOs.
> Of course that can be added later...

Yes, I was thinking that too.
I have had user ask about that before, so maybe that will be a 2019 
task.


> > +       help
> > +         This selects pinctrl driver for Renesas RZ/A2 platforms.
> 
> GPIO and pinctrl?

True.


> > +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6,
> PORT7,
> > +                       PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE,
> PORTF,
> > +                       PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM,
> RZA2_NPORTS};
> 
> The only reason for this enum is to fix the value of RZA2_NPORTS,  right?

At the moment, yes.
Originally I thought I would need these. But in the end, I did not.
So I can just define RZA2_NPORTS for now. Maybe for future SoCs,
That value will change depending on the SoC.


> The above two sets of macros split information in two locations, and
> partially
> duplicates it. I think it becomes easier to read if you combine them, and
> factor
> out the addition of the base address. E.g.
> 
>         #define RZA2_PDR(port)        (0x0000 + (port) * 2) /* Port
> Direction, 16-bit */
> 
> Then you can write:
> 
>         reg16 = readw(pfc_base + RZA2_PDR(port));
>         mask16 = RZA2_PDR_MASK << (pin * 2);
>         reg16 &= ~mask16;
>         writew(reg16, pfc_base + RZA2_PDR(port));
> 
> > +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8
> pin,

OK. I'm all for 'easier to read'!


> > +       mask16 = 0x03 << (pin * 2);
> 
> mask16 = RZA2_PDR_MASK << (pin * 2);

OK.

> > +       /* PFS Register Write Protect : OFF */
> > +       writeb(0x00, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=0 */
> > +       writeb(0x40, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=1 */
> 
> #define PWPR_B0WI        BIT(7)        /* Bit Write Disable */
> #define PWPR_PFSWE       BIT(6)        /* PFS Register Write Enable */

OK.

> > +       /* Set Pin function (interrupt disabled, ISEL=0) */
> > +       writeb(func, RZA2_PFS(pfc_base, port, pin));
> 
> #define PFS_ISEL        BIT(6)        /* Interrupt Select */

OK.

> > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> 
> GPIO offset numbers are identical to PIN numbers, right?
> So you can use RZA2_PIN_ID_TO_PORT(), RZA2_PIN_ID_TO_PIN()
> for consistency.

Good point!


> > +static int rza2_chip_direction_input(struct gpio_chip *chip,
> > +                                    unsigned int offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> > +
> > +       rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> 
> Perhaps pass offset to rza2_pin_to_gpio() directly?

OK. I can do that. Saves a couple of lines ;)


> > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> > +
> > +       return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1;
> 
> Might be easier to read if you maintain symmetry with below:
> 
>     return !!(readb(RZA2_PIDR(priv->base, port) & BIT(pin));

Cool.

> > +       if (value)
> > +               new_value |= (1 << pin);
> 
> new_value |= BIT(pin);
> 
> > +       else
> > +               new_value &= ~(1 << pin);
> 
> new_value &= ~BIT(pin);

I wondered about using BIT more often in the code.
Thanks.

> > +static const char * const rza2_gpio_names[] = {
> > +       "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> > +       "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> > +       "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> > +       "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> > +       "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> > +       "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> > +       "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> > +       "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> > +       "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> > +       "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> > +       "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> > +       "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> > +       "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> > +       "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> > +       "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> > +       "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> > +       "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> > +       "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> > +       /* port I does not exist */
> 
> Hence shouldn't you have 8 NULL entries here?

But there is no such port named "I". And even in the dt-bindings and other
documentation, the global pin ID is based off and a world where "I" is not
in the alphabet. So if I put 8 NULLs here, wouldn't that screw up all the
indexing??



> > +static struct gpio_chip chip = {
> > +       .names = rza2_gpio_names,
> 
> BTW, is their much value in filling gpio_chip.names[]?
> I had never seen that before.

It makes the files show up under /sys look nice.

For example, P5_6 is button SW4:

 $ echo 912 > /sys/class/gpio/export

Then you end up with "/sys/class/gpio/P5_6/"

 $ echo in > /sys/class/gpio/P5_6/direction
 $ cat /sys/class/gpio/P5_6/direction
 $ cat /sys/class/gpio/P5_6/value


> > +       .get = rza2_chip_get,
> > +       .set = rza2_chip_set,
> 
> You may want to implement .[gs]et_multiple(), too.

OK, I will have a look.


> > +struct pinctrl_gpio_range gpio_range;
> 
> Please make this static.
> Or even better, store it in rza2_pinctrl_priv?

OK.


> > +       for (i = 0; i < RZA2_NPINS; i++) {
> > +               unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
> > +               unsigned int port = RZA2_PIN_ID_TO_PORT(i);
> > +
> > +               pins[i].number = i;
> > +               pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL,
> "P%c_%u",
> > +                                             port_names[port], pin);
> 
> These are identical to rza2_gpio_names[], so can't you use those?

Good point! I'll try that.


> > +{
> > +       struct rza2_pinctrl_priv *priv =
> pinctrl_dev_get_drvdata(pctldev);
> > +       struct property *of_pins;
> > +       int i;
> > +       unsigned int *pins;
> > +       unsigned int *psel_val;
> > +       const char **pin_fn;
> > +       int ret, npins;
> > +       int gsel, fsel;
> 
> Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing
> declaration length.

https://lwn.net/Articles/758552/

"only a few maintainers insist on that, while most really do not care or
think that it's actively silly."

So are you one of those maintainers?????   :)


> > +       dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
> 
> %pOF ... np

OK.


> > +remove_group:
> > +       pinctrl_generic_remove_group(pctldev, gsel);
> > +
> > +       dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
> 
> dev_err?
> 
> %pOF ... np

Thanks.



Chris

Re: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Geert Uytterhoeven]

Hi Chris,

On Tue, Nov 13, 2018 at 7:43 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, November 12, 2018, Geert Uytterhoeven wrote:
> > > +static const char * const rza2_gpio_names[] = {
> > > +       "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> > > +       "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> > > +       "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> > > +       "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> > > +       "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> > > +       "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> > > +       "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> > > +       "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> > > +       "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> > > +       "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> > > +       "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> > > +       "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> > > +       "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> > > +       "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> > > +       "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> > > +       "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> > > +       "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> > > +       "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> > > +       /* port I does not exist */
> >
> > Hence shouldn't you have 8 NULL entries here?
>
> But there is no such port named "I". And even in the dt-bindings and other
> documentation, the global pin ID is based off and a world where "I" is not
> in the alphabet. So if I put 8 NULLs here, wouldn't that screw up all the
> indexing??

I'd swear there's an "I" in port_names[], but upon checking again, it must
have been my eyes that mislead me. Sorry for that.
Hence please forget my comment above.

> > > +static struct gpio_chip chip = {
> > > +       .names = rza2_gpio_names,
> >
> > BTW, is their much value in filling gpio_chip.names[]?
> > I had never seen that before.
>
> It makes the files show up under /sys look nice.
>
> For example, P5_6 is button SW4:
>
>  $ echo 912 > /sys/class/gpio/export
>
> Then you end up with "/sys/class/gpio/P5_6/"
>
>  $ echo in > /sys/class/gpio/P5_6/direction
>  $ cat /sys/class/gpio/P5_6/direction
>  $ cat /sys/class/gpio/P5_6/value

(Ah, the legacy and deprecated sysfs GPIO interface, being replaced
 by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod)

Cool, I didn't know that.
But you still need to know which number to write to the export file
in the first place?

> > > +       .get = rza2_chip_get,
> > > +       .set = rza2_chip_set,
> >
> > You may want to implement .[gs]et_multiple(), too.
>
> OK, I will have a look.

You can add that later.  It doesn't add functionality, but may improve
performance for bitbanging multiple pins.


> > > +{
> > > +       struct rza2_pinctrl_priv *priv =
> > pinctrl_dev_get_drvdata(pctldev);
> > > +       struct property *of_pins;
> > > +       int i;
> > > +       unsigned int *pins;
> > > +       unsigned int *psel_val;
> > > +       const char **pin_fn;
> > > +       int ret, npins;
> > > +       int gsel, fsel;
> >
> > Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing
> > declaration length.
>
> https://lwn.net/Articles/758552/
>
> "only a few maintainers insist on that, while most really do not care or
> think that it's actively silly."
>
> So are you one of those maintainers?????   :)

Sorry, got baptised by Laurent...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Geert,

On Tuesday, November 13, 2018, Geert Uytterhoeven wrote:
> > It makes the files show up under /sys look nice.
> >
> > For example, P5_6 is button SW4:
> >
> >  $ echo 912 > /sys/class/gpio/export
> >
> > Then you end up with "/sys/class/gpio/P5_6/"
> >
> >  $ echo in > /sys/class/gpio/P5_6/direction
> >  $ cat /sys/class/gpio/P5_6/direction
> >  $ cat /sys/class/gpio/P5_6/value
> 
> (Ah, the legacy and deprecated sysfs GPIO interface, being replaced
>  by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod)
> 
> Cool, I didn't know that.
> But you still need to know which number to write to the export file
> in the first place?

True, meaning the table does not help you as much as you want.
Jacopo also mentioned the new libgpiod.
So, I think I might just drop this table in the next revision.

What I really want to do is just say "make P5_6 an input" and
not have to convert to a global ID number. But, I'm not sure how
libgpiod is going to know what "P5_6" is.


> > > Some people prefer "reverse Xmas tree ordering" i.e. sorted by
> decreasing
> > > declaration length.
> >
> > https://lwn.net/Articles/758552/
> >
> > "only a few maintainers insist on that, while most really do not care or
> > think that it's actively silly."
> >
> > So are you one of those maintainers?????   :)
> 
> Sorry, got baptised by Laurent...

 (insert picture of Laurent handing out Kool-Aid here)


Chris

Re: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Geert Uytterhoeven]

Hi Chris,

On Tue, Nov 13, 2018 at 8:26 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, November 13, 2018, Geert Uytterhoeven wrote:
> > > It makes the files show up under /sys look nice.
> > >
> > > For example, P5_6 is button SW4:
> > >
> > >  $ echo 912 > /sys/class/gpio/export
> > >
> > > Then you end up with "/sys/class/gpio/P5_6/"
> > >
> > >  $ echo in > /sys/class/gpio/P5_6/direction
> > >  $ cat /sys/class/gpio/P5_6/direction
> > >  $ cat /sys/class/gpio/P5_6/value
> >
> > (Ah, the legacy and deprecated sysfs GPIO interface, being replaced
> >  by /dev/gpiochip[0-9]+ and https://github.com/brgl/libgpiod)
> >
> > Cool, I didn't know that.
> > But you still need to know which number to write to the export file
> > in the first place?
>
> True, meaning the table does not help you as much as you want.
> Jacopo also mentioned the new libgpiod.
> So, I think I might just drop this table in the next revision.
>
> What I really want to do is just say "make P5_6 an input" and
> not have to convert to a global ID number. But, I'm not sure how
> libgpiod is going to know what "P5_6" is.

There are two parts:
  1. New kernel /dev/gpiochip[0-9]+ interface

     Sample code comes with the kernel under tools/gpio/.
     E.g.
     root@koelsch:~# lsgpio -n gpiochip7
     GPIO chip: gpiochip7, "e6055800.gpio", 26 GPIO lines
      line  0: unnamed "SW30" [kernel active-low]
      line  1: unnamed "SW31" [kernel active-low]
      line  2: unnamed "SW32" [kernel active-low]
      line  3: unnamed "SW33" [kernel active-low]
      line  4: unnamed "SW34" [kernel active-low]
      line  5: unnamed "SW35" [kernel active-low]
      line  6: unnamed "SW36" [kernel active-low]
      line  7: unnamed unused
      line  8: unnamed unused
      line  9: unnamed unused
      line 10: unnamed unused
      line 11: unnamed unused
      line 12: unnamed unused
      line 13: unnamed unused
      line 14: unnamed unused
      line 15: unnamed unused
      line 16: unnamed unused
      line 17: unnamed "regulator-vcc-sdhi0" [kernel output]
      line 18: unnamed "regulator-vcc-sdhi1" [kernel output]
      line 19: unnamed "regulator-vcc-sdhi2" [kernel output]
      line 20: unnamed unused
      line 21: unnamed unused
      line 22: unnamed unused
      line 23: unnamed unused
      line 24: unnamed unused
      line 25: unnamed unused

  2. Userspace library libgpiod, incl. a few tools like
gpioinfo/gpioset/gpioget.
     These accept whatever reference to identify a GPIO.
     As your driver fills in pins[i].name, I expect you can pass names
like P5_6.

Have fun! ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Geert,

Just in case you were wondering...

On Monday, November 12, 2018, Geert Uytterhoeven wrote:
> > +       "PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
> > +       "PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
> > +       "PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
> > +       "PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
> > +};
> > +
> > +static struct gpio_chip chip = {
> > +       .names = rza2_gpio_names,
> 
> BTW, is their much value in filling gpio_chip.names[]?
> I had never seen that before.


On Tuesday, November 13, 2018, Geert Uytterhoeven wrote:
>   2. Userspace library libgpiod, incl. a few tools like
> gpioinfo/gpioset/gpioget.
>      These accept whatever reference to identify a GPIO.
>      As your driver fills in pins[i].name, I expect you can pass names
> like P5_6.


Actually, just filling in pins[i].name is not enough. You really need to
pass an array of strings to gpio_chip.name.

When struct gpio_chip.names = NULL, you get this:

$ gpioinfo
gpiochip0 - 176 lines:
        line   0:      unnamed       unused   input  active-high
        line   1:      unnamed       unused   input  active-high
        line   2:      unnamed       unused   input  active-high
        line   3:      unnamed       unused   input  active-high
        line   4:      unnamed       unused   input  active-high
        line   5:      unnamed       unused   input  active-high
        line   6:      unnamed       unused   input  active-high
        line   7:      unnamed       unused   input  active-high
        line   8:      unnamed       unused   input  active-high
        line   9:      unnamed       unused   input  active-high
        line  10:      unnamed       unused   input  active-high
        line  11:      unnamed       unused   input  active-high
        line  12:      unnamed       unused   input  active-high


When struct gpio_chip.names = rza2_gpio_names, you get this:

$ gpioinfo
gpiochip0 - 176 lines:
        line   0:       "P0_0"       unused   input  active-high
        line   1:       "P0_1"       unused   input  active-high
        line   2:       "P0_2"       unused   input  active-high
        line   3:       "P0_3"       unused   input  active-high
        line   4:       "P0_4"       unused   input  active-high
        line   5:       "P0_5"       unused   input  active-high
        line   6:       "P0_6"       unused   input  active-high
        line   7:       "P0_7"       unused   input  active-high
        line   8:       "P1_0"       unused   input  active-high
        line   9:       "P1_1"       unused   input  active-high
        line  10:       "P1_2"       unused   input  active-high
        line  11:       "P1_3"       unused   input  active-high
        line  12:       "P1_4"       unused   input  active-high

Then you can use gpiofind to convert a name into an ID.

$ gpiofind P6_0
gpiochip0 48


But to your point, libgpiod is way better than using /sys directly.

Chris

Re: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 19768 bytes --]

Hi Chris,
   a few more notes on top of what Geert said. Thanks for addressing
comments on the previous version.

On Wed, Nov 07, 2018 at 01:27:32PM -0500, Chris Brandt wrote:
> Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v3:
>  - Changed names from Px to PORTx because "PC" is already defined
> v2:
>  - fixed SOC part number in comments
>  - sorted #includes
>  - removed spaces in pfc_pin_port_name enum
>  - put RZA2_NPORTS at the end of pfc_pin_port_name enum
>  - added RZA2_ to the beginning of all #define macros
>  - put ( ) around all passed arguments in #define macros
>  - made helper macros to get register address easier
>  - use defines for pin direction bit settings
> ---
>  drivers/pinctrl/Kconfig        |  11 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-rza2.c | 530 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 542 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rza2.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 4d8c00eac742..3e4d890d4bd9 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,17 @@ config PINCTRL_RZA1
>  	help
>  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
>
> +config PINCTRL_RZA2
> +	bool "Renesas RZ/A2 gpio and pinctrl driver"
> +	depends on OF
> +	depends on ARCH_R7S9210 || COMPILE_TEST
> +	select GPIOLIB
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCONF
> +	help
> +	  This selects pinctrl driver for Renesas RZ/A2 platforms.
> +
>  config PINCTRL_RZN1
>  	bool "Renesas RZ/N1 pinctrl driver"
>  	depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 18a13c1e2c21..712184b74a5c 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
>  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
> +obj-$(CONFIG_PINCTRL_RZA2)	+= pinctrl-rza2.o
>  obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
>  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
>  obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
> diff --git a/drivers/pinctrl/pinctrl-rza2.c b/drivers/pinctrl/pinctrl-rza2.c
> new file mode 100644
> index 000000000000..3781c3f693e8
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rza2.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S9210) SoC
> + *
> + * Copyright (C) 2018 Chris Brandt
> + */
> +
> +/*
> + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2
> + * family.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "core.h"
> +#include "pinmux.h"
> +
> +#define DRIVER_NAME		"pinctrl-rza2"
> +
> +#define RZA2_PINS_PER_PORT	8
> +#define RZA2_NPINS		(RZA2_PINS_PER_PORT * RZA2_NPORTS)
> +#define RZA2_PIN_ID_TO_PORT(id)	((id) / RZA2_PINS_PER_PORT)
> +#define RZA2_PIN_ID_TO_PIN(id)	((id) % RZA2_PINS_PER_PORT)
> +
> +/*
> + * Use 16 lower bits [15:0] for pin identifier
> + * Use 16 higher bits [31:16] for pin mux function
> + */
> +#define MUX_PIN_ID_MASK		GENMASK(15, 0)
> +#define MUX_FUNC_MASK		GENMASK(31, 16)
> +#define MUX_FUNC_OFFS		16
> +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> +
> +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6, PORT7,
> +			PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE, PORTF,
> +			PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM, RZA2_NPORTS};
> +static const char port_names[] = "0123456789ABCDEFGHJKLM";
> +
> +struct rza2_pinctrl_priv {
> +	struct device *dev;
> +	void __iomem *base;
> +
> +	struct pinctrl_pin_desc *pins;
> +	struct pinctrl_desc desc;
> +	struct pinctrl_dev *pctl;
> +};
> +
> +#define RZA2_PDR_BASE(_b)	((_b) + 0x0000)	/* 16-bit, 2 bytes apart */
> +#define RZA2_PODR_BASE(_b)	((_b) + 0x0040)	/* 8-bit, 1 byte apart */
> +#define RZA2_PIDR_BASE(_b)	((_b) + 0x0060)	/* 8-bit, 1 byte apart */
> +#define RZA2_PMR_BASE(_b)	((_b) + 0x0080)	/* 8-bit, 1 byte apart */
> +#define RZA2_DSCR_BASE(_b)	((_b) + 0x0140)	/* 16-bit, 2 bytes apart */
> +#define RZA2_PFS_BASE(_b)	((_b) + 0x0200)	/* 8-bit, 8 bytes apart */
> +#define RZA2_PWPR(_b)		((_b) + 0x02FF)	/* 8-bit */

0x2ff

> +#define RZA2_PFENET(_b)		((_b) + 0x0820)	/* 8-bit */
> +#define RZA2_PPOC(_b)		((_b) + 0x0900)	/* 32-bit */
> +#define RZA2_PHMOMO(_b)		((_b) + 0x0980)	/* 32-bit */
> +#define RZA2_PCKIO(_b)		((_b) + 0x09D0)	/* 8-bit */
> +

0x09d0

> +#define RZA2_PDR(_b, _port)		(RZA2_PDR_BASE(_b) + ((_port) * 2))
> +#define RZA2_PODR(_b, _port)		(RZA2_PODR_BASE(_b) + (_port))
> +#define RZA2_PIDR(_b, _port)		(RZA2_PIDR_BASE(_b) + (_port))
> +#define RZA2_PMR(_b, _port)		(RZA2_PMR_BASE(_b) + (_port))
> +#define RZA2_DSCR(_b, _port)		(RZA2_DSCR_BASE(_b) + ((_port) * 2))
> +#define RZA2_PFS(_b, _port, _pin)	(RZA2_PFS_BASE(_b) + ((_port) * 8) \
> +					+ (_pin))
> +#define RZA2_PDR_HIGHZ		0x00

This is not used (I know I've suggested that, sorry :)

> +#define RZA2_PDR_INPUT		0x02
> +#define RZA2_PDR_OUTPUT		0x03
> +#define RZA2_PDR_MASK		0x03
> +
> +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin,
> +				  u8 func)
> +{
> +	u8 reg8;
> +	u16 reg16;
> +	u16 mask16;
> +

I would prefer the reverse xmas order too, but I'm not saying out loud
as I understand is something hard to enforce, as it's a very minor
issue :)

> +	/* Set pin to 'Non-use (Hi-z input protection)'  */
> +	reg16 = readw(RZA2_PDR(pfc_base, port));
> +	mask16 = 0x03 << (pin * 2);
> +	reg16 &= ~mask16;
> +	writew(reg16, RZA2_PDR(pfc_base, port));
> +
> +	/* Temporarily switch to GPIO */
> +	reg8 = readb(RZA2_PMR(pfc_base, port));
> +	reg8 &= ~BIT(pin);
> +	writeb(reg8, RZA2_PMR(pfc_base, port));
> +
> +	/* PFS Register Write Protect : OFF */
> +	writeb(0x00, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
> +	writeb(0x40, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=1 */
> +
> +	/* Set Pin function (interrupt disabled, ISEL=0) */
> +	writeb(func, RZA2_PFS(pfc_base, port, pin));
> +
> +	/* PFS Register Write Protect : ON */
> +	writeb(0x00, RZA2_PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
> +	writeb(0x80, RZA2_PWPR(pfc_base));	/* B0WI=1, PFSWE=0 */
> +
> +	/* Port Mode  : Peripheral module pin functions */
> +	reg8 = readb(RZA2_PMR(pfc_base, port));
> +	reg8 |= BIT(pin);
> +	writeb(reg8, RZA2_PMR(pfc_base, port));
> +}
> +
> +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir)
> +{
> +	u16 reg16;
> +	u16 mask16;
> +
> +	reg16 = readw(RZA2_PDR(pfc_base, port));
> +	mask16 = RZA2_PDR_MASK << (pin * 2);
> +	reg16 &= ~mask16;
> +
> +	if (dir == GPIOF_DIR_IN)
> +		reg16 |= RZA2_PDR_INPUT << (pin * 2);	/* pin as input */
> +	else
> +		reg16 |= RZA2_PDR_OUTPUT << (pin * 2);	/* pin as output */
> +
> +	writew(reg16, RZA2_PDR(pfc_base, port));
> +}
> +
> +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +	u16 reg16;
> +
> +	reg16 = readw(RZA2_PDR(priv->base, port));
> +	reg16 = (reg16 >> (pin * 2)) & RZA2_PDR_MASK;
> +
> +	if (reg16 == RZA2_PDR_OUTPUT)
> +		return GPIOF_DIR_OUT;
> +
> +	if (reg16 == RZA2_PDR_INPUT)
> +		return GPIOF_DIR_IN;
> +
> +	/*
> +	 * This GPIO controller has a default Hi-Z state that is not input or
> +	 * output, so force the pin to input now.
> +	 */
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> +
> +	return GPIOF_DIR_IN;
> +}
> +
> +static int rza2_chip_direction_input(struct gpio_chip *chip,
> +				     unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> +
> +	return 0;
> +}
> +
> +static int rza2_chip_direction_output(struct gpio_chip *chip,
> +				      unsigned int offset, int val)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT);
> +
> +	return 0;
> +}
> +
> +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1;
> +}
> +
> +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset,
> +			  int value)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +	u8 new_value = readb(RZA2_PODR(priv->base, port));
> +
> +	if (value)
> +		new_value |= (1 << pin);
> +	else
> +		new_value &= ~(1 << pin);

As Geert suggested, use BIT() ?
> +
> +	writeb(new_value, RZA2_PODR(priv->base, port));
> +}
> +
> +static const char * const rza2_gpio_names[] = {
> +	"P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> +	"P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> +	"P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> +	"P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> +	"P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> +	"P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> +	"P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> +	"P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> +	"P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> +	"P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> +	"PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> +	"PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> +	"PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> +	"PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> +	"PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> +	"PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> +	"PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> +	"PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> +	/* port I does not exist */
> +	"PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
> +	"PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
> +	"PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
> +	"PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
> +};
> +
> +static struct gpio_chip chip = {
> +	.names = rza2_gpio_names,
> +	.base = -1,
> +	.ngpio = RZA2_NPINS,
> +	.get_direction = rza2_chip_get_direction,
> +	.direction_input = rza2_chip_direction_input,
> +	.direction_output = rza2_chip_direction_output,
> +	.get = rza2_chip_get,
> +	.set = rza2_chip_set,
> +};
> +
> +struct pinctrl_gpio_range gpio_range;
> +
> +static int rza2_gpio_register(struct rza2_pinctrl_priv *priv)
> +{
> +	struct device_node *np = priv->dev->of_node;
> +	struct of_phandle_args of_args;
> +	int ret;
> +
> +	if (!of_property_read_bool(np, "gpio-controller")) {
> +		dev_info(priv->dev, "No gpio controller registered\n");
> +		return 0;
> +	}

The bindings say this is mandatory... What do you think, would that
make sense to have no gpio-controller support for this driver (testing
apart :)

> +
> +	chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np);
> +	chip.of_node = np;
> +	chip.parent = priv->dev;
> +
> +	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
> +					       &of_args);
> +	if (ret) {
> +		dev_err(priv->dev, "Unable to parse gpio-ranges\n");
> +		return ret;
> +	}
> +
> +	gpio_range.id = of_args.args[0];

Do you think of_args need to be validated?
As I understand them, id and pin_base are fixed at 0, while the pin
number depends on the soc version/revision.

Now I wonder if you would need a gpio-ranges property at all, but
that's fine, it doesn't hurt...

> +	gpio_range.name = chip.label;
> +	gpio_range.pin_base = gpio_range.base = of_args.args[1];
> +	gpio_range.npins = of_args.args[2];
> +	gpio_range.gc = &chip;
> +
> +	/* Register our gpio chip with gpiolib */
> +	ret = devm_gpiochip_add_data(priv->dev, &chip, priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Register pin range with pinctrl core */
> +	pinctrl_add_gpio_range(priv->pctl, &gpio_range);
> +
> +	dev_dbg(priv->dev, "Registered gpio controller\n");
> +
> +	return 0;
> +}
> +
> +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv)
> +{
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int i;
> +	int ret;
> +
> +	pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL);
> +	if (!pins)
> +		return -ENOMEM;
> +
> +	priv->pins = pins;
> +	priv->desc.pins = pins;
> +	priv->desc.npins = RZA2_NPINS;
> +
> +	for (i = 0; i < RZA2_NPINS; i++) {
> +		unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
> +		unsigned int port = RZA2_PIN_ID_TO_PORT(i);
> +
> +		pins[i].number = i;
> +		pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u",
> +					      port_names[port], pin);
> +	}
> +
> +	ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv,
> +					     &priv->pctl);
> +	if (ret) {
> +		dev_err(priv->dev, "pinctrl registration failed\n");
> +		return ret;
> +	}
> +
> +	ret = pinctrl_enable(priv->pctl);
> +	if (ret) {
> +		dev_err(priv->dev, "pinctrl enable failed\n");
> +		return ret;
> +	}
> +
> +	ret = rza2_gpio_register(priv);
> +	if (ret) {
> +		dev_err(priv->dev, "GPIO registration failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * For each DT node, create a single pin mapping. That pin mapping will only
> + * contain a single group of pins, and that group of pins will only have a
> + * single function that can be selected.
> + */
> +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			       struct device_node *np,
> +			       struct pinctrl_map **map,
> +			       unsigned int *num_maps)
> +{
> +	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +	struct property *of_pins;
> +	int i;
> +	unsigned int *pins;
> +	unsigned int *psel_val;
> +	const char **pin_fn;
> +	int ret, npins;
> +	int gsel, fsel;
> +
> +	/* Find out how many pins to map */
> +	of_pins = of_find_property(np, "pinmux", NULL);
> +	if (!of_pins) {
> +		dev_info(priv->dev, "Missing pinmux property\n");
> +		return -ENOENT;
> +	}
> +	npins = of_pins->length / sizeof(u32);
> +
> +	pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL);
> +	psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val),
> +				GFP_KERNEL);
> +	pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL);
> +	if (!pins || !psel_val || !pin_fn)
> +		return -ENOMEM;
> +
> +	/* Collect pin locations and mux settings from DT properties */
> +	for (i = 0; i < npins; ++i) {
> +		u32 value;
> +
> +		ret = of_property_read_u32_index(np, "pinmux", i, &value);
> +		if (ret)
> +			return ret;
> +		pins[i] = value & MUX_PIN_ID_MASK;
> +		psel_val[i] = MUX_FUNC(value);
> +	}
> +
> +	/* Register a single pin group listing all the pins we read from DT */
> +	gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL);
> +	if (gsel < 0)
> +		return gsel;
> +
> +	/*
> +	 * Register a single group function where the 'data' is an array PSEL
> +	 * register values read from DT.
> +	 */
> +	pin_fn[0] = np->name;
> +	fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1,
> +					   psel_val);
> +	if (fsel < 0) {
> +		ret = fsel;
> +		goto remove_group;
> +	}
> +
> +	dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
> +
> +	/* Create map where to retrieve function and mux settings from */
> +	*num_maps = 0;
> +	*map = kzalloc(sizeof(**map), GFP_KERNEL);
> +	if (!*map) {
> +		ret = -ENOMEM;
> +		goto remove_function;
> +	}
> +
> +	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> +	(*map)->data.mux.group = np->name;
> +	(*map)->data.mux.function = np->name;
> +	*num_maps = 1;
> +
> +	return 0;
> +
> +remove_function:
> +	pinmux_generic_remove_function(pctldev, fsel);
> +
> +remove_group:
> +	pinctrl_generic_remove_group(pctldev, gsel);
> +
> +	dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
> +
> +	return ret;
> +}
> +
> +static void rza2_dt_free_map(struct pinctrl_dev *pctldev,
> +			     struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	kfree(map);
> +}
> +
> +static const struct pinctrl_ops rza2_pinctrl_ops = {
> +	.get_groups_count	= pinctrl_generic_get_group_count,
> +	.get_group_name		= pinctrl_generic_get_group_name,
> +	.get_group_pins		= pinctrl_generic_get_group_pins,
> +	.dt_node_to_map		= rza2_dt_node_to_map,
> +	.dt_free_map		= rza2_dt_free_map,
> +};
> +
> +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			   unsigned int group)
> +{
> +	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +	struct function_desc *func;
> +	struct group_desc *grp;
> +	int i;
> +	unsigned int *psel_val;
> +
> +	grp = pinctrl_generic_get_group(pctldev, group);
> +	if (!grp)
> +		return -EINVAL;
> +
> +	func = pinmux_generic_get_function(pctldev, selector);
> +	if (!func)
> +		return -EINVAL;
> +
> +	psel_val = func->data;
> +
> +	for (i = 0; i < grp->num_pins; ++i) {
> +		dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n",
> +			port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])],
> +			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
> +			psel_val[i]);
> +		rza2_set_pin_function(
> +			priv->base,
> +			RZA2_PIN_ID_TO_PORT(grp->pins[i]),
> +			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
> +			psel_val[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinmux_ops rza2_pinmux_ops = {
> +	.get_functions_count	= pinmux_generic_get_function_count,
> +	.get_function_name	= pinmux_generic_get_function_name,
> +	.get_function_groups	= pinmux_generic_get_function_groups,
> +	.set_mux		= rza2_set_mux,
> +	.strict			= true,
> +};
> +
> +static int rza2_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct rza2_pinctrl_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->desc.name		= DRIVER_NAME;
> +	priv->desc.pctlops	= &rza2_pinctrl_ops;
> +	priv->desc.pmxops	= &rza2_pinmux_ops;
> +	priv->desc.owner	= THIS_MODULE;
> +
> +	ret = rza2_pinctrl_register(priv);
> +	if (ret)
> +		return ret;
> +
> +	pr_info("RZ/A2 pin controller registered\n");
nit: dev_info

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rza2_pinctrl_of_match[] = {
> +	{
> +		.compatible	= "renesas,r7s9210-pinctrl",
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver rza2_pinctrl_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = rza2_pinctrl_of_match,
> +	},
> +	.probe = rza2_pinctrl_probe,
> +};
> +
> +static int __init rza2_pinctrl_init(void)
> +{
> +	return platform_driver_register(&rza2_pinctrl_driver);
> +}
> +core_initcall(rza2_pinctrl_init);
> +
> +

nit: multiple empty lines

With this minor fixed, please add my
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
> +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.16.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

Thank you for your review....again ;)

On Tuesday, November 13, 2018, jacopo mondi wrote:
> I would prefer the reverse xmas order too, but I'm not saying out loud
> as I understand is something hard to enforce, as it's a very minor
> issue :)

The next version will be very festive!

        #
        #
  .#%###%##%##.
   .##%###%##.
    .%##%###.
     .#%##%.
      .###.
       .#.
        t

Christmas in Australia


> > +	if (!of_property_read_bool(np, "gpio-controller")) {
> > +		dev_info(priv->dev, "No gpio controller registered\n");
> > +		return 0;
> > +	}
> 
> The bindings say this is mandatory... What do you think, would that
> make sense to have no gpio-controller support for this driver (testing
> apart :)

Good point. I don't need to check for this.
If it doesn't work..."RTFM"


> > +	gpio_range.id = of_args.args[0];
> 
> Do you think of_args need to be validated?
> As I understand them, id and pin_base are fixed at 0, while the pin
> number depends on the soc version/revision.
> 
> Now I wonder if you would need a gpio-ranges property at all, but
> that's fine, it doesn't hurt...

My idea was that if another RZ/A2 with a different package size is 
created, or even a RZ/A3 with the same pin controller, nothing in the driver 
will have to change because the gpio-ranges is passed in and will tell 
you how many ports you have.

As for validating the values, the only thing I can really check is that:
  of_args.args[2] == RZA2_NPINS

Of course, now that I say that, I realize that if/when it does come time
to expand this driver beyond the 1 SOC that exists today, I will have 
to stop using that hard coded RZA2_NPINS value...but I'll deal with that 
when the time comes.


> > +	ret = rza2_pinctrl_register(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pr_info("RZ/A2 pin controller registered\n");
> nit: dev_info

Actually, I changed it to this to anticipate the day when more than one
SOC is supported:

	dev_info(&pdev->dev, "Registered ports P0 - P%c\n",
		 port_names[priv->desc.npins / RZA2_PINS_PER_PORT - 1]);


> With this minor fixed, please add my
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thank you.

Chris

Re: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Geert Uytterhoeven]

Hi Chris,

On Thu, Nov 15, 2018 at 12:41 AM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> > > +   gpio_range.id = of_args.args[0];
> >
> > Do you think of_args need to be validated?
> > As I understand them, id and pin_base are fixed at 0, while the pin
> > number depends on the soc version/revision.
> >
> > Now I wonder if you would need a gpio-ranges property at all, but
> > that's fine, it doesn't hurt...
>
> My idea was that if another RZ/A2 with a different package size is
> created, or even a RZ/A3 with the same pin controller, nothing in the driver
> will have to change because the gpio-ranges is passed in and will tell
> you how many ports you have.
>
> As for validating the values, the only thing I can really check is that:
>   of_args.args[2] == RZA2_NPINS
>
> Of course, now that I say that, I realize that if/when it does come time
> to expand this driver beyond the 1 SOC that exists today, I will have
> to stop using that hard coded RZA2_NPINS value...but I'll deal with that
> when the time comes.

Not that there is an inherent danger in taking of_args.args[2] without
validation to support future SoCs without driver changes.
port_names[] is a fixed size array, so an SoC with more pins will
cause memory access beyond the end of the array. Possibly there are
other locations that need to be changed.

So IMHO it's better to have explicit support for different SoCs using
different compatible values, so you can store the number of pins in
of_device_id.data, and validate against that.

> > > +   ret = rza2_pinctrl_register(priv);
> > > +   if (ret)
> > > +           return ret;
> > > +
> > > +   pr_info("RZ/A2 pin controller registered\n");
> > nit: dev_info
>
> Actually, I changed it to this to anticipate the day when more than one
> SOC is supported:
>
>         dev_info(&pdev->dev, "Registered ports P0 - P%c\n",
>                  port_names[priv->desc.npins / RZA2_PINS_PER_PORT - 1]);

See, if npins becomes larger, you'll overflow port_names[].

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Geert,

On Thursday, November 15, 2018, Geert Uytterhoeven wrote:
> > As for validating the values, the only thing I can really check is that:
> >   of_args.args[2] == RZA2_NPINS
> >
> > Of course, now that I say that, I realize that if/when it does come time
> > to expand this driver beyond the 1 SOC that exists today, I will have
> > to stop using that hard coded RZA2_NPINS value...but I'll deal with that
> > when the time comes.
> 
> Not that there is an inherent danger in taking of_args.args[2] without
> validation to support future SoCs without driver changes.
> port_names[] is a fixed size array, so an SoC with more pins will
> cause memory access beyond the end of the array. Possibly there are
> other locations that need to be changed.
> 
> So IMHO it's better to have explicit support for different SoCs using
> different compatible values, so you can store the number of pins in
> of_device_id.data, and validate against that.

I will fill in of_device_id.data with the number of ports for each SOC 
(only 1 SOC today) and then use that number throughout the driver instead 
of the hard coded number.
I'll also check that of_args.args[2] matches of_device_id.data.

Then, it should be real easy to add support for another SOC when it 
comes along. RZ/T1 which has the same controller has ports up to
Port "U" for example.

Chris

[PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Add device tree binding documentation and header file for Renesas R7S9210
(RZ/A2) SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v3:
 - Added Reviewed-by
v2:
 * Moved gpio-controller to required
 * Wrote a better description of what the sub-nodes are for
 * Added pinmux property description
 * Changed macro RZA2_PIN_ID to RZA2_PIN
---
 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 88 ++++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 ++++++++++++
 2 files changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
new file mode 100644
index 000000000000..622d37a7225b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
@@ -0,0 +1,88 @@
+Renesas RZ/A2 combined Pin and GPIO controller
+
+The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
+Pin multiplexing and GPIO configuration is performed on a per-pin basis.
+Each port features up to 8 pins, each of them configurable for GPIO
+function (port mode) or in alternate function mode.
+Up to 8 different alternate function modes exist for each single pin.
+
+Pin controller node
+-------------------
+
+Required properties:
+  - compatible: should be:
+    - "renesas,r7s9210-pinctrl": for RZ/A2M
+  - reg
+    Address base and length of the memory area where the pin controller
+    hardware is mapped to.
+  - gpio-controller
+    This pin controller also controls pins as GPIO
+  - #gpio-cells
+    Must be 2
+  - gpio-ranges
+    Expresses the total number GPIO ports/pins in this SoC
+
+
+Example: Pin controller node for RZ/A2M SoC (r7s9210)
+
+	pinctrl: pin-controller@fcffe000 {
+		compatible = "renesas,r7s9210-pinctrl";
+		reg = <0xfcffe000 0x9D1>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-ranges = <&pinctrl 0 0 176>;
+	};
+
+Sub-nodes
+---------
+
+The child nodes of the pin controller designate pins to be used for
+specific peripheral functions or as GPIO.
+
+- Pin multiplexing sub-nodes:
+  A pin multiplexing sub-node describes how to configure a set of
+  (or a single) pin in some desired alternate function mode.
+  The values for the pinmux properties are a combination of port name, pin
+  number and the desired function index. Use the RZA2_PINMUX macro located
+  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
+  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
+  to express the desired port pin.
+
+  Required properties:
+    - pinmux:
+      integer array representing pin number and pin multiplexing configuration.
+      When a pin has to be configured in alternate function mode, use this
+      property to identify the pin by its global index, and provide its
+      alternate function configuration number along with it.
+      When multiple pins are required to be configured as part of the same
+      alternate function they shall be specified as members of the same
+      argument list of a single "pinmux" property.
+      Helper macros to ease assembling the pin index from its position
+      (port where it sits on and pin number) and alternate function identifier
+      are provided by the pin controller header file at:
+      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>
+      Integers values in "pinmux" argument list are assembled as:
+      ((PORT * 8 + PIN) | MUX_FUNC << 16)
+
+  Example: Board specific pins configuration
+
+	&pinctrl {
+		/* Serial Console */
+		scif4_pins: serial4 {
+			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
+				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
+		};
+	};
+
+  Example: Assigning a GPIO:
+
+	leds {
+		status = "okay";
+		compatible = "gpio-leds";
+
+		led0 {
+			/* P6_0 */
+			gpios = <&pinctrl RZA2_PIN(P6, 0) GPIO_ACTIVE_HIGH>;
+		};
+	};
diff --git a/include/dt-bindings/pinctrl/r7s9210-pinctrl.h b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
new file mode 100644
index 000000000000..1e2671b61c0a
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Defines macros and constants for Renesas RZ/A2 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
+
+#define RZA2_PINS_PER_PORT	8
+
+/* Port names as labeled in the Hardware Manual */
+#define P0 0
+#define P1 1
+#define P2 2
+#define P3 3
+#define P4 4
+#define P5 5
+#define P6 6
+#define P7 7
+#define P8 8
+#define P9 9
+#define PA 10
+#define PB 11
+#define PC 12
+#define PD 13
+#define PE 14
+#define PF 15
+#define PG 16
+#define PH 17
+/* No I */
+#define PJ 18
+#define PK 19
+#define PL 20
+#define PM 21
+
+/*
+ * Create the pin index from its bank and position numbers and store in
+ * the upper 8 bits the alternate function identifier
+ */
+#define RZA2_PINMUX(b, p, f)	((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))
+
+/*
+ * Convert a port and pin label to its global pin index
+ */
+ #define RZA2_PIN(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))
+
+#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H */
-- 
2.16.1

Re: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Geert Uytterhoeven]

Hi Chris,

On Wed, Nov 7, 2018 at 7:27 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

> new file mode 100644
> index 000000000000..622d37a7225b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,88 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.
> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:
> +    - "renesas,r7s9210-pinctrl": for RZ/A2M

On RZ/A1, the datasheet called this "Ports", and the corresponding compatible
value is "renesas,r7s72100-ports".
On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-gpio"?
Hmm, then you may want to call the single node gpio-controller instead of
pin-controller (and all references to it)? It's really both.

On RZ/A1, it's different, as you have a single pin-controller node, with GPIO
subnodes.

> +  - reg
> +    Address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +  - gpio-controller
> +    This pin controller also controls pins as GPIO
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC

number of

> +
> +
> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +       pinctrl: pin-controller@fcffe000 {
> +               compatible = "renesas,r7s9210-pinctrl";
> +               reg = <0xfcffe000 0x9D1>;

0x9d1

BTW, that's a real odd number. What about rounding up to the hardware
granularity, i.e. 0x1000?

> +
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +               gpio-ranges = <&pinctrl 0 0 176>;
> +       };
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller designate pins to be used for
> +specific peripheral functions or as GPIO.
> +
> +- Pin multiplexing sub-nodes:
> +  A pin multiplexing sub-node describes how to configure a set of
> +  (or a single) pin in some desired alternate function mode.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h

RZA2_PIN

> +  to express the desired port pin.
> +
> +  Required properties:
> +    - pinmux:
> +      integer array representing pin number and pin multiplexing configuration.
> +      When a pin has to be configured in alternate function mode, use this
> +      property to identify the pin by its global index, and provide its
> +      alternate function configuration number along with it.
> +      When multiple pins are required to be configured as part of the same
> +      alternate function they shall be specified as members of the same
> +      argument list of a single "pinmux" property.
> +      Helper macros to ease assembling the pin index from its position
> +      (port where it sits on and pin number) and alternate function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>

include/dt-bindings/pinctrl/r7s9210-pinctrl.h (or
<dt-bindings/pinctrl/r7s9210-pinctrl.h>)

> +      Integers values in "pinmux" argument list are assembled as:
> +      ((PORT * 8 + PIN) | MUX_FUNC << 16)

> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/A2 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +
> +#define RZA2_PINS_PER_PORT     8
> +
> +/* Port names as labeled in the Hardware Manual */
> +#define P0 0
> +#define P1 1
> +#define P2 2
> +#define P3 3
> +#define P4 4
> +#define P5 5
> +#define P6 6
> +#define P7 7
> +#define P8 8
> +#define P9 9
> +#define PA 10
> +#define PB 11
> +#define PC 12

This may conflict with MIPS again ;-)
Oh, you don't include the bindings header in the driver source file, and doing
so would cause issues with (previous version of) the enum...

Still, would it make sense to call these PORTx instead of Px?
The register descriptions use PORTx.<reg>.

> +#define PD 13
> +#define PE 14
> +#define PF 15
> +#define PG 16
> +#define PH 17
> +/* No I */
> +#define PJ 18
> +#define PK 19
> +#define PL 20
> +#define PM 21

There's no PM in my datasheet. Should that be JP0?
Oh, the register descriptions do use PORTM.

> +
> +/*
> + * Create the pin index from its bank and position numbers and store in
> + * the upper 8 bits the alternate function identifier

These are not really the upper 8 bits, unless your wordsize is 24-bit ;-)

"upper 16 bits", like on RZ/A1?

> + */
> +#define RZA2_PINMUX(b, p, f)   ((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Hi Geert,

On Monday, November 12, 2018 1, Geert Uytterhoeven wrote:
> > +Required properties:
> > +  - compatible: should be:
> > +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> 
> On RZ/A1, the datasheet called this "Ports", and the corresponding
> compatible
> value is "renesas,r7s72100-ports".
> On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-
> gpio"?
> Hmm, then you may want to call the single node gpio-controller instead of
> pin-controller (and all references to it)? It's really both.
> 
> On RZ/A1, it's different, as you have a single pin-controller node, with
> GPIO
> subnodes.

So here's where we get into the interesting discussions.

You're going off the title of the chapters in the hardware manual. But, 
I'm looking at what the IP is (and where it was uses in other SoCs).

For RZ/A1, the pin controller/GPIO is a horrible piece of HW I've never 
seen before and hope to never see again.

For RZ/A2, the controllers came from the RZ/T1. In that manual, the 
chapter was call "Multi-Function Pin Controller (MPC)" (Chapter 18). The 
GPIO was in Chapter 17 called "I/O Ports".
Then for RZ/A2, they simply combined the two chapters into one since the
hardware was also 'combined' and just picked a name "GPIO". (they 
basically copy/pasted the text from the two chapters)

So, what's the rule of naming? Does it really have to match exactly what
it says in the hardware manual?
I'm assuming an RZ/A3 would use this same pin controller.


> > +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> > +
> > +       pinctrl: pin-controller@fcffe000 {
> > +               compatible = "renesas,r7s9210-pinctrl";
> > +               reg = <0xfcffe000 0x9D1>;
> 
> 0x9d1
> 
> BTW, that's a real odd number. What about rounding up to the hardware
> granularity, i.e. 0x1000?

I remember us getting into trouble once because numbers were rounded up 
or down and then conflicted with other nodes/register addresses. So, I 
was putting number exactly as they are. But if you want, I can round it 
up.


> > +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-
> pinctrl.h
> 
> RZA2_PIN

Good catch! Thank you.



> > +      are provided by the pin controller header file at:
> > +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>
> 
> include/dt-bindings/pinctrl/r7s9210-pinctrl.h (or
> <dt-bindings/pinctrl/r7s9210-pinctrl.h>)

Thanks.

> > +/* Port names as labeled in the Hardware Manual */
> > +#define P0 0
> > +#define P1 1
> > +#define P2 2
> > +#define P3 3
> > +#define P4 4
> > +#define P5 5
> > +#define P6 6
> > +#define P7 7
> > +#define P8 8
> > +#define P9 9
> > +#define PA 10
> > +#define PB 11
> > +#define PC 12
> 
> This may conflict with MIPS again ;-)

Damn MIPS!


> Oh, you don't include the bindings header in the driver source file, and
> doing
> so would cause issues with (previous version of) the enum...
> 
> Still, would it make sense to call these PORTx instead of Px?
> The register descriptions use PORTx.<reg>.

I liked Px because it made my lines in the device tree shorter.
But, I won't argue if you think it would be better to use PORTx (that's 
only 3 more characters).


> > +#define PM 21
> 
> There's no PM in my datasheet. Should that be JP0?
> Oh, the register descriptions do use PORTM.

Like you mentioned, they made it confusing because instead of calling 
the on pin on Port M "PM0" they called it "JP0" for 'JTAG PORT'.
From a hardware IP standpoint, it's a "Port M", so I wanted to call it 
that. I wanted to make it generic because if another SoC uses this same
controller, it might have more ports, so port M will really be a port M.


> > + * Create the pin index from its bank and position numbers and store in
> > + * the upper 8 bits the alternate function identifier
> 
> These are not really the upper 8 bits, unless your wordsize is 24-bit ;-)
> 
> "upper 16 bits", like on RZ/A1?

Opps. You're correct. Thanks!


Chris

Re: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Geert Uytterhoeven]

Hi Chris,

On Tue, Nov 13, 2018 at 5:38 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, November 12, 2018 1, Geert Uytterhoeven wrote:
> > > +Required properties:
> > > +  - compatible: should be:
> > > +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> >
> > On RZ/A1, the datasheet called this "Ports", and the corresponding
> > compatible
> > value is "renesas,r7s72100-ports".
> > On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-
> > gpio"?
> > Hmm, then you may want to call the single node gpio-controller instead of
> > pin-controller (and all references to it)? It's really both.
> >
> > On RZ/A1, it's different, as you have a single pin-controller node, with
> > GPIO
> > subnodes.
>
> So here's where we get into the interesting discussions.
>
> You're going off the title of the chapters in the hardware manual. But,
> I'm looking at what the IP is (and where it was uses in other SoCs).
>
> For RZ/A1, the pin controller/GPIO is a horrible piece of HW I've never
> seen before and hope to never see again.
>
> For RZ/A2, the controllers came from the RZ/T1. In that manual, the
> chapter was call "Multi-Function Pin Controller (MPC)" (Chapter 18). The
> GPIO was in Chapter 17 called "I/O Ports".
> Then for RZ/A2, they simply combined the two chapters into one since the
> hardware was also 'combined' and just picked a name "GPIO". (they
> basically copy/pasted the text from the two chapters)
>
> So, what's the rule of naming? Does it really have to match exactly what
> it says in the hardware manual?
> I'm assuming an RZ/A3 would use this same pin controller.

Thanks for the explanation!

I'm fine with "renesas,r7s9210-pinctrl".

> > > +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> > > +
> > > +       pinctrl: pin-controller@fcffe000 {
> > > +               compatible = "renesas,r7s9210-pinctrl";
> > > +               reg = <0xfcffe000 0x9D1>;
> >
> > 0x9d1
> >
> > BTW, that's a real odd number. What about rounding up to the hardware
> > granularity, i.e. 0x1000?
>
> I remember us getting into trouble once because numbers were rounded up
> or down and then conflicted with other nodes/register addresses. So, I
> was putting number exactly as they are. But if you want, I can round it
> up.

If you're worried for an overlap with another node in DT, keep on using
(lower case) 0x9d1.

The mapping will be rounded up to PAGE_SIZE anyway :-)

> > > +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-
> > pinctrl.h
> >
> > RZA2_PIN
>
> Good catch! Thank you.

Note that you still do have RZA2_PIN_ID_TO_PORT() and
RZA2_PIN_ID_TO_PIN() macros in the driver.
But RZA2_PIN_TO_PIN() sounds really lame...

> > > +/* Port names as labeled in the Hardware Manual */
> > > +#define P0 0
> > > +#define P1 1
> > > +#define P2 2
> > > +#define P3 3
> > > +#define P4 4
> > > +#define P5 5
> > > +#define P6 6
> > > +#define P7 7
> > > +#define P8 8
> > > +#define P9 9
> > > +#define PA 10
> > > +#define PB 11
> > > +#define PC 12
> >
> > This may conflict with MIPS again ;-)
>
> Damn MIPS!
>
>
> > Oh, you don't include the bindings header in the driver source file, and
> > doing
> > so would cause issues with (previous version of) the enum...
> >
> > Still, would it make sense to call these PORTx instead of Px?
> > The register descriptions use PORTx.<reg>.
>
> I liked Px because it made my lines in the device tree shorter.
> But, I won't argue if you think it would be better to use PORTx (that's
> only 3 more characters).

Any preference from the DT people?

> > > +#define PM 21
> >
> > There's no PM in my datasheet. Should that be JP0?
> > Oh, the register descriptions do use PORTM.
>
> Like you mentioned, they made it confusing because instead of calling
> the on pin on Port M "PM0" they called it "JP0" for 'JTAG PORT'.
> From a hardware IP standpoint, it's a "Port M", so I wanted to call it
> that. I wanted to make it generic because if another SoC uses this same
> controller, it might have more ports, so port M will really be a port M.

OK.

Perhaps adding a convenience definition

    #define JP PM

may be a good idea? Or just a comment?

    #define PM 21        /* JP */


Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Hi Geert,

On Tuesday, November 13, 2018 1, Geert Uytterhoeven wrote:
> Perhaps adding a convenience definition
> 
>     #define JP PM
> 
> may be a good idea? Or just a comment?
> 
>     #define PM 21        /* JP */

Either one is OK I guess. I'll see which one makes more sense as I 
reworked the driver.


> Anyway, we're getting closer to bikeshedding,

:)


Chris

Re: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Linus Walleij]

On Tue, Nov 13, 2018 at 6:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I assume you are queueing the RZ/A2 stuff as well Geert?
Acked-by: Linus Walleij <linus.walleij@linaro.org>
FWIW, I trust Geert to have the final say on Renesas code.

Yours,
Linus Walleij

Re: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Geert Uytterhoeven]

Hi Linus,

On Mon, Nov 19, 2018 at 2:22 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 13, 2018 at 6:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I assume you are queueing the RZ/A2 stuff as well Geert?

Yes, that's the plan...

> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> FWIW, I trust Geert to have the final say on Renesas code.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5979 bytes --]

Hi Chris,
    thanks for the patch

Just two minor things, so please add my
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>


On Wed, Nov 07, 2018 at 01:27:33PM -0500, Chris Brandt wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - Added Reviewed-by
> v2:
>  * Moved gpio-controller to required
>  * Wrote a better description of what the sub-nodes are for
>  * Added pinmux property description
>  * Changed macro RZA2_PIN_ID to RZA2_PIN
> ---
>  .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 88 ++++++++++++++++++++++
>  include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 ++++++++++++
>  2 files changed, 135 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
>  create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> new file mode 100644
> index 000000000000..622d37a7225b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,88 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.
> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:

s/should/shall ?

> +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> +  - reg
> +    Address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +  - gpio-controller
> +    This pin controller also controls pins as GPIO
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC
> +
> +

Two empty lines.

Thanks
   j

> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +	pinctrl: pin-controller@fcffe000 {
> +		compatible = "renesas,r7s9210-pinctrl";
> +		reg = <0xfcffe000 0x9D1>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-ranges = <&pinctrl 0 0 176>;
> +	};
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller designate pins to be used for
> +specific peripheral functions or as GPIO.
> +
> +- Pin multiplexing sub-nodes:
> +  A pin multiplexing sub-node describes how to configure a set of
> +  (or a single) pin in some desired alternate function mode.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
> +  to express the desired port pin.
> +
> +  Required properties:
> +    - pinmux:
> +      integer array representing pin number and pin multiplexing configuration.
> +      When a pin has to be configured in alternate function mode, use this
> +      property to identify the pin by its global index, and provide its
> +      alternate function configuration number along with it.
> +      When multiple pins are required to be configured as part of the same
> +      alternate function they shall be specified as members of the same
> +      argument list of a single "pinmux" property.
> +      Helper macros to ease assembling the pin index from its position
> +      (port where it sits on and pin number) and alternate function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>
> +      Integers values in "pinmux" argument list are assembled as:
> +      ((PORT * 8 + PIN) | MUX_FUNC << 16)
> +
> +  Example: Board specific pins configuration
> +
> +	&pinctrl {
> +		/* Serial Console */
> +		scif4_pins: serial4 {
> +			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
> +				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
> +		};
> +	};
> +
> +  Example: Assigning a GPIO:
> +
> +	leds {
> +		status = "okay";
> +		compatible = "gpio-leds";
> +
> +		led0 {
> +			/* P6_0 */
> +			gpios = <&pinctrl RZA2_PIN(P6, 0) GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> diff --git a/include/dt-bindings/pinctrl/r7s9210-pinctrl.h b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> new file mode 100644
> index 000000000000..1e2671b61c0a
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/A2 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +
> +#define RZA2_PINS_PER_PORT	8
> +
> +/* Port names as labeled in the Hardware Manual */
> +#define P0 0
> +#define P1 1
> +#define P2 2
> +#define P3 3
> +#define P4 4
> +#define P5 5
> +#define P6 6
> +#define P7 7
> +#define P8 8
> +#define P9 9
> +#define PA 10
> +#define PB 11
> +#define PC 12
> +#define PD 13
> +#define PE 14
> +#define PF 15
> +#define PG 16
> +#define PH 17
> +/* No I */
> +#define PJ 18
> +#define PK 19
> +#define PL 20
> +#define PM 21
> +
> +/*
> + * Create the pin index from its bank and position numbers and store in
> + * the upper 8 bits the alternate function identifier
> + */
> +#define RZA2_PINMUX(b, p, f)	((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))
> +
> +/*
> + * Convert a port and pin label to its global pin index
> + */
> + #define RZA2_PIN(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))
> +
> +#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H */
> --
> 2.16.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v4 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

On Tuesday, November 13, 2018, jacopo mondi wrote:
> Just two minor things, so please add my
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks Jacopo!

Chris