[PATCH v2 00/77] SH pin control and GPIO rework with OF support

[PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Laurent Pinchart]

Hi everybody,

Here's the second version of the SH pin control and GPIO rework patches. I've
added OF support for PFC instantiation and GPIO mappings that was missing from
v1. PINCTRL bindings are still missing and will come soon.

The first version of this patch set is available at
https://lkml.org/lkml/2012/11/20/823. I haven't applied the ack's I've received
on patches that have been extensively reworked in v2. Sorry for the
inconvenience. All comments received on v1 should have been addressed.

The GPIO DT bindings have been tested on an SH73A0 platform (KZM-A9-GT) with
the on-board GPIO LEDs. Please see patch 71/77 for a question regarding those
bindings.

The patches are still based on top of v3.7-rc6. You can get them from my git
tree at
 
	git://linuxtv.org/pinchartl/fbdev.git pinmux

The code has been compile-tested on all modified platforms except SH7264 and
SH7269, and runtime tested on SH7372 (Mackerel), SH73A0 (KZM-A9-GT) and
R8A7740 (Armadillo) so far. I will runtime test it on R8A7779 (Marzen).

The SH7264 and SH7269 platforms have no gpiolib support so the PFC code can't
be compiled for them. As the currently implemented arch-level pinmux support
also depends on generic GPIO, we're moving from a situation where the code
isn't used to a different situation where the code isn't used. I don't
consider that as a regression.

Sorry for the numerous checkpatch warnings, patches that move code around or
rename files don't modify the content to make review easier, and thus carry
warnings from the existing code.

Changes since v1:

- Split SoC changes in one patch per SoC to make backporting easier
- Dropped SH7267 and SH7277 changes as support for those SoCs will be removed
  from the kernel
- Removed the sh_pfc_register() function on ARM
- Added OF support for PFC instantiation and GPIO bindings
- Added PFC DT nodes for the Mackerel and KZM9G boards
- Added GPIO LEDs DT nodes for the KZM9G board

Laurent Pinchart (76):
  sh-pfc: Split platform data from the sh_pfc structure
  sh-pfc: Move private definitions and declarations to private header
  sh-pfc: Merge PFC core and pinctrl
  sh-pfc: Merge PFC core and gpio
  sh-pfc: Move platform device and driver to the core
  sh-pfc: Let the compiler decide whether to inline functions
  sh-pfc: Remove check for impossible error condition
  sh-pfc: Sort headers alphabetically
  sh-pfc: Split platform device and platform driver registration
  sh-pfc: Support passing resources through platform device
  ARM: shmobile: Select PINCTRL
  ARM: shmobile: r8a7740: Register PFC platform device
  ARM: shmobile: r8a7779: Register PFC platform device
  ARM: shmobile: sh7372: Register PFC platform device
  ARM: shmobile: sh73a0: Register PFC platform device
  ARM: shmobile: r8a7740: Add pin control resources
  ARM: shmobile: sh7372: Add pin control resources
  ARM: shmobile: sh73a0: Add pin control resources
  sh: Add PFC platform device registration helper function
  sh: sh7203: Register PFC platform device
  sh: sh7264: Register PFC platform device
  sh: sh7269: Register PFC platform device
  sh: sh7720: Register PFC platform device
  sh: sh7722: Register PFC platform device
  sh: sh7723: Register PFC platform device
  sh: sh7724: Register PFC platform device
  sh: sh7734: Register PFC platform device
  sh: sh7757: Register PFC platform device
  sh: sh7785: Register PFC platform device
  sh: sh7786: Register PFC platform device
  sh: shx3: Register PFC platform device
  sh-pfc: Remove platform device registration
  sh-pfc: Remove unused resource and num_resources platform data fields
  sh-pfc: Move driver from drivers/sh/ to drivers/pinctrl/
  sh-pfc: Support pinmux info in driver data instead of platform data
  sh-pfc: Add r8a7740 pinmux support
  sh-pfc: Add r8a7779 pinmux support
  sh-pfc: Add sh7372 pinmux support
  sh-pfc: Add sh73a0 pinmux support
  ARM: shmobile: r8a7740: Use driver-provided pinmux info
  ARM: shmobile: r8a7779: Use driver-provided pinmux info
  ARM: shmobile: sh7372: Use driver-provided pinmux info
  ARM: shmobile: sh73a0: Use driver-provided pinmux info
  sh-pfc: Add sh7203 pinmux support
  sh-pfc: Add sh7264 pinmux support
  sh-pfc: Add sh7269 pinmux support
  sh-pfc: Add sh7720 pinmux support
  sh-pfc: Add sh7722 pinmux support
  sh-pfc: Add sh7723 pinmux support
  sh-pfc: Add sh7724 pinmux support
  sh-pfc: Add sh7734 pinmux support
  sh-pfc: Add sh7757 pinmux support
  sh-pfc: Add sh7785 pinmux support
  sh-pfc: Add sh7786 pinmux support
  sh-pfc: Add shx3 pinmux support
  sh: sh7203: pinmux: Use driver-provided pinmux info
  sh: sh7264: pinmux: Use driver-provided pinmux info
  sh: sh7269: pinmux: Use driver-provided pinmux info
  sh: sh7720: pinmux: Use driver-provided pinmux info
  sh: sh7722: pinmux: Use driver-provided pinmux info
  sh: sh7723: pinmux: Use driver-provided pinmux info
  sh: sh7724: pinmux: Use driver-provided pinmux info
  sh: sh7734: pinmux: Use driver-provided pinmux info
  sh: sh7757: pinmux: Use driver-provided pinmux info
  sh: sh7785: pinmux: Use driver-provided pinmux info
  sh: sh7786: pinmux: Use driver-provided pinmux info
  sh: shx3: pinmux: Use driver-provided pinmux info
  sh: Remove unused sh_pfc_register_info() function
  sh-pfc: Remove pinmux_info definition
  sh-pfc: Move sh_pfc.h from include/linux/ to driver directory
  sh-pfc: Add OF support
  ARM: shmobile: r8a7740: Add pin control device in device tree
  ARM: shmobile: armadillo: Populate platform devices from device tree
  ARM: shmobile: kzm9g: Add pin control device in device tree
  ARM: shmobile: kzm9g: Populate platform devices from device tree
  ARM: shmobile: kzm9g: Add LED1-LED4 to the device tree

Nobuhiro Iwamatsu (1):
  ARM: shmobile: Include DTSI of r8a7740 to armadillo800eva

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |   43 +
 arch/arm/Kconfig                                   |    1 +
 arch/arm/boot/dts/r8a7740-armadillo800eva.dts      |    2 +-
 arch/arm/boot/dts/r8a7740.dtsi                     |    6 +
 arch/arm/boot/dts/sh73a0-kzm9g.dts                 |   18 +-
 arch/arm/boot/dts/sh73a0.dtsi                      |   32 +
 arch/arm/mach-shmobile/Makefile                    |    5 -
 arch/arm/mach-shmobile/board-armadillo800eva.c     |    4 +-
 arch/arm/mach-shmobile/board-kzm9g.c               |    3 +-
 arch/arm/mach-shmobile/setup-r8a7740.c             |   26 +
 arch/arm/mach-shmobile/setup-r8a7779.c             |   25 +
 arch/arm/mach-shmobile/setup-sh7372.c              |   26 +
 arch/arm/mach-shmobile/setup-sh73a0.c              |   25 +
 arch/sh/Kconfig                                    |   12 +
 arch/sh/include/asm/gpio.h                         |    2 +-
 arch/sh/include/cpu-common/cpu/pfc.h               |   26 +
 arch/sh/kernel/cpu/Makefile                        |    2 +-
 arch/sh/kernel/cpu/pfc.c                           |   33 +
 arch/sh/kernel/cpu/sh2a/pinmux-sh7203.c            | 1582 +-----------
 arch/sh/kernel/cpu/sh2a/pinmux-sh7264.c            | 2121 +---------------
 arch/sh/kernel/cpu/sh2a/pinmux-sh7269.c            | 2823 +-------------------
 arch/sh/kernel/cpu/sh3/pinmux-sh7720.c             | 1226 +---------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7722.c            | 1778 +------------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7723.c            | 1893 +-------------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7724.c            | 2210 +---------------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7734.c            | 2470 +-----------------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7757.c            | 2267 +----------------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7785.c            | 1294 +---------
 arch/sh/kernel/cpu/sh4a/pinmux-sh7786.c            |  822 +------
 arch/sh/kernel/cpu/sh4a/pinmux-shx3.c              |  573 +----
 drivers/pinctrl/Kconfig                            |    1 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/sh-pfc/Kconfig                     |  116 +
 drivers/pinctrl/sh-pfc/Makefile                    |   21 +
 drivers/{sh/pfc => pinctrl/sh-pfc}/core.c          |  294 ++-
 drivers/pinctrl/sh-pfc/core.h                      |   72 +
 drivers/{sh/pfc => pinctrl/sh-pfc}/gpio.c          |  111 +-
 .../pinctrl/sh-pfc}/pfc-r8a7740.c                  |   11 +-
 .../pinctrl/sh-pfc}/pfc-r8a7779.c                  |   29 +-
 drivers/pinctrl/sh-pfc/pfc-sh7203.c                | 1592 +++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7264.c                | 2131 +++++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7269.c                | 2834 ++++++++++++++++++++
 .../pinctrl/sh-pfc}/pfc-sh7372.c                   |   11 +-
 .../pinctrl/sh-pfc}/pfc-sh73a0.c                   |   11 +-
 drivers/pinctrl/sh-pfc/pfc-sh7720.c                | 1236 +++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7722.c                | 1779 ++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7723.c                | 1903 +++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7724.c                | 2225 +++++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7734.c                | 2475 +++++++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7757.c                | 2282 ++++++++++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7785.c                | 1304 +++++++++
 drivers/pinctrl/sh-pfc/pfc-sh7786.c                |  837 ++++++
 drivers/pinctrl/sh-pfc/pfc-shx3.c                  |  582 ++++
 drivers/{sh/pfc => pinctrl/sh-pfc}/pinctrl.c       |  149 +-
 {include/linux => drivers/pinctrl/sh-pfc}/sh_pfc.h |   43 +-
 drivers/sh/Kconfig                                 |    1 -
 drivers/sh/Makefile                                |    1 -
 drivers/sh/pfc/Kconfig                             |   26 -
 drivers/sh/pfc/Makefile                            |    3 -
 59 files changed, 22007 insertions(+), 21424 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
 create mode 100644 arch/arm/boot/dts/sh73a0.dtsi
 create mode 100644 arch/sh/include/cpu-common/cpu/pfc.h
 create mode 100644 arch/sh/kernel/cpu/pfc.c
 create mode 100644 drivers/pinctrl/sh-pfc/Kconfig
 create mode 100644 drivers/pinctrl/sh-pfc/Makefile
 rename drivers/{sh/pfc => pinctrl/sh-pfc}/core.c (62%)
 create mode 100644 drivers/pinctrl/sh-pfc/core.h
 rename drivers/{sh/pfc => pinctrl/sh-pfc}/gpio.c (59%)
 rename {arch/arm/mach-shmobile => drivers/pinctrl/sh-pfc}/pfc-r8a7740.c (99%)
 rename {arch/arm/mach-shmobile => drivers/pinctrl/sh-pfc}/pfc-r8a7779.c (99%)
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7203.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7264.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7269.c
 rename {arch/arm/mach-shmobile => drivers/pinctrl/sh-pfc}/pfc-sh7372.c (99%)
 rename {arch/arm/mach-shmobile => drivers/pinctrl/sh-pfc}/pfc-sh73a0.c (99%)
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7720.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7722.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7723.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7724.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7734.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7757.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7785.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-sh7786.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-shx3.c
 rename drivers/{sh/pfc => pinctrl/sh-pfc}/pinctrl.c (76%)
 rename {include/linux => drivers/pinctrl/sh-pfc}/sh_pfc.h (83%)
 delete mode 100644 drivers/sh/pfc/Kconfig
 delete mode 100644 drivers/sh/pfc/Makefile

