diff mbox

[RFC] pinctrl: add a driver for Energy Micro's efm32 SoCs

Message ID 1323384057-31452-1-git-send-email-u.kleine-koenig@pengutronix.de
State New
Headers show

Commit Message

Uwe Kleine-König Dec. 8, 2011, 10:40 p.m. UTC
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Note that there is no support yet for efm32 in mainline, so ARCH_EFM32
isn't defined.
---
 drivers/pinctrl/Kconfig                    |    6 +
 drivers/pinctrl/Makefile                   |    1 +
 drivers/pinctrl/pinmux-efm32.c             |  352 ++++++++++++++++++++++++++++
 include/linux/platform_data/efm32-pinctl.h |   60 +++++
 4 files changed, 419 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pinctrl/pinmux-efm32.c
 create mode 100644 include/linux/platform_data/efm32-pinctl.h

Comments

Arnd Bergmann Dec. 8, 2011, 10:55 p.m. UTC | #1
On Thursday 08 December 2011 23:40:57 Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Note that there is no support yet for efm32 in mainline, so ARCH_EFM32
> isn't defined.

Since the platform is not yet part of the architecture, it will have
to use device tree based probing when it gets added. I would suggest
only merging drivers for it that can deal with this and do not rely
on platform_data.

Other than that, the driver looks ok to me.

	Arnd
Stephen Warren Dec. 8, 2011, 11:14 p.m. UTC | #2
Uwe Kleine-König wrote at Thursday, December 08, 2011 3:41 PM:

> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig


> +config PINMUX_EFM32

> +	bool "EFM32 pinmux driver"

> +	depends on ARCH_EFM32

> +	default y

> +	select PINMUX


LinusW,

Since this is the pinctrl not pinmux subsystem now, does it make sense
to name the driver Kconfig options PINCTRL_FOO and the files pinctrl-
foo.c? I see that the coh901 driver already does that, and I followed
that convention in my Tegra pinctrl patches.

Uwe,

> diff --git a/drivers/pinctrl/pinmux-efm32.c b/drivers/pinctrl/pinmux-efm32.c


> +#define DRIVER_NAME "efm32-pinctl"


pinctrl (with an r) would match the subsystem name better.

> +static const char *efm32_pinctl_pctl_get_group_name(struct pinctrl_dev *pctldev,

> +		unsigned selector)

> +{

> +	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;

> +	const struct efm32_pinctl_pdata *pdata = ddata->pdata;

> +

> +	return pdata->groups[selector]->name;

> +}


Presumably, the set of pins, groups, and functions is determined by the
SoC HW. Platform data is usually board-specific rather than SoC specific.
You have two choices here: You could either parse this data from device
tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
or do what I've done in the Tegra pinctrl driver, and simply put each
SoC's data into the driver and select which one to use based on the DT
compatible flag; I didn't see the point of putting data in to the DT that
was identical for every board using a given SoC.

> +struct efm32_pmx_func {

> +	const char *name;

> +	const char **groups;

> +	const unsigned ngroups;

> +	const unsigned *mode;

> +};

> +

> +static const char *efm32_us1_groups[] = {

> +	"us1_loc0",

> +	"us1_loc1",

> +	"us1_loc2",

> +};

> +

> +/* order: TX, RX, CS, CLK */

> +static const unsigned efm32_us_modes[] = {

> +	EFM32_MODE_PUSHPULL_HIGH, EFM32_MODE_INPUT,

> +	EFM32_MODE_DISABLE, EFM32_MODE_DISABLE

> +};

> +

> +#define EFM32_PMXFUNC(_name, num) {					\

> +		.name = #_name #num,					\

> +		.groups = efm32_ ## _name ## num ## _groups,		\

> +		.ngroups = ARRAY_SIZE(efm32_ ## _name ## num ## _groups),\

> +		.mode = efm32_ ## _name ## _modes,			\

> +	}

> +

> +static const struct efm32_pmx_func efm32_pmx_funcs[] = {

> +	EFM32_PMXFUNC(us, 1),

> +};


If you're getting all your information from pdata, I'm not sure why
part of it is hard-coded into the driver...

> +static int efm32_pinctrl_pmx_enable(struct pinctrl_dev *pctldev,

> +		unsigned func_selector,

> +		unsigned group_selector)

> +{

> +	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;

> +

> +	const struct efm32_pmx_func *func;

> +	const char *groupname;

> +	const struct efm32_pinctl_group *group;

> +	unsigned i;

> +

> +	efm32_pinctrl_dbg(ddata, "%s(%u, %u)\n",

> +			__func__, func_selector, group_selector);

> +

> +	func = &efm32_pmx_funcs[func_selector];

> +	groupname = func->groups[group_selector];

> +	group = efm32_pinctrl_lookup_group(pctldev, groupname);


I'm not sure that's correct; group_selector is a global identifier, not
something that can only be interpreted relative to a particular function.
In other words, shouldn't those last 3 lines be:

func = &efm32_pmx_funcs[func_selector];
group = pdata->groups[group_selector];

or something like that?

> +static int __devinit efm32_pinctrl_probe(struct platform_device *pdev)

> +{

> +	int ret = -ENOMEM;

> +	struct efm32_pinctrl_ddata *ddata;

> +	const struct resource *res;

> +

> +       	ddata = kzalloc(sizeof(*ddata), GFP_KERNEL);


I think there's an indentation error there.

If you use devm_kzalloc, and also devm_ioremap below, you can avoid
having to write some of the cleanup code at all.

> +	ddata->pinctrldev = pinctrl_register(&ddata->pinctrldesc,

> +			&pdev->dev, ddata);

> +	if (!ddata->pinctrldev) {

> +		ret = -EINVAL;

> +		dev_dbg(&pdev->dev, "failed to register pinctrl device");

> +

> +		clk_unprepare(ddata->clk);

> +err_clk_prepare:

> +

> +		clk_put(ddata->clk);

> +err_clk_get:

> +

> +		iounmap(ddata->base);

> +err_ioremap:

> +err_get_base:

> +err_platdata:

> +		kfree(ddata);

> +	} else

> +		efm32_pinctrl_dbg(ddata, "initialized (%p, %p)\n", ddata, ddata->pinctrldev);

> +

> +err_ddata_kzalloc:

> +	return ret;

> +}


That's a pretty unusual code structure. You'd usually put all the err_foo:
labels right at the end of the function, and have even that last if()
condition jump to an error:

	ddata->pinctrldev = pinctrl_register(&ddata->pinctrldesc,
			&pdev->dev, ddata);
	if (!ddata->pinctrldev) {
		ret = -EINVAL;
		dev_dbg(&pdev->dev, "failed to register pinctrl device");
		goto err_register;
	} else
		efm32_pinctrl_dbg(ddata, "initialized (%p, %p)\n", ddata, ddata->pinctrldev);

	return 0;

err_register:
	clk_unprepare(ddata->clk);
err_clk_prepare:
	clk_put(ddata->clk);
err_clk_get:
	iounmap(ddata->base);
err_ioremap:
err_get_base:
err_platdata:
	kfree(ddata);
err_ddata_kzalloc:
	return ret;
}

... that keeps all the error if() blocks looking the same.

-- 
nvpbulic
Shawn Guo Dec. 9, 2011, 1:01 a.m. UTC | #3
On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
> Presumably, the set of pins, groups, and functions is determined by the
> SoC HW. Platform data is usually board-specific rather than SoC specific.
> You have two choices here: You could either parse this data from device
> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
> or do what I've done in the Tegra pinctrl driver, and simply put each
> SoC's data into the driver and select which one to use based on the DT
> compatible flag; I didn't see the point of putting data in to the DT that
> was identical for every board using a given SoC.
> 
I'm not sure about tegra, but for imx, it's very difficult to enumerate
all these data and list them in pinctrl driver.  Sascha gave a few
examples when we discussed about it in another thread.  The TX and RX
pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
data lines.  Display interface could have 16, 24, 32 data lines, etc.
All these options are chosen by board design for given soc pinmux
design.  So putting this data into device tree makes sense for imx too.
Stephen Warren Dec. 9, 2011, 3:44 a.m. UTC | #4
On 12/08/2011 06:01 PM, Shawn Guo wrote:
> On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
>> Presumably, the set of pins, groups, and functions is determined by the
>> SoC HW. Platform data is usually board-specific rather than SoC specific.
>> You have two choices here: You could either parse this data from device
>> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
>> or do what I've done in the Tegra pinctrl driver, and simply put each
>> SoC's data into the driver and select which one to use based on the DT
>> compatible flag; I didn't see the point of putting data in to the DT that
>> was identical for every board using a given SoC.
>>
> I'm not sure about tegra, but for imx, it's very difficult to enumerate
> all these data and list them in pinctrl driver.  Sascha gave a few
> examples when we discussed about it in another thread.  The TX and RX
> pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
> data lines.  Display interface could have 16, 24, 32 data lines, etc.
> All these options are chosen by board design for given soc pinmux
> design.  So putting this data into device tree makes sense for imx too.

