[PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Marcin Wojtas]

This commit adds suspend/resume support by saving and restoring registers'
state in a dedicated array.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-38x.c | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
index 6ec82c6..094cb48 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
@@ -23,6 +23,7 @@
 #include "pinctrl-mvebu.h"
 
 static void __iomem *mpp_base;
+static u32 *mpp_saved_regs;
 
 static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config)
 {
@@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
 	const struct of_device_id *match =
 		of_match_device(armada_38x_pinctrl_of_match, &pdev->dev);
 	struct resource *res;
+	int nregs;
 
 	if (!match)
 		return -ENODEV;
@@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
 	soc->modes = armada_38x_mpp_modes;
 	soc->nmodes = armada_38x_mpp_controls[0].npins;
 
+	nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
+
+	mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32),
+				      GFP_KERNEL);
+	if (!mpp_saved_regs)
+		return -ENOMEM;
+
 	pdev->dev.platform_data = soc;
 
 	return mvebu_pinctrl_probe(pdev);
 }
 
+int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);
+	int i, nregs;
+
+	nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
+
+	for (i = 0; i < nregs; i++)
+		mpp_saved_regs[i] = readl(mpp_base + i * 4);
+
+	return 0;
+}
+
+int armada_38x_pinctrl_resume(struct platform_device *pdev)
+{
+	struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);
+	int i, nregs;
+
+	nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
+
+	for (i = 0; i < nregs; i++)
+		writel(mpp_saved_regs[i], mpp_base + i * 4);
+
+	return 0;
+}
+
 static int armada_38x_pinctrl_remove(struct platform_device *pdev)
 {
 	return mvebu_pinctrl_remove(pdev);
@@ -458,6 +493,8 @@ static struct platform_driver armada_38x_pinctrl_driver = {
 	},
 	.probe = armada_38x_pinctrl_probe,
 	.remove = armada_38x_pinctrl_remove,
+	.suspend = armada_38x_pinctrl_suspend,
+	.resume = armada_38x_pinctrl_resume,
 };
 
 module_platform_driver(armada_38x_pinctrl_driver);
-- 
1.8.3.1

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Russell King - ARM Linux]

On Sat, Oct 17, 2015 at 11:28:48PM +0200, Marcin Wojtas wrote:
> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
> index 6ec82c6..094cb48 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
> @@ -23,6 +23,7 @@
>  #include "pinctrl-mvebu.h"
>  
>  static void __iomem *mpp_base;
> +static u32 *mpp_saved_regs;

I'm not a fan of unnecessary global variables.  It adds to the bulk of
the kernel when built-in (even though it's in .bss, it still has a cost)
and these all add up when built-in.

Please make it part of the driver data allocated at probe time.

>  static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config)
>  {
> @@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
>  	const struct of_device_id *match =
>  		of_match_device(armada_38x_pinctrl_of_match, &pdev->dev);
>  	struct resource *res;
> +	int nregs;
>  
>  	if (!match)
>  		return -ENODEV;
> @@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
>  	soc->modes = armada_38x_mpp_modes;
>  	soc->nmodes = armada_38x_mpp_controls[0].npins;
>  
> +	nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
> +
> +	mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32),
> +				      GFP_KERNEL);
> +	if (!mpp_saved_regs)
> +		return -ENOMEM;
> +
>  	pdev->dev.platform_data = soc;

The 'soc' is stored in platform data, not driver data, but...

>  
>  	return mvebu_pinctrl_probe(pdev);
>  }
>  
> +int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);

You access it through driver data.  Isn't the driver data here a
struct mvebu_pinctrl pointer?  See platform_set_drvdata() in
mvebu_pinctrl_probe().

> +	int i, nregs;
> +
> +	nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
> +
> +	for (i = 0; i < nregs; i++)
> +		mpp_saved_regs[i] = readl(mpp_base + i * 4);
> +
> +	return 0;
> +}
> +
> +int armada_38x_pinctrl_resume(struct platform_device *pdev)
> +{
> +	struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);

Ditto.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Marcin Wojtas]

Hi Russell,

Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a
fix then, too) and it worked. I must have missed, because I got proper
registers' number and values in suspend/resume routines. As
pinctrl-armada-xp.c needs also a small fix and in order not to
duplicate code, how about a following solution:
- *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info
- common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c
(now there will be two users AXP and A38X)