-- 
Regards,

Laurent Pinchart

[PATCH v2 71/77] sh-pfc: Add OF support

[Laurent Pinchart]

Support device instantiation through the device tree. The compatible
property is used to select the SoC pinmux information.

Set the gpio_chip device field to the PFC device to enable automatic
GPIO OF support.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: devicetree-discuss@lists.ozlabs.org
---
 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |   43 ++++++++++++++
 drivers/pinctrl/sh-pfc/core.c                      |   62 +++++++++++++++++++-
 drivers/pinctrl/sh-pfc/gpio.c                      |    1 +
 3 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt

Only GPIO DT bindings are currently implemented, pinmux bindings are still
work in progress and will be posted soon.

On the GPIO side, some DT bindings require aliases for the GPIO (and pinctrl)
nodes. What's the reason behind that ?

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
new file mode 100644
index 0000000..f08d8c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
@@ -0,0 +1,43 @@
+* Renesas GPIO and Pin Mux/Config controller
+
+Required Properties:
+- compatible: should be one of the following.
+  - "renesas,pfc-r8a7740": for R8A7740 (R-Mobile A1) compatible pin-controller.
+  - "renesas,pfc-r8a7779": for R8A7779 (R-Car H1) compatible pin-controller.
+  - "renesas,pfc-sh7372": for SH7372 (SH-Mobile AP4) compatible pin-controller.
+  - "renesas,pfc-sh73a0": for SH73A0 (SH-Mobile AG5) compatible pin-controller.
+
+- reg: Base address and length of each memory resource used by the pin
+  controller hardware module.
+
+- gpio-controller: Marks the device node as a gpio controller.
+
+- #gpio-cells: Should be 2. The first cell is the pin number and the second cell
+  is used to specify optional parameters (currently unused).
+
+  The syntax of the gpio specifier used by client nodes should be the following
+  with values derived from the SoC user manual.
+
+  <[phandle of the gpio controller node]
+   [pin number within the gpio controller]
+   [flags and pull up/down]>
+
+
+Example 1: SH73A0 (SH-Mobile AG5) pin controller node
+
+	gpio: pfc@e6050000 {
+		compatible = "renesas,pfc-sh73a0";
+		reg = <0xe6050000 0x8000>,
+		      <0xe605801c 0x1c>;
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
+
+Example 2: A GPIO LED node that references a GPIO
+
+	leds {
+		compatible = "gpio-leds";
+		led1 {
+			gpios = <&gpio 20 1>; /* Active low */
+		};
+	};
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 062308f..9cbe684 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -19,6 +19,7 @@
 #include <linux/ioport.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/platform_device.h>
@@ -498,8 +499,55 @@ int sh_pfc_config_gpio(struct sh_pfc *pfc, unsigned gpio, int pinmux_type,
 	return -1;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id sh_pfc_of_table[] = {
+#ifdef CONFIG_PINCTRL_PFC_R8A7740
+	{
+		.compatible = "renesas,pfc-r8a7740",
+		.data = &r8a7740_pinmux_info,
+	},
+#endif
+#ifdef CONFIG_PINCTRL_PFC_R8A7779
+	{
+		.compatible = "renesas,pfc-r8a7779",
+		.data = &r8a7779_pinmux_info,
+	},
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7367
+	{
+		.compatible = "renesas,pfc-sh7367",
+		.data = &sh7367_pinmux_info,
+	},
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7372
+	{
+		.compatible = "renesas,pfc-sh7372",
+		.data = &sh7372_pinmux_info,
+	},
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7377
+	{
+		.compatible = "renesas,pfc-sh7377",
+		.data = &sh7377_pinmux_info,
+	},
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH73A0
+	{
+		.compatible = "renesas,pfc-sh73a0",
+		.data = &sh73a0_pinmux_info,
+	},
+#endif
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sh_pfc_of_table);
+#endif
+
 static int sh_pfc_probe(struct platform_device *pdev)
 {
+	const struct platform_device_id *platid = platform_get_device_id(pdev);
+#ifdef CONFIG_OF
+	struct device_node *np = pdev->dev.of_node;
+#endif
 	struct sh_pfc_soc_info *info;
 	struct sh_pfc *pfc;
 	int ret;
@@ -509,8 +557,15 @@ static int sh_pfc_probe(struct platform_device *pdev)
 	 */
 	BUILD_BUG_ON(PINMUX_FLAG_TYPE > ((1 << PINMUX_FLAG_DBIT_SHIFT) - 1));
 
-	info = pdev->id_entry->driver_data
-	      ? (void *)pdev->id_entry->driver_data : pdev->dev.platform_data;
+	if (platid)
+		info = (void *)platid->driver_data;
+#ifdef CONFIG_OF
+	else if (np)
+		info = (void *)of_match_device(sh_pfc_of_table, &pdev->dev)->data;
+#endif
+	else
+		info = pdev->dev.platform_data;
+
 	if (info = NULL)
 		return -ENODEV;
 
@@ -640,6 +695,9 @@ static struct platform_driver sh_pfc_driver = {
 	.driver		= {
 		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
+#ifdef CONFIG_OF
+		.of_match_table = sh_pfc_of_table,
+#endif
 	},
 };
 
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 22df3fe..1431c5e 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -132,6 +132,7 @@ static void sh_pfc_gpio_setup(struct sh_pfc_chip *chip)
 	WARN_ON(pfc->info->first_gpio != 0); /* needs testing */
 
 	gc->label = pfc->info->name;
+	gc->dev = pfc->dev;
 	gc->owner = THIS_MODULE;
 	gc->base = pfc->info->first_gpio;
 	gc->ngpio = (pfc->info->last_gpio - pfc->info->first_gpio) + 1;
-- 
1.7.8.6

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Linus Walleij]

On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:

> Here's the second version of the SH pin control and GPIO rework patches. I've
> added OF support for PFC instantiation and GPIO mappings that was missing from
> v1. PINCTRL bindings are still missing and will come soon.

So I've tried the only way I could to review this by cloning your tree
and actually inspecting the end result ... overall it's looking very good!

Here are assorted comments:

- Some use of __devicexit/__devinit, this will be deleted in the v3.8
  merge window so just remove this everywhere.

- You're using the method to add ranges from the pinctrl side of
  things. This is basically deprecated with the changes to gpiolib
  I make in this merge window. If you study the way I changed
  the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
  from being done in the pin controller to being done in the
  gpiolib part, you will get the picture. The big upside is that
  (A) makes the pin and GPIO references to the local GPIO
  chip and pin controller and (B) that this supports adding ranges
  from the device tree, which is probably what you want in the
  end...

- The above probably means you can get rid of
  sh_pfc_register_gpiochip() and decouple the pinctrl
  and gpiolib code like I did in my patch set
  (commit 8604ac34e in the pinctrl tree) and just
  initialize the GPIO with some module_init().
  But I might be wrong!

- sh_pfc_register_pinctrl() in pinctrl.c is using kzalloc/kfree,
  but since it has a struct sh_pfc, which contains a
  struct device *, I suspect you can use devm_kzalloc()
  and cut the kfree():s. This probably applies in more
  places in the driver.

- core.c contains pfc_ioremap() and pfc_iounmap()
  which actually seem to exist much due to the fact
  that devm_kzalloc() and devm_ioremap_nocache() did not
  exist at the time. By using devm_* helpers and
  maybe also inlining the code I think it'll look way
  smoother.

- sh_pfc.h contains this:
  typedef unsigned short pinmux_enum_t;
  typedef unsigned short pinmux_flag_t;
  But custom typedefs in the kernel is discouraged unless
  there is a good reason for. What about you search/replace
  this thing with u16 everywhere. I really like u16...
  I can see macros building up some gigantic bit pattern in
  these but in the end it's still just 16 unsigned bits.
  The name "pinmux_enum_t" is very dangerously
  close to the builting "enum" so it scares me a bit
  for that reason too.

- sf_pfc.h contains a number of structs which I just
  have no chance of understanding unless they are
  supplied with something real nice like kerneldoc,
  struct pinmux_gpio, pinmux_cfg_reg, pinmux_data_reg,
  pinmux_irq, pinmux_range and sh_pfc_soc_info all
  need some doc I think, I just don't understand these
  structs.

- Same goes for the helper macros.

- In core.h document struct pfc_window, i.e. what are
  these windows? What do we mean by a window?
  How many of them exist in a certain configuration
  typically? etc, stuff you want to know when reading
  the code.

- struct sh_pfc is however quite self-explanatory.

- The pfc-* drivers include things like:
  #include <cpu/sh7785.h>
  #include <mach/r8a7740.h>
  #include <mach/irqs.h>
  #include <mach/r8a7779.h>
  #include <mach/sh73a0.h>
  #include <mach/irqs.h>
  Why? Especially <mach/irqs.h> would be nice to
  get rid of, as it sort of defeats the idea of passing
  IRQs as resources or allocating them dynamically.
  In some cases it seems these includes are just
  surplus, so please look over this...

- This stuff in setup_data_regs():
  rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg,
                                                    drp->reg_width);
  You know, I think shadow registers is just another name
  for regmap-mmio. Please consult
  drivers/base/regmap/regmap-mmio.c and tell me if I'm
  wrong. It's not like I'm going to require you to convert this
  to regmap from day 1 if this is legacy stuff but it's probably
  the same thing.

- In core.c, gpio_read_raw_reg() and gpio_write_raw_reg()
  are looking like marshalling functions to me. This is also
  what regmap is about. Marshalling and caching/shadow.
  If you keep the functions as they are, atleast rename them
  to sh_pfc_* because the gpio_* namespace is for gpiolib.
  (There are a few other functions that need to be prefixed
  in that file by the way.)

- While keeping Magnus' and Paul's names as authors in the
  files it would be proper if you atleast add your own name since
  you've probably written most of the code in these files now.
  Also for MODULE_AUTHOR().

Let's have this patchset finished for v3.9 say? :-)

Yours,
Linus Walleij

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Laurent Pinchart]