The pinctrl driver should simply represent the raw options that the HW
supports on each individual pin. This table should be readily enumerable
(it's probably already in the SoC's datasheet), and of non-exponential
size. In this table, considerations such as display bus width do not
come into play; you just note that a certain 32 pins could support the
display function.

The board-specific selection of which function to use for each pin/group
(which is where the actual selection of e.g. 16/24/32-bit display bus
comes in) is provided by the board-specific mapping table, which I agree
makes perfect sense to put into device tree, since it's potentially
highly variable.
Shawn Guo Dec. 9, 2011, 4:32 a.m. UTC | #5
On Thu, Dec 08, 2011 at 08:44:03PM -0700, Stephen Warren wrote:
> On 12/08/2011 06:01 PM, Shawn Guo wrote:
> > On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
> >> Presumably, the set of pins, groups, and functions is determined by the
> >> SoC HW. Platform data is usually board-specific rather than SoC specific.
> >> You have two choices here: You could either parse this data from device
> >> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
> >> or do what I've done in the Tegra pinctrl driver, and simply put each
> >> SoC's data into the driver and select which one to use based on the DT
> >> compatible flag; I didn't see the point of putting data in to the DT that
> >> was identical for every board using a given SoC.
> >>
> > I'm not sure about tegra, but for imx, it's very difficult to enumerate
> > all these data and list them in pinctrl driver.  Sascha gave a few
> > examples when we discussed about it in another thread.  The TX and RX
> > pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
> > data lines.  Display interface could have 16, 24, 32 data lines, etc.
> > All these options are chosen by board design for given soc pinmux
> > design.  So putting this data into device tree makes sense for imx too.
> 
> The pinctrl driver should simply represent the raw options that the HW
> supports on each individual pin. This table should be readily enumerable

It is enumerable.  But I guess I said it's difficult (and not worth).

> (it's probably already in the SoC's datasheet), and of non-exponential
> size. In this table, considerations such as display bus width do not
> come into play; you just note that a certain 32 pins could support the
> display function.
> 
> The board-specific selection of which function to use for each pin/group
> (which is where the actual selection of e.g. 16/24/32-bit display bus
> comes in) is provided by the board-specific mapping table, which I agree
> makes perfect sense to put into device tree, since it's potentially
> highly variable.
> 
The situation is there might be a few possible pin groups for each
function of 16/24/32-bit display.   Let's think about the UART case
I put above, to enumerate all the groups, we will have 16 groups for
one UART instance.  And we have 5 UART ports on recent imx SoC.
Stephen Warren Dec. 9, 2011, 4:47 a.m. UTC | #6
On 12/08/2011 09:32 PM, Shawn Guo wrote:
> On Thu, Dec 08, 2011 at 08:44:03PM -0700, Stephen Warren wrote:
>> On 12/08/2011 06:01 PM, Shawn Guo wrote:
>>> On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
>>>> Presumably, the set of pins, groups, and functions is determined by the
>>>> SoC HW. Platform data is usually board-specific rather than SoC specific.
>>>> You have two choices here: You could either parse this data from device
>>>> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
>>>> or do what I've done in the Tegra pinctrl driver, and simply put each
>>>> SoC's data into the driver and select which one to use based on the DT
>>>> compatible flag; I didn't see the point of putting data in to the DT that
>>>> was identical for every board using a given SoC.
>>>>
>>> I'm not sure about tegra, but for imx, it's very difficult to enumerate
>>> all these data and list them in pinctrl driver.  Sascha gave a few
>>> examples when we discussed about it in another thread.  The TX and RX
>>> pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
>>> data lines.  Display interface could have 16, 24, 32 data lines, etc.
>>> All these options are chosen by board design for given soc pinmux
>>> design.  So putting this data into device tree makes sense for imx too.
>>
>> The pinctrl driver should simply represent the raw options that the HW
>> supports on each individual pin. This table should be readily enumerable
...
> The situation is there might be a few possible pin groups for each
> function of 16/24/32-bit display.   Let's think about the UART case
> I put above, to enumerate all the groups, we will have 16 groups for
> one UART instance.  And we have 5 UART ports on recent imx SoC.

I'm not familiar with imx. For my education, which of the following is true:

a)

* UART TX signal can muxed onto 1 of 16 pins
* UART RX signal can muxed onto 1 of 16 pins

In this case, you'd just list say "UART A TX" as a legal function for
each of those 16 pins. Yes, that's a lot of places, but still do-able.

(16 possible positions would be a lot, but I suppose it is possible in HW)

b)

* UART TX signal can muxed onto 1 of 4 pins
* UART RX signal can muxed onto 1 of 4 pins
* So, there are 16 (4*4) combinations of these two settings

In this case, you'd just list say "UART A TX" for the 4 pins, and "UART
A RX" for the other 4 pins. The 16 combinations don't come into play at
all; the board-specific mapping table would pick 1 single combination,
and represent this as:

pin X: function = UART A TX
pin Y: function = UART A RX

Either way though, whether you put the data into tables in the driver or
in the SoC .dtsi file (included by each board's .dts file), the pinctrl
driver's table really should list all possible options, and the pinctrl
mapping table should select just one; that's kinda the whole point of
the tables. If you try to put just the used subset into device tree, you
won't be able to put it into the .dtsi file since it'll be
board-specific, and you'll end up forcing board .dts authors to recreate
that driver's complete configuration table every time, which seems like
a lot of wasted work. (and yes, this might bloat the .dtsi file, and
mean a lot of useless device tree parsing, which is why I personally
chose not to put the SoC tables into the device tree).

Thanks.
Shawn Guo Dec. 9, 2011, 5:14 a.m. UTC | #7
On Thu, Dec 08, 2011 at 09:47:24PM -0700, Stephen Warren wrote:
> On 12/08/2011 09:32 PM, Shawn Guo wrote:
> > On Thu, Dec 08, 2011 at 08:44:03PM -0700, Stephen Warren wrote:
> >> On 12/08/2011 06:01 PM, Shawn Guo wrote:
> >>> On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
> >>>> Presumably, the set of pins, groups, and functions is determined by the
> >>>> SoC HW. Platform data is usually board-specific rather than SoC specific.
> >>>> You have two choices here: You could either parse this data from device
> >>>> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
> >>>> or do what I've done in the Tegra pinctrl driver, and simply put each
> >>>> SoC's data into the driver and select which one to use based on the DT
> >>>> compatible flag; I didn't see the point of putting data in to the DT that
> >>>> was identical for every board using a given SoC.
> >>>>
> >>> I'm not sure about tegra, but for imx, it's very difficult to enumerate
> >>> all these data and list them in pinctrl driver.  Sascha gave a few
> >>> examples when we discussed about it in another thread.  The TX and RX
> >>> pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
> >>> data lines.  Display interface could have 16, 24, 32 data lines, etc.
> >>> All these options are chosen by board design for given soc pinmux
> >>> design.  So putting this data into device tree makes sense for imx too.
> >>
> >> The pinctrl driver should simply represent the raw options that the HW
> >> supports on each individual pin. This table should be readily enumerable
> ...
> > The situation is there might be a few possible pin groups for each
> > function of 16/24/32-bit display.   Let's think about the UART case
> > I put above, to enumerate all the groups, we will have 16 groups for
> > one UART instance.  And we have 5 UART ports on recent imx SoC.
> 
> I'm not familiar with imx. For my education, which of the following is true:
> 

It's the case b).

> a)
> 
> * UART TX signal can muxed onto 1 of 16 pins
> * UART RX signal can muxed onto 1 of 16 pins
> 
> In this case, you'd just list say "UART A TX" as a legal function for
> each of those 16 pins. Yes, that's a lot of places, but still do-able.
> 
> (16 possible positions would be a lot, but I suppose it is possible in HW)
> 
> b)
> 
> * UART TX signal can muxed onto 1 of 4 pins
> * UART RX signal can muxed onto 1 of 4 pins
> * So, there are 16 (4*4) combinations of these two settings
> 
> In this case, you'd just list say "UART A TX" for the 4 pins, and "UART
> A RX" for the other 4 pins. The 16 combinations don't come into play at
> all; the board-specific mapping table would pick 1 single combination,
> and represent this as:
> 
> pin X: function = UART A TX
> pin Y: function = UART A RX
> 

We really do not expect per pin function mapping but per block function
mapping.  That said, the UART driver is going to tell pinctrl subsystem
"I'm UARTA, please set all my pins up for me".

