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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191108114058.GE32742@smile.fi.intel.com>
Date:   Fri, 8 Nov 2019 13:40:58 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Rahul Tanwar <rahul.tanwar@...ux.intel.com>
Cc:     linus.walleij@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh@...nel.org, qi-ming.wu@...el.com,
        yixin.zhu@...ux.intel.com, cheol.yong.kim@...el.com
Subject: Re: [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for
 a new SoC

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