Hi Linus,

Thank you for the review.

On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > Here's the second version of the SH pin control and GPIO rework patches.
> > I've added OF support for PFC instantiation and GPIO mappings that was
> > missing from v1. PINCTRL bindings are still missing and will come soon.
> 
> So I've tried the only way I could to review this by cloning your tree
> and actually inspecting the end result ... overall it's looking very good!
> 
> Here are assorted comments:
> 
> - Some use of __devicexit/__devinit, this will be deleted in the v3.8
>   merge window so just remove this everywhere.

Will fix.

> - You're using the method to add ranges from the pinctrl side of
>   things. This is basically deprecated with the changes to gpiolib
>   I make in this merge window. If you study the way I changed
>   the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
>   from being done in the pin controller to being done in the
>   gpiolib part, you will get the picture. The big upside is that
>   (A) makes the pin and GPIO references to the local GPIO
>   chip and pin controller and (B) that this supports adding ranges
>   from the device tree, which is probably what you want in the
>   end...

OK, I will have a look at the code.

> - The above probably means you can get rid of
>   sh_pfc_register_gpiochip() and decouple the pinctrl
>   and gpiolib code like I did in my patch set
>   (commit 8604ac34e in the pinctrl tree) and just
>   initialize the GPIO with some module_init().
>   But I might be wrong!

