diff mbox series

[v3,2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

Message ID 20180917163609.30457-3-phil.edworthy@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Renesas R9A06G032 PINCTRL Driver | expand

Commit Message

Phil Edworthy Sept. 17, 2018, 4:36 p.m. UTC
This provides a pinctrl driver for the Renesas RZ/N1 device family.

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v3:
 - Use standard DT props instead of proprietary ones.
 - Replace virtual pins used for MDIO muxing with extra funcs.
 - Use pinctrl_utils funcs to handle the maps.
 - Remove the dbg functions to keep things simple.

v2:
 - Change filename to generic rzn1, instead of device specific.
 - Changed Kconfig symbol and file name to generic rzn1 family.
 - Added "renesas,rzn1-pinctrl" compatible fallback string
 - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
   - Removed global ptr
   - Removed unused code accessing parent of node.
   - Removed check for null OF np that can't happen.
   - Replaced overlapping enums with #defines
 - Renamed some variables and symbols to clarify their use
 - Fix error handling during probe
 - Move probe from postcore_initcall to subsys_initcall to ensure
   drivers that require clocks get them instead of having to defer
   probing.
---
 drivers/pinctrl/Kconfig        |  10 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-rzn1.c | 926 +++++++++++++++++++++++++++++++++
 3 files changed, 937 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rzn1.c

Comments

Jacopo Mondi Sept. 18, 2018, 10:43 a.m. UTC | #1
Hi Phil,
   thanks for the patch

On Mon, Sep 17, 2018 at 05:36:08PM +0100, Phil Edworthy wrote:
> This provides a pinctrl driver for the Renesas RZ/N1 device family.
>
> Based on a patch originally written by Michel Pollet at Renesas.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v3:
>  - Use standard DT props instead of proprietary ones.
>  - Replace virtual pins used for MDIO muxing with extra funcs.
>  - Use pinctrl_utils funcs to handle the maps.
>  - Remove the dbg functions to keep things simple.
>
> v2:
>  - Change filename to generic rzn1, instead of device specific.
>  - Changed Kconfig symbol and file name to generic rzn1 family.
>  - Added "renesas,rzn1-pinctrl" compatible fallback string
>  - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
>    - Removed global ptr
>    - Removed unused code accessing parent of node.
>    - Removed check for null OF np that can't happen.
>    - Replaced overlapping enums with #defines
>  - Renamed some variables and symbols to clarify their use
>  - Fix error handling during probe
>  - Move probe from postcore_initcall to subsys_initcall to ensure
>    drivers that require clocks get them instead of having to defer
>    probing.
> ---
>  drivers/pinctrl/Kconfig        |  10 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-rzn1.c | 926 +++++++++++++++++++++++++++++++++
>  3 files changed, 937 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index e86752be1f19..e524eb101384 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,16 @@ config PINCTRL_RZA1
>  	help
>  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
>
> +config PINCTRL_RZN1
> +	bool "Renesas RZ/N1 pinctrl driver"
> +	depends on OF
> +	depends on ARCH_RZN1 || COMPILE_TEST
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCONF
> +	help
> +	  This selects pinctrl driver for Renesas RZ/N1 devices.
> +
>  config PINCTRL_SINGLE
>  	tristate "One-register-per-pin type device tree based pinctrl driver"
>  	depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 46ef9bd52096..d07f9a20f6ae 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
>  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
> +obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
>  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
>  obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
>  obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
> diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
> new file mode 100644
> index 000000000000..8f0caa266dbb
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rzn1.c
> @@ -0,0 +1,926 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
> + *
> + * Phil Edworthy <phil.edworthy@renesas.com>
> + * Based on a driver originally written by Michel Pollet at Renesas.
> + */
> +
> +#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "core.h"
> +#include "pinconf.h"
> +#include "pinctrl-utils.h"
> +
> +/* Field positions and masks in the pinmux registers */
> +#define RZN1_L1_PIN_DRIVE_STRENGTH	10
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
> +#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
> +#define RZN1_L1_PIN_PULL		8
> +#define RZN1_L1_PIN_PULL_NONE		0
> +#define RZN1_L1_PIN_PULL_UP		1
> +#define RZN1_L1_PIN_PULL_DOWN		3
> +#define RZN1_L1_FUNCTION		0
> +#define RZN1_L1_FUNC_MASK		0xf
> +#define RZN1_L1_FUNCTION_L2		0xf
> +
> +/*
> + * The hardware manual describes two levels of multiplexing, but it's more
> + * logical to think of the hardware as three levels, with level 3 consisting of
> + * the multiplexing for Ethernet MDIO signals.
> + *
> + * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf) specifying
> + * that level 2 functions are used instead. Level 2 has a lot more options,
> + * going from 0 to 61. Level 3 allows selection of MDIO functions which can be
> + * floating, or one of seven internal peripherals. Unfortunately, there are two
> + * level 2 functions that can select MDIO, and two MDIO channels so we have four
> + * sets of level 3 functions.
> + *
> + * For this driver, we've compounded the numbers together, so:
> + *    0 to   9 is level 1
> + *   10 to  71 is 10 + level 2 number
> + *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
> + *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
> + *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
> + *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
> + * Examples:
> + *  Function 28 corresponds UART0
> + *  Function 73 corresponds to MDIO0 to GMAC0
> + *
> + * There are 170 configurable pins (called PL_GPIO in the datasheet).
> + */
> +
> +/*
> + * Structure detailing the HW registers on the RZ/N1 devices.
> + * Both the Level 1 mux registers and Level 2 mux registers have the same
> + * structure. The only difference is that Level 2 has additional MDIO registers
> + * at the end.
> + */
> +struct rzn1_pinctrl_regs {
> +	union {
> +		u32	conf[170];
> +		u8	pad0[0x400];

Is pad0 actually used?

> +	};
> +	u32	status_protect;	/* 0x400 */
> +	/* MDIO mux registers, level2 only */
> +	u32	l2_mdio[2];
> +};
> +
> +/**
> + * struct rzn1_pmx_func - describes rzn1 pinmux functions
> + * @name: the name of this specific function
> + * @groups: corresponding pin groups
> + * @num_groups: the number of groups
> + */
> +struct rzn1_pmx_func {
> +	const char *name;
> +	const char **groups;
> +	unsigned int num_groups;
> +};
> +
> +/**
> + * struct rzn1_pin_group - describes an rzn1 pin group
> + * @name: the name of this specific pin group
> + * @func: the name of the function selected by this group
> + * @npins: the number of pins in this group array, i.e. the number of
> + *	elements in .pins so we can iterate over that array
> + * @pin_ids: array of pin_ids, i.e. the value used to select the mux
> + * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
> + */
> +struct rzn1_pin_group {
> +	const char *name;
> +	const char *func;
> +	unsigned int npins;
> +	u8 *pin_ids;
> +	u32 *pins;
> +};
> +
> +struct rzn1_pinctrl {
> +	struct device *dev;
> +	struct clk *clk;
> +	struct pinctrl_dev *pctl;
> +	struct rzn1_pinctrl_regs __iomem *lev1;
> +	struct rzn1_pinctrl_regs __iomem *lev2;
> +	u32 lev1_protect_phys;
> +	u32 lev2_protect_phys;
> +	int mdio_func[2];
> +
> +	struct rzn1_pin_group *groups;
> +	unsigned int ngroups;
> +
> +	struct rzn1_pmx_func *functions;
> +	unsigned int nfunctions;
> +};
> +
> +#define RZN1_PINS_PROP "pinmux"
> +
> +#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
> +
> +static const struct pinctrl_pin_desc rzn1_pins[] = {
> +	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3), RZN1_PIN(4),
> +	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8), RZN1_PIN(9),
> +	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13), RZN1_PIN(14),
> +	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18), RZN1_PIN(19),
> +	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23), RZN1_PIN(24),
> +	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28), RZN1_PIN(29),
> +	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33), RZN1_PIN(34),
> +	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38), RZN1_PIN(39),
> +	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43), RZN1_PIN(44),
> +	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48), RZN1_PIN(49),
> +	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53), RZN1_PIN(54),
> +	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58), RZN1_PIN(59),
> +	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63), RZN1_PIN(64),
> +	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68), RZN1_PIN(69),
> +	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73), RZN1_PIN(74),
> +	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78), RZN1_PIN(79),
> +	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83), RZN1_PIN(84),
> +	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88), RZN1_PIN(89),
> +	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93), RZN1_PIN(94),
> +	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98), RZN1_PIN(99),
> +	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
> +	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
> +	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
> +	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
> +	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
> +	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
> +	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
> +	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
> +	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
> +	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
> +	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
> +	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
> +	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
> +	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
> +	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
> +	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
> +	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
> +	RZN1_PIN(168), RZN1_PIN(169),
> +};
> +
> +enum {
> +	LOCK_LEVEL1 = 0x1,
> +	LOCK_LEVEL2 = 0x2,
> +	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,

0x03 is already (0x01 | 0x02) :)
Not a big deal though.

