| Message ID | 1328646473-11828-1-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 07, 2012 at 06:27:53PM -0200, Fabio Estevam wrote: > commit c7e963f6 (net/smsc911x: Add regulator support) requires that regulator are provided > to smsc911x chip. > > Provide regulator support to 3ds_debugboard so that mx31pdk/mx27pdk boards can have > ethernet working again. > > Tested on a mx31pdk board. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Sascha, > > If you think this patch is OK, then I can submit patches for fixing > the other i.MX boards that have smsc911x. > > arch/arm/configs/imx_v6_v7_defconfig | 1 + > arch/arm/plat-mxc/3ds_debugboard.c | 42 ++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig > index 3a4fb2e..0b437d1 100644 > --- a/arch/arm/configs/imx_v6_v7_defconfig > +++ b/arch/arm/configs/imx_v6_v7_defconfig > @@ -126,6 +126,7 @@ CONFIG_WATCHDOG=y > CONFIG_IMX2_WDT=y > CONFIG_MFD_MC13XXX=y > CONFIG_REGULATOR=y > +CONFIG_REGULATOR_FIXED_VOLTAGE=y > CONFIG_REGULATOR_MC13892=y > CONFIG_USB=y > CONFIG_USB_EHCI_HCD=y > diff --git a/arch/arm/plat-mxc/3ds_debugboard.c b/arch/arm/plat-mxc/3ds_debugboard.c > index f0ba072..37c9160 100644 > --- a/arch/arm/plat-mxc/3ds_debugboard.c > +++ b/arch/arm/plat-mxc/3ds_debugboard.c > @@ -16,6 +16,8 @@ > #include <linux/platform_device.h> > #include <linux/gpio.h> > #include <linux/smsc911x.h> > +#include <linux/regulator/machine.h> > +#include <linux/regulator/fixed.h> > > #include <mach/hardware.h> > > @@ -61,6 +63,44 @@ > > static void __iomem *brd_io; > > +#if defined(CONFIG_REGULATOR_FIXED_VOLTAGE) || defined(CONFIG_REGULATOR_FIXED_VOLTAGE_MODULE) > +static struct regulator_consumer_supply debugboard_smsc_supplies[] = { > + REGULATOR_SUPPLY("vdd33a", NULL), > + REGULATOR_SUPPLY("vddvario", NULL), > +}; > + > +static struct regulator_init_data debugboard_smsc_init_data = { > + .constraints = { > + .name = "3V3", > + .always_on = 1, > + }, > + .consumer_supplies = debugboard_smsc_supplies, > + .num_consumer_supplies = ARRAY_SIZE(debugboard_smsc_supplies), > +}; > + > +static struct fixed_voltage_config debugboard_smsc_pdata = { > + .supply_name = "board-3V3", > + .microvolts = 3300000, > + .gpio = -EINVAL, > + .enabled_at_boot = 1, > + .init_data = &debugboard_smsc_init_data, > +}; > +static struct platform_device debugboard_voltage_regulator = { > + .name = "reg-fixed-voltage", > + .id = -1, > + .num_resources = 0, > + .dev = { > + .platform_data = &debugboard_smsc_pdata, > + }, > +}; > +static void __init debugboard_add_regulators(void) > +{ > + platform_device_register(&debugboard_voltage_regulator); > +} > +#else > +static void __init debugboard_add_regulators(void) {} > +#endif I really don't want to have that much code for saying 'we don't have a regulator, just make this stupid driver happy'. This is fine for one board, but not for the more than a dozen users of this driver in the tree. We should really add a helper to simplify this. Sascha
Hi Sascha, On 2/13/12, Sascha Hauer <s.hauer@pengutronix.de> wrote: > I really don't want to have that much code for saying 'we don't have a > regulator, just make this stupid driver happy'. This is fine for one > board, but not for the more than a dozen users of this driver in the tree. > We should really add a helper to simplify this. Will you consider applying this patch to fix 3ds_debugboard at least? It is too bad to lose Ethernet on mx31pdk. As far as the other users of the smsx911x driver in the tree, I have no way to know whether each board does use a regulator or not, so that their code can be fixed properly. Regards, Fabio Estevam
On Mon, Feb 13, 2012 at 09:27:41AM +0100, Sascha Hauer wrote: > I really don't want to have that much code for saying 'we don't have a > regulator, just make this stupid driver happy'. This is fine for one > board, but not for the more than a dozen users of this driver in the tree. > We should really add a helper to simplify this. If the board really has so few software controlled regulators then they should just be using the dummy regulator support which already exists, someone should just add a knob to turn it on automatically.
On Mon, Feb 13, 2012 at 06:48:21AM -0800, Mark Brown wrote: > On Mon, Feb 13, 2012 at 09:27:41AM +0100, Sascha Hauer wrote: > > > I really don't want to have that much code for saying 'we don't have a > > regulator, just make this stupid driver happy'. This is fine for one > > board, but not for the more than a dozen users of this driver in the tree. > > We should really add a helper to simplify this. > > If the board really has so few software controlled regulators then they > should just be using the dummy regulator support which already exists, > someone should just add a knob to turn it on automatically. I thought we agree that generally turning on the dummy regulator is a bad idea. Now you are advocating for the dummy regulator again. Sascha
On Tue, Feb 14, 2012 at 11:58:23AM +0100, Sascha Hauer wrote: > On Mon, Feb 13, 2012 at 06:48:21AM -0800, Mark Brown wrote: > > If the board really has so few software controlled regulators then they > > should just be using the dummy regulator support which already exists, > > someone should just add a knob to turn it on automatically. > I thought we agree that generally turning on the dummy regulator is a > bad idea. Now you are advocating for the dummy regulator again. Well, no. I'm saying that people are being far too trigger happy with it and that if someone wants to add a mechanism for turning it on they need to deal with the fact that it currntly warns every time it does anything but there are some resonable use cases, especially on very old boards. The main thing here is to avoid these driver specific bodges that people keep churning out again and again, it's quite depressing really.
On Thu, Feb 16, 2012 at 08:32:36AM +0100, Sascha Hauer wrote: > On Tue, Feb 14, 2012 at 09:29:04AM -0800, Mark Brown wrote: > > The main thing here is to avoid these driver specific bodges that people > > keep churning out again and again, it's quite depressing really. > I think this churning will continue until we either make the dummy > regulator non optional and drop this warning that gets printed each > time it is used, or we at least provide a way to easily add a fixed That's obviously not a good idea, if we do that we may as well just drop all error checking from the API. > dummy regulator without adding >20 lines of code to each board just > for saying that we don't have a regulator for this particular device. It's not per device, of course - there's an overhead from putting a fixed regulator in but then per supply it's just a line. > +#if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE) > +struct platform_device *regulator_register_fixed(const char *name, int id, > + int microvolts, struct regulator_consumer_supply *supplies, > + int num_supplies); > +#else > +static struct platform_device *regulator_register_fixed(const char *name, int id, > + int microvolts, struct regulator_consumer_supply *supplies, > + int num_supplies) > +{ > + return NULL; > +} > +#endif This is obviously not good for users, they'd still have to do error checking to determine if the device was created or not and then manually register the device with the driver core and ideally also care if that worked or not. I'm not sure something like this will really save enough unless the device actually gets registered by the function, otherwise it's going to be converting data to code. I'd also drop the microvolts and name parameters, if people are going to be doing enough work to describe the individual rails on the board they're probably not going to be put off by having to register a platform device. Of course with device tree this all becomes moot as this won't be happening from code anyway...
On Wed, Feb 15, 2012 at 11:58:26PM -0800, Mark Brown wrote: > On Thu, Feb 16, 2012 at 08:32:36AM +0100, Sascha Hauer wrote: > > On Tue, Feb 14, 2012 at 09:29:04AM -0800, Mark Brown wrote: > > > > The main thing here is to avoid these driver specific bodges that people > > > keep churning out again and again, it's quite depressing really. > > > I think this churning will continue until we either make the dummy > > regulator non optional and drop this warning that gets printed each > > time it is used, or we at least provide a way to easily add a fixed > > That's obviously not a good idea, if we do that we may as well just drop > all error checking from the API. > > > dummy regulator without adding >20 lines of code to each board just > > for saying that we don't have a regulator for this particular device. > > It's not per device, of course - there's an overhead from putting a > fixed regulator in but then per supply it's just a line. You mean one supply if the voltages are the same, right? Otherwise we need multiple fixed regulators (or we shouldn't claim that the board-dummy-fixed-catch-all regulator has a particular voltage) > > > +#if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE) > > +struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies); > > +#else > > +static struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies) > > +{ > > + return NULL; > > +} > > +#endif > > This is obviously not good for users, they'd still have to do error > checking to determine if the device was created or not and then manually > register the device with the driver core and ideally also care if that > worked or not. I understand that error checking is a good idea, but what do you mean with 'manually register the device with the core'? The regulator is registered with the core in this function. > I'm not sure something like this will really save enough > unless the device actually gets registered by the function, otherwise > it's going to be converting data to code. > > I'd also drop the microvolts and name parameters, if people are going to > be doing enough work to describe the individual rails on the board > they're probably not going to be put off by having to register a > platform device. > > Of course with device tree this all becomes moot as this won't be > happening from code anyway... I wonder what the devicetree guys will do with this situation anyway. The devicetree won't describe regulators that are actually not present in the hardware, does it? Don't get me wrong. All I want is just a way for people to be able to add regulator support to drivers *without* breaking its users. Normally we have the policy in the kernel that changes to the kernel do not break its users. The smsc case violated this and it will happen again. In the end it doesn't even matter if a particular board could control a supply via software or not. Patches should not simply declare all users of a driver as broken. Sascha
On Thu, Feb 16, 2012 at 10:13:52AM +0100, Sascha Hauer wrote: > On Wed, Feb 15, 2012 at 11:58:26PM -0800, Mark Brown wrote: > > It's not per device, of course - there's an overhead from putting a > > fixed regulator in but then per supply it's just a line. > You mean one supply if the voltages are the same, right? Otherwise > we need multiple fixed regulators (or we shouldn't claim that the > board-dummy-fixed-catch-all regulator has a particular voltage) I'd expect that anyone who's bothered by all this stuff wouldn't be going to fill in the voltages accurately. > > This is obviously not good for users, they'd still have to do error > > checking to determine if the device was created or not and then manually > > register the device with the driver core and ideally also care if that > > worked or not. > I understand that error checking is a good idea, but what do you mean > with 'manually register the device with the core'? The regulator is > registered with the core in this function. Oh, in that case it seems really odd that it's returning a pointer to the device rather than just returning success or failure. > > Of course with device tree this all becomes moot as this won't be > > happening from code anyway... > I wonder what the devicetree guys will do with this situation anyway. > The devicetree won't describe regulators that are actually not present > in the hardware, does it? In all these cases there is actually a physical supply of some kind; if there isn't one then you'd expect the driver would have explicit code to handle that in some way. > Don't get me wrong. All I want is just a way for people to be able to > add regulator support to drivers *without* breaking its users. Normally > we have the policy in the kernel that changes to the kernel do not break > its users. The smsc case violated this and it will happen again. In the > end it doesn't even matter if a particular board could control a supply > via software or not. Patches should not simply declare all users of a > driver as broken. Yes, I'm quite disappointed with people who are adding regulator support without making an effort to go round all the uers and at least notify the existing users of the device that they need updates. There's a limited amount we can do in the core
On Thu, Feb 16, 2012 at 08:27:04AM -0800, Mark Brown wrote: > On Thu, Feb 16, 2012 at 10:13:52AM +0100, Sascha Hauer wrote: > > On Wed, Feb 15, 2012 at 11:58:26PM -0800, Mark Brown wrote: > > > > It's not per device, of course - there's an overhead from putting a > > > fixed regulator in but then per supply it's just a line. > > > You mean one supply if the voltages are the same, right? Otherwise > > we need multiple fixed regulators (or we shouldn't claim that the > > board-dummy-fixed-catch-all regulator has a particular voltage) > > I'd expect that anyone who's bothered by all this stuff wouldn't be > going to fill in the voltages accurately. Ok, so then I'll drop the voltage parameter. > > > > This is obviously not good for users, they'd still have to do error > > > checking to determine if the device was created or not and then manually > > > register the device with the driver core and ideally also care if that > > > worked or not. > > > I understand that error checking is a good idea, but what do you mean > > with 'manually register the device with the core'? The regulator is > > registered with the core in this function. > > Oh, in that case it seems really odd that it's returning a pointer to > the device rather than just returning success or failure. It has to as it gives you a pointer which you can use to unregister the device later. Also it's in line with the platform helpers like platform_device_register_resndata, platform_device_register_simple and others. > > > > Of course with device tree this all becomes moot as this won't be > > > happening from code anyway... > > > I wonder what the devicetree guys will do with this situation anyway. > > The devicetree won't describe regulators that are actually not present > > in the hardware, does it? > > In all these cases there is actually a physical supply of some kind; if > there isn't one then you'd expect the driver would have explicit code to > handle that in some way. Yes, there is one. But I think it will only be described in the device tree if it's software controllable. > > > Don't get me wrong. All I want is just a way for people to be able to > > add regulator support to drivers *without* breaking its users. Normally > > we have the policy in the kernel that changes to the kernel do not break > > its users. The smsc case violated this and it will happen again. In the > > end it doesn't even matter if a particular board could control a supply > > via software or not. Patches should not simply declare all users of a > > driver as broken. > > Yes, I'm quite disappointed with people who are adding regulator support > without making an effort to go round all the uers and at least notify > the existing users of the device that they need updates. There's a > limited amount we can do in the core Are you then willing to merge my patch to make it easier to not break boards? I would remove the voltage parameter and fix a remaining Kconfig/ifdef issue then. Sascha
On Fri, Feb 17, 2012 at 08:40:07AM +0100, Sascha Hauer wrote: > On Thu, Feb 16, 2012 at 08:27:04AM -0800, Mark Brown wrote: > > Oh, in that case it seems really odd that it's returning a pointer to > > the device rather than just returning success or failure. > It has to as it gives you a pointer which you can use to unregister the > device later. Also it's in line with the platform helpers like > platform_device_register_resndata, platform_device_register_simple and > others. Hrm, right. Again I'd expect all users to be in board code so not ever unregistering anything. > > In all these cases there is actually a physical supply of some kind; if > > there isn't one then you'd expect the driver would have explicit code to > > handle that in some way. > Yes, there is one. But I think it will only be described in the device > tree if it's software controllable. I don't see why that needs to be the case. > > without making an effort to go round all the uers and at least notify > > the existing users of the device that they need updates. There's a > > limited amount we can do in the core > Are you then willing to merge my patch to make it easier to not break > boards? I would remove the voltage parameter and fix a remaining > Kconfig/ifdef issue then. Well, if you send a patch that looks sensible and is signed off and whatnot yes. I've no problem with a helper function.
On Wed, Feb 15, 2012 at 11:58:26PM -0800, Mark Brown wrote: > On Thu, Feb 16, 2012 at 08:32:36AM +0100, Sascha Hauer wrote: > > On Tue, Feb 14, 2012 at 09:29:04AM -0800, Mark Brown wrote: > > > > The main thing here is to avoid these driver specific bodges that people > > > keep churning out again and again, it's quite depressing really. > > > I think this churning will continue until we either make the dummy > > regulator non optional and drop this warning that gets printed each > > time it is used, or we at least provide a way to easily add a fixed > > That's obviously not a good idea, if we do that we may as well just drop > all error checking from the API. > > > dummy regulator without adding >20 lines of code to each board just > > for saying that we don't have a regulator for this particular device. > > It's not per device, of course - there's an overhead from putting a > fixed regulator in but then per supply it's just a line. > > > +#if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE) > > +struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies); > > +#else > > +static struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies) > > +{ > > + return NULL; > > +} > > +#endif > > This is obviously not good for users, they'd still have to do error > checking to determine if the device was created or not and then manually > register the device with the driver core and ideally also care if that > worked or not. I'm not sure something like this will really save enough > unless the device actually gets registered by the function, otherwise > it's going to be converting data to code. > > I'd also drop the microvolts and name parameters, if people are going to > be doing enough work to describe the individual rails on the board > they're probably not going to be put off by having to register a > platform device. If I drop the microvolts parameter what microvolts should I assume? I tried 0 but the regulator core does not like it: machine_constraints_voltage: dummy: unsupportable voltage constraints (with max: -2147483648 min: 2147483647) Sascha
On Mon, Feb 27, 2012 at 10:04:53AM +0100, Sascha Hauer wrote: > If I drop the microvolts parameter what microvolts should I assume? I > tried 0 but the regulator core does not like it: > machine_constraints_voltage: dummy: unsupportable voltage constraints > (with max: -2147483648 min: 2147483647) I'd expect zero to work, unless something wants a particular voltage in which case you really should be filling things in properly. The fact that the fixed voltage regulator supports get_voltage() might be confusing things. Like I say if people can be bothered filling voltages in I suspect they're not your target audience for this.
On Mon, Feb 27, 2012 at 01:29:08PM +0000, Mark Brown wrote: > On Mon, Feb 27, 2012 at 10:04:53AM +0100, Sascha Hauer wrote: > > > If I drop the microvolts parameter what microvolts should I assume? I > > tried 0 but the regulator core does not like it: > > > machine_constraints_voltage: dummy: unsupportable voltage constraints > > > (with max: -2147483648 min: 2147483647) > > I'd expect zero to work, unless something wants a particular voltage in > which case you really should be filling things in properly. The fact > that the fixed voltage regulator supports get_voltage() might be > confusing things. > > Like I say if people can be bothered filling voltages in I suspect > they're not your target audience for this. The culprit is in machine_constraints_voltage: > > int min_uV = INT_MAX; > int max_uV = INT_MIN; > > ... > > /* initial: [cmin..cmax] valid, [min_uV..max_uV] not */ > for (i = 0; i < count; i++) { > int value; > > value = ops->list_voltage(rdev, i); > if (value <= 0) > continue; list_voltage returns 0 if the fixed regulator is initialized to 0 microvolts, so... > > /* maybe adjust [min_uV..max_uV] */ > if (value >= cmin && value < min_uV) > min_uV = value; > if (value <= cmax && value > max_uV) > max_uV = value; max_uV/min_uV are not initialized... > } > > /* final: [min_uV..max_uV] valid iff constraints valid */ > if (max_uV < min_uV) { > rdev_err(rdev, "unsupportable voltage constraints\n"); > return -EINVAL; > } And we end here. I don't know what to do here. Probably the check for value <= 0 should be removed, but then again the description shows: * @list_voltage: Return one of the supported voltages, in microvolts; zero * if the selector indicates a voltage that is unusable on this system; * or negative errno. Selectors range from zero to one less than * regulator_desc.n_voltages. Voltages may be reported in any order. (I wonder if negative voltages are intentionally not supported) Sascha
On Mon, Feb 27, 2012 at 06:20:20PM +0100, Sascha Hauer wrote: > I don't know what to do here. Probably the check for value <= 0 should > be removed, but then again the description shows: No, zero is intentionally a valid response - it means that the selector is invalid (for example, due to being documented as a reserved value) allowing a non-error return that doesn't provide a voltage. The fixed regulator should be fixed to return an error if it hasn't got a voltage configured.
On Mon, Feb 27, 2012 at 11:54:00PM +0000, Mark Brown wrote: > On Mon, Feb 27, 2012 at 06:20:20PM +0100, Sascha Hauer wrote: > > > I don't know what to do here. Probably the check for value <= 0 should > > be removed, but then again the description shows: > > No, zero is intentionally a valid response - it means that the selector > is invalid (for example, due to being documented as a reserved value) > allowing a non-error return that doesn't provide a voltage. The fixed > regulator should be fixed to return an error if it hasn't got a voltage > configured. Hm, you asked me to drop the voltage parameter from the helper function registering a fixed regulator as dummy. When the fixed regulator bails out in this case I can't see how this can be done. Sascha
On Tue, Feb 28, 2012 at 09:27:15AM +0100, Sascha Hauer wrote: > On Mon, Feb 27, 2012 at 11:54:00PM +0000, Mark Brown wrote: > > No, zero is intentionally a valid response - it means that the selector > > is invalid (for example, due to being documented as a reserved value) > > allowing a non-error return that doesn't provide a voltage. The fixed > > regulator should be fixed to return an error if it hasn't got a voltage > > configured. > Hm, you asked me to drop the voltage parameter from the helper function > registering a fixed regulator as dummy. When the fixed regulator bails > out in this case I can't see how this can be done. Can you be more specific about what you think would break here please?
On Tue, Feb 28, 2012 at 10:52:22AM +0000, Mark Brown wrote: > On Tue, Feb 28, 2012 at 09:27:15AM +0100, Sascha Hauer wrote: > > On Mon, Feb 27, 2012 at 11:54:00PM +0000, Mark Brown wrote: > > > > No, zero is intentionally a valid response - it means that the selector > > > is invalid (for example, due to being documented as a reserved value) > > > allowing a non-error return that doesn't provide a voltage. The fixed > > > regulator should be fixed to return an error if it hasn't got a voltage > > > configured. > > > Hm, you asked me to drop the voltage parameter from the helper function > > registering a fixed regulator as dummy. When the fixed regulator bails > > out in this case I can't see how this can be done. > > Can you be more specific about what you think would break here please? We started this discussion with a patch of mine in which I wanted to add a helper function to register a fixed regulator. You asked me to drop the voltage parameter in the helper function as this is mainly used as a dummy regulator. I then tried to remove the voltage parameter and it turned out that the regulator core needs a voltage because otherwise it bails out in machine_constraints_voltage(), more specifically in ops->list_voltage(rdev, i) returing 0 for a dummy fixed regulator. You then answered that the fixed regulator should be fixed to return an error if it hasn't got a voltage configured. At this point the circle is complete and we can start the discussion again. Sascha
On Wed, Feb 29, 2012 at 09:33:17AM +0100, Sascha Hauer wrote: > On Tue, Feb 28, 2012 at 10:52:22AM +0000, Mark Brown wrote: > > Can you be more specific about what you think would break here please? > We started this discussion with a patch of mine in which I wanted to add > a helper function to register a fixed regulator. You asked me to drop > the voltage parameter in the helper function as this is mainly used as > a dummy regulator. I then tried to remove the voltage parameter and it > turned out that the regulator core needs a voltage because otherwise it > bails out in machine_constraints_voltage(), more specifically in > ops->list_voltage(rdev, i) returing 0 for a dummy fixed regulator. You > then answered that the fixed regulator should be fixed to return an error > if it hasn't got a voltage configured. At this point the circle is > complete and we can start the discussion again. I'm sorry but you're still not explaining what the problem is - what makes you say that reporting errors and so on instead of zero volts would cause a problem?
On Wed, Feb 15, 2012 at 11:58:26PM -0800, Mark Brown wrote: > On Thu, Feb 16, 2012 at 08:32:36AM +0100, Sascha Hauer wrote: > > On Tue, Feb 14, 2012 at 09:29:04AM -0800, Mark Brown wrote: > > > > The main thing here is to avoid these driver specific bodges that people > > > keep churning out again and again, it's quite depressing really. > > > I think this churning will continue until we either make the dummy > > regulator non optional and drop this warning that gets printed each > > time it is used, or we at least provide a way to easily add a fixed > > That's obviously not a good idea, if we do that we may as well just drop > all error checking from the API. > > > dummy regulator without adding >20 lines of code to each board just > > for saying that we don't have a regulator for this particular device. > > It's not per device, of course - there's an overhead from putting a > fixed regulator in but then per supply it's just a line. > > > +#if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE) > > +struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies); > > +#else > > +static struct platform_device *regulator_register_fixed(const char *name, int id, > > + int microvolts, struct regulator_consumer_supply *supplies, > > + int num_supplies) > > +{ > > + return NULL; > > +} > > +#endif > > This is obviously not good for users, they'd still have to do error > checking to determine if the device was created or not and then manually > register the device with the driver core and ideally also care if that > worked or not. I'm not sure something like this will really save enough > unless the device actually gets registered by the function, otherwise > it's going to be converting data to code. > > I'd also drop the microvolts and name parameters, Ok, I tried to do this. I Changed the function to: static struct platform_device *regulator_register_fixed(int id, struct regulator_consumer_supply *supplies, int num_supplies) Now I have to register a fixed regulator with some invented value for microvolts. What value should I choose? I tried 0 but the core won't accept this as explained earlier, in short regulator_register() returns with an error. Sascha
On Thu, Mar 01, 2012 at 11:08:47AM +0100, Sascha Hauer wrote: > Ok, I tried to do this. I Changed the function to: > static struct platform_device *regulator_register_fixed(int id, > struct regulator_consumer_supply *supplies, int num_supplies) > Now I have to register a fixed regulator with some invented value > for microvolts. What value should I choose? I tried 0 but the core > won't accept this as explained earlier, in short regulator_register() > returns with an error. I've already suggested just fixing the fixed voltage regulator to be more sensible when it doesn't have a voltage but you say that won't work without explaining why you think that is the case and are still ignoring my repeated queries about that.
On Thu, Mar 01, 2012 at 10:52:40AM +0000, Mark Brown wrote: > On Thu, Mar 01, 2012 at 11:08:47AM +0100, Sascha Hauer wrote: > > > Ok, I tried to do this. I Changed the function to: > > > static struct platform_device *regulator_register_fixed(int id, > > struct regulator_consumer_supply *supplies, int num_supplies) > > > Now I have to register a fixed regulator with some invented value > > for microvolts. What value should I choose? I tried 0 but the core > > won't accept this as explained earlier, in short regulator_register() > > returns with an error. > > I've already suggested just fixing the fixed voltage regulator to be > more sensible when it doesn't have a voltage but you say that won't work > without explaining why you think that is the case and are still ignoring > my repeated queries about that. Maybe because I don't understand what you mean. What should the fixed regulator do when voltage == 0? Sascha
On Thu, Mar 01, 2012 at 07:20:25PM +0100, Sascha Hauer wrote: > On Thu, Mar 01, 2012 at 10:52:40AM +0000, Mark Brown wrote: > > I've already suggested just fixing the fixed voltage regulator to be > > more sensible when it doesn't have a voltage but you say that won't work > > without explaining why you think that is the case and are still ignoring > > my repeated queries about that. > Maybe because I don't understand what you mean. What should the fixed > regulator do when voltage == 0? Not report that it has a voltage so report an error when asked and not claim you can enumerate them.
On Thu, Mar 01, 2012 at 06:21:39PM +0000, Mark Brown wrote: > On Thu, Mar 01, 2012 at 07:20:25PM +0100, Sascha Hauer wrote: > > On Thu, Mar 01, 2012 at 10:52:40AM +0000, Mark Brown wrote: > > > > I've already suggested just fixing the fixed voltage regulator to be > > > more sensible when it doesn't have a voltage but you say that won't work > > > without explaining why you think that is the case and are still ignoring > > > my repeated queries about that. > > > Maybe because I don't understand what you mean. What should the fixed > > regulator do when voltage == 0? > > Not report that it has a voltage so report an error when asked and not > claim you can enumerate them. Now I got it. That was a loooong way ;) Thanks Sascha
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig index 3a4fb2e..0b437d1 100644 --- a/arch/arm/configs/imx_v6_v7_defconfig +++ b/arch/arm/configs/imx_v6_v7_defconfig @@ -126,6 +126,7 @@ CONFIG_WATCHDOG=y CONFIG_IMX2_WDT=y CONFIG_MFD_MC13XXX=y CONFIG_REGULATOR=y +CONFIG_REGULATOR_FIXED_VOLTAGE=y CONFIG_REGULATOR_MC13892=y CONFIG_USB=y CONFIG_USB_EHCI_HCD=y diff --git a/arch/arm/plat-mxc/3ds_debugboard.c b/arch/arm/plat-mxc/3ds_debugboard.c index f0ba072..37c9160 100644 --- a/arch/arm/plat-mxc/3ds_debugboard.c +++ b/arch/arm/plat-mxc/3ds_debugboard.c @@ -16,6 +16,8 @@ #include <linux/platform_device.h> #include <linux/gpio.h> #include <linux/smsc911x.h> +#include <linux/regulator/machine.h> +#include <linux/regulator/fixed.h> #include <mach/hardware.h> @@ -61,6 +63,44 @@ static void __iomem *brd_io; +#if defined(CONFIG_REGULATOR_FIXED_VOLTAGE) || defined(CONFIG_REGULATOR_FIXED_VOLTAGE_MODULE) +static struct regulator_consumer_supply debugboard_smsc_supplies[] = { + REGULATOR_SUPPLY("vdd33a", NULL), + REGULATOR_SUPPLY("vddvario", NULL), +}; + +static struct regulator_init_data debugboard_smsc_init_data = { + .constraints = { + .name = "3V3", + .always_on = 1, + }, + .consumer_supplies = debugboard_smsc_supplies, + .num_consumer_supplies = ARRAY_SIZE(debugboard_smsc_supplies), +}; + +static struct fixed_voltage_config debugboard_smsc_pdata = { + .supply_name = "board-3V3", + .microvolts = 3300000, + .gpio = -EINVAL, + .enabled_at_boot = 1, + .init_data = &debugboard_smsc_init_data, +}; +static struct platform_device debugboard_voltage_regulator = { + .name = "reg-fixed-voltage", + .id = -1, + .num_resources = 0, + .dev = { + .platform_data = &debugboard_smsc_pdata, + }, +}; +static void __init debugboard_add_regulators(void) +{ + platform_device_register(&debugboard_voltage_regulator); +} +#else +static void __init debugboard_add_regulators(void) {} +#endif + static struct resource smsc911x_resources[] = { { .flags = IORESOURCE_MEM, @@ -187,6 +227,8 @@ int __init mxc_expio_init(u32 base, u32 p_irq) irq_set_irq_type(p_irq, IRQF_TRIGGER_LOW); irq_set_chained_handler(p_irq, mxc_expio_irq_handler); + debugboard_add_regulators(); + /* Register Lan device on the debugboard */ smsc911x_resources[0].start = LAN9217_BASE_ADDR(base); smsc911x_resources[0].end = LAN9217_BASE_ADDR(base) + 0x100 - 1;
commit c7e963f6 (net/smsc911x: Add regulator support) requires that regulator are provided to smsc911x chip. Provide regulator support to 3ds_debugboard so that mx31pdk/mx27pdk boards can have ethernet working again. Tested on a mx31pdk board. Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> --- Sascha, If you think this patch is OK, then I can submit patches for fixing the other i.MX boards that have smsc911x. arch/arm/configs/imx_v6_v7_defconfig | 1 + arch/arm/plat-mxc/3ds_debugboard.c | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 0 deletions(-)