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
