[RFC 0/5] Renesas RZ series pinctrl driver

[RFC 0/5] Renesas RZ series pinctrl driver

[Jacopo Mondi]

Hello,
   after having discussed in great detail the RZ series per-pin PFC hardware
peculiarities, this is a proposal for a possible pin-based pin controller
driver for SoC devices of Renesas RZ family.

This RFC series adds a minimal driver infrastructure which supports pin
multiplexing via explicit per-pin settings performed in device tree sources.

The driver is composed by a "core" module, which aims to be generic enough to
support different RZ SoC device, and a "SoC" module, which is instead specific
to the single device.
Right now, the only "SoC" module support implemented is for RZ/A1H (Genmai
and GR-Peach boards).

Why an "SoC" module, if a single SoC is supported then?
RZ devices with a pin-based PFC hardware have different register layouts and
available pin functions, the "SoC" module is requested to enumerate its pins,
and provide function for HW interfacing (currently only set_mux() is supported).
This should make adding support for new chips fairly easy.

One note on the current DT ABI:
right now a pin configuration is specified in DTS using utility macros defined
in the (currently undocumented)
arch/arm/boot/dts/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h header file.
Each pin configuration is a triplet of u32 in the form of

<BANK PIN ALTERNATE_FUNC_#>

It should be fairly easy adding additional parameters to configure
what was missing in the original group-based PFC driver for RZ devices
(I'm thinking of IO mode control, input buffer configuration, bi-directional
configuration etc).
Once these configuration parameters have been retrieved from the DTS, they can
be passed down to the "SoC" module, and let it deal with them as it does right
now with mux settings.

Currently, there is no support for pinconfig operations, and no GPIO support.
I'm planning to work on GPIO integration as soon as I have sent this out, but
I wanted to send this first to start collecting comments as soon as I could.

I have tested the correctness of mux settings printing out register values,
and enabling/disabling the SCIF2 module connected to serial debug interface.

The series makes use of newly introduced pin[ctrl|mux]_generic functions,
currently only available in Linus Walleij's linux-pinctrl.git tree.
I have merged the devel/ branch of that tree on top of Geert's renesas-drivers
master one.
The merge is available for clone at git://jmondi.org/linux in
jmondi/renesas/pinctrl-devel branch, for the interested ones.

Sending out as RFC, as I do expect lot of comments both from Renesas people that
has a deeper knowledge of other RZ Series SoCs than me, and from gpio/pinctrl
people as this is the first pin control driver I send out (so please bear with
me on this :)

Thank you
   j

Jacopo Mondi (5):
  pinctrl: rz-pfc: Add Renesas RZ pinctrl core module
  pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver
  arm: dts: dt-bindings: Add Renesas RZ pinctrl header
  arm: dts: r7s1000: Add pincontroller node
  arm: dts: genmai: Add SCIF2 pin group

 arch/arm/boot/dts/r7s72100-genmai.dts            |  13 +
 arch/arm/boot/dts/r7s72100.dtsi                  |  12 +
 drivers/pinctrl/Kconfig                          |   1 +
 drivers/pinctrl/Makefile                         |   1 +
 drivers/pinctrl/rz-pfc/Kconfig                   |  25 ++
 drivers/pinctrl/rz-pfc/Makefile                  |   2 +
 drivers/pinctrl/rz-pfc/pinctrl-rz.c              | 433 +++++++++++++++++++++++
 drivers/pinctrl/rz-pfc/pinctrl-rz.h              | 114 ++++++
 drivers/pinctrl/rz-pfc/pinctrl-rza1.c            | 346 ++++++++++++++++++
 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h |  19 +
 10 files changed, 966 insertions(+)
 create mode 100644 drivers/pinctrl/rz-pfc/Kconfig
 create mode 100644 drivers/pinctrl/rz-pfc/Makefile
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h

-- 
2.7.4

[RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Jacopo Mondi]

Add core module for per-pin Renesas RZ series pin controller.
The core module allows SoC driver to register their pins and SoC
specific operations and interfaces with pinctrl and pinmux core on their
behalf.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/pinctrl/Kconfig             |   1 +
 drivers/pinctrl/Makefile            |   1 +
 drivers/pinctrl/rz-pfc/Kconfig      |  18 ++
 drivers/pinctrl/rz-pfc/Makefile     |   1 +
 drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 ++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++
 6 files changed, 582 insertions(+)
 create mode 100644 drivers/pinctrl/rz-pfc/Kconfig
 create mode 100644 drivers/pinctrl/rz-pfc/Makefile
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..6d72e58 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -294,6 +294,7 @@ source "drivers/pinctrl/mvebu/Kconfig"
 source "drivers/pinctrl/nomadik/Kconfig"
 source "drivers/pinctrl/pxa/Kconfig"
 source "drivers/pinctrl/qcom/Kconfig"
+source "drivers/pinctrl/rz-pfc/Kconfig"
 source "drivers/pinctrl/samsung/Kconfig"
 source "drivers/pinctrl/sh-pfc/Kconfig"
 source "drivers/pinctrl/spear/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f43..96e7ece 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_MVEBU)	+= mvebu/
 obj-y				+= nomadik/
 obj-$(CONFIG_PINCTRL_PXA)	+= pxa/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
+obj-$(CONFIG_PINCTRL_RZ_PINCTRL)	+= rz-pfc/
 obj-$(CONFIG_PINCTRL_SAMSUNG)	+= samsung/
 obj-$(CONFIG_PINCTRL_SH_PFC)	+= sh-pfc/
 obj-$(CONFIG_PINCTRL_SPEAR)	+= spear/
diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig
new file mode 100644
index 0000000..3714c10
--- /dev/null
+++ b/drivers/pinctrl/rz-pfc/Kconfig
@@ -0,0 +1,18 @@
+#
+# Renesas RZ pinctrl drivers
+#
+
+if ARCH_RENESAS
+
+config PINCTRL_RZ_PINCTRL
+	select PINMUX
+	select PINCONF
+	select GPIOLIB
+	select GENERIC_PINCONF
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCTRL_GROUPS
+	def_bool y
+	help
+	  This enables pin control drivers for Renesas RZ platforms
+
+endif
diff --git a/drivers/pinctrl/rz-pfc/Makefile b/drivers/pinctrl/rz-pfc/Makefile
new file mode 100644
index 0000000..cba8283
--- /dev/null
+++ b/drivers/pinctrl/rz-pfc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PINCTRL_RZ_PINCTRL)		+= pinctrl-rz.o
diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.c b/drivers/pinctrl/rz-pfc/pinctrl-rz.c
new file mode 100644
index 0000000..3efbf03
--- /dev/null
+++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c
@@ -0,0 +1,447 @@
+/*
+ * Pinctrl support for Renesas RZ Series
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ * Copyright (C) 2017 Renesas Electronics Corporation
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "../core.h"
+#include "../devicetree.h"
+#include "../pinmux.h"
+
+#include "pinctrl-rz.h"
+
+#define DRIVER_NAME		"rz-pinctrl"
+#define RZ_PIN_ARGS_COUNT	3
+
+/**
+ * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position [bank:pin]
+ *
+ * This can be improved, as it walks all the pins reported by the SoC driver
+ *
+ * @return: pin number between [0 - npins]; -1 if not found
+ */
+static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl,
+				   unsigned int bank, unsigned int pin)
+{
+	struct rz_pinctrl_info *info;
+	struct rz_pin_desc *rz_pin;
+	int i, npins;
+
+	info = rz_pinctrl->info;
+	npins = info->npins;
+	for (i = 0; i < npins; ++i) {
+		rz_pin = &info->pins[i];
+
+		/*
+		 * return the pin index in the array, not the pin number,
+		 * so that we can access it easily when muxing group's pins
+		 */
+		if (rz_pin->bank == bank && rz_pin->pin == pin)
+			return i;
+	}
+
+	return -1;
+}
+
+/* ----------------------------------------------------------------------------
+ * pinctrl operations
+ */
+static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+			    unsigned int pin)
+{
+	struct rz_pinctrl_dev *rz_pinctrl;
+	struct rz_pinctrl_info *info;
+
+	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	info = rz_pinctrl->info;
+
+	seq_printf(s, "%s %s\n", info->pins[pin].name, DRIVER_NAME);
+}
+
+/**
+ * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups and
+ *			 functions
+ *
+ * Pins for RZ series pin controller described by "renesas-rz,pins" property
+ * are arrays of u32 values in the form:
+ *
+ * "renesas-rz,pins" = <BANK PIN MUX>, ... ;
+ *
+ * Parse the list arguments and identify pins through their position
+ * (bank and pin offset) and save the provided mux mode for later use.
+ *
+ * TODO: the array can be expended to support additional parameters for
+ *	 pin configurations values (IO direction etc)
+ *
+ * @pctldev: pin controller device
+ * @np_config: device tree node to parse
+ * @map: pointer to pin map (output)
+ * @num_maps: number of collected maps (output)
+ *
+ * @return: 0 for success; != 0 otherwise
+ */
+static int rz_dt_node_to_map(struct pinctrl_dev *pctldev,
+			     struct device_node *np_config,
+			     struct pinctrl_map **map, unsigned int *num_maps)
+{
+	struct rz_pinctrl_dev *rz_pinctrl;
+	const char *prop_name = "renesas-rz,pins",
+		   *grpname, **fngrps;
+	unsigned int *grpins,
+		     *mux_modes;
+	int i, npins, ret;
+
+	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	npins = pinctrl_count_index_with_args(np_config, prop_name);
+	if (npins <= 0) {
+		dev_err(rz_pinctrl->dev, "Missing pins configuration prop\n");
+		return -EINVAL;
+	}
+
+	mux_modes = devm_kzalloc(rz_pinctrl->dev, sizeof(*mux_modes) * npins,
+				 GFP_KERNEL);
+	grpins = devm_kzalloc(rz_pinctrl->dev, sizeof(*grpins) * npins,
+			      GFP_KERNEL);
+	if (unlikely(!grpins || !mux_modes))
+		return -ENOMEM;
+
+	/*
+	 * functions are made of 1 group only;
+	 * in facts, functions and groups are identical for RZ pin controller,
+	 * except that functions carry an array of mux modes (aka alternate
+	 * functions IDs)
+	 */
+	fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL);
+	if (unlikely(!fngrps)) {
+		ret = -ENOMEM;
+		goto free_pins;
+	}
+
+	*num_maps = 0;
+	(*map) = devm_kzalloc(rz_pinctrl->dev, sizeof(**map), GFP_KERNEL);
+	if (unlikely(!*map)) {
+		ret = -ENOMEM;
+		goto free_fngrps;
+	}
+
+	/* collect pin numbers and mux confs to create groups and functions */
+	for (i = 0; i < npins; ++i) {
+		struct of_phandle_args of_pins_args;
+		unsigned int bank, offs, mode, pin_idx;
+
+		/* DTS identifies pin by position: bank and pin offset */
+		ret = pinctrl_parse_index_with_args(np_config, prop_name, i,
+						    &of_pins_args);
+		if (ret)
+			goto free_map;
+
+		if (of_pins_args.args_count < RZ_PIN_ARGS_COUNT) {
+			dev_err(rz_pinctrl->dev,
+				"Wrong arguments number for renesas-rz,pins");
+			ret = -EINVAL;
+			goto free_map;
+		}
+
+		bank = of_pins_args.args[0];
+		offs = of_pins_args.args[1];
+		mode = of_pins_args.args[2];
+
+		/* pin_idx is the index on the static pin array */
+		pin_idx = rz_pinctrl_pos_to_index(rz_pinctrl, bank, offs);
+		if (pin_idx < 0) {
+			dev_err(rz_pinctrl->dev,
+				"Invalid pin position %d-%d\n", bank, offs);
+			ret = -EINVAL;
+			goto free_map;
+		}
+
+		grpins[i] = pin_idx;
+		mux_modes[i] = mode;
+	}
+
+	grpname = np_config->name;
+	fngrps[0] = grpname;
+
+	mutex_lock(&rz_pinctrl->mutex);
+	ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins, NULL);
+	if (ret) {
+		mutex_unlock(&rz_pinctrl->mutex);
+		goto free_map;
+	}
+
+	ret = pinmux_generic_add_function(pctldev, grpname, fngrps,
+					  1, mux_modes);
+	if (ret) {
+		mutex_unlock(&rz_pinctrl->mutex);
+		goto free_map;
+	}
+	mutex_unlock(&rz_pinctrl->mutex);
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = grpname;
+	(*map)->data.mux.function = grpname;
+	*num_maps = 1;
+
+	return 0;
+
+free_map:
+	devm_kfree(rz_pinctrl->dev, *map);
+free_fngrps:
+	devm_kfree(rz_pinctrl->dev, fngrps);
+free_pins:
+	devm_kfree(rz_pinctrl->dev, mux_modes);
+	devm_kfree(rz_pinctrl->dev, grpins);
+	return ret;
+}
+
+/**
+ * rz_dt_free_map()
+ */
+static void rz_dt_free_map(struct pinctrl_dev *pctldev,
+			   struct pinctrl_map *map, unsigned int num_maps)
+{
+	struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	devm_kfree(rz_pinctrl->dev, map);
+}
+
+static const struct pinctrl_ops rz_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,
+	.pin_dbg_show = rz_pin_dbg_show,
+	.dt_node_to_map = rz_dt_node_to_map,
+	.dt_free_map = rz_dt_free_map,
+};
+
+/* ----------------------------------------------------------------------------
+ * pinconfig operations
+ */
+
+/* TODO */
+
+/* ----------------------------------------------------------------------------
+ * pinmux operations
+ */
+
+/**
+ * rz_pinmux_set() - Retrieve pins from a group and apply mux settings
+ *
+ * @pctldev: pin controller device
+ * @selector: Function selector
+ * @group: Group selector
+ * @return: 0 for success; != otherwise
+ */
+static int rz_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector,
+			 unsigned int group)
+{
+	struct rz_pinctrl_dev *rz_pinctrl;
+	struct rz_pinctrl_info *info;
+	struct rz_pinctrl_ops *ops;
+	struct group_desc *grp;
+	struct function_desc *func;
+	unsigned int npins, *mux_modes;
+	int i;
+
+	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	info = rz_pinctrl->info;
+	ops = info->ops;
+
+	grp = pinctrl_generic_get_group(pctldev, group);
+	if (!grp)
+		return -EINVAL;
+
+	func = pinmux_generic_get_function(pctldev, selector);
+	if (!func)
+		return -EINVAL;
+
+	npins = grp->num_pins;
+	mux_modes = (unsigned int *)func->data;
+
+	for (i = 0; i < npins; ++i) {
+		unsigned int pin_idx, mux_mode;
+		struct rz_pin_desc *pin;
+		int ret;
+
+		/*
+		 * use pin index to retrieve pin descriptor;
+		 * then provide it to the set_mux SoC operation
+		 */
+		pin_idx = grp->pins[i];
+		pin = &info->pins[pin_idx];
+		mux_mode = mux_modes[i];
+
+		if (!ops || !ops->set_mux) {
+			dev_err(rz_pinctrl->dev, "Pin muxing not supported\n");
+			return -EINVAL;
+		}
+
+		ret = ops->set_mux(rz_pinctrl, pin, mux_mode);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+struct pinmux_ops rz_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 = rz_pinmux_set,
+};
+
+/* ----------------------------------------------------------------------------
+ * rz pinctrl operations
+ */
+
+/**
+ * rz_add_pins() - Enumerate pins reported by SoC driver in pin desc array
+ *
+ * @rz_pinctrl: RZ pincontroller
+ *
+ * @return: 0 for success; != 0 otherwise
+ */
+static int rz_add_pins(struct rz_pinctrl_dev *rz_pinctrl)
+{
+	struct rz_pinctrl_info *info;
+	struct rz_pin_desc *pin;
+	unsigned int npins, i;
+
+	info = rz_pinctrl->info;
+	npins = info->npins;
+
+	rz_pinctrl->pins = devm_kzalloc(rz_pinctrl->dev,
+					npins * sizeof(*rz_pinctrl->pins),
+					GFP_KERNEL);
+	if (unlikely(!rz_pinctrl->pins))
+		return -ENOMEM;
+
+	rz_pinctrl->desc.pins = rz_pinctrl->pins;
+	rz_pinctrl->desc.npins = npins;
+
+	for (i = 0; i < npins; ++i) {
+		pin = &info->pins[i];
+
+		rz_pinctrl->pins[i].number = pin->number;
+		rz_pinctrl->pins[i].name = pin->name;
+	}
+
+	return 0;
+}
+
+/**
+ * rz_pinctrl_map_res() - Map memory resources for pincontroller
+ *
+ * @pdev: platform device
+ * @rz_pincrl: RZ pincontroller
+ *
+ * @return: 0 for success; != 0 otherwise
+ */
+static int rz_pinctrl_map_res(struct platform_device *pdev,
+			      struct rz_pinctrl_dev *rz_pinctrl)
+{
+	struct resource *res;
+	struct rz_pinctrl_res *rz_res;
+	unsigned int i, nres;
+
+	nres = pdev->num_resources;
+	rz_res = devm_kzalloc(&pdev->dev, nres * sizeof(*rz_res),
+			      GFP_KERNEL);
+	if (unlikely(!rz_res))
+		return -ENOMEM;
+
+	rz_pinctrl->nres = nres;
+	rz_pinctrl->res = rz_res;
+
+	for (i = 0; i < nres; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			return -ENODEV;
+
+		rz_res = &rz_pinctrl->res[i];
+
+		rz_res->start = res->start;
+		rz_res->size = resource_size(res);
+		rz_res->base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(rz_res->base))
+			return PTR_ERR(rz_res->base);
+	}
+
+	return 0;
+}
+
+/**
+ * rz_pinctrl_probe() - Register pincontroller driver operations
+ *
+ * @pdev: platform device
+ * @info: SoC device info structure
+ */
+int rz_pinctrl_probe(struct platform_device *pdev, struct rz_pinctrl_info *info)
+{
+	int ret;
+	struct rz_pinctrl_dev *rz_pinctrl;
+
+	rz_pinctrl = devm_kzalloc(&pdev->dev, sizeof(*rz_pinctrl), GFP_KERNEL);
+	if (!rz_pinctrl)
+		return -ENOMEM;
+
+	rz_pinctrl->dev = &pdev->dev;
+	rz_pinctrl->info = info;
+
+	ret = rz_pinctrl_map_res(pdev, rz_pinctrl);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, rz_pinctrl);
+
+	rz_pinctrl->desc.name = DRIVER_NAME;
+	rz_pinctrl->desc.pctlops = &rz_pinctrl_ops;
+	rz_pinctrl->desc.pmxops = &rz_pinmux_ops;
+	rz_pinctrl->desc.owner = THIS_MODULE;
+
+	ret = rz_add_pins(rz_pinctrl);
+	if (ret)
+		return ret;
+
+	mutex_init(&rz_pinctrl->mutex);
+
+	ret = pinctrl_register_and_init(&rz_pinctrl->desc, rz_pinctrl->dev,
+					rz_pinctrl, &rz_pinctrl->pctl);
+	if (ret) {
+		dev_err(&pdev->dev, "could not register rz pinctrl driver\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+void rz_pinctrl_remove(struct platform_device *pdev)
+{
+	struct rz_pinctrl_dev *rz_pinctrl = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(rz_pinctrl->pctl);
+}
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
+MODULE_DESCRIPTION("Pinctrl driver for Reneas RZ series");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.h b/drivers/pinctrl/rz-pfc/pinctrl-rz.h
new file mode 100644
index 0000000..b873c89
--- /dev/null
+++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.h
@@ -0,0 +1,114 @@
+/*
+ * Pinctrl support for Renesas RZ Series
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ * Copyright (C) 2017 Renesas Electronics Corporation
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#ifndef __RZ_PINCTRL_H__
+#define __RZ_PINCTRL_H__
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/types.h>
+
+/* ----------------------------------------------------------------------------
+ * pinctrl-rz data types
+ */
+#define RZ_PIN_NAME(bank, pin)						\
+	PIN_##bank##_##pin
+
+#define RZ_PIN_DESC(b, p)						\
+	{ .number = RZ_PIN_NAME(b, p),					\
+	  .name = __stringify(RZ_PIN_NAME(b, p)),			\
+	  .bank = b, .pin = p }
+
+/**
+ * rz_pin_desc - Single RZ pin descriptor
+ *
+ * @number: pin number as enumerated by SoC driver
+ * @name: pin name as reported by SoC driver
+ * @bank: pin bank location
+ * @pin: pin register offset
+ */
+struct rz_pin_desc {
+	unsigned int bank, pin;
+	unsigned int number;
+	const char *name;
+};
+
+/**
+ * rz_pinctrl_info - SoC info data
+ *
+ * @ops: SoC specific operations
+ * @npins: number of pins
+ * @pins: pin array
+ */
+struct rz_pinctrl_ops;
+struct rz_pinctrl_info {
+	struct rz_pinctrl_ops *ops;
+
+	unsigned int npins;
+	struct rz_pin_desc *pins;
+};
+
+/**
+ * rz_pinctrl_res - Memory resource for RZ SoC
+ *
+ * @start: physical address base
+ * @size: memory region size
+ * @base: logical address base
+ */
+struct rz_pinctrl_res {
+	resource_size_t start;
+	resource_size_t size;
+	void __iomem *base;
+};
+
+/**
+ * rz_pinctrl_dev - RZ pincontroller device
+ *
+ * @dev: device
+ * @info: SoC data
+ * @nres: number of memory regions
+ * @res: memory regions
+ * @mutex: protect pin*_generic functions
+ * @pins: pin array for pinctrl core
+ * @desc: pincontroller desc for pinctrl core
+ * @pctl: pinctrl device
+ */
+struct rz_pinctrl_dev {
+	struct device *dev;
+
+	struct rz_pinctrl_info *info;
+
+	unsigned int nres;
+	struct rz_pinctrl_res *res;
+
+	struct mutex mutex;
+
+	struct pinctrl_pin_desc *pins;
+	struct pinctrl_desc desc;
+	struct pinctrl_dev *pctl;
+};
+
+/**
+ * rz_pinctrl_ops - SoC operations
+ *
+ * @set_mux: perform pin muxing on SoC registers
+ */
+struct rz_pinctrl_ops {
+	int (*set_mux)(struct rz_pinctrl_dev *, struct rz_pin_desc *,
+		       unsigned int mux_mode);
+};
+
+/* ----------------------------------------------------------------------------
+ * pinctrl-rz prototypes
+ */
+int rz_pinctrl_probe(struct platform_device *pdev,
+		     struct rz_pinctrl_info *info);
+void rz_pinctrl_remove(struct platform_device *pdev);
+
+#endif
-- 
2.7.4

[RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Jacopo Mondi]

Add pin controller driver for Renesas RZ/A1 SoC.
The SoC driver registers to rz-pfc core module and provides pin
description array and SoC specific pin mux operation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/pinctrl/rz-pfc/Kconfig        |   7 +
 drivers/pinctrl/rz-pfc/Makefile       |   1 +
 drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 ++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c

diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig
index 3714c10..2b9c111 100644
--- a/drivers/pinctrl/rz-pfc/Kconfig
+++ b/drivers/pinctrl/rz-pfc/Kconfig
@@ -15,4 +15,11 @@ config PINCTRL_RZ_PINCTRL
 	help
 	  This enables pin control drivers for Renesas RZ platforms
 
+config PINCTRL_RZA1_PINCTRL
+    depends on ARCH_R7S72100
+	select PINCTRL_RZ_PINCTRL
+	def_bool y
+	help
+	  This enables pin control drivers for Renesas RZ/A1 SoC
+
 endif
diff --git a/drivers/pinctrl/rz-pfc/Makefile b/drivers/pinctrl/rz-pfc/Makefile
index cba8283..e3befa5 100644
--- a/drivers/pinctrl/rz-pfc/Makefile
+++ b/drivers/pinctrl/rz-pfc/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_PINCTRL_RZ_PINCTRL)		+= pinctrl-rz.o
+obj-$(CONFIG_PINCTRL_RZA1_PINCTRL)		+= pinctrl-rza1.o
diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
new file mode 100644
index 0000000..221f048
--- /dev/null
+++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
@@ -0,0 +1,347 @@
+/*
+ * Pinctrl support for Renesas RZ/A1 SoC
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ * Copyright (C) 2017 Renesas Electronics Corporation
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#include "pinctrl-rz.h"
+
+#define RZA1_REG_DBG
+
+/* memory resources indexes */
+#define MEM_RES_LOW	0 /* PORTn_base */
+#define MEM_RES_HIGH	1 /* PORTn_base + 0x4000 */
+
+/* displacements from PORTn_base */
+#define PMC_REG		0x400
+#define PFC_REG		0x500
+#define PFCE_REG	0x600
+#define PFCEA_REG	0xA00
+
+/* displacements from PORTn_base + 0x4000 */
+#define PIBC_REG	0x000
+#define PBDC_REG	0x100
+#define PIPC_REG	0x200
+
+/* build register address using memory base, offset and bank number */
+#define RZA1_ADDR(mem_base, reg, bank)	\
+	((mem_base) + (reg) + (bank * 4))
+
+/* ----------------------------------------------------------------------------
+ * pin enumeration
+ */
+enum rza1_pins {
+	/* Port 0 */
+	RZ_PIN_NAME(0, 0) = 0, RZ_PIN_NAME(0, 1), RZ_PIN_NAME(0, 2),
+	RZ_PIN_NAME(0, 3), RZ_PIN_NAME(0, 4), RZ_PIN_NAME(0, 5),
+
+	/* Port 1 */
+	RZ_PIN_NAME(1, 0), RZ_PIN_NAME(1, 1), RZ_PIN_NAME(1, 2),
+	RZ_PIN_NAME(1, 3), RZ_PIN_NAME(1, 4), RZ_PIN_NAME(1, 5),
+	RZ_PIN_NAME(1, 6), RZ_PIN_NAME(1, 7), RZ_PIN_NAME(1, 8),
+	RZ_PIN_NAME(1, 9), RZ_PIN_NAME(1, 10), RZ_PIN_NAME(1, 11),
+	RZ_PIN_NAME(1, 12), RZ_PIN_NAME(1, 13), RZ_PIN_NAME(1, 14),
+	RZ_PIN_NAME(1, 15),
+
+	/* Port 2 */
+	RZ_PIN_NAME(2, 0), RZ_PIN_NAME(2, 1), RZ_PIN_NAME(2, 2),
+	RZ_PIN_NAME(2, 3), RZ_PIN_NAME(2, 4), RZ_PIN_NAME(2, 5),
+	RZ_PIN_NAME(2, 6), RZ_PIN_NAME(2, 7), RZ_PIN_NAME(2, 8),
+	RZ_PIN_NAME(2, 9), RZ_PIN_NAME(2, 10), RZ_PIN_NAME(2, 11),
+	RZ_PIN_NAME(2, 12), RZ_PIN_NAME(2, 13), RZ_PIN_NAME(2, 14),
+	RZ_PIN_NAME(2, 15),
+
+	/* Port 3 */
+	RZ_PIN_NAME(3, 0), RZ_PIN_NAME(3, 1), RZ_PIN_NAME(3, 2),
+	RZ_PIN_NAME(3, 3), RZ_PIN_NAME(3, 4), RZ_PIN_NAME(3, 5),
+	RZ_PIN_NAME(3, 6), RZ_PIN_NAME(3, 7), RZ_PIN_NAME(3, 8),
+	RZ_PIN_NAME(3, 9), RZ_PIN_NAME(3, 10), RZ_PIN_NAME(3, 11),
+	RZ_PIN_NAME(3, 12), RZ_PIN_NAME(3, 13), RZ_PIN_NAME(3, 14),
+	RZ_PIN_NAME(3, 15),
+
+	/* Port 4 */
+	RZ_PIN_NAME(4, 0), RZ_PIN_NAME(4, 1), RZ_PIN_NAME(4, 2),
+	RZ_PIN_NAME(4, 3), RZ_PIN_NAME(4, 4), RZ_PIN_NAME(4, 5),
+	RZ_PIN_NAME(4, 6), RZ_PIN_NAME(4, 7), RZ_PIN_NAME(4, 8),
+	RZ_PIN_NAME(4, 9), RZ_PIN_NAME(4, 10), RZ_PIN_NAME(4, 11),
+	RZ_PIN_NAME(4, 12), RZ_PIN_NAME(4, 13), RZ_PIN_NAME(4, 14),
+	RZ_PIN_NAME(4, 15),
+
+	/* Port 5 */
+	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
+	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
+	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
+	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
+
+	/* Port 6 */
+	RZ_PIN_NAME(6, 0), RZ_PIN_NAME(6, 1), RZ_PIN_NAME(6, 2),
+	RZ_PIN_NAME(6, 3), RZ_PIN_NAME(6, 4), RZ_PIN_NAME(6, 5),
+	RZ_PIN_NAME(6, 6), RZ_PIN_NAME(6, 7), RZ_PIN_NAME(6, 8),
+	RZ_PIN_NAME(6, 9), RZ_PIN_NAME(6, 10), RZ_PIN_NAME(6, 11),
+	RZ_PIN_NAME(6, 12), RZ_PIN_NAME(6, 13), RZ_PIN_NAME(6, 14),
+	RZ_PIN_NAME(6, 15),
+
+	/* Port 7 */
+	RZ_PIN_NAME(7, 0), RZ_PIN_NAME(7, 1), RZ_PIN_NAME(7, 2),
+	RZ_PIN_NAME(7, 3), RZ_PIN_NAME(7, 4), RZ_PIN_NAME(7, 5),
+	RZ_PIN_NAME(7, 6), RZ_PIN_NAME(7, 7), RZ_PIN_NAME(7, 8),
+	RZ_PIN_NAME(7, 9), RZ_PIN_NAME(7, 10), RZ_PIN_NAME(7, 11),
+	RZ_PIN_NAME(7, 12), RZ_PIN_NAME(7, 13), RZ_PIN_NAME(7, 14),
+	RZ_PIN_NAME(7, 15),
+
+	/* Port 8 */
+	RZ_PIN_NAME(8, 0), RZ_PIN_NAME(8, 1), RZ_PIN_NAME(8, 2),
+	RZ_PIN_NAME(8, 3), RZ_PIN_NAME(8, 4), RZ_PIN_NAME(8, 5),
+	RZ_PIN_NAME(8, 6), RZ_PIN_NAME(8, 7), RZ_PIN_NAME(8, 8),
+	RZ_PIN_NAME(8, 9), RZ_PIN_NAME(8, 10), RZ_PIN_NAME(8, 11),
+	RZ_PIN_NAME(8, 12), RZ_PIN_NAME(8, 13), RZ_PIN_NAME(8, 14),
+	RZ_PIN_NAME(8, 15),
+
+	/* Port 9 */
+	RZ_PIN_NAME(9, 0), RZ_PIN_NAME(9, 1), RZ_PIN_NAME(9, 2),
+	RZ_PIN_NAME(9, 3), RZ_PIN_NAME(9, 4), RZ_PIN_NAME(9, 5),
+	RZ_PIN_NAME(9, 6), RZ_PIN_NAME(9, 7),
+
+	/* Port 10 */
+	RZ_PIN_NAME(10, 0), RZ_PIN_NAME(10, 1), RZ_PIN_NAME(10, 2),
+	RZ_PIN_NAME(10, 3), RZ_PIN_NAME(10, 4), RZ_PIN_NAME(10, 5),
+	RZ_PIN_NAME(10, 6), RZ_PIN_NAME(10, 7), RZ_PIN_NAME(10, 8),
+	RZ_PIN_NAME(10, 9), RZ_PIN_NAME(10, 10), RZ_PIN_NAME(10, 11),
+	RZ_PIN_NAME(10, 12), RZ_PIN_NAME(10, 13), RZ_PIN_NAME(10, 14),
+	RZ_PIN_NAME(10, 15),
+
+	/* Port 10 */
+	RZ_PIN_NAME(11, 0), RZ_PIN_NAME(11, 1), RZ_PIN_NAME(11, 2),
+	RZ_PIN_NAME(11, 3), RZ_PIN_NAME(11, 4), RZ_PIN_NAME(11, 5),
+	RZ_PIN_NAME(11, 6), RZ_PIN_NAME(11, 7), RZ_PIN_NAME(11, 8),
+	RZ_PIN_NAME(11, 9), RZ_PIN_NAME(11, 10), RZ_PIN_NAME(11, 11),
+	RZ_PIN_NAME(11, 12), RZ_PIN_NAME(11, 13), RZ_PIN_NAME(11, 14),
+	RZ_PIN_NAME(11, 15),
+};
+
+struct rz_pin_desc pins[] = {
+	/* Port 0 */
+	RZ_PIN_DESC(0, 0), RZ_PIN_DESC(0, 1), RZ_PIN_DESC(0, 2),
+	RZ_PIN_DESC(0, 3), RZ_PIN_DESC(0, 4), RZ_PIN_DESC(0, 5),
+
+	/* Port 1 */
+	RZ_PIN_DESC(1, 0), RZ_PIN_DESC(1, 1), RZ_PIN_DESC(1, 2),
+	RZ_PIN_DESC(1, 3), RZ_PIN_DESC(1, 4), RZ_PIN_DESC(1, 5),
+	RZ_PIN_DESC(1, 6), RZ_PIN_DESC(1, 7), RZ_PIN_DESC(1, 8),
+	RZ_PIN_DESC(1, 9), RZ_PIN_DESC(1, 10), RZ_PIN_DESC(1, 11),
+	RZ_PIN_DESC(1, 12), RZ_PIN_DESC(1, 13), RZ_PIN_DESC(1, 14),
+	RZ_PIN_DESC(1, 15),
+
+	/* Port 2 */
+	RZ_PIN_DESC(2, 0), RZ_PIN_DESC(2, 1), RZ_PIN_DESC(2, 2),
+	RZ_PIN_DESC(2, 3), RZ_PIN_DESC(2, 4), RZ_PIN_DESC(2, 5),
+	RZ_PIN_DESC(2, 6), RZ_PIN_DESC(2, 7), RZ_PIN_DESC(2, 8),
+	RZ_PIN_DESC(2, 9), RZ_PIN_DESC(2, 10), RZ_PIN_DESC(2, 11),
+	RZ_PIN_DESC(2, 12), RZ_PIN_DESC(2, 13), RZ_PIN_DESC(2, 14),
+	RZ_PIN_DESC(2, 15),
+
+	/* Port 3 */
+	RZ_PIN_DESC(3, 0), RZ_PIN_DESC(3, 1), RZ_PIN_DESC(3, 2),
+	RZ_PIN_DESC(3, 3), RZ_PIN_DESC(3, 4), RZ_PIN_DESC(3, 5),
+	RZ_PIN_DESC(3, 6), RZ_PIN_DESC(3, 7), RZ_PIN_DESC(3, 8),
+	RZ_PIN_DESC(3, 9), RZ_PIN_DESC(3, 10), RZ_PIN_DESC(3, 11),
+	RZ_PIN_DESC(3, 12), RZ_PIN_DESC(3, 13), RZ_PIN_DESC(3, 14),
+	RZ_PIN_DESC(3, 15),
+
+	/* Port 4 */
+	RZ_PIN_DESC(4, 0), RZ_PIN_DESC(4, 1), RZ_PIN_DESC(4, 2),
+	RZ_PIN_DESC(4, 3), RZ_PIN_DESC(4, 4), RZ_PIN_DESC(4, 5),
+	RZ_PIN_DESC(4, 6), RZ_PIN_DESC(4, 7), RZ_PIN_DESC(4, 8),
+	RZ_PIN_DESC(4, 9), RZ_PIN_DESC(4, 10), RZ_PIN_DESC(4, 11),
+	RZ_PIN_DESC(4, 12), RZ_PIN_DESC(4, 13), RZ_PIN_DESC(4, 14),
+	RZ_PIN_DESC(4, 15),
+
+	/* Port 5 */
+	RZ_PIN_DESC(5, 0), RZ_PIN_DESC(5, 1), RZ_PIN_DESC(5, 2),
+	RZ_PIN_DESC(5, 3), RZ_PIN_DESC(5, 4), RZ_PIN_DESC(5, 5),
+	RZ_PIN_DESC(5, 6), RZ_PIN_DESC(5, 7), RZ_PIN_DESC(5, 8),
+	RZ_PIN_DESC(5, 9), RZ_PIN_DESC(5, 10),
+
+	/* Port 6 */
+	RZ_PIN_DESC(6, 0), RZ_PIN_DESC(6, 1), RZ_PIN_DESC(6, 2),
+	RZ_PIN_DESC(6, 3), RZ_PIN_DESC(6, 4), RZ_PIN_DESC(6, 5),
+	RZ_PIN_DESC(6, 6), RZ_PIN_DESC(6, 7), RZ_PIN_DESC(6, 8),
+	RZ_PIN_DESC(6, 9), RZ_PIN_DESC(6, 10), RZ_PIN_DESC(6, 11),
+	RZ_PIN_DESC(6, 12), RZ_PIN_DESC(6, 13), RZ_PIN_DESC(6, 14),
+	RZ_PIN_DESC(6, 15),
+
+	/* Port 7 */
+	RZ_PIN_DESC(7, 0), RZ_PIN_DESC(7, 1), RZ_PIN_DESC(7, 2),
+	RZ_PIN_DESC(7, 3), RZ_PIN_DESC(7, 4), RZ_PIN_DESC(7, 5),
+	RZ_PIN_DESC(7, 6), RZ_PIN_DESC(7, 7), RZ_PIN_DESC(7, 8),
+	RZ_PIN_DESC(7, 9), RZ_PIN_DESC(7, 10), RZ_PIN_DESC(7, 11),
+	RZ_PIN_DESC(7, 12), RZ_PIN_DESC(7, 13), RZ_PIN_DESC(7, 14),
+	RZ_PIN_DESC(7, 15),
+
+	/* Port 8 */
+	RZ_PIN_DESC(8, 0), RZ_PIN_DESC(8, 1), RZ_PIN_DESC(8, 2),
+	RZ_PIN_DESC(8, 3), RZ_PIN_DESC(8, 4), RZ_PIN_DESC(8, 5),
+	RZ_PIN_DESC(8, 6), RZ_PIN_DESC(8, 7), RZ_PIN_DESC(8, 8),
+	RZ_PIN_DESC(8, 9), RZ_PIN_DESC(8, 10), RZ_PIN_DESC(8, 11),
+	RZ_PIN_DESC(8, 12), RZ_PIN_DESC(8, 13), RZ_PIN_DESC(8, 14),
+	RZ_PIN_DESC(8, 15),
+
+	/* Port 9 */
+	RZ_PIN_DESC(9, 0), RZ_PIN_DESC(9, 1), RZ_PIN_DESC(9, 2),
+	RZ_PIN_DESC(9, 3), RZ_PIN_DESC(9, 4), RZ_PIN_DESC(9, 5),
+	RZ_PIN_DESC(9, 6), RZ_PIN_DESC(9, 7),
+
+	/* Port 10 */
+	RZ_PIN_DESC(10, 0), RZ_PIN_DESC(10, 1), RZ_PIN_DESC(10, 2),
+	RZ_PIN_DESC(10, 3), RZ_PIN_DESC(10, 4), RZ_PIN_DESC(10, 5),
+	RZ_PIN_DESC(10, 6), RZ_PIN_DESC(10, 7), RZ_PIN_DESC(10, 8),
+	RZ_PIN_DESC(10, 9), RZ_PIN_DESC(10, 10), RZ_PIN_DESC(10, 11),
+	RZ_PIN_DESC(10, 12), RZ_PIN_DESC(10, 13), RZ_PIN_DESC(10, 14),
+	RZ_PIN_DESC(10, 15),
+
+	/* Port 11 */
+	RZ_PIN_DESC(11, 0), RZ_PIN_DESC(11, 1), RZ_PIN_DESC(11, 2),
+	RZ_PIN_DESC(11, 3), RZ_PIN_DESC(11, 4), RZ_PIN_DESC(11, 5),
+	RZ_PIN_DESC(11, 6), RZ_PIN_DESC(11, 7), RZ_PIN_DESC(11, 8),
+	RZ_PIN_DESC(11, 9), RZ_PIN_DESC(11, 10), RZ_PIN_DESC(11, 11),
+	RZ_PIN_DESC(11, 12), RZ_PIN_DESC(11, 13), RZ_PIN_DESC(11, 14),
+	RZ_PIN_DESC(11, 15),
+};
+
+/* ----------------------------------------------------------------------------
+ * SoC operations
+ */
+
+static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
+				int bank, int pin, int set)
+{
+	void __iomem *mem = RZA1_ADDR(res->base, reg, bank);
+	u16 val = set ? ioread16(mem) | 1 << pin :
+			ioread16(mem) & ~(1 << pin);
+#ifdef RZA1_REG_DBG
+	u16 temp = ioread16(mem);
+
+	pr_err("%p %p %p - %4x %4x\n",
+	       (void *)res->start, res->base, mem, temp, val);
+#endif
+	iowrite16(val, mem);
+}
+
+/**
+ * rza1_set_mux() - Configure alternate function settings for the selected pin
+ *
+ * @pinctrl: RZ pincontroller
+ * @pin: Pin to configure
+ * @mux_mode: Alternate function number
+ *
+ * @return: 0 for success; != 0 otherwise
+ */
+static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl,
+			struct rz_pin_desc *pin_desc, unsigned int mux_mode)
+{
+	struct rz_pinctrl_res *res;
+	unsigned int bank = pin_desc->bank,
+		     pin = pin_desc->pin;
+
+	/*
+	 * disable input buffer and bi-control direction before entering
+	 * alternate mode and let alternate function drive the IO mode by
+	 * setting PIPCn to 1
+	 */
+	res = &pinctrl->res[MEM_RES_HIGH];
+	rza1_set_bit(res, PIBC_REG, bank, pin, 0);
+	rza1_set_bit(res, PBDC_REG, bank, pin, 0);
+	rza1_set_bit(res, PIPC_REG, bank, pin, 1);
+
+	/* TODO:
+	 * all alternate functions except a few (3) need PIPCn = 1;
+	 * find a way to identify those 3 functions, do not set PIPCn to 1
+	 * and set PMn according to some flag passed as parameter from DTS
+	 */
+
+	/*
+	 * enable alternate function mode and select it.
+	 *
+	 * ----------------------------------------------------
+	 * Alternate mode selection table:
+	 *
+	 * PMC	PFC	PFCE	PFCAE	AlternateMode mux_mode
+	 * 1	0	0	0	1		0
+	 * 1	1	0	0	2		1
+	 * 1	0	1	0	3		2
+	 * 1	1	1	0	4		3
+	 * 1	0	0	1	5		4
+	 * 1	1	0	1	6		5
+	 * 1	0	1	1	7		6
+	 * 1	1	1	1	8		7
+	 * ----------------------------------------------------
+	 */
+	res = &pinctrl->res[MEM_RES_LOW];
+	rza1_set_bit(res, PMC_REG, bank, pin, 1);
+	rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1);
+	rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2);
+	rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4);
+
+	return 0;
+}
+
+static struct rz_pinctrl_ops rza1_pinctrl_ops = {
+	.set_mux = rza1_set_mux,
+};
+
+static struct rz_pinctrl_info rza1_info = {
+	.ops = &rza1_pinctrl_ops,
+
+	.npins = ARRAY_SIZE(pins),
+	.pins = pins,
+};
+
+static const struct of_device_id rza1_pinctrl_of_match[] = {
+	{ .compatible = "renesas,rza1-pinctrl", },
+	{ }
+};
+
+static int rza1_pinctrl_probe(struct platform_device *pdev)
+{
+	return rz_pinctrl_probe(pdev, &rza1_info);
+}
+
+static int rza1_pinctrl_remove(struct platform_device *pdev)
+{
+	rz_pinctrl_remove(pdev);
+
+	return 0;
+}
+
+static struct platform_driver rza1_pinctrl_driver = {
+	.driver = {
+		.name = "rza1_pinctrl_driver",
+		.of_match_table = rza1_pinctrl_of_match,
+	},
+	.probe = rza1_pinctrl_probe,
+	.remove = rza1_pinctrl_remove,
+};
+
+static int __init rza1_pinctrl_init(void)
+{
+	return platform_driver_register(&rza1_pinctrl_driver);
+}
+core_initcall(rza1_pinctrl_init);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
+MODULE_DESCRIPTION("Pinctl driver for Reneas RZ/A1 SoC");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Jacopo Mondi]

