[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Matt Porter]

This fixes smsc911x support on platforms using gpmc_smsc911x_init().
Commit c7e963f6888816f04d1f5da0e07bec4e0092f227 added the requirement
that platforms provide vdd33a and vddvario supplies.

Signed-off-by: Matt Porter <mporter@ti.com>
---
 arch/arm/mach-omap2/gpmc-smsc911x.c |   44 +++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
index 9970331..da8399e 100644
--- a/arch/arm/mach-omap2/gpmc-smsc911x.c
+++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
@@ -19,6 +19,8 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/smsc911x.h>
+#include <linux/regulator/fixed.h>
+#include <linux/regulator/machine.h>
 
 #include <plat/board.h>
 #include <plat/gpmc.h>
@@ -42,6 +44,43 @@ static struct smsc911x_platform_config gpmc_smsc911x_config = {
 	.flags		= SMSC911X_USE_16BIT,
 };
 
+static struct regulator_consumer_supply gpmc_smsc911x_supply[] = {
+	REGULATOR_SUPPLY("vddvario", "smsc911x.0"),
+	REGULATOR_SUPPLY("vdd33a", "smsc911x.0"),
+};
+
+/* Generic regulator definition to satisfy smsc911x */
+static struct regulator_init_data gpmc_smsc911x_reg_init_data = {
+	.constraints = {
+		.min_uV			= 3300000,
+		.max_uV			= 3300000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL
+					| REGULATOR_MODE_STANDBY,
+		.valid_ops_mask		= REGULATOR_CHANGE_MODE
+					| REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies	= ARRAY_SIZE(gpmc_smsc911x_supply),
+	.consumer_supplies	= gpmc_smsc911x_supply,
+};
+
+static struct fixed_voltage_config gpmc_smsc911x_fixed_reg_data = {
+	.supply_name		= "gpmc_smsc911x",
+	.microvolts		= 3300000,
+	.gpio			= -EINVAL,
+	.startup_delay		= 0,
+	.enable_high		= 0,
+	.enabled_at_boot	= 1,
+	.init_data		= &gpmc_smsc911x_reg_init_data,
+};
+
+static struct platform_device gpmc_smsc911x_regulator = {
+	.name		= "reg-fixed-voltage",
+	.id		= 0,
+	.dev = {
+		.platform_data	= &gpmc_smsc911x_fixed_reg_data,
+	},
+};
+
 /*
  * Initialize smsc911x device connected to the GPMC. Note that we
  * assume that pin multiplexing is done in the board-*.c file,
@@ -55,6 +94,11 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
 
 	gpmc_cfg = board_data;
 
+	if (platform_device_register(&gpmc_smsc911x_regulator) < 0) {
+		pr_err("Unable to register smsc911x regulators\n");
+		return;
+	}
+
 	if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) {
 		pr_err("Failed to request GPMC mem region\n");
 		return;
-- 
1.7.5.4

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Russell King - ARM Linux]

On Mon, Feb 13, 2012 at 11:43:42AM -0500, Matt Porter wrote:
> This fixes smsc911x support on platforms using gpmc_smsc911x_init().
> Commit c7e963f6888816f04d1f5da0e07bec4e0092f227 added the requirement
> that platforms provide vdd33a and vddvario supplies.

Thanks.  Only one comment on this.

> +static struct platform_device gpmc_smsc911x_regulator = {
> +	.name		= "reg-fixed-voltage",
> +	.id		= 0,

$ grep -A1 reg-fixed-voltage arch/arm/*omap* -r
arch/arm/mach-omap2/board-omap3evm.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-omap3evm.c-	.id		= 1,
--
arch/arm/mach-omap2/board-zoom-peripherals.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-zoom-peripherals.c-	.id		= 1,
--
arch/arm/mach-omap2/board-overo.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-overo.c-	.id		= 1,
--
arch/arm/mach-omap2/board-omap4panda.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-omap4panda.c-	.id		= 1,
--
arch/arm/mach-omap2/board-rm680.c:	.name			= "reg-fixed-voltage",
arch/arm/mach-omap2/board-rm680.c-	.dev			= {
--
arch/arm/mach-omap2/board-igep0020.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-igep0020.c-	.id		= 0,
--
arch/arm/mach-omap2/board-4430sdp.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-4430sdp.c-	.id		= -1,
--
arch/arm/mach-omap2/board-4430sdp.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-4430sdp.c-	.id		= 1,
--
arch/arm/mach-omap2/board-omap3pandora.c:	.name		= "reg-fixed-voltage",
arch/arm/mach-omap2/board-omap3pandora.c-	.id		= 1,

igep0020.c does this:

static struct omap_smsc911x_platform_data smsc911x_cfg = {
        .cs             = IGEP2_SMSC911X_CS,
        .gpio_irq       = IGEP2_SMSC911X_GPIO,
        .gpio_reset     = -EINVAL,
        .flags          = SMSC911X_USE_32BIT | SMSC911X_SAVE_MAC_ADDRESS,
};

static inline void __init igep2_init_smsc911x(void)
{
        gpmc_smsc911x_init(&smsc911x_cfg);
}

so it seems to use the gpmc_smsc stuff as well.  So we're ending up with
two "reg-fixed-voltage" devices both with ID 0.  That's not going to work,
and the driver model/sysfs will give you a nice warning there about that.

I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
similar to what I did for 8250 to control the allocation of platform
device IDs for this, otherwise we're going to keep on running over this
problem.

/*
 * Allocate 8250 platform device IDs.  Nothing is implied by
 * the numbering here, except for the legacy entry being -1.
 */
enum {
	PLAT8250_DEV_LEGACY = -1,
	PLAT8250_DEV_PLATFORM,
	PLAT8250_DEV_PLATFORM1,
	PLAT8250_DEV_PLATFORM2,
	PLAT8250_DEV_FOURPORT,
	PLAT8250_DEV_ACCENT,
	PLAT8250_DEV_BOCA,
	PLAT8250_DEV_EXAR_ST16C554,
	PLAT8250_DEV_HUB6,
	PLAT8250_DEV_MCA,
	PLAT8250_DEV_AU1X00,
	PLAT8250_DEV_SM501,
};

Added Mark for his comment.

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Mark Brown]

