[PATCH 2/2] net/smsc911x: Add regulator support

[PATCH 2/2] net/smsc911x: Add regulator support

[Linus Walleij]

From: Robert Marklund <robert.marklund@stericsson.com>

Add some basic regulator support for the power pins, as needed
by the ST-Ericsson Snowball platform that powers up the SMSC911
chip using an external regulator.

Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Signed-off-by: Robert Marklund <robert.marklund@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/smsc/smsc911x.c |  128 +++++++++++++++++++++++++++++++---
 1 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 8843071..fca01eb 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -44,6 +44,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sched.h>
 #include <linux/timer.h>
 #include <linux/bug.h>
@@ -138,6 +139,10 @@ struct smsc911x_data {
 
 	/* register access functions */
 	const struct smsc911x_ops *ops;
+
+	/* regulators */
+	struct regulator *regulator_vddvario;
+	struct regulator *regulator_vdd33a;
 };
 
 /* Easy access to information */
@@ -362,6 +367,86 @@ out:
 	spin_unlock_irqrestore(&pdata->dev_lock, flags);
 }
 
+/*
+ * Enable or disable resources, currently just regulators.
+ */
+static int smsc911x_enable_disable_resources(struct platform_device *pdev,
+					     bool enable)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct smsc911x_data *pdata = netdev_priv(ndev);
+	int err = 0;
+
+	/* enable/disable regulator for vddvario */
+	if (pdata->regulator_vddvario) {
+		if (enable) {
+			err = regulator_enable(pdata->regulator_vddvario);
+			if (err < 0) {
+			  netdev_err(ndev, "regulator_enable failed for "
+				     "vddvario");
+			}
+		} else
+			err = regulator_disable(pdata->regulator_vddvario);
+	}
+
+	/* enable/disable regulator for vdd33a */
+	if (pdata->regulator_vdd33a) {
+		if (enable) {
+			err = regulator_enable(pdata->regulator_vdd33a);
+			if (err < 0) {
+				netdev_err(ndev, "regulator_enable failed for "
+					   "vdd33a");
+			}
+		} else
+			err = regulator_disable(pdata->regulator_vdd33a);
+	}
+	return err;
+}
+
+/*
+ * Request or free resources, currently just regulators.
+ *
+ * The SMSC911x has two power pins: vddvario and vdd33a, in designs where
+ * these are not always-on we need to request regulators to be turned on
+ * before we can try to access the device registers.
+ */
+static int smsc911x_request_free_resources(struct platform_device *pdev,
+		bool request)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct smsc911x_data *pdata = netdev_priv(ndev);
+	int err = 0;
+
+	/* Request regulator for vddvario */
+	if (request && !pdata->regulator_vddvario) {
+		pdata->regulator_vddvario = regulator_get(&pdev->dev,
+				"vddvario");
+		if (IS_ERR(pdata->regulator_vddvario)) {
+			netdev_err(ndev, "Failed to get regulator vddvario\n");
+			err = PTR_ERR(pdata->regulator_vddvario);
+			pdata->regulator_vddvario = NULL;
+		}
+	} else if (!request && pdata->regulator_vddvario) {
+		regulator_put(pdata->regulator_vddvario);
+		pdata->regulator_vddvario = NULL;
+	}
+
+	/* Request regulator for vdd33a */
+	if (request && !pdata->regulator_vdd33a) {
+		pdata->regulator_vdd33a = regulator_get(&pdev->dev, "vdd33a");
+		if (IS_ERR(pdata->regulator_vdd33a)) {
+			netdev_err(ndev, "Failed to get regulator vdd33a\n");
+			err = PTR_ERR(pdata->regulator_vdd33a);
+			pdata->regulator_vdd33a = NULL;
+		}
+	} else if (!request && pdata->regulator_vdd33a) {
+		regulator_put(pdata->regulator_vdd33a);
+		pdata->regulator_vdd33a = NULL;
+	}
+
+	return err;
+}
+
 /* waits for MAC not busy, with timeout.  Only called by smsc911x_mac_read
  * and smsc911x_mac_write, so assumes mac_lock is held */
 static int smsc911x_mac_complete(struct smsc911x_data *pdata)
@@ -2065,6 +2150,7 @@ static int __devexit smsc911x_drv_remove(struct platform_device *pdev)
 	struct net_device *dev;
 	struct smsc911x_data *pdata;
 	struct resource *res;
+	int retval;
 
 	dev = platform_get_drvdata(pdev);
 	BUG_ON(!dev);
@@ -2092,6 +2178,12 @@ static int __devexit smsc911x_drv_remove(struct platform_device *pdev)
 
 	iounmap(pdata->ioaddr);
 
+	if (smsc911x_enable_disable_resources(pdev, false))
+		pr_warn("Could not disable resource\n");
+
+	retval = smsc911x_request_free_resources(pdev, false);
+	/* ignore not all have regulators */
+
 	free_netdev(dev);
 
 	return 0;
@@ -2218,10 +2310,24 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 	pdata->dev = dev;
 	pdata->msg_enable = ((1 << debug) - 1);
 
+	platform_set_drvdata(pdev, dev);
+
+	retval = smsc911x_request_free_resources(pdev, true);
+	if (retval) {
+		pr_err("Could request regulators needed aborting\n");
+		goto out_return_resources;
+	}
+
+	retval = smsc911x_enable_disable_resources(pdev, true);
+	if (retval) {
+		pr_err("Could enable regulators needed aborting\n");
+		goto out_disable_resources;
+	}
+
 	if (pdata->ioaddr == NULL) {
 		SMSC_WARN(pdata, probe, "Error smsc911x base address invalid");
 		retval = -ENOMEM;
-		goto out_free_netdev_2;
+		goto out_disable_resources;
 	}
 
 	retval = smsc911x_probe_config_dt(&pdata->config, np);
@@ -2233,7 +2339,7 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 
 	if (retval) {
 		SMSC_WARN(pdata, probe, "Error smsc911x config not found");
-		goto out_unmap_io_3;
+		goto out_disable_resources;
 	}
 
 	/* assume standard, non-shifted, access to HW registers */
@@ -2244,7 +2350,7 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 
 	retval = smsc911x_init(dev);
 	if (retval < 0)
-		goto out_unmap_io_3;
+		goto out_disable_resources;
 
 	/* configure irq polarity and type before connecting isr */
 	if (pdata->config.irq_polarity == SMSC911X_IRQ_POLARITY_ACTIVE_HIGH)
@@ -2264,15 +2370,13 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 	if (retval) {
 		SMSC_WARN(pdata, probe,
 			  "Unable to claim requested irq: %d", dev->irq);
-		goto out_unmap_io_3;
+		goto out_free_irq;
 	}
 