Add dt-bindings header for Renesas RZ pincontroller.
The header defines macros for pin description and alternate function
numbers.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h

diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
new file mode 100644
index 0000000..92816d4
--- /dev/null
+++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
@@ -0,0 +1,19 @@
+/*
+ * Defines macros and constants for Renesas RZ pin controller and muxer
+ */
+
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
+
+#define RZ_PIN(b, p) b p
+
+#define ALTERNATE_FUNC_1	0
+#define ALTERNATE_FUNC_2	1
+#define ALTERNATE_FUNC_3	2
+#define ALTERNATE_FUNC_4	3
+#define ALTERNATE_FUNC_5	4
+#define ALTERNATE_FUNC_6	5
+#define ALTERNATE_FUNC_7	6
+#define ALTERNATE_FUNC_8	7
+
+#endif
-- 
2.7.4

[RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Jacopo Mondi]

Add pincontroller node compatible with the new Renesas RZ/A1
pincontroller driver.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 3dd427d..764006d 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -171,6 +171,18 @@
 		};
 	};
 
+	pinctrl: pinctrl@fcfe3000 {
+		compatible = "renesas,rza1-pinctrl";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		#pinctrl-cells = <2>;
+
+		reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */
+		      <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */
+		      <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */
+	};
+
 	scif0: serial@e8007000 {
 		compatible = "renesas,scif-r7s72100", "renesas,scif";
 		reg = <0xe8007000 64>;
-- 
2.7.4

[RFC 5/5] arm: dts: genmai: Add SCIF2 pin group

[Jacopo Mondi]

Add  TxD and RxD pin configuration for SCIF2 serial communication
interface on r7s72100 Genmai board.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 118a8e2..ea649c7 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -11,6 +11,7 @@
 
 /dts-v1/;
 #include "r7s72100.dtsi"
+#include "include/dt-bindings/pinctrl/pinctrl-renesas-rz.h"
 
 / {
 	model = "Genmai";
@@ -34,6 +35,18 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 	};
+
+};
+
+&pinctrl {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif2_pins>;
+
+	scif2_pins: serial2 {
+		/* P3_0 as TxD2; P3_2 as RxD2 */
+		renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,
+				  <RZ_PIN(3, 2) ALTERNATE_FUNC_4>;
+	};
 };
 
 &extal_clk {
-- 
2.7.4

RE: [RFC 0/5] Renesas RZ series pinctrl driver

[Chris Brandt]

Hi Jacopo,

Thanks for the patches.

On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> Right now, the only "SoC" module support implemented is for RZ/A1H (Genmai
> and GR-Peach boards).

I'm going to give it a try on the RZ/A1 RSK board.


> I have tested the correctness of mux settings printing out register values,
> and enabling/disabling the SCIF2 module connected to serial debug
> interface.

I can give it a try on things like Eth, I2C, SPI, SDHI, MMC.


> One note on the current DT ABI:
> right now a pin configuration is specified in DTS using utility macros
> defined in the (currently undocumented) arch/arm/boot/dts/include/dt-
> bindings/pinctrl/pinctrl-renesas-rz.h header file.
> Each pin configuration is a triplet of u32 in the form of
> 
> <BANK PIN ALTERNATE_FUNC_#>
> 
> It should be fairly easy adding additional parameters to configure what
> was missing in the original group-based PFC driver for RZ devices (I'm
> thinking of IO mode control, input buffer configuration, bi-directional
> configuration etc).

SDHI is one that will need another parameter. Half the pins need Bidirection
(PBDC) enabled, and the other half need PBDC disabled.
SDHI data pins are PBDC=1, the others are PBDC=0.


> The series makes use of newly introduced pin[ctrl|mux]_generic functions,
> currently only available in Linus Walleij's linux-pinctrl.git tree.

Hmmm, I was hoping to 'easily' back port this to 4.9.
I guess if it's the best way forward, then maybe I can back port those new
generic functions for the LTSI tree.


Chris

RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Chris Brandt]

Hi Jacopo,

On Wednesday, January 25, 2017, Jacopo Mondi wrote:
>  drivers/pinctrl/Kconfig             |   1 +
>  drivers/pinctrl/Makefile            |   1 +
>  drivers/pinctrl/rz-pfc/Kconfig      |  18 ++
>  drivers/pinctrl/rz-pfc/Makefile     |   1 +
>  drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447
> ++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++
>  6 files changed, 582 insertions(+)
>  create mode 100644 drivers/pinctrl/rz-pfc/Kconfig  create mode 100644
> drivers/pinctrl/rz-pfc/Makefile  create mode 100644 drivers/pinctrl/rz-
> pfc/pinctrl-rz.c
>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h



I think we should try to avoid the rz naming as much as possible since
this driver will hopefully be useful for other future Renesas devices if
they move to a similar pin-control type method. Maybe future "R-car" SoCs?
Or, maybe Renesas Marketing decides to come up with a new name for SoCs.

Otherwise, you could end up with a directly like "mach-shmobile" filled
with a bunch of...well...not SH-Mobile parts.

Maybe just
  drivers/pinctrl/renesas/pinctrl-renesas.c



Chris

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[jacopo mondi]

Hi Chris,

On 26/01/2017 03:58, Chris Brandt wrote:
> Hi Jacopo,
>
> On Wednesday, January 25, 2017, Jacopo Mondi wrote:
>>  drivers/pinctrl/Kconfig             |   1 +
>>  drivers/pinctrl/Makefile            |   1 +
>>  drivers/pinctrl/rz-pfc/Kconfig      |  18 ++
>>  drivers/pinctrl/rz-pfc/Makefile     |   1 +
>>  drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447
>> ++++++++++++++++++++++++++++++++++++
>>  drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++
>>  6 files changed, 582 insertions(+)
>>  create mode 100644 drivers/pinctrl/rz-pfc/Kconfig  create mode 100644
>> drivers/pinctrl/rz-pfc/Makefile  create mode 100644 drivers/pinctrl/rz-
>> pfc/pinctrl-rz.c
>>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h
>
>
>
> I think we should try to avoid the rz naming as much as possible since
> this driver will hopefully be useful for other future Renesas devices if
> they move to a similar pin-control type method. Maybe future "R-car" SoCs?
> Or, maybe Renesas Marketing decides to come up with a new name for SoCs.
>
> Otherwise, you could end up with a directly like "mach-shmobile" filled
> with a bunch of...well...not SH-Mobile parts.
>
> Maybe just
>   drivers/pinctrl/renesas/pinctrl-renesas.c
>

Wouldn't this be confusing, as most of renesas SoC are supported through 
drivers/pinctrl/sh-pfc instead?

I agree on dropping the rz name, if more SoC will come and join the 
pin-based PFC realm...

Thanks
    j



>
>
> Chris
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Sergei Shtylyov]

Hello!

    s/r7s1000/r7s72100/ in the subject.

On 1/25/2017 9:09 PM, Jacopo Mondi wrote:

> Add pincontroller node compatible with the new Renesas RZ/A1
> pincontroller driver.

    Pin controller.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
> index 3dd427d..764006d 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -171,6 +171,18 @@
>  		};
>  	};
>
> +	pinctrl: pinctrl@fcfe3000 {

    I'd call the node "pin-controller@...".

MBR, Sergei

Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group

[Sergei Shtylyov]

On 1/25/2017 9:09 PM, Jacopo Mondi wrote:

> Add  TxD and RxD pin configuration for SCIF2 serial communication
> interface on r7s72100 Genmai board.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
> index 118a8e2..ea649c7 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts
[...]
> @@ -34,6 +35,18 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  	};
> +
> +};
> +
> +&pinctrl {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&scif2_pins>;
> +
> +	scif2_pins: serial2 {
> +		/* P3_0 as TxD2; P3_2 as RxD2 */
> +		renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,

    The comma should be after the vendor prefix ("renesas").

MBR, Sergei

RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Chris Brandt]

Hi Jacopo,

On Thursday, January 26, 2017, jacopo mondi wrote:
> > I think we should try to avoid the rz naming as much as possible since
> > this driver will hopefully be useful for other future Renesas devices
> > if they move to a similar pin-control type method. Maybe future "R-car"
> SoCs?
> > Or, maybe Renesas Marketing decides to come up with a new name for SoCs.
> >
> > Otherwise, you could end up with a directly like "mach-shmobile"
> > filled with a bunch of...well...not SH-Mobile parts.
> >
> > Maybe just
> >   drivers/pinctrl/renesas/pinctrl-renesas.c
> >
> 
> Wouldn't this be confusing, as most of renesas SoC are supported through
> drivers/pinctrl/sh-pfc instead?

It's already confusing that a bunch of ARM parts use a "sh" pin controller.

In reality, any Renesas device could be a peripheral mix of ex-Mitsubishi,
ex-Hitachi, ex-NEC or Renesas original. Or, some IP was licensed from a 3rd
party (ie, the SDHI/TMIO controller).
The only thing consistent is "Renesas".

> I agree on dropping the rz name, if more SoC will come and join the pin-
> based PFC realm...

I will suggest it for the next generation of SoCs, so we'll see what happens...

Thanks,
Chris

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add core module for per-pin Renesas RZ series pin controller.
> The core module allows SoC driver to register their pins and SoC
> specific operations and interfaces with pinctrl and pinmux core on their
> behalf.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I believe nothing besides the DT property "renesas-rz,pins" and the value
of the RZ_PIN_ARGS_COUNT macro is really specific to Renesas or RZ(A)?
So this could become something really generic, and part of core pinctrl?

> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c
> @@ -0,0 +1,447 @@

> +#define RZ_PIN_ARGS_COUNT      3

This should be a parameter in the SoC-specific subdriver.

> +/**
> + * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position [bank:pin]
> + *
> + * This can be improved, as it walks all the pins reported by the SoC driver
> + *
> + * @return: pin number between [0 - npins]; -1 if not found
> + */
> +static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl,
> +                                  unsigned int bank, unsigned int pin)
> +{
> +       struct rz_pinctrl_info *info;
> +       struct rz_pin_desc *rz_pin;

const

> +       int i, npins;

unsigned int

> +/* ----------------------------------------------------------------------------
> + * pinctrl operations
> + */
> +static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,

const ... pctldev

> +                           unsigned int pin)
> +{
> +       struct rz_pinctrl_dev *rz_pinctrl;
> +       struct rz_pinctrl_info *info;

const

> +static int rz_dt_node_to_map(struct pinctrl_dev *pctldev,
> +                            struct device_node *np_config,
> +                            struct pinctrl_map **map, unsigned int *num_maps)
> +{

> +       fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL);
> +       if (unlikely(!fngrps)) {

Please don't use unlikely.

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: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add pin controller driver for Renesas RZ/A1 SoC.
> The SoC driver registers to rz-pfc core module and provides pin
> description array and SoC specific pin mux operation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> --- a/drivers/pinctrl/rz-pfc/Makefile
> +++ b/drivers/pinctrl/rz-pfc/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PINCTRL_RZ_PINCTRL)               += pinctrl-rz.o
> +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL)             += pinctrl-rza1.o
> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> new file mode 100644
> index 0000000..221f048
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c