> +};
> +
> +static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
> +{
> +	/*
> +	 * The pinmux configuration is locked by writing the physical address of
> +	 * the status_protect register to itself. It is unlocked by writing the
> +	 * address | 1.
> +	 */
> +	if (lock & LOCK_LEVEL1) {
> +		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
> +
> +		writel(val, &ipctl->lev1->status_protect);
> +	}
> +
> +	if (lock & LOCK_LEVEL2) {
> +		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
> +
> +		writel(val, &ipctl->lev2->status_protect);
> +	}
> +}
> +
> +static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
> +				     u32 func)
> +{
> +	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> +		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
> +	ipctl->mdio_func[mdio] = func;
> +
> +	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
> +
> +	writel(func, &ipctl->lev2->l2_mdio[mdio]);
> +}
> +
> +/*
> + * Using a composite pin description, set the hardware pinmux registers
> + * with the corresponding values.
> + * Make sure to unlock write protection and reset it afterward.
> + *
> + * NOTE: There is no protection for potential concurrency, it is assumed these
> + * calls are serialized already.
> + */
> +static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
> +				u32 pin_config, u8 use_locks)
> +{
> +	u32 l1_cache;
> +	u32 l2_cache;
> +	u32 l1;
> +	u32 l2;
> +
> +	/* Level 3 MDIO multiplexing */
> +	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
> +	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
> +		u8 mdio_channel;
> +		u32 mdio_func;
> +
> +		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
> +			mdio_channel = 0;
> +		else
> +			mdio_channel = 1;
> +
> +		/* Get MDIO func, and convert the func to the level 2 number */
> +		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO0_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO;
> +		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO0_E1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> +		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO;
> +		} else {
> +			mdio_func = pin_config - RZN1_FUNC_MDIO1_E1_HIGHZ;
> +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> +		}
> +		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
> +	}
> +
> +	/* Note here, we do not allow anything past the MDIO Mux values */
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
> +	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +	l1_cache = l1;
> +	l2 = readl(&ipctl->lev2->conf[pin]);
> +	l2_cache = l2;
> +
> +	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin, pin_config);
> +
> +	if (pin_config < RZN1_FUNC_L2_OFFSET) {
> +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> +		l1 |= (pin_config << RZN1_L1_FUNCTION);
> +	} else {
> +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> +		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
> +
> +		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
> +	}
> +
> +	/* If either configuration changes, we update both anyway */
> +	if (l1 != l1_cache || l2 != l2_cache) {
> +		writel(l1, &ipctl->lev1->conf[pin]);
> +		writel(l2, &ipctl->lev2->conf[pin]);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
> +	const struct rzn1_pinctrl *ipctl, const char *name)
> +{
> +	const struct rzn1_pin_group *grp = NULL;
> +	int i;
> +
> +	for (i = 0; i < ipctl->ngroups; i++) {
> +		if (!strcmp(ipctl->groups[i].name, name)) {
> +			grp = &ipctl->groups[i];
> +			break;
> +		}
> +	}
> +
> +	return grp;
> +}
> +
> +static int rzn1_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->ngroups;
> +}
> +
> +static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
> +				       unsigned int selector)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->groups[selector].name;
> +}
> +
> +static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
> +			       unsigned int selector, const unsigned int **pins,
> +			       unsigned int *npins)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (selector >= ipctl->ngroups)
> +		return -EINVAL;
> +
> +	*pins = ipctl->groups[selector].pins;
> +	*npins = ipctl->groups[selector].npins;
> +
> +	return 0;
> +}
> +
> +/*
> + * This function is called for each pinctl 'Function' node.
> + * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
> + * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
> + * Each 'Group' uses pinmux = <...> to detail the pins and data used to select
> + * the functionality. Each 'Group' has optional pin configurations that apply
> + * to all pins in the 'Group'.
> + */
> +static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
> +				   struct device_node *np,
> +				   struct pinctrl_map **map,
> +				   unsigned int *num_maps)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct rzn1_pin_group *grp;
> +	unsigned long *configs = NULL;
> +	unsigned int num_configs = 0;
> +	unsigned int reserved_maps = *num_maps;
> +	unsigned int reserve = 1;
> +	int ret;
> +
> +	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
> +
> +	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
> +	if (!grp) {
> +		dev_err(ipctl->dev, "unable to find group for node %pOF\n", np);
> +		return -EINVAL;
> +	}
> +
> +	/* Get the group's pin configuration */
> +	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
> +					      &num_configs);
> +	if (ret < 0) {
> +		dev_err(ipctl->dev, "%s: could not parse node property\n",
> +			of_node_full_name(np));

Why %pOF above and of_node_full_name? I would use the first, if
nothing changed recently with OF naming.

> +		return ret;
> +	}
> +
> +	if (num_configs)
> +		reserve++;
> +
> +	/* Increase the number of maps to cover this group */
> +	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
> +					reserve);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Associate the group with the function */
> +	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps, num_maps,
> +					grp->name, grp->func);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (num_configs) {
> +		/* Associate the group's pin configuration with the group */
> +		ret = pinctrl_utils_add_map_configs(pctldev, map,
> +				&reserved_maps, num_maps, grp->name,
> +				configs, num_configs,
> +				PIN_MAP_TYPE_CONFIGS_GROUP);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
> +	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
> +		grp->func, grp->name, grp->npins);
> +
> +out:
> +	kfree(configs);
> +	return 0;

You never return error here (nit: newline before returning, here and
in other places)

> +}
> +
> +static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			       struct device_node *np,
> +			       struct pinctrl_map **map,
> +			       unsigned int *num_maps)
> +{
> +	struct device_node *child;
> +	int ret;
> +
> +	*map = NULL;
> +	*num_maps = 0;
> +
> +	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
> +	if (ret < 0)
> +		return ret;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinctrl_ops rzn1_pctrl_ops = {
> +	.get_groups_count = rzn1_get_groups_count,
> +	.get_group_name = rzn1_get_group_name,
> +	.get_group_pins = rzn1_get_group_pins,
> +	.dt_node_to_map = rzn1_dt_node_to_map,
> +	.dt_free_map = pinctrl_utils_free_map,
> +};
> +
> +static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->nfunctions;
> +}
> +
> +static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
> +					  unsigned int selector)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return ipctl->functions[selector].name;
> +}
> +
> +static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
> +			       unsigned int selector,
> +			       const char * const **groups,
> +			       unsigned int * const num_groups)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = ipctl->functions[selector].groups;
> +	*num_groups = ipctl->functions[selector].num_groups;
> +
> +	return 0;
> +}
> +
> +static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			unsigned int group)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct rzn1_pin_group *grp = &ipctl->groups[group];
> +	unsigned int i, grp_pins = grp->npins;
> +
> +	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
> +		ipctl->functions[selector].name, selector, grp->name, group);
> +
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
> +	for (i = 0; i < grp_pins; i++)
> +		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
> +	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
> +
> +	return 0;
> +}
> +
> +static const struct pinmux_ops rzn1_pmx_ops = {
> +	.get_functions_count = rzn1_pmx_get_funcs_count,
> +	.get_function_name = rzn1_pmx_get_func_name,
> +	.get_function_groups = rzn1_pmx_get_groups,
> +	.set_mux = rzn1_set_mux,
> +};
> +
> +static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *config)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	const u32 reg_drive[4] = { 4, 6, 8, 12 };
> +	u32 l1, l2;
> +	u32 pull, drive, l1mux;
> +	u32 arg = 0;
> +
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +
> +	l1mux = l1 & RZN1_L1_FUNC_MASK;
> +	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
> +	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		if (pull == RZN1_L1_PIN_PULL_UP)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		if (pull == RZN1_L1_PIN_PULL_DOWN)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (pull == RZN1_L1_PIN_PULL_NONE)
> +			arg = 1;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		arg = reg_drive[drive];
> +		break;
> +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		l2 = readl(&ipctl->lev1->conf[pin]);
> +		if (l1mux == RZN1_FUNC_HIGHZ)
> +			arg = 1;
> +		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
> +			arg = 1;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +			    unsigned long *configs, unsigned int num_configs)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param;
> +	int i;
> +	u32 arg;
> +	u32 l1, l1_cache;
> +	u32 drv;
> +
> +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> +		return -EINVAL;
> +
> +	l1 = readl(&ipctl->lev1->conf[pin]);
> +	l1_cache = l1;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_UP << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_DOWN << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
> +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> +			l1 |= (RZN1_L1_PIN_PULL_NONE << RZN1_L1_PIN_PULL);
> +			break;
> +		case PIN_CONFIG_DRIVE_STRENGTH:
> +			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin, arg);
> +			drv = 0;
> +			switch (arg) {
> +			case 4:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
> +				break;
> +			case 6:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
> +				break;
> +			case 8:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
> +				break;
> +			case 12:
> +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
> +				break;
> +			default:
> +				dev_err(ipctl->dev,
> +					"Drive strength %umA not supported\n",
> +					arg);
> +				return -EINVAL;
> +			}
> +
> +			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
> +			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
> +			break;
> +
> +		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
> +			l1 &= ~RZN1_L1_FUNC_MASK;
> +			l1 |= RZN1_FUNC_HIGHZ;
> +			break;
> +		default:

I'll re-comment on bindings too, but the pinconf properties you
support should be listed there

> +			return -ENOTSUPP;
> +		}
> +	}
> +
> +	if (l1 != l1_cache) {
> +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
> +		writel(l1, &ipctl->lev1->conf[pin]);
> +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
> +				     unsigned int selector,
> +				     unsigned long *configs,
> +				     unsigned int num_configs)
> +{
> +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> +	struct rzn1_pin_group *grp = &ipctl->groups[selector];
> +	int ret, i;
> +
> +	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
> +		grp->name, selector, configs, num_configs);
> +
> +	for (i = 0; i < grp->npins; i++) {
> +		unsigned int pin = grp->pins[i];
> +
> +		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops rzn1_pinconf_ops = {
> +	.pin_config_get = rzn1_pinconf_get,
> +	.pin_config_set = rzn1_pinconf_set,
> +	.pin_config_group_set = rzn1_pin_config_group_set,

Nit: you register a pinconf map with PIN_MAP_TYPE_CONFIGS_GROUP so the
'pinconf_[set|get]' should never be called. Have you tested if they
do ? (just for my better understanding of the pinctrl framework :)

> +};
> +
> +static struct pinctrl_desc rzn1_pinctrl_desc = {
> +	.pctlops = &rzn1_pctrl_ops,
> +	.pmxops = &rzn1_pmx_ops,
> +	.confops = &rzn1_pinconf_ops,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int rzn1_pinctrl_parse_groups(struct device_node *np,
> +				     struct rzn1_pin_group *grp,
> +				     struct rzn1_pinctrl *ipctl)
> +{
> +	int size;
> +	const __be32 *list;
> +	int i;

unsigned int i;

It is only used in the bottom for loop afaict

> +
> +	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
> +
> +	/* Initialise group */
> +	grp->name = np->name;
> +
> +	/*
> +	 * The binding format is
> +	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
> +	 * do sanity check and calculate pins number
> +	 */
> +	list = of_get_property(np, RZN1_PINS_PROP, &size);
> +	if (!list) {
> +		dev_err(ipctl->dev,
> +			"no " RZN1_PINS_PROP " property in node %s\n",
> +			np->full_name);

Isn't it better to print out the OF node name with %pOF? Here and
below...

> +		return -EINVAL;
> +	}
> +
> +	/* We do not check return since it's safe node passed down */
> +	if (!size) {
> +		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node %s\n",
> +			np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	grp->npins = size / sizeof(list[0]);
> +	if (!grp->npins)
> +		return 0;

If you get here the property is there and has > 0 length
> +
> +	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
> +					  grp->npins, sizeof(grp->pin_ids[0]),
> +					  GFP_KERNEL);
> +	grp->pins = devm_kmalloc_array(ipctl->dev,
> +				       grp->npins, sizeof(grp->pins[0]),
> +				       GFP_KERNEL);
> +	if (!grp->pin_ids || !grp->pins)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < grp->npins; i++) {
> +		u32 pin_id = be32_to_cpu(*list++);
> +
> +		grp->pins[i] = pin_id & 0xff;
> +		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
> +	}
> +
> +	return grp->npins;
> +}
> +
> +static int rzn1_pinctrl_count_function_groups(struct device_node *np)
> +{
> +	struct device_node *child;
> +	int count = 0;
> +
> +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
> +		count++;
> +
> +	for_each_child_of_node(np, child) {
> +		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
> +			count++;
> +	}
> +
> +	return count;
> +}
> +
> +static int rzn1_pinctrl_parse_functions(struct device_node *np,
> +					struct rzn1_pinctrl *ipctl, u32 index)
> +{
> +	struct device_node *child;
> +	struct rzn1_pmx_func *func;
> +	struct rzn1_pin_group *grp;
> +	u32 i = 0;
> +	int ret;
> +
> +	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
> +
> +	func = &ipctl->functions[index];
> +
> +	/* Initialise function */
> +	func->name = np->name;
> +	func->num_groups = rzn1_pinctrl_count_function_groups(np);
> +	dev_dbg(ipctl->dev, "function %s has %d groups\n",
> +		np->name, func->num_groups);

Maybe move this debug printout below the error check, it helps to know
parsing went fine.

> +	if (func->num_groups == 0) {
> +		dev_err(ipctl->dev, "no groups defined in %s\n", np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	func->groups = devm_kmalloc_array(ipctl->dev,
> +					  func->num_groups, sizeof(char *),
> +					  GFP_KERNEL);
> +	if (!func->groups)
> +		return -ENOMEM;
> +
> +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
> +		func->groups[i] = np->name;
> +		grp = &ipctl->groups[ipctl->ngroups];
> +		grp->func = func->name;
> +		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
> +		if (ret < 0)
> +			return ret;
> +		if (ret > 0) {

You never return grp->npins == 0; it's either error or > 0. I think
you can drop this check.

> +			i++;
> +			ipctl->ngroups++;
> +		}

This design allows a sub-node with the following layout

        n-1 {
                pinmux = <..>;

                sub-n-1 {
                        pinmux = <..>;
                };

                sub-n-2 {
                        pinmux = <..>;
                };
        };

The bindings documentation only allows either this

        n-1 {
                pinmux = <..>;
        };

Or this
        n-1 {
                sub-n-1 {
                        pinmux = <..>;
                };

                sub-n-2 {
                        pinmux = <..>;
                };
        };

So I guess it's one or the other here. What do you think?

> +	}
> +
> +	for_each_child_of_node(np, child) {
> +		func->groups[i] = child->name;
> +		grp = &ipctl->groups[ipctl->ngroups];
> +		grp->func = func->name;
> +		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
> +		if (ret < 0)
> +			return ret;
> +		if (ret > 0) {
> +			i++;
> +			ipctl->ngroups++;
> +		}
> +	}
> +
> +	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
> +		np->name, i, func->num_groups);
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
> +				 struct rzn1_pinctrl *ipctl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *child;
> +	unsigned int maxgroups = 0;
> +	u32 nfuncs = 0;
> +	u32 i = 0;
> +	int ret;
> +
> +	nfuncs = of_get_child_count(np);
> +	if (nfuncs <= 0) {
> +		dev_err(&pdev->dev, "no functions defined\n");
> +		return -EINVAL;
> +	}

Do you really want to fail if no pinmuxing sub-node is defined? I can
think of boards which includes the SoC .dtsi but do not define (yet)
any pinmux/conf sub-nodes. This shouldn't be an error.

> +
> +	ipctl->nfunctions = nfuncs;
> +	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
> +					      sizeof(*ipctl->functions),
> +					      GFP_KERNEL);
> +	if (!ipctl->functions)
> +		return -ENOMEM;
> +
> +	ipctl->ngroups = 0;
> +	for_each_child_of_node(np, child)
> +		maxgroups += rzn1_pinctrl_count_function_groups(child);
> +
> +	ipctl->groups = devm_kmalloc_array(&pdev->dev,
> +					   maxgroups,
> +					   sizeof(*ipctl->groups),
> +					   GFP_KERNEL);
> +	if (!ipctl->groups)
> +		return -ENOMEM;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzn1_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct rzn1_pinctrl *ipctl;
> +	struct resource *res;
> +	int ret;
> +
> +	/* Create state holders etc for this driver */
> +	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
> +	if (!ipctl)
> +		return -ENOMEM;
> +
> +	ipctl->mdio_func[0] = -1;
> +	ipctl->mdio_func[1] = -1;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
> +	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ipctl->lev1))
> +		return PTR_ERR(ipctl->lev1);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
> +	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ipctl->lev2))
> +		return PTR_ERR(ipctl->lev2);
> +
> +	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(ipctl->clk))
> +		return PTR_ERR(ipctl->clk);
> +	ret = clk_prepare_enable(ipctl->clk);
> +	if (ret)
> +		return ret;
> +
> +	ipctl->dev = &pdev->dev;
> +	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
> +	rzn1_pinctrl_desc.pins = rzn1_pins;
> +	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
> +
> +	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "fail to probe dt properties\n");
> +		goto err_clk;
> +	}
> +
> +	platform_set_drvdata(pdev, ipctl);
> +	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
> +	if (!ipctl->pctl) {
> +		dev_err(&pdev->dev, "could not register rzn1 pinctrl driver\n");
> +		ret = -EINVAL;
> +		goto err_clk;
> +	}

