Re: [PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Jesper Nilsson]

On Thu, Mar 30, 2017 at 02:07:33PM +0200, Linus Walleij wrote:
> On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson  
> wrote:
> 
> > Add pinctrl driver support for the Axis ARTPEC-6 SoC.
> > There are only some pins that actually have different
> > functions available, but all can control bias (pull-up/-down)
> > and drive strength.
> > Code originally written by Chris Paterson.
> >
> > Signed-off-by: Jesper Nilsson 
> 
> Very good start, but look at this:
> 
> > +#define ARTPEC6_PINMUX_P0_0_CTRL   0x000
> > +#define ARTPEC6_PINMUX_P0_1_CTRL   0x004
> > +#define ARTPEC6_PINMUX_P0_2_CTRL   0x008
> > +#define ARTPEC6_PINMUX_P0_3_CTRL   0x00C
> > +#define ARTPEC6_PINMUX_P0_4_CTRL   0x010
> > +#define ARTPEC6_PINMUX_P0_5_CTRL   0x014
> > +#define ARTPEC6_PINMUX_P0_6_CTRL   0x018
> > +#define ARTPEC6_PINMUX_P0_7_CTRL   0x01C
> > +#define ARTPEC6_PINMUX_P0_8_CTRL   0x020
> > +#define ARTPEC6_PINMUX_P0_9_CTRL   0x024
> > +#define ARTPEC6_PINMUX_P0_10_CTRL  0x028
> > +#define ARTPEC6_PINMUX_P0_11_CTRL  0x02C
> > +#define ARTPEC6_PINMUX_P0_12_CTRL  0x030
> > +#define ARTPEC6_PINMUX_P0_13_CTRL  0x034
> > +#define ARTPEC6_PINMUX_P0_14_CTRL  0x038
> > +#define ARTPEC6_PINMUX_P0_15_CTRL  0x03C
> 
> It's pretty clear that the stride is always 4 bytes is it not.

Agreed.

> > +static const unsigned int pin_regs[] = {
> > +   ARTPEC6_PINMUX_P0_0_CTRL,   /* Pin 0 */
> > +   ARTPEC6_PINMUX_P0_1_CTRL,
> > +   ARTPEC6_PINMUX_P0_2_CTRL,
> > +   ARTPEC6_PINMUX_P0_3_CTRL,
> > +   ARTPEC6_PINMUX_P0_4_CTRL,
> > +   ARTPEC6_PINMUX_P0_5_CTRL,
> > +   ARTPEC6_PINMUX_P0_6_CTRL,
> > +   ARTPEC6_PINMUX_P0_7_CTRL,
> > +   ARTPEC6_PINMUX_P0_8_CTRL,
> > +   ARTPEC6_PINMUX_P0_9_CTRL,
> > +   ARTPEC6_PINMUX_P0_10_CTRL,
> > +   ARTPEC6_PINMUX_P0_11_CTRL,
> > +   ARTPEC6_PINMUX_P0_12_CTRL,
> > +   ARTPEC6_PINMUX_P0_13_CTRL,
> > +   ARTPEC6_PINMUX_P0_14_CTRL,
> > +   ARTPEC6_PINMUX_P0_15_CTRL,
> 
> The oceans of redundant information are rising ;)
> 
> > +static const unsigned int uart0_regs0[] = {
> > +   ARTPEC6_PINMUX_P1_0_CTRL,
> > +   ARTPEC6_PINMUX_P1_1_CTRL,
> > +   ARTPEC6_PINMUX_P1_2_CTRL,
> > +   ARTPEC6_PINMUX_P1_3_CTRL,
> > +   ARTPEC6_PINMUX_P1_4_CTRL,
> > +   ARTPEC6_PINMUX_P1_5_CTRL,
> > +   ARTPEC6_PINMUX_P1_6_CTRL,
> > +   ARTPEC6_PINMUX_P1_7_CTRL,
> > +   ARTPEC6_PINMUX_P1_8_CTRL,
> > +   ARTPEC6_PINMUX_P1_9_CTRL,
> > +};
> 
> And rising.

:-)