> Either way though, whether you put the data into tables in the driver or
> in the SoC .dtsi file (included by each board's .dts file), the pinctrl

We are not going to list that into SoC .dtsi file.

> driver's table really should list all possible options, and the pinctrl
> mapping table should select just one; that's kinda the whole point of
> the tables. If you try to put just the used subset into device tree, you
> won't be able to put it into the .dtsi file since it'll be
> board-specific, and you'll end up forcing board .dts authors to recreate

That's what we are going to do.

> that driver's complete configuration table every time, which seems like
> a lot of wasted work.

If we understand that it's a board specific design for given SoC pinmux
superset, it's natural to me that the board (.dts) author should create
the subset for his board.

Regards,
Shawn

> (and yes, this might bloat the .dtsi file, and
> mean a lot of useless device tree parsing, which is why I personally
> chose not to put the SoC tables into the device tree).
>
Uwe Kleine-König Dec. 9, 2011, 9:23 a.m. UTC | #8
On Thu, Dec 08, 2011 at 11:55:53PM +0100, Arnd Bergmann wrote:
> On Thursday 08 December 2011 23:40:57 Uwe Kleine-König wrote:
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Note that there is no support yet for efm32 in mainline, so ARCH_EFM32
> > isn't defined.
> 
> Since the platform is not yet part of the architecture, it will have
> to use device tree based probing when it gets added. I would suggest
> only merging drivers for it that can deal with this and do not rely
> on platform_data.
I feared you will say that. These machines have at most 4MiB of RAM.
I'll have to evaluate if dt is too heavy for them.

Best regards
Uwe
Uwe Kleine-König Dec. 9, 2011, 9:31 a.m. UTC | #9
On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
> Uwe Kleine-König wrote at Thursday, December 08, 2011 3:41 PM:
> 
> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> 
> > +config PINMUX_EFM32
> > +	bool "EFM32 pinmux driver"
> > +	depends on ARCH_EFM32
> > +	default y
> > +	select PINMUX
> 
> LinusW,
> 
> Since this is the pinctrl not pinmux subsystem now, does it make sense
> to name the driver Kconfig options PINCTRL_FOO and the files pinctrl-
> foo.c? I see that the coh901 driver already does that, and I followed
> that convention in my Tegra pinctrl patches.
+1

> 
> Uwe,
> 
> > diff --git a/drivers/pinctrl/pinmux-efm32.c b/drivers/pinctrl/pinmux-efm32.c
> 
> > +#define DRIVER_NAME "efm32-pinctl"
> 
> pinctrl (with an r) would match the subsystem name better.
yeah, I saw some names with and some without r. I can switch to the
other variant for the next round.

> > +static const char *efm32_pinctl_pctl_get_group_name(struct pinctrl_dev *pctldev,
> > +		unsigned selector)
> > +{
> > +	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
> > +	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
> > +
> > +	return pdata->groups[selector]->name;
> > +}
> 
> Presumably, the set of pins, groups, and functions is determined by the
> SoC HW. Platform data is usually board-specific rather than SoC specific.
There is nothing stoping me putting both machine and SoC specific stuff
into pdata, isn't it?

> You have two choices here: You could either parse this data from device
> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
> or do what I've done in the Tegra pinctrl driver, and simply put each
> SoC's data into the driver and select which one to use based on the DT
> compatible flag; I didn't see the point of putting data in to the DT that
> was identical for every board using a given SoC.
> 
> > +struct efm32_pmx_func {
> > +	const char *name;
> > +	const char **groups;
> > +	const unsigned ngroups;
> > +	const unsigned *mode;
> > +};
> > +
> > +static const char *efm32_us1_groups[] = {
> > +	"us1_loc0",
> > +	"us1_loc1",
> > +	"us1_loc2",
> > +};
> > +
> > +/* order: TX, RX, CS, CLK */
> > +static const unsigned efm32_us_modes[] = {
> > +	EFM32_MODE_PUSHPULL_HIGH, EFM32_MODE_INPUT,
> > +	EFM32_MODE_DISABLE, EFM32_MODE_DISABLE
> > +};
> > +
> > +#define EFM32_PMXFUNC(_name, num) {					\
> > +		.name = #_name #num,					\
> > +		.groups = efm32_ ## _name ## num ## _groups,		\
> > +		.ngroups = ARRAY_SIZE(efm32_ ## _name ## num ## _groups),\
> > +		.mode = efm32_ ## _name ## _modes,			\
> > +	}
> > +
> > +static const struct efm32_pmx_func efm32_pmx_funcs[] = {
> > +	EFM32_PMXFUNC(us, 1),
> > +};
> 
> If you're getting all your information from pdata, I'm not sure why
> part of it is hard-coded into the driver...
I forgot to remove these. My first running driver had all the data
hardcoded as is done here. Then I switched to platform_data.

> > +static int efm32_pinctrl_pmx_enable(struct pinctrl_dev *pctldev,
> > +		unsigned func_selector,
> > +		unsigned group_selector)
> > +{
> > +	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
> > +
> > +	const struct efm32_pmx_func *func;
> > +	const char *groupname;
> > +	const struct efm32_pinctl_group *group;
> > +	unsigned i;
> > +
> > +	efm32_pinctrl_dbg(ddata, "%s(%u, %u)\n",
> > +			__func__, func_selector, group_selector);
> > +
> > +	func = &efm32_pmx_funcs[func_selector];
> > +	groupname = func->groups[group_selector];
> > +	group = efm32_pinctrl_lookup_group(pctldev, groupname);
> 
> I'm not sure that's correct; group_selector is a global identifier, not
> something that can only be interpreted relative to a particular function.
Ah, I thought it were relative to the function. IMHO that would make
sence. Then maybe we don't need the explicit per pin controller group
enumeration?!

> In other words, shouldn't those last 3 lines be:
> 
> func = &efm32_pmx_funcs[func_selector];
> group = pdata->groups[group_selector];
> 
> or something like that?
> 
> > +static int __devinit efm32_pinctrl_probe(struct platform_device *pdev)
> > +{
> > +	int ret = -ENOMEM;
> > +	struct efm32_pinctrl_ddata *ddata;
> > +	const struct resource *res;
> > +
> > +       	ddata = kzalloc(sizeof(*ddata), GFP_KERNEL);
> 
> I think there's an indentation error there.
uups, I shouldn't post patches that late in the night.
 
> If you use devm_kzalloc, and also devm_ioremap below, you can avoid
> having to write some of the cleanup code at all.
I'll look into that. I'm keen to save memory as long as it's reasonable
as the RAM size is quite tight. I need to do xip with (up to now) enough
flash. So a cleanup routine doesn't hurt much.
 
> > +	ddata->pinctrldev = pinctrl_register(&ddata->pinctrldesc,
> > +			&pdev->dev, ddata);
> > +	if (!ddata->pinctrldev) {
> > +		ret = -EINVAL;
> > +		dev_dbg(&pdev->dev, "failed to register pinctrl device");
> > +
> > +		clk_unprepare(ddata->clk);
> > +err_clk_prepare:
> > +
> > +		clk_put(ddata->clk);
> > +err_clk_get:
> > +
> > +		iounmap(ddata->base);
> > +err_ioremap:
> > +err_get_base:
> > +err_platdata:
> > +		kfree(ddata);
> > +	} else
> > +		efm32_pinctrl_dbg(ddata, "initialized (%p, %p)\n", ddata, ddata->pinctrldev);
> > +
> > +err_ddata_kzalloc:
> > +	return ret;
> > +}
> 
> That's a pretty unusual code structure. You'd usually put all the err_foo:
> labels right at the end of the function, and have even that last if()
> condition jump to an error:
> 
> 	ddata->pinctrldev = pinctrl_register(&ddata->pinctrldesc,
> 			&pdev->dev, ddata);
> 	if (!ddata->pinctrldev) {
> 		ret = -EINVAL;
> 		dev_dbg(&pdev->dev, "failed to register pinctrl device");
> 		goto err_register;
> 	} else
> 		efm32_pinctrl_dbg(ddata, "initialized (%p, %p)\n", ddata, ddata->pinctrldev);
> 
> 	return 0;
> 
> err_register:
> 	clk_unprepare(ddata->clk);
> err_clk_prepare:
> 	clk_put(ddata->clk);
> err_clk_get:
> 	iounmap(ddata->base);
> err_ioremap:
> err_get_base:
> err_platdata:
> 	kfree(ddata);
> err_ddata_kzalloc:
> 	return ret;
> }
> 
> ... that keeps all the error if() blocks looking the same.
personally I prefer my layout. It saves one goto :-)

Best regards
Uwe
Sascha Hauer Dec. 9, 2011, 11:08 a.m. UTC | #10
On Fri, Dec 09, 2011 at 01:14:14PM +0800, Shawn Guo wrote:
> On Thu, Dec 08, 2011 at 09:47:24PM -0700, Stephen Warren wrote:
> > 
> > pin X: function = UART A TX
> > pin Y: function = UART A RX
> > 
> 
> We really do not expect per pin function mapping but per block function
> mapping.  That said, the UART driver is going to tell pinctrl subsystem
> "I'm UARTA, please set all my pins up for me".

