Re: [PATCH 04/14] OF: pinctrl: MIPS: lantiq: implement lantiq/xway pinctrl support

[John Crispin <blogic@openwrt.org>]

Hi Linus,

Thanks. I will fold your comments with Stephen's into a V2 and resend it.

> Shouldn't this be:
>
> depends on SOC_TYPE_XWAY
> depends on PINCTRL_LANTIQ
>
> ?
>
> So LANTIQ selects it's pinctrl driver, the the xway SoC
> selects its driver and they both are dependent on their
> respective system.
>
The whole select/depends part is broken. I will clean this up properly

>> diff --git a/drivers/pinctrl/pinctrl-lantiq.h b/drivers/pinctrl/pinctrl-lantiq.h
>> +#define ARRAY_AND_SIZE(x)      (x), ARRAY_SIZE(x)
I was actually considering to drop this. Having a "," inside a macro is
a bit ugly.
It leads to the calling code invoking the function with N-1 parameters,
although the function takes N parameters. I find this a bit
confusing/inconsistent.


>> +/* macros to help us access the registers */
>> +#define gpio_getbit(m, r, p)   (!!(ltq_r32(m + r) & (1 << p)))
>> +#define gpio_setbit(m, r, p)   ltq_w32_mask(0, (1 << p), m + r)
>> +#define gpio_clearbit(m, r, p) ltq_w32_mask((1 << p), 0, m + r)
> So what makes this arch so fantastic that it needs its own read/write functions?
> (Just curious...)
Nothing. Its a legacy macro from a few years ago when I first added
lantiq support inside openwrt. I personally like the macro. I use it
wherever I access lantiq registers.
When accessing generic memory ranges, as in the nand driver, I use
writeb() and co.

Matter of taste really. I would prefer to keep it this way if there are
no guidelines against it.

>> +/* ---------  gpio_chip related code --------- */
>> +
>> +int gpio_to_irq(unsigned int gpio)
>> +{
>> +       return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(gpio_to_irq);
>> +
>> +int irq_to_gpio(unsigned int gpio)
>> +{
>> +       return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(irq_to_gpio);
> Can't you just leave them undefined?

I just checked how ARM does it. They use
   arch/arm/include/asm/gpio.h

Let me talk to Ralf about this and make a MIPS version of said header file.

Thanks,
John