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

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

[Matt Porter]

This fixes smsc911x support on platforms using gpmc_smsc911x_init().

Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
the requirement that platforms provide vdd33a and vddvario supplies.

Signed-off-by: Matt Porter <mporter@ti.com>
---
Changes for v2:
   - temporary fix to avoid platform device id conflicts
   - add commit summary from the smsc911x regulator support commit
   - incorporate platform device registration error cause reporting
Changes for v3:
   - remove unneeded local variable err

 arch/arm/mach-omap2/gpmc-smsc911x.c |   52 +++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
index 9970331..bbb870c 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,50 @@ 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,
+};
+
+/*
+ * Platform device id of 42 is a temporary fix to avoid conflicts
+ * with other reg-fixed-voltage devices. The real fix should
+ * involve the driver core providing a way of dynamically
+ * assigning a unique id on registration for platform devices
+ * in the same name space.
+ */
+static struct platform_device gpmc_smsc911x_regulator = {
+	.name		= "reg-fixed-voltage",
+	.id		= 42,
+	.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 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
 
 	gpmc_cfg = board_data;
 
+	ret = platform_device_register(&gpmc_smsc911x_regulator);
+	if (ret < 0) {
+		pr_err("Unable to register smsc911x regulators: %d\n", ret);
+		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

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

[Kevin Hilman]

Matt Porter <mporter@ti.com> writes:

> This fixes smsc911x support on platforms using gpmc_smsc911x_init().
>
> Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
> the requirement that platforms provide vdd33a and vddvario supplies.
>
> Signed-off-by: Matt Porter <mporter@ti.com>

[...]

>  /*
>   * Initialize smsc911x device connected to the GPMC. Note that we
>   * assume that pin multiplexing is done in the board-*.c file,
> @@ -55,6 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>  
>  	gpmc_cfg = board_data;
>  
> +	ret = platform_device_register(&gpmc_smsc911x_regulator);
> +	if (ret < 0) {
> +		pr_err("Unable to register smsc911x regulators: %d\n", ret);
> +		return;
> +	}
> +

Boards that have more than one instance of the smsc911x (OMAP3/Overo)
barf here because of trying to register the same device twice.

We need something like the patch below to make Overo boot again.

Kevin



>From 4114dea2fb897ab75d05eaa943d29454034fc025 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Thu, 1 Mar 2012 12:30:42 -0800
Subject: [PATCH] ARM: OMAP2+: gpmc-smsc911x: only register regulators once

commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add required smsc911x
regulators) added regulators which are registered during
gpmc_smsc911x_init().  However, some platforms (OMAP3/Overo) have more
than one instance of the SMSC911x and result in attempting to register
the same regulator more than once which causes a panic().  Fix this by
tracking the regulator registration ensuring only a single device is
registered.

Cc: Matt Porter <mporter@ti.com>
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/gpmc-smsc911x.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
index bbb870c..95e6c7d 100644
--- a/arch/arm/mach-omap2/gpmc-smsc911x.c
+++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
@@ -88,6 +88,8 @@ static struct platform_device gpmc_smsc911x_regulator = {
 	},
 };
 
+static bool regulator_registered;
+
 /*
  * Initialize smsc911x device connected to the GPMC. Note that we
  * assume that pin multiplexing is done in the board-*.c file,
@@ -101,10 +103,14 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
 
 	gpmc_cfg = board_data;
 
-	ret = platform_device_register(&gpmc_smsc911x_regulator);
-	if (ret < 0) {
-		pr_err("Unable to register smsc911x regulators: %d\n", ret);
-		return;
+	if (!regulator_registered) {
+		ret = platform_device_register(&gpmc_smsc911x_regulator);
+		if (ret < 0) {
+			pr_err("Unable to register smsc911x regulators: %d\n",
+			       ret);
+			return;
+		}
+		regulator_registered = true;
 	}
 
 	if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) {
-- 
1.7.9.2

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3504 bytes --]

Hi,

On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote:
> Matt Porter <mporter@ti.com> writes:
> 
> > This fixes smsc911x support on platforms using gpmc_smsc911x_init().
> >
> > Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
> > the requirement that platforms provide vdd33a and vddvario supplies.
> >
> > Signed-off-by: Matt Porter <mporter@ti.com>
> 
> [...]
> 
> >  /*
> >   * Initialize smsc911x device connected to the GPMC. Note that we
> >   * assume that pin multiplexing is done in the board-*.c file,
> > @@ -55,6 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
> >  
> >  	gpmc_cfg = board_data;
> >  
> > +	ret = platform_device_register(&gpmc_smsc911x_regulator);
> > +	if (ret < 0) {
> > +		pr_err("Unable to register smsc911x regulators: %d\n", ret);
> > +		return;
> > +	}
> > +
> 
> Boards that have more than one instance of the smsc911x (OMAP3/Overo)
> barf here because of trying to register the same device twice.
> 
> We need something like the patch below to make Overo boot again.
> 
> Kevin
> 
> 
> 
> From 4114dea2fb897ab75d05eaa943d29454034fc025 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@ti.com>
> Date: Thu, 1 Mar 2012 12:30:42 -0800
> Subject: [PATCH] ARM: OMAP2+: gpmc-smsc911x: only register regulators once
> 
> commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add required smsc911x
> regulators) added regulators which are registered during
> gpmc_smsc911x_init().  However, some platforms (OMAP3/Overo) have more
> than one instance of the SMSC911x and result in attempting to register
> the same regulator more than once which causes a panic().  Fix this by
> tracking the regulator registration ensuring only a single device is
> registered.
> 
> Cc: Matt Porter <mporter@ti.com>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc-smsc911x.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
> index bbb870c..95e6c7d 100644
> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c
> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
> @@ -88,6 +88,8 @@ static struct platform_device gpmc_smsc911x_regulator = {
>  	},
>  };
>  
> +static bool regulator_registered;
> +
>  /*
>   * Initialize smsc911x device connected to the GPMC. Note that we
>   * assume that pin multiplexing is done in the board-*.c file,
> @@ -101,10 +103,14 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>  
>  	gpmc_cfg = board_data;
>  
> -	ret = platform_device_register(&gpmc_smsc911x_regulator);
> -	if (ret < 0) {
> -		pr_err("Unable to register smsc911x regulators: %d\n", ret);
> -		return;
> +	if (!regulator_registered) {
> +		ret = platform_device_register(&gpmc_smsc911x_regulator);
> +		if (ret < 0) {
> +			pr_err("Unable to register smsc911x regulators: %d\n",
> +			       ret);
> +			return;
> +		}
> +		regulator_registered = true;

Wow, this is quite a hack. Is the regulator part of the SMSC911x device
or is it outside ? If it's outside the SMSC91xx (which I believe it is)
there should be a regulator driver instead of this hack. For boards
which don't provide such a regulator (and tie the supply pin to some
constant voltage source) there should be a constant regulator supplying
the pin. But this patch is quite hacky.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote:
>> Matt Porter <mporter@ti.com> writes:
>> 
>> > This fixes smsc911x support on platforms using gpmc_smsc911x_init().
>> >
>> > Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
>> > the requirement that platforms provide vdd33a and vddvario supplies.
>> >
>> > Signed-off-by: Matt Porter <mporter@ti.com>
>> 
>> [...]
>> 
>> >  /*
>> >   * Initialize smsc911x device connected to the GPMC. Note that we
>> >   * assume that pin multiplexing is done in the board-*.c file,
>> > @@ -55,6 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>> >  
>> >  	gpmc_cfg = board_data;
>> >  
>> > +	ret = platform_device_register(&gpmc_smsc911x_regulator);
>> > +	if (ret < 0) {
>> > +		pr_err("Unable to register smsc911x regulators: %d\n", ret);
>> > +		return;
>> > +	}
>> > +
>> 
>> Boards that have more than one instance of the smsc911x (OMAP3/Overo)
>> barf here because of trying to register the same device twice.
>> 
>> We need something like the patch below to make Overo boot again.
>> 
>> Kevin
>> 
>> 
>> 
>> From 4114dea2fb897ab75d05eaa943d29454034fc025 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@ti.com>
>> Date: Thu, 1 Mar 2012 12:30:42 -0800
>> Subject: [PATCH] ARM: OMAP2+: gpmc-smsc911x: only register regulators once
>> 
>> commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add required smsc911x
>> regulators) added regulators which are registered during
>> gpmc_smsc911x_init().  However, some platforms (OMAP3/Overo) have more
>> than one instance of the SMSC911x and result in attempting to register
>> the same regulator more than once which causes a panic().  Fix this by
>> tracking the regulator registration ensuring only a single device is
>> registered.
>> 
>> Cc: Matt Porter <mporter@ti.com>
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> ---
>>  arch/arm/mach-omap2/gpmc-smsc911x.c |   14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
>> index bbb870c..95e6c7d 100644
>> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c
>> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
>> @@ -88,6 +88,8 @@ static struct platform_device gpmc_smsc911x_regulator = {
>>  	},
>>  };
>>  
>> +static bool regulator_registered;
>> +
>>  /*
>>   * Initialize smsc911x device connected to the GPMC. Note that we
>>   * assume that pin multiplexing is done in the board-*.c file,
>> @@ -101,10 +103,14 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>>  
>>  	gpmc_cfg = board_data;
>>  
>> -	ret = platform_device_register(&gpmc_smsc911x_regulator);
>> -	if (ret < 0) {
>> -		pr_err("Unable to register smsc911x regulators: %d\n", ret);
>> -		return;
>> +	if (!regulator_registered) {
>> +		ret = platform_device_register(&gpmc_smsc911x_regulator);
>> +		if (ret < 0) {
>> +			pr_err("Unable to register smsc911x regulators: %d\n",
>> +			       ret);
>> +			return;
>> +		}
>> +		regulator_registered = true;
>
> Wow, this is quite a hack. Is the regulator part of the SMSC911x
> device or is it outside ? If it's outside the SMSC91xx (which I
> believe it is) there should be a regulator driver instead of this
> hack.  For boards which don't provide such a regulator (and tie the
> supply pin to some constant voltage source) there should be a constant
> regulator supplying the pin. But this patch is quite hacky.

Are you referring to my patch or to the original $SUBJECT patch?  It's
not terribly clear.

My patch doesn't add any regulators, it just fixes a bug when this init
function is called more than once.

The addition of the regulators was done in $SUBJECT patch, not my fix.

In either case $SUBJECT patch is already in Tony's fixes queue, so if it
is going be merged, then my fix above is needed also to not break
Overo and any other platform that has more than one smsc911x instance.

Kevin

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

[Russ Dill]

On Mon, Mar 5, 2012 at 12:18 PM, Kevin Hilman <khilman@ti.com> wrote:
> Felipe Balbi <balbi@ti.com> writes:
>
>> Hi,
>>
>> On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote:
>>> Matt Porter <mporter@ti.com> writes:
>>>
>>> > This fixes smsc911x support on platforms using gpmc_smsc911x_init().
>>> >
>>> > Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
>>> > the requirement that platforms provide vdd33a and vddvario supplies.
>>> >
>>> > Signed-off-by: Matt Porter <mporter@ti.com>
>>>
>>> [...]
>>>
>>> >  /*
>>> >   * Initialize smsc911x device connected to the GPMC. Note that we
>>> >   * assume that pin multiplexing is done in the board-*.c file,
>>> > @@ -55,6 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>>> >
>>> >    gpmc_cfg = board_data;
>>> >
>>> > +  ret = platform_device_register(&gpmc_smsc911x_regulator);
>>> > +  if (ret < 0) {
>>> > +          pr_err("Unable to register smsc911x regulators: %d\n", ret);
>>> > +          return;
>>> > +  }
>>> > +
>>>
>>> Boards that have more than one instance of the smsc911x (OMAP3/Overo)
>>> barf here because of trying to register the same device twice.
>>>
>>> We need something like the patch below to make Overo boot again.
>>>
>>> Kevin
>>>
>>>
>>>
>>> From 4114dea2fb897ab75d05eaa943d29454034fc025 Mon Sep 17 00:00:00 2001
>>> From: Kevin Hilman <khilman@ti.com>
>>> Date: Thu, 1 Mar 2012 12:30:42 -0800
>>> Subject: [PATCH] ARM: OMAP2+: gpmc-smsc911x: only register regulators once
>>>
>>> commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add required smsc911x
>>> regulators) added regulators which are registered during
>>> gpmc_smsc911x_init().  However, some platforms (OMAP3/Overo) have more
>>> than one instance of the SMSC911x and result in attempting to register
>>> the same regulator more than once which causes a panic().  Fix this by
>>> tracking the regulator registration ensuring only a single device is
>>> registered.
>>>
>>> Cc: Matt Porter <mporter@ti.com>
>>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/gpmc-smsc911x.c |   14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
>>> index bbb870c..95e6c7d 100644
>>> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c
>>> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
>>> @@ -88,6 +88,8 @@ static struct platform_device gpmc_smsc911x_regulator = {
>>>      },
>>>  };
>>>
>>> +static bool regulator_registered;
>>> +
>>>  /*
>>>   * Initialize smsc911x device connected to the GPMC. Note that we
>>>   * assume that pin multiplexing is done in the board-*.c file,
>>> @@ -101,10 +103,14 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>>>
>>>      gpmc_cfg = board_data;
>>>
>>> -    ret = platform_device_register(&gpmc_smsc911x_regulator);
>>> -    if (ret < 0) {
>>> -            pr_err("Unable to register smsc911x regulators: %d\n", ret);
>>> -            return;
>>> +    if (!regulator_registered) {
>>> +            ret = platform_device_register(&gpmc_smsc911x_regulator);
>>> +            if (ret < 0) {
>>> +                    pr_err("Unable to register smsc911x regulators: %d\n",
>>> +                           ret);
>>> +                    return;
>>> +            }
>>> +            regulator_registered = true;
>>
>> Wow, this is quite a hack. Is the regulator part of the SMSC911x
>> device or is it outside ? If it's outside the SMSC91xx (which I
>> believe it is) there should be a regulator driver instead of this
>> hack.  For boards which don't provide such a regulator (and tie the
>> supply pin to some constant voltage source) there should be a constant
>> regulator supplying the pin. But this patch is quite hacky.
>
> Are you referring to my patch or to the original $SUBJECT patch?  It's
> not terribly clear.
>
> My patch doesn't add any regulators, it just fixes a bug when this init
> function is called more than once.
>
> The addition of the regulators was done in $SUBJECT patch, not my fix.
>
> In either case $SUBJECT patch is already in Tony's fixes queue, so if it
> is going be merged, then my fix above is needed also to not break
> Overo and any other platform that has more than one smsc911x instance.
>
> Kevin


Have you tested this fix? The only regulator consumer supply would be:

static struct regulator_consumer_supply gpmc_smsc911x_supply[] = {
        REGULATOR_SUPPLY("vddvario", "smsc911x.0"),
        REGULATOR_SUPPLY("vdd33a", "smsc911x.0"),
};

I don't think the second smsc911x on the Overo, "smsc911x.1", would
find it due to the dev_id.

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

[Tony Lindgren]

* Russ Dill <Russ.Dill@ti.com> [120307 07:28]:
> On Mon, Mar 5, 2012 at 12:18 PM, Kevin Hilman <khilman@ti.com> wrote:
> >
> > In either case $SUBJECT patch is already in Tony's fixes queue, so if it
> > is going be merged, then my fix above is needed also to not break
> > Overo and any other platform that has more than one smsc911x instance.
> >
> > Kevin
> 
> 
> Have you tested this fix? The only regulator consumer supply would be:
> 
> static struct regulator_consumer_supply gpmc_smsc911x_supply[] = {
>         REGULATOR_SUPPLY("vddvario", "smsc911x.0"),
>         REGULATOR_SUPPLY("vdd33a", "smsc911x.0"),
> };
> 
> I don't think the second smsc911x on the Overo, "smsc911x.1", would
> find it due to the dev_id.

Hmm yeah sounds like there's some further changes needed. Dropping
the second version from fixes until we have somebody reply with
tested-by using two smsc911x instances.

Regards,

Tony

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

[Kevin Hilman]

Hi Russ,

Russ Dill <Russ.Dill@ti.com> writes:

> On Mon, Mar 5, 2012 at 12:18 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Felipe Balbi <balbi@ti.com> writes:
>>
>>> Hi,
>>>
>>> On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote:
>>>> Matt Porter <mporter@ti.com> writes:
>>>>
>>>> > This fixes smsc911x support on platforms using gpmc_smsc911x_init().
>>>> >
>>>> > Commit c7e963f6888816 (net/smsc911x: Add regulator support) added
>>>> > the requirement that platforms provide vdd33a and vddvario supplies.
>>>> >
>>>> > Signed-off-by: Matt Porter <mporter@ti.com>
>>>>
>>>> [...]
>>>>
>>>> >  /*
>>>> >   * Initialize smsc911x device connected to the GPMC. Note that we
>>>> >   * assume that pin multiplexing is done in the board-*.c file,
>>>> > @@ -55,6 +101,12 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>>>> >
>>>> >    gpmc_cfg = board_data;
>>>> >
>>>> > +  ret = platform_device_register(&gpmc_smsc911x_regulator);
>>>> > +  if (ret < 0) {
>>>> > +          pr_err("Unable to register smsc911x regulators: %d\n", ret);
>>>> > +          return;
>>>> > +  }
>>>> > +
>>>>
>>>> Boards that have more than one instance of the smsc911x (OMAP3/Overo)
>>>> barf here because of trying to register the same device twice.
>>>>
>>>> We need something like the patch below to make Overo boot again.
>>>>
>>>> Kevin
>>>>
>>>>
>>>>
>>>> From 4114dea2fb897ab75d05eaa943d29454034fc025 Mon Sep 17 00:00:00 2001
>>>> From: Kevin Hilman <khilman@ti.com>
>>>> Date: Thu, 1 Mar 2012 12:30:42 -0800
>>>> Subject: [PATCH] ARM: OMAP2+: gpmc-smsc911x: only register regulators once
>>>>
>>>> commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add required smsc911x
>>>> regulators) added regulators which are registered during
>>>> gpmc_smsc911x_init().  However, some platforms (OMAP3/Overo) have more
>>>> than one instance of the SMSC911x and result in attempting to register
>>>> the same regulator more than once which causes a panic().  Fix this by
>>>> tracking the regulator registration ensuring only a single device is
>>>> registered.
>>>>
>>>> Cc: Matt Porter <mporter@ti.com>
>>>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>>>> ---
>>>>  arch/arm/mach-omap2/gpmc-smsc911x.c |   14 ++++++++++----
>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
>>>> index bbb870c..95e6c7d 100644
>>>> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c
>>>> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
>>>> @@ -88,6 +88,8 @@ static struct platform_device gpmc_smsc911x_regulator = {
>>>>      },
>>>>  };
>>>>
>>>> +static bool regulator_registered;
>>>> +
>>>>  /*
>>>>   * Initialize smsc911x device connected to the GPMC. Note that we
>>>>   * assume that pin multiplexing is done in the board-*.c file,
>>>> @@ -101,10 +103,14 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data)
>>>>
>>>>      gpmc_cfg = board_data;
>>>>
>>>> -    ret = platform_device_register(&gpmc_smsc911x_regulator);
>>>> -    if (ret < 0) {
>>>> -            pr_err("Unable to register smsc911x regulators: %d\n", ret);
>>>> -            return;
>>>> +    if (!regulator_registered) {
>>>> +            ret = platform_device_register(&gpmc_smsc911x_regulator);
>>>> +            if (ret < 0) {
>>>> +                    pr_err("Unable to register smsc911x regulators: %d\n",
>>>> +                           ret);
>>>> +                    return;
>>>> +            }
>>>> +            regulator_registered = true;
>>>
>>> Wow, this is quite a hack. Is the regulator part of the SMSC911x
>>> device or is it outside ? If it's outside the SMSC91xx (which I
>>> believe it is) there should be a regulator driver instead of this
>>> hack.  For boards which don't provide such a regulator (and tie the
>>> supply pin to some constant voltage source) there should be a constant
>>> regulator supplying the pin. But this patch is quite hacky.
>>
>> Are you referring to my patch or to the original $SUBJECT patch?  It's
>> not terribly clear.
>>
>> My patch doesn't add any regulators, it just fixes a bug when this init
>> function is called more than once.
>>
>> The addition of the regulators was done in $SUBJECT patch, not my fix.
>>
>> In either case $SUBJECT patch is already in Tony's fixes queue, so if it
>> is going be merged, then my fix above is needed also to not break
>> Overo and any other platform that has more than one smsc911x instance.
>>
>> Kevin
>
>
> Have you tested this fix? 

Yes.  On 3530/Overo.

> The only regulator consumer supply would be:
>
> static struct regulator_consumer_supply gpmc_smsc911x_supply[] = {
>         REGULATOR_SUPPLY("vddvario", "smsc911x.0"),
>         REGULATOR_SUPPLY("vdd33a", "smsc911x.0"),
> };
>
> I don't think the second smsc911x on the Overo, "smsc911x.1", would
> find it due to the dev_id.

It's not about finding the second regulator.  As stated in the
changelog, it's about the duplicate attempt to register the exact same
platform_device.

Duplicate attempts to register the exact same platform_device cause
kobject to panic and give up[1].  So, any platform that calls
gpmc_smsc911x_init() twice (Overo and T35 in mainline) will panic on
boot.

This patch fixes those platforms so they can boot.

Kevin



[1]
[    0.337036] kobject (c06b1730): tried to init an initialized object, something is seriously wrong.
[    0.346679] [<c001421c>] (unwind_backtrace+0x0/0xf0) from [<c022b944>] (kobject_init+0x74/0x94)
[    0.355804] [<c022b944>] (kobject_init+0x74/0x94) from [<c0284fa8>] (device_initialize+0x20/0x90)
[    0.365112] [<c0284fa8>] (device_initialize+0x20/0x90) from [<c02894fc>] (platform_device_register+0x10/0x24)
[    0.375488] [<c02894fc>] (platform_device_register+0x10/0x24) from [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200)
[    0.386077] [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200) from [<c06165e0>] (overo_init+0xdc/0x27c)
[    0.395446] [<c06165e0>] (overo_init+0xdc/0x27c) from [<c0609380>] (customize_machine+0x1c/0x28)
[    0.404663] [<c0609380>] (customize_machine+0x1c/0x28) from [<c0008800>] (do_one_initcall+0x34/0x178)
[    0.414306] [<c0008800>] (do_one_initcall+0x34/0x178) from [<c06068ac>] (kernel_init+0x8c/0x130)
[    0.423553] [<c06068ac>] (kernel_init+0x8c/0x130) from [<c000e850>] (kernel_thread_exit+0x0/0x8)
[    0.432800] ------------[ cut here ]------------
[    0.437652] WARNING: at /work/kernel/omap/pm/fs/sysfs/dir.c:481 sysfs_add_one+0x88/0xb0()
[    0.446228] sysfs: cannot create duplicate filename '/devices/platform/reg-fixed-voltage.42'
[    0.455047] Modules linked in:
[    0.458312] [<c001421c>] (unwind_backtrace+0x0/0xf0) from [<c00391e4>] (warn_slowpath_common+0x4c/0x64)
[    0.468170] [<c00391e4>] (warn_slowpath_common+0x4c/0x64) from [<c0039290>] (warn_slowpath_fmt+0x30/0x40)
[    0.478179] [<c0039290>] (warn_slowpath_fmt+0x30/0x40) from [<c014eed8>] (sysfs_add_one+0x88/0xb0)
[    0.487548] [<c014eed8>] (sysfs_add_one+0x88/0xb0) from [<c014ef60>] (create_dir+0x60/0xc4)
[    0.496307] [<c014ef60>] (create_dir+0x60/0xc4) from [<c014f048>] (sysfs_create_dir+0x58/0x8c)
[    0.505340] [<c014f048>] (sysfs_create_dir+0x58/0x8c) from [<c022bbec>] (kobject_add_internal+0x9c/0x1b8)
[    0.515350] [<c022bbec>] (kobject_add_internal+0x9c/0x1b8) from [<c022bfd8>] (kobject_add+0x50/0x98)
[    0.524932] [<c022bfd8>] (kobject_add+0x50/0x98) from [<c0285a14>] (device_add+0xb8/0x358)
[    0.533599] [<c0285a14>] (device_add+0xb8/0x358) from [<c0289234>] (platform_device_add+0xf8/0x1a4)
[    0.543090] [<c0289234>] (platform_device_add+0xf8/0x1a4) from [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200)
[    0.553283] [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200) from [<c06165e0>] (overo_init+0xdc/0x27c)
[    0.562652] [<c06165e0>] (overo_init+0xdc/0x27c) from [<c0609380>] (customize_machine+0x1c/0x28)
[    0.571868] [<c0609380>] (customize_machine+0x1c/0x28) from [<c0008800>] (do_one_initcall+0x34/0x178)
[    0.581512] [<c0008800>] (do_one_initcall+0x34/0x178) from [<c06068ac>] (kernel_init+0x8c/0x130)
[    0.590728] [<c06068ac>] (kernel_init+0x8c/0x130) from [<c000e850>] (kernel_thread_exit+0x0/0x8)
[    0.600372] ---[ end trace 1b75b31a2719ed1c ]---
[    0.605285] kobject_add_internal failed for reg-fixed-voltage.42 with -EEXIST, don't try to register things with the same name in the same directory.
[    0.619262] [<c001421c>] (unwind_backtrace+0x0/0xf0) from [<c022bcf0>] (kobject_add_internal+0x1a0/0x1b8)
[    0.629272] [<c022bcf0>] (kobject_add_internal+0x1a0/0x1b8) from [<c022bfd8>] (kobject_add+0x50/0x98)
[    0.638946] [<c022bfd8>] (kobject_add+0x50/0x98) from [<c0285a14>] (device_add+0xb8/0x358)
[    0.647613] [<c0285a14>] (device_add+0xb8/0x358) from [<c0289234>] (platform_device_add+0xf8/0x1a4)
[    0.657073] [<c0289234>] (platform_device_add+0xf8/0x1a4) from [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200)
[    0.667266] [<c0619d7c>] (gpmc_smsc911x_init+0x1c/0x200) from [<c06165e0>] (overo_init+0xdc/0x27c)
[    0.676666] [<c06165e0>] (overo_init+0xdc/0x27c) from [<c0609380>] (customize_machine+0x1c/0x28)
[    0.685852] [<c0609380>] (customize_machine+0x1c/0x28) from [<c0008800>] (do_one_initcall+0x34/0x178)
[    0.695526] [<c0008800>] (do_one_initcall+0x34/0x178) from [<c06068ac>] (kernel_init+0x8c/0x130)
[    0.704711] [<c06068ac>] (kernel_init+0x8c/0x130) from [<c000e850>] (kernel_thread_exit+0x0/0x8)
[    0.713897] gpmc_smsc911x_init: Unable to register smsc911x regulators: -17

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

[Tony Lindgren]

* Kevin Hilman <khilman@ti.com> [120307 11:05]:
> >
> > I don't think the second smsc911x on the Overo, "smsc911x.1", would
> > find it due to the dev_id.
> 
> It's not about finding the second regulator.  As stated in the
> changelog, it's about the duplicate attempt to register the exact same
> platform_device.
> 
> Duplicate attempts to register the exact same platform_device cause
> kobject to panic and give up[1].  So, any platform that calls
> gpmc_smsc911x_init() twice (Overo and T35 in mainline) will panic on
> boot.
> 
> This patch fixes those platforms so they can boot.

Yeah but I guess the second smsc911x instance still would not work,
or am I missing something?

Regards,

Tony

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

[Kevin Hilman]

Tony Lindgren <tony@atomide.com> writes:

> * Kevin Hilman <khilman@ti.com> [120307 11:05]:
>> >
>> > I don't think the second smsc911x on the Overo, "smsc911x.1", would
>> > find it due to the dev_id.
>> 
>> It's not about finding the second regulator.  As stated in the
>> changelog, it's about the duplicate attempt to register the exact same
>> platform_device.
>> 
>> Duplicate attempts to register the exact same platform_device cause
>> kobject to panic and give up[1].  So, any platform that calls
>> gpmc_smsc911x_init() twice (Overo and T35 in mainline) will panic on
>> boot.
>> 
>> This patch fixes those platforms so they can boot.
>
> Yeah but I guess the second smsc911x instance still would not work,
> or am I missing something?

I don't know since my Overo expansion boards don't have a 2nd NIC, but I
suspect you're right.

However, my fix isn't addressing that.  I am fixing a problem where
mainline today will panic on some boards due to duplicate registration.

If the 2nd interface doesn't work, then the original patch that added
the regulators needs a rethink.  My patch to prevent the panic() is
needed for mainline.

Kevin

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

[Tony Lindgren]

* Kevin Hilman <khilman@ti.com> [120308 12:54]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Kevin Hilman <khilman@ti.com> [120307 11:05]:
> >> >
> >> > I don't think the second smsc911x on the Overo, "smsc911x.1", would
> >> > find it due to the dev_id.
> >> 
> >> It's not about finding the second regulator.  As stated in the
> >> changelog, it's about the duplicate attempt to register the exact same
> >> platform_device.
> >> 
> >> Duplicate attempts to register the exact same platform_device cause
> >> kobject to panic and give up[1].  So, any platform that calls
> >> gpmc_smsc911x_init() twice (Overo and T35 in mainline) will panic on
> >> boot.
> >> 
> >> This patch fixes those platforms so they can boot.
> >
> > Yeah but I guess the second smsc911x instance still would not work,
> > or am I missing something?
> 
> I don't know since my Overo expansion boards don't have a 2nd NIC, but I
> suspect you're right.
> 
> However, my fix isn't addressing that.  I am fixing a problem where
> mainline today will panic on some boards due to duplicate registration.
> 
> If the 2nd interface doesn't work, then the original patch that added
> the regulators needs a rethink.  My patch to prevent the panic() is
> needed for mainline.

With Kevin's second version of this patch applied we avoid the panic
on boards with more than one smsc911x.

After this patch, board-*.c files can add custom regulators for their
other smsc911x instances.

We should also add a regulator to the struct omap_smsc911x_platform_data,
and then only initialize the fixed regulator automatically
if (!board_data->regulator && !board_data->id).

So I've pushed Kevin's second version of the fix to fix-smsc911x-regulator
branch.

Regards,

Tony