I still think it should be "I'm board Y, please set all my pins up for me"

Drivers should not be bothered with pin muxing *at all*. When we add
pinmux_request to drivers all SoCs using this driver are doomed to
implement pinmux support regardless if they have any pinmux support in
hardware or not.
Where this leads we can currently see in current next branch with the
smc91x driver where one single board has a regulator for.  Suddenly all
other boards using this driver stopped working because they have no
regulator provided for the driver.
There is this CONFIG_REGULATOR_DUMMY option which will cause *all*
regulator_request to succeed on all regulators in this binary which
disqualifies this option for multiboard/SoC Kernels.

A pin setup is board specific and not driver specific. There are only
some corner cases where a driver has to do something with the pin setup
like for example on i.MX6 where the drive strength setting has to be
changed with different card speeds.

There is also no value in being able to say which pins are allocated
by which driver. All we really want is being able to abstract the
pinmux in some common way and being able to detect conflicts.

Sascha
Uwe Kleine-König Dec. 9, 2011, 1:01 p.m. UTC | #11
On Fri, Dec 09, 2011 at 12:08:07PM +0100, Sascha Hauer wrote:
> On Fri, Dec 09, 2011 at 01:14:14PM +0800, Shawn Guo wrote:
> > On Thu, Dec 08, 2011 at 09:47:24PM -0700, Stephen Warren wrote:
> > > 
> > > pin X: function = UART A TX
> > > pin Y: function = UART A RX
> > > 
> > 
> > We really do not expect per pin function mapping but per block function
> > mapping.  That said, the UART driver is going to tell pinctrl subsystem
> > "I'm UARTA, please set all my pins up for me".
> 
> I still think it should be "I'm board Y, please set all my pins up for me"
> 
> Drivers should not be bothered with pin muxing *at all*. When we add
> pinmux_request to drivers all SoCs using this driver are doomed to
> implement pinmux support regardless if they have any pinmux support in
> hardware or not.
> Where this leads we can currently see in current next branch with the
> smc91x driver where one single board has a regulator for.  Suddenly all
> other boards using this driver stopped working because they have no
> regulator provided for the driver.
It's the same problem with clks. IMHO that's OK.

> There is this CONFIG_REGULATOR_DUMMY option which will cause *all*
> regulator_request to succeed on all regulators in this binary which
> disqualifies this option for multiboard/SoC Kernels.
> A pin setup is board specific and not driver specific. There are only
An upside of making drivers pinctrl-aware is that it might solve the
hardware conflicts on development boards. And the way pinctrl works it's
still the board that sets up the connection between device and how which
pins are configured.

Best regards
Uwe
Arnd Bergmann Dec. 9, 2011, 1:55 p.m. UTC | #12
On Friday 09 December 2011, Uwe Kleine-König wrote:
> On Thu, Dec 08, 2011 at 11:55:53PM +0100, Arnd Bergmann wrote:
> > On Thursday 08 December 2011 23:40:57 Uwe Kleine-König wrote:
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Note that there is no support yet for efm32 in mainline, so ARCH_EFM32
> > > isn't defined.
> > 
> > Since the platform is not yet part of the architecture, it will have
> > to use device tree based probing when it gets added. I would suggest
> > only merging drivers for it that can deal with this and do not rely
> > on platform_data.
>
> I feared you will say that. These machines have at most 4MiB of RAM.
> I'll have to evaluate if dt is too heavy for them.

What's the state of the platform code? Is this based on Catalin's
Cortex-M3 tree? I've heard a few people express interest in getting
various M3 based SoCs supported mainline and I think that would
be a good idea, but I would also expect that device tree is one
of the lesser problems here.

	Arnd
Uwe Kleine-König Dec. 9, 2011, 2:15 p.m. UTC | #13
On Fri, Dec 09, 2011 at 01:55:16PM +0000, Arnd Bergmann wrote:
> On Friday 09 December 2011, Uwe Kleine-König wrote:
> > On Thu, Dec 08, 2011 at 11:55:53PM +0100, Arnd Bergmann wrote:
> > > On Thursday 08 December 2011 23:40:57 Uwe Kleine-König wrote:
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Note that there is no support yet for efm32 in mainline, so ARCH_EFM32
> > > > isn't defined.
> > > 
> > > Since the platform is not yet part of the architecture, it will have
> > > to use device tree based probing when it gets added. I would suggest
> > > only merging drivers for it that can deal with this and do not rely
> > > on platform_data.
> >
> > I feared you will say that. These machines have at most 4MiB of RAM.
> > I'll have to evaluate if dt is too heavy for them.
> 
> What's the state of the platform code? Is this based on Catalin's
> Cortex-M3 tree? I've heard a few people express interest in getting
> various M3 based SoCs supported mainline and I think that would
> be a good idea, but I would also expect that device tree is one
> of the lesser problems here.
Yes, I took a few patches from there and rebased them to 3.2-rc3. The
patches I have from Catalin are:

131dbba Cortex-M3: Add base support for Cortex-M3
48cc6a8 Cortex-M3: Add support for exception handling
2aebf66 Cortex-M3: Add NVIC support
75bf249 Cortex-M3: Allow the building of Cortex-M3 kernel port
66dcc1e Cortex-M3: Add VFP support
a541241 [ARM] Do not call flush_cache_user_range with mmap_sem held
6083ea7 ARM: Handle instruction cache maintenance fault properly

I didn't look into these deeply yet to check how ready they are for
mainline, but my machine boots to a prompt (which doesn't allow to start
anything because it doesn't have enough memory to allocate the stack for
a new program. I will soon get a machine that has 4MiB of RAM instead of
the 1MiB I have now).

Best regards
Uwe
Dong Aisheng Dec. 9, 2011, 3:03 p.m. UTC | #14
2011/12/9 Stephen Warren <swarren@nvidia.com>:
> On 12/08/2011 09:32 PM, Shawn Guo wrote:
>> On Thu, Dec 08, 2011 at 08:44:03PM -0700, Stephen Warren wrote:
>>> On 12/08/2011 06:01 PM, Shawn Guo wrote:
>>>> On Thu, Dec 08, 2011 at 03:14:40PM -0800, Stephen Warren wrote:
>>>>> Presumably, the set of pins, groups, and functions is determined by the
>>>>> SoC HW. Platform data is usually board-specific rather than SoC specific.
>>>>> You have two choices here: You could either parse this data from device
>>>>> tree as Arnd suggested (and I think as the TI OMAP pinctrl driver will),
>>>>> or do what I've done in the Tegra pinctrl driver, and simply put each
>>>>> SoC's data into the driver and select which one to use based on the DT
>>>>> compatible flag; I didn't see the point of putting data in to the DT that
>>>>> was identical for every board using a given SoC.
>>>>>
>>>> I'm not sure about tegra, but for imx, it's very difficult to enumerate
>>>> all these data and list them in pinctrl driver.  Sascha gave a few
>>>> examples when we discussed about it in another thread.  The TX and RX
>>>> pin of UART has 4 options each.  SD could possibly have 1, 4, and 8
>>>> data lines.  Display interface could have 16, 24, 32 data lines, etc.
>>>> All these options are chosen by board design for given soc pinmux
>>>> design.  So putting this data into device tree makes sense for imx too.
>>>
>>> The pinctrl driver should simply represent the raw options that the HW
>>> supports on each individual pin. This table should be readily enumerable
> ...
>> The situation is there might be a few possible pin groups for each
>> function of 16/24/32-bit display.   Let's think about the UART case
>> I put above, to enumerate all the groups, we will have 16 groups for
>> one UART instance.  And we have 5 UART ports on recent imx SoC.
>
> I'm not familiar with imx. For my education, which of the following is true:
>
> a)
>
> * UART TX signal can muxed onto 1 of 16 pins
> * UART RX signal can muxed onto 1 of 16 pins
>
> In this case, you'd just list say "UART A TX" as a legal function for
> each of those 16 pins. Yes, that's a lot of places, but still do-able.
>
Yes,  it's a way to fix the issue Sascha raised.
But it will increase the code size heavily and using complexity.
Passing table from pdata or devicetree has another benifit that it will make
things simple since board knows which function uses which pin group,
that means you do not have to define all possible pin groups for one function.