> > +   {
> > +   .name = "uart0grp0",
> > +   .pins = uart0_pins0,
> > +   .num_pins = ARRAY_SIZE(uart0_pins0),
> > +   .reg_offsets = uart0_regs0,
> > +   .num_regs = ARRAY_SIZE(uart0_regs0),
> > +   .config = ARTPEC6_CONFIG_1,
> > +   },
> 
> So what if the struct artpec6_pin_group...
> 
> > +struct artpec6_pin_group {
> > +   const char *name;
> > +   const unsigned int *pins;
> > +   const unsigned int num_pins;
> > +   const unsigned int *reg_offsets;
> > +   const unsigned int num_regs;
> > +   unsigned char  config;
> > +};
> 
> Instead of reg_offsets had reg_offset_base, and you just
> calculate it with base + pin * 4 when you need it?

Yes, that would be much clearer.

> I also highly suspect that num_pins and num_regs above
> are exactly the same number in all cases, correct? You
> probably only need num_pins.

Agreed.

> > +static struct artpec6_pmx_func artpec6_pmx_functions[] = {
> 
> Needs const
> 
> > +static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
> > +   unsigned int function, unsigned int 
> > group,
> > +   bool enable)
> > +{
> > +   unsigned int regval, val;
> > +   int i;
> > +   const unsigned int *pmx_regs;
> > +   struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +   pmx_regs = artpec6_pin_groups[group].reg_offsets;
> > +
> > +   for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
> > +   /* Ports 4-8 do not have a SEL field and are always 
> > selected */
> > +   if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
> > +   continue;
> > +
> > +   if (!strcmp(artpec6_pmx_get_fname(pctldev, function), 
> > "gpio")) {
> > +   /* GPIO is always config 0 */
> > +   val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   } else {
> > +   if (enable)
> > +   val = artpec6_pin_groups[group].config
> > +   << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   else
> > +   val = ARTPEC6_CONFIG_0
> > +   << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   }
> > +

Re: [PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Jesper Nilsson]

On Thu, Mar 30, 2017 at 02:07:33PM +0200, Linus Walleij wrote:
> On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson  
> wrote:
> 
> > Add pinctrl driver support for the Axis ARTPEC-6 SoC.
> > There are only some pins that actually have different
> > functions available, but all can control bias (pull-up/-down)
> > and drive strength.
> > Code originally written by Chris Paterson.
> >
> > Signed-off-by: Jesper Nilsson 
> 
> Very good start, but look at this:
> 
> > +#define ARTPEC6_PINMUX_P0_0_CTRL   0x000
> > +#define ARTPEC6_PINMUX_P0_1_CTRL   0x004
> > +#define ARTPEC6_PINMUX_P0_2_CTRL   0x008
> > +#define ARTPEC6_PINMUX_P0_3_CTRL   0x00C
> > +#define ARTPEC6_PINMUX_P0_4_CTRL   0x010
> > +#define ARTPEC6_PINMUX_P0_5_CTRL   0x014
> > +#define ARTPEC6_PINMUX_P0_6_CTRL   0x018
> > +#define ARTPEC6_PINMUX_P0_7_CTRL   0x01C
> > +#define ARTPEC6_PINMUX_P0_8_CTRL   0x020
> > +#define ARTPEC6_PINMUX_P0_9_CTRL   0x024
> > +#define ARTPEC6_PINMUX_P0_10_CTRL  0x028
> > +#define ARTPEC6_PINMUX_P0_11_CTRL  0x02C
> > +#define ARTPEC6_PINMUX_P0_12_CTRL  0x030
> > +#define ARTPEC6_PINMUX_P0_13_CTRL  0x034
> > +#define ARTPEC6_PINMUX_P0_14_CTRL  0x038
> > +#define ARTPEC6_PINMUX_P0_15_CTRL  0x03C
> 
> It's pretty clear that the stride is always 4 bytes is it not.

Agreed.

> > +static const unsigned int pin_regs[] = {
> > +   ARTPEC6_PINMUX_P0_0_CTRL,   /* Pin 0 */
> > +   ARTPEC6_PINMUX_P0_1_CTRL,
> > +   ARTPEC6_PINMUX_P0_2_CTRL,
> > +   ARTPEC6_PINMUX_P0_3_CTRL,
> > +   ARTPEC6_PINMUX_P0_4_CTRL,
> > +   ARTPEC6_PINMUX_P0_5_CTRL,
> > +   ARTPEC6_PINMUX_P0_6_CTRL,
> > +   ARTPEC6_PINMUX_P0_7_CTRL,
> > +   ARTPEC6_PINMUX_P0_8_CTRL,
> > +   ARTPEC6_PINMUX_P0_9_CTRL,
> > +   ARTPEC6_PINMUX_P0_10_CTRL,
> > +   ARTPEC6_PINMUX_P0_11_CTRL,
> > +   ARTPEC6_PINMUX_P0_12_CTRL,
> > +   ARTPEC6_PINMUX_P0_13_CTRL,
> > +   ARTPEC6_PINMUX_P0_14_CTRL,
> > +   ARTPEC6_PINMUX_P0_15_CTRL,
> 
> The oceans of redundant information are rising ;)
> 
> > +static const unsigned int uart0_regs0[] = {
> > +   ARTPEC6_PINMUX_P1_0_CTRL,
> > +   ARTPEC6_PINMUX_P1_1_CTRL,
> > +   ARTPEC6_PINMUX_P1_2_CTRL,
> > +   ARTPEC6_PINMUX_P1_3_CTRL,
> > +   ARTPEC6_PINMUX_P1_4_CTRL,
> > +   ARTPEC6_PINMUX_P1_5_CTRL,
> > +   ARTPEC6_PINMUX_P1_6_CTRL,
> > +   ARTPEC6_PINMUX_P1_7_CTRL,
> > +   ARTPEC6_PINMUX_P1_8_CTRL,
> > +   ARTPEC6_PINMUX_P1_9_CTRL,
> > +};
> 
> And rising.

:-)

