can: flexcan: add a regulator for transceiver

PATCH New Shawn Guo 2 participants 6 messages Jul 3, 2012 CAN network drivers

Share & tools

Shawn Guo

author

2012-07-03 12:45 UTC #0 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

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 <[email protected]>

---
 drivers/net/can/flexcan.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

drivers/net/can/flexcan.c +15 -0

index 81d4741..33ff7f0 100644
@@ -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);

Signature · 1 line

1.7.5.4

Mark Brown

2012-07-03 13:07 UTC #1 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

drivers/net/can/flexcan.c static inline void flexcan_write(u32 val, void __iomem *addr) · line 234

Quoted 6 lines · Shawn Guo

drivers/net/can/flexcan.c +6 -0

+	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.

Shawn Guo

author

2012-07-03 14:17 UTC #2 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

Quoted 12 lines · Mark Brown

Review
Quoted 6 lines · Shawn Guo
(quoted diff) +6 -0
+	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<----

drivers/net/can/flexcan.c +8 -0

index 81d4741..c521aa4 100644
@@ -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);

Mark Brown

2012-07-03 14:46 UTC #3 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

Quoted 8 lines · Shawn Guo

Quoted 3 lines · Mark Brown
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.

drivers/net/can/flexcan.c static inline void flexcan_write(u32 val, void __iomem *addr) · line 233

Quoted 7 lines

static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on) {
drivers/net/can/flexcan.c +5 -0
+       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...

Quoted 1 line

+ 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>

Shawn Guo

author

2012-07-03 15:27 UTC #4 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

Quoted 7 lines · Mark Brown

Quoted 5 lines · Shawn Guo
Quoted 3 lines · Mark Brown
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 14 lines

drivers/net/can/flexcan.c static inline void flexcan_write(u32 val, void __iomem *addr) · line 233
Quoted 7 lines
static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on) {
drivers/net/can/flexcan.c +5 -0
+       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...
Quoted 1 line
+ priv->reg_xcvr = devm_regulator_get(&pdev->dev, "xcvr");
...add error checking here the code should be fine, yes.

Signature · 2 lines

Regards,
Shawn

Mark Brown

2012-07-03 16:56 UTC #5 lore raw

PATCH 1/1 can: flexcan: add a regulator for transceiver

Quoted 11 lines · Shawn Guo

Quoted 2 lines · Mark Brown
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.

Quoted 4 lines

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.