On Mon, Feb 13, 2012 at 04:52:09PM +0000, Russell King - ARM Linux wrote:

> I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
> similar to what I did for 8250 to control the allocation of platform
> device IDs for this, otherwise we're going to keep on running over this
> problem.

> Added Mark for his comment.

Hrm, seems slightly icky but the enum will work if we can decide how to
add elements to the enum since it'll be easy to bloat a lot it if it's
central.  It's tempting to suggest just using a random number to assign
the IDs randomly rather than have a central registry but obviously
there's no guarantees there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120213/9f69c9d0/attachment.sig>

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [120213 08:25]:
> On Mon, Feb 13, 2012 at 04:52:09PM +0000, Russell King - ARM Linux wrote:
> 
> > I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
> > similar to what I did for 8250 to control the allocation of platform
> > device IDs for this, otherwise we're going to keep on running over this
> > problem.
> 
> > Added Mark for his comment.
> 
> Hrm, seems slightly icky but the enum will work if we can decide how to
> add elements to the enum since it'll be easy to bloat a lot it if it's
> central.  It's tempting to suggest just using a random number to assign
> the IDs randomly rather than have a central registry but obviously
> there's no guarantees there.

Can't we just leave out the .id and have it automatically assigned?

Regards,

Tony

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Russell King - ARM Linux]

On Mon, Feb 13, 2012 at 09:52:34AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@opensource.wolfsonmicro.com> [120213 08:25]:
> > On Mon, Feb 13, 2012 at 04:52:09PM +0000, Russell King - ARM Linux wrote:
> > 
> > > I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
> > > similar to what I did for 8250 to control the allocation of platform
> > > device IDs for this, otherwise we're going to keep on running over this
> > > problem.
> > 
> > > Added Mark for his comment.
> > 
> > Hrm, seems slightly icky but the enum will work if we can decide how to
> > add elements to the enum since it'll be easy to bloat a lot it if it's
> > central.  It's tempting to suggest just using a random number to assign
> > the IDs randomly rather than have a central registry but obviously
> > there's no guarantees there.
> 
> Can't we just leave out the .id and have it automatically assigned?

