Re: [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC

Hi Andy,

On 8/11/2019 7:40 PM, Andy Shevchenko wrote:

On Fri, Nov 08, 2019 at 05:42:22PM +0800, Rahul Tanwar wrote:

quoted

Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which
controls pin multiplexing & configuration including GPIO functions selection &
GPIO attributes configuration.

This IP is not based on & does not have anything in common with Chassis
specification. The pinctrl drivers under pinctrl/intel/* are all based upon
Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use
pinctrl framework under pinctrl/intel/* and it requires a separate new driver.

Add a new GPIO & pin control framework based driver for this IP.
+static void eqbr_gpio_enable_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct eqbr_gpio_ctrl *gctrl = gpiochip_get_data(gc);
+	unsigned int offset = irqd_to_hwirq(d);
+	unsigned long flags;
+
+	gc->direction_input(gc, offset);

Does this any IO?
If so, between above and below a window of possible race.
Ditto for all other functions that do something similar.

gpio-mmio lib uses its own spin lock when it does IO in gc->direction_input()
And that would set pin direction as input (hw ensures that the pin is not
driven high when set direction) while below will enable interrupton that
pin. I do not see any possible race condition in that..

quoted

+	raw_spin_lock_irqsave(&gctrl->lock, flags);
+	writel(BIT(offset), gctrl->membase + GPIO_IRNRNSET);
+	raw_spin_unlock_irqrestore(&gctrl->lock, flags);
+}
+		ret = bgpio_init(&gctrl->chip, dev, gctrl->bank->nr_pins / 8,
+				 gctrl->membase + GPIO_IN,
+				 gctrl->membase + GPIO_OUTSET,
+				 gctrl->membase + GPIO_OUTCLR,
+				 gctrl->membase + GPIO_DIR,
+				 NULL,
+				 0);

One line?

Sure, missed it.

quoted

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

From style point of view is better to have
	... foo = offset / X;
	... bar = offset % X;

directly in definition block. Moreover, for example, on x86 it might be
converted by compiler to single idiv call in assembly that returns in
(eax, edx) both values at once.

Ok, i will change it like that but will have to introduce one more variable.

quoted

+	val = PARSE_DRV_CURRENT(val, offset);
+
+	return val;
+}
+	if (!(bank->aval_pinmap & BIT(offset))) {
+		dev_err(pctl->dev,
+			"PIN: %u is not valid, pinbase: %u, bitmap: %u\n",
+			pin, bank->pin_base, bank->aval_pinmap);
+		return -ENODEV;
+	}

Looks like aval_pinmap is NIH of valid_mask bitmap in GPIO library.
Can you check if it suits your purposes?

I did check about it earlier & now as well. It doesn't suit our purpose.
aval_pinmapin the driver indicates whether pad control of this pad is
available or not. It's for all pins irrespective of if it is used as a
gpio or not. Whereas, valid_mask bitmap of GPIO library is just to
indicate if that gpio line is valid or not. valid_mask would have been
useful if this driver was purely a GPIO driver.

quoted

+static bool is_func_exist(struct eqbr_pmx_func *funcs, const char *name,
+			 unsigned int nr_funcs, unsigned int *idx)
+{
+	int i;
+
+	if (!funcs || !nr_funcs)
+		return false;

nr_funcs check is a dup of the one in for loop.

Sure, noted.

quoted

+	for (i = 0; i < nr_funcs; i++) {
+		if (funcs[i].name && (strcmp(funcs[i].name, name) == 0) ) {

An extra space, but you may use !strcmp() and make it shorter without redundant
parentheses.

Well noted, thanks.

quoted

+			*idx = i;
+			return true;
+		}
+	}
+
+	return false;
+}
+		switch (op) {
+			case OP_COUNT_NR_FUNCS:

case goes usually on the same column as switch.

Sure, will change, didn't know about it. Thanks.

quoted

+				if (!is_func_exist(funcs, fn_name,
+						   *nr_funcs, &fid))
+					*nr_funcs = *nr_funcs + 1;
+				break;
+
+			case OP_ADD_FUNCS:
+				if (!is_func_exist(funcs, fn_name,
+						   *nr_funcs, &fid))
+					funcs[i].name = fn_name;
+				break;
+
+			case OP_COUNT_NR_FUNC_GRPS:
+				if (is_func_exist(funcs, fn_name,
+						  *nr_funcs, &fid))
+					funcs[fid].nr_groups++;
+				break;
+
+			case OP_ADD_FUNC_GRPS:
+				if (is_func_exist(funcs, fn_name,
+						  *nr_funcs, &fid)) {
+					for(j=0;

Other style issueS.

Will fix in v6, thanks.

quoted

+		}
+	for (i = 0; i < nr_funcs; i++) {
+		if (funcs[i].nr_groups) {

	if (!foo)
		continue;
?

Sure, will change. Thanks.

Regards,
Rahul