> (16 possible positions would be a lot, but I suppose it is possible in HW)
>
> b)
>
> * UART TX signal can muxed onto 1 of 4 pins
> * UART RX signal can muxed onto 1 of 4 pins
> * So, there are 16 (4*4) combinations of these two settings
>
> In this case, you'd just list say "UART A TX" for the 4 pins, and "UART
> A RX" for the other 4 pins. The 16 combinations don't come into play at
> all; the board-specific mapping table would pick 1 single combination,
> and represent this as:
>
> pin X: function = UART A TX
> pin Y: function = UART A RX
>
> Either way though, whether you put the data into tables in the driver or
> in the SoC .dtsi file (included by each board's .dts file), the pinctrl
> driver's table really should list all possible options, and the pinctrl
> mapping table should select just one; that's kinda the whole point of
> the tables. If you try to put just the used subset into device tree, you
> won't be able to put it into the .dtsi file since it'll be
> board-specific, and you'll end up forcing board .dts authors to recreate
> that driver's complete configuration table every time, which seems like
> a lot of wasted work. (and yes, this might bloat the .dtsi file, and
> mean a lot of useless device tree parsing, which is why I personally
> chose not to put the SoC tables into the device tree).
>
I agree that parsing data from device tree increases much effort
because we can not using any help functions like microes in dts file and we have
to construct the structure we need in driver by parsing that data.
And it will increase dtsi file's size a lot when after adding all functions.

I wonder if we can define that data as a pdata in a .c file and only
get a pointer
from dts file. That would make life much easier.

Anyway, we're trying the way of parsing data from devicetree now for mx6q
pinmux driver.
I will send out patches soon when it's done for the discussion.

> Thanks.
>
> --
> nvpublic
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Stephen Warren Dec. 9, 2011, 4:49 p.m. UTC | #15
Dong Aisheng wrote at Friday, December 09, 2011 8:03 AM:
...
> I agree that parsing data from device tree increases much effort
> because we can not using any help functions like microes in dts file and we have
> to construct the structure we need in driver by parsing that data.
> And it will increase dtsi file's size a lot when after adding all functions.
> 
> I wonder if we can define that data as a pdata in a .c file and only
> get a pointer from dts file. That would make life much easier.

You can't get pointers themselves directly from a .dts file, since the
.dts file author has no idea what the memory layout (or even which OS)
is present on the running system.

An alternative is to key off the compatible flag in the device tree,
and use that to index a table that points at the various structures you
want pointers to.

struct of_device_id's .data field, and the function of_match_device()
are for exactly that purpose; see the way the Tegra pinctrl driver that
I posted uses them for an example of mapping compatible flag to per-
SoC initialization function pointers.
Stephen Warren Dec. 9, 2011, 4:53 p.m. UTC | #16
Shawn Guo wrote at Thursday, December 08, 2011 10:14 PM:
> On Thu, Dec 08, 2011 at 09:47:24PM -0700, Stephen Warren wrote:
...
> > I'm not familiar with imx. For my education, which of the following is true:
> >
> 
> It's the case b).
> 
> > a)
> >
> > * UART TX signal can muxed onto 1 of 16 pins
> > * UART RX signal can muxed onto 1 of 16 pins
> >
> > In this case, you'd just list say "UART A TX" as a legal function for
> > each of those 16 pins. Yes, that's a lot of places, but still do-able.
> >
> > (16 possible positions would be a lot, but I suppose it is possible in HW)
> >
> > b)
> >
> > * UART TX signal can muxed onto 1 of 4 pins
> > * UART RX signal can muxed onto 1 of 4 pins
> > * So, there are 16 (4*4) combinations of these two settings
> >
> > In this case, you'd just list say "UART A TX" for the 4 pins, and "UART
> > A RX" for the other 4 pins. The 16 combinations don't come into play at
> > all; the board-specific mapping table would pick 1 single combination,
> > and represent this as:
> >
> > pin X: function = UART A TX
> > pin Y: function = UART A RX
> 
> We really do not expect per pin function mapping but per block function
> mapping.  That said, the UART driver is going to tell pinctrl subsystem
> "I'm UARTA, please set all my pins up for me".

That's fine; UART A can operate that way with pinctrl used as intended.

The way the pinctrl subsystem is defined is:

The SoC-specific pinctrl driver exposes the per-pin available functions.

The board-specific mapping table selects the function to use for each pin,
based on the board's requirements.

Any other way isn't fully consistent with the way pinctrl works. If we
do see a need for just a single per-board table rather than a separate
per-SoC table, and a per-board mapping table, we should rethink the
pinctrl subsystem...

> > Either way though, whether you put the data into tables in the driver or
> > in the SoC .dtsi file (included by each board's .dts file), the pinctrl
> 
> We are not going to list that into SoC .dtsi file.
> 
> > driver's table really should list all possible options, and the pinctrl
> > mapping table should select just one; that's kinda the whole point of
> > the tables. If you try to put just the used subset into device tree, you
> > won't be able to put it into the .dtsi file since it'll be
> > board-specific, and you'll end up forcing board .dts authors to recreate
> 
> That's what we are going to do.
> 
> > that driver's complete configuration table every time, which seems like
> > a lot of wasted work.

I re-iterate that point...
Tony Lindgren Dec. 9, 2011, 5:24 p.m. UTC | #17
* Stephen Warren <swarren@nvidia.com> [111209 08:17]:
> Dong Aisheng wrote at Friday, December 09, 2011 8:03 AM:
> ...
> > I agree that parsing data from device tree increases much effort
> > because we can not using any help functions like microes in dts file and we have
> > to construct the structure we need in driver by parsing that data.
> > And it will increase dtsi file's size a lot when after adding all functions.
> > 
> > I wonder if we can define that data as a pdata in a .c file and only
> > get a pointer from dts file. That would make life much easier.
> 
> You can't get pointers themselves directly from a .dts file, since the
> .dts file author has no idea what the memory layout (or even which OS)
> is present on the running system.
> 
> An alternative is to key off the compatible flag in the device tree,
> and use that to index a table that points at the various structures you
> want pointers to.
> 
> struct of_device_id's .data field, and the function of_match_device()
> are for exactly that purpose; see the way the Tegra pinctrl driver that
> I posted uses them for an example of mapping compatible flag to per-
> SoC initialization function pointers.

For letting a device do it's pingroup in DT, I've played with the
following:

	/*      mux func phandle mux func name    hw initial flags */
	pins = <&uart3_rx_irrx>, "uart3_rx_irrx", <0xdeadbeef>,
		<&uart3_tx_irtx>, "uart3_tx_irtx", <0xdeadbeef>;

But it seems that doing mixed-property arrays gets nasty as any
data after a string has a high likelihood of getting misaligned
since a mixed-property array is a string array with the string
embedded in it with the registers. That leads into nasty string
parsing that can be extremely flakey with buggy DT data. So it
seems that using a string there is only safe as the last element,
which means we can't use names for multiple pins.

So I've pretty much come to the conclusion that we would have to
use something like this instead:

	/*      phandle        f hw specific initial flags */
	pins = <&uart3_rx_irrx 0 0xdeadbeef
		&uart3_tx_irtx 0 0xdeadbeef>;

This however has a problem for cases where we may not have a phandle
in DT for the mux function. For example, let's assume that we'll have
tens of thousands of lines of mux data for omaps (we already have
over 6k LOC) and just want to load that from /lib/firmware to avoid
bloating the kernel. In that case we won't have the phandle for the
mux function in DT.

It would be _really_nice_ to use DT in the missing phandle case also
for doing the device to mux function mapping based on a function name
when no phandle is available.

But so far I have not found a good way of doing both phandle and
optional name type thing. Anybody got some good ideas for that?

BTW, this same problem applies also to clock framwork where we may
have multiple sources of clocks to register and map to devices.

Regards,

Tony
Tony Lindgren Dec. 9, 2011, 5:53 p.m. UTC | #18
* Tony Lindgren <tony@atomide.com> [111209 08:53]:
> * Stephen Warren <swarren@nvidia.com> [111209 08:17]:
> > Dong Aisheng wrote at Friday, December 09, 2011 8:03 AM:
> > ...
> > > I agree that parsing data from device tree increases much effort
> > > because we can not using any help functions like microes in dts file and we have
> > > to construct the structure we need in driver by parsing that data.
> > > And it will increase dtsi file's size a lot when after adding all functions.
> > > 
> > > I wonder if we can define that data as a pdata in a .c file and only
> > > get a pointer from dts file. That would make life much easier.
> > 
> > You can't get pointers themselves directly from a .dts file, since the
> > .dts file author has no idea what the memory layout (or even which OS)
> > is present on the running system.
> > 
> > An alternative is to key off the compatible flag in the device tree,
> > and use that to index a table that points at the various structures you
> > want pointers to.
> > 
> > struct of_device_id's .data field, and the function of_match_device()
> > are for exactly that purpose; see the way the Tegra pinctrl driver that
> > I posted uses them for an example of mapping compatible flag to per-
> > SoC initialization function pointers.
> 
> For letting a device do it's pingroup in DT, I've played with the
> following:
> 
> 	/*      mux func phandle mux func name    hw initial flags */
> 	pins = <&uart3_rx_irrx>, "uart3_rx_irrx", <0xdeadbeef>,
> 		<&uart3_tx_irtx>, "uart3_tx_irtx", <0xdeadbeef>;
> 
> But it seems that doing mixed-property arrays gets nasty as any
> data after a string has a high likelihood of getting misaligned
> since a mixed-property array is a string array with the string
> embedded in it with the registers. That leads into nasty string
> parsing that can be extremely flakey with buggy DT data. So it
> seems that using a string there is only safe as the last element,
> which means we can't use names for multiple pins.
> 
> So I've pretty much come to the conclusion that we would have to
> use something like this instead:
> 
> 	/*      phandle        f hw specific initial flags */
> 	pins = <&uart3_rx_irrx 0 0xdeadbeef
> 		&uart3_tx_irtx 0 0xdeadbeef>;
> 
> This however has a problem for cases where we may not have a phandle
> in DT for the mux function. For example, let's assume that we'll have
> tens of thousands of lines of mux data for omaps (we already have
> over 6k LOC) and just want to load that from /lib/firmware to avoid
> bloating the kernel. In that case we won't have the phandle for the
> mux function in DT.
> 
> It would be _really_nice_ to use DT in the missing phandle case also
> for doing the device to mux function mapping based on a function name
> when no phandle is available.
> 
> But so far I have not found a good way of doing both phandle and
> optional name type thing. Anybody got some good ideas for that?

Oh forgot to mention of course what Benoit came up which is reg-names.
That of course is doable with both phandles and pin names:

So optionally either

	pins = <&uart3_rx_irrx &uart3_tx_irtx>;
or

	pin-names = "uart3_rx_irrx",  "uart3_tx_irtx";

and both pins and pin-names could have optional hardware specific
board default flags:

	pin-flags = <0xdeadbeef 0xdeadbeef>;

The pin-flags could end up being defines if dtc will at some point
support preprocessing the .dtc files.
 
> BTW, this same problem applies also to clock framwork where we may
> have multiple sources of clocks to register and map to devices.

Regards,

Tony
Linus Walleij Dec. 10, 2011, 12:04 a.m. UTC | #19
2011/12/9 Stephen Warren <swarren@nvidia.com>:

> Uwe Kleine-König wrote at Thursday, December 08, 2011 3:41 PM:
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>
>> +config PINMUX_EFM32
>> +     bool "EFM32 pinmux driver"
>> +     depends on ARCH_EFM32
>> +     default y
>> +     select PINMUX
>
> LinusW,
>
> Since this is the pinctrl not pinmux subsystem now, does it make sense
> to name the driver Kconfig options PINCTRL_FOO and the files pinctrl-
> foo.c? I see that the coh901 driver already does that, and I followed
> that convention in my Tegra pinctrl patches.

Hm, the U300 pinmux is named pinmux-u300.c because it can only do
pinmux. It cannot do anything else. But if it's confusing we can surely
name everything pinctrl-*.

Yours,
Linus Walleij
Stephen Warren Dec. 10, 2011, 12:14 a.m. UTC | #20
Tony Lindgren wrote at Friday, December 09, 2011 10:53 AM:
> * Tony Lindgren <tony@atomide.com> [111209 08:53]:
...
> > For letting a device do it's pingroup in DT, I've played with the
> > following:
> >
> > 	/*      mux func phandle mux func name    hw initial flags */
> > 	pins = <&uart3_rx_irrx>, "uart3_rx_irrx", <0xdeadbeef>,
> > 		<&uart3_tx_irtx>, "uart3_tx_irtx", <0xdeadbeef>;
> >
> > But it seems that doing mixed-property arrays gets nasty as any
...
> > So I've pretty much come to the conclusion that we would have to
> > use something like this instead:
> >
> > 	/*      phandle        f hw specific initial flags */
> > 	pins = <&uart3_rx_irrx 0 0xdeadbeef
> > 		&uart3_tx_irtx 0 0xdeadbeef>;
> >
> > This however has a problem for cases where we may not have a phandle
> > in DT for the mux function. For example, let's assume that we'll have
> > tens of thousands of lines of mux data for omaps (we already have
> > over 6k LOC) and just want to load that from /lib/firmware to avoid
> > bloating the kernel. In that case we won't have the phandle for the
> > mux function in DT.
...
> Oh forgot to mention of course what Benoit came up which is reg-names.
> That of course is doable with both phandles and pin names:
> 
> So optionally either
> 
> 	pins = <&uart3_rx_irrx &uart3_tx_irtx>;
> or
> 
> 	pin-names = "uart3_rx_irrx",  "uart3_tx_irtx";

Tony,

It sounds like you've already started working on some DT bindings for
pin muxing. Are you just at the thinking stage as above, or do you have
any concrete code? I'd obviously be interested in looking at any early
bindings to see if I can port the Tegra pinctrl driver to use them.

Thanks.
Linus Walleij Dec. 10, 2011, 12:18 a.m. UTC | #21
On Fri, Dec 9, 2011 at 12:08 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> I still think it should be "I'm board Y, please set all my pins up for me"

This is basically what the current pinmux hog concept does.

> Drivers should not be bothered with pin muxing *at all*.

Not if they are simple. Like - set them up once and for all and
then forget about it.

It gets problematic when we get to sleep states and the system
need to reconfigure all pins on say idle or deep sleep.

We also have heard about hardware routing the *same*
IO block (an I2C port IIRC) to two different sets of pins at
*runtime*, i.e. route it to pin 1-4 do some traffic, route
it to pin 9-12, do some other traffic...

> There are only
> some corner cases where a driver has to do something with the pin setup
> like for example on i.MX6 where the drive strength setting has to be
> changed with different card speeds.

I fear that is just the top of an iceberg :-P

> There is also no value in being able to say which pins are allocated
> by which driver. All we really want is being able to abstract the
> pinmux in some common way and being able to detect conflicts.

It's a value to have it in debugfs, where it is right now, and that
is optional. It helped me a lot when developing a driver...

Thanks,
Linus Walleij
Tony Lindgren Dec. 11, 2011, 7:34 p.m. UTC | #22
* Stephen Warren <swarren@nvidia.com> [111209 15:42]:
> Tony Lindgren wrote at Friday, December 09, 2011 10:53 AM:
> > * Tony Lindgren <tony@atomide.com> [111209 08:53]:
> ...
> > > For letting a device do it's pingroup in DT, I've played with the
> > > following:
> > >
> > > 	/*      mux func phandle mux func name    hw initial flags */
> > > 	pins = <&uart3_rx_irrx>, "uart3_rx_irrx", <0xdeadbeef>,
> > > 		<&uart3_tx_irtx>, "uart3_tx_irtx", <0xdeadbeef>;
> > >
> > > But it seems that doing mixed-property arrays gets nasty as any
> ...
> > > So I've pretty much come to the conclusion that we would have to
> > > use something like this instead:
> > >
> > > 	/*      phandle        f hw specific initial flags */
> > > 	pins = <&uart3_rx_irrx 0 0xdeadbeef
> > > 		&uart3_tx_irtx 0 0xdeadbeef>;
> > >
> > > This however has a problem for cases where we may not have a phandle
> > > in DT for the mux function. For example, let's assume that we'll have
> > > tens of thousands of lines of mux data for omaps (we already have
> > > over 6k LOC) and just want to load that from /lib/firmware to avoid
> > > bloating the kernel. In that case we won't have the phandle for the
> > > mux function in DT.
> ...
> > Oh forgot to mention of course what Benoit came up which is reg-names.
> > That of course is doable with both phandles and pin names:
> > 
> > So optionally either
> > 
> > 	pins = <&uart3_rx_irrx &uart3_tx_irtx>;
> > or
> > 
> > 	pin-names = "uart3_rx_irrx",  "uart3_tx_irtx";
> 
> Tony,
> 
> It sounds like you've already started working on some DT bindings for
> pin muxing. Are you just at the thinking stage as above, or do you have
> any concrete code? I'd obviously be interested in looking at any early
> bindings to see if I can port the Tegra pinctrl driver to use them.

Yeah I have some code but it still needs a bit more work.. The DT bindings
part I have only briefly played with so far. Mostly to verify that trying
to use the mixed-property arrays will get messy.

For parsing the pins as phandles, drivers/of/gpio.c is a nice example.

After still thinking more about it, I think the cleanest and most flexible
option is using both the pins phandle array or pin-names string array,
combined with a separate pin-flags initial value array. That way we can
use the DT generated maps also for non-dt pinmux drivers where we don't
have phandles around.

Regards,

Tony
Sascha Hauer Dec. 12, 2011, 2:37 p.m. UTC | #23
On Sat, Dec 10, 2011 at 01:18:43AM +0100, Linus Walleij wrote:
> On Fri, Dec 9, 2011 at 12:08 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > I still think it should be "I'm board Y, please set all my pins up for me"
> 
> This is basically what the current pinmux hog concept does.
> 
> > Drivers should not be bothered with pin muxing *at all*.
> 
> Not if they are simple. Like - set them up once and for all and
> then forget about it.
> 
> It gets problematic when we get to sleep states and the system
> need to reconfigure all pins on say idle or deep sleep.

If you need to reconfigure your pins in deep sleep and you control
the pins in the driver this seems to suggest that your device won't
suspend properly if the driver is not loaded?

Sascha
Uwe Kleine-König Dec. 12, 2011, 3:29 p.m. UTC | #24
On Mon, Dec 12, 2011 at 03:37:50PM +0100, Sascha Hauer wrote:
> On Sat, Dec 10, 2011 at 01:18:43AM +0100, Linus Walleij wrote:
> > On Fri, Dec 9, 2011 at 12:08 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > I still think it should be "I'm board Y, please set all my pins up for me"
> > 
> > This is basically what the current pinmux hog concept does.
> > 
> > > Drivers should not be bothered with pin muxing *at all*.
> > 
> > Not if they are simple. Like - set them up once and for all and
> > then forget about it.
> > 
> > It gets problematic when we get to sleep states and the system
> > need to reconfigure all pins on say idle or deep sleep.
> 
> If you need to reconfigure your pins in deep sleep and you control
> the pins in the driver this seems to suggest that your device won't
> suspend properly if the driver is not loaded?
If the pins have to be inactive (for some definition of inactive) for
sleep it should work well without the driver loaded, shouldn't it?

Best regards
Uwe
Linus Walleij Dec. 13, 2011, 12:41 a.m. UTC | #25
On Mon, Dec 12, 2011 at 3:37 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Sat, Dec 10, 2011 at 01:18:43AM +0100, Linus Walleij wrote:
>> On Fri, Dec 9, 2011 at 12:08 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>>
>> > Drivers should not be bothered with pin muxing *at all*.
>>
>> Not if they are simple. Like - set them up once and for all and
>> then forget about it.
>>
>> It gets problematic when we get to sleep states and the system
>> need to reconfigure all pins on say idle or deep sleep.
>
> If you need to reconfigure your pins in deep sleep and you control
> the pins in the driver this seems to suggest that your device won't
> suspend properly if the driver is not loaded?

In our case (ux500) it's more like all pins have a defined nice
state for active, sleep and idle. (Pull-ups, downs, wakeups etc
set in a certain way.)

Then there are some real agressive stuff PM people start to do.
Some of them require you to shut down things in a certain
sequence, like this device has to have its pins turned off first,
the clock released, then regulator, etc. And others do it in the
reverse order, or any order.

If all your hardware was engineered the
same way with this in mind it could probably be done
centrally, like the nice stuff Magnus Damm has for managing
clocks in drivers/base/power/clock_ops.c for all SHmobile
clocks. Would be real nice if it worked for us, sadly not all
hardware is designed in such a consistent way.

For our hardware it's better to spread it out across the
drivers and then consolidate once we have the entire
picture, I'm afraid. For others it will be different, maybe
your systems will never need it and you can keep all in
central places.

Linus Walleij
diff mbox

Patch

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index e17e2f8..5067056 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -20,6 +20,12 @@  config DEBUG_PINCTRL
 	help
 	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
 
+config PINMUX_EFM32
+	bool "EFM32 pinmux driver"
+	depends on ARCH_EFM32
+	default y
+	select PINMUX
+
 config PINMUX_SIRF
 	bool "CSR SiRFprimaII pinmux driver"
 	depends on ARCH_PRIMA2
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 50a2e2f..61846b0 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -4,5 +4,6 @@  ccflags-$(CONFIG_DEBUG_PINCTRL)	+= -DDEBUG
 
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
+obj-$(CONFIG_PINMUX_EFM32)	+= pinmux-efm32.o
 obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
 obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
diff --git a/drivers/pinctrl/pinmux-efm32.c b/drivers/pinctrl/pinmux-efm32.c
new file mode 100644
index 0000000..2cb1ba5
--- /dev/null
+++ b/drivers/pinctrl/pinmux-efm32.c
@@ -0,0 +1,352 @@ 
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/efm32-pinctl.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+#include "core.h"
+
+#define DRIVER_NAME "efm32-pinctl"
+
+#define EFM32_REG_CTRL		0x00
+#define EFM32_REG_MODEL		0x04
+#define EFM32_REG_MODEH		0x04
+#define EFM32_REG_DOUTSET	0x10
+#define EFM32_REG_DOUTCLR	0x14
+
+/*
+ * The lower 4 bits of these values go into the MODE register,
+ * the 5th into DOUT if the 6th bit is set
+ */
+#define EFM32_MODE_DISABLE		0x20
+#define EFM32_MODE_INPUT		0x21
+#define EFM32_MODE_PUSHPULL		0x04
+#define EFM32_MODE_PUSHPULL_LOW		0x24
+#define EFM32_MODE_PUSHPULL_HIGH	0x34
+
+struct efm32_pinctrl_ddata {
+	struct pinctrl_desc pinctrldesc;
+	const struct efm32_pinctl_pdata *pdata;
+	struct platform_device *pdev;
+	struct pinctrl_dev *pinctrldev;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+#define efm32_pinctrl_dbg(ddata, fmt, arg...)				\
+	dev_dbg(&ddata->pdev->dev, fmt, ##arg)
+
+static int efm32_pinctl_pctl_list_groups(struct pinctrl_dev *pctldev,
+		unsigned selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	if (selector >= pdata->ngroups)
+		return -EINVAL;
+	return 0;
+}
+
+static const char *efm32_pinctl_pctl_get_group_name(struct pinctrl_dev *pctldev,
+		unsigned selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	return pdata->groups[selector]->name;
+}
+
+static int efm32_pinctl_pctl_get_group_pins(struct pinctrl_dev *pctldev,
+		unsigned selector,
+		const unsigned **pins,
+		unsigned *npins)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	*pins = pdata->groups[selector]->pins;
+	*npins = pdata->groups[selector]->npins;
+
+	return 0;
+}
+
+#if defined(CONFIG_DEBUG_FS)
+static void efm32_pinctl_pctl_pin_dbg_show(struct pinctrl_dev *pctldev,
+		struct seq_file *s, unsigned offset)
+{
+	seq_printf(s, " " DRIVER_NAME);
+}
+#else
+#define efm32_pinctl_pctl_pin_dbg_show NULL
+#endif
+
+static struct pinctrl_ops efm32_pinctrl_pctlops = {
+	.list_groups = efm32_pinctl_pctl_list_groups,
+	.get_group_name = efm32_pinctl_pctl_get_group_name,
+	.get_group_pins = efm32_pinctl_pctl_get_group_pins,
+	.pin_dbg_show = efm32_pinctl_pctl_pin_dbg_show,
+};
+
+struct efm32_pmx_func {
+	const char *name;
+	const char **groups;
+	const unsigned ngroups;
+	const unsigned *mode;
+};
+
+static const char *efm32_us1_groups[] = {
+	"us1_loc0",
+	"us1_loc1",
+	"us1_loc2",
+};
+
+/* order: TX, RX, CS, CLK */
+static const unsigned efm32_us_modes[] = {
+	EFM32_MODE_PUSHPULL_HIGH, EFM32_MODE_INPUT,
+	EFM32_MODE_DISABLE, EFM32_MODE_DISABLE
+};
+
+#define EFM32_PMXFUNC(_name, num) {					\
+		.name = #_name #num,					\
+		.groups = efm32_ ## _name ## num ## _groups,		\
+		.ngroups = ARRAY_SIZE(efm32_ ## _name ## num ## _groups),\
+		.mode = efm32_ ## _name ## _modes,			\
+	}
+
+static const struct efm32_pmx_func efm32_pmx_funcs[] = {
+	EFM32_PMXFUNC(us, 1),
+};
+
+static int efm32_pinctrl_pmx_list_functions(struct pinctrl_dev *pctldev,
+	       	unsigned selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	if (selector >= pdata->nfuncs)
+		return -EINVAL;
+	return 0;
+}
+
+static const char *efm32_pinctrl_pmx_get_function_name(
+		struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	return pdata->funcs[selector].name;
+}
+
+static int efm32_pinctrl_pmx_get_function_groups(
+		struct pinctrl_dev *pctldev,
+		unsigned selector,
+		const char * const **groups,
+		unsigned * const ngroups)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	*groups = pdata->funcs[selector].groups;
+	*ngroups = pdata->funcs[selector].ngroups;
+	return 0;
+}
+
+static void efm32_pinctrl_pmx_config(struct efm32_pinctrl_ddata *ddata,
+		unsigned pin, unsigned mode)
+{
+	unsigned bank = pin / 16;
+	unsigned bankpin = pin % 16;
+	unsigned bank_regoff = bank * 0x24;
+	unsigned mode_regoff = bankpin < 8 ? EFM32_REG_MODEL : EFM32_REG_MODEH;
+	unsigned dout_regoff =
+		mode & 0x10 ? EFM32_REG_DOUTSET : EFM32_REG_DOUTCLR;
+	u32 regmode;
+
+	efm32_pinctrl_dbg(ddata, "config(%u, 0x%x)\n", pin, mode);
+
+	/*
+	 * first set/unset DOUT unless the pin will be disabled. This prevents
+	 * most glitches in practise.
+	 */
+	if (mode & 0x20 && mode & 0xf)
+		writel(1 << bankpin, ddata->base + bank_regoff + dout_regoff);
+
+	regmode = readl(ddata->base + bank_regoff + mode_regoff);
+
+	regmode &= ~(0xf << (4 * (bankpin % 8)));
+	regmode |= (mode & 0xf) << (4 * (bankpin % 8));
+
+	efm32_pinctrl_dbg(ddata, "[%03x] <- %08x\n",
+		       	bank_regoff + mode_regoff, regmode);
+	writel(regmode, ddata->base + bank_regoff + mode_regoff);
+
+	if (mode & 0x20 && !(mode & 0xf))
+		writel(1 << bankpin, ddata->base + bank_regoff + dout_regoff);
+}
+
+static const struct efm32_pinctl_group *efm32_pinctrl_lookup_group(
+		struct pinctrl_dev *pctldev, const char *name)
+{
+	unsigned i;
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+	const struct efm32_pinctl_pdata *pdata = ddata->pdata;
+
+	for (i = 0; i < pdata->ngroups; ++i) {
+		if (!strcmp(pdata->groups[i]->name, name))
+			return pdata->groups[i];
+	}
+
+	return NULL;
+}
+
+static int efm32_pinctrl_pmx_enable(struct pinctrl_dev *pctldev,
+		unsigned func_selector,
+		unsigned group_selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+
+	const struct efm32_pmx_func *func;
+	const char *groupname;
+	const struct efm32_pinctl_group *group;
+	unsigned i;
+
+	efm32_pinctrl_dbg(ddata, "%s(%u, %u)\n",
+			__func__, func_selector, group_selector);
+
+	func = &efm32_pmx_funcs[func_selector];
+	groupname = func->groups[group_selector];
+	group = efm32_pinctrl_lookup_group(pctldev, groupname);
+
+	if (!group)
+		return -EINVAL;
+
+	for (i = 0; i < group->npins; ++i)
+		efm32_pinctrl_pmx_config(ddata, group->pins[i], func->mode[i]);
+
+	return 0;
+}
+
+static void efm32_pinctrl_pmx_disable(struct pinctrl_dev *pctldev,
+		unsigned func_selector,
+		unsigned group_selector)
+{
+	struct efm32_pinctrl_ddata *ddata = pctldev->driver_data;
+
+	const struct efm32_pmx_func *func = &efm32_pmx_funcs[func_selector];
+	const char *groupname = func->groups[group_selector];
+	const struct efm32_pinctl_group *group = efm32_pinctrl_lookup_group(pctldev, groupname);
+	unsigned i;
+
+	for (i = 0; i < group->npins; ++i)
+		efm32_pinctrl_pmx_config(ddata, group->pins[i],
+				EFM32_MODE_DISABLE);
+}
+
+static struct pinmux_ops efm32_pinctrl_pmxops = {
+	.list_functions = efm32_pinctrl_pmx_list_functions,
+	.get_function_name = efm32_pinctrl_pmx_get_function_name,
+	.get_function_groups = efm32_pinctrl_pmx_get_function_groups,
+	.enable = efm32_pinctrl_pmx_enable,
+	.disable = efm32_pinctrl_pmx_disable,
+};
+
+static int __devinit efm32_pinctrl_probe(struct platform_device *pdev)
+{
+	int ret = -ENOMEM;
+	struct efm32_pinctrl_ddata *ddata;
+	const struct resource *res;
+
+       	ddata = kzalloc(sizeof(*ddata), GFP_KERNEL);
+	if (!ddata) {
+		dev_dbg(&pdev->dev, "allocating ddata failed\n");
+		goto err_ddata_kzalloc;
+	}
+
+	ddata->pdev = pdev;
+	ddata->pdata = dev_get_platdata(&pdev->dev);
+
+	if (!ddata->pdata) {
+		dev_dbg(&pdev->dev, "no platform data\n");
+		goto err_platdata;
+	}
+
+	ddata->pinctrldesc.name = DRIVER_NAME;
+	ddata->pinctrldesc.pins = ddata->pdata->pins;
+	ddata->pinctrldesc.npins = ddata->pdata->npins;
+	ddata->pinctrldesc.maxpin = ddata->pdata->npins;
+	ddata->pinctrldesc.pctlops = &efm32_pinctrl_pctlops;
+	ddata->pinctrldesc.pmxops = &efm32_pinctrl_pmxops;
+	ddata->pinctrldesc.owner = THIS_MODULE;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENODEV;
+		dev_dbg(&pdev->dev, "getting base address failed\n");
+		goto err_get_base;
+	}
+
+	ddata->base = ioremap(res->start, 0x140);
+	if (!ddata->base) {
+		ret = -ENOMEM;
+		dev_dbg(&pdev->dev, "failed to remap\n");
+		goto err_ioremap;
+	}
+
+	ddata->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ddata->clk)) {
+		ret = PTR_ERR(ddata->clk);
+		dev_dbg(&pdev->dev, "failed to get clock\n");
+		goto err_clk_get;
+	}
+
+	ret = clk_prepare(ddata->clk);
+	if (ret) {
+		dev_dbg(&pdev->dev, "failed to prepare clock\n");
+		goto err_clk_prepare;
+	}
+
+	ddata->pinctrldev = pinctrl_register(&ddata->pinctrldesc,
+			&pdev->dev, ddata);
+	if (!ddata->pinctrldev) {
+		ret = -EINVAL;
+		dev_dbg(&pdev->dev, "failed to register pinctrl device");
+
+		clk_unprepare(ddata->clk);
+err_clk_prepare:
+
+		clk_put(ddata->clk);
+err_clk_get:
+
+		iounmap(ddata->base);
+err_ioremap:
+err_get_base:
+err_platdata:
+		kfree(ddata);
+	} else
+		efm32_pinctrl_dbg(ddata, "initialized (%p, %p)\n", ddata, ddata->pinctrldev);
+
+err_ddata_kzalloc:
+	return ret;
+}
+
+static struct platform_driver efm32_pinctrl_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+	.probe = efm32_pinctrl_probe,
+};
+
+static int __init efm32_pinctrl_init(void)
+{
+	return platform_driver_register(&efm32_pinctrl_driver);
+}
+arch_initcall(efm32_pinctrl_init);
+
+MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>");
+MODULE_DESCRIPTION("efm32 pin control");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/efm32-pinctl.h b/include/linux/platform_data/efm32-pinctl.h
new file mode 100644
index 0000000..38d0f9e6
--- /dev/null
+++ b/include/linux/platform_data/efm32-pinctl.h
@@ -0,0 +1,60 @@ 
+#ifndef __LINUX_PLATFORM_DATA_EFM32_PINCTL_H__
+#define __LINUX_PLATFORM_DATA_EFM32_PINCTL_H__
+/*
+ * The lower 4 bits of these values go into the MODE register,
+ * the 5th into DOUT if the 6th bit is set
+ */
+#define EFM32_MODE_DISABLE              0x20
+#define EFM32_MODE_INPUT                0x21
+#define EFM32_MODE_PUSHPULL             0x04
+#define EFM32_MODE_PUSHPULL_LOW         0x24
+#define EFM32_MODE_PUSHPULL_HIGH        0x34
+
+/**
+ * struct efm32_pinctl_group
+ * @name: name of the group
+ * @pins: list of pins that form the group
+ * @npins: length of @pins
+ */
+struct efm32_pinctl_group {
+	const char *name;
+	const unsigned *pins;
+	unsigned npins;
+};
+
+/**
+ * struct efm32_pinctl_muxfunc
+ * @name: name of the function
+ * @groups: list of groups this function can be muxed to
+ * @ngroups: length of @groups
+ * @modes: modes to set for the pins when the function is enabled
+ *
+ * All groups must have the same length and order and their length must match
+ * the length of @modes. When the function is enabled for group g, g->pins[i]
+ * gets assigned mode modes[i].
+ */
+struct efm32_pinctl_muxfunc {
+	const char *name;
+	const char *const *groups;
+	unsigned ngroups;
+	const unsigned *modes;
+};
+
+/**
+ * @pins: list of pins available
+ * @npins: length of @pins
+ * @groups: list of groups available
+ * @ngroups: length of @groups
+ * @funcs: list of functions available
+ * @nfuncs: length oflength of @funcs
+ */
+struct efm32_pinctl_pdata {
+	const struct pinctrl_pin_desc *pins;
+	unsigned npins;
+	const struct efm32_pinctl_group *const *groups;
+	unsigned ngroups;
+	const struct efm32_pinctl_muxfunc *funcs;
+	unsigned nfuncs;
+};
+
+#endif /* ifndef __LINUX_PLATFORM_DATA_EFM32_PINCTL_H__ */