GPIO and pinmuxing are controlled by the same hardware block, with shared 
registers (there's one register by pin that controls the direction, pull-
ups/pull-downs and function selection). That's why I've implemented them in a 
single driver.

> - sh_pfc_register_pinctrl() in pinctrl.c is using kzalloc/kfree,
>   but since it has a struct sh_pfc, which contains a
>   struct device *, I suspect you can use devm_kzalloc()
>   and cut the kfree():s. This probably applies in more
>   places in the driver.

Agreed, I'll fix that.

> - core.c contains pfc_ioremap() and pfc_iounmap()
>   which actually seem to exist much due to the fact
>   that devm_kzalloc() and devm_ioremap_nocache() did not
>   exist at the time. By using devm_* helpers and
>   maybe also inlining the code I think it'll look way
>   smoother.

Will fix as well.

> - sh_pfc.h contains this:
>   typedef unsigned short pinmux_enum_t;
>   typedef unsigned short pinmux_flag_t;
>   But custom typedefs in the kernel is discouraged unless
>   there is a good reason for. What about you search/replace
>   this thing with u16 everywhere. I really like u16...
>   I can see macros building up some gigantic bit pattern in
>   these but in the end it's still just 16 unsigned bits.
>   The name "pinmux_enum_t" is very dangerously
>   close to the builting "enum" so it scares me a bit
>   for that reason too.

