* [PATCH v2] can: c_can: add xceiver enable/disable support
@ 2016-01-20 13:44 Michael Grzeschik
2016-01-20 14:01 ` Markus Pargmann
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Michael Grzeschik @ 2016-01-20 13:44 UTC (permalink / raw)
To: mkl, wg; +Cc: linux-can, netdev, linux-kernel, kernel
This patch adds support to enable and disable the xceiver
in case it's switchable by the regulator framework.
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2:
- always returning PTR_ERR in case devm_regulator_get fails
- removed inline wrapper functions with checks for xceiver == NULL
drivers/net/can/c_can/c_can.c | 12 ++++++++++++
drivers/net/can/c_can/c_can.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index f91b094..0723aeb 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -36,6 +36,7 @@
#include <linux/io.h>
#include <linux/pm_runtime.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/regulator/consumer.h>
#include <linux/can.h>
#include <linux/can/dev.h>
@@ -612,6 +613,10 @@ static int c_can_start(struct net_device *dev)
else
pinctrl_pm_select_default_state(priv->device);
+ err = regulator_enable(priv->reg_xceiver);
+ if (err)
+ return err;
+
return 0;
}
@@ -626,6 +631,9 @@ static void c_can_stop(struct net_device *dev)
/* deactivate pins */
pinctrl_pm_select_sleep_state(dev->dev.parent);
+
+ regulator_disable(priv->reg_xceiver);
+
priv->can.state = CAN_STATE_STOPPED;
}
@@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
*/
pinctrl_pm_select_sleep_state(dev->dev.parent);
+ priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
+ if (IS_ERR(priv->reg_xceiver))
+ return PTR_ERR(priv->reg_xceiver);
+
c_can_pm_runtime_enable(priv);
dev->flags |= IFF_ECHO; /* we support local echo */
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 8acdc7f..59246e3 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -213,6 +213,7 @@ struct c_can_priv {
u32 comm_rcv_high;
u32 rxmasked;
u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
+ struct regulator *reg_xceiver;
};
struct net_device *alloc_c_can_dev(void);
--
2.7.0.rc3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 13:44 [PATCH v2] can: c_can: add xceiver enable/disable support Michael Grzeschik
@ 2016-01-20 14:01 ` Markus Pargmann
2016-01-20 14:11 ` Kurt Van Dijck
2016-01-20 16:19 ` Bjørn Mork
2 siblings, 0 replies; 7+ messages in thread
From: Markus Pargmann @ 2016-01-20 14:01 UTC (permalink / raw)
To: kernel; +Cc: Michael Grzeschik, mkl, wg, netdev, linux-kernel, linux-can
[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]
Hi,
On Wednesday 20 January 2016 14:44:03 Michael Grzeschik wrote:
> This patch adds support to enable and disable the xceiver
> in case it's switchable by the regulator framework.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Reviewed-by: Markus Pargmann <mpa@pengutronix.de>
> ---
> v1 -> v2:
> - always returning PTR_ERR in case devm_regulator_get fails
> - removed inline wrapper functions with checks for xceiver == NULL
>
> drivers/net/can/c_can/c_can.c | 12 ++++++++++++
> drivers/net/can/c_can/c_can.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index f91b094..0723aeb 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -36,6 +36,7 @@
> #include <linux/io.h>
> #include <linux/pm_runtime.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/regulator/consumer.h>
>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -612,6 +613,10 @@ static int c_can_start(struct net_device *dev)
> else
> pinctrl_pm_select_default_state(priv->device);
>
> + err = regulator_enable(priv->reg_xceiver);
> + if (err)
> + return err;
> +
> return 0;
> }
>
> @@ -626,6 +631,9 @@ static void c_can_stop(struct net_device *dev)
>
> /* deactivate pins */
> pinctrl_pm_select_sleep_state(dev->dev.parent);
> +
> + regulator_disable(priv->reg_xceiver);
> +
> priv->can.state = CAN_STATE_STOPPED;
> }
>
> @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
> */
> pinctrl_pm_select_sleep_state(dev->dev.parent);
>
> + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
> + if (IS_ERR(priv->reg_xceiver))
> + return PTR_ERR(priv->reg_xceiver);
> +
> c_can_pm_runtime_enable(priv);
>
> dev->flags |= IFF_ECHO; /* we support local echo */
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 8acdc7f..59246e3 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -213,6 +213,7 @@ struct c_can_priv {
> u32 comm_rcv_high;
> u32 rxmasked;
> u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
> + struct regulator *reg_xceiver;
> };
>
> struct net_device *alloc_c_can_dev(void);
>
--
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 |
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 13:44 [PATCH v2] can: c_can: add xceiver enable/disable support Michael Grzeschik
2016-01-20 14:01 ` Markus Pargmann
@ 2016-01-20 14:11 ` Kurt Van Dijck
2016-01-20 14:29 ` Marc Kleine-Budde
2016-01-20 14:29 ` Markus Pargmann
2016-01-20 16:19 ` Bjørn Mork
2 siblings, 2 replies; 7+ messages in thread
From: Kurt Van Dijck @ 2016-01-20 14:11 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: mkl, wg, linux-can, netdev, linux-kernel, kernel
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index f91b094..0723aeb 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
> */
> pinctrl_pm_select_sleep_state(dev->dev.parent);
>
> + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
I assume "xceiver" is the shorter name for "transceiver"?
In that case, I suggest changing the devicetree label to "transceiver".
It would become a mess if different drivers use different names.
I see no real benefit for naming it "xceiver". "trx" is even shorter :-)
See also http://www.acronymfinder.com/TRX.html
The internals, like variable names, do not really matter here.
I haven't looked at other driver, yet the argument still stands.
Kind regards,
Kurt
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 14:11 ` Kurt Van Dijck
@ 2016-01-20 14:29 ` Marc Kleine-Budde
2016-01-20 14:29 ` Markus Pargmann
1 sibling, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2016-01-20 14:29 UTC (permalink / raw)
To: Michael Grzeschik, wg, linux-can, netdev, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]
On 01/20/2016 03:11 PM, Kurt Van Dijck wrote:
>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index f91b094..0723aeb 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
>> */
>> pinctrl_pm_select_sleep_state(dev->dev.parent);
>>
>> + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
>
> I assume "xceiver" is the shorter name for "transceiver"?
> In that case, I suggest changing the devicetree label to "transceiver".
> It would become a mess if different drivers use different names.
> I see no real benefit for naming it "xceiver". "trx" is even shorter :-)
> See also http://www.acronymfinder.com/TRX.html
>
> The internals, like variable names, do not really matter here.
>
> I haven't looked at other driver, yet the argument still stands.
The transceiver is named xceiver in all drivers.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 14:11 ` Kurt Van Dijck
2016-01-20 14:29 ` Marc Kleine-Budde
@ 2016-01-20 14:29 ` Markus Pargmann
1 sibling, 0 replies; 7+ messages in thread
From: Markus Pargmann @ 2016-01-20 14:29 UTC (permalink / raw)
To: kernel
Cc: Kurt Van Dijck, Michael Grzeschik, netdev, linux-kernel,
linux-can, mkl, wg
[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]
Hi,
On Wednesday 20 January 2016 15:11:51 Kurt Van Dijck wrote:
>
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index f91b094..0723aeb 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
> > */
> > pinctrl_pm_select_sleep_state(dev->dev.parent);
> >
> > + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
>
> I assume "xceiver" is the shorter name for "transceiver"?
> In that case, I suggest changing the devicetree label to "transceiver".
> It would become a mess if different drivers use different names.
> I see no real benefit for naming it "xceiver". "trx" is even shorter :-)
> See also http://www.acronymfinder.com/TRX.html
>
> The internals, like variable names, do not really matter here.
>
> I haven't looked at other driver, yet the argument still stands.
Oh right and perhaps it is necessary to add some documentation for the
devicetree binding if it is not generic already? In that case the DT
mailing list is missing as well.
Best Regards,
Markus
--
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 |
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 13:44 [PATCH v2] can: c_can: add xceiver enable/disable support Michael Grzeschik
2016-01-20 14:01 ` Markus Pargmann
2016-01-20 14:11 ` Kurt Van Dijck
@ 2016-01-20 16:19 ` Bjørn Mork
2016-01-20 16:46 ` Michael Grzeschik
2 siblings, 1 reply; 7+ messages in thread
From: Bjørn Mork @ 2016-01-20 16:19 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: mkl, wg, linux-can, netdev, linux-kernel, kernel
Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
> */
> pinctrl_pm_select_sleep_state(dev->dev.parent);
>
> + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
> + if (IS_ERR(priv->reg_xceiver))
> + return PTR_ERR(priv->reg_xceiver);
> +
> c_can_pm_runtime_enable(priv);
>
> dev->flags |= IFF_ECHO; /* we support local echo */
Do you really want to leave priv->reg_xceiver pointing to an ERR_PTR in
case of error?
Bjørn
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] can: c_can: add xceiver enable/disable support
2016-01-20 16:19 ` Bjørn Mork
@ 2016-01-20 16:46 ` Michael Grzeschik
0 siblings, 0 replies; 7+ messages in thread
From: Michael Grzeschik @ 2016-01-20 16:46 UTC (permalink / raw)
To: Bjørn Mork; +Cc: netdev, linux-kernel, linux-can, mkl, kernel, wg
Hi,
On Wed, Jan 20, 2016 at 05:19:18PM +0100, Bjørn Mork wrote:
> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
>
> > @@ -1263,6 +1271,10 @@ int register_c_can_dev(struct net_device *dev)
> > */
> > pinctrl_pm_select_sleep_state(dev->dev.parent);
> >
> > + priv->reg_xceiver = devm_regulator_get(priv->device, "xceiver");
> > + if (IS_ERR(priv->reg_xceiver))
> > + return PTR_ERR(priv->reg_xceiver);
> > +
> > c_can_pm_runtime_enable(priv);
> >
> > dev->flags |= IFF_ECHO; /* we support local echo */
>
> Do you really want to leave priv->reg_xceiver pointing to an ERR_PTR in
> case of error?
No, therefore the priv->reg_xceiver will be returned in case of error.
This codepath is called once on device registration.
Michael
--
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 |
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-20 16:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-20 13:44 [PATCH v2] can: c_can: add xceiver enable/disable support Michael Grzeschik
2016-01-20 14:01 ` Markus Pargmann
2016-01-20 14:11 ` Kurt Van Dijck
2016-01-20 14:29 ` Marc Kleine-Budde
2016-01-20 14:29 ` Markus Pargmann
2016-01-20 16:19 ` Bjørn Mork
2016-01-20 16:46 ` Michael Grzeschik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.