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

From: Andy Shevchenko
Date: Fri Nov 08 2019 - 06:41:18 EST


On Fri, Nov 08, 2019 at 05:42:22PM +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.
>
> 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.

> + 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?

> +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.

> + 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?

> +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.

> + 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.

> + *idx = i;
> + return true;
> + }
> + }
> +
> + return false;
> +}

> + switch (op) {
> + case OP_COUNT_NR_FUNCS:

case goes usually on the same column as switch.

> + 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.

> + j < funcs[fid].nr_groups;
> + j++) {
> + if (!funcs[fid].groups[j])
> + break;
> + }
> + funcs[fid].groups[j] = prop->value;
> + }
> + break;
> +
> + default:
> + return -EINVAL;

> +

Redundant blank line.

> + }

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

if (!foo)
continue;
?

> + funcs[i].groups = devm_kcalloc(dev, funcs[i].nr_groups,
> + sizeof(*(funcs[i].groups)),
> + GFP_KERNEL);
> + if (!funcs[i].groups)
> + return -ENOMEM;
> + }
> + }

--
With Best Regards,
Andy Shevchenko