[PATCH] can: flexcan: add a regulator for transceiver

[PATCH] can: flexcan: add a regulator for transceiver

[Shawn Guo]

Some system designs may have a controllable power supply for
transceiver.  Add a regulator for that, which can be turned on
or off in flexcan_transceiver_switch function.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/net/can/flexcan.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 81d4741..33ff7f0 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,13 @@ static inline void flexcan_write(u32 val, void __iomem *addr)
  */
 static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
 {
+	if (priv->reg_xcvr) {
+		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);
 }
@@ -928,6 +937,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	struct flexcan_priv *priv;
 	struct resource *mem;
 	struct clk *clk = NULL;
+	struct regulator *reg_xcvr;
 	struct pinctrl *pinctrl;
 	void __iomem *base;
 	resource_size_t mem_size;
@@ -957,6 +967,10 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		clock_freq = clk_get_rate(clk);
 	}
 
+	reg_xcvr = devm_regulator_get(&pdev->dev, "xcvr");
+	if (IS_ERR(reg_xcvr))
+		reg_xcvr = NULL;
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
 	if (!mem || irq <= 0) {
@@ -997,6 +1011,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	priv->base = base;
 	priv->dev = dev;
 	priv->clk = clk;
+	priv->reg_xcvr = reg_xcvr;
 	priv->pdata = pdev->dev.platform_data;
 
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
-- 
1.7.5.4

[PATCH] can: flexcan: add a regulator for transceiver

[Mark Brown]

On Tue, Jul 03, 2012 at 08:46:11PM +0800, Shawn Guo wrote:

> +	if (priv->reg_xcvr) {
> +		if (on)
> +			regulator_enable(priv->reg_xcvr);
> +		else
> +			regulator_disable(priv->reg_xcvr);
> +	}

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.

[PATCH] can: flexcan: add a regulator for transceiver

[Shawn Guo]

On Tue, Jul 03, 2012 at 02:07:47PM +0100, Mark Brown wrote:
> On Tue, Jul 03, 2012 at 08:46:11PM +0800, Shawn Guo wrote:
> 
> > +	if (priv->reg_xcvr) {
> > +		if (on)
> > +			regulator_enable(priv->reg_xcvr);
> > +		else
> > +			regulator_disable(priv->reg_xcvr);
> > +	}
> 
> 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?

So the patch will becomes the following.

Regards,
Shawn

---8<----

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);

[PATCH] can: flexcan: add a regulator for transceiver

[Mark Brown]

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.
-------------- 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/20120703/782081de/attachment.sig>

[PATCH] can: flexcan: add a regulator for transceiver

[Shawn Guo]

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.

-- 
Regards,
Shawn

[PATCH] can: flexcan: add a regulator for transceiver

[Mark Brown]

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.