Please let me know what you think.

Best regards,
Marcin



2015-10-18 0:29 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Oct 17, 2015 at 11:28:48PM +0200, Marcin Wojtas wrote:
>> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
>> index 6ec82c6..094cb48 100644
>> --- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
>> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
>> @@ -23,6 +23,7 @@
>>  #include "pinctrl-mvebu.h"
>>
>>  static void __iomem *mpp_base;
>> +static u32 *mpp_saved_regs;
>
> I'm not a fan of unnecessary global variables.  It adds to the bulk of
> the kernel when built-in (even though it's in .bss, it still has a cost)
> and these all add up when built-in.
>
> Please make it part of the driver data allocated at probe time.
>
>>  static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config)
>>  {
>> @@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
>>       const struct of_device_id *match =
>>               of_match_device(armada_38x_pinctrl_of_match, &pdev->dev);
>>       struct resource *res;
>> +     int nregs;
>>
>>       if (!match)
>>               return -ENODEV;
>> @@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
>>       soc->modes = armada_38x_mpp_modes;
>>       soc->nmodes = armada_38x_mpp_controls[0].npins;
>>
>> +     nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
>> +
>> +     mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32),
>> +                                   GFP_KERNEL);
>> +     if (!mpp_saved_regs)
>> +             return -ENOMEM;
>> +
>>       pdev->dev.platform_data = soc;
>
> The 'soc' is stored in platform data, not driver data, but...
>
>>
>>       return mvebu_pinctrl_probe(pdev);
>>  }
>>
>> +int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +     struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);
>
> You access it through driver data.  Isn't the driver data here a
> struct mvebu_pinctrl pointer?  See platform_set_drvdata() in
> mvebu_pinctrl_probe().
>
>> +     int i, nregs;
>> +
>> +     nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG);
>> +
>> +     for (i = 0; i < nregs; i++)
>> +             mpp_saved_regs[i] = readl(mpp_base + i * 4);
>> +
>> +     return 0;
>> +}
>> +
>> +int armada_38x_pinctrl_resume(struct platform_device *pdev)
>> +{
>> +     struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev);
>
> Ditto.
>
> --
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Thomas Petazzoni]

Hello Marcin,

On Sun, 18 Oct 2015 10:43:42 +0200, Marcin Wojtas wrote:

> Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a
> fix then, too) and it worked. I must have missed, because I got proper
> registers' number and values in suspend/resume routines. As
> pinctrl-armada-xp.c needs also a small fix and in order not to
> duplicate code, how about a following solution:
> - *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info

I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
a read-only structure that only describes static information giving
SoC-specific details for pin-muxing. The idea is that in the event
where you had multiple pinctrl in the same system, you would still have
only one instance of mvebu_pinctrl_soc_info.

So, I'd prefer if mpp_saved_regs and mpp_base became members of a new
structure, that gets allocated at ->probe() time, on a per-instance
basis.

> - common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c
> (now there will be two users AXP and A38X)

Not sure how to handle that if the per-instance structure is defined on
a per-SoC basis, but I'm interested in seeing proposals.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Marcin Wojtas]

Hi Thomas,

2015-10-18 16:01 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello Marcin,
>
> On Sun, 18 Oct 2015 10:43:42 +0200, Marcin Wojtas wrote:
>
>> Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a
>> fix then, too) and it worked. I must have missed, because I got proper
>> registers' number and values in suspend/resume routines. As
>> pinctrl-armada-xp.c needs also a small fix and in order not to
>> duplicate code, how about a following solution:
>> - *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info
>
> I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
> a read-only structure that only describes static information giving
> SoC-specific details for pin-muxing. The idea is that in the event
> where you had multiple pinctrl in the same system, you would still have
> only one instance of mvebu_pinctrl_soc_info.

Ok, understood. What with current static globals, like mpp_base? This
is a problem when we consider hypothetical multi-pintrl system...

>
> So, I'd prefer if mpp_saved_regs and mpp_base became members of a new
> structure, that gets allocated at ->probe() time, on a per-instance
> basis.
>
>> - common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c
>> (now there will be two users AXP and A38X)
>
> Not sure how to handle that if the per-instance structure is defined on
> a per-SoC basis, but I'm interested in seeing proposals.
>

In genereal, I think storing additional global data is not
starightforward, as dev->platform_data and dev->driver_data are
currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I
propose the following:

