ARM: 3ds_debugboard: Let ethernet be functional again

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 <[email protected]>

---
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(-)

arch/arm/configs/imx_v6_v7_defconfig +1 -0

index 3a4fb2e..0b437d1 100644
@@ -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

arch/arm/plat-mxc/3ds_debugboard.c +29 -0

index f0ba072..37c9160 100644
@@ -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		= {

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.

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.

Quoted 4 lines · Sascha Hauer

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.

Quoted 3 lines · Mark Brown

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.

Quoted 8 lines · Sascha Hauer

Quoted 3 lines · Mark Brown

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.

Quoted 2 lines · Mark Brown

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

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.

Quoted 8 lines · Sascha Hauer

Quoted 2 lines · Mark Brown

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.

Quoted 2 lines

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.

Quoted 10 lines

include/linux/regulator/fixed.h +9 -0

+#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)
+{

include/linux/regulator/fixed.h struct fixed_voltage_config { · line 64

Quoted 3 lines

include/linux/regulator/fixed.h +3 -0

+	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...

Quoted 2 lines · Mark Brown

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)

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 4 lines · Sascha Hauer

Quoted 2 lines · Mark Brown

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

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

Quoted 2 lines · Mark Brown

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 8 lines · Sascha Hauer

Quoted 2 lines · Mark Brown

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.

Quoted 2 lines

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.

include/linux/regulator/fixed.h struct fixed_voltage_config { · line 64

Quoted 12 lines

include/linux/regulator/fixed.h +12 -0

+#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)

Quoted 6 lines · Sascha Hauer

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.

	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;

		/* maybe adjust [min_uV..max_uV] */
		if (value >= cmin && value < min_uV)
			min_uV = value;
		if (value <= cmax && value > max_uV)
			max_uV = value;

	}

	/* final: [min_uV..max_uV] valid iff constraints valid */
	if (max_uV < min_uV) {
		rdev_err(rdev, "unsupportable voltage constraints\n");
		return -EINVAL;
	}

I don't know what to do here. Probably the check for value <= 0 should be removed, but then again the description shows:

Quoted 2 lines · Sascha Hauer

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.

Quoted 5 lines · Mark Brown

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.

Quoted 7 lines · Sascha Hauer

Quoted 5 lines · Mark Brown

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.

Quoted 3 lines

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?

Quoted 1 line · Mark Brown

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.

Quoted 8 lines · Sascha Hauer

Quoted 2 lines · Mark Brown

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.

Quoted 2 lines

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.

include/linux/regulator/fixed.h struct fixed_voltage_config { · line 64

Quoted 12 lines

include/linux/regulator/fixed.h +12 -0

+#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.

Quoted 9 lines · Sascha Hauer

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.

Quoted 4 lines · Mark Brown

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?

Quoted 6 lines · Sascha Hauer

Quoted 4 lines · Mark Brown

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.

Quoted 2 lines

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.