* [PATCH] [RFCv2] can: add CAN interface termination API
@ 2017-01-08 16:35 Oliver Hartkopp
2017-01-09 9:25 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Oliver Hartkopp @ 2017-01-08 16:35 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
This patch adds a netlink interface to configure the CAN bus termination of
CAN interfaces.
Inside the driver an array of supported termination values is defined:
const u16 drvname_termination = { 60, 120, CAN_TERMINATION_DISABLED };
struct drvname_priv *priv;
priv = netdev_priv(dev);
priv->termination_const = &drvname_termination;
priv->termination_const_size = sizeof(drvname_termination);
priv->termination = CAN_TERMINATION_DISABLED;
And the funtion to set the value has to be defined:
priv->do_set_termination = drvname_set_termination;
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 34 +++++++++++++++++++++++++++++++++-
include/linux/can/dev.h | 4 ++++
include/uapi/linux/can/netlink.h | 5 +++++
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6208c0b400..f98825f6ea2e 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -958,6 +958,27 @@ static int can_changelink(struct net_device *dev,
}
}
+ if (data[IFLA_CAN_TERMINATION]) {
+ u16 t = nla_get_u16(data[IFLA_CAN_TERMINATION]);
+ int num_t = priv->termination_const_size / sizeof(u16);
+ int i;
+
+ /* check whether given value is supported by the interface */
+ for (i = 0; i < num_t && t != priv->termination_const[i]; i++);
+ if (i >= num_t)
+ return -EINVAL;
+
+ if (priv->do_set_termination) {
+ /* Finally, set the termination */
+ err = priv->do_set_termination(dev, t);
+ if (err)
+ return err;
+
+ priv->termination = t;
+ } else
+ return -EOPNOTSUPP;
+ }
+
return 0;
}
@@ -980,6 +1001,11 @@ static size_t can_get_size(const struct net_device *dev)
size += nla_total_size(sizeof(struct can_bittiming));
if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
size += nla_total_size(sizeof(struct can_bittiming_const));
+ if (priv->termination_const &&
+ priv->termination_const_size) { /* IFLA_CAN_TERMINATION_CONST */
+ size += nla_total_size(priv->termination_const_size);
+ size += nla_total_size(sizeof(u16)); /* IFLA_CAN_TERMINATION */
+ }
return size;
}
@@ -1018,7 +1044,13 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
(priv->data_bittiming_const &&
nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
sizeof(*priv->data_bittiming_const),
- priv->data_bittiming_const)))
+ priv->data_bittiming_const)) ||
+
+ (priv->termination_const && priv->termination_const_size &&
+ nla_put(skb, IFLA_CAN_TERMINATION_CONST,
+ priv->termination_const_size,
+ priv->termination_const) &&
+ nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination)))
return -EMSGSIZE;
return 0;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5f5270941ba0..7aa0eb1a75e3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -38,6 +38,9 @@ struct can_priv {
struct can_bittiming bittiming, data_bittiming;
const struct can_bittiming_const *bittiming_const,
*data_bittiming_const;
+ const u16 *termination_const;
+ const int termination_const_size;
+ u16 termination;
struct can_clock clock;
enum can_state state;
@@ -53,6 +56,7 @@ struct can_priv {
int (*do_set_bittiming)(struct net_device *dev);
int (*do_set_data_bittiming)(struct net_device *dev);
int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
+ int (*do_set_termination)(struct net_device *dev, u16 term);
int (*do_get_state)(const struct net_device *dev,
enum can_state *state);
int (*do_get_berr_counter)(const struct net_device *dev,
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 94ffe0c83ce7..7414771926fb 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -127,9 +127,14 @@ enum {
IFLA_CAN_BERR_COUNTER,
IFLA_CAN_DATA_BITTIMING,
IFLA_CAN_DATA_BITTIMING_CONST,
+ IFLA_CAN_TERMINATION,
+ IFLA_CAN_TERMINATION_CONST,
__IFLA_CAN_MAX
};
#define IFLA_CAN_MAX (__IFLA_CAN_MAX - 1)
+/* u16 termination range: 1..65535 Ohms */
+#define CAN_TERMINATION_DISABLED 0
+
#endif /* !_UAPI_CAN_NETLINK_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] [RFCv2] can: add CAN interface termination API
2017-01-08 16:35 [PATCH] [RFCv2] can: add CAN interface termination API Oliver Hartkopp
@ 2017-01-09 9:25 ` Marc Kleine-Budde
2017-01-09 17:13 ` Oliver Hartkopp
0 siblings, 1 reply; 4+ messages in thread
From: Marc Kleine-Budde @ 2017-01-09 9:25 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 5295 bytes --]
On 01/08/2017 05:35 PM, Oliver Hartkopp wrote:
> This patch adds a netlink interface to configure the CAN bus termination of
> CAN interfaces.
>
> Inside the driver an array of supported termination values is defined:
>
> const u16 drvname_termination = { 60, 120, CAN_TERMINATION_DISABLED };
drvname_termination[]
>
> struct drvname_priv *priv;
> priv = netdev_priv(dev);
>
> priv->termination_const = &drvname_termination;
> priv->termination_const_size = sizeof(drvname_termination);
ARRAY_SIZE()
> priv->termination = CAN_TERMINATION_DISABLED;
>
> And the funtion to set the value has to be defined:
>
> priv->do_set_termination = drvname_set_termination;
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> drivers/net/can/dev.c | 34 +++++++++++++++++++++++++++++++++-
> include/linux/can/dev.h | 4 ++++
> include/uapi/linux/can/netlink.h | 5 +++++
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 8d6208c0b400..f98825f6ea2e 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -958,6 +958,27 @@ static int can_changelink(struct net_device *dev,
> }
> }
>
> + if (data[IFLA_CAN_TERMINATION]) {
> + u16 t = nla_get_u16(data[IFLA_CAN_TERMINATION]);
> + int num_t = priv->termination_const_size / sizeof(u16);
ARRAY_SIZE...or see below:
> + int i;
> +
> + /* check whether given value is supported by the interface */
> + for (i = 0; i < num_t && t != priv->termination_const[i]; i++);
Nitpick: unusual for() loop, better make more explicit what's going on:
for (i = 0; i < ARRAY_SIZE(priv->termination_const_size), i++) {
if (t == priv->termination_const[i])
break;
}
> + if (i >= num_t)
> + return -EINVAL;
> +
> + if (priv->do_set_termination) {
What about moving the check for priv->do_set_termination in front of the
loop?
> + /* Finally, set the termination */
> + err = priv->do_set_termination(dev, t);
> + if (err)
> + return err;
> +
> + priv->termination = t;
> + } else
> + return -EOPNOTSUPP;
{ } on both legs of the else
> + }
> +
> return 0;
> }
>
> @@ -980,6 +1001,11 @@ static size_t can_get_size(const struct net_device *dev)
> size += nla_total_size(sizeof(struct can_bittiming));
> if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
> size += nla_total_size(sizeof(struct can_bittiming_const));
> + if (priv->termination_const &&
> + priv->termination_const_size) { /* IFLA_CAN_TERMINATION_CONST */
> + size += nla_total_size(priv->termination_const_size);
> + size += nla_total_size(sizeof(u16)); /* IFLA_CAN_TERMINATION */
> + }
>
> return size;
> }
> @@ -1018,7 +1044,13 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
> (priv->data_bittiming_const &&
> nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
> sizeof(*priv->data_bittiming_const),
> - priv->data_bittiming_const)))
> + priv->data_bittiming_const)) ||
> +
> + (priv->termination_const && priv->termination_const_size &&
> + nla_put(skb, IFLA_CAN_TERMINATION_CONST,
> + priv->termination_const_size,
> + priv->termination_const) &&
> + nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination)))
> return -EMSGSIZE;
>
> return 0;
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 5f5270941ba0..7aa0eb1a75e3 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -38,6 +38,9 @@ struct can_priv {
> struct can_bittiming bittiming, data_bittiming;
> const struct can_bittiming_const *bittiming_const,
> *data_bittiming_const;
> + const u16 *termination_const;
> + const int termination_const_size;
> + u16 termination;
> struct can_clock clock;
>
> enum can_state state;
> @@ -53,6 +56,7 @@ struct can_priv {
> int (*do_set_bittiming)(struct net_device *dev);
> int (*do_set_data_bittiming)(struct net_device *dev);
> int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
> + int (*do_set_termination)(struct net_device *dev, u16 term);
> int (*do_get_state)(const struct net_device *dev,
> enum can_state *state);
> int (*do_get_berr_counter)(const struct net_device *dev,
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index 94ffe0c83ce7..7414771926fb 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -127,9 +127,14 @@ enum {
> IFLA_CAN_BERR_COUNTER,
> IFLA_CAN_DATA_BITTIMING,
> IFLA_CAN_DATA_BITTIMING_CONST,
> + IFLA_CAN_TERMINATION,
> + IFLA_CAN_TERMINATION_CONST,
> __IFLA_CAN_MAX
> };
>
> #define IFLA_CAN_MAX (__IFLA_CAN_MAX - 1)
>
> +/* u16 termination range: 1..65535 Ohms */
> +#define CAN_TERMINATION_DISABLED 0
> +
> #endif /* !_UAPI_CAN_NETLINK_H */
>
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: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [RFCv2] can: add CAN interface termination API
2017-01-09 9:25 ` Marc Kleine-Budde
@ 2017-01-09 17:13 ` Oliver Hartkopp
2017-01-10 10:37 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Oliver Hartkopp @ 2017-01-09 17:13 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Hello Marc,
On 01/09/2017 10:25 AM, Marc Kleine-Budde wrote:
> On 01/08/2017 05:35 PM, Oliver Hartkopp wrote:
>> This patch adds a netlink interface to configure the CAN bus termination of
>> CAN interfaces.
>>
>> Inside the driver an array of supported termination values is defined:
>>
>> const u16 drvname_termination = { 60, 120, CAN_TERMINATION_DISABLED };
>
> drvname_termination[]
>
Ok.
>>
>> struct drvname_priv *priv;
>> priv = netdev_priv(dev);
>>
>> priv->termination_const = &drvname_termination;
>> priv->termination_const_size = sizeof(drvname_termination);
>
> ARRAY_SIZE()
When we use ARRAY_SIZE(drvname_termination) here ...
>> priv->termination = CAN_TERMINATION_DISABLED;
>>
>> And the funtion to set the value has to be defined:
>>
>> priv->do_set_termination = drvname_set_termination;
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>> drivers/net/can/dev.c | 34 +++++++++++++++++++++++++++++++++-
>> include/linux/can/dev.h | 4 ++++
>> include/uapi/linux/can/netlink.h | 5 +++++
>> 3 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 8d6208c0b400..f98825f6ea2e 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -958,6 +958,27 @@ static int can_changelink(struct net_device *dev,
>> }
>> }
>>
>> + if (data[IFLA_CAN_TERMINATION]) {
>> + u16 t = nla_get_u16(data[IFLA_CAN_TERMINATION]);
>> + int num_t = priv->termination_const_size / sizeof(u16);
... this needs to turn into
int num_t = priv->termination_const_size;
but ...
>
> ARRAY_SIZE...or see below:
>
>> + int i;
>> +
>> + /* check whether given value is supported by the interface */
>> + for (i = 0; i < num_t && t != priv->termination_const[i]; i++);
>
> Nitpick: unusual for() loop, better make more explicit what's going on:
>
> for (i = 0; i < ARRAY_SIZE(priv->termination_const_size), i++) {
... this does never work as it references an integer ...
> if (t == priv->termination_const[i])
> break;
> }
>
>> + if (i >= num_t)
>> + return -EINVAL;
>> +
>> + if (priv->do_set_termination) {
>
> What about moving the check for priv->do_set_termination in front of the
> loop?
>
>> + /* Finally, set the termination */
>> + err = priv->do_set_termination(dev, t);
>> + if (err)
>> + return err;
>> +
>> + priv->termination = t;
>> + } else
>> + return -EOPNOTSUPP;
>
> { } on both legs of the else
>
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -980,6 +1001,11 @@ static size_t can_get_size(const struct net_device *dev)
>> size += nla_total_size(sizeof(struct can_bittiming));
>> if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
>> size += nla_total_size(sizeof(struct can_bittiming_const));
>> + if (priv->termination_const &&
>> + priv->termination_const_size) { /* IFLA_CAN_TERMINATION_CONST */
>> + size += nla_total_size(priv->termination_const_size);
... this will break ...
>> + size += nla_total_size(sizeof(u16)); /* IFLA_CAN_TERMINATION */
>> + }
>>
>> return size;
>> }
>> @@ -1018,7 +1044,13 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
>> (priv->data_bittiming_const &&
>> nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
>> sizeof(*priv->data_bittiming_const),
>> - priv->data_bittiming_const)))
>> + priv->data_bittiming_const)) ||
>> +
>> + (priv->termination_const && priv->termination_const_size &&
>> + nla_put(skb, IFLA_CAN_TERMINATION_CONST,
>> + priv->termination_const_size,
... and this too.
>> + priv->termination_const) &&
>> + nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination)))
>> return -EMSGSIZE;
>>
I just wonder, which is the most readable representation here?
Regards,
Oliver
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [RFCv2] can: add CAN interface termination API
2017-01-09 17:13 ` Oliver Hartkopp
@ 2017-01-10 10:37 ` Marc Kleine-Budde
0 siblings, 0 replies; 4+ messages in thread
From: Marc Kleine-Budde @ 2017-01-10 10:37 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 4009 bytes --]
On 01/09/2017 06:13 PM, Oliver Hartkopp wrote:
> Hello Marc,
>
> On 01/09/2017 10:25 AM, Marc Kleine-Budde wrote:
>> On 01/08/2017 05:35 PM, Oliver Hartkopp wrote:
>>> This patch adds a netlink interface to configure the CAN bus termination of
>>> CAN interfaces.
>>>
>>> Inside the driver an array of supported termination values is defined:
>>>
>>> const u16 drvname_termination = { 60, 120, CAN_TERMINATION_DISABLED };
>>
>> drvname_termination[]
>>
>
> Ok.
>
>>>
>>> struct drvname_priv *priv;
>>> priv = netdev_priv(dev);
>>>
>>> priv->termination_const = &drvname_termination;
>>> priv->termination_const_size = sizeof(drvname_termination);
>>
>> ARRAY_SIZE()
>
> When we use ARRAY_SIZE(drvname_termination) here ...
Remigiusz's patch using _len = ARRAY_SIZE() here and _len * (sizeof())
in the netlink interface looks nice to me.
>>> priv->termination = CAN_TERMINATION_DISABLED;
>>>
>>> And the funtion to set the value has to be defined:
>>>
>>> priv->do_set_termination = drvname_set_termination;
>>>
>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>> ---
>>> drivers/net/can/dev.c | 34 +++++++++++++++++++++++++++++++++-
>>> include/linux/can/dev.h | 4 ++++
>>> include/uapi/linux/can/netlink.h | 5 +++++
>>> 3 files changed, 42 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index 8d6208c0b400..f98825f6ea2e 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -958,6 +958,27 @@ static int can_changelink(struct net_device *dev,
>>> }
>>> }
>>>
>>> + if (data[IFLA_CAN_TERMINATION]) {
>>> + u16 t = nla_get_u16(data[IFLA_CAN_TERMINATION]);
>>> + int num_t = priv->termination_const_size / sizeof(u16);
>
> ... this needs to turn into
>
> int num_t = priv->termination_const_size;
>
> but ...
>
>>
>> ARRAY_SIZE...or see below:
>>
>>> + int i;
>>> +
>>> + /* check whether given value is supported by the interface */
>>> + for (i = 0; i < num_t && t != priv->termination_const[i]; i++);
>>
>> Nitpick: unusual for() loop, better make more explicit what's going on:
>>
>> for (i = 0; i < ARRAY_SIZE(priv->termination_const_size), i++) {
>
> ... this does never work as it references an integer ...
Yes, doh!
[...]
>>> @@ -980,6 +1001,11 @@ static size_t can_get_size(const struct net_device *dev)
>>> size += nla_total_size(sizeof(struct can_bittiming));
>>> if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
>>> size += nla_total_size(sizeof(struct can_bittiming_const));
>>> + if (priv->termination_const &&
>>> + priv->termination_const_size) { /* IFLA_CAN_TERMINATION_CONST */
>>> + size += nla_total_size(priv->termination_const_size);
>
> ... this will break ...
>
>>> + size += nla_total_size(sizeof(u16)); /* IFLA_CAN_TERMINATION */
>>> + }
>>>
>>> return size;
>>> }
>>> @@ -1018,7 +1044,13 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
>>> (priv->data_bittiming_const &&
>>> nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
>>> sizeof(*priv->data_bittiming_const),
>>> - priv->data_bittiming_const)))
>>> + priv->data_bittiming_const)) ||
>>> +
>>> + (priv->termination_const && priv->termination_const_size &&
>>> + nla_put(skb, IFLA_CAN_TERMINATION_CONST,
>>> + priv->termination_const_size,
>
> ... and this too.
>
>>> + priv->termination_const) &&
>>> + nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination)))
>>> return -EMSGSIZE;
>>>
>
> I just wonder, which is the most readable representation here?
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: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-10 10:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-08 16:35 [PATCH] [RFCv2] can: add CAN interface termination API Oliver Hartkopp
2017-01-09 9:25 ` Marc Kleine-Budde
2017-01-09 17:13 ` Oliver Hartkopp
2017-01-10 10:37 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox