[01/11] pinctrl: mvebu: pinctrl driver core

Commit Message

Sebastian Hesselbarth Aug. 11, 2012, 12:56 p.m. UTC

This patch adds a pinctrl driver core for Marvell SoCs plus DT
binding documentation. This core driver will be used by SoC family
specific drivers, i.e. Armada XP, Armada 370, Dove, Kirkwood, aso.

Signed-off-by: Sebastian Hesselbarth <[e-mail]>
Signed-off-by: Thomas Petazzoni <[e-mail]>
---
Cc: Grant Likely <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Rob Landley <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Lior Amsalem <[e-mail]>
Cc: Andrew Lunn <[e-mail]>
Cc: Gregory CLEMENT <[e-mail]>
Cc: Ben Dooks <[e-mail]>
Cc: Sebastian Hesselbarth <[e-mail]>
Cc: Thomas Petazzoni <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 .../bindings/pinctrl/marvell,mvebu-pinctrl.txt     |   46 ++
 drivers/pinctrl/Kconfig                            |   10 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/pinctrl-mvebu.c                    |  758 ++++++++++++++++++++
 drivers/pinctrl/pinctrl-mvebu.h                    |  175 +++++
 5 files changed, 990 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,mvebu-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-mvebu.c
 create mode 100644 drivers/pinctrl/pinctrl-mvebu.h

Comments

Linus Walleij Aug. 20, 2012, 9:09 a.m. UTC | #1

On Sat, Aug 11, 2012 at 2:56 PM, Sebastian Hesselbarth
<[e-mail]> wrote:

> This patch adds a pinctrl driver core for Marvell SoCs plus DT
> binding documentation. This core driver will be used by SoC family
> specific drivers, i.e. Armada XP, Armada 370, Dove, Kirkwood, aso.

Thanks for dealing with this, much appreaciated!

Are you taking this patch series through some Marvell tree or do you want
me to carry it in the pinctrl tree?

(...)
> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,mvebu-pinctrl.txt
> @@ -0,0 +1,46 @@
> +* Marvell SoC pinctrl core driver for mpp
> +
> +The pinctrl driver enables Marvell SoCs to configure the multi-purpose pins
> +(mpp) to a specific function. For each SoC family there is a SoC specific
> +driver using this core driver.
> +
> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +A Marvell SoC pin configuration node is a node of a group of pins which can
> +be used for a specific device or function. Each node requires one or more
> +mpp pins or group of pins and a mpp function common to all pins.
> +
> +Required properties for pinctrl driver:
> +- compatible: "marvell,<soc>-pinctrl"
> +  Please refer to each marvell,<soc>-pinctrl.txt binding doc for supported SoCs.
> +
> +Required properties for pin configuration node:
> +- marvell,pins: string array of mpp pins or group of pins to be muxed.
> +- marvell,function: string representing to function to mux to for all
> +    marvell,pins given in this pin configuration node. The function has to be
> +    common for all marvell,pins. Please refer to marvell,<soc>-pinctrl.txt for
> +    valid pin/pin group names and available function names for each SoC.
> +
> +Examples:
> +
> +uart1: serial@12100 {
> +       compatible = "ns16550a";
> +       reg = <0x12100 0x100>;
> +       reg-shift = <2>;
> +       interrupts = <7>;
> +       clock-frequency = <166666667>;

It's got nothing to do with this patch, but getting a clock frequency
out of the DT instead of getting it from the clk_get_rate(clk) and
the clock tree seems absurd... (But maybe this platform does not
even have a clk implementation?)

> +
> +       pinctrl-0 = <&pmx_uart1_sw>;
> +};
> +
> +pinctrl: pinctrl@d0200 {
> +       compatible = "marvell,dove-pinctrl";
> +       reg = <0xd0200 0x20>;
> +
> +       pmx_uart1_sw: pmx-uart1-sw {
> +               marvell,pins = "mpp_uart1";
> +               marvell,function = "uart1";
> +       };
> +};

This looks very good.

(...)
> +#include <linux/pinctrl/consumer.h>

Why are you including this header?
(There are valid reasons, state them in a comment,
e.g. /* to access pinctrl_gpio* in GPIO portions */)

> +struct mvebu_pinctrl_function {
> +       unsigned fid;
> +       const char *name;
> +       const char *setting;
> +       const char **groups;
> +       unsigned num_groups;
> +};
> +
> +struct mvebu_pinctrl_group {
> +       const char *name;
> +       struct mvebu_mpp_ctrl *ctrl;
> +       struct mvebu_mpp_ctrl_setting *settings;
> +       unsigned num_settings;
> +       unsigned gid;
> +       unsigned *pins;
> +       unsigned npins;
> +};
> +
> +struct mvebu_pinctrl {
> +       struct device *dev;
> +       struct pinctrl_dev *pctldev;
> +       struct pinctrl_desc desc;
> +       void __iomem *base;
> +       struct mvebu_pinctrl_group *groups;
> +       unsigned num_groups;
> +       struct mvebu_pinctrl_function *functions;
> +       unsigned num_functions;
> +       unsigned variant;
> +};

There is some other "variant" field elsewhere that is
a u8, is this correct? In this and the other case, should
this "variant" rather be an enum?

> +struct mvebu_pinctrl_group *mvebu_pinctrl_find_group_by_pid(
> +       struct mvebu_pinctrl *pctl, unsigned pid)
> +{
> +       unsigned n;
> +       for (n = 0; n < pctl->num_groups; n++) {
> +               if (pid >= pctl->groups[n].pins[0] &&
> +                   pid < pctl->groups[n].pins[0] +
> +                       pctl->groups[n].npins)
> +                       return &pctl->groups[n];
> +       }
> +       return NULL;
> +}
> +
> +struct mvebu_pinctrl_group *mvebu_pinctrl_find_group_by_name(
> +       struct mvebu_pinctrl *pctl, const char *name)
> +{
> +       unsigned n;
> +       for (n = 0; n < pctl->num_groups; n++) {
> +               if (strcmp(name, pctl->groups[n].name) == 0)
> +                       return &pctl->groups[n];
> +       }
> +       return NULL;
> +}
> +
> +struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_setting_by_val(
> +       struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp,
> +       unsigned long config)
> +{
> +       unsigned n;
> +       for (n = 0; n < grp->num_settings; n++) {
> +               if (config == grp->settings[n].val) {
> +                       if (!pctl->variant || (pctl->variant &
> +                                              grp->settings[n].variant))
> +                               return &grp->settings[n];
> +               }
> +       }
> +       return NULL;
> +}
> +
> +struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_setting_by_name(
> +       struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp,
> +       const char *name)
> +{
> +       unsigned n;
> +       for (n = 0; n < grp->num_settings; n++) {
> +               if (strcmp(name, grp->settings[n].name) == 0) {
> +                       if (!pctl->variant || (pctl->variant &
> +                                              grp->settings[n].variant))
> +                               return &grp->settings[n];
> +               }
> +       }
> +       return NULL;
> +}
> +
> +struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_gpio_setting(
> +       struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp)
> +{
> +       unsigned n;
> +       for (n = 0; n < grp->num_settings; n++) {
> +               if (grp->settings[n].flags &
> +                       (MVEBU_SETTING_GPO | MVEBU_SETTING_GPI)) {
> +                       if (!pctl->variant || (pctl->variant &
> +                                               grp->settings[n].variant))
> +                               return &grp->settings[n];
> +               }
> +       }
> +       return NULL;
> +}
> +
> +struct mvebu_pinctrl_function *mvebu_pinctrl_find_function_by_name(
> +       struct mvebu_pinctrl *pctl, const char *name)
> +{
> +       unsigned n;
> +       for (n = 0; n < pctl->num_functions; n++) {
> +               if (strcmp(name, pctl->functions[n].name) == 0)
> +                       return &pctl->functions[n];
> +       }
> +       return NULL;
> +}
> +
> +static int mvebu_common_mpp_get(struct mvebu_pinctrl *pctl,
> +                               struct mvebu_pinctrl_group *grp,
> +                               unsigned long *config)
> +{
> +       unsigned pin = grp->gid;
> +       unsigned off = (pin / 8) * 4;
> +       unsigned shift = (pin % 8) * 4;

Some magic numbers here. Either use relevant #define:s or
put an inline comment that explains what is going on.

> +
> +       *config = readl(pctl->base + off);
> +       *config >>= shift;
> +       *config &= 0xf;

Here too...

> +
> +       return 0;
> +}
> +
> +static int mvebu_common_mpp_set(struct mvebu_pinctrl *pctl,
> +                               struct mvebu_pinctrl_group *grp,
> +                               unsigned long config)
> +{
> +       unsigned pin = grp->gid;
> +       unsigned off = (pin / 8) * 4;
> +       unsigned shift = (pin % 8) * 4;
> +       unsigned long reg;
> +
> +       reg = readl(pctl->base + off);
> +       reg &= ~(0xf << shift);
> +       reg |= (config << shift);
> +       writel(reg, pctl->base + off);
> +
> +       return 0;
> +}