> > +   {
> > +   .name = "uart0grp0",
> > +   .pins = uart0_pins0,
> > +   .num_pins = ARRAY_SIZE(uart0_pins0),
> > +   .reg_offsets = uart0_regs0,
> > +   .num_regs = ARRAY_SIZE(uart0_regs0),
> > +   .config = ARTPEC6_CONFIG_1,
> > +   },
> 
> So what if the struct artpec6_pin_group...
> 
> > +struct artpec6_pin_group {
> > +   const char *name;
> > +   const unsigned int *pins;
> > +   const unsigned int num_pins;
> > +   const unsigned int *reg_offsets;
> > +   const unsigned int num_regs;
> > +   unsigned char  config;
> > +};
> 
> Instead of reg_offsets had reg_offset_base, and you just
> calculate it with base + pin * 4 when you need it?

Yes, that would be much clearer.

> I also highly suspect that num_pins and num_regs above
> are exactly the same number in all cases, correct? You
> probably only need num_pins.

Agreed.

> > +static struct artpec6_pmx_func artpec6_pmx_functions[] = {
> 
> Needs const
> 
> > +static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
> > +   unsigned int function, unsigned int 
> > group,
> > +   bool enable)
> > +{
> > +   unsigned int regval, val;
> > +   int i;
> > +   const unsigned int *pmx_regs;
> > +   struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +   pmx_regs = artpec6_pin_groups[group].reg_offsets;
> > +
> > +   for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
> > +   /* Ports 4-8 do not have a SEL field and are always 
> > selected */
> > +   if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
> > +   continue;
> > +
> > +   if (!strcmp(artpec6_pmx_get_fname(pctldev, function), 
> > "gpio")) {
> > +   /* GPIO is always config 0 */
> > +   val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   } else {
> > +   if (enable)
> > +   val = artpec6_pin_groups[group].config
> > +   << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   else
> > +   val = ARTPEC6_CONFIG_0
> > +   << ARTPEC6_PINMUX_SEL_SHIFT;
> > +   }
> > +
> > +   regval = readl(pmx->base + 

Re: [PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Linus Walleij]

On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson  wrote:

> Add pinctrl driver support for the Axis ARTPEC-6 SoC.
> There are only some pins that actually have different
> functions available, but all can control bias (pull-up/-down)
> and drive strength.
> Code originally written by Chris Paterson.
>
> Signed-off-by: Jesper Nilsson 

Very good start, but look at this:


> +#define ARTPEC6_PINMUX_P0_0_CTRL   0x000
> +#define ARTPEC6_PINMUX_P0_1_CTRL   0x004
> +#define ARTPEC6_PINMUX_P0_2_CTRL   0x008
> +#define ARTPEC6_PINMUX_P0_3_CTRL   0x00C
> +#define ARTPEC6_PINMUX_P0_4_CTRL   0x010
> +#define ARTPEC6_PINMUX_P0_5_CTRL   0x014
> +#define ARTPEC6_PINMUX_P0_6_CTRL   0x018
> +#define ARTPEC6_PINMUX_P0_7_CTRL   0x01C
> +#define ARTPEC6_PINMUX_P0_8_CTRL   0x020
> +#define ARTPEC6_PINMUX_P0_9_CTRL   0x024
> +#define ARTPEC6_PINMUX_P0_10_CTRL  0x028
> +#define ARTPEC6_PINMUX_P0_11_CTRL  0x02C
> +#define ARTPEC6_PINMUX_P0_12_CTRL  0x030
> +#define ARTPEC6_PINMUX_P0_13_CTRL  0x034
> +#define ARTPEC6_PINMUX_P0_14_CTRL  0x038
> +#define ARTPEC6_PINMUX_P0_15_CTRL  0x03C

It's pretty clear that the stride is always 4 bytes is it not.


> +static const unsigned int pin_regs[] = {
> +   ARTPEC6_PINMUX_P0_0_CTRL,   /* Pin 0 */
> +   ARTPEC6_PINMUX_P0_1_CTRL,
> +   ARTPEC6_PINMUX_P0_2_CTRL,
> +   ARTPEC6_PINMUX_P0_3_CTRL,
> +   ARTPEC6_PINMUX_P0_4_CTRL,
> +   ARTPEC6_PINMUX_P0_5_CTRL,
> +   ARTPEC6_PINMUX_P0_6_CTRL,
> +   ARTPEC6_PINMUX_P0_7_CTRL,
> +   ARTPEC6_PINMUX_P0_8_CTRL,
> +   ARTPEC6_PINMUX_P0_9_CTRL,
> +   ARTPEC6_PINMUX_P0_10_CTRL,
> +   ARTPEC6_PINMUX_P0_11_CTRL,
> +   ARTPEC6_PINMUX_P0_12_CTRL,
> +   ARTPEC6_PINMUX_P0_13_CTRL,
> +   ARTPEC6_PINMUX_P0_14_CTRL,
> +   ARTPEC6_PINMUX_P0_15_CTRL,

The oceans of redundant information are rising ;)

> +static const unsigned int uart0_regs0[] = {
> +   ARTPEC6_PINMUX_P1_0_CTRL,
> +   ARTPEC6_PINMUX_P1_1_CTRL,
> +   ARTPEC6_PINMUX_P1_2_CTRL,
> +   ARTPEC6_PINMUX_P1_3_CTRL,
> +   ARTPEC6_PINMUX_P1_4_CTRL,
> +   ARTPEC6_PINMUX_P1_5_CTRL,
> +   ARTPEC6_PINMUX_P1_6_CTRL,
> +   ARTPEC6_PINMUX_P1_7_CTRL,
> +   ARTPEC6_PINMUX_P1_8_CTRL,
> +   ARTPEC6_PINMUX_P1_9_CTRL,
> +};

And rising.

> +   {
> +   .name = "uart0grp0",
> +   .pins = uart0_pins0,
> +   .num_pins = ARRAY_SIZE(uart0_pins0),
> +   .reg_offsets = uart0_regs0,
> +   .num_regs = ARRAY_SIZE(uart0_regs0),
> +   .config = ARTPEC6_CONFIG_1,
> +   },

So what if the struct artpec6_pin_group...

> +struct artpec6_pin_group {
> +   const char *name;
> +   const unsigned int *pins;
> +   const unsigned int num_pins;
> +   const unsigned int *reg_offsets;
> +   const unsigned int num_regs;
> +   unsigned char  config;
> +};

Instead of reg_offsets had reg_offset_base, and you just
calculate it with base + pin * 4 when you need it?

I also highly suspect that num_pins and num_regs above
are exactly the same number in all cases, correct? You
probably only need num_pins.

> +static struct artpec6_pmx_func artpec6_pmx_functions[] = {

Needs const

> +static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
> +   unsigned int function, unsigned int group,
> +   bool enable)
> +{
> +   unsigned int regval, val;
> +   int i;
> +   const unsigned int *pmx_regs;
> +   struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +   pmx_regs = artpec6_pin_groups[group].reg_offsets;
> +
> +   for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
> +   /* Ports 4-8 do not have a SEL field and are always selected 
> */
> +   if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
> +   continue;
> +
> +   if (!strcmp(artpec6_pmx_get_fname(pctldev, function), 
> "gpio")) {
> +   /* GPIO is always config 0 */
> +   val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
> +   } else {
> +   if (enable)
> +   val = artpec6_pin_groups[group].config
> +   << ARTPEC6_PINMUX_SEL_SHIFT;
> +   else
> +   val = ARTPEC6_CONFIG_0
> +   << ARTPEC6_PINMUX_SEL_SHIFT;
> +   }
> +
> +   regval = readl(pmx->base + pmx_regs[i]);
> +   regval &= ~ARTPEC6_PINMUX_SEL_MASK;
> +   regval |= val;
> +   writel(regval, pmx->base + pmx_regs[i]);
> +   }

So thus look can be different and include something like +=4 for the registers
for each iteration.

> --- /dev/null
> +++ 

Re: [PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Linus Walleij]

On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson  wrote:

> Add pinctrl driver support for the Axis ARTPEC-6 SoC.
> There are only some pins that actually have different
> functions available, but all can control bias (pull-up/-down)
> and drive strength.
> Code originally written by Chris Paterson.
>
> Signed-off-by: Jesper Nilsson 

Very good start, but look at this:


> +#define ARTPEC6_PINMUX_P0_0_CTRL   0x000
> +#define ARTPEC6_PINMUX_P0_1_CTRL   0x004
> +#define ARTPEC6_PINMUX_P0_2_CTRL   0x008
> +#define ARTPEC6_PINMUX_P0_3_CTRL   0x00C
> +#define ARTPEC6_PINMUX_P0_4_CTRL   0x010
> +#define ARTPEC6_PINMUX_P0_5_CTRL   0x014
> +#define ARTPEC6_PINMUX_P0_6_CTRL   0x018
> +#define ARTPEC6_PINMUX_P0_7_CTRL   0x01C
> +#define ARTPEC6_PINMUX_P0_8_CTRL   0x020
> +#define ARTPEC6_PINMUX_P0_9_CTRL   0x024
> +#define ARTPEC6_PINMUX_P0_10_CTRL  0x028
> +#define ARTPEC6_PINMUX_P0_11_CTRL  0x02C
> +#define ARTPEC6_PINMUX_P0_12_CTRL  0x030
> +#define ARTPEC6_PINMUX_P0_13_CTRL  0x034
> +#define ARTPEC6_PINMUX_P0_14_CTRL  0x038
> +#define ARTPEC6_PINMUX_P0_15_CTRL  0x03C

It's pretty clear that the stride is always 4 bytes is it not.


> +static const unsigned int pin_regs[] = {
> +   ARTPEC6_PINMUX_P0_0_CTRL,   /* Pin 0 */
> +   ARTPEC6_PINMUX_P0_1_CTRL,
> +   ARTPEC6_PINMUX_P0_2_CTRL,
> +   ARTPEC6_PINMUX_P0_3_CTRL,
> +   ARTPEC6_PINMUX_P0_4_CTRL,
> +   ARTPEC6_PINMUX_P0_5_CTRL,
> +   ARTPEC6_PINMUX_P0_6_CTRL,
> +   ARTPEC6_PINMUX_P0_7_CTRL,
> +   ARTPEC6_PINMUX_P0_8_CTRL,
> +   ARTPEC6_PINMUX_P0_9_CTRL,
> +   ARTPEC6_PINMUX_P0_10_CTRL,
> +   ARTPEC6_PINMUX_P0_11_CTRL,
> +   ARTPEC6_PINMUX_P0_12_CTRL,
> +   ARTPEC6_PINMUX_P0_13_CTRL,
> +   ARTPEC6_PINMUX_P0_14_CTRL,
> +   ARTPEC6_PINMUX_P0_15_CTRL,

The oceans of redundant information are rising ;)

> +static const unsigned int uart0_regs0[] = {
> +   ARTPEC6_PINMUX_P1_0_CTRL,
> +   ARTPEC6_PINMUX_P1_1_CTRL,
> +   ARTPEC6_PINMUX_P1_2_CTRL,
> +   ARTPEC6_PINMUX_P1_3_CTRL,
> +   ARTPEC6_PINMUX_P1_4_CTRL,
> +   ARTPEC6_PINMUX_P1_5_CTRL,
> +   ARTPEC6_PINMUX_P1_6_CTRL,
> +   ARTPEC6_PINMUX_P1_7_CTRL,
> +   ARTPEC6_PINMUX_P1_8_CTRL,
> +   ARTPEC6_PINMUX_P1_9_CTRL,
> +};

And rising.

> +   {
> +   .name = "uart0grp0",
> +   .pins = uart0_pins0,
> +   .num_pins = ARRAY_SIZE(uart0_pins0),
> +   .reg_offsets = uart0_regs0,
> +   .num_regs = ARRAY_SIZE(uart0_regs0),
> +   .config = ARTPEC6_CONFIG_1,
> +   },

So what if the struct artpec6_pin_group...

> +struct artpec6_pin_group {
> +   const char *name;
> +   const unsigned int *pins;
> +   const unsigned int num_pins;
> +   const unsigned int *reg_offsets;
> +   const unsigned int num_regs;
> +   unsigned char  config;
> +};

Instead of reg_offsets had reg_offset_base, and you just
calculate it with base + pin * 4 when you need it?

I also highly suspect that num_pins and num_regs above
are exactly the same number in all cases, correct? You
probably only need num_pins.

> +static struct artpec6_pmx_func artpec6_pmx_functions[] = {

Needs const

> +static void artpec6_pmx_select_func(struct pinctrl_dev *pctldev,
> +   unsigned int function, unsigned int group,
> +   bool enable)
> +{
> +   unsigned int regval, val;
> +   int i;
> +   const unsigned int *pmx_regs;
> +   struct artpec6_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +   pmx_regs = artpec6_pin_groups[group].reg_offsets;
> +
> +   for (i = 0; i < artpec6_pin_groups[group].num_regs; i++) {
> +   /* Ports 4-8 do not have a SEL field and are always selected 
> */
> +   if (pmx_regs[i] >= ARTPEC6_PINMUX_P4_0_CTRL)
> +   continue;
> +
> +   if (!strcmp(artpec6_pmx_get_fname(pctldev, function), 
> "gpio")) {
> +   /* GPIO is always config 0 */
> +   val = ARTPEC6_CONFIG_0 << ARTPEC6_PINMUX_SEL_SHIFT;
> +   } else {
> +   if (enable)
> +   val = artpec6_pin_groups[group].config
> +   << ARTPEC6_PINMUX_SEL_SHIFT;
> +   else
> +   val = ARTPEC6_CONFIG_0
> +   << ARTPEC6_PINMUX_SEL_SHIFT;
> +   }
> +
> +   regval = readl(pmx->base + pmx_regs[i]);
> +   regval &= ~ARTPEC6_PINMUX_SEL_MASK;
> +   regval |= val;
> +   writel(regval, pmx->base + pmx_regs[i]);
> +   }

So thus look can be different and include something like +=4 for the registers
for each iteration.

> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-artpec6.h

You don't need to have these defines in 

[PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Jesper Nilsson]

Add pinctrl driver support for the Axis ARTPEC-6 SoC.
There are only some pins that actually have different
functions available, but all can control bias (pull-up/-down)
and drive strength.
Code originally written by Chris Paterson.

Signed-off-by: Jesper Nilsson 
---
 MAINTAINERS   |1 +
 drivers/pinctrl/Kconfig   |   11 +
 drivers/pinctrl/Makefile  |1 +
 drivers/pinctrl/pinctrl-artpec6.c | 1279 +
 drivers/pinctrl/pinctrl-artpec6.h |  151 +
 5 files changed, 1443 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.c
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7563bd6..d2b2449 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1088,6 +1088,7 @@ L:linux-arm-ker...@axis.com
 F: arch/arm/mach-artpec
 F: arch/arm/boot/dts/artpec6*
 F: drivers/clk/axis
+F: drivers/pinctrl/pinctrl-artpec*
 F: Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
 
 ARM/ASPEED MACHINE SUPPORT
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..37af5e3 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -41,6 +41,17 @@ config PINCTRL_ADI2
  future processors. This option is selected automatically when specific
  machine and arch are selected to build.
 
+config PINCTRL_ARTPEC6
+bool "Axis ARTPEC-6 pin controller driver"
+depends on MACH_ARTPEC6
+select PINMUX
+select GENERIC_PINCONF
+help
+  This is the driver for the Axis ARTPEC-6 pin controller. This driver
+  supports pin function multiplexing as well as pin bias and drive
+  strength configuration. Device tree integration instructions can be
+  found in 
Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
+
 config PINCTRL_AS3722
tristate "Pinctrl and GPIO driver for ams AS3722 PMIC"
depends on MFD_AS3722 && GPIOLIB
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f43..193dbba 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCONF)   += pinconf.o
 obj-$(CONFIG_OF)   += devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)  += pinconf-generic.o
 obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o
+obj-$(CONFIG_PINCTRL_ARTPEC6)   += pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)   += pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_BF54x)+= pinctrl-adi2-bf54x.o
 obj-$(CONFIG_PINCTRL_BF60x)+= pinctrl-adi2-bf60x.o
diff --git a/drivers/pinctrl/pinctrl-artpec6.c 
b/drivers/pinctrl/pinctrl-artpec6.c
new file mode 100644
index 000..ff80c8a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-artpec6.c
@@ -0,0 +1,1279 @@
+/*
+ * Driver for the Axis ARTPEC-6 pin controller
+ *
+ * Author: Chris Paterson 
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+#include "pinctrl-artpec6.h"
+
+struct artpec6_pmx {
+   struct device  *dev;
+   struct pinctrl_dev *pctl;
+   void __iomem   *base;
+   struct pinctrl_pin_desc *pins;
+   unsigned int   num_pins;
+   struct artpec6_pin_group *pin_groups;
+   unsigned int   num_pin_groups;
+   struct artpec6_pmx_func *functions;
+   unsigned int   num_functions;
+};
+
+struct artpec6_pin_group {
+   const char *name;
+   const unsigned int *pins;
+   const unsigned int num_pins;
+   const unsigned int *reg_offsets;
+   const unsigned int num_regs;
+   unsigned char  config;
+};
+
+struct artpec6_pmx_func {
+   const char *name;
+   const char * const *groups;
+   const unsigned int num_groups;
+};
+
+/* pins */
+static struct pinctrl_pin_desc artpec6_pins[] = {
+   PINCTRL_PIN(0, "GPIO0"),
+   PINCTRL_PIN(1, "GPIO1"),
+   PINCTRL_PIN(2, "GPIO2"),
+   PINCTRL_PIN(3, "GPIO3"),
+   PINCTRL_PIN(4, "GPIO4"),
+   PINCTRL_PIN(5, "GPIO5"),
+   PINCTRL_PIN(6, "GPIO6"),
+   PINCTRL_PIN(7, "GPIO7"),
+   PINCTRL_PIN(8, "GPIO8"),
+   PINCTRL_PIN(9, "GPIO9"),
+   PINCTRL_PIN(10, "GPIO10"),
+   PINCTRL_PIN(11, "GPIO11"),
+   PINCTRL_PIN(12, "GPIO12"),
+   PINCTRL_PIN(13, "GPIO13"),
+   PINCTRL_PIN(14, "GPIO14"),
+   PINCTRL_PIN(15, "GPIO15"),
+   PINCTRL_PIN(16, "GPIO16"),
+   PINCTRL_PIN(17, "GPIO17"),
+   PINCTRL_PIN(18, "GPIO18"),
+   PINCTRL_PIN(19, "GPIO19"),
+   PINCTRL_PIN(20, "GPIO20"),
+   

[PATCH 2/3] pinctrl: Add pincontrol driver for ARTPEC-6 SoC

[Jesper Nilsson]

Add pinctrl driver support for the Axis ARTPEC-6 SoC.
There are only some pins that actually have different
functions available, but all can control bias (pull-up/-down)
and drive strength.
Code originally written by Chris Paterson.

Signed-off-by: Jesper Nilsson 
---
 MAINTAINERS   |1 +
 drivers/pinctrl/Kconfig   |   11 +
 drivers/pinctrl/Makefile  |1 +
 drivers/pinctrl/pinctrl-artpec6.c | 1279 +
 drivers/pinctrl/pinctrl-artpec6.h |  151 +
 5 files changed, 1443 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.c
 create mode 100644 drivers/pinctrl/pinctrl-artpec6.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7563bd6..d2b2449 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1088,6 +1088,7 @@ L:linux-arm-ker...@axis.com
 F: arch/arm/mach-artpec
 F: arch/arm/boot/dts/artpec6*
 F: drivers/clk/axis
+F: drivers/pinctrl/pinctrl-artpec*
 F: Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
 
 ARM/ASPEED MACHINE SUPPORT
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..37af5e3 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -41,6 +41,17 @@ config PINCTRL_ADI2
  future processors. This option is selected automatically when specific
  machine and arch are selected to build.
 
+config PINCTRL_ARTPEC6
+bool "Axis ARTPEC-6 pin controller driver"
+depends on MACH_ARTPEC6
+select PINMUX
+select GENERIC_PINCONF
+help
+  This is the driver for the Axis ARTPEC-6 pin controller. This driver
+  supports pin function multiplexing as well as pin bias and drive
+  strength configuration. Device tree integration instructions can be
+  found in 
Documentation/devicetree/bindings/pinctrl/axis,artpec6-pinctrl.txt
+
 config PINCTRL_AS3722
tristate "Pinctrl and GPIO driver for ams AS3722 PMIC"
depends on MFD_AS3722 && GPIOLIB
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f43..193dbba 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCONF)   += pinconf.o
 obj-$(CONFIG_OF)   += devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)  += pinconf-generic.o
 obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o
