[PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
Commit c7e963f616 (net/smsc911x: Add regulator support) breaks
registration of gpmc connected smcs911x devices on machines with
regulator support, but without dummy regulator support.
Commit e4b0b2cbbb (ARM: OMAP2+: gpmc-smsc911x: add
required smsc911x regulators) fixed the issue for boards with single
smsc911x devices, but attempted to register the fixed voltage regulator
twice for boards with 2 devices which fails.
A proper fix needs to supply a proper regulator/consumer mapping for
each smsc911x device. The below patch does that. However, it also
points out how unmanageable things will become if regulators are required
for each and every device for the sake of the one board where they need
to be enabled.
In the case of the smsc911x driver, the ST-Ericsson Snowball has regulators
that need to be powered up for the smsc911x to function. Robert Marklund
added functionality to the smsc911x driver to power up its regulators at
probe time and power them off at remove time. One can see how quickly
unwieldy this would become if it were done for every device. Either the
functionality needs to be moved to Snowball board init code, or a generic
framework needs to be made for attaching regulators to arbitrary devices.
Incidentally, while the patch for the smsc911x regulators is in, it does not
appear that the patch for the snowball to supply these is in.
This patch has been compile tested, but not run tested.
Signed-off-by: Russ Dill russ.d...@ti.com
---
arch/arm/mach-omap2/board-cm-t35.c | 34 +-
arch/arm/mach-omap2/board-igep0020.c| 15 +++--
arch/arm/mach-omap2/board-ldp.c | 14 ++--
arch/arm/mach-omap2/board-omap3evm.c| 21 --
arch/arm/mach-omap2/board-omap3logic.c | 18 +++--
arch/arm/mach-omap2/board-omap3stalker.c| 15 +++--
arch/arm/mach-omap2/board-overo.c | 32 +-
arch/arm/mach-omap2/board-zoom-debugboard.c | 14 ++--
arch/arm/mach-omap2/gpmc-smsc911x.c | 79 +--
arch/arm/plat-omap/include/plat/gpmc-smsc911x.h |6 +-
10 files changed, 154 insertions(+), 94 deletions(-)
diff --git a/arch/arm/mach-omap2/board-cm-t35.c
b/arch/arm/mach-omap2/board-cm-t35.c
index d73316e..4090ca1 100644
--- a/arch/arm/mach-omap2/board-cm-t35.c
+++ b/arch/arm/mach-omap2/board-cm-t35.c
@@ -65,26 +65,28 @@
#include linux/smsc911x.h
#include plat/gpmc-smsc911x.h
-static struct omap_smsc911x_platform_data cm_t35_smsc911x_cfg = {
- .id = 0,
- .cs = CM_T35_SMSC911X_CS,
- .gpio_irq = CM_T35_SMSC911X_GPIO,
- .gpio_reset = -EINVAL,
- .flags = SMSC911X_USE_32BIT | SMSC911X_SAVE_MAC_ADDRESS,
-};
-
-static struct omap_smsc911x_platform_data sb_t35_smsc911x_cfg = {
- .id = 1,
- .cs = SB_T35_SMSC911X_CS,
- .gpio_irq = SB_T35_SMSC911X_GPIO,
- .gpio_reset = -EINVAL,
- .flags = SMSC911X_USE_32BIT | SMSC911X_SAVE_MAC_ADDRESS,
+static struct omap_smsc911x_platform_data cm_smsc911x_cfg[] = {
+ {
+ .id = 0,
+ .cs = CM_T35_SMSC911X_CS,
+ .gpio_irq = CM_T35_SMSC911X_GPIO,
+ .gpio_reset = -EINVAL,
+ .flags = SMSC911X_USE_32BIT
+ | SMSC911X_SAVE_MAC_ADDRESS,
+ },
+ {
+ .id = 1,
+ .cs = SB_T35_SMSC911X_CS,
+ .gpio_irq = SB_T35_SMSC911X_GPIO,
+ .gpio_reset = -EINVAL,
+ .flags = SMSC911X_USE_32BIT
+ | SMSC911X_SAVE_MAC_ADDRESS,
+ }
};
static void __init cm_t35_init_ethernet(void)
{
- gpmc_smsc911x_init(cm_t35_smsc911x_cfg);
- gpmc_smsc911x_init(sb_t35_smsc911x_cfg);
+ gpmc_smsc911x_init(cm_smsc911x_cfg, ARRAY_SIZE(cm_smsc911x_cfg));
}
#else
static inline void __init cm_t35_init_ethernet(void) { return; }
diff --git a/arch/arm/mach-omap2/board-igep0020.c
b/arch/arm/mach-omap2/board-igep0020.c
index a59ace0..b5523b5 100644
--- a/arch/arm/mach-omap2/board-igep0020.c
+++ b/arch/arm/mach-omap2/board-igep0020.c
@@ -206,16 +206,19 @@ static void __init igep_flash_init(void) {}
#include linux/smsc911x.h
#include plat/gpmc-smsc911x.h
-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 struct omap_smsc911x_platform_data smsc911x_cfg[] = {
+ {
+ .cs = IGEP2_SMSC911X_CS,
+ .gpio_irq = IGEP2_SMSC911X_GPIO,
+ .gpio_reset = -EINVAL,
+ .flags = SMSC911X_USE_32BIT
+ |
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Wed, Mar 21, 2012 at 10:55:54AM -0700, Russ Dill wrote:
unwieldy this would become if it were done for every device. Either the
functionality needs to be moved to Snowball board init code, or a generic
framework needs to be made for attaching regulators to arbitrary devices.
Hrm? Adding regulator supply mappings anywhere other than the
initialisation for a specific board would be extremely unusual and
rather suspicious.
+ supplies = kcalloc(ARRAY_SIZE(smsc911x_refs) * num,
+ sizeof(struct regulator_consumer_supply), GFP_KERNEL);
+ if (!supplies) {
+ pr_err(Failed to allocate memory\n);
+ return;
+ }
+
+ for (i = 0; i ARRAY_SIZE(smsc911x_refs) * num; i++) {
+ int id;
+ char *name;
+
+ id = board_data[i / num].id;
+ if (id != -1)
+ name = kasprintf(GFP_KERNEL, smsc911x.%d, id);
+ else
+ name = kstrdup(smsc911x, GFP_KERNEL);
This seems pretty much insane, it's costing a lot more to faff around
like this than it's worth. Just do the setup in the individual boards,
if you really have no idea what's supplying the device (which seems a
bit unusual, more boards like this have things coming off the PMIC than
don't) there's now regulator_register_fixed() which cuts down on the
boilerplate a little.
I'd have complained about the original code if I'd noticed it wasn't a
patch for a particular board as the breakage you've found is obvious.
The regulation constraints it adds are bogus too, it's setting
REGULATOR_CHANGE_MODE on a regulator that doesn't support modes and
REGULATOR_CHANGE_STATUS without supplying the enable GPIO.
signature.asc
Description: Digital signature
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Mar 21, 2012, at 2:13 PM, Mark Brown wrote:
On Wed, Mar 21, 2012 at 10:55:54AM -0700, Russ Dill wrote:
+for (i = 0; i ARRAY_SIZE(smsc911x_refs) * num; i++) {
+int id;
+char *name;
+
+id = board_data[i / num].id;
+if (id != -1)
+name = kasprintf(GFP_KERNEL, smsc911x.%d, id);
+else
+name = kstrdup(smsc911x, GFP_KERNEL);
This seems pretty much insane, it's costing a lot more to faff around
like this than it's worth. Just do the setup in the individual boards,
if you really have no idea what's supplying the device (which seems a
bit unusual, more boards like this have things coming off the PMIC than
don't) there's now regulator_register_fixed() which cuts down on the
boilerplate a little.
I'd have complained about the original code if I'd noticed it wasn't a
patch for a particular board as the breakage you've found is obvious.
The regulation constraints it adds are bogus too, it's setting
REGULATOR_CHANGE_MODE on a regulator that doesn't support modes and
REGULATOR_CHANGE_STATUS without supplying the enable GPIO.
The original patch for the problem was specific to the fixed regulator
on the OMAP3EVM/AMDM37xxEVM board...it was rejected. I was asked to
put something generic in gpmc-smsc911x.c like this.
-Matt
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Mar 21, 2012, at 2:13 PM, Mark Brown wrote: On Wed, Mar 21, 2012 at 10:55:54AM -0700, Russ Dill wrote: unwieldy this would become if it were done for every device. Either the functionality needs to be moved to Snowball board init code, or a generic framework needs to be made for attaching regulators to arbitrary devices. Hrm? Adding regulator supply mappings anywhere other than the initialisation for a specific board would be extremely unusual and rather suspicious. Just to be clear, here is the thread on the board-specific approach: https://lkml.org/lkml/2012/2/8/415 -Matt -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
* Porter, Matt mpor...@ti.com [120321 11:27]: On Mar 21, 2012, at 2:13 PM, Mark Brown wrote: On Wed, Mar 21, 2012 at 10:55:54AM -0700, Russ Dill wrote: unwieldy this would become if it were done for every device. Either the functionality needs to be moved to Snowball board init code, or a generic framework needs to be made for attaching regulators to arbitrary devices. Hrm? Adding regulator supply mappings anywhere other than the initialisation for a specific board would be extremely unusual and rather suspicious. The issue here is that we don't want to copy paste the dummy fixed regulator all over the board-*.c files, and we don't know how the real regulator is wired up. Just to be clear, here is the thread on the board-specific approach: https://lkml.org/lkml/2012/2/8/415 We should use the real regulator if passed from board file. And if no real regulator is passed, just use the dummy fixed regulator in gpmc-smsc911x.c. This patch I posted should fix the situation and allow adding the real regulators to board-*.c files when they become known: http://www.spinics.net/lists/linux-omap/msg66714.html Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Wed, Mar 21, 2012 at 11:39:57AM -0700, Tony Lindgren wrote: * Porter, Matt mpor...@ti.com [120321 11:27]: Hrm? Adding regulator supply mappings anywhere other than the initialisation for a specific board would be extremely unusual and rather suspicious. The issue here is that we don't want to copy paste the dummy fixed regulator all over the board-*.c files, and we don't know how the real regulator is wired up. Well, I don't think it's too unreasonable especially now we've got the fixed helper stuff which slims it right down - it makes it clear there's a missing thing that might need to get filled in and makes it easier to use the same regulator for other devices. Just to be clear, here is the thread on the board-specific approach: https://lkml.org/lkml/2012/2/8/415 We should use the real regulator if passed from board file. And if no real regulator is passed, just use the dummy fixed regulator in gpmc-smsc911x.c. Yes, it should definitely be conditional. This patch I posted should fix the situation and allow adding the real regulators to board-*.c files when they become known: http://www.spinics.net/lists/linux-omap/msg66714.html That should be changed to pass in a boolean flag rather than a pointer to platform device - the board may not have direct access to the relevant regulator (eg, if it's part of a MFD) or the regulator may be on another bus like I2C (for simpler regulator only devices). signature.asc Description: Digital signature
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
* Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:03]: On Wed, Mar 21, 2012 at 11:39:57AM -0700, Tony Lindgren wrote: * Porter, Matt mpor...@ti.com [120321 11:27]: Hrm? Adding regulator supply mappings anywhere other than the initialisation for a specific board would be extremely unusual and rather suspicious. The issue here is that we don't want to copy paste the dummy fixed regulator all over the board-*.c files, and we don't know how the real regulator is wired up. Well, I don't think it's too unreasonable especially now we've got the fixed helper stuff which slims it right down - it makes it clear there's a missing thing that might need to get filled in and makes it easier to use the same regulator for other devices. Right I guess that's a one liner macro now. Just to be clear, here is the thread on the board-specific approach: https://lkml.org/lkml/2012/2/8/415 We should use the real regulator if passed from board file. And if no real regulator is passed, just use the dummy fixed regulator in gpmc-smsc911x.c. Yes, it should definitely be conditional. This patch I posted should fix the situation and allow adding the real regulators to board-*.c files when they become known: http://www.spinics.net/lists/linux-omap/msg66714.html That should be changed to pass in a boolean flag rather than a pointer to platform device - the board may not have direct access to the relevant regulator (eg, if it's part of a MFD) or the regulator may be on another bus like I2C (for simpler regulator only devices). Hmm I see. This means that we need to patch some board files anyways for the boolean flag to use the fixed regulator. This is because for some cases vddvario and vdd33a regulators can come from the mfd/tps/twl chip and it's unsafe to assume that gpmc-smsc911x.c can set up these regulators automatically. Passing a boolean flag to not set up the default regulator would work too, but we'd rather eventually see the real board specific regulators being patched in. So if that's the case, we might as well patch the board files to add the fixed regulators for each one and drop all the regulator code from gpmc-smsc911x.c. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Wed, Mar 21, 2012 at 12:30:47PM -0700, Tony Lindgren wrote: * Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:03]: That should be changed to pass in a boolean flag rather than a pointer to platform device - the board may not have direct access to the relevant regulator (eg, if it's part of a MFD) or the regulator may be on another bus like I2C (for simpler regulator only devices). Hmm I see. This means that we need to patch some board files anyways for the boolean flag to use the fixed regulator. This is because for some cases vddvario and vdd33a regulators can come from the mfd/tps/twl chip and it's unsafe to assume that gpmc-smsc911x.c can set up these regulators automatically. Passing a boolean flag to not set up the default regulator would work too, but we'd rather eventually see the real board specific regulators being patched in. Yes, ideally the boards would do everything and gpmc-smsc911x.c should be able to completely ignore regulators. So if that's the case, we might as well patch the board files to add the fixed regulators for each one and drop all the regulator code from gpmc-smsc911x.c. That's my preferred option, hopefully with the helpers we have for regulator registration we shouldn't need to add device specific helpers. signature.asc Description: Digital signature
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
* Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:41]: On Wed, Mar 21, 2012 at 12:30:47PM -0700, Tony Lindgren wrote: * Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:03]: That should be changed to pass in a boolean flag rather than a pointer to platform device - the board may not have direct access to the relevant regulator (eg, if it's part of a MFD) or the regulator may be on another bus like I2C (for simpler regulator only devices). Hmm I see. This means that we need to patch some board files anyways for the boolean flag to use the fixed regulator. This is because for some cases vddvario and vdd33a regulators can come from the mfd/tps/twl chip and it's unsafe to assume that gpmc-smsc911x.c can set up these regulators automatically. Passing a boolean flag to not set up the default regulator would work too, but we'd rather eventually see the real board specific regulators being patched in. Yes, ideally the boards would do everything and gpmc-smsc911x.c should be able to completely ignore regulators. OK, great, let's do that then. So if that's the case, we might as well patch the board files to add the fixed regulators for each one and drop all the regulator code from gpmc-smsc911x.c. That's my preferred option, hopefully with the helpers we have for regulator registration we shouldn't need to add device specific helpers. Russ, care to update your patch accordingly? Those helpers are queued in linux-next. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Correct registration of multiple gpmc smsc911x devices.
On Wed, Mar 21, 2012 at 12:41 PM, Tony Lindgren t...@atomide.com wrote: * Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:41]: On Wed, Mar 21, 2012 at 12:30:47PM -0700, Tony Lindgren wrote: * Mark Brown broo...@opensource.wolfsonmicro.com [120321 12:03]: That should be changed to pass in a boolean flag rather than a pointer to platform device - the board may not have direct access to the relevant regulator (eg, if it's part of a MFD) or the regulator may be on another bus like I2C (for simpler regulator only devices). Hmm I see. This means that we need to patch some board files anyways for the boolean flag to use the fixed regulator. This is because for some cases vddvario and vdd33a regulators can come from the mfd/tps/twl chip and it's unsafe to assume that gpmc-smsc911x.c can set up these regulators automatically. Passing a boolean flag to not set up the default regulator would work too, but we'd rather eventually see the real board specific regulators being patched in. Yes, ideally the boards would do everything and gpmc-smsc911x.c should be able to completely ignore regulators. OK, great, let's do that then. So if that's the case, we might as well patch the board files to add the fixed regulators for each one and drop all the regulator code from gpmc-smsc911x.c. That's my preferred option, hopefully with the helpers we have for regulator registration we shouldn't need to add device specific helpers. Russ, care to update your patch accordingly? Those helpers are queued in linux-next. No problem. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
