Hi Andreas,
On Sat, Feb 17, 2018 at 10:41:48PM +0100, Andreas F?rber wrote:

Am 17.02.2018 um 21:44 schrieb Manivannan Sadhasivam:

quoted

Add pinctrl driver for Actions Semi S900 SoC. The driver supports
pinctrl, pinmux and pinconf functionalities through a range of registers
common to both gpio driver and pinctrl driver.

Pinmux functionality is available only for the pin groups while the
pinconf functionality is available for both pin groups and individual
pins.

Signed-off-by: Manivannan Sadhasivam <redacted>
---
 drivers/pinctrl/Kconfig                |    1 +
 drivers/pinctrl/Makefile               |    1 +
 drivers/pinctrl/actions/Kconfig        |   12 +
 drivers/pinctrl/actions/Makefile       |    2 +
 drivers/pinctrl/actions/pinctrl-owl.c  |  573 ++++++++
 drivers/pinctrl/actions/pinctrl-owl.h  |  178 +++
 drivers/pinctrl/actions/pinctrl-s900.c | 2536 ++++++++++++++++++++++++++++++++
 7 files changed, 3303 insertions(+)
 create mode 100644 drivers/pinctrl/actions/Kconfig
 create mode 100644 drivers/pinctrl/actions/Makefile
 create mode 100644 drivers/pinctrl/actions/pinctrl-owl.c
 create mode 100644 drivers/pinctrl/actions/pinctrl-owl.h
 create mode 100644 drivers/pinctrl/actions/pinctrl-s900.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0f254b35c378..838c8fff8c24 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig

@@ -368,6 +368,7 @@ config PINCTRL_OCELOT
 	select GENERIC_PINMUX_FUNCTIONS
 	select REGMAP_MMIO
 
+source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
 source "drivers/pinctrl/berlin/Kconfig"

diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d3692633e9ed..bf41d2484a44 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile

@@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
 
+obj-$(CONFIG_ARCH_ACTIONS)	+= actions/

Here you require CONFIG_ARCH_ACTIONS ...

How about making this as obj-y and having separate
CONFIG_PINCTRL_OWL and CONFIG_PINCTRL_S900 options in platform
makefile?

quoted

 obj-$(CONFIG_ARCH_ASPEED)	+= aspeed/
 obj-y				+= bcm/
 obj-$(CONFIG_PINCTRL_BERLIN)	+= berlin/

diff --git a/drivers/pinctrl/actions/Kconfig b/drivers/pinctrl/actions/Kconfig
new file mode 100644
index 000000000000..6075909d04e9
--- /dev/null
+++ b/drivers/pinctrl/actions/Kconfig

@@ -0,0 +1,12 @@
+config PINCTRL_OWL
+        bool

Indentation.

Ack.

quoted

+	depends on (ARCH_ACTIONS || COMPILE_TEST) && OF

... so || COMPILE_TEST becomes moot.

Use CONFIG_PINCTRL_OWL in Makefile instead?

Yes, as said above.

quoted

+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+
+config PINCTRL_S900
+	bool "Actions Semi S900 pinctrl driver"
+	select PINCTRL_OWL
+	help
+	  Say Y here to enable Actions S900 pinctrl driver

Repeating the shortened name does not seem too helpful?

Okay. Will make Actions Semi S900 everywhere.

quoted

diff --git a/drivers/pinctrl/actions/Makefile b/drivers/pinctrl/actions/Makefile
new file mode 100644
index 000000000000..bd232d28400f
--- /dev/null
+++ b/drivers/pinctrl/actions/Makefile

@@ -0,0 +1,2 @@
+obj-$(CONFIG_PINCTRL_OWL)	+= pinctrl-owl.o
+obj-$(CONFIG_PINCTRL_S900) 	+= pinctrl-s900.o

diff --git a/drivers/pinctrl/actions/pinctrl-owl.c b/drivers/pinctrl/actions/pinctrl-owl.c
new file mode 100644
index 000000000000..7ed8625c30f3
--- /dev/null
+++ b/drivers/pinctrl/actions/pinctrl-owl.c