1. Create a new structure:
struct mvebu_pinctrl_pm_info {
    void __iomem *base;
    static u32 *mpp_saved_regs;
    int nregs;
}

2. Add new field to struct mvebu_pinctrl:
struct mvebu_pinctrl_pm_info *pm_info;

3. In armada_38x/xp_pinctrl_probe we do the allocations and pass
struct pm_info using dev->driver data (later in mvebu_pinctrl_probe it
will be replaced by struct mvebu_pinctrl):
platform_set_drvdata(pdev, pm_info);

return mvebu_pinctrl_probe(pdev);

4. In mvebu_pinctrl_probe:
struct mvebu_pinctrl_pm_info *pm_info = platform_get_drvdata(pdev);
(...)

if (pm_info)
    pctl->pm_info = pm_info;
platform_set_drvdata(pdev, pctl);

5. Now, we can simply use all stored data in common
mvebu_pinctrl_suspend/resume.

I hope the above is clear. I'm looking forward to your opinion.

Beste regards,
Marcin

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Thomas Petazzoni]

Hello,

On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote:

> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
> > a read-only structure that only describes static information giving
> > SoC-specific details for pin-muxing. The idea is that in the event
> > where you had multiple pinctrl in the same system, you would still have
> > only one instance of mvebu_pinctrl_soc_info.
> 
> Ok, understood. What with current static globals, like mpp_base? This
> is a problem when we consider hypothetical multi-pintrl system...

The current driver is indeed not designed for multiple instances of the
same pinctrl controller. But that's exactly what Russell is asking for.

> In genereal, I think storing additional global data is not
> starightforward, as dev->platform_data and dev->driver_data are
> currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I
> propose the following:
>
> 
> 1. Create a new structure:
> struct mvebu_pinctrl_pm_info {

This definitely shouldn't be called "pm_info", because 'base' is not PM
related. It should be mvebu_pinctrl_state or something like that.

>     void __iomem *base;
>     static u32 *mpp_saved_regs;
>     int nregs;
> }
> 
> 2. Add new field to struct mvebu_pinctrl:
> struct mvebu_pinctrl_pm_info *pm_info;

Does not work because "mvebu_pinctrl_pm_info" cannot be a generic
structure, it has to be a per-SoC driver structure, since the set of
registers to save for PM reasons is different from one SoC to the
other. Also, some SoC have only one "base" pointer, some others (like
Dove) have multiple.

So it should be the other way around: the SoC-specific driver create a
structure, and this structure points back to the mvebu_pinctrl
structure.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Marcin Wojtas]

Thomas,

2015-10-19 9:23 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello,
>
> On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote:
>
>> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
>> > a read-only structure that only describes static information giving
>> > SoC-specific details for pin-muxing. The idea is that in the event
>> > where you had multiple pinctrl in the same system, you would still have
>> > only one instance of mvebu_pinctrl_soc_info.
>>
>> Ok, understood. What with current static globals, like mpp_base? This
>> is a problem when we consider hypothetical multi-pintrl system...
>
> The current driver is indeed not designed for multiple instances of the
> same pinctrl controller. But that's exactly what Russell is asking for.
>

In each mvebu pinctl there are <soc>_mpp_ctrl_get/set force usage of
global mpp_base. In order not to use it this way the functions have to
be redesigned. IMO having an acces to pdev would be sufficient (please
see my proposal below), but yet I don't have a clear idea, how to do
it,

>> In genereal, I think storing additional global data is not
>> starightforward, as dev->platform_data and dev->driver_data are
>> currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I
>> propose the following:
>>
>>
>> 1. Create a new structure:
>> struct mvebu_pinctrl_pm_info {
>
> This definitely shouldn't be called "pm_info", because 'base' is not PM
> related. It should be mvebu_pinctrl_state or something like that.
>
>>     void __iomem *base;
>>     static u32 *mpp_saved_regs;
>>     int nregs;
>> }
>>
>> 2. Add new field to struct mvebu_pinctrl:
>> struct mvebu_pinctrl_pm_info *pm_info;
>
> Does not work because "mvebu_pinctrl_pm_info" cannot be a generic
> structure, it has to be a per-SoC driver structure, since the set of
> registers to save for PM reasons is different from one SoC to the
> other. Also, some SoC have only one "base" pointer, some others (like
> Dove) have multiple.
>
> So it should be the other way around: the SoC-specific driver create a
> structure, and this structure points back to the mvebu_pinctrl
> structure.
>

