From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759895Ab2CGThF (ORCPT ); Wed, 7 Mar 2012 14:37:05 -0500 Received: from na3sys009aog104.obsmtp.com ([74.125.149.73]:42725 "EHLO na3sys009aog104.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758876Ab2CGThC convert rfc822-to-8bit (ORCPT ); Wed, 7 Mar 2012 14:37:02 -0500 From: Kevin Hilman To: Russ Dill Cc: balbi@ti.com, Matt Porter , Tony Lindgren , Russell King , Linux OMAP List , Linux ARM Kernel List , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] ARM: OMAP2+: gpmc-smsc911x: add required smsc911x regulators Organization: Texas Instruments, Inc. References: <1330006564-13290-1-git-send-email-mporter@ti.com> <87wr74gis6.fsf@ti.com> <20120301204553.GA21841@legolas.emea.dhcp.ti.com> <87pqcqnc7u.fsf@ti.com> Date: Wed, 07 Mar 2012 11:36:57 -0800 In-Reply-To: (Russ Dill's message of "Wed, 7 Mar 2012 10:00:08 -0600") Message-ID: <87sjhkjj8m.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Russ, Russ Dill writes: > On Mon, Mar 5, 2012 at 12:18 PM, Kevin Hilman wrote: >> Felipe Balbi writes: >> >>> Hi, >>> >>> On Thu, Mar 01, 2012 at 12:36:57PM -0800, Kevin Hilman wrote: >>>> Matt Porter 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 >>>> >>>> [...] >>>> >>>> >  /* >>>> >   * 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 >>>> 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 >>>> Signed-off-by: Kevin Hilman >>>> --- >>>>  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] [] (unwind_backtrace+0x0/0xf0) from [] (kobject_init+0x74/0x94) [ 0.355804] [] (kobject_init+0x74/0x94) from [] (device_initialize+0x20/0x90) [ 0.365112] [] (device_initialize+0x20/0x90) from [] (platform_device_register+0x10/0x24) [ 0.375488] [] (platform_device_register+0x10/0x24) from [] (gpmc_smsc911x_init+0x1c/0x200) [ 0.386077] [] (gpmc_smsc911x_init+0x1c/0x200) from [] (overo_init+0xdc/0x27c) [ 0.395446] [] (overo_init+0xdc/0x27c) from [] (customize_machine+0x1c/0x28) [ 0.404663] [] (customize_machine+0x1c/0x28) from [] (do_one_initcall+0x34/0x178) [ 0.414306] [] (do_one_initcall+0x34/0x178) from [] (kernel_init+0x8c/0x130) [ 0.423553] [] (kernel_init+0x8c/0x130) from [] (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] [] (unwind_backtrace+0x0/0xf0) from [] (warn_slowpath_common+0x4c/0x64) [ 0.468170] [] (warn_slowpath_common+0x4c/0x64) from [] (warn_slowpath_fmt+0x30/0x40) [ 0.478179] [] (warn_slowpath_fmt+0x30/0x40) from [] (sysfs_add_one+0x88/0xb0) [ 0.487548] [] (sysfs_add_one+0x88/0xb0) from [] (create_dir+0x60/0xc4) [ 0.496307] [] (create_dir+0x60/0xc4) from [] (sysfs_create_dir+0x58/0x8c) [ 0.505340] [] (sysfs_create_dir+0x58/0x8c) from [] (kobject_add_internal+0x9c/0x1b8) [ 0.515350] [] (kobject_add_internal+0x9c/0x1b8) from [] (kobject_add+0x50/0x98) [ 0.524932] [] (kobject_add+0x50/0x98) from [] (device_add+0xb8/0x358) [ 0.533599] [] (device_add+0xb8/0x358) from [] (platform_device_add+0xf8/0x1a4) [ 0.543090] [] (platform_device_add+0xf8/0x1a4) from [] (gpmc_smsc911x_init+0x1c/0x200) [ 0.553283] [] (gpmc_smsc911x_init+0x1c/0x200) from [] (overo_init+0xdc/0x27c) [ 0.562652] [] (overo_init+0xdc/0x27c) from [] (customize_machine+0x1c/0x28) [ 0.571868] [] (customize_machine+0x1c/0x28) from [] (do_one_initcall+0x34/0x178) [ 0.581512] [] (do_one_initcall+0x34/0x178) from [] (kernel_init+0x8c/0x130) [ 0.590728] [] (kernel_init+0x8c/0x130) from [] (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] [] (unwind_backtrace+0x0/0xf0) from [] (kobject_add_internal+0x1a0/0x1b8) [ 0.629272] [] (kobject_add_internal+0x1a0/0x1b8) from [] (kobject_add+0x50/0x98) [ 0.638946] [] (kobject_add+0x50/0x98) from [] (device_add+0xb8/0x358) [ 0.647613] [] (device_add+0xb8/0x358) from [] (platform_device_add+0xf8/0x1a4) [ 0.657073] [] (platform_device_add+0xf8/0x1a4) from [] (gpmc_smsc911x_init+0x1c/0x200) [ 0.667266] [] (gpmc_smsc911x_init+0x1c/0x200) from [] (overo_init+0xdc/0x27c) [ 0.676666] [] (overo_init+0xdc/0x27c) from [] (customize_machine+0x1c/0x28) [ 0.685852] [] (customize_machine+0x1c/0x28) from [] (do_one_initcall+0x34/0x178) [ 0.695526] [] (do_one_initcall+0x34/0x178) from [] (kernel_init+0x8c/0x130) [ 0.704711] [] (kernel_init+0x8c/0x130) from [] (kernel_thread_exit+0x0/0x8) [ 0.713897] gpmc_smsc911x_init: Unable to register smsc911x regulators: -17