@@ -0,0 +1,573 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * OWL SoC's Pinctrl driver
+ *
+ * Copyright (c) 2014 Actions Semi Inc.
+ * Author: David Liu <liuwei@actions-semi.com>
+ *
+ * Copyright (c) 2018 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "../core.h"
+#include "../pinctrl-utils.h"
+#include "pinctrl-owl.h"
+
+/**
+ * struct owl_pinctrl - pinctrl state of the device
+ * @dev: device handle
+ * @pctrldev: pinctrl handle
+ * @lock: spinlock to protect registers
+ * @soc: reference to soc_data
+ * @base: pinctrl register base address
+ */
+struct owl_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctrldev;
+	raw_spinlock_t lock;
+	struct clk *clk;
+	const struct owl_pinctrl_soc_data *soc;
+	void __iomem *base;
+};
+
+static int owl_get_groups_count(struct pinctrl_dev *pctrldev)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	return pctrl->soc->ngroups;
+}
+
+static const char *owl_get_group_name(struct pinctrl_dev *pctrldev,
+				unsigned int group)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	return pctrl->soc->groups[group].name;
+}
+
+static int owl_get_group_pins(struct pinctrl_dev *pctrldev,
+				unsigned int group,
+				const unsigned int **pins,
+				unsigned int *num_pins)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	*pins = pctrl->soc->groups[group].pads;
+	*num_pins = pctrl->soc->groups[group].npads;
+
+	return 0;
+}
+
+static void owl_pin_dbg_show(struct pinctrl_dev *pctrldev,
+				struct seq_file *s,
+				unsigned int offset)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	seq_printf(s, "%s", dev_name(pctrl->dev));
+}
+
+static struct pinctrl_ops owl_pinctrl_ops = {
+	.get_groups_count = owl_get_groups_count,
+	.get_group_name = owl_get_group_name,
+	.get_group_pins = owl_get_group_pins,
+	.pin_dbg_show = owl_pin_dbg_show,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int owl_get_funcs_count(struct pinctrl_dev *pctrldev)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	return pctrl->soc->nfunctions;
+}
+
+static const char *owl_get_func_name(struct pinctrl_dev *pctrldev,
+				unsigned int function)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	return pctrl->soc->functions[function].name;
+}
+
+static int owl_get_func_groups(struct pinctrl_dev *pctrldev,
+				unsigned int function,
+				const char * const **groups,
+				unsigned int * const num_groups)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+
+	*groups = pctrl->soc->functions[function].groups;
+	*num_groups = pctrl->soc->functions[function].ngroups;
+
+	return 0;
+}
+
+static inline int get_group_mfp_mask_val(const struct owl_pingroup *g,
+				int function,
+				u32 *mask,
+				u32 *val)
+{
+	int id;
+	u32 option_num;
+	u32 option_mask;
+
+	for (id = 0; id < g->nfuncs; id++) {
+		if (g->funcs[id] == function)
+			break;
+	}
+	if (WARN_ON(id == g->nfuncs))
+		return -EINVAL;
+
+	option_num = (1 << g->mfpctl_width);
+	if (id > option_num)
+		id -= option_num;
+
+	option_mask = option_num - 1;
+	*mask = (option_mask  << g->mfpctl_shift);
+	*val = (id << g->mfpctl_shift);
+
+	return 0;
+}
+
+static int owl_set_mux(struct pinctrl_dev *pctrldev,
+				unsigned int function,
+				unsigned int group)
+{
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+	const struct owl_pingroup *g;
+	unsigned long flags;
+	u32 val, mask, mfpval;
+
+	g = &pctrl->soc->groups[group];
+
+	if (get_group_mfp_mask_val(g, function, &mask, &val))
+		return -EINVAL;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+	mfpval = readl(pctrl->base + g->mfpctl_reg);
+	mfpval &= ~mask;
+	mfpval |= val;
+	writel(mfpval, pctrl->base + g->mfpctl_reg);
+
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
+static struct pinmux_ops owl_pinmux_ops = {
+	.get_functions_count = owl_get_funcs_count,
+	.get_function_name = owl_get_func_name,
+	.get_function_groups = owl_get_func_groups,
+	.set_mux = owl_set_mux,
+};
+
+static int owl_pad_pinconf_reg(const struct owl_padinfo *info,
+				unsigned int param,
+				u32 *reg,
+				u32 *bit,
+				u32 *width)
+{
+	switch (param) {
+	case PIN_CONFIG_BIAS_BUS_HOLD:
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (!info->pullctl)
+			return -EINVAL;
+		*reg = info->pullctl->reg;
+		*bit = info->pullctl->shift;
+		*width = info->pullctl->width;
+		break;
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		if (!info->st)
+			return -EINVAL;
+		*reg = info->st->reg;
+		*bit = info->st->shift;
+		*width = info->st->width;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_pad_pinconf_arg2val(const struct owl_padinfo *info,
+				unsigned int param,
+				u32 *arg)
+{
+	switch (param) {
+	case PIN_CONFIG_BIAS_BUS_HOLD:
+		*arg = OWL_PINCONF_PULL_HOLD;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		*arg = OWL_PINCONF_PULL_HIZ;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		*arg = OWL_PINCONF_PULL_DOWN;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		*arg = OWL_PINCONF_PULL_UP;
+		break;
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		*arg = (*arg >= 1 ? 1 : 0);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_pad_pinconf_val2arg(const struct owl_padinfo *padinfo,
+				unsigned int param,
+				u32 *arg)
+{
+	switch (param) {
+	case PIN_CONFIG_BIAS_BUS_HOLD:
+		*arg = *arg == OWL_PINCONF_PULL_HOLD;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		*arg = *arg == OWL_PINCONF_PULL_HIZ;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		*arg = *arg == OWL_PINCONF_PULL_DOWN;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		*arg = *arg == OWL_PINCONF_PULL_UP;
+		break;
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		*arg = *arg == 1;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_pin_config_get(struct pinctrl_dev *pctrldev,
+				unsigned int pin,
+				unsigned long *config)
+{
+	int ret = 0;
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+	const struct owl_padinfo *info;
+	unsigned int param = pinconf_to_config_param(*config);
+	u32 reg, bit, width;
+	u32 tmp, mask;
+	u32 arg = 0;
+
+	info = &pctrl->soc->padinfo[pin];
+
+	ret = owl_pad_pinconf_reg(info, param, &reg, &bit, &width);
+	if (ret)
+		return ret;
+
+	tmp = readl(pctrl->base + reg);
+	mask = (1 << width) - 1;
+	arg = (tmp >> bit) & mask;
+
+	ret = owl_pad_pinconf_val2arg(info, param, &arg);
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return ret;
+}
+
+static int owl_pin_config_set(struct pinctrl_dev *pctrldev,
+				unsigned int pin,
+				unsigned long *configs,
+				unsigned int num_configs)
+{
+	int ret = 0;
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+	const struct owl_padinfo *info;
+	unsigned long flags;
+	unsigned int param;
+	u32 reg, bit, width;
+	u32 mask = 0, tmp, arg = 0;
+	int i;
+
+	info = &pctrl->soc->padinfo[pin];
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		ret = owl_pad_pinconf_reg(info, param, &reg, &bit, &width);
+		if (ret)
+			return ret;
+
+		ret = owl_pad_pinconf_arg2val(info, param, &arg);
+		if (ret)
+			return ret;
+
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+		mask = (1 << width) - 1;
+		mask = mask << bit;
+		tmp = readl(pctrl->base + reg);
+		tmp &= ~mask;
+		tmp |= arg << bit;
+		writel(tmp, pctrl->base + reg);
+
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return ret;
+}
+
+static int owl_group_pinconf_reg(const struct owl_pingroup *g,
+				unsigned int param,
+				u32 *reg,
+				u32 *bit,
+				u32 *width)
+{
+	switch (param) {
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		if (g->drv_reg < 0)
+			return -EINVAL;
+		*reg = g->drv_reg;
+		*bit = g->drv_shift;
+		*width = g->drv_width;
+		break;
+	case PIN_CONFIG_SLEW_RATE:
+		if (g->sr_reg < 0)
+			return -EINVAL;
+		*reg = g->sr_reg;
+		*bit = g->sr_shift;
+		*width = g->sr_width;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_group_pinconf_arg2val(const struct owl_pingroup *g,
+				unsigned int param,
+				u32 *arg)
+{
+	switch (param) {
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		switch (*arg) {
+		case 2:
+			*arg = OWL_PINCONF_DRV_2MA;
+			break;
+		case 4:
+			*arg = OWL_PINCONF_DRV_4MA;
+			break;
+		case 8:
+			*arg = OWL_PINCONF_DRV_8MA;
+			break;
+		case 12:
+			*arg = OWL_PINCONF_DRV_12MA;
+			break;
+		default:
+			return -EINVAL;
+		}
+	case PIN_CONFIG_SLEW_RATE:
+		if (*arg)
+			*arg = OWL_PINCONF_SLEW_FAST;
+		else
+			*arg = OWL_PINCONF_SLEW_SLOW;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_group_pinconf_val2arg(const struct owl_pingroup *g,
+				unsigned int param,
+				u32 *arg)
+{
+	switch (param) {
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		switch (*arg) {
+		case OWL_PINCONF_DRV_2MA:
+			*arg = 2;
+			break;
+		case OWL_PINCONF_DRV_4MA:
+			*arg = 4;
+			break;
+		case OWL_PINCONF_DRV_8MA:
+			*arg = 8;
+			break;
+		case OWL_PINCONF_DRV_12MA:
+			*arg = 12;
+			break;
+		default:
+			return -EINVAL;
+		}
+	case PIN_CONFIG_SLEW_RATE:
+		if (*arg)
+			*arg = 1;
+		else
+			*arg = 0;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int owl_group_config_get(struct pinctrl_dev *pctrldev,
+				unsigned int group,
+				unsigned long *config)
+{
+	int ret = 0;
+	const struct owl_pingroup *g;
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+	unsigned int param = pinconf_to_config_param(*config);
+	u32 reg, bit, width;
+	u32 mask, tmp, arg = 0;
+
+	g = &pctrl->soc->groups[group];
+
+	ret = owl_group_pinconf_reg(g, param, &reg, &bit, &width);
+	if (ret)
+		return ret;
+
+	tmp = readl(pctrl->base + reg);
+	mask = (1 << width) - 1;
+	arg = (tmp >> bit) & mask;
+
+	ret = owl_group_pinconf_val2arg(g, param, &arg);
+	if (ret)
+		return ret;
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return ret;
+
+}
+
+static int owl_group_config_set(struct pinctrl_dev *pctrldev,
+				unsigned int group,
+				unsigned long *configs,
+				unsigned int num_configs)
+{
+	int ret = 0;
+	const struct owl_pingroup *g;
+	struct owl_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrldev);
+	unsigned long flags;
+	unsigned int param;
+	u32 reg, bit, width;
+	u32 mask, arg = 0;
+	u32 tmp;
+	int i;
+
+	g = &pctrl->soc->groups[group];
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		ret = owl_group_pinconf_reg(g, param, &reg, &bit, &width);
+		if (ret)
+			return ret;
+
+		ret = owl_group_pinconf_arg2val(g, param, &arg);
+		if (ret)
+			return ret;
+
+		/* Update register */
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+
+		mask = (1 << width) - 1;
+		mask = mask << bit;
+		tmp = readl(pctrl->base + reg);
+		tmp &= ~mask;
+		tmp |= arg << bit;
+		writel(tmp, pctrl->base + reg);
+
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return ret;
+}
+
+static const struct pinconf_ops owl_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = owl_pin_config_get,
+	.pin_config_set = owl_pin_config_set,
+	.pin_config_group_get = owl_group_config_get,
+	.pin_config_group_set = owl_group_config_set,
+};
+
+static struct pinctrl_desc owl_pinctrl_desc = {
+	.pctlops = &owl_pinctrl_ops,
+	.pmxops = &owl_pinmux_ops,
+	.confops = &owl_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+int owl_pinctrl_probe(struct platform_device *pdev,
+				struct owl_pinctrl_soc_data *soc_data)
+{
+	struct resource *res;
+	struct owl_pinctrl *pctrl;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pctrl->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pctrl->base))
+		return PTR_ERR(pctrl->base);
+
+	/* enable GPIO/MFP clock */
+	pctrl->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pctrl->clk)) {
+		dev_err(&pdev->dev, "no clock defined\n");
+		return -ENODEV;

You need to return PTR_ERR(pctrl->clk) to make deferred loading work.

Ack. 

quoted

+	}
+	clk_prepare_enable(pctrl->clk);
+
+	raw_spin_lock_init(&pctrl->lock);
+
+	owl_pinctrl_desc.name = dev_name(&pdev->dev);
+	owl_pinctrl_desc.pins = soc_data->pins;
+	owl_pinctrl_desc.npins = soc_data->npins;
+
+	pctrl->soc = soc_data;
+	pctrl->dev = &pdev->dev;
+
+	pctrl->pctrldev = devm_pinctrl_register(&pdev->dev,
+					&owl_pinctrl_desc, pctrl);
+	if (IS_ERR(pctrl->pctrldev)) {
+		dev_err(&pdev->dev, "could not register Actions OWL pinmux driver\n");
+		return PTR_ERR(pctrl->pctrldev);
+	}
+
+	platform_set_drvdata(pdev, pctrl);
+
+	pr_info("Initialized Actions OWL pin control driver\n");

Drop?

Ack.

quoted

+
+	return 0;
+}

[snip]

Let's discuss concerns about the naming and design separately.

So, as per our discussion I'll be making all the pin names as lowercase
and the register name based pin groups will be changed to handle pinmuxing
using individual pins.

Thanks,
Mani

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)