| Message ID | 1318834597-3479-1-git-send-email-robert.marklund@stericsson.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Oct 17, 2011 at 08:56:37AM +0200, Robert Marklund wrote: > + /* Request regulator for vddvario */ > + if (request && !pdata->regulator_vddvario) { > + pdata->regulator_vddvario = regulator_get(&pdev->dev, > + "vddvario"); > + if (IS_ERR(pdata->regulator_vddvario)) { > + netdev_warn(ndev, > + "%s: Failed to get regulator '%s'\n", > + __func__, "vddvario"); > + pdata->regulator_vddvario = NULL; > + } No, this is broken - look at how other devices use the regulator API. The driver should just request and use the regulators unconditionally and let the stubbing and mapping facilities the API has deal with ensuring that they always succeed. As a side note the use of "pdata" as a name for the driver internal data is really not helpful, pdata is traditionally the platform data passed in by the machine (which would be even more broken). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: den 17 oktober 2011 12:53 > To: Robert MARKLUND > Cc: netdev@vger.kernel.org; Steve Glendinning; Mathieu Poirer > Subject: Re: [PATCH] smsc911x: Add regulator support > > On Mon, Oct 17, 2011 at 08:56:37AM +0200, Robert Marklund wrote: > > > + /* Request regulator for vddvario */ > > + if (request && !pdata->regulator_vddvario) { > > + pdata->regulator_vddvario = regulator_get(&pdev- > >dev, > > + "vddvario"); > > + if (IS_ERR(pdata->regulator_vddvario)) { > > + netdev_warn(ndev, > > + "%s: Failed to > get regulator '%s'\n", > > + __func__, > "vddvario"); > > + pdata->regulator_vddvario = NULL; > > + } > > No, this is broken - look at how other devices use the regulator API. > The driver should just request and use the regulators unconditionally > and let the stubbing and mapping facilities the API has deal with > ensuring that they always succeed. [Robert MARKLUND] So what you mean is get them and use them and ignore all the return codes, and let the FW take care of the error handling ? > > As a side note the use of "pdata" as a name for the driver internal > data > is really not helpful, pdata is traditionally the platform data passed > in by the machine (which would be even more broken). [Robert MARKLUND] In the driver they have used this name for this structure throughout the file I just followed that. Personally I think it will be more confusing to change the name of this structure in just this new function. /R -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 17, 2011 at 01:30:06PM +0200, Robert MARKLUND wrote: You should fix your mail client to word wrap within paragraphs, I've reformatted it for legibility. Also leave a blank line between paragraphs for the same reason. > > No, this is broken - look at how other devices use the regulator API. > > The driver should just request and use the regulators unconditionally > > and let the stubbing and mapping facilities the API has deal with > > ensuring that they always succeed. > So what you mean is get them and use them and ignore all the return > codes, and let the FW take care of the error handling ? No, you should do what all the other drivers do and actually pay attention to the errors. If we can't get power to the device that's a pretty serious problem and the driver ought to fail. > > As a side note the use of "pdata" as a name for the driver internal data > > is really not helpful, pdata is traditionally the platform data passed > > in by the machine (which would be even more broken). > In the driver they have used this name for this structure throughout > the file I just followed that. Personally I think it will be more > confusing to change the name of this structure in just this new > function. I think someone should send a patch renaming the data throughout the entire driver, it's a terrible name for an embedded context. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: den 17 oktober 2011 14:36 > To: Robert MARKLUND > Cc: netdev@vger.kernel.org; Steve Glendinning; Mathieu Poirer > Subject: Re: [PATCH] smsc911x: Add regulator support > > On Mon, Oct 17, 2011 at 01:30:06PM +0200, Robert MARKLUND wrote: > > You should fix your mail client to word wrap within paragraphs, I've > reformatted it for legibility. Also leave a blank line between > paragraphs for the same reason. I have tried but it's not as easy as it sounds :) Does it look better now ? > > > > No, this is broken - look at how other devices use the regulator API. > > > The driver should just request and use the regulators unconditionally > > > and let the stubbing and mapping facilities the API has deal with > > > ensuring that they always succeed. > > > So what you mean is get them and use them and ignore all the return > > codes, and let the FW take care of the error handling ? > > No, you should do what all the other drivers do and actually pay > attention to the errors. If we can't get power to the device that's a > pretty serious problem and the driver ought to fail. Then I don't understand the initial comment. Can you please elaborate that one. > > > > As a side note the use of "pdata" as a name for the driver internal data > > > is really not helpful, pdata is traditionally the platform data passed > > > in by the machine (which would be even more broken). > > > In the driver they have used this name for this structure throughout > > the file I just followed that. Personally I think it will be more > > confusing to change the name of this structure in just this new > > function. > > I think someone should send a patch renaming the data throughout the > entire driver, it's a terrible name for an embedded context. I agree let this someone do it. In the meanwhile isn't it best to keep the style that in the driver as of now. /R -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 17, 2011 at 04:13:47PM +0200, Robert MARKLUND wrote: > > You should fix your mail client to word wrap within paragraphs, I've > > reformatted it for legibility. Also leave a blank line between > > paragraphs for the same reason. > I have tried but it's not as easy as it sounds :) > Does it look better now ? Yes, thanks. > > No, you should do what all the other drivers do and actually pay > > attention to the errors. If we can't get power to the device that's a > > pretty serious problem and the driver ought to fail. > Then I don't understand the initial comment. > Can you please elaborate that one. If we fail to grab a critical resource for the device like the power supply then we should be failing passing the error back. As I keep saying this is what all the other regulator consumers are doing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: den 17 oktober 2011 16:34 > To: Robert MARKLUND > Cc: netdev@vger.kernel.org; Steve Glendinning; Mathieu Poirer > Subject: Re: [PATCH] smsc911x: Add regulator support > > On Mon, Oct 17, 2011 at 04:13:47PM +0200, Robert MARKLUND wrote: > > > > You should fix your mail client to word wrap within paragraphs, I've > > > reformatted it for legibility. Also leave a blank line between > > > paragraphs for the same reason. > > > I have tried but it's not as easy as it sounds :) > > Does it look better now ? > > Yes, thanks. > > > > No, you should do what all the other drivers do and actually pay > > > attention to the errors. If we can't get power to the device that's a > > > pretty serious problem and the driver ought to fail. > > > Then I don't understand the initial comment. > > Can you please elaborate that one. > > If we fail to grab a critical resource for the device like the power > supply then we should be failing passing the error back. As I keep > saying this is what all the other regulator consumers are doing. Most of the boards out there don't have any regulators for the chip(power is always on). So when we can't get the regulator, we assume that the power is already on. That's why it hasn't been implemented before. So adding this functionality will make all the other boards out there fail, to start the driver. /R -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 17, 2011 at 05:28:08PM +0200, Robert MARKLUND wrote: > > If we fail to grab a critical resource for the device like the power > > supply then we should be failing passing the error back. As I keep > > saying this is what all the other regulator consumers are doing. > Most of the boards out there don't have any regulators for the chip(power is always on). > So when we can't get the regulator, we assume that the power is already on. > That's why it hasn't been implemented before. > So adding this functionality will make all the other boards out there fail, to start the driver. Clearly this issue is going to apply to every single user of the regulator API which is one reason why it's insane to open code handling of missing regulators in individual regulator consumer drivers. As I keep saying you should unconditionally use the regulators and let the regulator API facilities for stubbing itself out deal with this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c index b9016a3..4de3bd8 100644 --- a/drivers/net/smsc911x.c +++ b/drivers/net/smsc911x.c @@ -53,6 +53,8 @@ #include <linux/phy.h> #include <linux/smsc911x.h> #include <linux/device.h> +#include <linux/regulator/consumer.h> + #include "smsc911x.h" #define SMSC_CHIPNAME "smsc911x" @@ -133,6 +135,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 */ @@ -357,6 +363,81 @@ out: spin_unlock_irqrestore(&pdata->dev_lock, flags); } +/* Enable resources(clocks and regulators) */ +static int smsc911x_enable_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/diable regulator for vddvario */ + if (pdata->regulator_vddvario) { + if (enable) { + err = regulator_enable(pdata->regulator_vddvario); + if (err < 0) { + netdev_err(ndev, "%s: regulator_enable failed '%s'\n", + __func__, "vddvario"); + } + } else + err = regulator_disable(pdata->regulator_vdd33a); + } + + /* enable/diableregulator for vdd33a */ + if (pdata->regulator_vdd33a) { + if (enable) { + err = regulator_enable(pdata->regulator_vdd33a); + if (err < 0) { + netdev_err(ndev, "%s: regulator_enable failed '%s'\n", + __func__, "vdd33a"); + } + } else + err = regulator_disable(pdata->regulator_vdd33a); + } + return err; +} + + +/* Request resources(clocks and regulators) */ +static int smsc911x_request_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_warn(ndev, + "%s: Failed to get regulator '%s'\n", + __func__, "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_vddvario) { + pdata->regulator_vdd33a = regulator_get(&pdev->dev, + "vdd33a"); + if (IS_ERR(pdata->regulator_vdd33a)) { + netdev_warn(ndev, + "%s: Failed to get regulator '%s'\n", + __func__, "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) @@ -2047,6 +2128,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); @@ -2074,6 +2156,12 @@ static int __devexit smsc911x_drv_remove(struct platform_device *pdev) iounmap(pdata->ioaddr); + if (smsc911x_enable_resources(pdev, false)) + pr_warn("Could not disable resource\n"); + + retval = smsc911x_request_resources(pdev, false); + /* ignore not all have regulators */ + free_netdev(dev); return 0; @@ -2104,6 +2192,7 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev) unsigned int intcfg = 0; int res_size, irq_flags; int retval; + int to = 100; pr_info("Driver version %s\n", SMSC_DRV_VERSION); @@ -2158,6 +2247,17 @@ 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_resources(pdev, true); + /* ignore not all have regulators */ + + retval = smsc911x_enable_resources(pdev, true); + if (retval) { + pr_warn("Could not enable resource\n"); + goto out_0; + } + if (pdata->ioaddr == NULL) { SMSC_WARN(pdata, probe, "Error smsc911x base address invalid"); retval = -ENOMEM; @@ -2170,6 +2270,18 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev) if (config->shift) pdata->ops = &shifted_smsc911x_ops; + /* poll the READY bit in PMT_CTRL. Any other access to the device is + * forbidden while this bit isn't set. Try for 100ms + */ + while (!(smsc911x_reg_read(pdata, PMT_CTRL) & PMT_CTRL_READY_) && --to) + udelay(1000); + + if (to == 0) { + pr_err("Device not READY in 100ms aborting\n"); + goto out_0; + } + + retval = smsc911x_init(dev); if (retval < 0) goto out_unmap_io_3; @@ -2262,6 +2374,7 @@ out_0: return retval; } + #ifdef CONFIG_PM /* This implementation assumes the devices remains powered on its VDDVARIO * pins during suspend. */
Add some regulator support, there can be necessary to add more regulators to suite all power save needs. But this is a start. Also add a wait for the chip to be ready after the regulators are enabled, this was a bug in the old implementation. Signed-off-by: Robert Marklund <robert.marklund@stericsson.com> --- drivers/net/smsc911x.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 113 insertions(+), 0 deletions(-)