Same comment as previous function.

(...)
> +static void mvebu_pinctrl_dt_free_map(struct pinctrl_dev *pctldev,
> +                               struct pinctrl_map *map, unsigned num_maps)
> +{
> +       kfree(map);
> +}

Is it possible to use devm_* managed devm_kzalloc() for this map
so you don't need to free it explicitly?

(Maybe not, just checking.)

The rest of this file is looking so good that I find nothing to comment on.

(...)
> diff --git a/drivers/pinctrl/pinctrl-mvebu.h b/drivers/pinctrl/pinctrl-mvebu.h
(...)
> +/*

Kerneldoc begins with /** have you really tested to generate
kerneldoc?

> + * struct mvebu_mpp_ctrl - describe a mpp control
> + * @name: name of the control group
> + * @pid: first pin id handled by this control
> + * @npins: number of pins controlled by this control
> + * @mpp_get: (optional) special function to get mpp setting
> + * @mpp_set: (optional) special function to set mpp setting
> + * @mpp_gpio_req: (optional) special function to request gpio
> + * @mpp_gpio_dir: (optional) special function to set gpio direction
> + *
> + * if optional mpp_get/_set functions are set these
> + * are used to get/set a specific mode. Otherwise it is
> + * assumed that the mpp control is based on 4-bit groups
> + * in subsequent registers.
> + *
> + * mpp_gpio_req/_dir functions can be used to allow pin settings
> + * with varying gpio pins.
> + */
> +struct mvebu_mpp_ctrl {
> +       const char *name;
> +       u8 pid;
> +       u8 npins;

So, there will never be > 256 pins on a Marvell platform?

Just checking...

> +       unsigned *pins;
> +       int (*mpp_get)(struct mvebu_mpp_ctrl *ctrl, unsigned long *config);
> +       int (*mpp_set)(struct mvebu_mpp_ctrl *ctrl, unsigned long config);
> +       int (*mpp_gpio_req)(struct mvebu_mpp_ctrl *ctrl, u8 pid);
> +       int (*mpp_gpio_dir)(struct mvebu_mpp_ctrl *ctrl, u8 pid, bool input);
> +};
> +
> +/*

/**

> + * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
> + * @val: ctrl setting value

It is not obvious to me what this means, it it possible to elaborate
on how this member is defined and used?

> + * @name: ctrl setting name, e.g. uart2, spi0 - unique per mpp_mode
> + * @subname: (optional) additional ctrl setting name, e.g. rts, cts
> + * @variant: (optional) variant identifier mask

This thing again, there are lots of "variants" around here.
Is it a candidate for an enum?

> + * @flags: (private) flags to store gpi/gpo/gpio capabilities

Write something about the two available flags and what they
mean maybe?

> + *
> + * ctrl setting name will be used to switch to this setting in
> + * DT description, e.g. marvell,function = "uart2". subname
> + * is only for debugging purposes.
> + * If name is one of "gpi", "gpo", "gpio" gpio capabilities are
> + * parsed during initialization.
> + */
> +struct mvebu_mpp_ctrl_setting {
> +       u8 val;
> +       const char *name;
> +       const char *subname;
> +       u8 variant;

enum?

> +       u8 flags;
> +#define  MVEBU_SETTING_GPO     (1 << 0)
> +#define  MVEBU_SETTING_GPI     (1 << 1)

So these are the possible flags I suspect. (Looking good.)

> +};
> +
> +/*
> + * struct mvebu_mpp_mode - link ctrl and settings
> + * @pid: first pin id handled by this mode
> + * @settings: list of settings available for this mode
> + */
> +struct mvebu_mpp_mode {
> +       u8 pid;
> +       struct mvebu_mpp_ctrl_setting *settings;
> +};
> +
> +/*
> + * struct mvebu_pinctrl_soc_info -
> + *       SoC specific info passed to pinctrl-mvebu
> + * @variant: variant mask of soc_info
> + * @controls: list of available mvebu_mpp_ctrls
> + * @ncontrols: number of available mvebu_mpp_ctrls
> + * @modes: list of available mvebu_mpp_modes
> + * @nmodes: number of available mvebu_mpp_modes
> + * @gpioranges: list of pinctrl_gpio_ranges
> + * @ngpioranges: number of available pinctrl_gpio_ranges
> + */
> +struct mvebu_pinctrl_soc_info {
> +       u8 variant;

Why is this an u8 rather than an enum?

> +       struct mvebu_mpp_ctrl *controls;
> +       int ncontrols;
> +       struct mvebu_mpp_mode *modes;
> +       int nmodes;
> +       struct pinctrl_gpio_range *gpioranges;
> +       int ngpioranges;
> +};

(...)
> +#if defined(CONFIG_DEBUG_FS)
> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
> +       _MPP_VAR_FUNCTION(_val, _name, _subname, _mask)
> +#else
> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
> +       _MPP_VAR_FUNCTION(_val, _name, NULL, _mask)
> +#endif

This looks a bit kludgy. Is the purpose to save memory
if debugfs is not used?

This looks good overall.

Yours,
Linus Walleij

Sebastian Hesselbarth Aug. 20, 2012, 9:46 a.m. UTC | #2

On 8/20/12, Linus Walleij <[e-mail]> wrote:
> Are you taking this patch series through some Marvell tree or do you want
> me to carry it in the pinctrl tree?

Hi Linus,

I think it would be better to take it through the Marvell tree of Jason
Cooper. It is only for Marvell SoCs anyway.