It's totally scary, and I want it to go away. It's old code that I still need 
to fix, you can expect a v3 with more than 77 patches :-)

> - sf_pfc.h contains a number of structs which I just
>   have no chance of understanding unless they are
>   supplied with something real nice like kerneldoc,
>   struct pinmux_gpio, pinmux_cfg_reg, pinmux_data_reg,
>   pinmux_irq, pinmux_range and sh_pfc_soc_info all
>   need some doc I think, I just don't understand these
>   structs.
>
> - Same goes for the helper macros.

Those structures and macros come from the existing driver (drivers/sh/pfc). I 
want to replace them with a cleaner pinctrl-aware implementation, that's still 
work in progress that should be in v3.

> - In core.h document struct pfc_window, i.e. what are
>   these windows? What do we mean by a window?
>   How many of them exist in a certain configuration
>   typically? etc, stuff you want to know when reading
>   the code.

It's basically an ioremapped region. The driver gets register physical 
addresses in the SoC info structures, and walks through the ioremapped regions 
when it needs to access the registers to find the ioremapped address.

> - struct sh_pfc is however quite self-explanatory.
> 
> - The pfc-* drivers include things like:
>   #include <cpu/sh7785.h>
>   #include <mach/r8a7740.h>
>   #include <mach/irqs.h>
>   #include <mach/r8a7779.h>
>   #include <mach/sh73a0.h>
>   #include <mach/irqs.h>
>   Why?

