[RFC 0/5] pinctrl: SPEAr Updates

[RFC 0/5] pinctrl: SPEAr Updates

[Viresh Kumar]

Hi Linus,

This patchset mainly focuses on providing support to configure SPEAr pads as
gpios. For this i have added a gpiolib based driver and few changes in other
SPEAr pinctrl drivers.

It also contains minor fixes.

I haven't tested it till now, only compile tested.
I wanted to get a first level of review, so posting this set.

One thing currently broken is: Passing gpio base via DT.

Viresh Kumar (5):
  pinctrl: SPEAr: Don't set all non muxreg bits on pinctrl_disable
  pinctrl: SPEAr1310: Fix pin numbers for clcd_high_res
  pinctrl: SPEAr: Add plgpio driver
  pinctrl: SPEAr: Add gpio ranges support
  SPEAr: Add plgpio node in device tree dtsi files

 arch/arm/boot/dts/spear1310.dtsi          |   16 +
 arch/arm/boot/dts/spear1340.dtsi          |   15 +
 arch/arm/boot/dts/spear310.dtsi           |   14 +
 arch/arm/boot/dts/spear320.dtsi           |   15 +
 drivers/pinctrl/spear/Kconfig             |   11 +
 drivers/pinctrl/spear/Makefile            |    1 +
 drivers/pinctrl/spear/pinctrl-plgpio.c    |  736 ++++++++++++++++++++++++++
 drivers/pinctrl/spear/pinctrl-spear.c     |   98 +++-
 drivers/pinctrl/spear/pinctrl-spear.h     |   15 +
 drivers/pinctrl/spear/pinctrl-spear1310.c |  813 ++++++++++++++++++++++++++++-
 drivers/pinctrl/spear/pinctrl-spear1340.c |    9 +
 drivers/pinctrl/spear/pinctrl-spear300.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear310.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear320.c  |    2 +
 drivers/pinctrl/spear/pinctrl-spear3xx.c  |  121 +++++
 15 files changed, 1857 insertions(+), 13 deletions(-)
 create mode 100644 drivers/pinctrl/spear/pinctrl-plgpio.c

-- 
1.7.9

[RFC 4/5] pinctrl: SPEAr: Add gpio ranges support

[Linus Walleij]

On Fri, Apr 27, 2012 at 1:43 PM, Viresh Kumar <viresh.kumar@st.com> wrote:

> Most of SPEAr SoCs, which support pinctrl, can configure & use pads as gpio.
> This patch gpio enable support for SPEAr pinctrl drivers.

OK...

> +static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
> + ? ? ? {
> + ? ? ? ? ? ? ? .pins = i2c0_pins,
> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(i2c0_pins),
> + ? ? ? ? ? ? ? .muxreg = {
> + ? ? ? ? ? ? ? ? ? ? ? .reg = PAD_FUNCTION_EN_0,
> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_I2C0_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? .val = 0,
> + ? ? ? ? ? ? ? },
> + ? ? ? }, {
> + ? ? ? ? ? ? ? .pins = ssp0_pins,
> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(ssp0_pins),
> + ? ? ? ? ? ? ? .muxreg = {
> + ? ? ? ? ? ? ? ? ? ? ? .reg = PAD_FUNCTION_EN_0,
> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_SSP0_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? .val = 0,
> + ? ? ? ? ? ? ? },
> + ? ? ? }, {

(...)

Hm first I wonder what i2c0 and ssp0 have to do with GPIO...
Well whatever, maybe split out a special patch just adding the
groups?

Please cut down this code by using a clever macro:

#define SPEAR_PCTL_GRP(a,b) \
{ \
    .pins =  a##_pins, \
    .npins = ARRAY_SIZE(a##_pins), \
    .muxreg = { \
        .reg = PAD_FUNCTION_EN_2, \
        .mask PMX_##b##_MASK, \
        .val = 0, \
    }, \
}

Then:

static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
    SPEAR_PCTL_GRP(i2c0, I2C0),
    SPEAR_PCTL_GRP(ssp0, SSP0),
    SPEAR_PCTL_GRP(ssp0_cs0, SSP0_CS0),
    (...)
};

(You get the picture.)

> +static struct spear_gpio_pingroup spear3xx_gpio_pingroup[] = {
> + ? ? ? {
> + ? ? ? ? ? ? ? .pins = firda_pins,
> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(firda_pins),
> + ? ? ? ? ? ? ? .muxreg = {
> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_FIRDA_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? .val = PMX_FIRDA_MASK,
> + ? ? ? ? ? ? ? },
> + ? ? ? }, {
> + ? ? ? ? ? ? ? .pins = i2c_pins,
> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(i2c_pins),
> + ? ? ? ? ? ? ? .muxreg = {
> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_I2C_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? .val = PMX_I2C_MASK,
> + ? ? ? ? ? ? ? },
> + ? ? ? }, {

And do the same thing here.

Yours,
Linus Walleij

[RFC 0/5] pinctrl: SPEAr Updates

[Linus Walleij]

On Fri, Apr 27, 2012 at 1:43 PM, Viresh Kumar <viresh.kumar@st.com> wrote:

> This patchset mainly focuses on providing support to configure SPEAr pads as
> gpios. For this i have added a gpiolib based driver and few changes in other
> SPEAr pinctrl drivers.
>
> It also contains minor fixes.
>
> I haven't tested it till now, only compile tested.
> I wanted to get a first level of review, so posting this set.
>
> One thing currently broken is: Passing gpio base via DT.

Overall this looks good, just want you to use macros as commented
in one patch.

Yours,
Linus Walleij

[RFC 4/5] pinctrl: SPEAr: Add gpio ranges support

[Arnd Bergmann]

On Monday 07 May 2012, Linus Walleij wrote:
> Hm first I wonder what i2c0 and ssp0 have to do with GPIO...
> Well whatever, maybe split out a special patch just adding the
> groups?
> 
> Please cut down this code by using a clever macro:
> 
> #define SPEAR_PCTL_GRP(a,b) \
> { \
>     .pins =  a##_pins, \
>     .npins = ARRAY_SIZE(a##_pins), \
>     .muxreg = { \
>         .reg = PAD_FUNCTION_EN_2, \
>         .mask PMX_##b##_MASK, \
>         .val = 0, \
>     }, \
> }
> 
> Then:
> 
> static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
>     SPEAR_PCTL_GRP(i2c0, I2C0),
>     SPEAR_PCTL_GRP(ssp0, SSP0),
>     SPEAR_PCTL_GRP(ssp0_cs0, SSP0_CS0),
>     (...)
> };
> 
> (You get the picture.)

If you allow me to give my 2 cents, I would recommend the exact opposite
in general: resist the urge to use complex macros for everything!

When that gets too hard, please at least use macros that do not concatenate
C identifies because that makes it really hard to grep for where a constant
gets used. 

A more acceptable macro would be 

#define SPEAR_PCTL_GRP(_pins, _mask) \
{ \
	.pins =  _pins, \
	.npins = ARRAY_SIZE(_pins), \
	.muxreg = { \
	        .reg = PAD_FUNCTION_EN_2, \
	        .mask _val, \
	        .val = 0, \
	}, \
}

but open-coding is generally ok too. Note that you can sometimes save a lot
of lines if you don't first define all those constants as macros but
treat a table like this as the place where you put the raw numbers
from the data sheet, if that the only code location in which they are
used.

	Arnd

[RFC 4/5] pinctrl: SPEAr: Add gpio ranges support

[viresh kumar]

On Mon, May 7, 2012 at 6:08 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Apr 27, 2012 at 1:43 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
>> +static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
>> + ? ? ? {
>> + ? ? ? ? ? ? ? .pins = i2c0_pins,
>> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(i2c0_pins),
>> + ? ? ? ? ? ? ? .muxreg = {
>> + ? ? ? ? ? ? ? ? ? ? ? .reg = PAD_FUNCTION_EN_0,
>> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_I2C0_MASK,
>> + ? ? ? ? ? ? ? ? ? ? ? .val = 0,
>> + ? ? ? ? ? ? ? },
>> + ? ? ? }, {
>> + ? ? ? ? ? ? ? .pins = ssp0_pins,
>> + ? ? ? ? ? ? ? .npins = ARRAY_SIZE(ssp0_pins),
>> + ? ? ? ? ? ? ? .muxreg = {
>> + ? ? ? ? ? ? ? ? ? ? ? .reg = PAD_FUNCTION_EN_0,
>> + ? ? ? ? ? ? ? ? ? ? ? .mask = PMX_SSP0_MASK,
>> + ? ? ? ? ? ? ? ? ? ? ? .val = 0,
>> + ? ? ? ? ? ? ? },
>> + ? ? ? }, {
>
> Hm first I wonder what i2c0 and ssp0 have to do with GPIO...

I didn't wanted to create new Arrays for exactly the same pins. so used
earlier ones that belonged to peripherals.

> Well whatever, maybe split out a special patch just adding the
> groups?

If i don't use them in that patch, we will get compilation warnings after it.

> Please cut down this code by using a clever macro:
>
> #define SPEAR_PCTL_GRP(a,b) \
> { \
> ? ?.pins = ?a##_pins, \
> ? ?.npins = ARRAY_SIZE(a##_pins), \
> ? ?.muxreg = { \
> ? ? ? ?.reg = PAD_FUNCTION_EN_2, \
> ? ? ? ?.mask PMX_##b##_MASK, \
> ? ? ? ?.val = 0, \
> ? ?}, \
> }
>
> Then:
>
> static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
> ? ?SPEAR_PCTL_GRP(i2c0, I2C0),
> ? ?SPEAR_PCTL_GRP(ssp0, SSP0),
> ? ?SPEAR_PCTL_GRP(ssp0_cs0, SSP0_CS0),
> ? ?(...)
> };
>
> (You get the picture.)

Yes. Will do.

[RFC 4/5] pinctrl: SPEAr: Add gpio ranges support

[viresh kumar]

On Mon, May 7, 2012 at 8:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 07 May 2012, Linus Walleij wrote:
>> Hm first I wonder what i2c0 and ssp0 have to do with GPIO...
>> Well whatever, maybe split out a special patch just adding the
>> groups?
>>
>> Please cut down this code by using a clever macro:
>>
>> #define SPEAR_PCTL_GRP(a,b) \
>> { \
>> ? ? .pins = ?a##_pins, \
>> ? ? .npins = ARRAY_SIZE(a##_pins), \
>> ? ? .muxreg = { \
>> ? ? ? ? .reg = PAD_FUNCTION_EN_2, \
>> ? ? ? ? .mask PMX_##b##_MASK, \
>> ? ? ? ? .val = 0, \
>> ? ? }, \
>> }
>>
>> Then:
>>
>> static struct spear_gpio_pingroup spear1310_gpio_pingroup[] = {
>> ? ? SPEAR_PCTL_GRP(i2c0, I2C0),
>> ? ? SPEAR_PCTL_GRP(ssp0, SSP0),
>> ? ? SPEAR_PCTL_GRP(ssp0_cs0, SSP0_CS0),
>> ? ? (...)
>> };
>>
>> (You get the picture.)
>
> If you allow me to give my 2 cents, I would recommend the exact opposite
> in general: resist the urge to use complex macros for everything!
>
> When that gets too hard, please at least use macros that do not concatenate
> C identifies because that makes it really hard to grep for where a constant
> gets used.
>
> A more acceptable macro would be
>
> #define SPEAR_PCTL_GRP(_pins, _mask) \
> { \
> ? ? ? ?.pins = ?_pins, \
> ? ? ? ?.npins = ARRAY_SIZE(_pins), \
> ? ? ? ?.muxreg = { \
> ? ? ? ? ? ? ? ?.reg = PAD_FUNCTION_EN_2, \
> ? ? ? ? ? ? ? ?.mask _val, \
> ? ? ? ? ? ? ? ?.val = 0, \
> ? ? ? ?}, \
> }
>
> but open-coding is generally ok too. Note that you can sometimes save a lot
> of lines if you don't first define all those constants as macros but
> treat a table like this as the place where you put the raw numbers
> from the data sheet, if that the only code location in which they are
> used.

Hmm. Will try to do something better.

--
viresh

[RFC 4/5] pinctrl: SPEAr: Add gpio ranges support

[Linus Walleij]

On Mon, May 7, 2012 at 5:58 PM, viresh kumar <viresh.linux@gmail.com> wrote:
> On Mon, May 7, 2012 at 8:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> but open-coding is generally ok too. Note that you can sometimes save a lot
>> of lines if you don't first define all those constants as macros but
>> treat a table like this as the place where you put the raw numbers
>> from the data sheet, if that the only code location in which they are
>> used.
>
> Hmm. Will try to do something better.

No big deal, you can keep the table as long as I don't get Torvalds backfire
on me for introducing too many lines of code. I'm a bit sensitive about that
but it's not like it's super-important.

I've come to accept the fact that pin controller have many lines of code,
since they bascially contain the tables that other platforms put in BIOS
firmware, and these systems typically don't have BIOS:es so...

Thanks,
Linus Walleij