> +
> +/* ----------------------------------------------------------------------------
> + * SoC operations
> + */
> +
> +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
> +                               int bank, int pin, int set)
> +{
> +       void __iomem *mem = RZA1_ADDR(res->base, reg, bank);
> +       u16 val = set ? ioread16(mem) | 1 << pin :
> +                       ioread16(mem) & ~(1 << pin);
> +#ifdef RZA1_REG_DBG
> +       u16 temp = ioread16(mem);
> +
> +       pr_err("%p %p %p - %4x %4x\n",
> +              (void *)res->start, res->base, mem, temp, val);

Please drop the cast, take the address, and use %pa to format a
resource_size_t, cfr. Documentation/printk-formats.txt.

> + static struct rz_pinctrl_ops rza1_pinctrl_ops = {

const

> +       .set_mux = rza1_set_mux,
> +};
> +
> +static struct rz_pinctrl_info rza1_info = {

const

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: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add dt-bindings header for Renesas RZ pincontroller.
> The header defines macros for pin description and alternate function
> numbers.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
>
> diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
> new file mode 100644
> index 0000000..92816d4
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
> @@ -0,0 +1,19 @@
> +/*
> + * Defines macros and constants for Renesas RZ pin controller and muxer
> + */
> +
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
> +
> +#define RZ_PIN(b, p) b p

And the advantage of this macro is?

> +#define ALTERNATE_FUNC_1       0
> +#define ALTERNATE_FUNC_2       1
> +#define ALTERNATE_FUNC_3       2
> +#define ALTERNATE_FUNC_4       3
> +#define ALTERNATE_FUNC_5       4
> +#define ALTERNATE_FUNC_6       5
> +#define ALTERNATE_FUNC_7       6
> +#define ALTERNATE_FUNC_8       7

I have mixed feelings about these macros:
  1. They're long to type,
  2. They just map from n to n-1.

Why not use plain numbers 1..8 (the alternate function numbering in the
datasheet is 1-based), and subtract 1 in the C code?

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: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add pincontroller node compatible with the new Renesas RZ/A1
> pincontroller driver.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
> index 3dd427d..764006d 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -171,6 +171,18 @@
>                 };
>         };
>
> +       pinctrl: pinctrl@fcfe3000 {
> +               compatible = "renesas,rza1-pinctrl";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               #pinctrl-cells = <2>;

Souldn't that be <3>?
E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers.

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: [RFC 0/5] Renesas RZ series pinctrl driver

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>    after having discussed in great detail the RZ series per-pin PFC hardware
> peculiarities, this is a proposal for a possible pin-based pin controller
> driver for SoC devices of Renesas RZ family.

"RZ" is a bad name, as there are (currently) 3 RZ subfamilies, and they
differ a lot. So it's better to use "RZ/A".
Or perhaps even "RZ/A1", as you never know what's going to happen with "2"...

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

[RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver

[Jacopo Mondi]

Hello,
   sorry if I'm sending 2 patches on top of an RFC series with comments
still pending, but these patches enabled me to properly test pin configuration
sequence in order to access the internal EEPROM through RIIC2 interface on
pins 1_4 and 1_5.

The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies on.

When sending v2 of the whole series I'll probably squash these, but if someone
is testing the RFC series I wanted to make sure he does not waste his time with
a broken driver.

Thanks
   j

Jacopo Mondi (2):
  arm: dts: genmai: Configure RIIC2 pins
  pinctrl: rz-pfc: Fix RZ/A1 pin function configuration

 arch/arm/boot/dts/r7s72100-genmai.dts |  8 ++++-
 drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------
 2 files changed, 43 insertions(+), 20 deletions(-)

-- 
2.7.4

[RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins

[Jacopo Mondi]

Add pin configuration for RIIC2 pins interface.
The i2c2 is connected to internal eeprom.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index ea649c7..0068a1a 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -40,13 +40,19 @@
 
 &pinctrl {
 	pinctrl-names = "default";
-	pinctrl-0 = <&scif2_pins>;
+	pinctrl-0 = <&scif2_pins &i2c2_pins>;
 
 	scif2_pins: serial2 {
 		/* P3_0 as TxD2; P3_2 as RxD2 */
 		renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,
 				  <RZ_PIN(3, 2) ALTERNATE_FUNC_4>;
 	};
+
+	i2c2_pins: i2c2 {
+		/* RIIC2: P1_4 as SCL, P1_5 as SDA */
+		renesas-rz,pins = <RZ_PIN(1, 4) ALTERNATE_FUNC_1>,
+				  <RZ_PIN(1, 5) ALTERNATE_FUNC_1>;
+	};
 };
 
 &extal_clk {
-- 
2.7.4

[RFC fixes 2/2] pinctrl: rz-pfc: Fix RZ/A1 pin function configuration

[Jacopo Mondi]

Fix alternate function configuration sequence for RZ/A1 SoC.
The pin is first configured as simple input port, then alternate
function is configured.
Always enable input buffer for now as long as we don't have
configuration paramters coming from device tree.

Tested accessing embedded EEPROM chip through RIIC2 interface.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
index 221f048..53b71c8 100644
--- a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
+++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
@@ -27,6 +27,7 @@
 #define MEM_RES_HIGH	1 /* PORTn_base + 0x4000 */
 
 /* displacements from PORTn_base */
+#define PM_REG		0x300
 #define PMC_REG		0x400
 #define PFC_REG		0x500
 #define PFCE_REG	0x600
@@ -235,8 +236,8 @@ static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
 #ifdef RZA1_REG_DBG
 	u16 temp = ioread16(mem);
 
-	pr_err("%p %p %p - %4x %4x\n",
-	       (void *)res->start, res->base, mem, temp, val);
+	pr_err("%p %p %p - %d:%d - %4x %4x\n",
+	       (void *)res->start, res->base, mem, bank, pin, temp, val);
 #endif
 	iowrite16(val, mem);
 }
@@ -253,25 +254,35 @@ static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
 static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl,
 			struct rz_pin_desc *pin_desc, unsigned int mux_mode)
 {
-	struct rz_pinctrl_res *res;
+	struct rz_pinctrl_res *res_low, *res_high;
 	unsigned int bank = pin_desc->bank,
 		     pin = pin_desc->pin;
 
+	res_high = &pinctrl->res[MEM_RES_HIGH];
+	res_low = &pinctrl->res[MEM_RES_LOW];
+
 	/*
-	 * disable input buffer and bi-control direction before entering
-	 * alternate mode and let alternate function drive the IO mode by
-	 * setting PIPCn to 1
+	 * reset pin state disabling input buffer and bi-directional control
+	 * and configure it as input port before configuring alternate
+	 * function later
 	 */
-	res = &pinctrl->res[MEM_RES_HIGH];
-	rza1_set_bit(res, PIBC_REG, bank, pin, 0);
-	rza1_set_bit(res, PBDC_REG, bank, pin, 0);
-	rza1_set_bit(res, PIPC_REG, bank, pin, 1);
+	rza1_set_bit(res_high, PIBC_REG, bank, pin, 0);
+	rza1_set_bit(res_high, PBDC_REG, bank, pin, 0);
 
-	/* TODO:
-	 * all alternate functions except a few (3) need PIPCn = 1;
-	 * find a way to identify those 3 functions, do not set PIPCn to 1
-	 * and set PMn according to some flag passed as parameter from DTS
+	rza1_set_bit(res_low, PM_REG, bank, pin, 1);
+	rza1_set_bit(res_low, PMC_REG, bank, pin, 0);
+	rza1_set_bit(res_high, PIPC_REG, bank, pin, 0);
+
+	/*
+	 * pins that has to be used as input, even in alternate function mode,
+	 * needs input buffer enabled.
+	 * Set PBDCn to enable bi-control which enables input buffer
+	 * consequentialy.
+	 *
+	 * TODO: Add a flag to dts pin configuration to specify when a pin
+	 * requires input buffer enabled and set PBDCn conditionally.
 	 */
+	rza1_set_bit(res_high, PBDC_REG, bank, pin, 1);
 
 	/*
 	 * enable alternate function mode and select it.
@@ -290,11 +301,17 @@ static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl,
 	 * 1	1	1	1	8		7
 	 * ----------------------------------------------------
 	 */
-	res = &pinctrl->res[MEM_RES_LOW];
-	rza1_set_bit(res, PMC_REG, bank, pin, 1);
-	rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1);
-	rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2);
-	rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4);
+	rza1_set_bit(res_low, PFC_REG, bank, pin, mux_mode & 0x1);
+	rza1_set_bit(res_low, PFCE_REG, bank, pin, mux_mode & 0x2);
+	rza1_set_bit(res_low, PFCEA_REG, bank, pin, mux_mode & 0x4);
+
+	/* TODO:
+	 * all alternate functions except a few (4) need PIPCn = 1;
+	 * find a way to identify those 3 functions, do not set PIPCn to 1
+	 * and set PMn according to some flag passed as parameter from DTS
+	 */
+	rza1_set_bit(res_high, PIPC_REG, bank, pin, 1);
+	rza1_set_bit(res_low, PMC_REG, bank, pin, 1);
 
 	return 0;
 }
-- 
2.7.4

RE: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver

[Chris Brandt]

Hi Jacopo,

On Friday, January 27, 2017, Jacopo Mondi wrote:
> Hello,
>    sorry if I'm sending 2 patches on top of an RFC series with comments
> still pending, but these patches enabled me to properly test pin
> configuration sequence in order to access the internal EEPROM through
> RIIC2 interface on pins 1_4 and 1_5.
> 
> The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies
> on.
> 
> When sending v2 of the whole series I'll probably squash these, but if
> someone is testing the RFC series I wanted to make sure he does not waste
> his time with a broken driver.
> 
> Thanks
>    j
> 
> Jacopo Mondi (2):
>   arm: dts: genmai: Configure RIIC2 pins
>   pinctrl: rz-pfc: Fix RZ/A1 pin function configuration
> 
>  arch/arm/boot/dts/r7s72100-genmai.dts |  8 ++++-  drivers/pinctrl/rz-
> pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------
>  2 files changed, 43 insertions(+), 20 deletions(-)


Preliminary testing shows that I2C pin muxing works. Nice job!

Testing:
- RZ/A1H RSK board
- u-boot modified to make sure pins are put back to GPIO-IN
- RIIC ch3 is connected to a I2C port expander that has 3 LEDs attached
- using a heartbeat kernel thread that blinks the LEDs

Of course, more testing is needed to make sure there is no "smoke and mirrors"
going on like there was with the MSTP clock driver ;)


Note that the I2C pin need to be configured at "bi-directional" but there is
no way to specify that from DT, so that has to be added as a parameter.

I am very happy about how easy it is to set the pins up! It almost matches
what I do in our BSP code today, so I didn't even have to look at the HW schematic.

I'll give Ethernet and SDHI a try. For RSPI, I have to go manually wire up a
SPI flash again.


Chris

Re: [RFC 0/5] Renesas RZ series pinctrl driver

[Linus Walleij]

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:

>    after having discussed in great detail the RZ series per-pin PFC hardware
> peculiarities, this is a proposal for a possible pin-based pin controller
> driver for SoC devices of Renesas RZ family.
>
> This RFC series adds a minimal driver infrastructure which supports pin
> multiplexing via explicit per-pin settings performed in device tree sources.

I think this is what Laurent had in mind when he said something about
that he should have taken the per-pin approach from day 1.

I would especially like to see Laurent's review and ACK on this series
for that reason so we don't create something less than what he is
expecting for the future of Renesas pin control.

The series as such seems fine and is reusing Tony's work to generalize
pin controllers putting functions and groups into the device tree in a nice
way, which makes it relevant for him to have a look as well, if he has
time.

Yours,
Linus Walleij

Re: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver

[jacopo mondi]

Hi Chris,
    thanks for testing the series

On 27/01/2017 22:09, Chris Brandt wrote:
> Hi Jacopo,
>
> On Friday, January 27, 2017, Jacopo Mondi wrote:
>> Hello,
>>    sorry if I'm sending 2 patches on top of an RFC series with comments
>> still pending, but these patches enabled me to properly test pin
>> configuration sequence in order to access the internal EEPROM through
>> RIIC2 interface on pins 1_4 and 1_5.
>>
>> The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies
>> on.
>>
>> When sending v2 of the whole series I'll probably squash these, but if
>> someone is testing the RFC series I wanted to make sure he does not waste
>> his time with a broken driver.
>>
>> Thanks
>>    j
>>
>> Jacopo Mondi (2):
>>   arm: dts: genmai: Configure RIIC2 pins
>>   pinctrl: rz-pfc: Fix RZ/A1 pin function configuration
>>
>>  arch/arm/boot/dts/r7s72100-genmai.dts |  8 ++++-  drivers/pinctrl/rz-
>> pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------
>>  2 files changed, 43 insertions(+), 20 deletions(-)
>
>
> Preliminary testing shows that I2C pin muxing works. Nice job!
>
> Testing:
> - RZ/A1H RSK board
> - u-boot modified to make sure pins are put back to GPIO-IN
> - RIIC ch3 is connected to a I2C port expander that has 3 LEDs attached
> - using a heartbeat kernel thread that blinks the LEDs
>
> Of course, more testing is needed to make sure there is no "smoke and mirrors"
> going on like there was with the MSTP clock driver ;)
>
>
> Note that the I2C pin need to be configured at "bi-directional" but there is
> no way to specify that from DT, so that has to be added as a parameter.

That's something I would like to discuss quite soon.
One general thing I would like is having the so-called "additional 
parameters" part of the SoC driver module, as my gut feeling is that 
different PFC hw requires different configuration options.

In example, the RZ/A1 SoC requires the input/output direction of the pin 
to be specified even when the alternate function is supposed to drive it 
(in order to enable/disable input buffer). The same applies to 
bi-directional port control as you pointed out in your example 
(bi-directional enables input buffer, but that's a consequence of how hw 
works there).

I'm almost sure the same won't be required by all existing and 
forth-coming Renesas SoCs with a pin-based PFC hardware, but maybe other 
parameters I cannot think of right now will have to be specified instead.

If we want to keep the configuration flags SoC-specific, the most easy 
way to pass them down from core module to SoC module would be 
compressing the alternate function number and other configurations in a 
single u32.

In that case the dts will look like:

renesasa,rz-pins = <BANK PORT (ALTERNATE_FUNC_# | IO_MODE | DIRECTION)>;

We could even add a parameter to separate ALTERNATE_FUNCTION from the 
additional configurations, but I don't see any advantage in doing this 
at the moment.

Can people with a broader knowledge of Renesas' SoC series ack/nack my 
assumptions here?

Thanks
    j

Re: [RFC 0/5] Renesas RZ series pinctrl driver

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170130 05:53]:
> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> 
> >    after having discussed in great detail the RZ series per-pin PFC hardware
> > peculiarities, this is a proposal for a possible pin-based pin controller
> > driver for SoC devices of Renesas RZ family.
> >
> > This RFC series adds a minimal driver infrastructure which supports pin
> > multiplexing via explicit per-pin settings performed in device tree sources.
> 
> I think this is what Laurent had in mind when he said something about
> that he should have taken the per-pin approach from day 1.
> 
> I would especially like to see Laurent's review and ACK on this series
> for that reason so we don't create something less than what he is
> expecting for the future of Renesas pin control.
> 
> The series as such seems fine and is reusing Tony's work to generalize
> pin controllers putting functions and groups into the device tree in a nice
> way, which makes it relevant for him to have a look as well, if he has
> time.

Well I don't have the series in my inbox, but browsing through
the patchwork seems OK. The things to check would be:

1. Do you guys really need RZ_PIN_DESC? Just let pinctrl
   framework generate the names, then do some simple user
   space tool that decodes the debugfs output. If you really
   need names, allow the consumer to set the name like we do
   for GPIOs and interrupts when requesting the resource.

2. Make sure you're using hardware offsets for any indexing
   to avoid adding artificial mapping tables between the driver
   and the the dts file. Many times new features are enabled
   between SoC revisions and a numbered index gets out of sync
   and needs constant patching just like we have for interrupt
   numbers earlier.. I don't think that's an issue with this
   series, but please check one more time for easy maintenance :)

Regards,

Tony

Re: [RFC 0/5] Renesas RZ series pinctrl driver

[Geert Uytterhoeven]

Hi Linus,

On Mon, Jan 30, 2017 at 2:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>    after having discussed in great detail the RZ series per-pin PFC hardware
>> peculiarities, this is a proposal for a possible pin-based pin controller
>> driver for SoC devices of Renesas RZ family.
>>
>> This RFC series adds a minimal driver infrastructure which supports pin
>> multiplexing via explicit per-pin settings performed in device tree sources.
>
> I think this is what Laurent had in mind when he said something about
> that he should have taken the per-pin approach from day 1.
>
> I would especially like to see Laurent's review and ACK on this series
> for that reason so we don't create something less than what he is
> expecting for the future of Renesas pin control.

It depends on the actual hardware: while per-pin settings are suitable for
SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's not
suitable for SoCs where that's not the case, and where the hardware has
group-wise configuration (e.g. on R-Car).

Hence IMHO "the future of Renesas pin control" will remain a split
personality for a while ;-)

Laurent: please kick me if you disagree...

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: [RFC 0/5] Renesas RZ series pinctrl driver

[jacopo mondi]

Hi Tony,

On 30/01/2017 16:53, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [170130 05:53]:
>> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>
>>>    after having discussed in great detail the RZ series per-pin PFC hardware
>>> peculiarities, this is a proposal for a possible pin-based pin controller
>>> driver for SoC devices of Renesas RZ family.
>>>
>>> This RFC series adds a minimal driver infrastructure which supports pin
>>> multiplexing via explicit per-pin settings performed in device tree sources.
>>
>> I think this is what Laurent had in mind when he said something about
>> that he should have taken the per-pin approach from day 1.
>>
>> I would especially like to see Laurent's review and ACK on this series
>> for that reason so we don't create something less than what he is
>> expecting for the future of Renesas pin control.
>>
>> The series as such seems fine and is reusing Tony's work to generalize
>> pin controllers putting functions and groups into the device tree in a nice
>> way, which makes it relevant for him to have a look as well, if he has
>> time.
>
> Well I don't have the series in my inbox, but browsing through
> the patchwork seems OK. The things to check would be:

Sorry about this, I thought sending to linux-gpio was enough.
We'll keep you in the loop now...

>
> 1. Do you guys really need RZ_PIN_DESC? Just let pinctrl
>    framework generate the names, then do some simple user
>    space tool that decodes the debugfs output. If you really
>    need names, allow the consumer to set the name like we do
>    for GPIOs and interrupts when requesting the resource.
>

Well, more than names I have used the RZ_PIN_DESC macro as an extended 
version of the PINCTRL_PIN one, to set bank and pin fields of the 
rz_pin_desc structure that describes a pin.

The proposed DTS syntax identifies pins by their position (bank:pin) 
instead that by index, so I need to have those information available in 
pin description to identify them.

> 2. Make sure you're using hardware offsets for any indexing
>    to avoid adding artificial mapping tables between the driver
>    and the the dts file. Many times new features are enabled
>    between SoC revisions and a numbered index gets out of sync
>    and needs constant patching just like we have for interrupt
>    numbers earlier.. I don't think that's an issue with this
>    series, but please check one more time for easy maintenance :)
>

As we describe pins "by position", if I got your comment properly, we 
should be safe, as indexes are purely incremental values from [0-npins].

I know pinctrl-single (and other drivers such as imx one if I'm not 
wrong) uses pin indexes to calculate register offsets. This series 
doesn't but uses the [bank:pin] couple for that purpose instead.

Thanks
    j


> Regards,
>
> Tony
>

RE: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver

[Chris Brandt]

Hi Jacopo,

On Monday, January 30, 2017, Jacopo Mondi wrote:
> > Note that the I2C pin need to be configured at "bi-directional" but
> > there is no way to specify that from DT, so that has to be added as a
> parameter.
> 
> That's something I would like to discuss quite soon.
> One general thing I would like is having the so-called "additional
> parameters" part of the SoC driver module, as my gut feeling is that
> different PFC hw requires different configuration options.

In the API I used for the current RZ/A1 BSP, I used an extra parameter
to pass in any non-normal settings.


> In example, the RZ/A1 SoC requires the input/output direction of the pin
> to be specified even when the alternate function is supposed to drive it
> (in order to enable/disable input buffer). The same applies to bi-
> directional port control as you pointed out in your example (bi-
> directional enables input buffer, but that's a consequence of how hw works
> there).
> 
> I'm almost sure the same won't be required by all existing and forth-
> coming Renesas SoCs with a pin-based PFC hardware, but maybe other
> parameters I cannot think of right now will have to be specified instead.

I would guess for one reason or another, there is always some exception
in the hardware manual when it comes to pins settings. Meaning, there are
restrictions in the I/O pad circuits that the HW designers find it easier
to tell the SW guys to deal with it instead of trying to make it 'automatic'
in HW.
So...you'll always have the need for an extra parameter.


> If we want to keep the configuration flags SoC-specific, the most easy way
> to pass them down from core module to SoC module would be compressing the
> alternate function number and other configurations in a single u32.
> 
> In that case the dts will look like:
> 
> renesasa,rz-pins = <BANK PORT (ALTERNATE_FUNC_# | IO_MODE | DIRECTION)>;
> 
> We could even add a parameter to separate ALTERNATE_FUNCTION from the
> additional configurations, but I don't see any advantage in doing this at
> the moment.

As I said, I made "additional configs" a separate parameter, but combining
it with ALTERNATE_FUNC_# is fine with me. I have not seen an SoC that allowed
any more than 8 different functions per pin, so you have lots of bits
in u32 in order to set flags bits.

However, as already mentioned by Geert, "ALTERNATE_FUNC_#" is too long of
a name. Just "ALT_#" is fine.

    renesasa,rz-pins = <BANK PORT (ALT_# | IO_MODE | DIRECTION)>;

Geert's suggestion was to just use the actual number (1-8), ALT_# might make
it easier to decode if you are combining multiple things into a single u32.

There is one more idea....but I heist to suggest it.
You just pass in the port and alternate function number, and you do a lookup
table for settings such as bi-direction or port direction register.
You only put into the table the pin settings that are "not normal".

The benefit is that the user doesn't have to know what pins needs what special
tweak/flag/setting....but of course it start to make the driver more complex.



Chris

Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group

[Laurent Pinchart]

Hello,

On Thursday 26 Jan 2017 12:51:03 Sergei Shtylyov wrote:
> On 1/25/2017 9:09 PM, Jacopo Mondi wrote:
> > Add  TxD and RxD pin configuration for SCIF2 serial communication
> > interface on r7s72100 Genmai board.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > 
> >  arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts
> > b/arch/arm/boot/dts/r7s72100-genmai.dts index 118a8e2..ea649c7 100644
> > --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> > +++ b/arch/arm/boot/dts/r7s72100-genmai.dts
> 
> [...]
> 
> > @@ -34,6 +35,18 @@
> >  		#address-cells = <1>;
> >  		#size-cells = <1>;
> >  	};
> > +

That's not needed.

> > +};
> > +
> > +&pinctrl {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&scif2_pins>;
> > +
> > +	scif2_pins: serial2 {
> > +		/* P3_0 as TxD2; P3_2 as RxD2 */
> > +		renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,
> 
> The comma should be after the vendor prefix ("renesas").

And the "-rz" suffix isn't needed, "renesas,pins" will do.

-- 
Regards,

Laurent Pinchart

Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday 25 Jan 2017 19:09:46 Jacopo Mondi wrote:
> Add pincontroller node compatible with the new Renesas RZ/A1
> pincontroller driver.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi
> b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..764006d 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -171,6 +171,18 @@
>  		};
>  	};
> 
> +	pinctrl: pinctrl@fcfe3000 {
> +		compatible = "renesas,rza1-pinctrl";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		#pinctrl-cells = <2>;
> +
> +		reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */
> +		      <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */

What's the reason for splitting those registers in two sets ? Maybe you can 
explain that in the DT bindings documentation that this patch series is 
missing ;-)