Unfortunately, there's no such infrastructure in the driver model.

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [120213 09:31]:
> On Mon, Feb 13, 2012 at 09:52:34AM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie@opensource.wolfsonmicro.com> [120213 08:25]:
> > > On Mon, Feb 13, 2012 at 04:52:09PM +0000, Russell King - ARM Linux wrote:
> > > 
> > > > I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
> > > > similar to what I did for 8250 to control the allocation of platform
> > > > device IDs for this, otherwise we're going to keep on running over this
> > > > problem.
> > > 
> > > > Added Mark for his comment.
> > > 
> > > Hrm, seems slightly icky but the enum will work if we can decide how to
> > > add elements to the enum since it'll be easy to bloat a lot it if it's
> > > central.  It's tempting to suggest just using a random number to assign
> > > the IDs randomly rather than have a central registry but obviously
> > > there's no guarantees there.
> > 
> > Can't we just leave out the .id and have it automatically assigned?
> 
> Unfortunately, there's no such infrastructure in the driver model.

Hmm OK, -1 seems to be just used for name in platform_device_add().

Tony

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Matt Porter]

On Mon, Feb 13, 2012 at 08:56:18AM -0800, Mark Brown wrote:
> On Mon, Feb 13, 2012 at 04:52:09PM +0000, Russell King - ARM Linux wrote:
> 
> > I think OMAP (or even the reg-fixed-voltage folk) needs to create an enum
> > similar to what I did for 8250 to control the allocation of platform
> > device IDs for this, otherwise we're going to keep on running over this
> > problem.
> 
> > Added Mark for his comment.
> 
> Hrm, seems slightly icky but the enum will work if we can decide how to
> add elements to the enum since it'll be easy to bloat a lot it if it's
> central.  It's tempting to suggest just using a random number to assign
> the IDs randomly rather than have a central registry but obviously
> there's no guarantees there.

Another option might be to provide a generic platform device helper
that instantiates a per-named-platform-device id list that simply
provides the next available id sequentially as requests for platform
device registration arrive.

arch init:

platform_create_id_list_create("reg-fixed-voltage"); 

platform init:

/* I = 0..N, .id filled in by _autoid() registration */
static struct platform_device my_fixed_regulatorN = {
	.name		= "reg-fixed-voltage",
	.dev = ...
}

platform_device_register_autoid(&my_fixed_regulator0); 
platform_device_register_autoid(&my_fixed_regulator1); 
platform_device_register_autoid(&my_fixed_regulator2); 
...
platform_device_register_autoid(&my_fixed_regulatorN); 

Something like this might be able to solve this once for the platform
device users don't need a fixed enum id. It would require changes to
platform_device_register() to update the list as static assignment
of ids occurs so those will not be used by the _autoid() method.

-Matt

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Mark Brown]

On Mon, Feb 13, 2012 at 10:14:10AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [120213 09:31]:
> > On Mon, Feb 13, 2012 at 09:52:34AM -0800, Tony Lindgren wrote:

> > > Can't we just leave out the .id and have it automatically assigned?

It'd be nice but...

> > Unfortunately, there's no such infrastructure in the driver model.

> Hmm OK, -1 seems to be just used for name in platform_device_add().

-1 is a valid ID also, it means "there's only one of these things so
don't display a number".  Which is sad but there we are.  I'm at ELC/ABS
so I might try and find Greg in person here to see if we can come up
with something better, it seems like this is something the core ought to
be able to help with in much the same way that the USB stuff can.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120213/6b485802/attachment.sig>

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Russell King - ARM Linux]

Two more points:

On Mon, Feb 13, 2012 at 11:43:42AM -0500, Matt Porter wrote:
> This fixes smsc911x support on platforms using gpmc_smsc911x_init().
> Commit c7e963f6888816f04d1f5da0e07bec4e0092f227 added the requirement

Always include the commit summary as well here, so:

Commit c7e963f6888816 (net/smsc911x: Add regulator support) added the ...