-	platform_set_drvdata(pdev, dev);
-
 	retval = register_netdev(dev);
 	if (retval) {
 		SMSC_WARN(pdata, probe, "Error %i registering device", retval);
-		goto out_unset_drvdata_4;
+		goto out_free_irq;
 	} else {
 		SMSC_TRACE(pdata, probe,
 			   "Network interface: \"%s\"", dev->name);
@@ -2321,12 +2425,14 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 
 out_unregister_netdev_5:
 	unregister_netdev(dev);
-out_unset_drvdata_4:
-	platform_set_drvdata(pdev, NULL);
+out_free_irq:
 	free_irq(dev->irq, dev);
-out_unmap_io_3:
+out_disable_resources:
+	(void)smsc911x_enable_disable_resources(pdev, false);
+out_return_resources:
+	(void)smsc911x_request_free_resources(pdev, false);
+	platform_set_drvdata(pdev, NULL);
 	iounmap(pdata->ioaddr);
-out_free_netdev_2:
 	free_netdev(dev);
 out_release_io_1:
 	release_mem_region(res->start, resource_size(res));
-- 
1.7.3.2

Re: [PATCH 2/2] net/smsc911x: Add regulator support

[Mark Brown]

On Wed, Oct 26, 2011 at 10:05:56AM +0200, Linus Walleij wrote:

> +	/* enable/disable regulator for vddvario */
> +	if (pdata->regulator_vddvario) {

This has the same issue as last time - if you've got conditional code
like this in the body of the driver something is going wrong.  Unless
the supply is genuinely optional and might not be physically present on
some systems the driver should fail if it can't get it.  The regulator
API will stub itself out when not in use.

Re: [PATCH 2/2] net/smsc911x: Add regulator support

[Linus Walleij]

On 10/26/2011 10:38 AM, Mark Brown wrote:
> On Wed, Oct 26, 2011 at 10:05:56AM +0200, Linus Walleij wrote:
>
>    
>> +	/* enable/disable regulator for vddvario */
>> +	if (pdata->regulator_vddvario) {
>>      
> This has the same issue as last time - if you've got conditional code
> like this in the body of the driver something is going wrong.  Unless
> the supply is genuinely optional and might not be physically present on
> some systems the driver should fail if it can't get it.  The regulator
> API will stub itself out when not in use.
>    

That solves the issue for platforms with no regulator
support at all.

Then we have platforms with regulator support, but no
regulator for this hardware, because that one happens
to be always-on in these systems.

And they do not have CONFIG_REGULATOR_DUMMY
either.

So the driver probe will fail.

How do we solve this?

Shall we have CONFIG_SMC911X select
REGULATOR_DUMMY?

Yours,
Linus Walleij

Re: [PATCH 2/2] net/smsc911x: Add regulator support

[Mark Brown]

On Wed, Oct 26, 2011 at 11:25:09AM +0200, Linus Walleij wrote:
> On 10/26/2011 10:38 AM, Mark Brown wrote:

> >This has the same issue as last time - if you've got conditional code
> >like this in the body of the driver something is going wrong.  Unless
> >the supply is genuinely optional and might not be physically present on
> >some systems the driver should fail if it can't get it.  The regulator
> >API will stub itself out when not in use.

> That solves the issue for platforms with no regulator
> support at all.

No, it solves the problem for all platforms.

> Then we have platforms with regulator support, but no
> regulator for this hardware, because that one happens
> to be always-on in these systems.

Right, this is extremely common and is exactly what the fixed voltage
regulator is there for - if you've got an always on regulator in your
system define a fixed voltage regulator to represent it.

> Shall we have CONFIG_SMC911X select
> REGULATOR_DUMMY?

No, nothing should be selecting that.  Users can enable it for their
systems if they want it.

Re: [PATCH 2/2] net/smsc911x: Add regulator support

[Linus Walleij]

On 10/26/2011 12:17 PM, Mark Brown wrote:
>> Then we have platforms with regulator support, but no
>> regulator for this hardware, because that one happens
>> to be always-on in these systems.
>>      
> Right, this is extremely common and is exactly what the fixed voltage
> regulator is there for - if you've got an always on regulator in your
> system define a fixed voltage regulator to represent it.
>    

Aha you mean I need to go into the platform/board files
for all systems using the smsc911x and add a fixed voltage
regulator?

OK that can't be too hard...

Yours,
Linus Walleij

RE: [PATCH 2/2] net/smsc911x: Add regulator support

[Robert MARKLUND]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: den 26 oktober 2011 10:39
> To: Linus WALLEIJ
> Cc: netdev@vger.kernel.org; Steve Glendinning; Mathieu Poirer; Robert MARKLUND; Linus Walleij
> Subject: Re: [PATCH 2/2] net/smsc911x: Add regulator support
> 
> On Wed, Oct 26, 2011 at 10:05:56AM +0200, Linus Walleij wrote:
> 
> > +	/* enable/disable regulator for vddvario */
> > +	if (pdata->regulator_vddvario) {
> 
> This has the same issue as last time - if you've got conditional code
> like this in the body of the driver something is going wrong.  Unless
> the supply is genuinely optional and might not be physically present on
> some systems the driver should fail if it can't get it.  The regulator
> API will stub itself out when not in use.

My mistake I didn't remember to remove them.
If you review the rest of the code you see that the driver will not start if we can't get the regulators.
So that code will never run in the case where pdata->regulator_vddvario == 0.

So this will leave all the platforms using this driver and have full constraints to fail to start this driver.
If they don't create dummy regulators.

Just so we are on the clear with that.

/R

Re: [PATCH 2/2] net/smsc911x: Add regulator support

[Mark Brown]

On Wed, Oct 26, 2011 at 12:44:06PM +0200, Linus Walleij wrote:

> Aha you mean I need to go into the platform/board files
> for all systems using the smsc911x and add a fixed voltage
> regulator?

> OK that can't be too hard...

Either that or just ping all the relevant developers and let the know;
half the point of the config option is so that if something goes wrong
it's easy to get the system running.