>> +uart1: serial@12100 {
>> +       compatible = "ns16550a";
>> +       reg = <0x12100 0x100>;
>> +       reg-shift = <2>;
>> +       interrupts = <7>;
>> +       clock-frequency = <166666667>;
>
> It's got nothing to do with this patch, but getting a clock frequency
> out of the DT instead of getting it from the clk_get_rate(clk) and
> the clock tree seems absurd... (But maybe this platform does not
> even have a clk implementation?)

It's of_serial's implementation. I patched that once for getting
frequency out of "clocks" property but then I got busy with
porting mach-dove and pinctrl.. Marvell SoCs do have a clk
implementation and as soon as of_serial can handle "clocks"
property it will be used for sure. I can remove "clock-frequency"
from the example anyway as it is not really part of pinctrl
binding documentation.

> (...)
>> +#include <linux/pinctrl/consumer.h>
>
> Why are you including this header?
> (There are valid reasons, state them in a comment,
> e.g. /* to access pinctrl_gpio* in GPIO portions */)

I guess it was a left-over from my experiments with
default pinmux configuration until you pointed me to
the correct way of having the pinctrl itself hog muxes.

I'll remove that include.

>> +struct mvebu_pinctrl {
>> +       struct device *dev;
>> +       struct pinctrl_dev *pctldev;
>> +       struct pinctrl_desc desc;
>> +       void __iomem *base;
>> +       struct mvebu_pinctrl_group *groups;
>> +       unsigned num_groups;
>> +       struct mvebu_pinctrl_function *functions;
>> +       unsigned num_functions;
>> +       unsigned variant;
>> +};
>
> There is some other "variant" field elsewhere that is
> a u8, is this correct? In this and the other case, should
> this "variant" rather be an enum?

I'll review the variant types but inside pinctrl-mvebu variant
is used as a bit mask to distinguish different variants. Anyway,
they should always be the same size.

>> +static int mvebu_common_mpp_get(struct mvebu_pinctrl *pctl,
>> +                               struct mvebu_pinctrl_group *grp,
>> +                               unsigned long *config)
>> +{
>> +       unsigned pin = grp->gid;
>> +       unsigned off = (pin / 8) * 4;
>> +       unsigned shift = (pin % 8) * 4;
>
> Some magic numbers here. Either use relevant #define:s or
> put an inline comment that explains what is going on.

Ok.

>> +static void mvebu_pinctrl_dt_free_map(struct pinctrl_dev *pctldev,
>> +                               struct pinctrl_map *map, unsigned
>> num_maps)
>> +{
>> +       kfree(map);
>> +}
>
> Is it possible to use devm_* managed devm_kzalloc() for this map
> so you don't need to free it explicitly?
>
> (Maybe not, just checking.)

Hmm, I guess not as I thought I've read not to use devm_kfree when
you allocate _and_ free stuff on runtime without removing the device
itself, right?

>> diff --git a/drivers/pinctrl/pinctrl-mvebu.h
>> b/drivers/pinctrl/pinctrl-mvebu.h
> (...)
>> +/*
>
> Kerneldoc begins with /** have you really tested to generate
> kerneldoc?

Nope, I wasn't even aware of that, sorry. I just copied it over
from some other pinctrl include. I'll check for compatibility with
kerneldoc.

>> +struct mvebu_mpp_ctrl {
>> +       const char *name;
>> +       u8 pid;
>> +       u8 npins;
>
> So, there will never be > 256 pins on a Marvell platform?

Well, with all current platforms we are well below 100. I guess
256 max (muxable) pins will be enough.

>> + * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
>> + * @val: ctrl setting value
>
> It is not obvious to me what this means, it it possible to elaborate
> on how this member is defined and used?

Well, I see if I can clarify the description but wrt the datasheet it
_should_ be quite obvious.

>> + * @variant: (optional) variant identifier mask
>
> This thing again, there are lots of "variants" around here.
> Is it a candidate for an enum?

As it is a mask, I don't think enum fits here.

>> + * @flags: (private) flags to store gpi/gpo/gpio capabilities
>
> Write something about the two available flags and what they
> mean maybe?

Ok.

>> +#define  MVEBU_SETTING_GPO     (1 << 0)
>> +#define  MVEBU_SETTING_GPI     (1 << 1)
>
> So these are the possible flags I suspect. (Looking good.)

Yes, some mpp pins on Marvell SoCs are out-only, while
normally they are in-out. I haven't seen in-only yet but better
to have the flags ready if someone stumbles upon one.

>> +#if defined(CONFIG_DEBUG_FS)
>> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
>> +       _MPP_VAR_FUNCTION(_val, _name, _subname, _mask)
>> +#else
>> +#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)         \
>> +       _MPP_VAR_FUNCTION(_val, _name, NULL, _mask)
>> +#endif
>
> This looks a bit kludgy. Is the purpose to save memory
> if debugfs is not used?

Yes, that was the intention. Subnames are not used by pinctrl at all,
other for debugfs files. Although, there are some pins that have two different
settings for the same device name, e.g. sata(prsnt) and sata(act), I don't
think it is a good idea to use the subname but have sata(prsnt) and sata-1(act)
instead. Otherwise, you'll always have to look up the specific function for
each pin. OTOH, I could check for "sata" and take the subname into account
when it is given in marvell,function. But it will not help reducing the required
pinmux descriptions in DT because you still have to distinguish both and
there are only 2 out of ~50 groups that have them for Dove.

> This looks good overall.

Thanks!
In some internal review with Andrew I also added a spinlock to
mvebu_pinconf_get/_set that will protect all calls to generic and specific
_get/_set register accesses. Moreover, I replaced clk_get_sys in pinctrl-dove
with the devm_ counterpart and removed the explicit clk_put.

Sebastian

Thomas Petazzoni Aug. 20, 2012, 12:51 p.m. UTC | #3

Hello,

Le Mon, 20 Aug 2012 11:46:14 +0200,
Sebastian Hesselbarth <[e-mail]> a écrit :

> >> +uart1: serial@12100 {
> >> +       compatible = "ns16550a";
> >> +       reg = <0x12100 0x100>;
> >> +       reg-shift = <2>;
> >> +       interrupts = <7>;
> >> +       clock-frequency = <166666667>;
> >
> > It's got nothing to do with this patch, but getting a clock frequency
> > out of the DT instead of getting it from the clk_get_rate(clk) and
> > the clock tree seems absurd... (But maybe this platform does not
> > even have a clk implementation?)
> 
> It's of_serial's implementation. I patched that once for getting
> frequency out of "clocks" property but then I got busy with
> porting mach-dove and pinctrl.. Marvell SoCs do have a clk
> implementation and as soon as of_serial can handle "clocks"
> property it will be used for sure. I can remove "clock-frequency"
> from the example anyway as it is not really part of pinctrl
> binding documentation.

We are also working on using the clk framework for the 370/XP support
(my colleague Grégory in Cc has started working on this last week), and
we also want to be able to get the serial clock-frequency from the clk
framework instead of an explicit value in the DT node. But that's a
separate topic :)

> > Is it possible to use devm_* managed devm_kzalloc() for this map
> > so you don't need to free it explicitly?
> >
> > (Maybe not, just checking.)
> 
> Hmm, I guess not as I thought I've read not to use devm_kfree when
> you allocate _and_ free stuff on runtime without removing the device
> itself, right?

It is also my understanding that devm_*() functions should be used to
allocate things that should persist until the device is removed. But I
might be wrong here.

> >> +struct mvebu_mpp_ctrl {
> >> +       const char *name;
> >> +       u8 pid;
> >> +       u8 npins;
> >
> > So, there will never be > 256 pins on a Marvell platform?
> 
> Well, with all current platforms we are well below 100. I guess
> 256 max (muxable) pins will be enough.

Agreed, and this structure is completely internal to the kernel, so we
can easily change it in the future if needed.

> >> + * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
> >> + * @val: ctrl setting value
> >
> > It is not obvious to me what this means, it it possible to elaborate
> > on how this member is defined and used?
> 
> Well, I see if I can clarify the description but wrt the datasheet it
> _should_ be quite obvious.

I think the setting/function/group/control terminology would benefit
from an explanation, as it isn't very easy to figure out what all these
words mean in the context of the pinctrl-mvebu driver.

> In some internal review with Andrew I also added a spinlock to
> mvebu_pinconf_get/_set that will protect all calls to generic and specific
> _get/_set register accesses. Moreover, I replaced clk_get_sys in pinctrl-dove
> with the devm_ counterpart and removed the explicit clk_put.

Yes, I had seen this discussion, but I am not sure it is needed: it
seems the pinctrl core calls all the pinconf_set/pinconf_get methods
with the pinctrl_mutex held. When I wrote an initial pinctrl driver for
370/XP I had the same question as Andrew and my conclusion was that the
locking done by the pinctrl subsystem core was sufficient.

Best regards,

Thomas

Linus Walleij Aug. 20, 2012, 2:11 p.m. UTC | #4

On Sat, Aug 11, 2012 at 2:56 PM, Sebastian Hesselbarth
<[e-mail]> wrote:

> This patch adds a pinctrl driver core for Marvell SoCs plus DT
> binding documentation. This core driver will be used by SoC family
> specific drivers, i.e. Armada XP, Armada 370, Dove, Kirkwood, aso.
(...)
> +config PINCTRL_MVEBU
> +       bool "Marvell SoC pin controller drivers"
> +       depends on ARCH_MVEBU

So altering earlier statements I suspect this should be simply:

depends on PLAT_ORION

Yours,
Linus Walleij

Linus Walleij Aug. 20, 2012, 2:18 p.m. UTC | #5

On Mon, Aug 20, 2012 at 11:46 AM, Sebastian Hesselbarth
<[e-mail]> wrote:
> On 8/20/12, Linus Walleij <[e-mail]> wrote:

>> Are you taking this patch series through some Marvell tree or do you want
>> me to carry it in the pinctrl tree?
>
> I think it would be better to take it through the Marvell tree of Jason
> Cooper. It is only for Marvell SoCs anyway.

OK.

>> There is some other "variant" field elsewhere that is
>> a u8, is this correct? In this and the other case, should
>> this "variant" rather be an enum?
>
> I'll review the variant types but inside pinctrl-mvebu variant
> is used as a bit mask to distinguish different variants. Anyway,
> they should always be the same size.

Aha bitmask, seems you can only have 8 different variants of
the Marvell's then?

>> Is it possible to use devm_* managed devm_kzalloc() for this map
>> so you don't need to free it explicitly?
>>
>> (Maybe not, just checking.)
>
> Hmm, I guess not as I thought I've read not to use devm_kfree when
> you allocate _and_ free stuff on runtime without removing the device
> itself, right?

You're right.

>> So, there will never be > 256 pins on a Marvell platform?
>
> Well, with all current platforms we are well below 100. I guess
> 256 max (muxable) pins will be enough.

OK.

>>> + * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
>>> + * @val: ctrl setting value
>>
>> It is not obvious to me what this means, it it possible to elaborate
>> on how this member is defined and used?
>
> Well, I see if I can clarify the description but wrt the datasheet it
> _should_ be quite obvious.

I mainly worry about being able to read the code and figure out
what's going on, if the datasheet is vital then pls include some
link to it or so in the header of the file (but preferrably the code
should speak for itself).

>>> + * @variant: (optional) variant identifier mask
>>
>> This thing again, there are lots of "variants" around here.
>> Is it a candidate for an enum?
>
> As it is a mask, I don't think enum fits here.

OK. No big deal anyway.

> In some internal review with Andrew I also added a spinlock to
> mvebu_pinconf_get/_set that will protect all calls to generic and specific
> _get/_set register accesses. Moreover, I replaced clk_get_sys in pinctrl-dove
> with the devm_ counterpart and removed the explicit clk_put.

I had some separate comments on that clk thing...
Anyway, looking forward to v2, this is progressing quickly
I think.

Yours,
Linus Walleij

Sebastian Hesselbarth Aug. 20, 2012, 2:51 p.m. UTC | #6

On 08/20/2012 04:18 PM, Linus Walleij wrote:
>> I'll review the variant types but inside pinctrl-mvebu variant
>> is used as a bit mask to distinguish different variants. Anyway,
>> they should always be the same size.
>
> Aha bitmask, seems you can only have 8 different variants of
> the Marvell's then?

Only 8 different variants of one SoC! Currently, there are 5 for
Kirkwood, none for Dove. I think there are two more Kirkwood's that
never appeared in public, but it is still < 8.

>> Well, I see if I can clarify the description but wrt the datasheet it
>> _should_ be quite obvious.
>
> I mainly worry about being able to read the code and figure out
> what's going on, if the datasheet is vital then pls include some
> link to it or so in the header of the file (but preferrably the code
> should speak for itself).

Ok, I clarify whats meant by mvebu_*_ctrl/ctrl_setting/mode. I am not
that happy with that names either, but honestly pinctrl core already
took all names that I'd have chosen. Therefore, I chose some that do
not interfere with pinctrl core as the meaning is different.

Sebastian

Andrew Lunn Aug. 25, 2012, 8:22 a.m. UTC | #7

On Mon, Aug 20, 2012 at 11:09:51AM +0200, Linus Walleij wrote:
> On Sat, Aug 11, 2012 at 2:56 PM, Sebastian Hesselbarth
> <[e-mail]> wrote:
> 
> > This patch adds a pinctrl driver core for Marvell SoCs plus DT
> > binding documentation. This core driver will be used by SoC family
> > specific drivers, i.e. Armada XP, Armada 370, Dove, Kirkwood, aso.
> 
> Thanks for dealing with this, much appreaciated!
> 
> Are you taking this patch series through some Marvell tree or do you want
> me to carry it in the pinctrl tree?

Hi Linus

We can carry it thought the Marvell tree. 

   Andrew

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,mvebu-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/marvell,mvebu-pinctrl.txt
new file mode 100644
index 0000000..b3e5066
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/marvell,mvebu-pinctrl.txt
@@ -0,0 +1,46 @@ 
+* Marvell SoC pinctrl core driver for mpp
+
+The pinctrl driver enables Marvell SoCs to configure the multi-purpose pins
+(mpp) to a specific function. For each SoC family there is a SoC specific
+driver using this core driver.
+
+Please refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+A Marvell SoC pin configuration node is a node of a group of pins which can
+be used for a specific device or function. Each node requires one or more
+mpp pins or group of pins and a mpp function common to all pins.
+
+Required properties for pinctrl driver:
+- compatible: "marvell,<soc>-pinctrl"
+  Please refer to each marvell,<soc>-pinctrl.txt binding doc for supported SoCs.
+
+Required properties for pin configuration node:
+- marvell,pins: string array of mpp pins or group of pins to be muxed.
+- marvell,function: string representing to function to mux to for all
+    marvell,pins given in this pin configuration node. The function has to be
+    common for all marvell,pins. Please refer to marvell,<soc>-pinctrl.txt for
+    valid pin/pin group names and available function names for each SoC.
+
+Examples:
+
+uart1: serial@12100 {
+	compatible = "ns16550a";
+	reg = <0x12100 0x100>;
+	reg-shift = <2>;
+	interrupts = <7>;
+	clock-frequency = <166666667>;
+
+	pinctrl-0 = <&pmx_uart1_sw>;
+};
+
+pinctrl: pinctrl@d0200 {
+	compatible = "marvell,dove-pinctrl";
+	reg = <0xd0200 0x20>;
+
+	pmx_uart1_sw: pmx-uart1-sw {
+		marvell,pins = "mpp_uart1";
+		marvell,function = "uart1";
+	};
+};
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 54e3588..b968335 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -145,6 +145,16 @@  config PINCTRL_COH901
 	  COH 901 335 and COH 901 571/3. They contain 3, 5 or 7
 	  ports of 8 GPIO pins each.
 
+config PINCTRL_MVEBU
+	bool "Marvell SoC pin controller drivers"
+	depends on ARCH_MVEBU
+	select PINMUX
+	select PINCONF
+	help
+	  Say yes here to support pinctrl driver on Marvell SoCs.
+	  This is only the driver core and additionally needs a SoC specific
+	  driver.
+
 source "drivers/pinctrl/spear/Kconfig"
 
 endmenu
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index f40b1f8..007ed32 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -29,5 +29,6 @@  obj-$(CONFIG_PINCTRL_TEGRA20)	+= pinctrl-tegra20.o
 obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
 obj-$(CONFIG_PINCTRL_COH901)	+= pinctrl-coh901.o
+obj-$(CONFIG_PINCTRL_MVEBU)	+= pinctrl-mvebu.o
 
 obj-$(CONFIG_PLAT_SPEAR)	+= spear/
diff --git a/drivers/pinctrl/pinctrl-mvebu.c b/drivers/pinctrl/pinctrl-mvebu.c
new file mode 100644
index 0000000..09172be
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mvebu.c
@@ -0,0 +1,758 @@ 
+/*
+ * Marvell MVEBU pinctrl core driver
+ *
+ * Authors: Sebastian Hesselbarth <[e-mail]>
+ *          Thomas Petazzoni <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinctrl-mvebu.h"
+
+struct mvebu_pinctrl_function {
+	unsigned fid;
+	const char *name;
+	const char *setting;
+	const char **groups;
+	unsigned num_groups;
+};
+
+struct mvebu_pinctrl_group {
+	const char *name;
+	struct mvebu_mpp_ctrl *ctrl;
+	struct mvebu_mpp_ctrl_setting *settings;
+	unsigned num_settings;
+	unsigned gid;
+	unsigned *pins;
+	unsigned npins;
+};
+
+struct mvebu_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_desc desc;
+	void __iomem *base;
+	struct mvebu_pinctrl_group *groups;
+	unsigned num_groups;
+	struct mvebu_pinctrl_function *functions;
+	unsigned num_functions;
+	unsigned variant;
+};
+
+struct mvebu_pinctrl_group *mvebu_pinctrl_find_group_by_pid(
+	struct mvebu_pinctrl *pctl, unsigned pid)
+{
+	unsigned n;
+	for (n = 0; n < pctl->num_groups; n++) {
+		if (pid >= pctl->groups[n].pins[0] &&
+		    pid < pctl->groups[n].pins[0] +
+			pctl->groups[n].npins)
+			return &pctl->groups[n];
+	}
+	return NULL;
+}
+
+struct mvebu_pinctrl_group *mvebu_pinctrl_find_group_by_name(
+	struct mvebu_pinctrl *pctl, const char *name)
+{
+	unsigned n;
+	for (n = 0; n < pctl->num_groups; n++) {
+		if (strcmp(name, pctl->groups[n].name) == 0)
+			return &pctl->groups[n];
+	}
+	return NULL;
+}
+
+struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_setting_by_val(
+	struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp,
+	unsigned long config)
+{
+	unsigned n;
+	for (n = 0; n < grp->num_settings; n++) {
+		if (config == grp->settings[n].val) {
+			if (!pctl->variant || (pctl->variant &
+					       grp->settings[n].variant))
+				return &grp->settings[n];
+		}
+	}
+	return NULL;
+}
+
+struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_setting_by_name(
+	struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp,
+	const char *name)
+{
+	unsigned n;
+	for (n = 0; n < grp->num_settings; n++) {
+		if (strcmp(name, grp->settings[n].name) == 0) {
+			if (!pctl->variant || (pctl->variant &
+					       grp->settings[n].variant))
+				return &grp->settings[n];
+		}
+	}
+	return NULL;
+}
+
+struct mvebu_mpp_ctrl_setting *mvebu_pinctrl_find_gpio_setting(
+	struct mvebu_pinctrl *pctl, struct mvebu_pinctrl_group *grp)
+{
+	unsigned n;
+	for (n = 0; n < grp->num_settings; n++) {
+		if (grp->settings[n].flags &
+			(MVEBU_SETTING_GPO | MVEBU_SETTING_GPI)) {
+			if (!pctl->variant || (pctl->variant &
+						grp->settings[n].variant))
+				return &grp->settings[n];
+		}
+	}
+	return NULL;
+}
+
+struct mvebu_pinctrl_function *mvebu_pinctrl_find_function_by_name(
+	struct mvebu_pinctrl *pctl, const char *name)
+{
+	unsigned n;
+	for (n = 0; n < pctl->num_functions; n++) {
+		if (strcmp(name, pctl->functions[n].name) == 0)
+			return &pctl->functions[n];
+	}
+	return NULL;
+}
+
+static int mvebu_common_mpp_get(struct mvebu_pinctrl *pctl,
+				struct mvebu_pinctrl_group *grp,
+				unsigned long *config)
+{
+	unsigned pin = grp->gid;
+	unsigned off = (pin / 8) * 4;
+	unsigned shift = (pin % 8) * 4;
+
+	*config = readl(pctl->base + off);
+	*config >>= shift;
+	*config &= 0xf;
+
+	return 0;
+}
+
+static int mvebu_common_mpp_set(struct mvebu_pinctrl *pctl,
+				struct mvebu_pinctrl_group *grp,
+				unsigned long config)
+{
+	unsigned pin = grp->gid;
+	unsigned off = (pin / 8) * 4;
+	unsigned shift = (pin % 8) * 4;
+	unsigned long reg;
+
+	reg = readl(pctl->base + off);
+	reg &= ~(0xf << shift);
+	reg |= (config << shift);
+	writel(reg, pctl->base + off);
+
+	return 0;
+}
+
+static int mvebu_pinconf_group_get(struct pinctrl_dev *pctldev,
+				unsigned gid, unsigned long *config)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_group *grp = &pctl->groups[gid];
+
+	if (!grp->ctrl)
+		return -EINVAL;
+
+	if (grp->ctrl->mpp_get)
+		return grp->ctrl->mpp_get(grp->ctrl, config);
+
+	return mvebu_common_mpp_get(pctl, grp, config);
+}
+
+static int mvebu_pinconf_group_set(struct pinctrl_dev *pctldev,
+				unsigned gid, unsigned long config)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_group *grp = &pctl->groups[gid];
+
+	if (!grp->ctrl)
+		return -EINVAL;
+
+	if (grp->ctrl->mpp_set)
+		return grp->ctrl->mpp_set(grp->ctrl, config);
+
+	return mvebu_common_mpp_set(pctl, grp, config);
+}
+
+static void mvebu_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
+					struct seq_file *s, unsigned gid)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_group *grp = &pctl->groups[gid];
+	struct mvebu_mpp_ctrl_setting *curr;
+	unsigned long config;
+	unsigned n;
+
+	if (mvebu_pinconf_group_get(pctldev, gid, &config))
+		return;
+
+	curr = mvebu_pinctrl_find_setting_by_val(pctl, grp, config);
+
+	if (curr) {
+		seq_printf(s, "current: %s", curr->name);
+		if (curr->subname)
+			seq_printf(s, "(%s)", curr->subname);
+		if (curr->flags & (MVEBU_SETTING_GPO | MVEBU_SETTING_GPI)) {
+			seq_printf(s, "(");
+			if (curr->flags & MVEBU_SETTING_GPI)
+				seq_printf(s, "i");
+			if (curr->flags & MVEBU_SETTING_GPO)
+				seq_printf(s, "o");
+			seq_printf(s, ")");
+		}
+	} else
+		seq_printf(s, "current: UNKNOWN");
+
+	if (grp->num_settings > 1) {
+		seq_printf(s, ", available = [");
+		for (n = 0; n < grp->num_settings; n++) {
+			if (curr == &grp->settings[n])
+				continue;
+
+			/* skip unsupported settings for this variant */
+			if (pctl->variant &&
+			    !(pctl->variant & grp->settings[n].variant))
+				continue;
+
+			seq_printf(s, " %s", grp->settings[n].name);
+			if (grp->settings[n].subname)
+				seq_printf(s, "(%s)", grp->settings[n].subname);
+			if (grp->settings[n].flags &
+				(MVEBU_SETTING_GPO | MVEBU_SETTING_GPI)) {
+				seq_printf(s, "(");
+				if (grp->settings[n].flags & MVEBU_SETTING_GPI)
+					seq_printf(s, "i");
+				if (grp->settings[n].flags & MVEBU_SETTING_GPO)
+					seq_printf(s, "o");
+				seq_printf(s, ")");
+			}
+		}
+		seq_printf(s, " ]");
+	}
+	return;
+}
+
+struct pinconf_ops mvebu_pinconf_ops = {
+	.pin_config_group_get = mvebu_pinconf_group_get,
+	.pin_config_group_set = mvebu_pinconf_group_set,
+	.pin_config_group_dbg_show = mvebu_pinconf_group_dbg_show,
+};
+
+static int mvebu_pinmux_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->num_functions;
+}
+
+static const char *mvebu_pinmux_get_func_name(struct pinctrl_dev *pctldev,
+					unsigned fid)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->functions[fid].name;
+}
+
+static int mvebu_pinmux_get_groups(struct pinctrl_dev *pctldev, unsigned fid,
+				const char * const **groups,
+				unsigned * const num_groups)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pctl->functions[fid].groups;
+	*num_groups = pctl->functions[fid].num_groups;
+	return 0;
+}
+
+static int mvebu_pinmux_enable(struct pinctrl_dev *pctldev, unsigned fid,
+			unsigned gid)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_function *func = &pctl->functions[fid];
+	struct mvebu_pinctrl_group *grp = &pctl->groups[gid];
+	struct mvebu_mpp_ctrl_setting *setting;
+	int ret;
+
+	setting = mvebu_pinctrl_find_setting_by_name(pctl, grp,
+						     func->setting);
+	if (!setting) {
+		dev_err(pctl->dev,
+			"unable to find setting %s in group %s\n",
+			func->setting, func->groups[gid]);
+		return -EINVAL;
+	}
+
+	ret = mvebu_pinconf_group_set(pctldev, grp->gid, setting->val);
+	if (ret) {
+		dev_err(pctl->dev, "cannot set group %s to %s\n",
+			func->groups[gid], func->setting);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mvebu_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_group *grp;
+	struct mvebu_mpp_ctrl_setting *setting;
+
+	grp = mvebu_pinctrl_find_group_by_pid(pctl, offset);
+	if (!grp)
+		return -EINVAL;
+
+	if (grp->ctrl->mpp_gpio_req)
+		return grp->ctrl->mpp_gpio_req(grp->ctrl, offset);
+
+	setting = mvebu_pinctrl_find_gpio_setting(pctl, grp);
+	if (!setting)
+		return -ENOTSUPP;
+
+	return mvebu_pinconf_group_set(pctldev, grp->gid, setting->val);
+}
+
+static int mvebu_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct mvebu_pinctrl_group *grp;
+	struct mvebu_mpp_ctrl_setting *setting;
+
+	grp = mvebu_pinctrl_find_group_by_pid(pctl, offset);
+	if (!grp)
+		return -EINVAL;
+
+	if (grp->ctrl->mpp_gpio_dir)
+		return grp->ctrl->mpp_gpio_dir(grp->ctrl, offset, input);
+
+	setting = mvebu_pinctrl_find_gpio_setting(pctl, grp);
+	if (!setting)
+		return -ENOTSUPP;
+
+	if ((input && (setting->flags & MVEBU_SETTING_GPI)) ||
+	    (!input && (setting->flags & MVEBU_SETTING_GPO)))
+		return 0;
+
+	return -ENOTSUPP;
+}
+
+static struct pinmux_ops mvebu_pinmux_ops = {
+	.get_functions_count = mvebu_pinmux_get_funcs_count,
+	.get_function_name = mvebu_pinmux_get_func_name,
+	.get_function_groups = mvebu_pinmux_get_groups,
+	.gpio_request_enable = mvebu_pinmux_gpio_request_enable,
+	.gpio_set_direction = mvebu_pinmux_gpio_set_direction,
+	.enable = mvebu_pinmux_enable,
+};
+
+static int mvebu_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	return pctl->num_groups;
+}
+
+static const char *mvebu_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						unsigned gid)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	return pctl->groups[gid].name;
+}
+
+static int mvebu_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					unsigned gid, const unsigned **pins,
+					unsigned *num_pins)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	*pins = pctl->groups[gid].pins;
+	*num_pins = pctl->groups[gid].npins;
+	return 0;
+}
+
+static int mvebu_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
+					struct device_node *np,
+					struct pinctrl_map **map,
+					unsigned *num_maps)
+{
+	struct mvebu_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct property *prop;
+	const char *function;
+	const char *group;
+	int ret, nmaps, n;
+
+	*map = NULL;
+	*num_maps = 0;
+
+	ret = of_property_read_string(np, "marvell,function", &function);
+	if (ret) {
+		dev_err(pctl->dev,
+			"missing marvell,function in node %s\n", np->name);
+		return 0;
+	}
+
+	nmaps = of_property_count_strings(np, "marvell,pins");
+	if (nmaps < 0) {
+		dev_err(pctl->dev,
+			"missing marvell,pins in node %s\n", np->name);
+		return 0;
+	}
+
+	*map = kmalloc(nmaps * sizeof(struct pinctrl_map), GFP_KERNEL);
+	if (map == NULL) {
+		dev_err(pctl->dev,
+			"cannot allocate pinctrl_map memory for %s\n",
+			np->name);
+		return -ENOMEM;
+	}
+
+	n = 0;
+	of_property_for_each_string(np, "marvell,pins", prop, group) {
+		struct mvebu_pinctrl_group *grp =
+			mvebu_pinctrl_find_group_by_name(pctl, group);
+
+		if (!grp) {
+			dev_err(pctl->dev, "unknown pin %s", group);
+			continue;
+		}
+
+		if (!mvebu_pinctrl_find_setting_by_name(pctl, grp, function)) {
+			dev_err(pctl->dev, "unsupported function %s on pin %s",
+				function, group);
+			continue;
+		}
+
+		(*map)[n].type = PIN_MAP_TYPE_MUX_GROUP;
+		(*map)[n].data.mux.group = group;
+		(*map)[n].data.mux.function = np->name;
+		n++;
+	}
+
+	*num_maps = nmaps;
+
+	return 0;
+}
+
+static void mvebu_pinctrl_dt_free_map(struct pinctrl_dev *pctldev,
+				struct pinctrl_map *map, unsigned num_maps)
+{
+	kfree(map);
+}
+
+static struct pinctrl_ops mvebu_pinctrl_ops = {
+	.get_groups_count = mvebu_pinctrl_get_groups_count,
+	.get_group_name = mvebu_pinctrl_get_group_name,
+	.get_group_pins = mvebu_pinctrl_get_group_pins,
+	.dt_node_to_map = mvebu_pinctrl_dt_node_to_map,
+	.dt_free_map = mvebu_pinctrl_dt_free_map,
+};
+
+static int __devinit mvebu_pinctrl_dt_parse_function(struct mvebu_pinctrl *pctl,
+					struct device_node *np, unsigned idx)
+{
+	struct mvebu_pinctrl_function *func = &pctl->functions[idx];
+	struct property *prop;
+	const char *setting;
+	const char *group;
+	unsigned n = 0;
+	int ret, num_groups;
+
+	ret = of_property_read_string(np, "marvell,function", &setting);
+	if (ret) {
+		dev_err(pctl->dev,
+			"missing marvell,function in node %s\n", np->name);
+		return -EINVAL;
+	}
+
+	num_groups = of_property_count_strings(np, "marvell,pins");
+	if (num_groups <= 0) {
+		dev_err(pctl->dev,
+			"missing marvell,pins in node %s\n", np->name);
+		return -EINVAL;
+	}
+
+	func->fid = idx;
+	func->name = np->name;
+	func->setting = setting;
+	func->num_groups = num_groups;
+	func->groups = devm_kzalloc(pctl->dev, num_groups * sizeof(char *),
+				    GFP_KERNEL);
+	if (!func->groups) {
+		dev_err(pctl->dev,
+			"unable to alloc function groups\n");
+		return -ENOMEM;
+	}
+
+	of_property_for_each_string(np, "marvell,pins", prop, group) {
+		struct mvebu_pinctrl_group *grp =
+			mvebu_pinctrl_find_group_by_name(pctl, group);
+
+		if (!grp) {
+			dev_err(pctl->dev, "unknown pin %s", group);
+			return -EINVAL;
+		}
+
+		if (!mvebu_pinctrl_find_setting_by_name(pctl, grp, setting)) {
+			dev_err(pctl->dev, "unsupported function %s on pin %s",
+				setting, group);
+			return -EINVAL;
+		}
+
+		func->groups[n++] = group;
+	}
+	return 0;
+}
+
+static int __devinit mvebu_pinctrl_dt_parse(struct platform_device *pdev,
+					struct mvebu_pinctrl *pctl)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *np;
+	unsigned nfuncs = 0, n = 0;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	nfuncs = of_get_child_count(node);
+	if (nfuncs <= 0) {
+		dev_warn(pctl->dev, "no function defined in device node\n");
+		return 0;
+	}
+
+	pctl->num_functions = nfuncs;
+	pctl->functions = devm_kzalloc(&pdev->dev, nfuncs *
+				sizeof(struct mvebu_pinctrl_function),
+				GFP_KERNEL);
+	if (!pctl->functions) {
+		dev_err(pctl->dev, "unable to alloc functions\n");
+		return -ENOMEM;
+	}
+
+	for_each_child_of_node(node, np) {
+		ret = mvebu_pinctrl_dt_parse_function(pctl, np, n++);
+		if (ret) {
+			dev_warn(pctl->dev, "failed to parse function %s\n",
+				np->name);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+int __devinit mvebu_pinctrl_probe(struct platform_device *pdev)
+{
+	struct mvebu_pinctrl_soc_info *soc = dev_get_platdata(&pdev->dev);
+	struct mvebu_pinctrl *pctl;
+	struct resource *res;
+	void __iomem *iomem;
+	struct pinctrl_pin_desc *pdesc;
+	unsigned gid, n, k;
+	int ret;
+
+	if (!soc || !soc->controls || !soc->modes) {
+		dev_err(&pdev->dev, "wrong pinctrl soc info\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "unable to get resource\n");
+		return -ENODEV;
+	}
+
+	iomem = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!iomem) {
+		dev_err(&pdev->dev, "unable to ioremap\n");
+		return -ENODEV;
+	}
+
+	pctl = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_pinctrl),
+			GFP_KERNEL);
+	if (!pctl) {
+		dev_err(&pdev->dev, "unable to alloc driver\n");
+		return -ENOMEM;
+	}
+
+	pctl->desc.name = dev_name(&pdev->dev);
+	pctl->desc.owner = THIS_MODULE;
+	pctl->desc.pctlops = &mvebu_pinctrl_ops;
+	pctl->desc.pmxops = &mvebu_pinmux_ops;
+	pctl->desc.confops = &mvebu_pinconf_ops;
+	pctl->variant = soc->variant;
+	pctl->base = iomem;
+	pctl->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pctl);
+
+	/* count controls and create names for mvebu generic
+	   register controls; also does sanity checks */
+	pctl->num_groups = 0;
+	pctl->desc.npins = 0;
+	for (n = 0; n < soc->ncontrols; n++) {
+		struct mvebu_mpp_ctrl *ctrl = &soc->controls[n];
+		char *names;
+
+		pctl->desc.npins += ctrl->npins;
+		/* initial control pins */
+		for (k = 0; k < ctrl->npins; k++)
+			ctrl->pins[k] = ctrl->pid + k;
+
+		/* special soc specific control */
+		if (ctrl->mpp_get || ctrl->mpp_set) {
+			if (!ctrl->name || !ctrl->mpp_set || !ctrl->mpp_set) {
+				dev_err(&pdev->dev, "wrong soc control info\n");
+				return -EINVAL;
+			}
+			pctl->num_groups += 1;
+			continue;
+		}
+
+		/* generic mvebu register control */
+		names = devm_kzalloc(&pdev->dev, ctrl->npins * 8, GFP_KERNEL);
+		if (!names) {
+			dev_err(&pdev->dev, "failed to alloc mpp names\n");
+			return -ENOMEM;
+		}
+		for (k = 0; k < ctrl->npins; k++)
+			sprintf(names + 8*k, "mpp%d", ctrl->pid+k);
+		ctrl->name = names;
+		pctl->num_groups += ctrl->npins;
+	}
+
+	pdesc = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+			     sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
+	if (!pdesc) {
+		dev_err(&pdev->dev, "failed to alloc pinctrl pins\n");
+		return -ENOMEM;
+	}
+
+	for (n = 0; n < pctl->desc.npins; n++)
+		pdesc[n].number = n;
+	pctl->desc.pins = pdesc;
+
+	pctl->groups = devm_kzalloc(&pdev->dev, pctl->num_groups *
+			     sizeof(struct mvebu_pinctrl_group), GFP_KERNEL);
+	if (!pctl->groups) {
+		dev_err(&pdev->dev, "failed to alloc pinctrl groups\n");
+		return -ENOMEM;
+	}
+
+	/* assign mpp controls to groups */
+	gid = 0;
+	for (n = 0; n < soc->ncontrols; n++) {
+		struct mvebu_mpp_ctrl *ctrl = &soc->controls[n];
+		pctl->groups[gid].gid = gid;
+		pctl->groups[gid].ctrl = ctrl;
+		pctl->groups[gid].name = ctrl->name;
+		pctl->groups[gid].pins = ctrl->pins;
+		pctl->groups[gid].npins = ctrl->npins;
+
+		/* generic mvebu register control maps to a number of groups */
+		if (!ctrl->mpp_get && !ctrl->mpp_set) {
+			pctl->groups[gid].npins = 1;
+
+			for (k = 1; k < ctrl->npins; k++) {
+				gid++;
+				pctl->groups[gid].gid = gid;
+				pctl->groups[gid].ctrl = ctrl;
+				pctl->groups[gid].name = &ctrl->name[8*k];
+				pctl->groups[gid].pins = &ctrl->pins[k];
+				pctl->groups[gid].npins = 1;
+			}
+		}
+		gid++;
+	}
+
+	/* assign mpp modes to groups */
+	for (n = 0; n < soc->nmodes; n++) {
+		struct mvebu_mpp_mode *mode = &soc->modes[n];
+		struct mvebu_pinctrl_group *grp =
+			mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
+		unsigned num_settings;
+
+		if (!grp) {
+			dev_warn(&pdev->dev, "unknown pinctrl group %d\n",
+				mode->pid);
+			continue;
+		}
+
+		for (num_settings = 0; ;) {
+			struct mvebu_mpp_ctrl_setting *set =
+				&mode->settings[num_settings];
+
+			if (!set->name)
+				break;
+			num_settings++;
+
+			/* skip unsupported settings for this variant */
+			if (pctl->variant && !(pctl->variant & set->variant))
+				continue;
+
+			/* find gpio/gpo/gpi settings */
+			if (strcmp(set->name, "gpio") == 0)
+				set->flags = MVEBU_SETTING_GPI |
+					MVEBU_SETTING_GPO;
+			else if (strcmp(set->name, "gpo") == 0)
+				set->flags = MVEBU_SETTING_GPO;
+			else if (strcmp(set->name, "gpi") == 0)
+				set->flags = MVEBU_SETTING_GPI;
+		}
+
+		grp->settings = mode->settings;
+		grp->num_settings = num_settings;
+	}
+
+	ret = mvebu_pinctrl_dt_parse(pdev, pctl);
+	if (ret) {
+		/* look for pinmux functions in platform_device data */
+		dev_err(&pdev->dev, "unable to parse device tree\n");
+		return ret;
+	}
+
+	pctl->pctldev = pinctrl_register(&pctl->desc, &pdev->dev, pctl);
+	if (!pctl->pctldev) {
+		dev_err(&pdev->dev, "unable to register pinctrl driver\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "registered pinctrl driver\n");
+
+	/* register gpio ranges */
+	for (n = 0; n < soc->ngpioranges; n++)
+		pinctrl_add_gpio_range(pctl->pctldev, &soc->gpioranges[n]);
+
+	return 0;
+}
+
+int __devexit mvebu_pinctrl_remove(struct platform_device *pdev)
+{
+	struct mvebu_pinctrl *pctl = platform_get_drvdata(pdev);
+	pinctrl_unregister(pctl->pctldev);
+	return 0;
+}
diff --git a/drivers/pinctrl/pinctrl-mvebu.h b/drivers/pinctrl/pinctrl-mvebu.h
new file mode 100644
index 0000000..ee8057a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mvebu.h
@@ -0,0 +1,175 @@ 
+/*
+ * Marvell MVEBU pinctrl driver
+ *
+ * Authors: Sebastian Hesselbarth <[e-mail]>
+ *          Thomas Petazzoni <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __PINCTRL_MVEBU_H__
+#define __PINCTRL_MVEBU_H__
+
+/*
+ * struct mvebu_mpp_ctrl - describe a mpp control
+ * @name: name of the control group
+ * @pid: first pin id handled by this control
+ * @npins: number of pins controlled by this control
+ * @mpp_get: (optional) special function to get mpp setting
+ * @mpp_set: (optional) special function to set mpp setting
+ * @mpp_gpio_req: (optional) special function to request gpio
+ * @mpp_gpio_dir: (optional) special function to set gpio direction
+ *
+ * if optional mpp_get/_set functions are set these
+ * are used to get/set a specific mode. Otherwise it is
+ * assumed that the mpp control is based on 4-bit groups
+ * in subsequent registers.
+ *
+ * mpp_gpio_req/_dir functions can be used to allow pin settings
+ * with varying gpio pins.
+ */
+struct mvebu_mpp_ctrl {
+	const char *name;
+	u8 pid;
+	u8 npins;
+	unsigned *pins;
+	int (*mpp_get)(struct mvebu_mpp_ctrl *ctrl, unsigned long *config);
+	int (*mpp_set)(struct mvebu_mpp_ctrl *ctrl, unsigned long config);
+	int (*mpp_gpio_req)(struct mvebu_mpp_ctrl *ctrl, u8 pid);
+	int (*mpp_gpio_dir)(struct mvebu_mpp_ctrl *ctrl, u8 pid, bool input);
+};
+
+/*
+ * struct mvebu_mpp_ctrl_setting - describe a mpp ctrl setting
+ * @val: ctrl setting value
+ * @name: ctrl setting name, e.g. uart2, spi0 - unique per mpp_mode
+ * @subname: (optional) additional ctrl setting name, e.g. rts, cts
+ * @variant: (optional) variant identifier mask
+ * @flags: (private) flags to store gpi/gpo/gpio capabilities
+ *
+ * ctrl setting name will be used to switch to this setting in
+ * DT description, e.g. marvell,function = "uart2". subname
+ * is only for debugging purposes.
+ * If name is one of "gpi", "gpo", "gpio" gpio capabilities are
+ * parsed during initialization.
+ */
+struct mvebu_mpp_ctrl_setting {
+	u8 val;
+	const char *name;
+	const char *subname;
+	u8 variant;
+	u8 flags;
+#define  MVEBU_SETTING_GPO	(1 << 0)
+#define  MVEBU_SETTING_GPI	(1 << 1)
+};
+
+/*
+ * struct mvebu_mpp_mode - link ctrl and settings
+ * @pid: first pin id handled by this mode
+ * @settings: list of settings available for this mode
+ */
+struct mvebu_mpp_mode {
+	u8 pid;
+	struct mvebu_mpp_ctrl_setting *settings;
+};
+
+/*
+ * struct mvebu_pinctrl_soc_info -
+ *       SoC specific info passed to pinctrl-mvebu
+ * @variant: variant mask of soc_info
+ * @controls: list of available mvebu_mpp_ctrls
+ * @ncontrols: number of available mvebu_mpp_ctrls
+ * @modes: list of available mvebu_mpp_modes
+ * @nmodes: number of available mvebu_mpp_modes
+ * @gpioranges: list of pinctrl_gpio_ranges
+ * @ngpioranges: number of available pinctrl_gpio_ranges
+ */
+struct mvebu_pinctrl_soc_info {
+	u8 variant;
+	struct mvebu_mpp_ctrl *controls;
+	int ncontrols;
+	struct mvebu_mpp_mode *modes;
+	int nmodes;
+	struct pinctrl_gpio_range *gpioranges;
+	int ngpioranges;
+};
+
+#define MPP_REG_CTRL(_idl, _idh)				\
+	{							\
+		.name = NULL,					\
+		.pid = _idl,					\
+		.npins = _idh - _idl + 1,			\
+		.pins = (unsigned[_idh - _idl + 1]) { },	\
+		.mpp_get = NULL,				\
+		.mpp_set = NULL,				\
+		.mpp_gpio_req = NULL,				\
+		.mpp_gpio_dir = NULL,				\
+	}
+
+#define MPP_FUNC_CTRL(_idl, _idh, _name, _func)			\
+	{							\
+		.name = _name,					\
+		.pid = _idl,					\
+		.npins = _idh - _idl + 1,			\
+		.pins = (unsigned[_idh - _idl + 1]) { },	\
+		.mpp_get = _func ## _get,			\
+		.mpp_set = _func ## _set,			\
+		.mpp_gpio_req = NULL,				\
+		.mpp_gpio_dir = NULL,				\
+	}
+
+#define MPP_FUNC_GPIO_CTRL(_idl, _idh, _name, _func)		\
+	{							\
+		.name = _name,					\
+		.pid = _idl,					\
+		.npins = _idh - _idl + 1,			\
+		.pins = (unsigned[_idh - _idl + 1]) { },	\
+		.mpp_get = _func ## _get,			\
+		.mpp_set = _func ## _set,			\
+		.mpp_gpio_req = _func ## _gpio_req,		\
+		.mpp_gpio_dir = _func ## _gpio_dir,		\
+	}
+
+#define _MPP_VAR_FUNCTION(_val, _name, _subname, _mask)		\
+	{							\
+		.val = _val,					\
+		.name = _name,					\
+		.subname = _subname,				\
+		.variant = _mask,				\
+		.flags = 0,					\
+	}
+
+#if defined(CONFIG_DEBUG_FS)
+#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)		\
+	_MPP_VAR_FUNCTION(_val, _name, _subname, _mask)
+#else
+#define MPP_VAR_FUNCTION(_val, _name, _subname, _mask)		\
+	_MPP_VAR_FUNCTION(_val, _name, NULL, _mask)
+#endif
+
+#define MPP_FUNCTION(_val, _name, _subname)			\
+	MPP_VAR_FUNCTION(_val, _name, _subname, (u8)-1)
+
+#define MPP_MODE(_id, ...)					\
+	{							\
+		.pid = _id,					\
+		.settings = (struct mvebu_mpp_ctrl_setting[]){	\
+			__VA_ARGS__, { } },			\
+	}
+
+#define MPP_GPIO_RANGE(_id, _pinbase, _gpiobase, _npins)	\
+	{							\
+		.name = "mvebu-gpio",				\
+		.id = _id,					\
+		.pin_base = _pinbase,				\
+		.base = _gpiobase,				\
+		.npins = _npins,				\
+	}
+
+int mvebu_pinctrl_probe(struct platform_device *pdev);
+int mvebu_pinctrl_remove(struct platform_device *pdev);
+
+#endif