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:
>> 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..
>> + 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.
>> +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.
>> + 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.
>> +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.
>> + 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.
>> + *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.
>> + 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.
>> + }
>> + for (i = 0; i < nr_funcs; i++) {
>> + if (funcs[i].nr_groups) {
> if (!foo)
> continue;
> ?
Sure, will change. Thanks.
Regards,
Rahul