Bindings claim this is a pinctrl/gpio controller, but I don't see any
gpio support here. I'll comment on the bindings for reference too.

Thanks
   j

> +
> +	dev_info(&pdev->dev, "probed\n");
> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(ipctl->clk);
> +
> +	return ret;
> +}
> +
> +static int rzn1_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(ipctl->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rzn1_pinctrl_match[] = {
> +	{ .compatible = "renesas,rzn1-pinctrl", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
> +
> +static struct platform_driver rzn1_pinctrl_driver = {
> +	.probe	= rzn1_pinctrl_probe,
> +	.remove = rzn1_pinctrl_remove,
> +	.driver	= {
> +		.name		= "rzn1-pinctrl",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= rzn1_pinctrl_match,
> +	},
> +};
> +
> +static int __init _pinctrl_drv_register(void)
> +{
> +	return platform_driver_register(&rzn1_pinctrl_driver);
> +}
> +subsys_initcall(_pinctrl_drv_register);
> +
> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
Phil Edworthy Sept. 18, 2018, 11:55 a.m. UTC | #2
Hi Jacopo,

On 18 September 2018 11:44 jacopo mondi wrote:
> Hi Phil,
>    thanks for the patch
Thanks for the review!


> On Mon, Sep 17, 2018 at 05:36:08PM +0100, Phil Edworthy wrote:
> > This provides a pinctrl driver for the Renesas RZ/N1 device family.
> >
> > Based on a patch originally written by Michel Pollet at Renesas.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v3:
> >  - Use standard DT props instead of proprietary ones.
> >  - Replace virtual pins used for MDIO muxing with extra funcs.
> >  - Use pinctrl_utils funcs to handle the maps.
> >  - Remove the dbg functions to keep things simple.
> >
> > v2:
> >  - Change filename to generic rzn1, instead of device specific.
> >  - Changed Kconfig symbol and file name to generic rzn1 family.
> >  - Added "renesas,rzn1-pinctrl" compatible fallback string
> >  - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
> >    - Removed global ptr
> >    - Removed unused code accessing parent of node.
> >    - Removed check for null OF np that can't happen.
> >    - Replaced overlapping enums with #defines
> >  - Renamed some variables and symbols to clarify their use
> >  - Fix error handling during probe
> >  - Move probe from postcore_initcall to subsys_initcall to ensure
> >    drivers that require clocks get them instead of having to defer
> >    probing.
> > ---
> >  drivers/pinctrl/Kconfig        |  10 +
> >  drivers/pinctrl/Makefile       |   1 +
> >  drivers/pinctrl/pinctrl-rzn1.c | 926
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 937 insertions(+)
> >  create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
> >
> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> > e86752be1f19..e524eb101384 100644
> > --- a/drivers/pinctrl/Kconfig
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -195,6 +195,16 @@ config PINCTRL_RZA1
> >  	help
> >  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
> >
> > +config PINCTRL_RZN1
> > +	bool "Renesas RZ/N1 pinctrl driver"
> > +	depends on OF
> > +	depends on ARCH_RZN1 || COMPILE_TEST
> > +	select GENERIC_PINCTRL_GROUPS
> > +	select GENERIC_PINMUX_FUNCTIONS
> > +	select GENERIC_PINCONF
> > +	help
> > +	  This selects pinctrl driver for Renesas RZ/N1 devices.
> > +
> >  config PINCTRL_SINGLE
> >  	tristate "One-register-per-pin type device tree based pinctrl driver"
> >  	depends on OF
> > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> > 46ef9bd52096..d07f9a20f6ae 100644
> > --- a/drivers/pinctrl/Makefile
> > +++ b/drivers/pinctrl/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
> >  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
> >  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
> >  obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
> > +obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
> >  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
> >  obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
> >  obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c new file mode 100644 index
> > 000000000000..8f0caa266dbb
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -0,0 +1,926 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
> > + *
> > + * Phil Edworthy <phil.edworthy@renesas.com>
> > + * Based on a driver originally written by Michel Pollet at Renesas.
> > + */
> > +
> > +#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pinctrl/pinconf-generic.h> #include
> > +<linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> #include
> > +<linux/platform_device.h> #include <linux/slab.h> #include "core.h"
> > +#include "pinconf.h"
> > +#include "pinctrl-utils.h"
> > +
> > +/* Field positions and masks in the pinmux registers */
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH	10
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
> > +#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
> > +#define RZN1_L1_PIN_PULL		8
> > +#define RZN1_L1_PIN_PULL_NONE		0
> > +#define RZN1_L1_PIN_PULL_UP		1
> > +#define RZN1_L1_PIN_PULL_DOWN		3
> > +#define RZN1_L1_FUNCTION		0
> > +#define RZN1_L1_FUNC_MASK		0xf
> > +#define RZN1_L1_FUNCTION_L2		0xf
> > +
> > +/*
> > + * The hardware manual describes two levels of multiplexing, but it's
> > +more
> > + * logical to think of the hardware as three levels, with level 3
> > +consisting of
> > + * the multiplexing for Ethernet MDIO signals.
> > + *
> > + * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf)
> > +specifying
> > + * that level 2 functions are used instead. Level 2 has a lot more
> > +options,
> > + * going from 0 to 61. Level 3 allows selection of MDIO functions
> > +which can be
> > + * floating, or one of seven internal peripherals. Unfortunately,
> > +there are two
> > + * level 2 functions that can select MDIO, and two MDIO channels so
> > +we have four
> > + * sets of level 3 functions.
> > + *
> > + * For this driver, we've compounded the numbers together, so:
> > + *    0 to   9 is level 1
> > + *   10 to  71 is 10 + level 2 number
> > + *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
> > + *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
> > + *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
> > + *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
> > + * Examples:
> > + *  Function 28 corresponds UART0
> > + *  Function 73 corresponds to MDIO0 to GMAC0
> > + *
> > + * There are 170 configurable pins (called PL_GPIO in the datasheet).
> > + */
> > +
> > +/*
> > + * Structure detailing the HW registers on the RZ/N1 devices.
> > + * Both the Level 1 mux registers and Level 2 mux registers have the
> > +same
> > + * structure. The only difference is that Level 2 has additional MDIO
> > +registers
> > + * at the end.
> > + */
> > +struct rzn1_pinctrl_regs {
> > +	union {
> > +		u32	conf[170];
> > +		u8	pad0[0x400];
> 
> Is pad0 actually used?
No, it's just to implement the padding. Would you prefer not using a union
here?

> > +	};
> > +	u32	status_protect;	/* 0x400 */
> > +	/* MDIO mux registers, level2 only */
> > +	u32	l2_mdio[2];
> > +};
> > +
> > +/**
> > + * struct rzn1_pmx_func - describes rzn1 pinmux functions
> > + * @name: the name of this specific function
> > + * @groups: corresponding pin groups
> > + * @num_groups: the number of groups
> > + */
> > +struct rzn1_pmx_func {
> > +	const char *name;
> > +	const char **groups;
> > +	unsigned int num_groups;
> > +};
> > +
> > +/**
> > + * struct rzn1_pin_group - describes an rzn1 pin group
> > + * @name: the name of this specific pin group
> > + * @func: the name of the function selected by this group
> > + * @npins: the number of pins in this group array, i.e. the number of
> > + *	elements in .pins so we can iterate over that array
> > + * @pin_ids: array of pin_ids, i.e. the value used to select the mux
> > + * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
> > +*/ struct rzn1_pin_group {
> > +	const char *name;
> > +	const char *func;
> > +	unsigned int npins;
> > +	u8 *pin_ids;
> > +	u32 *pins;
> > +};
> > +
> > +struct rzn1_pinctrl {
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	struct pinctrl_dev *pctl;
> > +	struct rzn1_pinctrl_regs __iomem *lev1;
> > +	struct rzn1_pinctrl_regs __iomem *lev2;
> > +	u32 lev1_protect_phys;
> > +	u32 lev2_protect_phys;
> > +	int mdio_func[2];
> > +
> > +	struct rzn1_pin_group *groups;
> > +	unsigned int ngroups;
> > +
> > +	struct rzn1_pmx_func *functions;
> > +	unsigned int nfunctions;
> > +};
> > +
> > +#define RZN1_PINS_PROP "pinmux"
> > +
> > +#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
> > +
> > +static const struct pinctrl_pin_desc rzn1_pins[] = {
> > +	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3),
> RZN1_PIN(4),
> > +	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8),
> RZN1_PIN(9),
> > +	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13),
> RZN1_PIN(14),
> > +	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18),
> RZN1_PIN(19),
> > +	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23),
> RZN1_PIN(24),
> > +	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28),
> RZN1_PIN(29),
> > +	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33),
> RZN1_PIN(34),
> > +	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38),
> RZN1_PIN(39),
> > +	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43),
> RZN1_PIN(44),
> > +	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48),
> RZN1_PIN(49),
> > +	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53),
> RZN1_PIN(54),
> > +	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58),
> RZN1_PIN(59),
> > +	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63),
> RZN1_PIN(64),
> > +	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68),
> RZN1_PIN(69),
> > +	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73),
> RZN1_PIN(74),
> > +	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78),
> RZN1_PIN(79),
> > +	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83),
> RZN1_PIN(84),
> > +	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88),
> RZN1_PIN(89),
> > +	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93),
> RZN1_PIN(94),
> > +	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98),
> RZN1_PIN(99),
> > +	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
> > +	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
> > +	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
> > +	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
> > +	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
> > +	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
> > +	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
> > +	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
> > +	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
> > +	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
> > +	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
> > +	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
> > +	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
> > +	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
> > +	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
> > +	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
> > +	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
> > +	RZN1_PIN(168), RZN1_PIN(169),
> > +};
> > +
> > +enum {
> > +	LOCK_LEVEL1 = 0x1,
> > +	LOCK_LEVEL2 = 0x2,
> > +	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,
> 
> 0x03 is already (0x01 | 0x02) :)
> Not a big deal though.
Sure


> > +};
> > +
> > +static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8
> > +value) {
> > +	/*
> > +	 * The pinmux configuration is locked by writing the physical address
> of
> > +	 * the status_protect register to itself. It is unlocked by writing the
> > +	 * address | 1.
> > +	 */
> > +	if (lock & LOCK_LEVEL1) {
> > +		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
> > +
> > +		writel(val, &ipctl->lev1->status_protect);
> > +	}
> > +
> > +	if (lock & LOCK_LEVEL2) {
> > +		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
> > +
> > +		writel(val, &ipctl->lev2->status_protect);
> > +	}
> > +}
> > +
> > +static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
> > +				     u32 func)
> > +{
> > +	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > +		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> > +	ipctl->mdio_func[mdio] = func;
> > +
> > +	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
> > +
> > +	writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> > +
> > +/*
> > + * Using a composite pin description, set the hardware pinmux
> > +registers
> > + * with the corresponding values.
> > + * Make sure to unlock write protection and reset it afterward.
> > + *
> > + * NOTE: There is no protection for potential concurrency, it is
> > +assumed these
> > + * calls are serialized already.
> > + */
> > +static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
> > +				u32 pin_config, u8 use_locks)
> > +{
> > +	u32 l1_cache;
> > +	u32 l2_cache;
> > +	u32 l1;
> > +	u32 l2;
> > +
> > +	/* Level 3 MDIO multiplexing */
> > +	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
> > +	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
> > +		u8 mdio_channel;
> > +		u32 mdio_func;
> > +
> > +		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
> > +			mdio_channel = 0;
> > +		else
> > +			mdio_channel = 1;
> > +
> > +		/* Get MDIO func, and convert the func to the level 2
> number */
> > +		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO0_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO;
> > +		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO0_E1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> > +		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO;
> > +		} else {
> > +			mdio_func = pin_config -
> RZN1_FUNC_MDIO1_E1_HIGHZ;
> > +			pin_config = RZN1_FUNC_ETH_MDIO_E1;
> > +		}
> > +		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
> > +	}
> > +
> > +	/* Note here, we do not allow anything past the MDIO Mux values */
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
> > +	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +	l1_cache = l1;
> > +	l2 = readl(&ipctl->lev2->conf[pin]);
> > +	l2_cache = l2;
> > +
> > +	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin,
> > +pin_config);
> > +
> > +	if (pin_config < RZN1_FUNC_L2_OFFSET) {
> > +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> > +		l1 |= (pin_config << RZN1_L1_FUNCTION);
> > +	} else {
> > +		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
> > +		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
> > +
> > +		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
> > +	}
> > +
> > +	/* If either configuration changes, we update both anyway */
> > +	if (l1 != l1_cache || l2 != l2_cache) {
> > +		writel(l1, &ipctl->lev1->conf[pin]);
> > +		writel(l2, &ipctl->lev2->conf[pin]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
> > +	const struct rzn1_pinctrl *ipctl, const char *name) {
> > +	const struct rzn1_pin_group *grp = NULL;
> > +	int i;
> > +
> > +	for (i = 0; i < ipctl->ngroups; i++) {
> > +		if (!strcmp(ipctl->groups[i].name, name)) {
> > +			grp = &ipctl->groups[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	return grp;
> > +}
> > +
> > +static int rzn1_get_groups_count(struct pinctrl_dev *pctldev) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->ngroups;
> > +}
> > +
> > +static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
> > +				       unsigned int selector)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->groups[selector].name; }
> > +
> > +static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
> > +			       unsigned int selector, const unsigned int **pins,
> > +			       unsigned int *npins)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	if (selector >= ipctl->ngroups)
> > +		return -EINVAL;
> > +
> > +	*pins = ipctl->groups[selector].pins;
> > +	*npins = ipctl->groups[selector].npins;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This function is called for each pinctl 'Function' node.
> > + * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
> > + * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
> > + * Each 'Group' uses pinmux = <...> to detail the pins and data used
> > +to select
> > + * the functionality. Each 'Group' has optional pin configurations
> > +that apply
> > + * to all pins in the 'Group'.
> > + */
> > +static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
> > +				   struct device_node *np,
> > +				   struct pinctrl_map **map,
> > +				   unsigned int *num_maps)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	const struct rzn1_pin_group *grp;
> > +	unsigned long *configs = NULL;
> > +	unsigned int num_configs = 0;
> > +	unsigned int reserved_maps = *num_maps;
> > +	unsigned int reserve = 1;
> > +	int ret;
> > +
> > +	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
> > +
> > +	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
> > +	if (!grp) {
> > +		dev_err(ipctl->dev, "unable to find group for node %pOF\n",
> np);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get the group's pin configuration */
> > +	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
> > +					      &num_configs);
> > +	if (ret < 0) {
> > +		dev_err(ipctl->dev, "%s: could not parse node property\n",
> > +			of_node_full_name(np));
> 
> Why %pOF above and of_node_full_name? I would use the first, if nothing
> changed recently with OF naming.
You are right, %pOF should be used


> > +		return ret;
> > +	}
> > +
> > +	if (num_configs)
> > +		reserve++;
> > +
> > +	/* Increase the number of maps to cover this group */
> > +	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps,
> num_maps,
> > +					reserve);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	/* Associate the group with the function */
> > +	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps,
> num_maps,
> > +					grp->name, grp->func);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (num_configs) {
> > +		/* Associate the group's pin configuration with the group */
> > +		ret = pinctrl_utils_add_map_configs(pctldev, map,
> > +				&reserved_maps, num_maps, grp->name,
> > +				configs, num_configs,
> > +				PIN_MAP_TYPE_CONFIGS_GROUP);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> > +	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
> > +		grp->func, grp->name, grp->npins);
> > +
> > +out:
> > +	kfree(configs);
> > +	return 0;
> 
> You never return error here (nit: newline before returning, here and in other
> places)
Ok, will fix.


> > +}
> > +
> > +static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +			       struct device_node *np,
> > +			       struct pinctrl_map **map,
> > +			       unsigned int *num_maps)
> > +{
> > +	struct device_node *child;
> > +	int ret;
> > +
> > +	*map = NULL;
> > +	*num_maps = 0;
> > +
> > +	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		ret = rzn1_dt_node_to_map_one(pctldev, child, map,
> num_maps);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinctrl_ops rzn1_pctrl_ops = {
> > +	.get_groups_count = rzn1_get_groups_count,
> > +	.get_group_name = rzn1_get_group_name,
> > +	.get_group_pins = rzn1_get_group_pins,
> > +	.dt_node_to_map = rzn1_dt_node_to_map,
> > +	.dt_free_map = pinctrl_utils_free_map, };
> > +
> > +static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->nfunctions;
> > +}
> > +
> > +static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
> > +					  unsigned int selector)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	return ipctl->functions[selector].name; }
> > +
> > +static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
> > +			       unsigned int selector,
> > +			       const char * const **groups,
> > +			       unsigned int * const num_groups) {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	*groups = ipctl->functions[selector].groups;
> > +	*num_groups = ipctl->functions[selector].num_groups;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> > +			unsigned int group)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	struct rzn1_pin_group *grp = &ipctl->groups[group];
> > +	unsigned int i, grp_pins = grp->npins;
> > +
> > +	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
> > +		ipctl->functions[selector].name, selector, grp->name,
> group);
> > +
> > +	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
> > +	for (i = 0; i < grp_pins; i++)
> > +		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
> > +	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinmux_ops rzn1_pmx_ops = {
> > +	.get_functions_count = rzn1_pmx_get_funcs_count,
> > +	.get_function_name = rzn1_pmx_get_func_name,
> > +	.get_function_groups = rzn1_pmx_get_groups,
> > +	.set_mux = rzn1_set_mux,
> > +};
> > +
> > +static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > +			    unsigned long *config)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	enum pin_config_param param =
> pinconf_to_config_param(*config);
> > +	const u32 reg_drive[4] = { 4, 6, 8, 12 };
> > +	u32 l1, l2;
> > +	u32 pull, drive, l1mux;
> > +	u32 arg = 0;
> > +
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +
> > +	l1mux = l1 & RZN1_L1_FUNC_MASK;
> > +	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
> > +	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
> > +
> > +	switch (param) {
> > +	case PIN_CONFIG_BIAS_PULL_UP:
> > +		if (pull == RZN1_L1_PIN_PULL_UP)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_BIAS_PULL_DOWN:
> > +		if (pull == RZN1_L1_PIN_PULL_DOWN)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_BIAS_DISABLE:
> > +		if (pull == RZN1_L1_PIN_PULL_NONE)
> > +			arg = 1;
> > +		break;
> > +	case PIN_CONFIG_DRIVE_STRENGTH:
> > +		arg = reg_drive[drive];
> > +		break;
> > +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > +		l2 = readl(&ipctl->lev1->conf[pin]);
> > +		if (l1mux == RZN1_FUNC_HIGHZ)
> > +			arg = 1;
> > +		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
> > +			arg = 1;
> > +		break;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +
> > +	*config = pinconf_to_config_packed(param, arg);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > +			    unsigned long *configs, unsigned int num_configs)
> {
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	enum pin_config_param param;
> > +	int i;
> > +	u32 arg;
> > +	u32 l1, l1_cache;
> > +	u32 drv;
> > +
> > +	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
> > +		return -EINVAL;
> > +
> > +	l1 = readl(&ipctl->lev1->conf[pin]);
> > +	l1_cache = l1;
> > +
> > +	for (i = 0; i < num_configs; i++) {
> > +		param = pinconf_to_config_param(configs[i]);
> > +		arg = pinconf_to_config_argument(configs[i]);
> > +
> > +		switch (param) {
> > +		case PIN_CONFIG_BIAS_PULL_UP:
> > +			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_UP <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_BIAS_PULL_DOWN:
> > +			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_DOWN <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_BIAS_DISABLE:
> > +			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
> > +			l1 |= (RZN1_L1_PIN_PULL_NONE <<
> RZN1_L1_PIN_PULL);
> > +			break;
> > +		case PIN_CONFIG_DRIVE_STRENGTH:
> > +			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin,
> arg);
> > +			drv = 0;
> > +			switch (arg) {
> > +			case 4:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
> > +				break;
> > +			case 6:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
> > +				break;
> > +			case 8:
> > +				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
> > +				break;
> > +			case 12:
> > +				drv =
> RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
> > +				break;
> > +			default:
> > +				dev_err(ipctl->dev,
> > +					"Drive strength %umA not
> supported\n",
> > +					arg);
> > +				return -EINVAL;
> > +			}
> > +
> > +			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
> > +			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
> > +			break;
> > +
> > +		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > +			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
> > +			l1 &= ~RZN1_L1_FUNC_MASK;
> > +			l1 |= RZN1_FUNC_HIGHZ;
> > +			break;
> > +		default:
> 
> I'll re-comment on bindings too, but the pinconf properties you support
> should be listed there
Ok, I'll fix that.


> > +			return -ENOTSUPP;
> > +		}
> > +	}
> > +
> > +	if (l1 != l1_cache) {
> > +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
> > +		writel(l1, &ipctl->lev1->conf[pin]);
> > +		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
> > +				     unsigned int selector,
> > +				     unsigned long *configs,
> > +				     unsigned int num_configs)
> > +{
> > +	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> > +	struct rzn1_pin_group *grp = &ipctl->groups[selector];
> > +	int ret, i;
> > +
> > +	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
> > +		grp->name, selector, configs, num_configs);
> > +
> > +	for (i = 0; i < grp->npins; i++) {
> > +		unsigned int pin = grp->pins[i];
> > +
> > +		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pinconf_ops rzn1_pinconf_ops = {
> > +	.pin_config_get = rzn1_pinconf_get,
> > +	.pin_config_set = rzn1_pinconf_set,
> > +	.pin_config_group_set = rzn1_pin_config_group_set,
> 
> Nit: you register a pinconf map with PIN_MAP_TYPE_CONFIGS_GROUP so
> the 'pinconf_[set|get]' should never be called. Have you tested if they do ?
> (just for my better understanding of the pinctrl framework :)
Ah, right!  pin_config_get is not called at all. pin_config_set is only called
from rzn1_pin_config_group_set().

So, I guess that means I should implement pin_config_group_get as well.


> > +};
> > +
> > +static struct pinctrl_desc rzn1_pinctrl_desc = {
> > +	.pctlops = &rzn1_pctrl_ops,
> > +	.pmxops = &rzn1_pmx_ops,
> > +	.confops = &rzn1_pinconf_ops,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int rzn1_pinctrl_parse_groups(struct device_node *np,
> > +				     struct rzn1_pin_group *grp,
> > +				     struct rzn1_pinctrl *ipctl)
> > +{
> > +	int size;
> > +	const __be32 *list;
> > +	int i;
> 
> unsigned int i;
> 
> It is only used in the bottom for loop afaict
Yes indeed, I'll fix.


> > +
> > +	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
> > +
> > +	/* Initialise group */
> > +	grp->name = np->name;
> > +
> > +	/*
> > +	 * The binding format is
> > +	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
> > +	 * do sanity check and calculate pins number
> > +	 */
> > +	list = of_get_property(np, RZN1_PINS_PROP, &size);
> > +	if (!list) {
> > +		dev_err(ipctl->dev,
> > +			"no " RZN1_PINS_PROP " property in node %s\n",
> > +			np->full_name);
> 
> Isn't it better to print out the OF node name with %pOF? Here and below...
Sure


> > +		return -EINVAL;
> > +	}
> > +
> > +	/* We do not check return since it's safe node passed down */
> > +	if (!size) {
> > +		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node
> %s\n",
> > +			np->full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	grp->npins = size / sizeof(list[0]);
> > +	if (!grp->npins)
> > +		return 0;
> 
> If you get here the property is there and has > 0 length
True


> > +
> > +	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
> > +					  grp->npins, sizeof(grp->pin_ids[0]),
> > +					  GFP_KERNEL);
> > +	grp->pins = devm_kmalloc_array(ipctl->dev,
> > +				       grp->npins, sizeof(grp->pins[0]),
> > +				       GFP_KERNEL);
> > +	if (!grp->pin_ids || !grp->pins)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < grp->npins; i++) {
> > +		u32 pin_id = be32_to_cpu(*list++);
> > +
> > +		grp->pins[i] = pin_id & 0xff;
> > +		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
> > +	}
> > +
> > +	return grp->npins;
> > +}
> > +
> > +static int rzn1_pinctrl_count_function_groups(struct device_node *np)
> > +{
> > +	struct device_node *child;
> > +	int count = 0;
> > +
> > +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
> > +		count++;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		if (of_property_count_u32_elems(child, RZN1_PINS_PROP)
> > 0)
> > +			count++;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static int rzn1_pinctrl_parse_functions(struct device_node *np,
> > +					struct rzn1_pinctrl *ipctl, u32 index) {
> > +	struct device_node *child;
> > +	struct rzn1_pmx_func *func;
> > +	struct rzn1_pin_group *grp;
> > +	u32 i = 0;
> > +	int ret;
> > +
> > +	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
> > +
> > +	func = &ipctl->functions[index];
> > +
> > +	/* Initialise function */
> > +	func->name = np->name;
> > +	func->num_groups = rzn1_pinctrl_count_function_groups(np);
> > +	dev_dbg(ipctl->dev, "function %s has %d groups\n",
> > +		np->name, func->num_groups);
> 
> Maybe move this debug printout below the error check, it helps to know
> parsing went fine.
Makes sense


> > +	if (func->num_groups == 0) {
> > +		dev_err(ipctl->dev, "no groups defined in %s\n", np-
> >full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	func->groups = devm_kmalloc_array(ipctl->dev,
> > +					  func->num_groups, sizeof(char *),
> > +					  GFP_KERNEL);
> > +	if (!func->groups)
> > +		return -ENOMEM;
> > +
> > +	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
> > +		func->groups[i] = np->name;
> > +		grp = &ipctl->groups[ipctl->ngroups];
> > +		grp->func = func->name;
> > +		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret > 0) {
> 
> You never return grp->npins == 0; it's either error or > 0. I think you can drop
> this check.
> 
> > +			i++;
> > +			ipctl->ngroups++;
> > +		}
> 
> This design allows a sub-node with the following layout
> 
>         n-1 {
>                 pinmux = <..>;
> 
>                 sub-n-1 {
>                         pinmux = <..>;
>                 };
> 
>                 sub-n-2 {
>                         pinmux = <..>;
>                 };
>         };
> 
> The bindings documentation only allows either this
> 
>         n-1 {
>                 pinmux = <..>;
>         };
> 
> Or this
>         n-1 {
>                 sub-n-1 {
>                         pinmux = <..>;
>                 };
> 
>                 sub-n-2 {
>                         pinmux = <..>;
>                 };
>         };
> 
> So I guess it's one or the other here. What do you think?
Indeed, the bindings need updating to cover the extra case. I think
this extra case is quite neat.


> > +	}
> > +
> > +	for_each_child_of_node(np, child) {
> > +		func->groups[i] = child->name;
> > +		grp = &ipctl->groups[ipctl->ngroups];
> > +		grp->func = func->name;
> > +		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret > 0) {
> > +			i++;
> > +			ipctl->ngroups++;
> > +		}
> > +	}
> > +
> > +	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
> > +		np->name, i, func->num_groups);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
> > +				 struct rzn1_pinctrl *ipctl)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device_node *child;
> > +	unsigned int maxgroups = 0;
> > +	u32 nfuncs = 0;
> > +	u32 i = 0;
> > +	int ret;
> > +
> > +	nfuncs = of_get_child_count(np);
> > +	if (nfuncs <= 0) {
> > +		dev_err(&pdev->dev, "no functions defined\n");
> > +		return -EINVAL;
> > +	}
> 
> Do you really want to fail if no pinmuxing sub-node is defined? I can think of
> boards which includes the SoC .dtsi but do not define (yet) any pinmux/conf
> sub-nodes. This shouldn't be an error.
Makes sense.


> > +
> > +	ipctl->nfunctions = nfuncs;
> > +	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
> > +					      sizeof(*ipctl->functions),
> > +					      GFP_KERNEL);
> > +	if (!ipctl->functions)
> > +		return -ENOMEM;
> > +
> > +	ipctl->ngroups = 0;
> > +	for_each_child_of_node(np, child)
> > +		maxgroups += rzn1_pinctrl_count_function_groups(child);
> > +
> > +	ipctl->groups = devm_kmalloc_array(&pdev->dev,
> > +					   maxgroups,
> > +					   sizeof(*ipctl->groups),
> > +					   GFP_KERNEL);
> > +	if (!ipctl->groups)
> > +		return -ENOMEM;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_pinctrl_probe(struct platform_device *pdev) {
> > +	struct rzn1_pinctrl *ipctl;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	/* Create state holders etc for this driver */
> > +	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
> > +	if (!ipctl)
> > +		return -ENOMEM;
> > +
> > +	ipctl->mdio_func[0] = -1;
> > +	ipctl->mdio_func[1] = -1;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
> > +	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ipctl->lev1))
> > +		return PTR_ERR(ipctl->lev1);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
> > +	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ipctl->lev2))
> > +		return PTR_ERR(ipctl->lev2);
> > +
> > +	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
> > +	if (IS_ERR(ipctl->clk))
> > +		return PTR_ERR(ipctl->clk);
> > +	ret = clk_prepare_enable(ipctl->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ipctl->dev = &pdev->dev;
> > +	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
> > +	rzn1_pinctrl_desc.pins = rzn1_pins;
> > +	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
> > +
> > +	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "fail to probe dt properties\n");
> > +		goto err_clk;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, ipctl);
> > +	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
> > +	if (!ipctl->pctl) {
> > +		dev_err(&pdev->dev, "could not register rzn1 pinctrl
> driver\n");
> > +		ret = -EINVAL;
> > +		goto err_clk;
> > +	}
> 
> Bindings claim this is a pinctrl/gpio controller, but I don't see any gpio support
> here. I'll comment on the bindings for reference too.
Indeed, I'll remove from the binding.

Thanks again for the review!
Phil


> Thanks
>    j
> 
> > +
> > +	dev_info(&pdev->dev, "probed\n");
> > +
> > +	return 0;
> > +
> > +err_clk:
> > +	clk_disable_unprepare(ipctl->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rzn1_pinctrl_remove(struct platform_device *pdev) {
> > +	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
> > +
> > +	clk_disable_unprepare(ipctl->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rzn1_pinctrl_match[] = {
> > +	{ .compatible = "renesas,rzn1-pinctrl", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
> > +
> > +static struct platform_driver rzn1_pinctrl_driver = {
> > +	.probe	= rzn1_pinctrl_probe,
> > +	.remove = rzn1_pinctrl_remove,
> > +	.driver	= {
> > +		.name		= "rzn1-pinctrl",
> > +		.owner		= THIS_MODULE,
> > +		.of_match_table	= rzn1_pinctrl_match,
> > +	},
> > +};
> > +
> > +static int __init _pinctrl_drv_register(void) {
> > +	return platform_driver_register(&rzn1_pinctrl_driver);
> > +}
> > +subsys_initcall(_pinctrl_drv_register);
> > +
> > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
Jacopo Mondi Sept. 18, 2018, 1:33 p.m. UTC | #3
Hi Phil,

On Tue, Sep 18, 2018 at 11:55:16AM +0000, Phil Edworthy wrote:
> Hi Jacopo,
>

[snip]

> > > +
> > > +/*
> > > + * Structure detailing the HW registers on the RZ/N1 devices.
> > > + * Both the Level 1 mux registers and Level 2 mux registers have the
> > > +same
> > > + * structure. The only difference is that Level 2 has additional MDIO
> > > +registers
> > > + * at the end.
> > > + */
> > > +struct rzn1_pinctrl_regs {
> > > +	union {
> > > +		u32	conf[170];
> > > +		u8	pad0[0x400];
> >
> > Is pad0 actually used?
> No, it's just to implement the padding. Would you prefer not using a union
> here?

Oh, I did the math wrong, to me it was (32*170 > 8*400) but it's
actually (32*170 < 8*1024).

Also using a struct to define the memory region layout confused me and
I was about to ask "WHY ARE YOU RESERVING MEMORY HERE???" but this
type is just used for pointers, and it makes accessing HW locations
nicer actually (thanks Geert for having saved me a silly comment on this).

Cheers
   j
diff mbox series

Patch

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index e86752be1f19..e524eb101384 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,16 @@  config PINCTRL_RZA1
 	help
 	  This selects pinctrl driver for Renesas RZ/A1 platforms.
 
+config PINCTRL_RZN1
+	bool "Renesas RZ/N1 pinctrl driver"
+	depends on OF
+	depends on ARCH_RZN1 || COMPILE_TEST
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas RZ/N1 devices.
+
 config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 46ef9bd52096..d07f9a20f6ae 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -27,6 +27,7 @@  obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
+obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
new file mode 100644
index 000000000000..8f0caa266dbb
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -0,0 +1,926 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
+ *
+ * Phil Edworthy <phil.edworthy@renesas.com>
+ * Based on a driver originally written by Michel Pollet at Renesas.
+ */
+
+#include <dt-bindings/pinctrl/rzn1-pinctrl.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+/* Field positions and masks in the pinmux registers */
+#define RZN1_L1_PIN_DRIVE_STRENGTH	10
+#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA	0
+#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA	1
+#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA	2
+#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA	3
+#define RZN1_L1_PIN_PULL		8
+#define RZN1_L1_PIN_PULL_NONE		0
+#define RZN1_L1_PIN_PULL_UP		1
+#define RZN1_L1_PIN_PULL_DOWN		3
+#define RZN1_L1_FUNCTION		0
+#define RZN1_L1_FUNC_MASK		0xf
+#define RZN1_L1_FUNCTION_L2		0xf
+
+/*
+ * The hardware manual describes two levels of multiplexing, but it's more
+ * logical to think of the hardware as three levels, with level 3 consisting of
+ * the multiplexing for Ethernet MDIO signals.
+ *
+ * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf) specifying
+ * that level 2 functions are used instead. Level 2 has a lot more options,
+ * going from 0 to 61. Level 3 allows selection of MDIO functions which can be
+ * floating, or one of seven internal peripherals. Unfortunately, there are two
+ * level 2 functions that can select MDIO, and two MDIO channels so we have four
+ * sets of level 3 functions.
+ *
+ * For this driver, we've compounded the numbers together, so:
+ *    0 to   9 is level 1
+ *   10 to  71 is 10 + level 2 number
+ *   72 to  79 is 72 + MDIO0 source for level 2 MDIO function.
+ *   80 to  87 is 80 + MDIO0 source for level 2 MDIO_E1 function.
+ *   88 to  95 is 88 + MDIO1 source for level 2 MDIO function.
+ *   96 to 103 is 96 + MDIO1 source for level 2 MDIO_E1 function.
+ * Examples:
+ *  Function 28 corresponds UART0
+ *  Function 73 corresponds to MDIO0 to GMAC0
+ *
+ * There are 170 configurable pins (called PL_GPIO in the datasheet).
+ */
+
+/*
+ * Structure detailing the HW registers on the RZ/N1 devices.
+ * Both the Level 1 mux registers and Level 2 mux registers have the same
+ * structure. The only difference is that Level 2 has additional MDIO registers
+ * at the end.
+ */
+struct rzn1_pinctrl_regs {
+	union {
+		u32	conf[170];
+		u8	pad0[0x400];
+	};
+	u32	status_protect;	/* 0x400 */
+	/* MDIO mux registers, level2 only */
+	u32	l2_mdio[2];
+};
+
+/**
+ * struct rzn1_pmx_func - describes rzn1 pinmux functions
+ * @name: the name of this specific function
+ * @groups: corresponding pin groups
+ * @num_groups: the number of groups
+ */
+struct rzn1_pmx_func {
+	const char *name;
+	const char **groups;
+	unsigned int num_groups;
+};
+
+/**
+ * struct rzn1_pin_group - describes an rzn1 pin group
+ * @name: the name of this specific pin group
+ * @func: the name of the function selected by this group
+ * @npins: the number of pins in this group array, i.e. the number of
+ *	elements in .pins so we can iterate over that array
+ * @pin_ids: array of pin_ids, i.e. the value used to select the mux
+ * @pins: array of pins. Needed due to pinctrl_ops.get_group_pins()
+ */
+struct rzn1_pin_group {
+	const char *name;
+	const char *func;
+	unsigned int npins;
+	u8 *pin_ids;
+	u32 *pins;
+};
+
+struct rzn1_pinctrl {
+	struct device *dev;
+	struct clk *clk;
+	struct pinctrl_dev *pctl;
+	struct rzn1_pinctrl_regs __iomem *lev1;
+	struct rzn1_pinctrl_regs __iomem *lev2;
+	u32 lev1_protect_phys;
+	u32 lev2_protect_phys;
+	int mdio_func[2];
+
+	struct rzn1_pin_group *groups;
+	unsigned int ngroups;
+
+	struct rzn1_pmx_func *functions;
+	unsigned int nfunctions;
+};
+
+#define RZN1_PINS_PROP "pinmux"
+
+#define RZN1_PIN(pin) PINCTRL_PIN(pin, "pl_gpio"#pin)
+
+static const struct pinctrl_pin_desc rzn1_pins[] = {
+	RZN1_PIN(0), RZN1_PIN(1), RZN1_PIN(2), RZN1_PIN(3), RZN1_PIN(4),
+	RZN1_PIN(5), RZN1_PIN(6), RZN1_PIN(7), RZN1_PIN(8), RZN1_PIN(9),
+	RZN1_PIN(10), RZN1_PIN(11), RZN1_PIN(12), RZN1_PIN(13), RZN1_PIN(14),
+	RZN1_PIN(15), RZN1_PIN(16), RZN1_PIN(17), RZN1_PIN(18), RZN1_PIN(19),
+	RZN1_PIN(20), RZN1_PIN(21), RZN1_PIN(22), RZN1_PIN(23), RZN1_PIN(24),
+	RZN1_PIN(25), RZN1_PIN(26), RZN1_PIN(27), RZN1_PIN(28), RZN1_PIN(29),
+	RZN1_PIN(30), RZN1_PIN(31), RZN1_PIN(32), RZN1_PIN(33), RZN1_PIN(34),
+	RZN1_PIN(35), RZN1_PIN(36), RZN1_PIN(37), RZN1_PIN(38), RZN1_PIN(39),
+	RZN1_PIN(40), RZN1_PIN(41), RZN1_PIN(42), RZN1_PIN(43), RZN1_PIN(44),
+	RZN1_PIN(45), RZN1_PIN(46), RZN1_PIN(47), RZN1_PIN(48), RZN1_PIN(49),
+	RZN1_PIN(50), RZN1_PIN(51), RZN1_PIN(52), RZN1_PIN(53), RZN1_PIN(54),
+	RZN1_PIN(55), RZN1_PIN(56), RZN1_PIN(57), RZN1_PIN(58), RZN1_PIN(59),
+	RZN1_PIN(60), RZN1_PIN(61), RZN1_PIN(62), RZN1_PIN(63), RZN1_PIN(64),
+	RZN1_PIN(65), RZN1_PIN(66), RZN1_PIN(67), RZN1_PIN(68), RZN1_PIN(69),
+	RZN1_PIN(70), RZN1_PIN(71), RZN1_PIN(72), RZN1_PIN(73), RZN1_PIN(74),
+	RZN1_PIN(75), RZN1_PIN(76), RZN1_PIN(77), RZN1_PIN(78), RZN1_PIN(79),
+	RZN1_PIN(80), RZN1_PIN(81), RZN1_PIN(82), RZN1_PIN(83), RZN1_PIN(84),
+	RZN1_PIN(85), RZN1_PIN(86), RZN1_PIN(87), RZN1_PIN(88), RZN1_PIN(89),
+	RZN1_PIN(90), RZN1_PIN(91), RZN1_PIN(92), RZN1_PIN(93), RZN1_PIN(94),
+	RZN1_PIN(95), RZN1_PIN(96), RZN1_PIN(97), RZN1_PIN(98), RZN1_PIN(99),
+	RZN1_PIN(100), RZN1_PIN(101), RZN1_PIN(102), RZN1_PIN(103),
+	RZN1_PIN(104), RZN1_PIN(105), RZN1_PIN(106), RZN1_PIN(107),
+	RZN1_PIN(108), RZN1_PIN(109), RZN1_PIN(110), RZN1_PIN(111),
+	RZN1_PIN(112), RZN1_PIN(113), RZN1_PIN(114), RZN1_PIN(115),
+	RZN1_PIN(116), RZN1_PIN(117), RZN1_PIN(118), RZN1_PIN(119),
+	RZN1_PIN(120), RZN1_PIN(121), RZN1_PIN(122), RZN1_PIN(123),
+	RZN1_PIN(124), RZN1_PIN(125), RZN1_PIN(126), RZN1_PIN(127),
+	RZN1_PIN(128), RZN1_PIN(129), RZN1_PIN(130), RZN1_PIN(131),
+	RZN1_PIN(132), RZN1_PIN(133), RZN1_PIN(134), RZN1_PIN(135),
+	RZN1_PIN(136), RZN1_PIN(137), RZN1_PIN(138), RZN1_PIN(139),
+	RZN1_PIN(140), RZN1_PIN(141), RZN1_PIN(142), RZN1_PIN(143),
+	RZN1_PIN(144), RZN1_PIN(145), RZN1_PIN(146), RZN1_PIN(147),
+	RZN1_PIN(148), RZN1_PIN(149), RZN1_PIN(150), RZN1_PIN(151),
+	RZN1_PIN(152), RZN1_PIN(153), RZN1_PIN(154), RZN1_PIN(155),
+	RZN1_PIN(156), RZN1_PIN(157), RZN1_PIN(158), RZN1_PIN(159),
+	RZN1_PIN(160), RZN1_PIN(161), RZN1_PIN(162), RZN1_PIN(163),
+	RZN1_PIN(164), RZN1_PIN(165), RZN1_PIN(166), RZN1_PIN(167),
+	RZN1_PIN(168), RZN1_PIN(169),
+};
+
+enum {
+	LOCK_LEVEL1 = 0x1,
+	LOCK_LEVEL2 = 0x2,
+	LOCK_ALL = LOCK_LEVEL1 | LOCK_LEVEL2,
+};
+
+static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
+{
+	/*
+	 * The pinmux configuration is locked by writing the physical address of
+	 * the status_protect register to itself. It is unlocked by writing the
+	 * address | 1.
+	 */
+	if (lock & LOCK_LEVEL1) {
+		u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
+
+		writel(val, &ipctl->lev1->status_protect);
+	}
+
+	if (lock & LOCK_LEVEL2) {
+		u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2);
+
+		writel(val, &ipctl->lev2->status_protect);
+	}
+}
+
+static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, u8 mdio,
+				     u32 func)
+{
+	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
+		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
+	ipctl->mdio_func[mdio] = func;
+
+	dev_dbg(ipctl->dev, "setting mdio%d to %d\n", mdio, func);
+
+	writel(func, &ipctl->lev2->l2_mdio[mdio]);
+}
+
+/*
+ * Using a composite pin description, set the hardware pinmux registers
+ * with the corresponding values.
+ * Make sure to unlock write protection and reset it afterward.
+ *
+ * NOTE: There is no protection for potential concurrency, it is assumed these
+ * calls are serialized already.
+ */
+static int rzn1_set_hw_pin_func(struct rzn1_pinctrl *ipctl, u32 pin,
+				u32 pin_config, u8 use_locks)
+{
+	u32 l1_cache;
+	u32 l2_cache;
+	u32 l1;
+	u32 l2;
+
+	/* Level 3 MDIO multiplexing */
+	if (pin_config >= RZN1_FUNC_MDIO0_HIGHZ &&
+	    pin_config <= RZN1_FUNC_MDIO1_E1_SWITCH) {
+		u8 mdio_channel;
+		u32 mdio_func;
+
+		if (pin_config <= RZN1_FUNC_MDIO1_HIGHZ)
+			mdio_channel = 0;
+		else
+			mdio_channel = 1;
+
+		/* Get MDIO func, and convert the func to the level 2 number */
+		if (pin_config <= RZN1_FUNC_MDIO0_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO0_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO;
+		} else if (pin_config <= RZN1_FUNC_MDIO0_E1_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO0_E1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO_E1;
+		} else if (pin_config <= RZN1_FUNC_MDIO1_SWITCH) {
+			mdio_func = pin_config - RZN1_FUNC_MDIO1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO;
+		} else {
+			mdio_func = pin_config - RZN1_FUNC_MDIO1_E1_HIGHZ;
+			pin_config = RZN1_FUNC_ETH_MDIO_E1;
+		}
+		rzn1_pinctrl_mdio_select(ipctl, mdio_channel, mdio_func);
+	}
+
+	/* Note here, we do not allow anything past the MDIO Mux values */
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf) ||
+	    pin_config >= RZN1_FUNC_MDIO0_HIGHZ)
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+	l1_cache = l1;
+	l2 = readl(&ipctl->lev2->conf[pin]);
+	l2_cache = l2;
+
+	dev_dbg(ipctl->dev, "setting func for pin %d to %d\n", pin, pin_config);
+
+	if (pin_config < RZN1_FUNC_L2_OFFSET) {
+		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
+		l1 |= (pin_config << RZN1_L1_FUNCTION);
+	} else {
+		l1 &= ~(RZN1_L1_FUNC_MASK << RZN1_L1_FUNCTION);
+		l1 |= (RZN1_L1_FUNCTION_L2 << RZN1_L1_FUNCTION);
+
+		l2 = pin_config - RZN1_FUNC_L2_OFFSET;
+	}
+
+	/* If either configuration changes, we update both anyway */
+	if (l1 != l1_cache || l2 != l2_cache) {
+		writel(l1, &ipctl->lev1->conf[pin]);
+		writel(l2, &ipctl->lev2->conf[pin]);
+	}
+
+	return 0;
+}
+
+static const struct rzn1_pin_group *rzn1_pinctrl_find_group_by_name(
+	const struct rzn1_pinctrl *ipctl, const char *name)
+{
+	const struct rzn1_pin_group *grp = NULL;
+	int i;
+
+	for (i = 0; i < ipctl->ngroups; i++) {
+		if (!strcmp(ipctl->groups[i].name, name)) {
+			grp = &ipctl->groups[i];
+			break;
+		}
+	}
+
+	return grp;
+}
+
+static int rzn1_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->ngroups;
+}
+
+static const char *rzn1_get_group_name(struct pinctrl_dev *pctldev,
+				       unsigned int selector)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->groups[selector].name;
+}
+
+static int rzn1_get_group_pins(struct pinctrl_dev *pctldev,
+			       unsigned int selector, const unsigned int **pins,
+			       unsigned int *npins)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (selector >= ipctl->ngroups)
+		return -EINVAL;
+
+	*pins = ipctl->groups[selector].pins;
+	*npins = ipctl->groups[selector].npins;
+
+	return 0;
+}
+
+/*
+ * This function is called for each pinctl 'Function' node.
+ * Sub-nodes can be used to describe multiple 'Groups' for the 'Function'
+ * If there aren't any sub-nodes, the 'Group' is essentially the 'Function'.
+ * Each 'Group' uses pinmux = <...> to detail the pins and data used to select
+ * the functionality. Each 'Group' has optional pin configurations that apply
+ * to all pins in the 'Group'.
+ */
+static int rzn1_dt_node_to_map_one(struct pinctrl_dev *pctldev,
+				   struct device_node *np,
+				   struct pinctrl_map **map,
+				   unsigned int *num_maps)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct rzn1_pin_group *grp;
+	unsigned long *configs = NULL;
+	unsigned int num_configs = 0;
+	unsigned int reserved_maps = *num_maps;
+	unsigned int reserve = 1;
+	int ret;
+
+	dev_dbg(ipctl->dev, "processing node %pOF\n", np);
+
+	grp = rzn1_pinctrl_find_group_by_name(ipctl, np->name);
+	if (!grp) {
+		dev_err(ipctl->dev, "unable to find group for node %pOF\n", np);
+		return -EINVAL;
+	}
+
+	/* Get the group's pin configuration */
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
+					      &num_configs);
+	if (ret < 0) {
+		dev_err(ipctl->dev, "%s: could not parse node property\n",
+			of_node_full_name(np));
+		return ret;
+	}
+
+	if (num_configs)
+		reserve++;
+
+	/* Increase the number of maps to cover this group */
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
+					reserve);
+	if (ret < 0)
+		goto out;
+
+	/* Associate the group with the function */
+	ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps, num_maps,
+					grp->name, grp->func);
+	if (ret < 0)
+		goto out;
+
+	if (num_configs) {
+		/* Associate the group's pin configuration with the group */
+		ret = pinctrl_utils_add_map_configs(pctldev, map,
+				&reserved_maps, num_maps, grp->name,
+				configs, num_configs,
+				PIN_MAP_TYPE_CONFIGS_GROUP);
+		if (ret < 0)
+			goto out;
+	}
+
+	dev_dbg(pctldev->dev, "maps: function %s group %s (%d pins)\n",
+		grp->func, grp->name, grp->npins);
+
+out:
+	kfree(configs);
+	return 0;
+}
+
+static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct device_node *child;
+	int ret;
+
+	*map = NULL;
+	*num_maps = 0;
+
+	ret = rzn1_dt_node_to_map_one(pctldev, np, map, num_maps);
+	if (ret < 0)
+		return ret;
+
+	for_each_child_of_node(np, child) {
+		ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinctrl_ops rzn1_pctrl_ops = {
+	.get_groups_count = rzn1_get_groups_count,
+	.get_group_name = rzn1_get_group_name,
+	.get_group_pins = rzn1_get_group_pins,
+	.dt_node_to_map = rzn1_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int rzn1_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->nfunctions;
+}
+
+static const char *rzn1_pmx_get_func_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return ipctl->functions[selector].name;
+}
+
+static int rzn1_pmx_get_groups(struct pinctrl_dev *pctldev,
+			       unsigned int selector,
+			       const char * const **groups,
+			       unsigned int * const num_groups)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = ipctl->functions[selector].groups;
+	*num_groups = ipctl->functions[selector].num_groups;
+
+	return 0;
+}
+
+static int rzn1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			unsigned int group)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rzn1_pin_group *grp = &ipctl->groups[group];
+	unsigned int i, grp_pins = grp->npins;
+
+	dev_dbg(ipctl->dev, "set mux %s(%d) group %s(%d)\n",
+		ipctl->functions[selector].name, selector, grp->name, group);
+
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, LOCK_ALL);
+	for (i = 0; i < grp_pins; i++)
+		rzn1_set_hw_pin_func(ipctl, grp->pins[i], grp->pin_ids[i], 0);
+	rzn1_hw_set_lock(ipctl, LOCK_ALL, 0);
+
+	return 0;
+}
+
+static const struct pinmux_ops rzn1_pmx_ops = {
+	.get_functions_count = rzn1_pmx_get_funcs_count,
+	.get_function_name = rzn1_pmx_get_func_name,
+	.get_function_groups = rzn1_pmx_get_groups,
+	.set_mux = rzn1_set_mux,
+};
+
+static int rzn1_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	const u32 reg_drive[4] = { 4, 6, 8, 12 };
+	u32 l1, l2;
+	u32 pull, drive, l1mux;
+	u32 arg = 0;
+
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+
+	l1mux = l1 & RZN1_L1_FUNC_MASK;
+	pull = (l1 >> RZN1_L1_PIN_PULL) & 0x3;
+	drive = (l1 >> RZN1_L1_PIN_DRIVE_STRENGTH) & 0x3;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pull == RZN1_L1_PIN_PULL_UP)
+			arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (pull == RZN1_L1_PIN_PULL_DOWN)
+			arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pull == RZN1_L1_PIN_PULL_NONE)
+			arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		arg = reg_drive[drive];
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		l2 = readl(&ipctl->lev1->conf[pin]);
+		if (l1mux == RZN1_FUNC_HIGHZ)
+			arg = 1;
+		if (l1mux == RZN1_L1_FUNCTION_L2 && l2 == 0)
+			arg = 1;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int rzn1_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	int i;
+	u32 arg;
+	u32 l1, l1_cache;
+	u32 drv;
+
+	if (pin >= ARRAY_SIZE(ipctl->lev1->conf))
+		return -EINVAL;
+
+	l1 = readl(&ipctl->lev1->conf[pin]);
+	l1_cache = l1;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_PULL_UP:
+			dev_dbg(ipctl->dev, "set pin %d pull up\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_UP << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			dev_dbg(ipctl->dev, "set pin %d pull down\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_DOWN << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_BIAS_DISABLE:
+			dev_dbg(ipctl->dev, "set pin %d bias off\n", pin);
+			l1 &= ~(0x3 << RZN1_L1_PIN_PULL);
+			l1 |= (RZN1_L1_PIN_PULL_NONE << RZN1_L1_PIN_PULL);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			dev_dbg(ipctl->dev, "set pin %d drv %umA\n", pin, arg);
+			drv = 0;
+			switch (arg) {
+			case 4:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_4MA;
+				break;
+			case 6:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_6MA;
+				break;
+			case 8:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_8MA;
+				break;
+			case 12:
+				drv = RZN1_L1_PIN_DRIVE_STRENGTH_12MA;
+				break;
+			default:
+				dev_err(ipctl->dev,
+					"Drive strength %umA not supported\n",
+					arg);
+				return -EINVAL;
+			}
+
+			l1 &= ~(0x3 << RZN1_L1_PIN_DRIVE_STRENGTH);
+			l1 |= (drv << RZN1_L1_PIN_DRIVE_STRENGTH);
+			break;
+
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			dev_dbg(ipctl->dev, "set pin %d High-Z\n", pin);
+			l1 &= ~RZN1_L1_FUNC_MASK;
+			l1 |= RZN1_FUNC_HIGHZ;
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+	}
+
+	if (l1 != l1_cache) {
+		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, LOCK_LEVEL1);
+		writel(l1, &ipctl->lev1->conf[pin]);
+		rzn1_hw_set_lock(ipctl, LOCK_LEVEL1, 0);
+	}
+
+	return 0;
+}
+
+static int rzn1_pin_config_group_set(struct pinctrl_dev *pctldev,
+				     unsigned int selector,
+				     unsigned long *configs,
+				     unsigned int num_configs)
+{
+	struct rzn1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rzn1_pin_group *grp = &ipctl->groups[selector];
+	int ret, i;
+
+	dev_dbg(ipctl->dev, "group set %s selector:%d configs:%p/%d\n",
+		grp->name, selector, configs, num_configs);
+
+	for (i = 0; i < grp->npins; i++) {
+		unsigned int pin = grp->pins[i];
+
+		ret = rzn1_pinconf_set(pctldev, pin, configs, num_configs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops rzn1_pinconf_ops = {
+	.pin_config_get = rzn1_pinconf_get,
+	.pin_config_set = rzn1_pinconf_set,
+	.pin_config_group_set = rzn1_pin_config_group_set,
+};
+
+static struct pinctrl_desc rzn1_pinctrl_desc = {
+	.pctlops = &rzn1_pctrl_ops,
+	.pmxops = &rzn1_pmx_ops,
+	.confops = &rzn1_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static int rzn1_pinctrl_parse_groups(struct device_node *np,
+				     struct rzn1_pin_group *grp,
+				     struct rzn1_pinctrl *ipctl)
+{
+	int size;
+	const __be32 *list;
+	int i;
+
+	dev_dbg(ipctl->dev, "%s: %s\n", __func__, np->name);
+
+	/* Initialise group */
+	grp->name = np->name;
+
+	/*
+	 * The binding format is
+	 *	pinmux = <PIN_FUNC_ID CONFIG ...>,
+	 * do sanity check and calculate pins number
+	 */
+	list = of_get_property(np, RZN1_PINS_PROP, &size);
+	if (!list) {
+		dev_err(ipctl->dev,
+			"no " RZN1_PINS_PROP " property in node %s\n",
+			np->full_name);
+		return -EINVAL;
+	}
+
+	/* We do not check return since it's safe node passed down */
+	if (!size) {
+		dev_err(ipctl->dev, "Invalid " RZN1_PINS_PROP " in node %s\n",
+			np->full_name);
+		return -EINVAL;
+	}
+
+	grp->npins = size / sizeof(list[0]);
+	if (!grp->npins)
+		return 0;
+
+	grp->pin_ids = devm_kmalloc_array(ipctl->dev,
+					  grp->npins, sizeof(grp->pin_ids[0]),
+					  GFP_KERNEL);
+	grp->pins = devm_kmalloc_array(ipctl->dev,
+				       grp->npins, sizeof(grp->pins[0]),
+				       GFP_KERNEL);
+	if (!grp->pin_ids || !grp->pins)
+		return -ENOMEM;
+
+	for (i = 0; i < grp->npins; i++) {
+		u32 pin_id = be32_to_cpu(*list++);
+
+		grp->pins[i] = pin_id & 0xff;
+		grp->pin_ids[i] = (pin_id >> 8) & 0x7f;
+	}
+
+	return grp->npins;
+}
+
+static int rzn1_pinctrl_count_function_groups(struct device_node *np)
+{
+	struct device_node *child;
+	int count = 0;
+
+	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
+		count++;
+
+	for_each_child_of_node(np, child) {
+		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
+			count++;
+	}
+
+	return count;
+}
+
+static int rzn1_pinctrl_parse_functions(struct device_node *np,
+					struct rzn1_pinctrl *ipctl, u32 index)
+{
+	struct device_node *child;
+	struct rzn1_pmx_func *func;
+	struct rzn1_pin_group *grp;
+	u32 i = 0;
+	int ret;
+
+	dev_dbg(ipctl->dev, "parse function(%d): %s\n", index, np->name);
+
+	func = &ipctl->functions[index];
+
+	/* Initialise function */
+	func->name = np->name;
+	func->num_groups = rzn1_pinctrl_count_function_groups(np);
+	dev_dbg(ipctl->dev, "function %s has %d groups\n",
+		np->name, func->num_groups);
+	if (func->num_groups == 0) {
+		dev_err(ipctl->dev, "no groups defined in %s\n", np->full_name);
+		return -EINVAL;
+	}
+
+	func->groups = devm_kmalloc_array(ipctl->dev,
+					  func->num_groups, sizeof(char *),
+					  GFP_KERNEL);
+	if (!func->groups)
+		return -ENOMEM;
+
+	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0) {
+		func->groups[i] = np->name;
+		grp = &ipctl->groups[ipctl->ngroups];
+		grp->func = func->name;
+		ret = rzn1_pinctrl_parse_groups(np, grp, ipctl);
+		if (ret < 0)
+			return ret;
+		if (ret > 0) {
+			i++;
+			ipctl->ngroups++;
+		}
+	}
+
+	for_each_child_of_node(np, child) {
+		func->groups[i] = child->name;
+		grp = &ipctl->groups[ipctl->ngroups];
+		grp->func = func->name;
+		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
+		if (ret < 0)
+			return ret;
+		if (ret > 0) {
+			i++;
+			ipctl->ngroups++;
+		}
+	}
+
+	dev_dbg(ipctl->dev, "function %s parsed %d/%d groups\n",
+		np->name, i, func->num_groups);
+
+	return 0;
+}
+
+static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
+				 struct rzn1_pinctrl *ipctl)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *child;
+	unsigned int maxgroups = 0;
+	u32 nfuncs = 0;
+	u32 i = 0;
+	int ret;
+
+	nfuncs = of_get_child_count(np);
+	if (nfuncs <= 0) {
+		dev_err(&pdev->dev, "no functions defined\n");
+		return -EINVAL;
+	}
+
+	ipctl->nfunctions = nfuncs;
+	ipctl->functions = devm_kmalloc_array(&pdev->dev, nfuncs,
+					      sizeof(*ipctl->functions),
+					      GFP_KERNEL);
+	if (!ipctl->functions)
+		return -ENOMEM;
+
+	ipctl->ngroups = 0;
+	for_each_child_of_node(np, child)
+		maxgroups += rzn1_pinctrl_count_function_groups(child);
+
+	ipctl->groups = devm_kmalloc_array(&pdev->dev,
+					   maxgroups,
+					   sizeof(*ipctl->groups),
+					   GFP_KERNEL);
+	if (!ipctl->groups)
+		return -ENOMEM;
+
+	for_each_child_of_node(np, child) {
+		ret = rzn1_pinctrl_parse_functions(child, ipctl, i++);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int rzn1_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rzn1_pinctrl *ipctl;
+	struct resource *res;
+	int ret;
+
+	/* Create state holders etc for this driver */
+	ipctl = devm_kzalloc(&pdev->dev, sizeof(*ipctl), GFP_KERNEL);
+	if (!ipctl)
+		return -ENOMEM;
+
+	ipctl->mdio_func[0] = -1;
+	ipctl->mdio_func[1] = -1;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ipctl->lev1_protect_phys = (u32)res->start + 0x400;
+	ipctl->lev1 = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ipctl->lev1))
+		return PTR_ERR(ipctl->lev1);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	ipctl->lev2_protect_phys = (u32)res->start + 0x400;
+	ipctl->lev2 = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ipctl->lev2))
+		return PTR_ERR(ipctl->lev2);
+
+	ipctl->clk = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(ipctl->clk))
+		return PTR_ERR(ipctl->clk);
+	ret = clk_prepare_enable(ipctl->clk);
+	if (ret)
+		return ret;
+
+	ipctl->dev = &pdev->dev;
+	rzn1_pinctrl_desc.name = dev_name(&pdev->dev);
+	rzn1_pinctrl_desc.pins = rzn1_pins;
+	rzn1_pinctrl_desc.npins = ARRAY_SIZE(rzn1_pins);
+
+	ret = rzn1_pinctrl_probe_dt(pdev, ipctl);
+	if (ret) {
+		dev_err(&pdev->dev, "fail to probe dt properties\n");
+		goto err_clk;
+	}
+
+	platform_set_drvdata(pdev, ipctl);
+	ipctl->pctl = pinctrl_register(&rzn1_pinctrl_desc, &pdev->dev, ipctl);
+	if (!ipctl->pctl) {
+		dev_err(&pdev->dev, "could not register rzn1 pinctrl driver\n");
+		ret = -EINVAL;
+		goto err_clk;
+	}
+
+	dev_info(&pdev->dev, "probed\n");
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(ipctl->clk);
+
+	return ret;
+}
+
+static int rzn1_pinctrl_remove(struct platform_device *pdev)
+{
+	struct rzn1_pinctrl *ipctl = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(ipctl->clk);
+
+	return 0;
+}
+
+static const struct of_device_id rzn1_pinctrl_match[] = {
+	{ .compatible = "renesas,rzn1-pinctrl", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rzn1_pinctrl_match);
+
+static struct platform_driver rzn1_pinctrl_driver = {
+	.probe	= rzn1_pinctrl_probe,
+	.remove = rzn1_pinctrl_remove,
+	.driver	= {
+		.name		= "rzn1-pinctrl",
+		.owner		= THIS_MODULE,
+		.of_match_table	= rzn1_pinctrl_match,
+	},
+};
+
+static int __init _pinctrl_drv_register(void)
+{
+	return platform_driver_register(&rzn1_pinctrl_driver);
+}
+subsys_initcall(_pinctrl_drv_register);
+
+MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/N1 pinctrl driver");
+MODULE_LICENSE("GPL v2");