Because the code isn't new but comes from existing mach-level code.

>   Especially <mach/irqs.h> would be nice to
>   get rid of, as it sort of defeats the idea of passing
>   IRQs as resources or allocating them dynamically.
>   In some cases it seems these includes are just
>   surplus, so please look over this...

I'll have a look at it (maybe for v4 though ;-))

> - This stuff in setup_data_regs():
>   rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg,
>                                                     drp->reg_width);
>   You know, I think shadow registers is just another name
>   for regmap-mmio. Please consult
>   drivers/base/regmap/regmap-mmio.c and tell me if I'm
>   wrong. It's not like I'm going to require you to convert this
>   to regmap from day 1 if this is legacy stuff but it's probably
>   the same thing.

I'll have a look at it.

> - In core.c, gpio_read_raw_reg() and gpio_write_raw_reg()
>   are looking like marshalling functions to me. This is also
>   what regmap is about. Marshalling and caching/shadow.
>   If you keep the functions as they are, atleast rename them
>   to sh_pfc_* because the gpio_* namespace is for gpiolib.
>   (There are a few other functions that need to be prefixed
>   in that file by the way.)

OK.

> - While keeping Magnus' and Paul's names as authors in the
>   files it would be proper if you atleast add your own name since
>   you've probably written most of the code in these files now.
>   Also for MODULE_AUTHOR().

OK :-)

> Let's have this patchset finished for v3.9 say? :-)

I'll try to.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Linus Walleij]

On Thu, Dec 6, 2012 at 2:34 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

>> Let's have this patchset finished for v3.9 say? :-)
>
> I'll try to.

Any working condition along the way will basically be fine to merge
from my point of view. Doing so leaves the kernel in a way better
shape than before, and I fully trust you to go in and finalize it, so
I think we can move ahead quite swiftly.

Yours,
Linus Walleij

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Laurent Pinchart]

Hi Linus,

On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > Here's the second version of the SH pin control and GPIO rework patches.
> > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > missing from v1. PINCTRL bindings are still missing and will come soon.
> > 
> > So I've tried the only way I could to review this by cloning your tree
> > and actually inspecting the end result ... overall it's looking very good!
> > 
> > Here are assorted comments:

[snip]

> > - You're using the method to add ranges from the pinctrl side of
> >   things. This is basically deprecated with the changes to gpiolib
> >   I make in this merge window. If you study the way I changed
> >   the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> >   from being done in the pin controller to being done in the
> >   gpiolib part, you will get the picture. The big upside is that
> >   (A) makes the pin and GPIO references to the local GPIO
> >   chip and pin controller and (B) that this supports adding ranges
> >   from the device tree, which is probably what you want in the
> >   end...
> 
> OK, I will have a look at the code.

Do you have a tree with those patches ?

[snip]

> > - This stuff in setup_data_regs():
> >   rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> >   
> >   You know, I think shadow registers is just another name
> >   for regmap-mmio. Please consult
> >   drivers/base/regmap/regmap-mmio.c and tell me if I'm
> >   wrong. It's not like I'm going to require you to convert this
> >   to regmap from day 1 if this is legacy stuff but it's probably
> >   the same thing.
> 
> I'll have a look at it.

I've considered regmap but I think it's a bit overkill. True, the reg_shadow 
is a different name for regmap-mmio (or rather for a small subset of it), but 
I already have a data structure instance for each register due to other 
requirements of the driver, so storing the cached value there is pretty much 
free.

I might end up reworking the data registers related code in which case I will 
try to use regmap.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Laurent Pinchart]

Hi Linus,

On Friday 07 December 2012 19:35:33 Laurent Pinchart wrote:
> On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> > On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > > Here's the second version of the SH pin control and GPIO rework
> > > > patches.
> > > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > > missing from v1. PINCTRL bindings are still missing and will come
> > > > soon.
> > > 
> > > So I've tried the only way I could to review this by cloning your tree
> > > and actually inspecting the end result ... overall it's looking very
> > > good!
> 
> > > Here are assorted comments:
> [snip]
> 
> > > - You're using the method to add ranges from the pinctrl side of
> > >   things. This is basically deprecated with the changes to gpiolib
> > >   I make in this merge window. If you study the way I changed
> > >   the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> > >   from being done in the pin controller to being done in the
> > >   gpiolib part, you will get the picture. The big upside is that
> > >   (A) makes the pin and GPIO references to the local GPIO
> > >   chip and pin controller and (B) that this supports adding ranges
> > >   from the device tree, which is probably what you want in the
> > >   end...
> > 
> > OK, I will have a look at the code.
> 
> Do you have a tree with those patches ?