How about a following compromise:
Instead of forcing pm_info, we can add a generic pointer to struct
mvebu_pinctrl, e.g. void *priv; and enable passing it in a way I
proposed before.

This way each SoC-specific driver can attach whatever is needed for
its operation. For example in A38X/AXP it would be *base,
*mpp_saved_regs and nregs gathered in one structure, but it wouldn't
force anything specific for other machines. As a result in A38X/AXP in
suspend/resume routines, there would be no globals in use at all.

Best regards,
Marcin

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Marcin Wojtas]

Hi Thomas,

Found it - I think there is an easy way to get rid of all global
variables in each pinctrl-<soc>. It's enough to:
- extend struct mvebu_pinctrl with generic pointer
- pass SoC specific structure to mvebu_pinctrl_probe via dev->driver_data
- in mvebu_pinconf_group_set/get pass an additional argument to
mpp_set/get: struct mvebu_pinctrl *pctl.
- update mpp_set/get callbacks in each driver accordingly
This way in SoC-specific pinctrl driver we determine what data we want
to use and how. I can prepare a patchset with all described changes.
I'm looking forward to your feedback.

Best regards,
Marcin

2015-10-19 11:25 GMT+02:00 Marcin Wojtas <mw@semihalf.com>:
> Thomas,
>
> 2015-10-19 9:23 GMT+02:00 Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com>:
>> Hello,
>>
>> On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote:
>>
>>> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
>>> > a read-only structure that only describes static information giving
>>> > SoC-specific details for pin-muxing. The idea is that in the event
>>> > where you had multiple pinctrl in the same system, you would still have
>>> > only one instance of mvebu_pinctrl_soc_info.
>>>
>>> Ok, understood. What with current static globals, like mpp_base? This
>>> is a problem when we consider hypothetical multi-pintrl system...
>>
>> The current driver is indeed not designed for multiple instances of the
>> same pinctrl controller. But that's exactly what Russell is asking for.
>>
>
> In each mvebu pinctl there are <soc>_mpp_ctrl_get/set force usage of
> global mpp_base. In order not to use it this way the functions have to
> be redesigned. IMO having an acces to pdev would be sufficient (please
> see my proposal below), but yet I don't have a clear idea, how to do
> it,
>
>>> In genereal, I think storing additional global data is not
>>> starightforward, as dev->platform_data and dev->driver_data are
>>> currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I
>>> propose the following:
>>>
>>>
>>> 1. Create a new structure:
>>> struct mvebu_pinctrl_pm_info {
>>
>> This definitely shouldn't be called "pm_info", because 'base' is not PM
>> related. It should be mvebu_pinctrl_state or something like that.
>>
>>>     void __iomem *base;
>>>     static u32 *mpp_saved_regs;
>>>     int nregs;
>>> }
>>>
>>> 2. Add new field to struct mvebu_pinctrl:
>>> struct mvebu_pinctrl_pm_info *pm_info;
>>
>> Does not work because "mvebu_pinctrl_pm_info" cannot be a generic
>> structure, it has to be a per-SoC driver structure, since the set of
>> registers to save for PM reasons is different from one SoC to the
>> other. Also, some SoC have only one "base" pointer, some others (like
>> Dove) have multiple.
>>
>> So it should be the other way around: the SoC-specific driver create a
>> structure, and this structure points back to the mvebu_pinctrl
>> structure.
>>
>
> How about a following compromise:
> Instead of forcing pm_info, we can add a generic pointer to struct
> mvebu_pinctrl, e.g. void *priv; and enable passing it in a way I
> proposed before.
>
> This way each SoC-specific driver can attach whatever is needed for
> its operation. For example in A38X/AXP it would be *base,
> *mpp_saved_regs and nregs gathered in one structure, but it wouldn't
> force anything specific for other machines. As a result in A38X/AXP in
> suspend/resume routines, there would be no globals in use at all.
>
> Best regards,
> Marcin

Re: [PATCH] pinctrl: mvebu: armada-38x: add suspend/resume support

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 11:25:49AM +0200, Marcin Wojtas wrote:
> Thomas,
> 
> 2015-10-19 9:23 GMT+02:00 Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com>:
> > Hello,
> >
> > On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote:
> >
> >> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be
> >> > a read-only structure that only describes static information giving
> >> > SoC-specific details for pin-muxing. The idea is that in the event
> >> > where you had multiple pinctrl in the same system, you would still have
> >> > only one instance of mvebu_pinctrl_soc_info.
> >>
> >> Ok, understood. What with current static globals, like mpp_base? This
> >> is a problem when we consider hypothetical multi-pintrl system...
> >
> > The current driver is indeed not designed for multiple instances of the
> > same pinctrl controller. But that's exactly what Russell is asking for.
> >
> 
> In each mvebu pinctl there are <soc>_mpp_ctrl_get/set force usage of
> global mpp_base. In order not to use it this way the functions have to
> be redesigned. IMO having an acces to pdev would be sufficient (please
> see my proposal below), but yet I don't have a clear idea, how to do
> it,

I'm not sure having access to pdev would be sufficient -
struct mvebu_pinctrl is entirely private to pinctrl-mvebu.c, so you
wouldn't be able to dereference the driver data.

> > So it should be the other way around: the SoC-specific driver create a
> > structure, and this structure points back to the mvebu_pinctrl
> > structure.
> >
> 
> How about a following compromise:
> Instead of forcing pm_info, we can add a generic pointer to struct
> mvebu_pinctrl, e.g. void *priv; and enable passing it in a way I
> proposed before.
> 
> This way each SoC-specific driver can attach whatever is needed for
> its operation. For example in A38X/AXP it would be *base,
> *mpp_saved_regs and nregs gathered in one structure, but it wouldn't
> force anything specific for other machines. As a result in A38X/AXP in
> suspend/resume routines, there would be no globals in use at all.

The one thing I don't like about passing a 'void *' around is that it
will try to be re-used with __iomem pointers, and that'll cause sparse
to complain (quite rightfully - since sparse is designed to detect
direct dereferences of iomem pointers, and to do that it doesn't let
you trivially cast to/from an __iomem pointer to a normal pointer.
(not without using __force, which shouldn't be used in drivers.)

I wonder if there's a fairly nice solution to this:

Move away from mvebu_pinctrl_probe(), to
	void *mvebu_pinctrl_alloc(struct device *, size_t private_size);
	int mvebu_pinctrl_add(void *);

where mvebu_pinctrl_alloc() allocates and initialises the struct
mvebu_pinctrl, plus padding plus the private size.  The padding is
necessary to ensure that anything placed in the private area is
properly aligned (eg, 8-byte aligned.)  (This idea comes from netdev's
private data handling.)

These set the driver data to the private area, and return a pointer
to that private area.

Arrange for the get/set/whatever else functions to be passed this
same pointer.

What this means is that the individual drivers:

* get their private data wrapped up into the driver private data
* still don't need to know anything about mvebu_pinctrl
* can get at their private data using platform_get_drvdata()
* don't need any globals for anything

Something like this (by way of illustration):

 drivers/pinctrl/mvebu/pinctrl-armada-38x.c | 27 +++++++--
 drivers/pinctrl/mvebu/pinctrl-mvebu.c      | 97 ++++++++++++++++++++----------
 drivers/pinctrl/mvebu/pinctrl-mvebu.h      |  3 +
 3 files changed, 90 insertions(+), 37 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
index 6ec82c62dff7..1dc76f6f6486 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c
@@ -24,6 +24,10 @@
 
 static void __iomem *mpp_base;
 
+struct armada_38x_pinctrl_priv {
+	void __iomem *mpp_base;
+};
+
 static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config)
 {
 	return default_mpp_ctrl_get(mpp_base, pid, config);
@@ -421,6 +425,7 @@ static struct pinctrl_gpio_range armada_38x_mpp_gpio_ranges[] = {
 static int armada_38x_pinctrl_probe(struct platform_device *pdev)
 {
 	struct mvebu_pinctrl_soc_info *soc = &armada_38x_pinctrl_info;
+	struct armada_38x_pinctrl_priv *priv;
 	const struct of_device_id *match =
 		of_match_device(armada_38x_pinctrl_of_match, &pdev->dev);
 	struct resource *res;
@@ -428,10 +433,14 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
 	if (!match)
 		return -ENODEV;
 
+	priv = mvebu_pinctrl_alloc(pdev, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mpp_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mpp_base))
-		return PTR_ERR(mpp_base);
+	priv->mpp_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->mpp_base))
+		return PTR_ERR(priv->mpp_base);
 
 	soc->variant = (unsigned) match->data & 0xff;
 	soc->controls = armada_38x_mpp_controls;
@@ -443,7 +452,10 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev)
 
 	pdev->dev.platform_data = soc;
 