> +		      <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */

s/B/b/

> +	};
> +
>  	scif0: serial@e8007000 {
>  		compatible = "renesas,scif-r7s72100", "renesas,scif";
>  		reg = <0xe8007000 64>;

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Laurent Pinchart]

On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote:
> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote:
> > Add dt-bindings header for Renesas RZ pincontroller.
> > The header defines macros for pin description and alternate function
> > numbers.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > 
> >  include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
> > 
> > diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
> > b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h new file mode 100644
> > index 0000000..92816d4
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Defines macros and constants for Renesas RZ pin controller and muxer
> > + */
> > +
> > +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
> > +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
> > +
> > +#define RZ_PIN(b, p) b p
> 
> And the advantage of this macro is?
> 
> > +#define ALTERNATE_FUNC_1       0
> > +#define ALTERNATE_FUNC_2       1
> > +#define ALTERNATE_FUNC_3       2
> > +#define ALTERNATE_FUNC_4       3
> > +#define ALTERNATE_FUNC_5       4
> > +#define ALTERNATE_FUNC_6       5
> > +#define ALTERNATE_FUNC_7       6
> > +#define ALTERNATE_FUNC_8       7
> 
> I have mixed feelings about these macros:
>   1. They're long to type,
>   2. They just map from n to n-1.
> 
> Why not use plain numbers 1..8 (the alternate function numbering in the
> datasheet is 1-based), and subtract 1 in the C code?

I was about to mention the same. I think you can drop this patch and use the 
numbers directly.

-- 
Regards,

Laurent Pinchart

Re: [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday 27 Jan 2017 17:47:07 Jacopo Mondi wrote:
> Add pin configuration for RIIC2 pins interface.
> The i2c2 is connected to internal eeprom.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm/boot/dts/r7s72100-genmai.dts | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts
> b/arch/arm/boot/dts/r7s72100-genmai.dts index ea649c7..0068a1a 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts
> @@ -40,13 +40,19 @@
> 
>  &pinctrl {
>  	pinctrl-names = "default";
> -	pinctrl-0 = <&scif2_pins>;
> +	pinctrl-0 = <&scif2_pins &i2c2_pins>;

Referencing the SCIF2 and I2C2 pins nodes from the pinctrl node will result in 
the corresponding pins being configured when the pinctrl driver is probed. You 
could move this to the SCIF2 and I2C2 DT nodes, to have the pins configured as 
part of the probe sequence of the corresponding drivers. The advantage would 
be that pins would only be configured if the corresponding devices are enabled 
and get probed by a driver. This could save a bit of power if the boot loader 
configured the pins in low-power mode and the devices were unused (although in 
this very specific case I don't think it will make a difference).

> 
>  	scif2_pins: serial2 {
>  		/* P3_0 as TxD2; P3_2 as RxD2 */
>  		renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,
>  				  <RZ_PIN(3, 2) ALTERNATE_FUNC_4>;
>  	};
> +
> +	i2c2_pins: i2c2 {
> +		/* RIIC2: P1_4 as SCL, P1_5 as SDA */
> +		renesas-rz,pins = <RZ_PIN(1, 4) ALTERNATE_FUNC_1>,
> +				  <RZ_PIN(1, 5) ALTERNATE_FUNC_1>;
> +	};
>  };
> 
>  &extal_clk {

-- 
Regards,

Laurent Pinchart

Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Jan 30, 2017 at 7:17 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 26 Jan 2017 12:51:03 Sergei Shtylyov wrote:
>> On 1/25/2017 9:09 PM, Jacopo Mondi wrote:
>> > +&pinctrl {
>> > +   pinctrl-names = "default";
>> > +   pinctrl-0 = <&scif2_pins>;
>> > +
>> > +   scif2_pins: serial2 {
>> > +           /* P3_0 as TxD2; P3_2 as RxD2 */
>> > +           renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>,
>>
>> The comma should be after the vendor prefix ("renesas").
>
> And the "-rz" suffix isn't needed, "renesas,pins" will do.

Then it can just become "pins"? Or not?

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: [RFC 0/5] Renesas RZ series pinctrl driver

[Laurent Pinchart]

Hi Geert,

On Monday 30 Jan 2017 17:08:11 Geert Uytterhoeven wrote:
> On Mon, Jan 30, 2017 at 2:51 PM, Linus Walleij wrote:
> > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote:
> >> after having discussed in great detail the RZ series per-pin PFC
> >> hardware peculiarities, this is a proposal for a possible pin-based pin
> >> controller driver for SoC devices of Renesas RZ family.
> >> 
> >> This RFC series adds a minimal driver infrastructure which supports pin
> >> multiplexing via explicit per-pin settings performed in device tree
> >> sources.
> >
> > I think this is what Laurent had in mind when he said something about
> > that he should have taken the per-pin approach from day 1.
> > 
> > I would especially like to see Laurent's review and ACK on this series
> > for that reason so we don't create something less than what he is
> > expecting for the future of Renesas pin control.
> 
> It depends on the actual hardware: while per-pin settings are suitable for
> SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's not
> suitable for SoCs where that's not the case, and where the hardware has
> group-wise configuration (e.g. on R-Car).
> 
> Hence IMHO "the future of Renesas pin control" will remain a split
> personality for a while ;-)
> 
> Laurent: please kick me if you disagree...

I unfortunately agree. Maybe we could try to speed up the process by providing 
feedback to the hardware engineers ?

-- 
Regards,

Laurent Pinchart

RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Chris Brandt]

Hi Jacopo,

On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> +
> +	return 0;
> +
> +free_map:
> +	devm_kfree(rz_pinctrl->dev, *map);
> +free_fngrps:
> +	devm_kfree(rz_pinctrl->dev, fngrps);
> +free_pins:
> +	devm_kfree(rz_pinctrl->dev, mux_modes);
> +	devm_kfree(rz_pinctrl->dev, grpins);
> +	return ret;
> +}

Since one of the benefits of using devm_kzalloc is that if
the probe fails and returns an error, all the memory associated with
that device will automatically get freed, you 'might' not
need this code to free memory.

I say might because I'm not sure if returning an error here will
kill the driver or not. But, might be interesting to look into.


> +#define RZ_PIN_NAME(bank, pin)						\
> +	PIN_##bank##_##pin
> +
> +#define RZ_PIN_DESC(b, p)						\
> +	{ .number = RZ_PIN_NAME(b, p),					\
> +	  .name = __stringify(RZ_PIN_NAME(b, p)),			\

The hardware manual uses the names "P1_0" for ports, so it might
be better to match that format for consistency.


Chris

RE: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Chris Brandt]

Hi Jacopo,


On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> +	/* Port 5 */
> +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),

The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses
all 16 port pins on "port 5" so I'd like to include them as well.



> +static const struct of_device_id rza1_pinctrl_of_match[] = {
> +	{ .compatible = "renesas,rza1-pinctrl", },
> +	{ }
> +};

Since this PFC driver file is specifically for RZ/A1, I think a
better compatible string would be:

  .compatible = "renesas,r7s72100-renesas-pinctrl",


Chris

RE: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Chris Brandt]

Hi Jacopo,

On Monday, January 30, 2017, Laurent Pinchart wrote:
> > +	pinctrl: pinctrl@fcfe3000 {
> > +		compatible = "renesas,rza1-pinctrl";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		#pinctrl-cells = <2>;
> > +
> > +		reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */
> > +		      <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */
> 
> What's the reason for splitting those registers in two sets ? Maybe you
> can explain that in the DT bindings documentation that this patch series
> is missing ;-)


I left this out of my review comments, but even though the chip designers left
a BIG HOLE in the memory map of the PFC controller, I don't think it will
'cost' you anything by just mapping the whole area (dead space and all) and
getting rid of the "high and low" memory indexing thing that you are doing
in the driver.
There is nothing mapped in that dead area anyway.



Chris

RE: [RFC 0/5] Renesas RZ series pinctrl driver

[Chris Brandt]

On Monday, January 30, 2017, Laurent Pinchart wrote:
> > It depends on the actual hardware: while per-pin settings are suitable
> > for SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's
> > not suitable for SoCs where that's not the case, and where the
> > hardware has group-wise configuration (e.g. on R-Car).
> >
> > Hence IMHO "the future of Renesas pin control" will remain a split
> > personality for a while ;-)
> >
> > Laurent: please kick me if you disagree...
> 
> I unfortunately agree. Maybe we could try to speed up the process by
> providing feedback to the hardware engineers ?

For what it's worth, I fired off my 'rant and rave' last week about the
current R-Car PFC block so I'll be curious to see what they say. Of
course, I'm on the RZ side of the company, so maybe someone on the R-Car
side might have better luck.


Chris

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[jacopo mondi]

Hi Chris,

On 30/01/2017 20:19, Chris Brandt wrote:
> Hi Jacopo,
>
> On Wednesday, January 25, 2017, Jacopo Mondi wrote:
>> +
>> +	return 0;
>> +
>> +free_map:
>> +	devm_kfree(rz_pinctrl->dev, *map);
>> +free_fngrps:
>> +	devm_kfree(rz_pinctrl->dev, fngrps);
>> +free_pins:
>> +	devm_kfree(rz_pinctrl->dev, mux_modes);
>> +	devm_kfree(rz_pinctrl->dev, grpins);
>> +	return ret;
>> +}
>
> Since one of the benefits of using devm_kzalloc is that if
> the probe fails and returns an error, all the memory associated with
> that device will automatically get freed, you 'might' not
> need this code to free memory.
>
> I say might because I'm not sure if returning an error here will
> kill the driver or not. But, might be interesting to look into.
>

No, returning an error here would not kill the driver BUT if 
dt_node_to_map fails, dt_free_map is called immediately later [1]

So here we maybe should not use device managed memory as it does not 
bring anything, do not free *map as it is freed later in dt_free_map, 
but release fngrps mux_modes and grpins, as we lose reference to them 
outside the scope of this function.

Do you agree?

>

>> +#define RZ_PIN_NAME(bank, pin)						\
>> +	PIN_##bank##_##pin
>> +
>> +#define RZ_PIN_DESC(b, p)						\
>> +	{ .number = RZ_PIN_NAME(b, p),					\
>> +	  .name = __stringify(RZ_PIN_NAME(b, p)),			\
>
> The hardware manual uses the names "P1_0" for ports, so it might
> be better to match that format for consistency.
>
>

Noted

Thanks
    j


[1] http://lxr.free-electrons.com/source/drivers/pinctrl/devicetree.c#L236

Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[jacopo mondi]

Hi Geert, Laurent,

On 30/01/2017 19:30, Laurent Pinchart wrote:
> On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote:
>> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote:
>>> Add dt-bindings header for Renesas RZ pincontroller.
>>> The header defines macros for pin description and alternate function
>>> numbers.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>
>>>  include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
>>>
>>> diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
>>> b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h new file mode 100644
>>> index 0000000..92816d4
>>> --- /dev/null
>>> +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h
>>> @@ -0,0 +1,19 @@
>>> +/*
>>> + * Defines macros and constants for Renesas RZ pin controller and muxer
>>> + */
>>> +
>>> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
>>> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H
>>> +
>>> +#define RZ_PIN(b, p) b p
>>
>> And the advantage of this macro is?
>>

Nothing beside being cute

>>> +#define ALTERNATE_FUNC_1       0
>>> +#define ALTERNATE_FUNC_2       1
>>> +#define ALTERNATE_FUNC_3       2
>>> +#define ALTERNATE_FUNC_4       3
>>> +#define ALTERNATE_FUNC_5       4
>>> +#define ALTERNATE_FUNC_6       5
>>> +#define ALTERNATE_FUNC_7       6
>>> +#define ALTERNATE_FUNC_8       7
>>
>> I have mixed feelings about these macros:
>>   1. They're long to type,
>>   2. They just map from n to n-1.
>>
>> Why not use plain numbers 1..8 (the alternate function numbering in the
>> datasheet is 1-based), and subtract 1 in the C code?
>
> I was about to mention the same. I think you can drop this patch and use the
> numbers directly.
>

Please be aware there are values we'll have to define here, such as the 
additional configurations we're talking about in some other part of this 
email thread.

This may become something like:

#define ALT_FUNC_1       0
#define ALT_FUNC_2       1
#define ALT_FUNC_3       2
#define ALT_FUNC_4       3
#define ALT_FUNC_5       4
#define ALT_FUNC_6       5
#define ALT_FUNC_7       6
#define ALT_FUNC_8       7

#define INPUT_MODE	1
#define BIDRECTIONAL	2
#define ...

#define PIN(b, p) b p
#define PIN_MUX(func, conf)	\
	((func & 0xf) | ((conf) << 16))

and in DTS sources you would describe a pin as

pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>;

We can drop the PIN macro as it does not bring anything I agree, but 
it's nicer to see imho

Thanks
    j

Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Geert Uytterhoeven]

Hi Jacopo,

On Tue, Jan 31, 2017 at 10:12 AM, jacopo mondi <jacopo@jmondi.org> wrote:
> On 30/01/2017 19:30, Laurent Pinchart wrote:
>> On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote:
>>> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote:
>>>> Add dt-bindings header for Renesas RZ pincontroller.
>>>> The header defines macros for pin description and alternate function
>>>> numbers.

>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h

>>>> +#define RZ_PIN(b, p) b p
>>>
>>> And the advantage of this macro is?
>>>
>
> Nothing beside being cute

:-)

>>>> +#define ALTERNATE_FUNC_1       0
>>>> +#define ALTERNATE_FUNC_2       1
>>>> +#define ALTERNATE_FUNC_3       2
>>>> +#define ALTERNATE_FUNC_4       3
>>>> +#define ALTERNATE_FUNC_5       4
>>>> +#define ALTERNATE_FUNC_6       5
>>>> +#define ALTERNATE_FUNC_7       6
>>>> +#define ALTERNATE_FUNC_8       7
>>>
>>> I have mixed feelings about these macros:
>>>   1. They're long to type,
>>>   2. They just map from n to n-1.
>>>
>>> Why not use plain numbers 1..8 (the alternate function numbering in the
>>> datasheet is 1-based), and subtract 1 in the C code?
>>
>> I was about to mention the same. I think you can drop this patch and use
>> the
>> numbers directly.
>
> Please be aware there are values we'll have to define here, such as the
> additional configurations we're talking about in some other part of this
> email thread.

In that case you indeed have to OR values, but...

> This may become something like:
>
> #define ALT_FUNC_1       0
> #define ALT_FUNC_2       1
> #define ALT_FUNC_3       2
> #define ALT_FUNC_4       3
> #define ALT_FUNC_5       4
> #define ALT_FUNC_6       5
> #define ALT_FUNC_7       6
> #define ALT_FUNC_8       7
>
> #define INPUT_MODE      1
> #define BIDRECTIONAL    2
> #define ...

... if you integrate the shifts in the definitions above, e.g.

#define INPUT_MODE     (1 << 16)

> #define PIN(b, p) b p
> #define PIN_MUX(func, conf)     \
>         ((func & 0xf) | ((conf) << 16))
>
> and in DTS sources you would describe a pin as
>
> pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>;
>
> We can drop the PIN macro as it does not bring anything I agree, but it's
> nicer to see imho

... you can keep plain 1-based numbers for alternate functions, and use
the definitions for flags, e.g.:

     pins = <PIN(bank, pin) (1 | INPUT_MODE | BIDIRECTIONAL)>;

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: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[jacopo mondi]

Hi Geert,

On 26/01/2017 20:54, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> Add pincontroller node compatible with the new Renesas RZ/A1
>> pincontroller driver.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
>> index 3dd427d..764006d 100644
>> --- a/arch/arm/boot/dts/r7s72100.dtsi
>> +++ b/arch/arm/boot/dts/r7s72100.dtsi
>> @@ -171,6 +171,18 @@
>>                 };
>>         };
>>
>> +       pinctrl: pinctrl@fcfe3000 {
>> +               compatible = "renesas,rza1-pinctrl";
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               #pinctrl-cells = <2>;
>
> Souldn't that be <3>?
> E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers.
>

 From pinctrl-bindings.txt:

#pinctrl-cells:	Number of pin control cells in addition to the index 
within the pin controller device instance

So here it's (2 + 1) as at least the pin index (first parameter) is 
mandatory.

We're twisting the assumption of having "index" as first, single, 
parameter, as we have <bank pin mode> and the <bank, pin> pair 
identifies a pin.

Hope this is ok, and we can re-use existing bindings even if our 
semantic is a bit different.



> 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: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Geert Uytterhoeven]

Hi Jacopo,

On Tue, Jan 31, 2017 at 11:24 AM, jacopo mondi <jacopo@jmondi.org> wrote:
> On 26/01/2017 20:54, Geert Uytterhoeven wrote:
>> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org>
>> wrote:
>>>
>>> Add pincontroller node compatible with the new Renesas RZ/A1
>>> pincontroller driver.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/r7s72100.dtsi
>>> b/arch/arm/boot/dts/r7s72100.dtsi
>>> index 3dd427d..764006d 100644
>>> --- a/arch/arm/boot/dts/r7s72100.dtsi
>>> +++ b/arch/arm/boot/dts/r7s72100.dtsi
>>> @@ -171,6 +171,18 @@
>>>                 };
>>>         };
>>>
>>> +       pinctrl: pinctrl@fcfe3000 {
>>> +               compatible = "renesas,rza1-pinctrl";
>>> +               #address-cells = <1>;
>>> +               #size-cells = <0>;
>>> +
>>> +               #pinctrl-cells = <2>;
>>
>>
>> Souldn't that be <3>?
>> E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers.
>>
>
> From pinctrl-bindings.txt:
>
> #pinctrl-cells: Number of pin control cells in addition to the index within
> the pin controller device instance

IC. I incorrectly assumed it would be the number of cells after a pinctrl
phandle, which is superfluous here as these are subnodes of the pfc node.

> So here it's (2 + 1) as at least the pin index (first parameter) is
> mandatory.
>
> We're twisting the assumption of having "index" as first, single, parameter,
> as we have <bank pin mode> and the <bank, pin> pair identifies a pin.
>
> Hope this is ok, and we can re-use existing bindings even if our semantic is
> a bit different.

That's indeed a bit of twisting ;-)

You can fix that by keeping the RZ_PIN() macro, and changing it to e.g.

#define RZ_PIN(bank, pin)     ((bank) << 16 | (pin))

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: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Laurent Pinchart]

Hi Chris,

On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote:
> On Monday, January 30, 2017, Laurent Pinchart wrote:
> >> +	pinctrl: pinctrl@fcfe3000 {
> >> +		compatible = "renesas,rza1-pinctrl";
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		#pinctrl-cells = <2>;
> >> +
> >> +		reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */
> >> +		      <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */
> > 
> > What's the reason for splitting those registers in two sets ? Maybe you
> > can explain that in the DT bindings documentation that this patch series
> > is missing ;-)
> 
> I left this out of my review comments, but even though the chip designers
> left a BIG HOLE in the memory map of the PFC controller, I don't think it
> will 'cost' you anything by just mapping the whole area (dead space and
> all) and getting rid of the "high and low" memory indexing thing that you
> are doing in the driver.
> There is nothing mapped in that dead area anyway.

For the first two areas, I agree. The third area is a separate pin controller 
for the JTAG port, not multiplexed with the GPIO ports. I even wonder whether 
it should be split in a separate DT node.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Laurent Pinchart]

Hi Chris,

On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote:
> On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> > +	/* Port 5 */
> > +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> > +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> > +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> > +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> 
> The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses
> all 16 port pins on "port 5" so I'd like to include them as well.
> 
> > +static const struct of_device_id rza1_pinctrl_of_match[] = {
> > +	{ .compatible = "renesas,rza1-pinctrl", },
> > +	{ }
> > +};
> 
> Since this PFC driver file is specifically for RZ/A1, I think a
> better compatible string would be:
> 
>   .compatible = "renesas,r7s72100-renesas-pinctrl",

Do we need to repeat "renesas" in the name ? And given that the datasheet 
names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP core 
handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive.

-- 
Regards,

Laurent Pinchart

RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Chris Brandt]

Hi Jacopo,

On  Tuesday, January 31, 2017, Jacopo Mondi wrote:
> > Since one of the benefits of using devm_kzalloc is that if the probe
> > fails and returns an error, all the memory associated with that device
> > will automatically get freed, you 'might' not need this code to free
> > memory.
> >
> > I say might because I'm not sure if returning an error here will kill
> > the driver or not. But, might be interesting to look into.
> >
> 
> No, returning an error here would not kill the driver BUT if
> dt_node_to_map fails, dt_free_map is called immediately later [1]
> 
> So here we maybe should not use device managed memory as it does not bring
> anything, do not free *map as it is freed later in dt_free_map, but
> release fngrps mux_modes and grpins, as we lose reference to them outside
> the scope of this function.
> 
> Do you agree?

Like you said, there is no benefit one way of the other.

But, doing a grep of the pinctrl directory, devm_kzalloc is used more by
the other drivers than kzalloc, so maybe we just stick with devm_kzalloc

   (baah goes the sheep)


Chris



> -----Original Message-----
> From: jacopo mondi [mailto:jacopo@jmondi.org]
> Sent: Tuesday, January 31, 2017 4:01 AM
> To: Chris Brandt <Chris.Brandt@renesas.com>; Jacopo Mondi
> <jacopo+renesas@jmondi.org>; laurent.pinchart@ideasonboard.com;
> geert+renesas@glider.be; linus.walleij@linaro.org
> Cc: linux-renesas-soc@vger.kernel.org; linux-gpio@vger.kernel.org
> Subject: Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module
> 
> Hi Chris,
> 
> On 30/01/2017 20:19, Chris Brandt wrote:
> > Hi Jacopo,
> >
> > On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> >> +
> >> +	return 0;
> >> +
> >> +free_map:
> >> +	devm_kfree(rz_pinctrl->dev, *map);
> >> +free_fngrps:
> >> +	devm_kfree(rz_pinctrl->dev, fngrps);
> >> +free_pins:
> >> +	devm_kfree(rz_pinctrl->dev, mux_modes);
> >> +	devm_kfree(rz_pinctrl->dev, grpins);
> >> +	return ret;
> >> +}
> >
> > Since one of the benefits of using devm_kzalloc is that if the probe
> > fails and returns an error, all the memory associated with that device
> > will automatically get freed, you 'might' not need this code to free
> > memory.
> >
> > I say might because I'm not sure if returning an error here will kill
> > the driver or not. But, might be interesting to look into.
> >
> 
> No, returning an error here would not kill the driver BUT if
> dt_node_to_map fails, dt_free_map is called immediately later [1]
> 
> So here we maybe should not use device managed memory as it does not bring
> anything, do not free *map as it is freed later in dt_free_map, but
> release fngrps mux_modes and grpins, as we lose reference to them outside
> the scope of this function.
> 
> Do you agree?
> 
> >
> 
> >> +#define RZ_PIN_NAME(bank, pin)						\
> >> +	PIN_##bank##_##pin
> >> +
> >> +#define RZ_PIN_DESC(b, p)						\
> >> +	{ .number = RZ_PIN_NAME(b, p),					\
> >> +	  .name = __stringify(RZ_PIN_NAME(b, p)),			\
> >
> > The hardware manual uses the names "P1_0" for ports, so it might be
> > better to match that format for consistency.
> >
> >
> 
> Noted
> 
> Thanks
>     j
> 
> 
> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/devicetree.c#L236
> 

RE: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

[Chris Brandt]

On Tuesday, January 31, 2017, Geert Uytterhoeven wrote:
> > This may become something like:
> >
> > #define ALT_FUNC_1       0
> > #define ALT_FUNC_2       1
> > #define ALT_FUNC_3       2
> > #define ALT_FUNC_4       3
> > #define ALT_FUNC_5       4
> > #define ALT_FUNC_6       5
> > #define ALT_FUNC_7       6
> > #define ALT_FUNC_8       7


ALT_FUNC_1 is still too long if you're going to OR things on 1 line in the DT.

I personally like:
#define ALT_1       0


> > #define PIN(b, p) b p
> > #define PIN_MUX(func, conf)     \
> >         ((func & 0xf) | ((conf) << 16))
> >
> > and in DTS sources you would describe a pin as
> >
> > pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>;
> >
> > We can drop the PIN macro as it does not bring anything I agree, but
> > it's nicer to see imho
> 
> ... you can keep plain 1-based numbers for alternate functions, and use
> the definitions for flags, e.g.:
> 
>      pins = <PIN(bank, pin) (1 | INPUT_MODE | BIDIRECTIONAL)>;


To Geert's point, nothing is going to be "shorter" than just using
'1','2','3',etc..


I also think "BIDIRECTIONAL" >> "BIDIR"

Or....we could make life easy on people and #define the pins that
are tricky:


#define I2C		BIDIRECTIONAL
#define SDHI_DATA	BIDIRECTIONAL
#define SDHI_CTRL /* none */
#define RSPI	/* none */
#define ETHERNET	/* none */


Chris

RE: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node

[Chris Brandt]

Hi Laurent (and Jacopo)

On Tuesday, January 31, 2017, Laurent Pinchart wrote:
> On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote:
> > On Monday, January 30, 2017, Laurent Pinchart wrote:
> > >> +	pinctrl: pinctrl@fcfe3000 {
> > >> +		compatible = "renesas,rza1-pinctrl";
> > >> +		#address-cells = <1>;
> > >> +		#size-cells = <0>;
> > >> +
> > >> +		#pinctrl-cells = <2>;
> > >> +
> > >> +		reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */
> > >> +		      <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */
> > >
> > > What's the reason for splitting those registers in two sets ? Maybe
> > > you can explain that in the DT bindings documentation that this
> > > patch series is missing ;-)
> >
> > I left this out of my review comments, but even though the chip
> > designers left a BIG HOLE in the memory map of the PFC controller, I
> > don't think it will 'cost' you anything by just mapping the whole area
> > (dead space and
> > all) and getting rid of the "high and low" memory indexing thing that
> > you are doing in the driver.
> > There is nothing mapped in that dead area anyway.
> 
> For the first two areas, I agree. The third area is a separate pin
> controller for the JTAG port, not multiplexed with the GPIO ports. I even
> wonder whether it should be split in a separate DT node.


I think we should just forget about the JTAG pins completely.
Honestly, they are 2 pins that can only be configured as JTAG or a GPIO input
(not even output).
No one is using those pins for anything other than JTAG.

IMHO, adding extra support just for those 2 pins is basically a waste of code.


Chris

RE: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Chris Brandt]

Hi Laurent,

On Tuesday, January 31, 2017, Laurent Pinchart wrote:
> On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote:
> > On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> > > +	/* Port 5 */
> > > +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> > > +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> > > +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> > > +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> >
> > The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses all 16
> > port pins on "port 5" so I'd like to include them as well.
> >
> > > +static const struct of_device_id rza1_pinctrl_of_match[] = {
> > > +	{ .compatible = "renesas,rza1-pinctrl", },
> > > +	{ }
> > > +};
> >
> > Since this PFC driver file is specifically for RZ/A1, I think a better
> > compatible string would be:
> >
> >   .compatible = "renesas,r7s72100-renesas-pinctrl",
> 
> Do we need to repeat "renesas" in the name ? And given that the datasheet
> names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP
> core handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive.


Personally, I do not like "renesas,r7s72100-renesas-pinctrl". I was simply
trying to follow the naming guidelines for DT.

I like your suggestion of "renesas,r7s72100-ports" better.


Cheers

Chris

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Laurent Pinchart]

Hi Chris,

On Tuesday 31 Jan 2017 13:48:57 Chris Brandt wrote:
> On  Tuesday, January 31, 2017, Jacopo Mondi wrote:
> >> Since one of the benefits of using devm_kzalloc is that if the probe
> >> fails and returns an error, all the memory associated with that device
> >> will automatically get freed, you 'might' not need this code to free
> >> memory.
> >> 
> >> I say might because I'm not sure if returning an error here will kill
> >> the driver or not. But, might be interesting to look into.
> > 
> > No, returning an error here would not kill the driver BUT if
> > dt_node_to_map fails, dt_free_map is called immediately later [1]
> > 
> > So here we maybe should not use device managed memory as it does not bring
> > anything, do not free *map as it is freed later in dt_free_map, but
> > release fngrps mux_modes and grpins, as we lose reference to them outside
> > the scope of this function.
> > 
> > Do you agree?
> 
> Like you said, there is no benefit one way of the other.
> 
> But, doing a grep of the pinctrl directory, devm_kzalloc is used more by
> the other drivers than kzalloc, so maybe we just stick with devm_kzalloc
> 
>    (baah goes the sheep)

Given that the devm_*() functions incur an overhead, let's use kzalloc/kfree 
directly.

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday 25 Jan 2017 19:09:43 Jacopo Mondi wrote:
> Add core module for per-pin Renesas RZ series pin controller.
> The core module allows SoC driver to register their pins and SoC
> specific operations and interfaces with pinctrl and pinmux core on their
> behalf.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/pinctrl/Kconfig             |   1 +
>  drivers/pinctrl/Makefile            |   1 +
>  drivers/pinctrl/rz-pfc/Kconfig      |  18 ++
>  drivers/pinctrl/rz-pfc/Makefile     |   1 +
>  drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++
>  6 files changed, 582 insertions(+)
>  create mode 100644 drivers/pinctrl/rz-pfc/Kconfig
>  create mode 100644 drivers/pinctrl/rz-pfc/Makefile
>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c
>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h

As Chris pointed out, s/rz-pfc/renesas/ would be a good idea. This code isn't 
specific to RZ chips (even if it's only used by them at the moment), so the rz 
prefix and suffix should probably replaced with something else (in file names 
and in the code too).

[snip]

> diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig
> new file mode 100644
> index 0000000..3714c10
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Renesas RZ pinctrl drivers
> +#
> +
> +if ARCH_RENESAS
> +
> +config PINCTRL_RZ_PINCTRL

You can add a

	depends on ARCH_RENESAS

to replace the above if. It should actually be

	depends on ARCH_RENESAS || COMPILE_TEST

to extend compile-test coverage. An even better option could be to drop the 
depends line completely, and replace def_bool with bool. That way the option 
will only be enabled if explicitly selected by another option (as done in 
patch 2/5).

> +	select PINMUX
> +	select PINCONF
> +	select GPIOLIB
> +	select GENERIC_PINCONF
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCTRL_GROUPS
> +	def_bool y

Won't this enable the driver by default on all platforms, while only RZ needs 
it ? It's also customary to have the bool/tristate line as the very first line 
in the config option.

> +	help
> +	  This enables pin control drivers for Renesas RZ platforms
> +
> +endif

[snip]

> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.c
> b/drivers/pinctrl/rz-pfc/pinctrl-rz.c new file mode 100644
> index 0000000..3efbf03
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c
> @@ -0,0 +1,447 @@
> +/*
> + * Pinctrl support for Renesas RZ Series
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + * Copyright (C) 2017 Renesas Electronics Corporation
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +

No need for a blank line here.

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +

Or here.

> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "../core.h"
> +#include "../devicetree.h"
> +#include "../pinmux.h"
> +
> +#include "pinctrl-rz.h"
> +
> +#define DRIVER_NAME		"rz-pinctrl"
> +#define RZ_PIN_ARGS_COUNT	3
> +
> +/**
> + * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position
> [bank:pin]
> + *
> + * This can be improved, as it walks all the pins reported by the SoC
> driver
> + *
> + * @return: pin number between [0 - npins]; -1 if not found
> + */
> +static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl,
> +				   unsigned int bank, unsigned int pin)
> +{
> +	struct rz_pinctrl_info *info;
> +	struct rz_pin_desc *rz_pin;
> +	int i, npins;

i and npins are never negative, you can make them unsigned int. One variable 
declaration per line is also preferred.

> +
> +	info = rz_pinctrl->info;

You can move this to the variable declaration line.

> +	npins = info->npins;
> +	for (i = 0; i < npins; ++i) {

You can drop the npins variable and use info->npins directly.

> +		rz_pin = &info->pins[i];
> +
> +		/*
> +		 * return the pin index in the array, not the pin number,
> +		 * so that we can access it easily when muxing group's pins

Please start sentences with a capital letter and end them with a period.

> +		 */
> +		if (rz_pin->bank == bank && rz_pin->pin == pin)
> +			return i;
> +	}
> +
> +	return -1;

How about -EINVAL ? -1 is -EPERM, which would not be an appropriate error code 
if you end up leaking it to upper layers.

> +}
> +
> +/* ------------------------------------------------------------------------
> + * pinctrl operations
> + */
> +static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file
> *s,
> +			    unsigned int pin)
> +{
> +	struct rz_pinctrl_dev *rz_pinctrl;
> +	struct rz_pinctrl_info *info;
> +
> +	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +	info = rz_pinctrl->info;

You can also assign at declaration time here.

> +	seq_printf(s, "%s %s\n", info->pins[pin].name, DRIVER_NAME);

The pinctrl core already prints the pin name:

                seq_printf(s, "pin %d (%s) ", pin, desc->name);

                /* Driver-specific info per pin */
                if (ops->pin_dbg_show)
                        ops->pin_dbg_show(pctldev, s, pin);

You can just print the driver name here. I'm actually wondering whether 
printing the driver name is useful, or if we could simply drop this function.

> +}
> +
> +/**
> + * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups
> and
> + *			 functions

I don't think we have groups and functions, do we ?

> + *
> + * Pins for RZ series pin controller described by "renesas-rz,pins"
> property
> + * are arrays of u32 values in the form:

As mentioned elsewhere, "renesas,pins" will do. We could even use "pins", but 
then we'd have to comply with the pinctrl generic bindings (with a bin number 
in the first call instead of separate bank and pin for instance).

> + *
> + * "renesas-rz,pins" = <BANK PIN MUX>, ... ;
> + *
> + * Parse the list arguments and identify pins through their position
> + * (bank and pin offset) and save the provided mux mode for later use.
> + *
> + * TODO: the array can be expended to support additional parameters for
> + *	 pin configurations values (IO direction etc)
> + *
> + * @pctldev: pin controller device
> + * @np_config: device tree node to parse
> + * @map: pointer to pin map (output)
> + * @num_maps: number of collected maps (output)
> + *
> + * @return: 0 for success; != 0 otherwise

"; a negative error code otherwise" ? The function never returns a positive 
value.

I don't think kerneldoc uses @return. Please try to compile the documentation 
and fix errors and warnings.

> + */
> +static int rz_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			     struct device_node *np_config,
> +			     struct pinctrl_map **map, unsigned int *num_maps)
> +{
> +	struct rz_pinctrl_dev *rz_pinctrl;
> +	const char *prop_name = "renesas-rz,pins",
> +		   *grpname, **fngrps;
> +	unsigned int *grpins,
> +		     *mux_modes;

One variable declaration per line please.

> +	int i, npins, ret;

i is never negative, you can make it an unsigned int.

> +
> +	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);

You can initialize the variable at declaration time.

> +	npins = pinctrl_count_index_with_args(np_config, prop_name);
> +	if (npins <= 0) {
> +		dev_err(rz_pinctrl->dev, "Missing pins configuration prop\n");

Maybe (... "Missing %s property\n", prop_name); to be more explicit ?

> +		return -EINVAL;
> +	}
> +
> +	mux_modes = devm_kzalloc(rz_pinctrl->dev, sizeof(*mux_modes) * npins,
> +				 GFP_KERNEL);
> +	grpins = devm_kzalloc(rz_pinctrl->dev, sizeof(*grpins) * npins,
> +			      GFP_KERNEL);

As explained in another e-mail, you can use kzalloc() here instead of 
devm_kzalloc(). Or, even better, kcalloc().

> +	if (unlikely(!grpins || !mux_modes))
> +		return -ENOMEM;
> +
> +	/*
> +	 * functions are made of 1 group only;
> +	 * in facts, functions and groups are identical for RZ pin controller,
> +	 * except that functions carry an array of mux modes (aka alternate
> +	 * functions IDs)
> +	 */
> +	fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL);
> +	if (unlikely(!fngrps)) {
> +		ret = -ENOMEM;
> +		goto free_pins;
> +	}
> +
> +	*num_maps = 0;
> +	(*map) = devm_kzalloc(rz_pinctrl->dev, sizeof(**map), GFP_KERNEL);

No need for parentheses around *map.

> +	if (unlikely(!*map)) {
> +		ret = -ENOMEM;
> +		goto free_fngrps;
> +	}
> +
> +	/* collect pin numbers and mux confs to create groups and functions */
> +	for (i = 0; i < npins; ++i) {
> +		struct of_phandle_args of_pins_args;
> +		unsigned int bank, offs, mode, pin_idx;

You can spell offset fully.

> +
> +		/* DTS identifies pin by position: bank and pin offset */
> +		ret = pinctrl_parse_index_with_args(np_config, prop_name, i,
> +						    &of_pins_args);
> +		if (ret)
> +			goto free_map;
> +
> +		if (of_pins_args.args_count < RZ_PIN_ARGS_COUNT) {
> +			dev_err(rz_pinctrl->dev,
> +				"Wrong arguments number for renesas-rz,pins");
> +			ret = -EINVAL;
> +			goto free_map;
> +		}
> +
> +		bank = of_pins_args.args[0];
> +		offs = of_pins_args.args[1];
> +		mode = of_pins_args.args[2];
> +
> +		/* pin_idx is the index on the static pin array */
> +		pin_idx = rz_pinctrl_pos_to_index(rz_pinctrl, bank, offs);
> +		if (pin_idx < 0) {
> +			dev_err(rz_pinctrl->dev,
> +				"Invalid pin position %d-%d\n", bank, offs);

bank and offset are unsigned, you should use %u.

> +			ret = -EINVAL;
> +			goto free_map;
> +		}
> +
> +		grpins[i] = pin_idx;
> +		mux_modes[i] = mode;
> +	}
> +
> +	grpname = np_config->name;
> +	fngrps[0] = grpname;
> +
> +	mutex_lock(&rz_pinctrl->mutex);

This seems weird. The pinctrl generic functions indeed state that the caller 
must take care of locking, but there doesn't seem to be any lock protecting 
races between .dt_node_to_map() calling the generic functions below, and the 
generic .get_group_*() handlers.

> +	ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins, 
NULL);
> +	if (ret) {
> +		mutex_unlock(&rz_pinctrl->mutex);
> +		goto free_map;
> +	}
> +
> +	ret = pinmux_generic_add_function(pctldev, grpname, fngrps,
> +					  1, mux_modes);
> +	if (ret) {
> +		mutex_unlock(&rz_pinctrl->mutex);
> +		goto free_map;
> +	}
> +	mutex_unlock(&rz_pinctrl->mutex);
> +
> +	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> +	(*map)->data.mux.group = grpname;
> +	(*map)->data.mux.function = grpname;
> +	*num_maps = 1;
> +
> +	return 0;
> +
> +free_map:
> +	devm_kfree(rz_pinctrl->dev, *map);
> +free_fngrps:
> +	devm_kfree(rz_pinctrl->dev, fngrps);
> +free_pins:
> +	devm_kfree(rz_pinctrl->dev, mux_modes);
> +	devm_kfree(rz_pinctrl->dev, grpins);
> +	return ret;
> +}
> +
> +/**
> + * rz_dt_free_map()
> + */
> +static void rz_dt_free_map(struct pinctrl_dev *pctldev,
> +			   struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	devm_kfree(rz_pinctrl->dev, map);

Shouldn't you also free the other allocated pieces of memory (fngrps, 
mux_modes and grpins) ?