> @@ -55,6 +94,11 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>  
>  	gpmc_cfg = board_data;
>  
> +	if (platform_device_register(&gpmc_smsc911x_regulator) < 0) {
> +		pr_err("Unable to register smsc911x regulators\n");
> +		return;
> +	}

It's always a good idea to indicate why something failed:

	err = platform_device_register(&gpmc_smsc911x_regulator);
	if (err < 0) {
		pr_err("Unable to register smsc911x regulators: %d\n", err);
		return;
	}

so that people can see not only what failed, but also why it failed.

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Tony Lindgren]

* Mark Brown <broonie@opensource.wolfsonmicro.com> [120213 10:27]:
> On Mon, Feb 13, 2012 at 10:14:10AM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [120213 09:31]:
> > > On Mon, Feb 13, 2012 at 09:52:34AM -0800, Tony Lindgren wrote:
> 
> > > > Can't we just leave out the .id and have it automatically assigned?
> 
> It'd be nice but...
> 
> > > Unfortunately, there's no such infrastructure in the driver model.
> 
> > Hmm OK, -1 seems to be just used for name in platform_device_add().
> 
> -1 is a valid ID also, it means "there's only one of these things so
> don't display a number".  Which is sad but there we are.  I'm at ELC/ABS
> so I might try and find Greg in person here to see if we can come up
> with something better, it seems like this is something the core ought to
> be able to help with in much the same way that the USB stuff can.

Matt, care to refresh your original patch using some other number so
we can apply it as a regression fix for the -rc series while the
long term solution is being discussed?

Just use some random number for now with a comment, 42?

Regards,

Tony

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Matt Porter]

On Tue, Feb 21, 2012 at 10:46:48AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@opensource.wolfsonmicro.com> [120213 10:27]:
> > On Mon, Feb 13, 2012 at 10:14:10AM -0800, Tony Lindgren wrote:
> > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [120213 09:31]:
> > > > On Mon, Feb 13, 2012 at 09:52:34AM -0800, Tony Lindgren wrote:
> > 
> > > > > Can't we just leave out the .id and have it automatically assigned?
> > 
> > It'd be nice but...
> > 
> > > > Unfortunately, there's no such infrastructure in the driver model.
> > 
> > > Hmm OK, -1 seems to be just used for name in platform_device_add().
> > 
> > -1 is a valid ID also, it means "there's only one of these things so
> > don't display a number".  Which is sad but there we are.  I'm at ELC/ABS
> > so I might try and find Greg in person here to see if we can come up
> > with something better, it seems like this is something the core ought to
> > be able to help with in much the same way that the USB stuff can.
> 
> Matt, care to refresh your original patch using some other number so
> we can apply it as a regression fix for the -rc series while the
> long term solution is being discussed?
> 
> Just use some random number for now with a comment, 42?

Will do, I'll post an update with this approach.

-Matt

[PATCH] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators

[Matt Porter]

On Tue, Feb 14, 2012 at 11:08:36AM +0000, Russell King - ARM Linux wrote:
> Two more points:
> 
> On Mon, Feb 13, 2012 at 11:43:42AM -0500, Matt Porter wrote:
> > This fixes smsc911x support on platforms using gpmc_smsc911x_init().
> > Commit c7e963f6888816f04d1f5da0e07bec4e0092f227 added the requirement
> 
> Always include the commit summary as well here, so:
> 
> Commit c7e963f6888816 (net/smsc911x: Add regulator support) added the ...

Ok. Addressed in v2.

> > @@ -55,6 +94,11 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
> >  
> >  	gpmc_cfg = board_data;
> >  
> > +	if (platform_device_register(&gpmc_smsc911x_regulator) < 0) {
> > +		pr_err("Unable to register smsc911x regulators\n");
> > +		return;
> > +	}
> 
> It's always a good idea to indicate why something failed:
> 
> 	err = platform_device_register(&gpmc_smsc911x_regulator);
> 	if (err < 0) {
> 		pr_err("Unable to register smsc911x regulators: %d\n", err);
> 		return;
> 	}
> 
> so that people can see not only what failed, but also why it failed.

Looks much better, also addressed in v2.

-Matt