+obj-$(CONFIG_PINCTRL_ARTPEC6)   += pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)   += pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_BF54x)+= pinctrl-adi2-bf54x.o
 obj-$(CONFIG_PINCTRL_BF60x)+= pinctrl-adi2-bf60x.o
diff --git a/drivers/pinctrl/pinctrl-artpec6.c 
b/drivers/pinctrl/pinctrl-artpec6.c
new file mode 100644
index 000..ff80c8a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-artpec6.c
@@ -0,0 +1,1279 @@
+/*
+ * Driver for the Axis ARTPEC-6 pin controller
+ *
+ * Author: Chris Paterson 
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+#include "pinctrl-artpec6.h"
+
+struct artpec6_pmx {
+   struct device  *dev;
+   struct pinctrl_dev *pctl;
+   void __iomem   *base;
+   struct pinctrl_pin_desc *pins;
+   unsigned int   num_pins;
+   struct artpec6_pin_group *pin_groups;
+   unsigned int   num_pin_groups;
+   struct artpec6_pmx_func *functions;
+   unsigned int   num_functions;
+};
+
+struct artpec6_pin_group {
+   const char *name;
+   const unsigned int *pins;
+   const unsigned int num_pins;
+   const unsigned int *reg_offsets;
+   const unsigned int num_regs;
+   unsigned char  config;
+};
+
+struct artpec6_pmx_func {
+   const char *name;
+   const char * const *groups;
+   const unsigned int num_groups;
+};
+
+/* pins */
+static struct pinctrl_pin_desc artpec6_pins[] = {
+   PINCTRL_PIN(0, "GPIO0"),
+   PINCTRL_PIN(1, "GPIO1"),
+   PINCTRL_PIN(2, "GPIO2"),
+   PINCTRL_PIN(3, "GPIO3"),
+   PINCTRL_PIN(4, "GPIO4"),
+   PINCTRL_PIN(5, "GPIO5"),
+   PINCTRL_PIN(6, "GPIO6"),
+   PINCTRL_PIN(7, "GPIO7"),
+   PINCTRL_PIN(8, "GPIO8"),
+   PINCTRL_PIN(9, "GPIO9"),
+   PINCTRL_PIN(10, "GPIO10"),
+   PINCTRL_PIN(11, "GPIO11"),
+   PINCTRL_PIN(12, "GPIO12"),
+   PINCTRL_PIN(13, "GPIO13"),
+   PINCTRL_PIN(14, "GPIO14"),
+   PINCTRL_PIN(15, "GPIO15"),
+   PINCTRL_PIN(16, "GPIO16"),
+   PINCTRL_PIN(17, "GPIO17"),
+   PINCTRL_PIN(18, "GPIO18"),
+   PINCTRL_PIN(19, "GPIO19"),
+   PINCTRL_PIN(20, "GPIO20"),
+   PINCTRL_PIN(21, "GPIO21"),
+   PINCTRL_PIN(22, "GPIO22"),
+