> +}
> +
> +static const struct pinctrl_ops rz_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,
> +	.pin_dbg_show = rz_pin_dbg_show,
> +	.dt_node_to_map = rz_dt_node_to_map,
> +	.dt_free_map = rz_dt_free_map,
> +};
> +
> +/* ------------------------------------------------------------------------
> + * pinconfig operations
> + */
> +
> +/* TODO */

Yes ? :-)

> +/* ------------------------------------------------------------------------
> + * pinmux operations
> + */
> +
> +/**
> + * rz_pinmux_set() - Retrieve pins from a group and apply mux settings
> + *
> + * @pctldev: pin controller device
> + * @selector: Function selector
> + * @group: Group selector
> + * @return: 0 for success; != otherwise

Same comment as above.

> + */
> +static int rz_pinmux_set(struct pinctrl_dev *pctldev, unsigned int
> selector,
> +			 unsigned int group)
> +{
> +	struct rz_pinctrl_dev *rz_pinctrl;
> +	struct rz_pinctrl_info *info;
> +	struct rz_pinctrl_ops *ops;
> +	struct group_desc *grp;
> +	struct function_desc *func;
> +	unsigned int npins, *mux_modes;
> +	int i;

i is never negative.

> +
> +	rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +	info = rz_pinctrl->info;
> +	ops = info->ops;

You can do this at variable declaration time.

> +	grp = pinctrl_generic_get_group(pctldev, group);
> +	if (!grp)
> +		return -EINVAL;
> +
> +	func = pinmux_generic_get_function(pctldev, selector);
> +	if (!func)
> +		return -EINVAL;
> +
> +	npins = grp->num_pins;

No need for an intermediate variable, use grp->num_pins directly.

> +	mux_modes = (unsigned int *)func->data;
> +
> +	for (i = 0; i < npins; ++i) {
> +		unsigned int pin_idx, mux_mode;
> +		struct rz_pin_desc *pin;
> +		int ret;
> +
> +		/*
> +		 * use pin index to retrieve pin descriptor;
> +		 * then provide it to the set_mux SoC operation
> +		 */
> +		pin_idx = grp->pins[i];
> +		pin = &info->pins[pin_idx];
> +		mux_mode = mux_modes[i];
> +
> +		if (!ops || !ops->set_mux) {
> +			dev_err(rz_pinctrl->dev, "Pin muxing not 
supported\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = ops->set_mux(rz_pinctrl, pin, mux_mode);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +struct pinmux_ops rz_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 = rz_pinmux_set,
> +};
> +
> +/* ------------------------------------------------------------------------
> + * rz pinctrl operations
> + */
> +
> +/**
> + * rz_add_pins() - Enumerate pins reported by SoC driver in pin desc array

Does this function really enumerate pins ?
> + *
> + * @rz_pinctrl: RZ pincontroller
> + *
> + * @return: 0 for success; != 0 otherwise

Same as above, and some comment for below.

> + */
> +static int rz_add_pins(struct rz_pinctrl_dev *rz_pinctrl)
> +{
> +	struct rz_pinctrl_info *info;
> +	struct rz_pin_desc *pin;
> +	unsigned int npins, i;
> +
> +	info = rz_pinctrl->info;
> +	npins = info->npins;

Fold this with variable declaration. I'd get rid of the npins variable, you 
can use info->npins.

> +	rz_pinctrl->pins = devm_kzalloc(rz_pinctrl->dev,
> +					npins * sizeof(*rz_pinctrl->pins),
> +					GFP_KERNEL);

kcalloc.

> +	if (unlikely(!rz_pinctrl->pins))
> +		return -ENOMEM;
> +
> +	rz_pinctrl->desc.pins = rz_pinctrl->pins;
> +	rz_pinctrl->desc.npins = npins;
> +
> +	for (i = 0; i < npins; ++i) {
> +		pin = &info->pins[i];
> +
> +		rz_pinctrl->pins[i].number = pin->number;
> +		rz_pinctrl->pins[i].name = pin->name;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * rz_pinctrl_map_res() - Map memory resources for pincontroller

You can call the function ..._map_resources.

> + * @pdev: platform device
> + * @rz_pincrl: RZ pincontroller
> + *
> + * @return: 0 for success; != 0 otherwise
> + */
> +static int rz_pinctrl_map_res(struct platform_device *pdev,
> +			      struct rz_pinctrl_dev *rz_pinctrl)
> +{
> +	struct resource *res;
> +	struct rz_pinctrl_res *rz_res;
> +	unsigned int i, nres;
> +
> +	nres = pdev->num_resources;
> +	rz_res = devm_kzalloc(&pdev->dev, nres * sizeof(*rz_res),
> +			      GFP_KERNEL);

kcalloc() here too.

> +	if (unlikely(!rz_res))
> +		return -ENOMEM;
> +
> +	rz_pinctrl->nres = nres;
> +	rz_pinctrl->res = rz_res;
> +
> +	for (i = 0; i < nres; i++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!res)
> +			return -ENODEV;
> +
> +		rz_res = &rz_pinctrl->res[i];
> +
> +		rz_res->start = res->start;
> +		rz_res->size = resource_size(res);

If you remove the debugging statement from patch 2/5 you can drop the start 
and size fields.

> +		rz_res->base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(rz_res->base))
> +			return PTR_ERR(rz_res->base);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * rz_pinctrl_probe() - Register pincontroller driver operations
> + *
> + * @pdev: platform device
> + * @info: SoC device info structure
> + */
> +int rz_pinctrl_probe(struct platform_device *pdev, struct rz_pinctrl_info
> *info)
> +{
> +	int ret;
> +	struct rz_pinctrl_dev *rz_pinctrl;
> +
> +	rz_pinctrl = devm_kzalloc(&pdev->dev, sizeof(*rz_pinctrl), 
GFP_KERNEL);

kzalloc()

> +	if (!rz_pinctrl)
> +		return -ENOMEM;
> +
> +	rz_pinctrl->dev = &pdev->dev;
> +	rz_pinctrl->info = info;
> +
> +	ret = rz_pinctrl_map_res(pdev, rz_pinctrl);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, rz_pinctrl);
> +
> +	rz_pinctrl->desc.name = DRIVER_NAME;
> +	rz_pinctrl->desc.pctlops = &rz_pinctrl_ops;
> +	rz_pinctrl->desc.pmxops = &rz_pinmux_ops;
> +	rz_pinctrl->desc.owner = THIS_MODULE;
> +
> +	ret = rz_add_pins(rz_pinctrl);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&rz_pinctrl->mutex);

I'd move this earlier, to make sure the mutex is initialized before it gets 
used in case we start using it in rz_pinctrl_map_res() or rz_add_pins() later.

> +	ret = pinctrl_register_and_init(&rz_pinctrl->desc, rz_pinctrl->dev,
> +					rz_pinctrl, &rz_pinctrl->pctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not register rz pinctrl driver\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void rz_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct rz_pinctrl_dev *rz_pinctrl = platform_get_drvdata(pdev);
> +
> +	pinctrl_unregister(rz_pinctrl->pctl);
> +}

The code is always compiled in, and the device never goes away. I'd drop 
remove support, as it's completely useless. You then need to set the 
device_driver structure suppress_bind_attrs field.

> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> +MODULE_DESCRIPTION("Pinctrl driver for Reneas RZ series");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.h
> b/drivers/pinctrl/rz-pfc/pinctrl-rz.h new file mode 100644
> index 0000000..b873c89
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.h
> @@ -0,0 +1,114 @@
> +/*
> + * Pinctrl support for Renesas RZ Series
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + * Copyright (C) 2017 Renesas Electronics Corporation
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef __RZ_PINCTRL_H__
> +#define __RZ_PINCTRL_H__
> +
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/types.h>
> +
> +/*
> ---------------------------------------------------------------------------
> - + * pinctrl-rz data types
> + */
> +#define RZ_PIN_NAME(bank, pin)						
\
> +	PIN_##bank##_##pin
> +
> +#define RZ_PIN_DESC(b, p)						\
> +	{ .number = RZ_PIN_NAME(b, p),					\
> +	  .name = __stringify(RZ_PIN_NAME(b, p)),			\
> +	  .bank = b, .pin = p }
> +
> +/**
> + * rz_pin_desc - Single RZ pin descriptor
> + *
> + * @number: pin number as enumerated by SoC driver
> + * @name: pin name as reported by SoC driver
> + * @bank: pin bank location
> + * @pin: pin register offset
> + */
> +struct rz_pin_desc {
> +	unsigned int bank, pin;
> +	unsigned int number;
> +	const char *name;
> +};
> +
> +/**
> + * rz_pinctrl_info - SoC info data
> + *
> + * @ops: SoC specific operations
> + * @npins: number of pins
> + * @pins: pin array
> + */
> +struct rz_pinctrl_ops;
> +struct rz_pinctrl_info {
> +	struct rz_pinctrl_ops *ops;
> +
> +	unsigned int npins;
> +	struct rz_pin_desc *pins;
> +};
> +
> +/**
> + * rz_pinctrl_res - Memory resource for RZ SoC
> + *
> + * @start: physical address base
> + * @size: memory region size
> + * @base: logical address base
> + */
> +struct rz_pinctrl_res {
> +	resource_size_t start;
> +	resource_size_t size;
> +	void __iomem *base;
> +};
> +
> +/**
> + * rz_pinctrl_dev - RZ pincontroller device
> + *
> + * @dev: device
> + * @info: SoC data
> + * @nres: number of memory regions
> + * @res: memory regions
> + * @mutex: protect pin*_generic functions
> + * @pins: pin array for pinctrl core
> + * @desc: pincontroller desc for pinctrl core
> + * @pctl: pinctrl device
> + */
> +struct rz_pinctrl_dev {
> +	struct device *dev;
> +
> +	struct rz_pinctrl_info *info;
> +
> +	unsigned int nres;
> +	struct rz_pinctrl_res *res;
> +
> +	struct mutex mutex;
> +
> +	struct pinctrl_pin_desc *pins;
> +	struct pinctrl_desc desc;
> +	struct pinctrl_dev *pctl;
> +};
> +
> +/**
> + * rz_pinctrl_ops - SoC operations
> + *
> + * @set_mux: perform pin muxing on SoC registers
> + */
> +struct rz_pinctrl_ops {
> +	int (*set_mux)(struct rz_pinctrl_dev *, struct rz_pin_desc *,
> +		       unsigned int mux_mode);
> +};
> +
> +/* ------------------------------------------------------------------------
> + * pinctrl-rz prototypes
> + */
> +int rz_pinctrl_probe(struct platform_device *pdev,
> +		     struct rz_pinctrl_info *info);
> +void rz_pinctrl_remove(struct platform_device *pdev);
> +
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday 25 Jan 2017 19:09:44 Jacopo Mondi wrote:
> Add pin controller driver for Renesas RZ/A1 SoC.
> The SoC driver registers to rz-pfc core module and provides pin
> description array and SoC specific pin mux operation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/pinctrl/rz-pfc/Kconfig        |   7 +
>  drivers/pinctrl/rz-pfc/Makefile       |   1 +
>  drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 +++++++++++++++++++++++++++++++
>  3 files changed, 355 insertions(+)
>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> 
> diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig
> index 3714c10..2b9c111 100644
> --- a/drivers/pinctrl/rz-pfc/Kconfig
> +++ b/drivers/pinctrl/rz-pfc/Kconfig
> @@ -15,4 +15,11 @@ config PINCTRL_RZ_PINCTRL
>  	help
>  	  This enables pin control drivers for Renesas RZ platforms
> 
> +config PINCTRL_RZA1_PINCTRL
> +    depends on ARCH_R7S72100
> +	select PINCTRL_RZ_PINCTRL
> +	def_bool y
> +	help
> +	  This enables pin control drivers for Renesas RZ/A1 SoC
> +
>  endif
> diff --git a/drivers/pinctrl/rz-pfc/Makefile
> b/drivers/pinctrl/rz-pfc/Makefile index cba8283..e3befa5 100644
> --- a/drivers/pinctrl/rz-pfc/Makefile
> +++ b/drivers/pinctrl/rz-pfc/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PINCTRL_RZ_PINCTRL)		+= pinctrl-rz.o
> +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL)		+= pinctrl-rza1.o
> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c new file mode 100644
> index 0000000..221f048
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> @@ -0,0 +1,347 @@
> +/*
> + * Pinctrl support for Renesas RZ/A1 SoC
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + * Copyright (C) 2017 Renesas Electronics Corporation
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_device.h>

You can move those two lines between module.h and platform_device.h, there's 
no need to keep them separate.

> +
> +#include "pinctrl-rz.h"
> +
> +#define RZA1_REG_DBG
> +
> +/* memory resources indexes */
> +#define MEM_RES_LOW	0 /* PORTn_base */
> +#define MEM_RES_HIGH	1 /* PORTn_base + 0x4000 */
> +
> +/* displacements from PORTn_base */
> +#define PMC_REG		0x400
> +#define PFC_REG		0x500
> +#define PFCE_REG	0x600
> +#define PFCEA_REG	0xA00

The kernel mostly uses lowercase for hex values.

> +
> +/* displacements from PORTn_base + 0x4000 */
> +#define PIBC_REG	0x000
> +#define PBDC_REG	0x100
> +#define PIPC_REG	0x200
> +
> +/* build register address using memory base, offset and bank number */
> +#define RZA1_ADDR(mem_base, reg, bank)	\
> +	((mem_base) + (reg) + (bank * 4))

You should put parentheses around bank:

	((mem_base) + (reg) + (bank) * 4)

to avoid side effects if the bank parameter isn't a constant (that's not the 
case here, but it's a good practice anyway).

> +
> +/* ------------------------------------------------------------------------
> + * pin enumeration
> + */
> +enum rza1_pins {
> +	/* Port 0 */
> +	RZ_PIN_NAME(0, 0) = 0, RZ_PIN_NAME(0, 1), RZ_PIN_NAME(0, 2),

Is there a need for = 0 ?

> +	RZ_PIN_NAME(0, 3), RZ_PIN_NAME(0, 4), RZ_PIN_NAME(0, 5),
> +
> +	/* Port 1 */
> +	RZ_PIN_NAME(1, 0), RZ_PIN_NAME(1, 1), RZ_PIN_NAME(1, 2),
> +	RZ_PIN_NAME(1, 3), RZ_PIN_NAME(1, 4), RZ_PIN_NAME(1, 5),
> +	RZ_PIN_NAME(1, 6), RZ_PIN_NAME(1, 7), RZ_PIN_NAME(1, 8),
> +	RZ_PIN_NAME(1, 9), RZ_PIN_NAME(1, 10), RZ_PIN_NAME(1, 11),
> +	RZ_PIN_NAME(1, 12), RZ_PIN_NAME(1, 13), RZ_PIN_NAME(1, 14),
> +	RZ_PIN_NAME(1, 15),
> +
> +	/* Port 2 */
> +	RZ_PIN_NAME(2, 0), RZ_PIN_NAME(2, 1), RZ_PIN_NAME(2, 2),
> +	RZ_PIN_NAME(2, 3), RZ_PIN_NAME(2, 4), RZ_PIN_NAME(2, 5),
> +	RZ_PIN_NAME(2, 6), RZ_PIN_NAME(2, 7), RZ_PIN_NAME(2, 8),
> +	RZ_PIN_NAME(2, 9), RZ_PIN_NAME(2, 10), RZ_PIN_NAME(2, 11),
> +	RZ_PIN_NAME(2, 12), RZ_PIN_NAME(2, 13), RZ_PIN_NAME(2, 14),
> +	RZ_PIN_NAME(2, 15),
> +
> +	/* Port 3 */
> +	RZ_PIN_NAME(3, 0), RZ_PIN_NAME(3, 1), RZ_PIN_NAME(3, 2),
> +	RZ_PIN_NAME(3, 3), RZ_PIN_NAME(3, 4), RZ_PIN_NAME(3, 5),
> +	RZ_PIN_NAME(3, 6), RZ_PIN_NAME(3, 7), RZ_PIN_NAME(3, 8),
> +	RZ_PIN_NAME(3, 9), RZ_PIN_NAME(3, 10), RZ_PIN_NAME(3, 11),
> +	RZ_PIN_NAME(3, 12), RZ_PIN_NAME(3, 13), RZ_PIN_NAME(3, 14),
> +	RZ_PIN_NAME(3, 15),
> +
> +	/* Port 4 */
> +	RZ_PIN_NAME(4, 0), RZ_PIN_NAME(4, 1), RZ_PIN_NAME(4, 2),
> +	RZ_PIN_NAME(4, 3), RZ_PIN_NAME(4, 4), RZ_PIN_NAME(4, 5),
> +	RZ_PIN_NAME(4, 6), RZ_PIN_NAME(4, 7), RZ_PIN_NAME(4, 8),
> +	RZ_PIN_NAME(4, 9), RZ_PIN_NAME(4, 10), RZ_PIN_NAME(4, 11),
> +	RZ_PIN_NAME(4, 12), RZ_PIN_NAME(4, 13), RZ_PIN_NAME(4, 14),
> +	RZ_PIN_NAME(4, 15),
> +
> +	/* Port 5 */
> +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> +
> +	/* Port 6 */
> +	RZ_PIN_NAME(6, 0), RZ_PIN_NAME(6, 1), RZ_PIN_NAME(6, 2),
> +	RZ_PIN_NAME(6, 3), RZ_PIN_NAME(6, 4), RZ_PIN_NAME(6, 5),
> +	RZ_PIN_NAME(6, 6), RZ_PIN_NAME(6, 7), RZ_PIN_NAME(6, 8),
> +	RZ_PIN_NAME(6, 9), RZ_PIN_NAME(6, 10), RZ_PIN_NAME(6, 11),
> +	RZ_PIN_NAME(6, 12), RZ_PIN_NAME(6, 13), RZ_PIN_NAME(6, 14),
> +	RZ_PIN_NAME(6, 15),
> +
> +	/* Port 7 */
> +	RZ_PIN_NAME(7, 0), RZ_PIN_NAME(7, 1), RZ_PIN_NAME(7, 2),
> +	RZ_PIN_NAME(7, 3), RZ_PIN_NAME(7, 4), RZ_PIN_NAME(7, 5),
> +	RZ_PIN_NAME(7, 6), RZ_PIN_NAME(7, 7), RZ_PIN_NAME(7, 8),
> +	RZ_PIN_NAME(7, 9), RZ_PIN_NAME(7, 10), RZ_PIN_NAME(7, 11),
> +	RZ_PIN_NAME(7, 12), RZ_PIN_NAME(7, 13), RZ_PIN_NAME(7, 14),
> +	RZ_PIN_NAME(7, 15),
> +
> +	/* Port 8 */
> +	RZ_PIN_NAME(8, 0), RZ_PIN_NAME(8, 1), RZ_PIN_NAME(8, 2),
> +	RZ_PIN_NAME(8, 3), RZ_PIN_NAME(8, 4), RZ_PIN_NAME(8, 5),
> +	RZ_PIN_NAME(8, 6), RZ_PIN_NAME(8, 7), RZ_PIN_NAME(8, 8),
> +	RZ_PIN_NAME(8, 9), RZ_PIN_NAME(8, 10), RZ_PIN_NAME(8, 11),
> +	RZ_PIN_NAME(8, 12), RZ_PIN_NAME(8, 13), RZ_PIN_NAME(8, 14),
> +	RZ_PIN_NAME(8, 15),
> +
> +	/* Port 9 */
> +	RZ_PIN_NAME(9, 0), RZ_PIN_NAME(9, 1), RZ_PIN_NAME(9, 2),
> +	RZ_PIN_NAME(9, 3), RZ_PIN_NAME(9, 4), RZ_PIN_NAME(9, 5),
> +	RZ_PIN_NAME(9, 6), RZ_PIN_NAME(9, 7),
> +
> +	/* Port 10 */
> +	RZ_PIN_NAME(10, 0), RZ_PIN_NAME(10, 1), RZ_PIN_NAME(10, 2),
> +	RZ_PIN_NAME(10, 3), RZ_PIN_NAME(10, 4), RZ_PIN_NAME(10, 5),
> +	RZ_PIN_NAME(10, 6), RZ_PIN_NAME(10, 7), RZ_PIN_NAME(10, 8),
> +	RZ_PIN_NAME(10, 9), RZ_PIN_NAME(10, 10), RZ_PIN_NAME(10, 11),
> +	RZ_PIN_NAME(10, 12), RZ_PIN_NAME(10, 13), RZ_PIN_NAME(10, 14),
> +	RZ_PIN_NAME(10, 15),
> +
> +	/* Port 10 */

s/Port 10/Port 11/

> +	RZ_PIN_NAME(11, 0), RZ_PIN_NAME(11, 1), RZ_PIN_NAME(11, 2),
> +	RZ_PIN_NAME(11, 3), RZ_PIN_NAME(11, 4), RZ_PIN_NAME(11, 5),
> +	RZ_PIN_NAME(11, 6), RZ_PIN_NAME(11, 7), RZ_PIN_NAME(11, 8),
> +	RZ_PIN_NAME(11, 9), RZ_PIN_NAME(11, 10), RZ_PIN_NAME(11, 11),
> +	RZ_PIN_NAME(11, 12), RZ_PIN_NAME(11, 13), RZ_PIN_NAME(11, 14),
> +	RZ_PIN_NAME(11, 15),
> +};
> +
> +struct rz_pin_desc pins[] = {
> +	/* Port 0 */
> +	RZ_PIN_DESC(0, 0), RZ_PIN_DESC(0, 1), RZ_PIN_DESC(0, 2),
> +	RZ_PIN_DESC(0, 3), RZ_PIN_DESC(0, 4), RZ_PIN_DESC(0, 5),
> +
> +	/* Port 1 */
> +	RZ_PIN_DESC(1, 0), RZ_PIN_DESC(1, 1), RZ_PIN_DESC(1, 2),
> +	RZ_PIN_DESC(1, 3), RZ_PIN_DESC(1, 4), RZ_PIN_DESC(1, 5),
> +	RZ_PIN_DESC(1, 6), RZ_PIN_DESC(1, 7), RZ_PIN_DESC(1, 8),
> +	RZ_PIN_DESC(1, 9), RZ_PIN_DESC(1, 10), RZ_PIN_DESC(1, 11),
> +	RZ_PIN_DESC(1, 12), RZ_PIN_DESC(1, 13), RZ_PIN_DESC(1, 14),
> +	RZ_PIN_DESC(1, 15),
> +
> +	/* Port 2 */
> +	RZ_PIN_DESC(2, 0), RZ_PIN_DESC(2, 1), RZ_PIN_DESC(2, 2),
> +	RZ_PIN_DESC(2, 3), RZ_PIN_DESC(2, 4), RZ_PIN_DESC(2, 5),
> +	RZ_PIN_DESC(2, 6), RZ_PIN_DESC(2, 7), RZ_PIN_DESC(2, 8),
> +	RZ_PIN_DESC(2, 9), RZ_PIN_DESC(2, 10), RZ_PIN_DESC(2, 11),
> +	RZ_PIN_DESC(2, 12), RZ_PIN_DESC(2, 13), RZ_PIN_DESC(2, 14),
> +	RZ_PIN_DESC(2, 15),
> +
> +	/* Port 3 */
> +	RZ_PIN_DESC(3, 0), RZ_PIN_DESC(3, 1), RZ_PIN_DESC(3, 2),
> +	RZ_PIN_DESC(3, 3), RZ_PIN_DESC(3, 4), RZ_PIN_DESC(3, 5),
> +	RZ_PIN_DESC(3, 6), RZ_PIN_DESC(3, 7), RZ_PIN_DESC(3, 8),
> +	RZ_PIN_DESC(3, 9), RZ_PIN_DESC(3, 10), RZ_PIN_DESC(3, 11),
> +	RZ_PIN_DESC(3, 12), RZ_PIN_DESC(3, 13), RZ_PIN_DESC(3, 14),
> +	RZ_PIN_DESC(3, 15),
> +
> +	/* Port 4 */
> +	RZ_PIN_DESC(4, 0), RZ_PIN_DESC(4, 1), RZ_PIN_DESC(4, 2),
> +	RZ_PIN_DESC(4, 3), RZ_PIN_DESC(4, 4), RZ_PIN_DESC(4, 5),
> +	RZ_PIN_DESC(4, 6), RZ_PIN_DESC(4, 7), RZ_PIN_DESC(4, 8),
> +	RZ_PIN_DESC(4, 9), RZ_PIN_DESC(4, 10), RZ_PIN_DESC(4, 11),
> +	RZ_PIN_DESC(4, 12), RZ_PIN_DESC(4, 13), RZ_PIN_DESC(4, 14),
> +	RZ_PIN_DESC(4, 15),
> +
> +	/* Port 5 */
> +	RZ_PIN_DESC(5, 0), RZ_PIN_DESC(5, 1), RZ_PIN_DESC(5, 2),
> +	RZ_PIN_DESC(5, 3), RZ_PIN_DESC(5, 4), RZ_PIN_DESC(5, 5),
> +	RZ_PIN_DESC(5, 6), RZ_PIN_DESC(5, 7), RZ_PIN_DESC(5, 8),
> +	RZ_PIN_DESC(5, 9), RZ_PIN_DESC(5, 10),
> +
> +	/* Port 6 */
> +	RZ_PIN_DESC(6, 0), RZ_PIN_DESC(6, 1), RZ_PIN_DESC(6, 2),
> +	RZ_PIN_DESC(6, 3), RZ_PIN_DESC(6, 4), RZ_PIN_DESC(6, 5),
> +	RZ_PIN_DESC(6, 6), RZ_PIN_DESC(6, 7), RZ_PIN_DESC(6, 8),
> +	RZ_PIN_DESC(6, 9), RZ_PIN_DESC(6, 10), RZ_PIN_DESC(6, 11),
> +	RZ_PIN_DESC(6, 12), RZ_PIN_DESC(6, 13), RZ_PIN_DESC(6, 14),
> +	RZ_PIN_DESC(6, 15),
> +
> +	/* Port 7 */
> +	RZ_PIN_DESC(7, 0), RZ_PIN_DESC(7, 1), RZ_PIN_DESC(7, 2),
> +	RZ_PIN_DESC(7, 3), RZ_PIN_DESC(7, 4), RZ_PIN_DESC(7, 5),
> +	RZ_PIN_DESC(7, 6), RZ_PIN_DESC(7, 7), RZ_PIN_DESC(7, 8),
> +	RZ_PIN_DESC(7, 9), RZ_PIN_DESC(7, 10), RZ_PIN_DESC(7, 11),
> +	RZ_PIN_DESC(7, 12), RZ_PIN_DESC(7, 13), RZ_PIN_DESC(7, 14),
> +	RZ_PIN_DESC(7, 15),
> +
> +	/* Port 8 */
> +	RZ_PIN_DESC(8, 0), RZ_PIN_DESC(8, 1), RZ_PIN_DESC(8, 2),
> +	RZ_PIN_DESC(8, 3), RZ_PIN_DESC(8, 4), RZ_PIN_DESC(8, 5),
> +	RZ_PIN_DESC(8, 6), RZ_PIN_DESC(8, 7), RZ_PIN_DESC(8, 8),
> +	RZ_PIN_DESC(8, 9), RZ_PIN_DESC(8, 10), RZ_PIN_DESC(8, 11),
> +	RZ_PIN_DESC(8, 12), RZ_PIN_DESC(8, 13), RZ_PIN_DESC(8, 14),
> +	RZ_PIN_DESC(8, 15),
> +
> +	/* Port 9 */
> +	RZ_PIN_DESC(9, 0), RZ_PIN_DESC(9, 1), RZ_PIN_DESC(9, 2),
> +	RZ_PIN_DESC(9, 3), RZ_PIN_DESC(9, 4), RZ_PIN_DESC(9, 5),
> +	RZ_PIN_DESC(9, 6), RZ_PIN_DESC(9, 7),
> +
> +	/* Port 10 */
> +	RZ_PIN_DESC(10, 0), RZ_PIN_DESC(10, 1), RZ_PIN_DESC(10, 2),
> +	RZ_PIN_DESC(10, 3), RZ_PIN_DESC(10, 4), RZ_PIN_DESC(10, 5),
> +	RZ_PIN_DESC(10, 6), RZ_PIN_DESC(10, 7), RZ_PIN_DESC(10, 8),
> +	RZ_PIN_DESC(10, 9), RZ_PIN_DESC(10, 10), RZ_PIN_DESC(10, 11),
> +	RZ_PIN_DESC(10, 12), RZ_PIN_DESC(10, 13), RZ_PIN_DESC(10, 14),
> +	RZ_PIN_DESC(10, 15),
> +
> +	/* Port 11 */
> +	RZ_PIN_DESC(11, 0), RZ_PIN_DESC(11, 1), RZ_PIN_DESC(11, 2),
> +	RZ_PIN_DESC(11, 3), RZ_PIN_DESC(11, 4), RZ_PIN_DESC(11, 5),
> +	RZ_PIN_DESC(11, 6), RZ_PIN_DESC(11, 7), RZ_PIN_DESC(11, 8),
> +	RZ_PIN_DESC(11, 9), RZ_PIN_DESC(11, 10), RZ_PIN_DESC(11, 11),
> +	RZ_PIN_DESC(11, 12), RZ_PIN_DESC(11, 13), RZ_PIN_DESC(11, 14),
> +	RZ_PIN_DESC(11, 15),
> +};
> +
> +/* ------------------------------------------------------------------------
> + * SoC operations
> + */
> +
> +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
> +				int bank, int pin, int set)
> +{
> +	void __iomem *mem = RZA1_ADDR(res->base, reg, bank);
> +	u16 val = set ? ioread16(mem) | 1 << pin :
> +			ioread16(mem) & ~(1 << pin);

Is the compiler smart enough to avoid two calls to ioread16() ? If not, how 
about

	u16 val = ioread16(mem);

	if (set)
		val |= 1 << pin;
	else
		val &= ~(1 << pin);

> +#ifdef RZA1_REG_DBG
> +	u16 temp = ioread16(mem);
> +
> +	pr_err("%p %p %p - %4x %4x\n",
> +	       (void *)res->start, res->base, mem, temp, val);
> +#endif

Do you really need this in the driver, or was it useful during initial 
development only ?

> +	iowrite16(val, mem);

Is there a lock in the call stack that prevents a race condition here ? If not 
I'd add a spinlock around the read and write.

> +}
> +
> +/**
> + * rza1_set_mux() - Configure alternate function settings for the selected
> pin
> + *
> + * @pinctrl: RZ pincontroller
> + * @pin: Pin to configure

The parameter is named pin_desc. 

> + * @mux_mode: Alternate function number
> + *
> + * @return: 0 for success; != 0 otherwise
> + */
> +static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl,
> +			struct rz_pin_desc *pin_desc, unsigned int mux_mode)
> +{
> +	struct rz_pinctrl_res *res;
> +	unsigned int bank = pin_desc->bank,
> +		     pin = pin_desc->pin;

One declaration per line please, especially when initializing the variables at 
declaration time.

	unsigned int bank = pin_desc->bank;
	unsigned int pin = pin_desc->pin;

> +
> +	/*
> +	 * disable input buffer and bi-control direction before entering
> +	 * alternate mode and let alternate function drive the IO mode by
> +	 * setting PIPCn to 1

Sentences start with a capital letter and end with a period. Or a smiley in e-
mails, but usually not in driver code ;-) This comment applies to the whole 
patch series.

> +	 */
> +	res = &pinctrl->res[MEM_RES_HIGH];
> +	rza1_set_bit(res, PIBC_REG, bank, pin, 0);
> +	rza1_set_bit(res, PBDC_REG, bank, pin, 0);
> +	rza1_set_bit(res, PIPC_REG, bank, pin, 1);
> +
> +	/* TODO:
> +	 * all alternate functions except a few (3) need PIPCn = 1;
> +	 * find a way to identify those 3 functions, do not set PIPCn to 1
> +	 * and set PMn according to some flag passed as parameter from DTS

Nitpicking, I'd say "from DT", DTS is the source format.

> +	 */
> +
> +	/*
> +	 * enable alternate function mode and select it.
> +	 *
> +	 * ----------------------------------------------------
> +	 * Alternate mode selection table:
> +	 *
> +	 * PMC	PFC	PFCE	PFCAE	AlternateMode mux_mode
> +	 * 1	0	0	0	1		0
> +	 * 1	1	0	0	2		1
> +	 * 1	0	1	0	3		2
> +	 * 1	1	1	0	4		3
> +	 * 1	0	0	1	5		4
> +	 * 1	1	0	1	6		5
> +	 * 1	0	1	1	7		6
> +	 * 1	1	1	1	8		7
> +	 * ----------------------------------------------------
> +	 */
> +	res = &pinctrl->res[MEM_RES_LOW];
> +	rza1_set_bit(res, PMC_REG, bank, pin, 1);
> +	rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1);
> +	rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2);
> +	rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4);
> +
> +	return 0;
> +}
> +
> +static struct rz_pinctrl_ops rza1_pinctrl_ops = {
> +	.set_mux = rza1_set_mux,
> +};
> +
> +static struct rz_pinctrl_info rza1_info = {
> +	.ops = &rza1_pinctrl_ops,
> +
> +	.npins = ARRAY_SIZE(pins),
> +	.pins = pins,
> +};

