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

[PATCH 2/2 v3] 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.

Platforms that use regulators and the smsc911x and have no defined
regulator for the smsc911x and claim complete regulator
constraints with no dummy regulators will need to provide it, for
example using a fixed voltage regulator. It appears that this may
affect (apart from Ux500 Snowball) possibly these archs/machines
that from some grep:s appear to define both CONFIG_SMSC911X and
CONFIG_REGULATOR:

- ARM Freescale mx3 and OMAP 2 plus, Raumfeld machines
- Blackfin
- Super-H

Cc: Paul Mundt <lethal@linux-sh.org>
Cc: linux-sh@vger.kernel.org
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Tony Lindgren <tony@atomide.com>
Cc: linux-omap@vger.kernel.org
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: uclinux-dist-devel@blackfin.uclinux.org
Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Signed-off-by: Robert Marklund <robert.marklund@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Use bulk regulators on Mark's request.
- Add Cc-fileds to some possibly affected platforms.
ChangeLog v1->v2:
- Don't check for NULL regulators and error out properly if the
  regulators can't be found. All platforms using the smsc911x
  and the regulator framework simultaneously need to provide some
  kind of regulator for it.
---
 drivers/net/ethernet/smsc/smsc911x.c |  103 ++++++++++++++++++++++++++++++----
 1 files changed, 92 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 8843071..8ad15a6 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>
@@ -88,6 +89,8 @@ struct smsc911x_ops {
 				unsigned int *buf, unsigned int wordcount);
 };
 
+#define SMSC911X_NUM_SUPPLIES 2
+
 struct smsc911x_data {
 	void __iomem *ioaddr;
 
@@ -138,6 +141,9 @@ struct smsc911x_data {
 
 	/* register access functions */
 	const struct smsc911x_ops *ops;
+
+	/* regulators */
+	struct regulator_bulk_data supplies[SMSC911X_NUM_SUPPLIES];
 };
 
 /* Easy access to information */
@@ -362,6 +368,60 @@ 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 ret = 0;
+
+	/* enable/disable regulators */
+	if (enable) {
+		ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
+				pdata->supplies);
+		if (ret)
+			netdev_err(ndev, "failed to enable regulators %d\n",
+					ret);
+	} else
+		ret = regulator_bulk_disable(ARRAY_SIZE(pdata->supplies),
+				pdata->supplies);
+	return ret;
+}
+
+/*
+ * 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 ret = 0;
+
+	/* Request regulators */
+	if (request) {
+		pdata->supplies[0].supply = "vdd33a";
+		pdata->supplies[1].supply = "vddvario";
+		ret = regulator_bulk_get(&pdev->dev,
+				ARRAY_SIZE(pdata->supplies),
+				pdata->supplies);
+		if (ret)
+			netdev_err(ndev, "couldn't get regulators %d\n",
+					ret);
+	} else
+		regulator_bulk_free(ARRAY_SIZE(pdata->supplies),
+				pdata->supplies);
+
+	return ret;
+}
+
 /* 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 +2125,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 +2153,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 +2285,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 +2314,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 +2325,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 +2345,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 +2400,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 v3] net/smsc911x: Add regulator support

[Mike Frysinger]

On Thu, Oct 27, 2011 at 14:48, Linus Walleij wrote:
> Platforms that use regulators and the smsc911x and have no defined
> regulator for the smsc911x and claim complete regulator
> constraints with no dummy regulators will need to provide it, for
> example using a fixed voltage regulator. It appears that this may
> affect (apart from Ux500 Snowball) possibly these archs/machines
> that from some grep:s appear to define both CONFIG_SMSC911X and
> CONFIG_REGULATOR:
>
> - ARM Freescale mx3 and OMAP 2 plus, Raumfeld machines
> - Blackfin
> - Super-H

no Blackfin board in the tree uses regulators by default.  we do list
regulator resources with some boards so people can rebuild the
development system to support addon daughter cards.

my gut reaction: smsc911x is working just fine without regulator
support for many people, so why do we suddenly need to make it a
requirement ?  this is a fairly small amount of code, so adding a
smsc911x Kconfig symbol to control the regulator support seems like
overkill.  only other option would be to change the patch to not make
missing regulators non-fatal.  so i'd probably lean towards the latter
(and it sounds like you changed this with earlier versions).

> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>
> +#define SMSC911X_NUM_SUPPLIES 2

this gets used once (to define the array), so i wonder if it shouldn't
just be inlined

> +static int smsc911x_enable_disable_resources(struct platform_device *pdev,
> +                                            bool enable)
> +{
> ...
> +       if (enable) {
> +               ret = regulator_bulk_enable(ARRAY_SIZE(pdata->supplies),
> +                               pdata->supplies);
> +               if (ret)
> +                       netdev_err(ndev, "failed to enable regulators %d\n",
> +                                       ret);
> ...
> static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> ...
> +       retval = smsc911x_request_free_resources(pdev, true);
> +       if (retval) {
> +               pr_err("Could request regulators needed aborting\n");

you warn twice here, and the grammar in the later error is broken, and
uses pr_err() instead of netdev_err().  i would simply drop the latter
pr_err().

> +static int smsc911x_request_free_resources(struct platform_device *pdev,
> +               bool request)
> +{
> ...
> +       if (request) {
> +               ret = regulator_bulk_get(&pdev->dev,
> +                               ARRAY_SIZE(pdata->supplies),
> +                               pdata->supplies);
> +               if (ret)
> +                       netdev_err(ndev, "couldn't get regulators %d\n",
> +                                       ret);
> static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> ...
> +       retval = smsc911x_enable_disable_resources(pdev, true);
> +       if (retval) {
> +               pr_err("Could enable regulators needed aborting\n");

same exact issues with the request/free helper

> +       retval = smsc911x_request_free_resources(pdev, false);
> +       /* ignore not all have regulators */

old comment ?
-mike

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

[Mark Brown]

On Thu, Oct 27, 2011 at 03:21:47PM +0200, Mike Frysinger wrote:

> my gut reaction: smsc911x is working just fine without regulator
> support for many people, so why do we suddenly need to make it a
> requirement ?  this is a fairly small amount of code, so adding a
> smsc911x Kconfig symbol to control the regulator support seems like
> overkill.  only other option would be to change the patch to not make
> missing regulators non-fatal.  so i'd probably lean towards the latter
> (and it sounds like you changed this with earlier versions).

The regulator API contains a series of generic facilities for stubbing
itself out when not in use - there's no need for individual drivers to
worry about this stuff, they should just rely on the framework.  The
main one at the minute is REGULATOR_DUMMY which does what you suggest
and makes regulator_get() never fail.

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

[Mike Frysinger]

On Thu, Oct 27, 2011 at 17:46, Mark Brown wrote:
> On Thu, Oct 27, 2011 at 03:21:47PM +0200, Mike Frysinger wrote:
>> my gut reaction: smsc911x is working just fine without regulator
>> support for many people, so why do we suddenly need to make it a
>> requirement ?  this is a fairly small amount of code, so adding a
>> smsc911x Kconfig symbol to control the regulator support seems like
>> overkill.  only other option would be to change the patch to not make
>> missing regulators non-fatal.  so i'd probably lean towards the latter
>> (and it sounds like you changed this with earlier versions).
>
> The regulator API contains a series of generic facilities for stubbing
> itself out when not in use - there's no need for individual drivers to
> worry about this stuff, they should just rely on the framework.  The
> main one at the minute is REGULATOR_DUMMY which does what you suggest
> and makes regulator_get() never fail.

i saw that !CONFIG_REGULATOR works great.  my concern is that these
boards don't define any regulators for smsc resources, so if
CONFIG_REGULATOR is enabled to test out unrelated daughter cards, i
don't want the network driver suddenly failing.  Linus' comments
suggest that this is what would happen unless each board file has its
smsc platform resources extended.  maybe i misunderstood what he was saying ?
-mike

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

[Mark Brown]

On Thu, Oct 27, 2011 at 10:59:14PM +0200, Mike Frysinger wrote:

> i saw that !CONFIG_REGULATOR works great.  my concern is that these
> boards don't define any regulators for smsc resources, so if
> CONFIG_REGULATOR is enabled to test out unrelated daughter cards, i
> don't want the network driver suddenly failing.  Linus' comments
> suggest that this is what would happen unless each board file has its
> smsc platform resources extended.  maybe i misunderstood what he was saying ?

That's the case that's handled by CONFIG_REGULATOR_DUMMY - if the lookup
fails the core will provide a virtual regulator to the consumer.  If
you're running a setup like that you probably want to enable dummy
regulators anyway.

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

[Mike Frysinger]

On Thu, Oct 27, 2011 at 23:42, Mark Brown wrote:
> On Thu, Oct 27, 2011 at 10:59:14PM +0200, Mike Frysinger wrote:
>> i saw that !CONFIG_REGULATOR works great.  my concern is that these
>> boards don't define any regulators for smsc resources, so if
>> CONFIG_REGULATOR is enabled to test out unrelated daughter cards, i
>> don't want the network driver suddenly failing.  Linus' comments
>> suggest that this is what would happen unless each board file has its
>> smsc platform resources extended.  maybe i misunderstood what he was saying ?
>
> That's the case that's handled by CONFIG_REGULATOR_DUMMY - if the lookup
> fails the core will provide a virtual regulator to the consumer.  If
> you're running a setup like that you probably want to enable dummy
> regulators anyway.

ok, if people configuring need only select that Kconfig symbol to make
it work, i'm fine with things.  thanks for the explanation.
-mike

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

[Sascha Hauer]

Hi Linus,

On Thu, Oct 27, 2011 at 02:48:11PM +0200, Linus Walleij wrote:
> 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.
> 
> Platforms that use regulators and the smsc911x and have no defined
> regulator for the smsc911x and claim complete regulator
> constraints with no dummy regulators will need to provide it, for
> example using a fixed voltage regulator. It appears that this may
> affect (apart from Ux500 Snowball) possibly these archs/machines
> that from some grep:s appear to define both CONFIG_SMSC911X and
> CONFIG_REGULATOR:
> 
> - ARM Freescale mx3 and OMAP 2 plus, Raumfeld machines
> - Blackfin
> - Super-H
> 

...

>  
> +
> +/*
> + * 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)

I had to look twice at this function name. First I thought "request the
free resources?", which other resources would you request if not the
free ones? I think it would be nicer to have two functions instead.
Just my 2 cents.

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 |

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

[Mike Frysinger]

On Fri, Oct 28, 2011 at 22:33, Sascha Hauer wrote:
> On Thu, Oct 27, 2011 at 02:48:11PM +0200, Linus Walleij wrote:
>> +/*
>> + * 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)
>
> I had to look twice at this function name. First I thought "request the
> free resources?", which other resources would you request if not the
> free ones? I think it would be nicer to have two functions instead.
> Just my 2 cents.

i'll add my 2 cents and we'll almost have a nickle.  maybe i'm dense,
but i had to look (more than) twice at both funcs before i could get
my head around what was happening.  no, it's not complicated, but it
is unusual in the kernel world.  either that or i haven't read enough
kernel code to consider this a common paradigm.  hopefully it's the
former ;).
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html