[v1,1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC

Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP
which controls pin multiplexing & configuration including GPIO functions
selection & GPIO attributes configuration. Add GPIO & pin control framework
based driver for this IP.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 drivers/pinctrl/Kconfig               |   13 +
 drivers/pinctrl/Makefile              |    1 +
 drivers/pinctrl/pinctrl-equilibrium.c | 1377 +++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-equilibrium.h |  194 +++++
 4 files changed, 1585 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-equilibrium.c
 create mode 100644 drivers/pinctrl/pinctrl-equilibrium.h

On Thu, Sep 12, 2019 at 03:59:10PM +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP
> which controls pin multiplexing & configuration including GPIO functions
> selection & GPIO attributes configuration. Add GPIO & pin control framework
> based driver for this IP.

Can you put few more words to explain how new this IP, and why it requires a
complete separate driver?

> +#define PIN_NAME_FMT	"io-%d"
> +#define PIN_NAME_LEN	10
> +#define PAD_REG_OFF	0x100
> +
> +static const struct pin_config pin_cfg_type[] = {
> +	{"intel,pullup",		PINCONF_TYPE_PULL_UP},
> +	{"intel,pulldown",		PINCONF_TYPE_PULL_DOWN},
> +	{"intel,drive-current",		PINCONF_TYPE_DRIVE_CURRENT},
> +	{"intel,slew-rate",		PINCONF_TYPE_SLEW_RATE},
> +	{"intel,open-drain",		PINCONF_TYPE_OPEN_DRAIN},
> +	{"intel,output",		PINCONF_TYPE_OUTPUT},
> +};

Doesn't DT provide a generic naming scheme for these?

> +	val = readl(addr) & ~(mask << offset);
> +	writel(val | ((set & mask) << offset), addr);

More traditional pattern is

	val = readl(...);
	val = (val & ~mask) | (newval & mask);
	writel(val, ...);

> +	virq = irq_find_mapping(desc->irq_domain, offset);
> +	if (virq)
> +		return virq;

> +	else

Redundant.

> +		return irq_create_mapping(desc->irq_domain, offset);

Don't we have more clever helper for this? AFAIR something like this is done in
IRQ framework when you get a mapping from certain domain.


> +	gc->base		= -1; /* desc->bank->pin_base; */

Useless comment.

> +	ret = devm_gpiochip_add_data(dev, gc, desc);
> +	if (ret)
> +		dev_err(dev, "failed to register gpiochip: %s, err: %d\n",
> +			gc->label, ret);
> +
> +	return ret;

Simple
	return devm_gpiochip_add_data(...);
would be enough.

> +		writel(readl(addr) & (~BIT(offset)), addr);

Too many parentheses.

> +static int eqbr_gpio_irq_req_res(struct irq_data *d)
> +{
> +	struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
> +	unsigned int offset;
> +	int ret;
> +
> +	offset = irqd_to_hwirq(d);
> +
> +	/* gpio must be set as input */
> +	intel_eqbr_gpio_dir_input(&desc->chip, offset);
> +	ret = gpiochip_lock_as_irq(&desc->chip, offset);
> +	if (ret) {
> +		pr_err("%s: Failed to lock gpio %u as irq!\n",
> +		       desc->name, offset);
> +		return ret;
> +	}

Above is pretty much generic one...

> +	eqbr_gpio_enable_irq(d);

...and this may go to unmask() / enable() callback.

No?

> +	return 0;
> +}

> +static void eqbr_gpio_irq_rel_res(struct irq_data *d)
> +{
> +	struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
> +	unsigned int offset = irqd_to_hwirq(d);
> +
> +	eqbr_gpio_disable_irq(d);
> +	gpiochip_unlock_as_irq(&desc->chip, offset);
> +}

Ditto.

> +static void eqbr_irq_handler(struct irq_desc *desc)
> +{
> +	struct intel_gpio_desc *gc;
> +	struct irq_chip *ic;
> +	u32 pins, offset;
> +	unsigned int virq;
> +
> +	gc = irq_desc_get_handler_data(desc);
> +	ic = irq_desc_get_chip(desc);
> +
> +	chained_irq_enter(ic, desc);
> +	pins = readl(gc->membase + GPIO_IRNCR);
> +
> +	for_each_set_bit(offset, (unsigned long *)&pins, gc->bank->nr_pins) {
> +		virq = irq_linear_revmap(gc->irq_domain, offset);
> +		if (!virq)

> +			pr_err("gc[%s]:pin:%d irq not registered!\n",
> +			       gc->name, offset);

dev_err() ? But Why is it needed? Shouldn't be registered as a spurious IRQ for
later debugging?

> +		else
> +			generic_handle_irq(virq);
> +	}
> +	chained_irq_exit(ic, desc);
> +}

> +static int gpiolib_reg(struct intel_pinctrl_drv_data *drvdata)
> +{
> +	struct device_node *np;
> +	struct intel_gpio_desc *desc;
> +	struct device *dev;
> +	int i, ret;
> +	char name[32];
> +	struct resource res;
> +
> +	dev = drvdata->dev;
> +	for (i = 0; i < drvdata->nr_gpio_descs; i++) {
> +		desc = drvdata->gpio_desc + i;
> +		np = desc->node;

> +		sprintf(name, "gpiochip%d", i);
> +		desc->name = devm_kmemdup(dev, name,
> +					  strlen(name) + 1, GFP_KERNEL);

Have you run coccinelle scripts against this code?
Above is copy'n'paste of devm_kasprintf().

> +		if (!desc->name)
> +			return -ENOMEM;

> +		if (of_address_to_resource(np, 0, &res)) {
> +			dev_err(dev, "Failed to get GPIO register addrss\n");
> +			return -ENXIO;
> +		}
> +		desc->membase = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(desc->membase)) {
> +			dev_err(dev, "ioremap fail\n");
> +			return PTR_ERR(desc->membase);
> +		}

Can't you simple use devm_platform_ioremap_resource()?

> +		dev_dbg(dev, "gpio resource: %pr\n", &res);
> +		dev_dbg(dev, "gpiochip membase: %px\n", desc->membase);

Is it anyhow useful?

> +		desc->virq = irq_of_parse_and_map(np, 0);
> +		if (!desc->virq) {
> +			dev_err(dev, "%s: failed to parse and map irq\n",
> +				name);
> +			return -ENXIO;
> +		}
> +		raw_spin_lock_init(&desc->lock);
> +
> +		ret = gpiochip_setup(dev, desc);
> +		if (ret)
> +			return ret;
> +		ret = irqchip_setup(dev, desc);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

> +static int add_config(struct intel_pinctrl_drv_data *drvdata,
> +		      unsigned long **confs, unsigned int *nr_conf,
> +		      unsigned long pinconf)
> +{
> +	unsigned long *configs;
> +	struct device *dev = drvdata->dev;
> +	unsigned int num_conf = *nr_conf + 1;
> +
> +	if (!(*nr_conf)) {
> +		configs = devm_kcalloc(dev, 1, sizeof(pinconf), GFP_KERNEL);
> +		if (!configs)
> +			return -ENOMEM;
> +	} else {
> +		configs = devm_kmemdup(dev, *confs,
> +				       num_conf * sizeof(pinconf), GFP_KERNEL);
> +		if (!configs)
> +			return -ENOMEM;

> +		devm_kfree(dev, *confs);

This a red flag for using devm_*().
Either a sign of bad design or misplacement of devm_*().

> +	}
> +
> +	configs[num_conf - 1] = pinconf;
> +	*confs = configs;
> +	*nr_conf = num_conf;
> +
> +	return 0;
> +}

> +	/**

Are you sure about this?
Have you run kernel-doc script? Does it complain?

> +	 * Create pinctrl_map for each groups, per group per entry.
> +	 * Create pinctrl_map for pin config, per group per entry.
> +	 */
> +	if (nr_config)
> +		map_cnt++;

> +static void eqbr_dt_free_map(struct pinctrl_dev *pctldev,
> +			     struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	struct intel_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
> +	int i;
> +
> +	for (i = 0; i < num_maps; i++)
> +		if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
> +			devm_kfree(pctl->dev, map[i].data.configs.configs);
> +	devm_kfree(pctl->dev, map);

Yeah, no devm_*() here.

> +}

> +static const struct pinmux_ops eqbr_pinmux_ops = {
> +	.get_functions_count	= eqbr_pinmux_get_func_count,
> +	.get_function_name	= eqbr_pinmux_get_fname,
> +	.get_function_groups	= eqbr_pinmux_get_groups,
> +	.set_mux		= eqbr_pinmux_set_mux,
> +	.gpio_request_enable	= eqbr_pinmux_gpio_request,

> +	.strict			= 1,

Wrong type.

> +};

> +
> +static void set_drv_cur(void __iomem *mem, unsigned int offset,
> +			unsigned int set, raw_spinlock_t *lock)
> +{
> +	unsigned int idx = offset; /* 16 pin per register*/
> +	unsigned int reg;
> +

> +	idx = idx / DRV_CUR_PINS;

It can be done in the first place in the definition block.

> +	offset %= DRV_CUR_PINS;
> +	reg = REG_DRCC(idx);

> +	eqbr_set_val(mem + REG_DRCC(idx), offset * 2,
> +		     0x3, set, lock);

Quite enough space in previous line for arguments.

> +}

> +static int get_drv_cur(void __iomem *mem, unsigned int offset)
> +{
> +	unsigned int idx = offset; /* 0-15, 16-31 per register*/
> +	unsigned int val;
> +
> +	idx = idx / DRV_CUR_PINS;
> +	val = readl(mem + REG_DRCC(idx));
> +	offset %= DRV_CUR_PINS;
> +	val = PARSE_DRV_CURRENT(val, offset);
> +
> +	return val;
> +}

Ditto.

> +		case PINCONF_TYPE_PULL_UP:
> +			eqbr_set_val(mem + REG_PUEN, offset,
> +				     1, val, &pctl->lock);

Please, configure you editor better. The recommended limit is 80, not 60.

> +			break;
> +		case PINCONF_TYPE_PULL_DOWN:
> +			eqbr_set_val(mem + REG_PDEN, offset,
> +				     1, val, &pctl->lock);
> +			break;
> +		case PINCONF_TYPE_OPEN_DRAIN:
> +			eqbr_set_val(mem + REG_OD, offset,
> +				     1, val, &pctl->lock);
> +			break;
> +		case PINCONF_TYPE_DRIVE_CURRENT:
> +			set_drv_cur(mem, offset, val, &pctl->lock);
> +			break;
> +		case PINCONF_TYPE_SLEW_RATE:
> +			eqbr_set_val(mem + REG_SRC, offset,
> +				     1, val, &pctl->lock);
> +			break;

Ditto.

> +	val = !!(readl(mem + REG_PUEN) & BIT(offset));
> +	seq_printf(s, "PULL UP: %u\n", val);
> +	val = !!(readl(mem + REG_PDEN) & BIT(offset));
> +	seq_printf(s, "PULL DOWN: %u\n", val);
> +	val = !!(readl(mem + REG_OD) & BIT(offset));
> +	seq_printf(s, "OPEN DRAIN: %u\n", val);
> +	val = get_drv_cur(mem, offset);
> +	seq_printf(s, "DRIVE CURRENT: %u\n", val);
> +	val = !!(readl(mem + REG_SRC) & BIT(offset));
> +	seq_printf(s, "SLEW RATE: %u\n", val);
> +	gpio = get_gpio_desc_via_bank(pctl, bank);
> +	val = intel_eqbr_gpio_get_dir(&gpio->chip, offset);
> +	seq_printf(s, "OUTPUT: %u\n", !val);

I think GPIO library does it for you.

> +static int add_group_to_func(struct device *dev, struct intel_pmx_func *funcs,
> +			     unsigned int nr_funcs, unsigned int idx,
> +			     const char *grp_name)
> +{
> +	unsigned int nr_grps = funcs[idx].nr_groups + 1;
> +	const char **grps;
> +
> +	grps = devm_kmemdup(dev, funcs[idx].groups,
> +			    nr_grps * sizeof(*grps), GFP_KERNEL);
> +	if (!grps)
> +		return -ENOMEM;

> +	devm_kfree(dev, funcs[idx].groups);

Possible misplacement of devm_*().

> +	grps[nr_grps - 1] = grp_name;
> +	funcs[idx].groups = grps;
> +	funcs[idx].nr_groups = nr_grps;
> +
> +	return 0;
> +}

> +static int is_func_exist(struct intel_pmx_func *funcs, const char *name,
> +			 unsigned int nr_funcs, unsigned int *idx)
> +{
> +	int i;
> +
> +	if (!funcs || !nr_funcs)
> +		return 0;
> +
> +	for (i = 0; i < nr_funcs; i++) {
> +		if (strcmp(funcs[i].name, name) == 0) {
> +			*idx = i;
> +			return 1;
> +		}
> +	}
> +
> +	return 0;

NIH match_string(). But I think pin control core does it for drivers.

> +}

> +static void dump_pinctrl_group_func(struct intel_pinctrl_drv_data *drvdata)
> +{
> +	struct device *dev = drvdata->dev;
> +	const struct intel_pin_group *group;
> +	const struct intel_pmx_func *func;
> +	int i, j;
> +
> +	dev_info(dev, "Total %u groups, %u functions\n",
> +		 drvdata->nr_grps, drvdata->nr_funcs);
> +
> +	for (i = 0; i < drvdata->nr_grps; i++) {
> +		group = &drvdata->pin_grps[i];
> +
> +		dev_dbg(dev, "group name: %s, pin num: %u, pmx: %u\n",
> +			group->name, group->nr_pins, group->pmx);
> +		for (j = 0; j < group->nr_pins; j++)
> +			dev_dbg(dev, "pin[%d]: %u\n", j, group->pins[j]);
> +	}
> +
> +	for (i = 0; i < drvdata->nr_funcs; i++) {
> +		func = &drvdata->pmx_funcs[i];
> +
> +		dev_dbg(dev, "function name: %s, group num: %u\n",
> +			func->name, func->nr_groups);
> +		for (j = 0; j < func->nr_groups; j++)
> +			dev_dbg(dev, "group[%d]: %s\n", j, func->groups[j]);
> +	}
> +}

Ditto.

> +static int pinctrl_setup_from_dt(struct device *dev,
> +				 struct intel_pinctrl_drv_data *drvdata)
> +{

It's a very big function with potential of refactoring.
Also check what core provides.

> +}

> +static int pinctrl_reg(struct intel_pinctrl_drv_data *drvdata)
> +{

> +	drvdata->pctl_dev = devm_pinctrl_register(dev, pctl_desc, drvdata);
> +	if (IS_ERR(drvdata->pctl_dev)) {

> +		dev_err(dev, "Register pinctrl failed!\n");

Useless.

> +		return PTR_ERR(drvdata->pctl_dev);
> +	}
> +
> +	return 0;

PTR_ERR_OR_ZERO()

> +}

> +/**
> + * reset all pins to DEFAULT state, including below registers
> + * PINMUX set to GPIO
> + * DIR set to INPUT
> + * Clear PULLUP/PULLDOWN/SLEW RATE/DRIVE CURRENT/OPEN DRAIN
> + */

Please, read kernel doc documentation.

> +static void pinctrl_pin_reset(struct intel_pinctrl_drv_data *drvdata)
> +{

> +	for (i = 0; i < drvdata->nr_gpio_descs; i++) {
> +		gdesc = &drvdata->gpio_desc[i];

> +		for (pin = 0; pin < gdesc->bank->nr_pins; pin++) {
> +			if (!(BIT(pin) & gdesc->bank->aval_pinmap))
> +				continue;

for_each_set_bit()

> +		}
> +	}

> +}

> +static int intel_pinctrl_probe(struct platform_device *pdev)
> +{

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drvdata->membase = devm_ioremap_resource(dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(drvdata->membase))
> +		return PTR_ERR(drvdata->membase);

> +}

> +#define PINCONF_SET_MASK		(BIT(PINCONF_SET_SIZE) - 1)
> +#define PINCONF_TYPE_MASK		(BIT(PINCONF_TYPE_SIZE) - 1)

GENMASK()

Hi Andy,

Thanks for your comments. I agree & will address all your review concerns in
v2 except below mentioned points where i need more clarification.

On 12/9/2019 10:30 PM, Andy Shevchenko wrote:
>> +static const struct pin_config pin_cfg_type[] = {
>> +	{"intel,pullup",		PINCONF_TYPE_PULL_UP},
>> +	{"intel,pulldown",		PINCONF_TYPE_PULL_DOWN},
>> +	{"intel,drive-current",		PINCONF_TYPE_DRIVE_CURRENT},
>> +	{"intel,slew-rate",		PINCONF_TYPE_SLEW_RATE},
>> +	{"intel,open-drain",		PINCONF_TYPE_OPEN_DRAIN},
>> +	{"intel,output",		PINCONF_TYPE_OUTPUT},
>> +};
> Doesn't DT provide a generic naming scheme for these?

For pinctrl multiplexing/configuration nodes, DT does provide generic names
for some properties but it does not seem to mandate it. It states that the
content of pin mux/conf nodes is defined entirely by the binding of the pin
controller device. There are many other examples of pinctrl drivers which use
different prop names than generic ones. For e.g. Samsung. Our understanding
is: if the node is generic i.e. handled by framework, then it should use
generic name. But if the node is private to driver, then it is better to
prefix it with driver name to avoid any conflicts.
>> +	virq = irq_find_mapping(desc->irq_domain, offset);
>> +	if (virq)
>> +		return virq;
>>
>> +	else
>> +		return irq_create_mapping(desc->irq_domain, offset);
> Don't we have more clever helper for this? AFAIR something like this is done in
> IRQ framework when you get a mapping from certain domain.
>

I guess, you mean irq_domain_add_simple(). This function does optionally map
the IRQs but only statically assigned IRQs. We need dynamic gpio_to_irq
mappings which is why the gpio_chip->to_irq() is optionally provided so the
drivers requiring dynamic IRQ mappings can override this function.

But i can definitely get rid of redundant irq_find_mapping() because
irq_create_mapping() anyways invokes irq_find_mapping() first. I will remove
irq_find_mapping() and just use irq_create_mapping() here.
>> +static void eqbr_irq_handler(struct irq_desc *desc)
>> +{
>> +	struct intel_gpio_desc *gc;
>> +	struct irq_chip *ic;
>> +	u32 pins, offset;
>> +	unsigned int virq;
>> +
>> +	gc = irq_desc_get_handler_data(desc);
>> +	ic = irq_desc_get_chip(desc);
>> +
>> +	chained_irq_enter(ic, desc);
>> +	pins = readl(gc->membase + GPIO_IRNCR);
>> +
>> +	for_each_set_bit(offset, (unsigned long *)&pins, gc->bank->nr_pins) {
>> +		virq = irq_linear_revmap(gc->irq_domain, offset);
>> +		if (!virq)
>> +			pr_err("gc[%s]:pin:%d irq not registered!\n",
>> +			       gc->name, offset);
> dev_err() ? But Why is it needed? Shouldn't be registered as a spurious IRQ for
> later debugging?

IMHO, spurious IRQ can only be registered if none of the pins are valid.
Please see below alternative way of handling it & let me know if this
makes more sense.

@@ -313,6 +313,7 @@ static void eqbr_irq_handler(struct irq_desc *desc)
        struct irq_chip *ic;
        u32 pins, offset;
        unsigned int virq;
+       int handled = 0;

        gc = irq_desc_get_handler_data(desc);
        ic = irq_desc_get_chip(desc);
@@ -322,12 +323,16 @@ static void eqbr_irq_handler(struct irq_desc *desc)

        for_each_set_bit(offset, (unsigned long *)&pins, gc->bank->nr_pins) {
                virq = irq_linear_revmap(gc->irq_domain, offset);
-               if (!virq)
-                       pr_err("gc[%s]:pin:%d irq not registered!\n",
-                              gc->name, offset);
-               else
+               if (virq) {
                        generic_handle_irq(virq);
+                       handled++;
+               }
        }
+
+       /* Spurious interrupt */
+       if (handled == 0)
+               handle_bad_irq(desc);
+
        chained_irq_exit(ic, desc);
 }


>> +static int add_config(struct intel_pinctrl_drv_data *drvdata,
>> +		      unsigned long **confs, unsigned int *nr_conf,
>> +		      unsigned long pinconf)
>> +{
>> +	unsigned long *configs;
>> +	struct device *dev = drvdata->dev;
>> +	unsigned int num_conf = *nr_conf + 1;
>> +
>> +	if (!(*nr_conf)) {
>> +		configs = devm_kcalloc(dev, 1, sizeof(pinconf), GFP_KERNEL);
>> +		if (!configs)
>> +			return -ENOMEM;
>> +	} else {
>> +		configs = devm_kmemdup(dev, *confs,
>> +				       num_conf * sizeof(pinconf), GFP_KERNEL);
>> +		if (!configs)
>> +			return -ENOMEM;
>> +		devm_kfree(dev, *confs);
> This a red flag for using devm_*().
> Either a sign of bad design or misplacement of devm_*().

I can switch to non devm versions i.e. kmalloc/kfree for these buffer
allocations. But this leaves me with a fundamental question. As i
understand, devm*() variants are to allocate device specific resources
so you don't have to worry about releasing those resources when device
is unloaded. Does that mean that we should only use devm*() for resources
which have a lifetime from driver load until the driver is unloaded ?
For all other allocations which requires freeing up & reallocation during
the operational state of driver, we should use non devm variants i.e.
kmalloc/kfree ? If yes, then what's the purpose of devm_kfree() & other
similar APIs ?
>
>> +	val = !!(readl(mem + REG_PUEN) & BIT(offset));
>> +	seq_printf(s, "PULL UP: %u\n", val);
>> +	val = !!(readl(mem + REG_PDEN) & BIT(offset));
>> +	seq_printf(s, "PULL DOWN: %u\n", val);
>> +	val = !!(readl(mem + REG_OD) & BIT(offset));
>> +	seq_printf(s, "OPEN DRAIN: %u\n", val);
>> +	val = get_drv_cur(mem, offset);
>> +	seq_printf(s, "DRIVE CURRENT: %u\n", val);
>> +	val = !!(readl(mem + REG_SRC) & BIT(offset));
>> +	seq_printf(s, "SLEW RATE: %u\n", val);
>> +	gpio = get_gpio_desc_via_bank(pctl, bank);
>> +	val = intel_eqbr_gpio_get_dir(&gpio->chip, offset);
>> +	seq_printf(s, "OUTPUT: %u\n", !val);
> I think GPIO library does it for you.

Sorry, could not figure out what you mean. Do you mean no need to override
pinconf_ops->pin_config_dbg_show() ?
>
>> +static int is_func_exist(struct intel_pmx_func *funcs, const char *name,
>> +			 unsigned int nr_funcs, unsigned int *idx)
>> +{
>> +	int i;
>> +
>> +	if (!funcs || !nr_funcs)
>> +		return 0;
>> +
>> +	for (i = 0; i < nr_funcs; i++) {
>> +		if (strcmp(funcs[i].name, name) == 0) {
>> +			*idx = i;
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
> NIH match_string(). But I think pin control core does it for drivers.

match_string() searches a string within a array of strings. Here, we are
searching an array of structures, each struct having a unique string, to
match that unique string & return index of containing structure. Could
not find any helper for this purpose. Checked pin control core code as
well but could not find any routine to use for this purpose.

Regards,
Rahul

Hi Rahul,

this is an interesting patch!

Let's dive in and fix the things not already pointed out in review.

It will need some work but I am sure you can get there.

On Thu, Sep 12, 2019 at 9:59 AM Rahul Tanwar
<rahul.tanwar@linux.intel.com> wrote:

> Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP
> which controls pin multiplexing & configuration including GPIO functions
> selection & GPIO attributes configuration. Add GPIO & pin control framework
> based driver for this IP.
>
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
(...)

> +config PINCTRL_EQUILIBRIUM
> +       tristate "Generic pinctrl and GPIO driver for Intel Lightning Mountain SoC"
> +       select PINMUX
> +       select PINCONF
> +       select GPIOLIB
> +       select GPIOLIB_IRQCHIP

Nice use of the GPIOLIB_IRQCHIP.

Are you sure you can't just use GPIO_GENERIC as well?
This is almost always usable when you have a register with
n consecutive bits representing GPIO lines.

Look how we use bgpio_init() in e.g. drivers/gpio/gpio-ftgpio010.c
to cut down on boilerplate code, and we also get a spinlock
protection and .get/.set_multiple() implementation for free.

> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/machine.h>

Why do you need these two includes?

> +static const struct pin_config pin_cfg_type[] = {
> +       {"intel,pullup",                PINCONF_TYPE_PULL_UP},
> +       {"intel,pulldown",              PINCONF_TYPE_PULL_DOWN},
> +       {"intel,drive-current",         PINCONF_TYPE_DRIVE_CURRENT},
> +       {"intel,slew-rate",             PINCONF_TYPE_SLEW_RATE},
> +       {"intel,open-drain",            PINCONF_TYPE_OPEN_DRAIN},
> +       {"intel,output",                PINCONF_TYPE_OUTPUT},
> +};

So... if we are adding a new driver with a new DT binding,
why use the made-up "intel," prefixed flags when we have the
standard DT flags from
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
already handled by the core?

> +static inline void eqbr_set_val(void __iomem *addr, u32 offset,
> +                               u32 mask, u32 set, raw_spinlock_t *lock)
> +{
> +       u32 val;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(lock, flags);
> +       val = readl(addr) & ~(mask << offset);
> +       writel(val | ((set & mask) << offset), addr);
> +       raw_spin_unlock_irqrestore(lock, flags);
> +}

Mask-and-set is usually achieved with regmap-mmio if you
dont use GPIO_GENERIC, but I think you can just use
GPIO_GENERIC. All of these:

> +static int intel_eqbr_gpio_get_dir(struct gpio_chip *gc, unsigned int offset)
> +static int intel_eqbr_gpio_dir_input(struct gpio_chip *gc, unsigned int offset)
> +static int intel_eqbr_gpio_dir_output(struct gpio_chip *gc, unsigned int offset,
> +static void intel_eqbr_gpio_set(struct gpio_chip *gc,
> +                               unsigned int offset, int dir)
> +static int intel_eqbr_gpio_get(struct gpio_chip *gc, unsigned int offset)

Look very bit-per-bit mapped.

> +static int eqbr_irq_map(struct irq_domain *d,
> +                       unsigned int virq, irq_hw_number_t hw)
> +{
> +       struct intel_gpio_desc *desc = d->host_data;
> +
> +       irq_set_chip_data(virq, desc);
> +       irq_set_chip_and_handler(virq, desc->ic, handle_level_irq);
> +       return 0;
> +}
> +
> +static const struct irq_domain_ops gc_irqdomain_ops = {
> +       .map    = eqbr_irq_map,
> +       .xlate  = irq_domain_xlate_twocell,
> +};
> +
(...)
> +static int intel_eqbr_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct intel_gpio_desc *desc = gpiochip_get_data(gc);

Since struct gpio_desc means a per-line state container
and struct intel_gpio_desc refers to the whole chip, I think this
struct should be renamed something like struct eqbr_gpio.

> +       unsigned int virq;
> +
> +       if (!desc->irq_domain)
> +               return -ENODEV;
> +
> +       virq = irq_find_mapping(desc->irq_domain, offset);
> +       if (virq)
> +               return virq;
> +       else
> +               return irq_create_mapping(desc->irq_domain, offset);
> +}
> +
> +static int gpiochip_setup(struct device *dev, struct intel_gpio_desc *desc)
> +{
(...)
> +       gc->to_irq              = intel_eqbr_gpio_to_irq;

You don't need any of this funku stuff. The GPIOLIB_IRQCHIP
provides default implementations to do all this for you.
Just look in drivers/gpio/gpio-ftgpio010.c and follow
the pattern (look how I set up struct gpio_irq_chip using
*girq etc). If you need anything custom we need some
special motivation here.

> +       gc->of_node             = desc->node;
> +       gc->parent              = dev;

I would allocate a dynamic irqchip as part of the
struct intel_gpio_desc and populate it dynamically with
function pointers.

struct gpio_irq_chip *girq;

girq = &gc->irq;
girq->chip = ...

> +       ret = devm_gpiochip_add_data(dev, gc, desc);
> +       if (ret)
> +               dev_err(dev, "failed to register gpiochip: %s, err: %d\n",
> +                       gc->label, ret);
> +
> +       return ret;
> +}

(...)
> +static int eqbr_gpio_irq_req_res(struct irq_data *d)
> +{
> +       struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
> +       unsigned int offset;
> +       int ret;
> +
> +       offset = irqd_to_hwirq(d);
> +
> +       /* gpio must be set as input */
> +       intel_eqbr_gpio_dir_input(&desc->chip, offset);

Please move this to the .irq_enable() callback instead.

> +       ret = gpiochip_lock_as_irq(&desc->chip, offset);
> +       if (ret) {
> +               pr_err("%s: Failed to lock gpio %u as irq!\n",
> +                      desc->name, offset);
> +               return ret;
> +       }
> +       eqbr_gpio_enable_irq(d);

Why are you calling this here? It is premature I think,
isn't the call in .unmask() soon enough? The latter is
what we rely upon.

> +static void eqbr_gpio_irq_rel_res(struct irq_data *d)
> +{
> +       struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
> +       unsigned int offset = irqd_to_hwirq(d);
> +
> +       eqbr_gpio_disable_irq(d);

No need to do this, .irq_mask() has already done it at this
point.

> +       gpiochip_unlock_as_irq(&desc->chip, offset);
> +}

I think the core default implementations should be fine for both
reqres and relres.

> +static struct irq_chip eqbr_irq_chip = {
> +       .name                   = "gpio_irq",
> +       .irq_mask               = eqbr_gpio_disable_irq,
> +       .irq_unmask             = eqbr_gpio_enable_irq,
> +       .irq_ack                = eqbr_gpio_ack_irq,
> +       .irq_mask_ack           = eqbr_gpio_mask_ack_irq,
> +       .irq_set_type           = eqbr_gpio_set_irq_type,
> +       .irq_request_resources  = eqbr_gpio_irq_req_res,
> +       .irq_release_resources  = eqbr_gpio_irq_rel_res,
> +};

So please add a struct irq_chip to the state container
(struct intel_gpio_desc) and assign these functions directly
in probe (again look at gpio-ftgpio010.c).

> +static void eqbr_irq_handler(struct irq_desc *desc)
> +{
> +       struct intel_gpio_desc *gc;
> +       struct irq_chip *ic;
> +       u32 pins, offset;
> +       unsigned int virq;
> +
> +       gc = irq_desc_get_handler_data(desc);
> +       ic = irq_desc_get_chip(desc);

When using the GPIOLIB_IRQCHIP follow the pattern from
other drivers and assume the handler data is the struct gpio_chip
instead.

struct gpio_chip *gc = irq_desc_get_handler_data(desc);
struct intel_gpio_desc *i = gpiochip_get_data(gc);
(...)

> +static int irqchip_setup(struct device *dev, struct intel_gpio_desc *desc)
> +{
> +       struct device_node *np = desc->node;
> +
> +       if (!of_property_read_bool(np, "interrupt-controller")) {
> +               dev_info(dev, "gc %s: doesn't act as interrupt controller!\n",
> +                        desc->name);
> +               return 0;
> +       }

OK just skip assigning *girq with the chip etc for this case.

> +       desc->irq_domain = irq_domain_add_linear(desc->node,
> +                                                desc->bank->nr_pins,
> +                                                &gc_irqdomain_ops, desc);
> +       if (!desc->irq_domain) {
> +               dev_err(dev, "%s: failed to create gpio irq domain!\n",
> +                       desc->name);
> +               return -ENODEV;
> +       }
> +       irq_set_chained_handler_and_data(desc->virq, eqbr_irq_handler, desc);

Let GPIOLIB_IRQCHIP handle these things for you instead of
making your own domain etc.

> +static int gpiolib_reg(struct intel_pinctrl_drv_data *drvdata)
> +{
> +       struct device_node *np;
> +       struct intel_gpio_desc *desc;
> +       struct device *dev;
> +       int i, ret;
> +       char name[32];
> +       struct resource res;
> +
> +       dev = drvdata->dev;
> +       for (i = 0; i < drvdata->nr_gpio_descs; i++) {
> +               desc = drvdata->gpio_desc + i;
> +               np = desc->node;
> +               sprintf(name, "gpiochip%d", i);
> +               desc->name = devm_kmemdup(dev, name,
> +                                         strlen(name) + 1, GFP_KERNEL);
> +               if (!desc->name)
> +                       return -ENOMEM;
> +               if (of_address_to_resource(np, 0, &res)) {
> +                       dev_err(dev, "Failed to get GPIO register addrss\n");

Speling

> +                       return -ENXIO;
> +               }
> +               desc->membase = devm_ioremap_resource(dev, &res);
> +               if (IS_ERR(desc->membase)) {
> +                       dev_err(dev, "ioremap fail\n");
> +                       return PTR_ERR(desc->membase);
> +               }
> +               dev_dbg(dev, "gpio resource: %pr\n", &res);
> +               dev_dbg(dev, "gpiochip membase: %px\n", desc->membase);
> +
> +               desc->virq = irq_of_parse_and_map(np, 0);
> +               if (!desc->virq) {
> +                       dev_err(dev, "%s: failed to parse and map irq\n",
> +                               name);
> +                       return -ENXIO;
> +               }
> +               raw_spin_lock_init(&desc->lock);
> +
> +               ret = gpiochip_setup(dev, desc);
> +               if (ret)
> +                       return ret;
> +               ret = irqchip_setup(dev, desc);
> +               if (ret)
> +                       return ret;

Bake these two into a single function setting up gpio_chip and
irq_chip. With proper use of GPIOLIB_IRQCHIP it will be so
much simpler anyway.

> +static int parse_mux_info(struct device_node *np)
> +{
> +       int ret;
> +       const char *str;
> +
> +       ret = of_property_read_string(np, "intel,function", &str);
> +       if (ret)
> +               return -ENODEV;
> +       ret = of_property_read_string(np, "intel,groups", &str);
> +       if (ret)
> +               return -ENODEV;
> +
> +       return ret;
> +}

Again these are intel,foo-specific properties for things we already
have standard bindings for, so use those.

> +static int add_config(struct intel_pinctrl_drv_data *drvdata,
> +                     unsigned long **confs, unsigned int *nr_conf,
> +                     unsigned long pinconf)
> +{
> +       unsigned long *configs;
> +       struct device *dev = drvdata->dev;
> +       unsigned int num_conf = *nr_conf + 1;
> +
> +       if (!(*nr_conf)) {
> +               configs = devm_kcalloc(dev, 1, sizeof(pinconf), GFP_KERNEL);
> +               if (!configs)
> +                       return -ENOMEM;
> +       } else {
> +               configs = devm_kmemdup(dev, *confs,
> +                                      num_conf * sizeof(pinconf), GFP_KERNEL);
> +               if (!configs)
> +                       return -ENOMEM;
> +               devm_kfree(dev, *confs);
> +       }
> +
> +       configs[num_conf - 1] = pinconf;
> +       *confs = configs;
> +       *nr_conf = num_conf;
> +
> +       return 0;
> +}
> +
> +static void eqbr_add_map_mux(struct device_node *np, struct pinctrl_map **map,
> +                            int *index)
> +{
> +       int idx = *index;
> +       const char *function, *group;
> +
> +       of_property_read_string(np, "intel,function", &function);
> +       of_property_read_string(np, "intel,groups", &group);
> +
> +       (*map)[idx].type = PIN_MAP_TYPE_MUX_GROUP;
> +       (*map)[idx].data.mux.group = group;
> +       (*map)[idx].data.mux.function = function;
> +       *index = idx + 1;
> +}
> +
> +static void eqbr_add_map_configs(struct device_node *np,
> +                                struct pinctrl_map **map, int *index,
> +                                unsigned long *configs, unsigned int nr_config)
> +{
> +       int idx = *index;
> +       const char *group;
> +
> +       of_property_read_string(np, "intel,groups", &group);
> +       (*map)[idx].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +       (*map)[idx].data.configs.group_or_pin = group;
> +       (*map)[idx].data.configs.configs = configs;
> +       (*map)[idx].data.configs.num_configs = nr_config;
> +       *index = idx + 1;
> +}
> +
> +static int eqbr_dt_node_to_map(struct pinctrl_dev *pctldev,
> +                              struct device_node *np,
> +                              struct pinctrl_map **map, unsigned int *num_maps)
> +{
> +       struct intel_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int map_cnt, nr_config;
> +       unsigned long pin_conf, *configs = NULL;
> +       int i, ret;
> +       unsigned int val;
> +       bool func = false;
> +
> +       *map = NULL;
> +       *num_maps = map_cnt = nr_config = 0;
> +
> +       ret = parse_mux_info(np);
> +       if (!ret) {
> +               map_cnt++;
> +               func = true;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(pin_cfg_type); i++) {
> +               ret = of_property_read_u32(np, pin_cfg_type[i].property, &val);
> +               if (!ret) {
> +                       pin_conf = PINCONF_PACK(pin_cfg_type[i].type, val);
> +                       ret = add_config(pctl, &configs, &nr_config, pin_conf);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +
> +       /**
> +        * Create pinctrl_map for each groups, per group per entry.
> +        * Create pinctrl_map for pin config, per group per entry.
> +        */
> +       if (nr_config)
> +               map_cnt++;
> +
> +       *map = devm_kcalloc(pctl->dev, map_cnt, sizeof(**map), GFP_KERNEL);
> +       if (!*map)
> +               return -ENOMEM;
> +
> +       i = 0;
> +       if (func)
> +               eqbr_add_map_mux(np, map, &i);
> +       if (nr_config)
> +               eqbr_add_map_configs(np, map, &i, configs, nr_config);
> +
> +       *num_maps = map_cnt;
> +
> +       return 0;
> +}

With the library code for the standard bindings select
GENERIC_PINMUX_FUNCTIONS and select GENERIC_PINCONF
most of the above goes away as well.

> +static void eqbr_dt_free_map(struct pinctrl_dev *pctldev,
> +                            struct pinctrl_map *map, unsigned int num_maps)
> +{
> +       struct intel_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       int i;
> +
> +       for (i = 0; i < num_maps; i++)
> +               if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
> +                       devm_kfree(pctl->dev, map[i].data.configs.configs);
> +       devm_kfree(pctl->dev, map);
> +}

In this case I think you can use the library function
pinctrl_utils_free_map() just as is.

Now I ran out of time, but the generic advice is to use
library code and standard bindings as much as you can
and all will shrink down considerably. Start with the
above pointers and I will look closer after that!

Yours,
Linus Walleij

Hi Linus,

Thanks for taking time out to review.

On 5/10/2019 4:28 AM, Linus Walleij wrote:
>> +config PINCTRL_EQUILIBRIUM
>> +       tristate "Generic pinctrl and GPIO driver for Intel Lightning Mountain SoC"
>> +       select PINMUX
>> +       select PINCONF
>> +       select GPIOLIB
>> +       select GPIOLIB_IRQCHIP
> Nice use of the GPIOLIB_IRQCHIP.
>
> Are you sure you can't just use GPIO_GENERIC as well?
> This is almost always usable when you have a register with
> n consecutive bits representing GPIO lines.
>
> Look how we use bgpio_init() in e.g. drivers/gpio/gpio-ftgpio010.c
> to cut down on boilerplate code, and we also get a spinlock
> protection and .get/.set_multiple() implementation for free.

I went through gpio-mmio.c & gpio-ftgpio010.c code. My understanding is
that GPIO_GENERIC can support a max of 64 consecutive bits representing
GPIO lines. We have more than 100 GPIO's and they are spread out across
4 different banks with non consecutive registers i.e. DATA_IN_0~31@offset0x0,
DATA_IN_32~63@offset0x100 and so on. In other words, i think we can not
support memory mapped GPIO controller.

>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/pinctrl/machine.h>
> Why do you need these two includes?

Yes, these are redundant. I will remove them. Thanks.

>> +static const struct pin_config pin_cfg_type[] = {
>> +       {"intel,pullup",                PINCONF_TYPE_PULL_UP},
>> +       {"intel,pulldown",              PINCONF_TYPE_PULL_DOWN},
>> +       {"intel,drive-current",         PINCONF_TYPE_DRIVE_CURRENT},
>> +       {"intel,slew-rate",             PINCONF_TYPE_SLEW_RATE},
>> +       {"intel,open-drain",            PINCONF_TYPE_OPEN_DRAIN},
>> +       {"intel,output",                PINCONF_TYPE_OUTPUT},
>> +};
> So... if we are adding a new driver with a new DT binding,
> why use the made-up "intel," prefixed flags when we have the
> standard DT flags from
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> already handled by the core?

Yes, Andy & Rob have also raised same concerns. I will switch to using
standard DT properties & generic pinconf and remove redundant code.
Thanks.

>> +static inline void eqbr_set_val(void __iomem *addr, u32 offset,
>> +                               u32 mask, u32 set, raw_spinlock_t *lock)
>> +{
>> +       u32 val;
>> +       unsigned long flags;
>> +
>> +       raw_spin_lock_irqsave(lock, flags);
>> +       val = readl(addr) & ~(mask << offset);
>> +       writel(val | ((set & mask) << offset), addr);
>> +       raw_spin_unlock_irqrestore(lock, flags);
>> +}
> Mask-and-set is usually achieved with regmap-mmio if you
> dont use GPIO_GENERIC, but I think you can just use
> GPIO_GENERIC. All of these:

As mentioned above, we cannot use GPIO_GENERIC. And there was no specific
reason/motivation for us to use regmap-mmio because most of the driver's
that we referenced were using readl()/write(). Do you recommend us to remove
readl()/writel() and switch to regmap-mmio based design ?

>> +static int intel_eqbr_gpio_get_dir(struct gpio_chip *gc, unsigned int offset)
>> +static int intel_eqbr_gpio_dir_input(struct gpio_chip *gc, unsigned int offset)
>> +static int intel_eqbr_gpio_dir_output(struct gpio_chip *gc, unsigned int offset,
>> +static void intel_eqbr_gpio_set(struct gpio_chip *gc,
>> +                               unsigned int offset, int dir)
>> +static int intel_eqbr_gpio_get(struct gpio_chip *gc, unsigned int offset)
> Look very bit-per-bit mapped.
>
>> +static int intel_eqbr_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
>> +{
>> +       struct intel_gpio_desc *desc = gpiochip_get_data(gc);
> Since struct gpio_desc means a per-line state container
> and struct intel_gpio_desc refers to the whole chip, I think this
> struct should be renamed something like struct eqbr_gpio.

Just to clarify, we have 4 different GPIO banks and each GPIO bank is
implemented as a separate gpio_chip. So we have 4 instances of gpio_desc
each one having its own gpio_chip. What i mean to say is that gpio_desc
is actually not a per-line state container, its a per gpio_chip container.

>> +       unsigned int virq;
>> +
>> +       if (!desc->irq_domain)
>> +               return -ENODEV;
>> +
>> +       virq = irq_find_mapping(desc->irq_domain, offset);
>> +       if (virq)
>> +               return virq;
>> +       else
>> +               return irq_create_mapping(desc->irq_domain, offset);
>> +}
>> +
>> +static int gpiochip_setup(struct device *dev, struct intel_gpio_desc *desc)
>> +{
> (...)
>> +       gc->to_irq              = intel_eqbr_gpio_to_irq;
> You don't need any of this funku stuff. The GPIOLIB_IRQCHIP
> provides default implementations to do all this for you.
> Just look in drivers/gpio/gpio-ftgpio010.c and follow
> the pattern (look how I set up struct gpio_irq_chip using
> *girq etc). If you need anything custom we need some
> special motivation here.

Yes, i checked gpio-ftgpio010.c and agree that this is already handled
under GPIOLIB_IRQCHIP. I will make these changes in V2. Thanks.

>> +       gc->of_node             = desc->node;
>> +       gc->parent              = dev;
> I would allocate a dynamic irqchip as part of the
> struct intel_gpio_desc and populate it dynamically with
> function pointers.
>
> struct gpio_irq_chip *girq;
>
> girq = &gc->irq;
> girq->chip = ...

Agree, i will follow this approach as part of switching to reusing
GPIOLIB_IRQCHIP default implementations. Thanks.

>> +static int eqbr_gpio_irq_req_res(struct irq_data *d)
>> +{
>> +       struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
>> +       unsigned int offset;
>> +       int ret;
>> +
>> +       offset = irqd_to_hwirq(d);
>> +
>> +       /* gpio must be set as input */
>> +       intel_eqbr_gpio_dir_input(&desc->chip, offset);
> Please move this to the .irq_enable() callback instead.

Well noted.

>> +       ret = gpiochip_lock_as_irq(&desc->chip, offset);
>> +       if (ret) {
>> +               pr_err("%s: Failed to lock gpio %u as irq!\n",
>> +                      desc->name, offset);
>> +               return ret;
>> +       }
>> +       eqbr_gpio_enable_irq(d);
> Why are you calling this here? It is premature I think,
> isn't the call in .unmask() soon enough? The latter is
> what we rely upon.

Agree, have already changed it.

>> +static void eqbr_gpio_irq_rel_res(struct irq_data *d)
>> +{
>> +       struct intel_gpio_desc *desc = irq_data_get_irq_chip_data(d);
>> +       unsigned int offset = irqd_to_hwirq(d);
>> +
>> +       eqbr_gpio_disable_irq(d);
> No need to do this, .irq_mask() has already done it at this
> point.

Agree, have already changed it.

>> +       gpiochip_unlock_as_irq(&desc->chip, offset);
>> +}
> I think the core default implementations should be fine for both
> reqres and relres.

I also observed this when referring to gpio-ftgpio010.c. Will change to
default implementations.

>> +static struct irq_chip eqbr_irq_chip = {
>> +       .name                   = "gpio_irq",
>> +       .irq_mask               = eqbr_gpio_disable_irq,
>> +       .irq_unmask             = eqbr_gpio_enable_irq,
>> +       .irq_ack                = eqbr_gpio_ack_irq,
>> +       .irq_mask_ack           = eqbr_gpio_mask_ack_irq,
>> +       .irq_set_type           = eqbr_gpio_set_irq_type,
>> +       .irq_request_resources  = eqbr_gpio_irq_req_res,
>> +       .irq_release_resources  = eqbr_gpio_irq_rel_res,
>> +};
> So please add a struct irq_chip to the state container
> (struct intel_gpio_desc) and assign these functions directly
> in probe (again look at gpio-ftgpio010.c).

Yup, agree. Will change. Thanks.

>> +static void eqbr_irq_handler(struct irq_desc *desc)
>> +{
>> +       struct intel_gpio_desc *gc;
>> +       struct irq_chip *ic;
>> +       u32 pins, offset;
>> +       unsigned int virq;
>> +
>> +       gc = irq_desc_get_handler_data(desc);
>> +       ic = irq_desc_get_chip(desc);
> When using the GPIOLIB_IRQCHIP follow the pattern from
> other drivers and assume the handler data is the struct gpio_chip
> instead.
>
> struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> struct intel_gpio_desc *i = gpiochip_get_data(gc);
> (...)

Well noted.

>> +static int irqchip_setup(struct device *dev, struct intel_gpio_desc *desc)
>> +{
>> +       struct device_node *np = desc->node;
>> +
>> +       if (!of_property_read_bool(np, "interrupt-controller")) {
>> +               dev_info(dev, "gc %s: doesn't act as interrupt controller!\n",
>> +                        desc->name);
>> +               return 0;
>> +       }
> OK just skip assigning *girq with the chip etc for this case.

I understand. BTW, this call is in the probe path. I am planning to do girq
setup on the lines of gpio-ftgpio010.c in this function instead of probe.
So yes, i can skip assigning chip to girq for this case in this function.

>> +       desc->irq_domain = irq_domain_add_linear(desc->node,
>> +                                                desc->bank->nr_pins,
>> +                                                &gc_irqdomain_ops, desc);
>> +       if (!desc->irq_domain) {
>> +               dev_err(dev, "%s: failed to create gpio irq domain!\n",
>> +                       desc->name);
>> +               return -ENODEV;
>> +       }
>> +       irq_set_chained_handler_and_data(desc->virq, eqbr_irq_handler, desc);
> Let GPIOLIB_IRQCHIP handle these things for you instead of
> making your own domain etc.

Yes, got it now. Thanks.

>> +static int gpiolib_reg(struct intel_pinctrl_drv_data *drvdata)
>> +{
>> +       struct device_node *np;
>> +       struct intel_gpio_desc *desc;
>> +       struct device *dev;
>> +       int i, ret;
>> +       char name[32];
>> +       struct resource res;
>> +
>> +       dev = drvdata->dev;
>> +       for (i = 0; i < drvdata->nr_gpio_descs; i++) {
>> +               desc = drvdata->gpio_desc + i;
>> +               np = desc->node;
>> +               sprintf(name, "gpiochip%d", i);
>> +               desc->name = devm_kmemdup(dev, name,
>> +                                         strlen(name) + 1, GFP_KERNEL);
>> +               if (!desc->name)
>> +                       return -ENOMEM;
>> +               if (of_address_to_resource(np, 0, &res)) {
>> +                       dev_err(dev, "Failed to get GPIO register addrss\n");
> Speling

Well noted.

>> +                       return -ENXIO;
>> +               }
>> +               desc->membase = devm_ioremap_resource(dev, &res);
>> +               if (IS_ERR(desc->membase)) {
>> +                       dev_err(dev, "ioremap fail\n");
>> +                       return PTR_ERR(desc->membase);
>> +               }
>> +               dev_dbg(dev, "gpio resource: %pr\n", &res);
>> +               dev_dbg(dev, "gpiochip membase: %px\n", desc->membase);
>> +
>> +               desc->virq = irq_of_parse_and_map(np, 0);
>> +               if (!desc->virq) {
>> +                       dev_err(dev, "%s: failed to parse and map irq\n",
>> +                               name);
>> +                       return -ENXIO;
>> +               }
>> +               raw_spin_lock_init(&desc->lock);
>> +
>> +               ret = gpiochip_setup(dev, desc);
>> +               if (ret)
>> +                       return ret;
>> +               ret = irqchip_setup(dev, desc);
>> +               if (ret)
>> +                       return ret;
> Bake these two into a single function setting up gpio_chip and
> irq_chip. With proper use of GPIOLIB_IRQCHIP it will be so
> much simpler anyway.

Agree. Will change accordingly. Thanks.

>> +static int parse_mux_info(struct device_node *np)
>> +{
>> +       int ret;
>> +       const char *str;
>> +
>> +       ret = of_property_read_string(np, "intel,function", &str);
>> +       if (ret)
>> +               return -ENODEV;
>> +       ret = of_property_read_string(np, "intel,groups", &str);
>> +       if (ret)
>> +               return -ENODEV;
>> +
>> +       return ret;
>> +}
> Again these are intel,foo-specific properties for things we already
> have standard bindings for, so use those.

Yes, this will most likely go away because pinctrl_ops->dt_node_to_map()
uses this function to count group map entries. And based on your comments,
i will switch to using pinconf generic with standard properties which already
handles dt_node_to_map() & dt_free_map().

>> +static int add_config(struct intel_pinctrl_drv_data *drvdata,
>> +                     unsigned long **confs, unsigned int *nr_conf,
>> +                     unsigned long pinconf)
>> +{
>> +       unsigned long *configs;
>> +       struct device *dev = drvdata->dev;
>> +       unsigned int num_conf = *nr_conf + 1;
>> +
>> +       if (!(*nr_conf)) {
>> +               configs = devm_kcalloc(dev, 1, sizeof(pinconf), GFP_KERNEL);
>> +               if (!configs)
>> +                       return -ENOMEM;
>> +       } else {
>> +               configs = devm_kmemdup(dev, *confs,
>> +                                      num_conf * sizeof(pinconf), GFP_KERNEL);
>> +               if (!configs)
>> +                       return -ENOMEM;
>> +               devm_kfree(dev, *confs);
>> +       }
>> +
>> +       configs[num_conf - 1] = pinconf;
>> +       *confs = configs;
>> +       *nr_conf = num_conf;
>> +
>> +       return 0;
>> +}
>> +
>> +static void eqbr_add_map_mux(struct device_node *np, struct pinctrl_map **map,
>> +                            int *index)
>> +{
>> +       int idx = *index;
>> +       const char *function, *group;
>> +
>> +       of_property_read_string(np, "intel,function", &function);
>> +       of_property_read_string(np, "intel,groups", &group);
>> +
>> +       (*map)[idx].type = PIN_MAP_TYPE_MUX_GROUP;
>> +       (*map)[idx].data.mux.group = group;
>> +       (*map)[idx].data.mux.function = function;
>> +       *index = idx + 1;
>> +}
>> +
>> +static void eqbr_add_map_configs(struct device_node *np,
>> +                                struct pinctrl_map **map, int *index,
>> +                                unsigned long *configs, unsigned int nr_config)
>> +{
>> +       int idx = *index;
>> +       const char *group;
>> +
>> +       of_property_read_string(np, "intel,groups", &group);
>> +       (*map)[idx].type = PIN_MAP_TYPE_CONFIGS_GROUP;
>> +       (*map)[idx].data.configs.group_or_pin = group;
>> +       (*map)[idx].data.configs.configs = configs;
>> +       (*map)[idx].data.configs.num_configs = nr_config;
>> +       *index = idx + 1;
>> +}
>> +
>> +static int eqbr_dt_node_to_map(struct pinctrl_dev *pctldev,
>> +                              struct device_node *np,
>> +                              struct pinctrl_map **map, unsigned int *num_maps)
>> +{
>> +       struct intel_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +       unsigned int map_cnt, nr_config;
>> +       unsigned long pin_conf, *configs = NULL;
>> +       int i, ret;
>> +       unsigned int val;
>> +       bool func = false;
>> +
>> +       *map = NULL;
>> +       *num_maps = map_cnt = nr_config = 0;
>> +
>> +       ret = parse_mux_info(np);
>> +       if (!ret) {
>> +               map_cnt++;
>> +               func = true;
>> +       }
>> +
>> +       for (i = 0; i < ARRAY_SIZE(pin_cfg_type); i++) {
>> +               ret = of_property_read_u32(np, pin_cfg_type[i].property, &val);
>> +               if (!ret) {
>> +                       pin_conf = PINCONF_PACK(pin_cfg_type[i].type, val);
>> +                       ret = add_config(pctl, &configs, &nr_config, pin_conf);
>> +                       if (ret)
>> +                               return ret;
>> +               }
>> +       }
>> +
>> +       /**
>> +        * Create pinctrl_map for each groups, per group per entry.
>> +        * Create pinctrl_map for pin config, per group per entry.
>> +        */
>> +       if (nr_config)
>> +               map_cnt++;
>> +
>> +       *map = devm_kcalloc(pctl->dev, map_cnt, sizeof(**map), GFP_KERNEL);
>> +       if (!*map)
>> +               return -ENOMEM;
>> +
>> +       i = 0;
>> +       if (func)
>> +               eqbr_add_map_mux(np, map, &i);
>> +       if (nr_config)
>> +               eqbr_add_map_configs(np, map, &i, configs, nr_config);
>> +
>> +       *num_maps = map_cnt;
>> +
>> +       return 0;
>> +}
> With the library code for the standard bindings select
> GENERIC_PINMUX_FUNCTIONS and select GENERIC_PINCONF
> most of the above goes away as well.

Agree, clear about it now. All this goes away with GENERIC_PINCONF.
Not yet sure about GENERIC_PINMUX_FUNCTIONS. Need to test if generic
pinmux functions are ok to use for us. Seems not many driveruse
generic pinmux presently.

>> +static void eqbr_dt_free_map(struct pinctrl_dev *pctldev,
>> +                            struct pinctrl_map *map, unsigned int num_maps)
>> +{
>> +       struct intel_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
>> +       int i;
>> +
>> +       for (i = 0; i < num_maps; i++)
>> +               if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
>> +                       devm_kfree(pctl->dev, map[i].data.configs.configs);
>> +       devm_kfree(pctl->dev, map);
>> +}
> In this case I think you can use the library function
> pinctrl_utils_free_map() just as is.

Yup, thanks.

> Now I ran out of time, but the generic advice is to use
> library code and standard bindings as much as you can
> and all will shrink down considerably. Start with the
> above pointers and I will look closer after that!
>
> Yours,
> Linus Walleij

Regards,
Rahul

On Thu, Oct 10, 2019 at 6:35 AM Tanwar, Rahul
<rahul.tanwar@linux.intel.com> wrote:

> My understanding is
> that GPIO_GENERIC can support a max of 64 consecutive bits representing
> GPIO lines.

Correct, it also demand that all GPIOs are accessed in a single
register of 8, 16, 32 or 64 bits.

> We have more than 100 GPIO's and they are spread out across
> 4 different banks with non consecutive registers i.e. DATA_IN_0~31@offset0x0,
> DATA_IN_32~63@offset0x100 and so on. In other words, i think we can not
> support memory mapped GPIO controller.

But why can't you just create one device and thus one gpio_chip
per bank?

This is what most drivers do, it also often makes things easier.

The main reason for not doing this is usually something like
that some registers are shared inbetween the banks, or their
registers are mingled in the same MMIO region or so.

If they are just in different memory chunks then create
one device per bank, and all gets much simpler.

> As mentioned above, we cannot use GPIO_GENERIC. And there was no specific
> reason/motivation for us to use regmap-mmio because most of the driver's
> that we referenced were using readl()/write(). Do you recommend us to remove
> readl()/writel() and switch to regmap-mmio based design ?

I don't really know, usually whatever makes for the simplest
code. But check if you can use GPIO_GENERIC first.

> Just to clarify, we have 4 different GPIO banks and each GPIO bank is
> implemented as a separate gpio_chip. So we have 4 instances of gpio_desc
> each one having its own gpio_chip. What i mean to say is that gpio_desc
> is actually not a per-line state container, its a per gpio_chip container.

As you're already creating one gpio_chip per bank using GPIO_GENERIC
should be simple.

Just reference the memory cells directly when calling bgpio_init() for each
mmio address, you're already half done.

Yours,
Linus Walleij