You should make those structures static const (which will require updating 
patch 1/5). It's especially important to make ops structures const for 
security reasons.

> +static const struct of_device_id rza1_pinctrl_of_match[] = {
> +	{ .compatible = "renesas,rza1-pinctrl", },
> +	{ }
> +};

Drivers usually have a corresponding MODULE_DEVICE_TABLE(). As this code can't 
be compiled as a module I suppose it doesn't matter much.

> +static int rza1_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return rz_pinctrl_probe(pdev, &rza1_info);
> +}
> +
> +static int rza1_pinctrl_remove(struct platform_device *pdev)
> +{
> +	rz_pinctrl_remove(pdev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rza1_pinctrl_driver = {
> +	.driver = {
> +		.name = "rza1_pinctrl_driver",

No need for "_driver" here, "rza1_pinctrl" will do.

I'm also never sure whether we should use _ or - in driver names.

> +		.of_match_table = rza1_pinctrl_of_match,
> +	},
> +	.probe = rza1_pinctrl_probe,
> +	.remove = rza1_pinctrl_remove,
> +};
> +
> +static int __init rza1_pinctrl_init(void)
> +{
> +	return platform_driver_register(&rza1_pinctrl_driver);
> +}
> +core_initcall(rza1_pinctrl_init);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> +MODULE_DESCRIPTION("Pinctl driver for Reneas RZ/A1 SoC");

s/Pinctl/Pinctrl/ (or better, "Pin controller" ?)

> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[jacopo mondi]

Hi Laurent,

On 01/02/2017 16:21, Laurent Pinchart wrote:
> Hi Jacopo,
>
[snip]
>
>> +}
>> +
>> +/**
>> + * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups
>> and
>> + *			 functions
>
> I don't think we have groups and functions, do we ?
>

Sort of.. The hardware does not have that, but we create them from pin 
configuration sub-nodes, as the pinmux core API is sort of 
group/function centric (see the pinmux_ops.set_mux function prototype)

[snip]

> +	mutex_lock(&rz_pinctrl->mutex);

This seems weird. The pinctrl generic functions indeed state that the 
caller
must take care of locking, but there doesn't seem to be any lock protecting
races between .dt_node_to_map() calling the generic functions below, and 
the
generic .get_group_*() handlers.

> +	ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins,
NULL);
> +	if (ret) {
> +		mutex_unlock(&rz_pinctrl->mutex);
> +		goto free_map;
> +	}
> +
> +	ret = pinmux_generic_add_function(pctldev, grpname, fngrps,
> +					  1, mux_modes);
> +	if (ret) {
> +		mutex_unlock(&rz_pinctrl->mutex);
> +		goto free_map;
> +	}
> +	mutex_unlock(&rz_pinctrl->mutex);

radix trees are RCU protected structures, so no locking should be 
required when accessing them from different places, so I guess lock here 
is just to protect the num_groups and num_functions variables in core 
pin controller driver...

[snip]

>> +/**
>> + * rz_dt_free_map()
>> + */
>> +static void rz_dt_free_map(struct pinctrl_dev *pctldev,
>> +			   struct pinctrl_map *map, unsigned int num_maps)
>> +{
>> +	struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
>> +
>> +	devm_kfree(rz_pinctrl->dev, map);
>
> Shouldn't you also free the other allocated pieces of memory (fngrps,
> mux_modes and grpins) ?
>

That's an interesting one. If I have to free all data associated with 
that map (and I assume so) I shall be able to retrieve groups and 
functions that map represents and delete them and

Currently there is no generic pinctrl and pinmux function to retrieve a 
group or function by name, but only by their id (selector).
It would take 10minutes to add them, but I wonder if that's desirable or 
there are other ways to do so I haven't found out yet.

Linus, Tony and gpio people: do you have opinions on this? I can add 
those functions to core/pinmux in my next patch series if I get your ack 
here.

Thanks
    j

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[Tony Lindgren]

* jacopo mondi <jacopo@jmondi.org> [170206 10:16]:
> Currently there is no generic pinctrl and pinmux function to retrieve a
> group or function by name, but only by their id (selector).
> It would take 10minutes to add them, but I wonder if that's desirable or
> there are other ways to do so I haven't found out yet.
> 
> Linus, Tony and gpio people: do you have opinions on this? I can add those
> functions to core/pinmux in my next patch series if I get your ack here.

I would just get rid of the names and allow optionally configuring them
from the consumer. Just like we do for interrupts and GPIOs.

Regards,

Tony

Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

[jacopo mondi]

Hi Tony,

On 06/02/2017 19:28, Tony Lindgren wrote:
> * jacopo mondi <jacopo@jmondi.org> [170206 10:16]:
>> Currently there is no generic pinctrl and pinmux function to retrieve a
>> group or function by name, but only by their id (selector).
>> It would take 10minutes to add them, but I wonder if that's desirable or
>> there are other ways to do so I haven't found out yet.
>>
>> Linus, Tony and gpio people: do you have opinions on this? I can add those
>> functions to core/pinmux in my next patch series if I get your ack here.
>
> I would just get rid of the names and allow optionally configuring them
> from the consumer. Just like we do for interrupts and GPIOs.
>

Mostly, I wonder why so far nobody had to remove function and groups and 
release their associated data when deleting a map. It makes me think 
this is not required or I didn't fully get dt_free_map() purpose :/
Or, put it in another way, I see lot of code releasing memory associated 
with PIN_MAP_TYPE_CONFIGS_GROUP map types (as map.data.configs.configs 
it's easy accessible), but none releasing data associated to 
PIN_MAP_TYPE_MUX_GROUP map types...

Thanks
    j


> Regards,
>
> Tony
>