I should have looked myself for the tree before asking, sorry. I'll have a 
look at the changes you've added there and will rework the PFC driver 
accordingly.

I will send a v3 with fixes based on your comments. I might omit the DT 
patches this time and send a pull request, as the patch set is getting too big 
for my taste. Even though the result won't be perfect (yet :-)), it's still an 
improvement, and I'll send additional patches on top of that.

> > > - This stuff in setup_data_regs():
> > >   rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> > >   
> > >   You know, I think shadow registers is just another name for
> > >   regmap-mmio. Please consult drivers/base/regmap/regmap-mmio.c and
> > >   tell me if I'm wrong. It's not like I'm going to require you to
> > >   convert this to regmap from day 1 if this is legacy stuff but it's
> > >   probably the same thing.
> > 
> > I'll have a look at it.
> 
> I've considered regmap but I think it's a bit overkill. True, the reg_shadow
> is a different name for regmap-mmio (or rather for a small subset of it),
> but I already have a data structure instance for each register due to other
> requirements of the driver, so storing the cached value there is pretty
> much free.
> 
> I might end up reworking the data registers related code in which case I
> will try to use regmap.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Simon Horman]

On Wed, Dec 12, 2012 at 02:43:24AM +0100, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Friday 07 December 2012 19:35:33 Laurent Pinchart wrote:
> > On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> > > On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > > > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > > > Here's the second version of the SH pin control and GPIO rework
> > > > > patches.
> > > > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > > > missing from v1. PINCTRL bindings are still missing and will come
> > > > > soon.
> > > > 
> > > > So I've tried the only way I could to review this by cloning your tree
> > > > and actually inspecting the end result ... overall it's looking very
> > > > good!
> > 
> > > > Here are assorted comments:
> > [snip]
> > 
> > > > - You're using the method to add ranges from the pinctrl side of
> > > >   things. This is basically deprecated with the changes to gpiolib
> > > >   I make in this merge window. If you study the way I changed
> > > >   the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> > > >   from being done in the pin controller to being done in the
> > > >   gpiolib part, you will get the picture. The big upside is that
> > > >   (A) makes the pin and GPIO references to the local GPIO
> > > >   chip and pin controller and (B) that this supports adding ranges
> > > >   from the device tree, which is probably what you want in the
> > > >   end...
> > > 
> > > OK, I will have a look at the code.
> > 
> > Do you have a tree with those patches ?
> 
> I should have looked myself for the tree before asking, sorry. I'll have a 
> look at the changes you've added there and will rework the PFC driver 
> accordingly.
> 
> I will send a v3 with fixes based on your comments. I might omit the DT 
> patches this time and send a pull request, as the patch set is getting too big 
> for my taste. Even though the result won't be perfect (yet :-)), it's still an 
> improvement, and I'll send additional patches on top of that.

FWIW, I am quite comfortable with this approach.

> 
> > > > - This stuff in setup_data_regs():
> > > >   rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> > > >   
> > > >   You know, I think shadow registers is just another name for
> > > >   regmap-mmio. Please consult drivers/base/regmap/regmap-mmio.c and
> > > >   tell me if I'm wrong. It's not like I'm going to require you to
> > > >   convert this to regmap from day 1 if this is legacy stuff but it's
> > > >   probably the same thing.
> > > 
> > > I'll have a look at it.
> > 
> > I've considered regmap but I think it's a bit overkill. True, the reg_shadow
> > is a different name for regmap-mmio (or rather for a small subset of it),
> > but I already have a data structure instance for each register due to other
> > requirements of the driver, so storing the cached value there is pretty
> > much free.
> > 
> > I might end up reworking the data registers related code in which case I
> > will try to use regmap.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Linus Walleij]

On Wed, Dec 12, 2012 at 2:43 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I will send a v3 with fixes based on your comments. I might omit the DT
> patches this time and send a pull request, as the patch set is getting too big
> for my taste. Even though the result won't be perfect (yet :-)), it's still an
> improvement, and I'll send additional patches on top of that.

Sure the SH pinctrl business is already looking severa magnitudes better
after this so I will certainly pull it in.

I think I'll create a pinctrl-sh branch based off -rc1 after the merge window,
and we'll work on the sh stuff there for a while.

Yours,
Linus Walleij