-	return mvebu_pinctrl_probe(pdev);
+	/* until the set/get functions are converted */
+	mpp_base = priv->mpp_base;
+
+	return mvebu_pinctrl_add(priv);
 }
 
 static int armada_38x_pinctrl_remove(struct platform_device *pdev)
@@ -451,6 +463,13 @@ static int armada_38x_pinctrl_remove(struct platform_device *pdev)
 	return mvebu_pinctrl_remove(pdev);
 }
 
+static int armada_38x_pinctrl_suspend(struct platform_device *pdev)
+{
+	struct armada_38x_pinctrl_priv *priv = platform_get_drvdata(pdev);
+	...
+	return 0;
+}
+
 static struct platform_driver armada_38x_pinctrl_driver = {
 	.driver = {
 		.name = "armada-38x-pinctrl",
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index 77d2221d379d..e2f50fce2466 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -55,6 +55,9 @@ struct mvebu_pinctrl {
 	struct mvebu_pinctrl_function *functions;
 	unsigned num_functions;
 	u8 variant;
+
+	/* driver private data follows, must be last */
+	u64 private[0];
 };
 
 static struct mvebu_pinctrl_group *mvebu_pinctrl_find_group_by_pid(
@@ -470,8 +473,7 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, int *funcsize,
 	return 0;
 }
 
-static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
-					 struct mvebu_pinctrl *pctl)
+static int mvebu_pinctrl_build_functions(struct mvebu_pinctrl *pctl)
 {
 	struct mvebu_pinctrl_function *funcs;
 	int num = 0, funcsize = pctl->desc.npins;
@@ -479,7 +481,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 
 	/* we allocate functions for number of pins and hope
 	 * there are fewer unique functions than pins available */
-	funcs = devm_kzalloc(&pdev->dev, funcsize *
+	funcs = devm_kzalloc(pctl->dev, funcsize *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
 		return -ENOMEM;
@@ -498,7 +500,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 			ret = _add_function(funcs, &funcsize,
 					    grp->settings[s].name);
 			if (ret == -EOVERFLOW)
-				dev_err(&pdev->dev,
+				dev_err(pctl->dev,
 					"More functions than pins(%d)\n",
 					pctl->desc.npins);
 			if (ret < 0)
@@ -527,7 +529,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 
 			/* allocate group name array if not done already */
 			if (!f->groups) {
-				f->groups = devm_kzalloc(&pdev->dev,
+				f->groups = devm_kzalloc(pctl->dev,
 						 f->num_groups * sizeof(char *),
 						 GFP_KERNEL);
 				if (!f->groups)
@@ -545,10 +547,39 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	return 0;
 }
 
-int mvebu_pinctrl_probe(struct platform_device *pdev)
+static struct mvebu_pinctrl *private_to_pctl(void *private)
+{
+	return container_of(private, struct mvebu_pinctrl, private);
+}
+
+void *mvebu_pinctrl_alloc(struct platform_device *pdev, size_t private_size)
 {
-	struct mvebu_pinctrl_soc_info *soc = dev_get_platdata(&pdev->dev);
+	struct device *dev = &pdev->dev;
 	struct mvebu_pinctrl *pctl;
+
+	pctl = devm_kzalloc(dev, offsetof(struct mvebu_pinctrl, private) +
+			    private_size, GFP_KERNEL);
+	if (!pctl) {
+		dev_err(dev, "unable to alloc driver\n");
+		return NULL;
+	}
+
+	pctl->desc.name = dev_name(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->dev = dev;
+
+	platform_set_drvdata(pdev, pctl->private);
+
+	return pctl->private;
+}
+
+int mvebu_pinctrl_add(void *private)
+{
+	struct mvebu_pinctrl *pctl = private_to_pctl(private);
+	struct mvebu_pinctrl_soc_info *soc = dev_get_platdata(pctl->dev);
 	struct pinctrl_pin_desc *pdesc;
 	unsigned gid, n, k;
 	unsigned size, noname = 0;
@@ -557,25 +588,11 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 	int ret;
 
 	if (!soc || !soc->controls || !soc->modes) {
-		dev_err(&pdev->dev, "wrong pinctrl soc info\n");
+		dev_err(pctl->dev, "wrong pinctrl soc info\n");
 		return -EINVAL;
 	}
 
-	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->dev = &pdev->dev;
-	platform_set_drvdata(pdev, pctl);
 
 	/* count controls and create names for mvebu generic
 	   register controls; also does sanity checks */
@@ -602,10 +619,10 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 		}
 	}
 
-	pdesc = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+	pdesc = devm_kzalloc(pctl->dev, pctl->desc.npins *
 			     sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
 	if (!pdesc) {
-		dev_err(&pdev->dev, "failed to alloc pinctrl pins\n");
+		dev_err(pctl->dev, "failed to alloc pinctrl pins\n");
 		return -ENOMEM;
 	}
 
@@ -617,9 +634,9 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 	 * allocate groups and name buffers for unnamed groups.
 	 */
 	size = pctl->num_groups * sizeof(*pctl->groups) + noname * 8;
-	p = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	p = devm_kzalloc(pctl->dev, size, GFP_KERNEL);
 	if (!p) {
-		dev_err(&pdev->dev, "failed to alloc group data\n");
+		dev_err(pctl->dev, "failed to alloc group data\n");
 		return -ENOMEM;
 	}
 	pctl->groups = p;
@@ -668,7 +685,7 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 		unsigned num_settings;
 
 		if (!grp) {
-			dev_warn(&pdev->dev, "unknown pinctrl group %d\n",
+			dev_warn(pctl->dev, "unknown pinctrl group %d\n",
 				mode->pid);
 			continue;
 		}
@@ -699,19 +716,19 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 		grp->num_settings = num_settings;
 	}
 
-	ret = mvebu_pinctrl_build_functions(pdev, pctl);
+	ret = mvebu_pinctrl_build_functions(pctl);
 	if (ret) {
-		dev_err(&pdev->dev, "unable to build functions\n");
+		dev_err(pctl->dev, "unable to build functions\n");
 		return ret;
 	}
 
-	pctl->pctldev = pinctrl_register(&pctl->desc, &pdev->dev, pctl);
+	pctl->pctldev = pinctrl_register(&pctl->desc, pctl->dev, pctl);
 	if (IS_ERR(pctl->pctldev)) {
-		dev_err(&pdev->dev, "unable to register pinctrl driver\n");
+		dev_err(pctl->dev, "unable to register pinctrl driver\n");
 		return PTR_ERR(pctl->pctldev);
 	}
 
-	dev_info(&pdev->dev, "registered pinctrl driver\n");
+	dev_info(pctl->dev, "registered pinctrl driver\n");
 
 	/* register gpio ranges */
 	for (n = 0; n < soc->ngpioranges; n++)
@@ -720,9 +737,23 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }
 
+int mvebu_pinctrl_probe(struct platform_device *pdev)
+{
+	void *private;
+
+	private = mvebu_pinctrl_alloc(pdev, 0);
+	if (!private) {
+		dev_err(&pdev->dev, "unable to alloc driver\n");
+		return -ENOMEM;
+	}
+
+	return mvebu_pinctrl_add(private);
+}
+
 int mvebu_pinctrl_remove(struct platform_device *pdev)
 {
-	struct mvebu_pinctrl *pctl = platform_get_drvdata(pdev);
+	void *private = platform_get_drvdata(pdev);
+	struct mvebu_pinctrl *pctl = private_to_pctl(private);
 	pinctrl_unregister(pctl->pctldev);
 	return 0;
 }
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.h b/drivers/pinctrl/mvebu/pinctrl-mvebu.h
index 65a98e6f7265..aad87f9aad13 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.h
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.h
@@ -204,4 +204,7 @@ static inline int default_mpp_ctrl_set(void __iomem *base, unsigned int pid,
 int mvebu_pinctrl_probe(struct platform_device *pdev);
 int mvebu_pinctrl_remove(struct platform_device *pdev);
 
+void *mvebu_pinctrl_alloc(struct platform_device *pdev, size_t private_size);
+int mvebu_pinctrl_add(void *private);
+
 #endif

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.