| Message ID | 20120703141829.GG22705@S2101-09.ap.freescale.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 03, 2012 at 10:18:31PM +0800, Shawn Guo wrote: > On Tue, Jul 03, 2012 at 02:07:47PM +0100, Mark Brown wrote: > > No, the regulator API will stub itself out if not enabled, and if the > > supply is fixed then a fixed voltage regulator will do the job. We > > shouldn't be open coding this stuff in individual users. > Ah, yes. But when you say "a fixed voltage regulator", you actually > meant dummy regulator, right? No, I really do mean a fixed voltage regulator. Dummy regulators should essentially never be used in production. > static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on) > { > + if (on) > + regulator_enable(priv->reg_xcvr); > + else > + regulator_disable(priv->reg_xcvr); > + So long as the stack guarantees that you won't get unbalanced calls to this and... > + priv->reg_xcvr = devm_regulator_get(&pdev->dev, "xcvr"); ...add error checking here the code should be fine, yes.
On Tue, Jul 03, 2012 at 03:46:47PM +0100, Mark Brown wrote: > On Tue, Jul 03, 2012 at 10:18:31PM +0800, Shawn Guo wrote: > > On Tue, Jul 03, 2012 at 02:07:47PM +0100, Mark Brown wrote: > > > > No, the regulator API will stub itself out if not enabled, and if the > > > supply is fixed then a fixed voltage regulator will do the job. We > > > shouldn't be open coding this stuff in individual users. > > > Ah, yes. But when you say "a fixed voltage regulator", you actually > > meant dummy regulator, right? > > No, I really do mean a fixed voltage regulator. Dummy regulators should > essentially never be used in production. > In that case, my patch may need to stay unchanged. Some systems have a supply controlled by gpio and we need to have it be a Linux regulator to switch it, while other systems have the supply non-switchable which can be reasonably invisible to software. That said, the regulator is optional and the failure of devm_regulator_get should not make the driver probe fail. I guess your suggestion here is to make the regulator mandatory for the driver, which means those non-switchable supplies have to be also defined as Linux regulators for every single system. I'm not sure we want to do that just for saving a regulator pointer check. > > static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on) > > { > > + if (on) > > + regulator_enable(priv->reg_xcvr); > > + else > > + regulator_disable(priv->reg_xcvr); > > + > > So long as the stack guarantees that you won't get unbalanced calls to > this and... > > > + priv->reg_xcvr = devm_regulator_get(&pdev->dev, "xcvr"); > > ...add error checking here the code should be fine, yes.
On Tue, Jul 03, 2012 at 11:28:15PM +0800, Shawn Guo wrote: > On Tue, Jul 03, 2012 at 03:46:47PM +0100, Mark Brown wrote: > > No, I really do mean a fixed voltage regulator. Dummy regulators should > > essentially never be used in production. > In that case, my patch may need to stay unchanged. Some systems have > a supply controlled by gpio and we need to have it be a Linux regulator > to switch it, while other systems have the supply non-switchable which > can be reasonably invisible to software. That said, the regulator is > optional and the failure of devm_regulator_get should not make the > driver probe fail. No, really. > I guess your suggestion here is to make the regulator mandatory for > the driver, which means those non-switchable supplies have to be > also defined as Linux regulators for every single system. I'm not > sure we want to do that just for saving a regulator pointer check. If we go down that route what we need to do is to go through and remove all error checking for failure to get regulators and make the regulator API silently handle this since exactly the same thing applies to almost every supply in the system. What we shouldn't be doing is open coding this stupidity in every single regulator API user, that's just a pointless waste of everyone's time.
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index 81d4741..c521aa4 100644 --- a/drivers/net/can/flexcan.c +++ b/drivers/net/can/flexcan.c @@ -36,6 +36,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pinctrl/consumer.h> +#include <linux/regulator/consumer.h> #define DRV_NAME "flexcan" @@ -179,6 +180,7 @@ struct flexcan_priv { u32 reg_ctrl_default; struct clk *clk; + struct regulator *reg_xcvr; struct flexcan_platform_data *pdata; }; @@ -224,6 +226,11 @@ static inline void flexcan_write(u32 val, void __iomem *addr) */ static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on) { + if (on) + regulator_enable(priv->reg_xcvr); + else + regulator_disable(priv->reg_xcvr); + if (priv->pdata && priv->pdata->transceiver_switch) priv->pdata->transceiver_switch(on); } @@ -997,6 +1004,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev) priv->base = base; priv->dev = dev; priv->clk = clk; + priv->reg_xcvr = devm_regulator_get(&pdev->dev, "xcvr"); priv->pdata = pdev->dev.platform_data; netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);