[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Fabio Estevam]

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
+
 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;
-- 
1.7.1

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Fabio Estevam]

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

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120213/299be1a2/attachment-0001.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120214/a0696ce7/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

On Tue, Feb 14, 2012 at 09:29:04AM -0800, Mark Brown wrote:
> 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.

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.

The following could be an example how to add such a helper.

Sascha

8<-------------------------------------------------

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/Makefile       |    2 +-
 drivers/regulator/fixed-helper.c |   59 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/fixed.h  |   15 +++++++++
 3 files changed, 75 insertions(+), 1 deletions(-)
 create mode 100644 drivers/regulator/fixed-helper.c

diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 503bac8..f76deb9 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -3,7 +3,7 @@
 #
 
 
-obj-$(CONFIG_REGULATOR) += core.o dummy.o
+obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o
 obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
diff --git a/drivers/regulator/fixed-helper.c b/drivers/regulator/fixed-helper.c
new file mode 100644
index 0000000..8c601a9
--- /dev/null
+++ b/drivers/regulator/fixed-helper.c
@@ -0,0 +1,59 @@
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/fixed.h>
+
+struct fixed_regulator_data {
+	struct fixed_voltage_config cfg;
+	struct regulator_init_data init_data;
+	struct platform_device pdev;
+};
+
+static void regulator_fixed_release(struct device *dev)
+{
+	struct fixed_regulator_data *data = container_of(dev,
+			struct fixed_regulator_data, pdev.dev);
+	kfree(data);
+}
+
+/**
+ * regulator_register_fixed - register a no-op fixed regulator
+ * @name: supply name
+ * @id: platform device id
+ * @microvolts: voltage
+ * @supplies: consumers for this regulator
+ * @num_supplies: number of consumers
+ */
+struct platform_device *regulator_register_fixed(const char *name, int id,
+		int microvolts,
+		struct regulator_consumer_supply *supplies,
+		int num_supplies)
+{
+	struct fixed_regulator_data *data;
+	int size;
+
+	size = sizeof(*data);
+
+	data = kzalloc(size, GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	data->cfg.supply_name = name;
+	data->cfg.microvolts = microvolts;
+	data->cfg.gpio = -EINVAL;
+	data->cfg.enabled_at_boot = 1;
+	data->cfg.init_data = &data->init_data;
+
+	data->init_data.constraints.always_on = 1;
+	data->init_data.consumer_supplies = supplies;
+	data->init_data.num_consumer_supplies = num_supplies;
+
+	data->pdev.name = "reg-fixed-voltage";
+	data->pdev.id = id;
+	data->pdev.dev.platform_data = &data->cfg;
+	data->pdev.dev.release = regulator_fixed_release;
+
+	platform_device_register(&data->pdev);
+
+	return &data->pdev;
+}
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index ffd7d50..57289c0 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -48,4 +48,19 @@ struct fixed_voltage_config {
 	struct regulator_init_data *init_data;
 };
 
+struct regulator_consumer_supply;
+
+#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
+
 #endif
-- 
1.7.9



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120215/ee6153c8/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120216/37a0292b/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120217/e681d441/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120227/2d1c2e04/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120227/e08e3b52/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120228/7a5d4350/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120229/4cde0fad/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120301/17298457/attachment.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120301/2d078f19/attachment-0001.sig>

[PATCH] ARM: 3ds_debugboard: Let ethernet be functional again

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |