Re: [PATCH v3] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators
From: Kevin Hilman <hidden>
Date: 2012-03-07 19:37:05
Hi Russ, Russ Dill [off-list ref] writes:
On Mon, Mar 5, 2012 at 12:18 PM, Kevin Hilman [off-list ref] wrote:quoted
Felipe Balbi [off-list ref] writes:quoted
Hi, On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote:quoted
Matt Porter [off-list ref] writes:quoted
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 <redacted>[...]quoted
?/* ? * 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 <redacted> 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 <redacted> Signed-off-by: Kevin Hilman <redacted> --- ?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. KevinHave 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