* [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
@ 2017-01-06 12:53 Marc Kleine-Budde
2017-01-07 13:22 ` Oliver Hartkopp
0 siblings, 1 reply; 10+ messages in thread
From: Marc Kleine-Budde @ 2017-01-06 12:53 UTC (permalink / raw)
To: linux-can; +Cc: Marc Kleine-Budde
Until commit
08da7da41ea4 can: provide a separate bittiming_const parameter to
bittiming functions
it was possible to have devices not providing bittiming_const. This can
be used for hardware that only support pre-defined fixed bitrates.
Although no mainline driver is using this feature so far.
This patch re-introduces this feature for the bitrate and the data
bitrate (of CANFD controllers). The driver can specify the
{data_,}bittiming_const (if the bittiming parameters should be
calculated from the bittiming_const) as before or no
{data_,}bittiming_const but implement the do_set_{data,}bittiming
callback.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,
this patch should make the mcba_usb driver, which only support fixed bitrates,
a bit simpler.
regards,
Marc
drivers/net/can/dev.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6208c0b400..c240cd9f823b 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -284,10 +284,6 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
{
int err;
- /* Check if the CAN device has bit-timing parameters */
- if (!btc)
- return -EOPNOTSUPP;
-
/*
* Depending on the given can_bittiming parameter structure the CAN
* timing parameters are calculated based on the provided bitrate OR
@@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
/* Do not allow changing bittiming while running */
if (dev->flags & IFF_UP)
return -EBUSY;
+
+ /* Calculate bittiming parameters based on
+ * bittiming_const if set, otherwise pass bitrate
+ * directly via do_set_bitrate(). Bail out if neither
+ * is given.
+ */
+ if (!priv->bittiming_const && !priv->do_set_bittiming)
+ return -EOPNOTSUPP;
+
memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
- err = can_get_bittiming(dev, &bt, priv->bittiming_const);
- if (err)
- return err;
+ if (priv->bittiming_const) {
+ err = can_get_bittiming(dev, &bt,
+ priv->bittiming_const);
+ if (err)
+ return err;
+ }
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
/* Do not allow changing bittiming while running */
if (dev->flags & IFF_UP)
return -EBUSY;
+
+ /* Calculate bittiming parameters based on
+ * data_bittiming_const if set, otherwise pass bitrate
+ * directly via do_set_bitrate(). Bail out if neither
+ * is given.
+ */
+ if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
+ return -EOPNOTSUPP;
+
memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
sizeof(dbt));
- err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
- if (err)
- return err;
+ if (priv->data_bittiming_const) {
+ err = can_get_bittiming(dev, &dbt,
+ priv->data_bittiming_const);
+ if (err)
+ return err;
+ }
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-06 12:53 [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const Marc Kleine-Budde
@ 2017-01-07 13:22 ` Oliver Hartkopp
2017-01-09 10:39 ` Marc Kleine-Budde
0 siblings, 1 reply; 10+ messages in thread
From: Oliver Hartkopp @ 2017-01-07 13:22 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Hi Marc,
On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:
> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
> /* Do not allow changing bittiming while running */
> if (dev->flags & IFF_UP)
> return -EBUSY;
> +
> + /* Calculate bittiming parameters based on
> + * bittiming_const if set, otherwise pass bitrate
> + * directly via do_set_bitrate(). Bail out if neither
> + * is given.
> + */
> + if (!priv->bittiming_const && !priv->do_set_bittiming)
> + return -EOPNOTSUPP;
> +
I would suggest to remove this check and the comment here and better add
an else path with
return -EOPNOTSUPP;
at
if (priv->do_set_bittiming) { } else ...
> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> - err = can_get_bittiming(dev, &bt, priv->bittiming_const);
> - if (err)
> - return err;
> + if (priv->bittiming_const) {
> + err = can_get_bittiming(dev, &bt,
> + priv->bittiming_const);
> + if (err)
> + return err;
> + }
> memcpy(&priv->bittiming, &bt, sizeof(bt));
>
> if (priv->do_set_bittiming) {
> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
> /* Do not allow changing bittiming while running */
> if (dev->flags & IFF_UP)
> return -EBUSY;
> +
> + /* Calculate bittiming parameters based on
> + * data_bittiming_const if set, otherwise pass bitrate
> + * directly via do_set_bitrate(). Bail out if neither
> + * is given.
> + */
> + if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
> + return -EOPNOTSUPP;
> +
> memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
> sizeof(dbt));
> - err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
> - if (err)
> - return err;
> + if (priv->data_bittiming_const) {
> + err = can_get_bittiming(dev, &dbt,
> + priv->data_bittiming_const);
> + if (err)
> + return err;
> + }
> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>
> if (priv->do_set_data_bittiming) {
>
dito here.
Maybe this leads to two patches then:
1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
2. Allow to set bitrates without providing bittiming_const
Regards,
Oliver
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-07 13:22 ` Oliver Hartkopp
@ 2017-01-09 10:39 ` Marc Kleine-Budde
2017-01-09 17:23 ` Oliver Hartkopp
0 siblings, 1 reply; 10+ messages in thread
From: Marc Kleine-Budde @ 2017-01-09 10:39 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 4088 bytes --]
On 01/07/2017 02:22 PM, Oliver Hartkopp wrote:
> Hi Marc,
>
> On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:
>
>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>> /* Do not allow changing bittiming while running */
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>> +
>> + /* Calculate bittiming parameters based on
>> + * bittiming_const if set, otherwise pass bitrate
>> + * directly via do_set_bitrate(). Bail out if neither
>> + * is given.
>> + */
>> + if (!priv->bittiming_const && !priv->do_set_bittiming)
>> + return -EOPNOTSUPP;
>> +
>
> I would suggest to remove this check and the comment here and better add
> an else path with
>
> return -EOPNOTSUPP;
> at
> if (priv->do_set_bittiming) { } else ...
Then the code looks like this:
> if (data[IFLA_CAN_BITTIMING]) {
> struct can_bittiming bt;
>
> /* Do not allow changing bittiming while running */
> if (dev->flags & IFF_UP)
> return -EBUSY;
>
> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> if (priv->bittiming_const) {
> err = can_get_bittiming(dev, &bt,
> priv->bittiming_const);
> if (err)
> return err;
> }
> memcpy(&priv->bittiming, &bt, sizeof(bt));
>
> if (priv->do_set_bittiming) {
> /* Finally, set the bit-timing registers */
> err = priv->do_set_bittiming(dev);
> if (err)
> return err;
> } else if (!priv->bittiming_const) {
This "else" means an implicit "!priv->do_set_bittiming &&" which makes
the code IMHO quite hard to understand.
> /* Calculate bittiming parameters based on
> * bittiming_const if set, otherwise pass bitrate
> * directly via do_set_bitrate(). Bail out if neither
> * is given.
> */
> return -EOPNOTSUPP;
> }
> }
>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> - err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>> - if (err)
>> - return err;
>> + if (priv->bittiming_const) {
>> + err = can_get_bittiming(dev, &bt,
>> + priv->bittiming_const);
>> + if (err)
>> + return err;
>> + }
>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> if (priv->do_set_bittiming) {
>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>> /* Do not allow changing bittiming while running */
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>> +
>> + /* Calculate bittiming parameters based on
>> + * data_bittiming_const if set, otherwise pass bitrate
>> + * directly via do_set_bitrate(). Bail out if neither
>> + * is given.
>> + */
>> + if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>> + return -EOPNOTSUPP;
>> +
>> memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>> sizeof(dbt));
>> - err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>> - if (err)
>> - return err;
>> + if (priv->data_bittiming_const) {
>> + err = can_get_bittiming(dev, &dbt,
>> + priv->data_bittiming_const);
>> + if (err)
>> + return err;
>> + }
>> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>
>> if (priv->do_set_data_bittiming) {
>>
>
> dito here.
>
> Maybe this leads to two patches then:
>
> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
do_set_bittiming is and was not mandatory. Before your patch it was
mandatory to have one (bittiming_const or do_set_bittiming) or both.
With your patch bittiming_const got mandatory, too. My patch
re-introduces the "bittiming_const or do_set_bittiming" state, which is
directly reflected by the "if (!priv->bittiming_const &&
!priv->do_set_bittiming)" statement.
> 2. Allow to set bitrates without providing bittiming_const
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] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-09 10:39 ` Marc Kleine-Budde
@ 2017-01-09 17:23 ` Oliver Hartkopp
2017-01-10 8:39 ` Marc Kleine-Budde
0 siblings, 1 reply; 10+ messages in thread
From: Oliver Hartkopp @ 2017-01-09 17:23 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
On 01/09/2017 11:39 AM, Marc Kleine-Budde wrote:
> On 01/07/2017 02:22 PM, Oliver Hartkopp wrote:
>> Hi Marc,
>>
>> On 01/06/2017 01:53 PM, Marc Kleine-Budde wrote:
>>
>>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>>> /* Do not allow changing bittiming while running */
>>> if (dev->flags & IFF_UP)
>>> return -EBUSY;
>>> +
>>> + /* Calculate bittiming parameters based on
>>> + * bittiming_const if set, otherwise pass bitrate
>>> + * directly via do_set_bitrate(). Bail out if neither
>>> + * is given.
>>> + */
>>> + if (!priv->bittiming_const && !priv->do_set_bittiming)
>>> + return -EOPNOTSUPP;
>>> +
>>
>> I would suggest to remove this check and the comment here and better add
>> an else path with
>>
>> return -EOPNOTSUPP;
>> at
>> if (priv->do_set_bittiming) { } else ...
>
> Then the code looks like this:
>
>> if (data[IFLA_CAN_BITTIMING]) {
>> struct can_bittiming bt;
>>
>> /* Do not allow changing bittiming while running */
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>>
>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> if (priv->bittiming_const) {
>> err = can_get_bittiming(dev, &bt,
>> priv->bittiming_const);
>> if (err)
>> return err;
>> }
>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> if (priv->do_set_bittiming) {
>> /* Finally, set the bit-timing registers */
>> err = priv->do_set_bittiming(dev);
>> if (err)
>> return err;
>> } else if (!priv->bittiming_const) {
>
> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
> the code IMHO quite hard to understand.
No no no.
I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
As you suggested in the answer to my RFC these checks for existing
functions can be moved to the beginning of each code section.
>
>> /* Calculate bittiming parameters based on
>> * bittiming_const if set, otherwise pass bitrate
>> * directly via do_set_bitrate(). Bail out if neither
>> * is given.
>> */
>> return -EOPNOTSUPP;
>> }
>> }
>
>
>>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> - err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>>> - if (err)
>>> - return err;
>>> + if (priv->bittiming_const) {
>>> + err = can_get_bittiming(dev, &bt,
>>> + priv->bittiming_const);
>>> + if (err)
>>> + return err;
>>> + }
>>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>> if (priv->do_set_bittiming) {
>>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>>> /* Do not allow changing bittiming while running */
>>> if (dev->flags & IFF_UP)
>>> return -EBUSY;
>>> +
>>> + /* Calculate bittiming parameters based on
>>> + * data_bittiming_const if set, otherwise pass bitrate
>>> + * directly via do_set_bitrate(). Bail out if neither
>>> + * is given.
>>> + */
>>> + if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>>> + return -EOPNOTSUPP;
>>> +
>>> memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>>> sizeof(dbt));
>>> - err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>>> - if (err)
>>> - return err;
>>> + if (priv->data_bittiming_const) {
>>> + err = can_get_bittiming(dev, &dbt,
>>> + priv->data_bittiming_const);
>>> + if (err)
>>> + return err;
>>> + }
>>> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>>
>>> if (priv->do_set_data_bittiming) {
>>>
>>
>> dito here.
>>
>> Maybe this leads to two patches then:
>>
>> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
>
> do_set_bittiming is and was not mandatory.
This is broken - isn't it?
Does it make sense to make do_set_bittiming optional?
I would think do_set_bittiming is mandatory but bittiming_const can be
optional to support a fixed set of bitrates e.g. for the mcba_usb driver?!?
Regards,
Oliver
> Before your patch it was
> mandatory to have one (bittiming_const or do_set_bittiming) or both.
> With your patch bittiming_const got mandatory, too. My patch
> re-introduces the "bittiming_const or do_set_bittiming" state, which is
> directly reflected by the "if (!priv->bittiming_const &&
> !priv->do_set_bittiming)" statement.
>
>> 2. Allow to set bitrates without providing bittiming_const
>
> Marc
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-09 17:23 ` Oliver Hartkopp
@ 2017-01-10 8:39 ` Marc Kleine-Budde
2017-01-10 10:18 ` Kołłątaj, Remigiusz
2017-01-10 16:54 ` Oliver Hartkopp
0 siblings, 2 replies; 10+ messages in thread
From: Marc Kleine-Budde @ 2017-01-10 8:39 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 5813 bytes --]
On 01/09/2017 06:23 PM, Oliver Hartkopp wrote:
>>>> @@ -872,10 +868,22 @@ static int can_changelink(struct net_device *dev,
>>>> /* Do not allow changing bittiming while running */
>>>> if (dev->flags & IFF_UP)
>>>> return -EBUSY;
>>>> +
>>>> + /* Calculate bittiming parameters based on
>>>> + * bittiming_const if set, otherwise pass bitrate
>>>> + * directly via do_set_bitrate(). Bail out if neither
>>>> + * is given.
>>>> + */
>>>> + if (!priv->bittiming_const && !priv->do_set_bittiming)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>
>>> I would suggest to remove this check and the comment here and better add
>>> an else path with
>>>
>>> return -EOPNOTSUPP;
>>> at
>>> if (priv->do_set_bittiming) { } else ...
>>
>> Then the code looks like this:
>>
>>> if (data[IFLA_CAN_BITTIMING]) {
>>> struct can_bittiming bt;
>>>
>>> /* Do not allow changing bittiming while running */
>>> if (dev->flags & IFF_UP)
>>> return -EBUSY;
>>>
>>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> if (priv->bittiming_const) {
>>> err = can_get_bittiming(dev, &bt,
>>> priv->bittiming_const);
>>> if (err)
>>> return err;
>>> }
>>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>> if (priv->do_set_bittiming) {
>>> /* Finally, set the bit-timing registers */
>>> err = priv->do_set_bittiming(dev);
>>> if (err)
>>> return err;
>>> } else if (!priv->bittiming_const) {
>>
>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>> the code IMHO quite hard to understand.
>
> No no no.
>
> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
No, do_set_bittiming is not mandatory.
On SoC internal CAN cores it's easier to program the bittiming
parameters into the controller during open(). As long as the driver sets
the bittiming_const the framework will check if the bittiming parameters
are correct.
> As you suggested in the answer to my RFC these checks for existing
> functions can be moved to the beginning of each code section.
That's what my original patch proposes, here the resulting code of my patch:
> if (data[IFLA_CAN_BITTIMING]) {
> struct can_bittiming bt;
>
> /* Do not allow changing bittiming while running */
> if (dev->flags & IFF_UP)
> return -EBUSY;
>
> /* Calculate bittiming parameters based on
> * bittiming_const if set, otherwise pass bitrate
> * directly via do_set_bitrate(). Bail out if neither
> * is given.
> */
> if (!priv->bittiming_const && !priv->do_set_bittiming)
> return -EOPNOTSUPP;
>
> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> if (priv->bittiming_const) {
> err = can_get_bittiming(dev, &bt,
> priv->bittiming_const);
> if (err)
> return err;
> }
> memcpy(&priv->bittiming, &bt, sizeof(bt));
>
> if (priv->do_set_bittiming) {
> /* Finally, set the bit-timing registers */
> err = priv->do_set_bittiming(dev);
> if (err)
> return err;
> }
> }
>>> /* Calculate bittiming parameters based on
>>> * bittiming_const if set, otherwise pass bitrate
>>> * directly via do_set_bitrate(). Bail out if neither
>>> * is given.
>>> */
>>> return -EOPNOTSUPP;
>>> }
>>> }
>>
>>
>>>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>>> - err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>>>> - if (err)
>>>> - return err;
>>>> + if (priv->bittiming_const) {
>>>> + err = can_get_bittiming(dev, &bt,
>>>> + priv->bittiming_const);
>>>> + if (err)
>>>> + return err;
>>>> + }
>>>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>>
>>>> if (priv->do_set_bittiming) {
>>>> @@ -943,11 +951,23 @@ static int can_changelink(struct net_device *dev,
>>>> /* Do not allow changing bittiming while running */
>>>> if (dev->flags & IFF_UP)
>>>> return -EBUSY;
>>>> +
>>>> + /* Calculate bittiming parameters based on
>>>> + * data_bittiming_const if set, otherwise pass bitrate
>>>> + * directly via do_set_bitrate(). Bail out if neither
>>>> + * is given.
>>>> + */
>>>> + if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
>>>> sizeof(dbt));
>>>> - err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
>>>> - if (err)
>>>> - return err;
>>>> + if (priv->data_bittiming_const) {
>>>> + err = can_get_bittiming(dev, &dbt,
>>>> + priv->data_bittiming_const);
>>>> + if (err)
>>>> + return err;
>>>> + }
>>>> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>>>
>>>> if (priv->do_set_data_bittiming) {
>>>>
>>>
>>> dito here.
>>>
>>> Maybe this leads to two patches then:
>>>
>>> 1. Introduce the -EOPNOTSUPP for missing do_set[_data]_bittiming
>>
>> do_set_bittiming is and was not mandatory.
>
> This is broken - isn't it?
> Does it make sense to make do_set_bittiming optional?
No, it's perfectly fine, see above.
> I would think do_set_bittiming is mandatory but bittiming_const can be
> optional to support a fixed set of bitrates e.g. for the mcba_usb driver?!?
Until your patch ("08da7da41ea4 can: provide a separate bittiming_const
parameter to bittiming functions) bittiming_const was optional.
Recap: do_set_bittiming or bittiming_const (or both) must be provided.
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] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-10 8:39 ` Marc Kleine-Budde
@ 2017-01-10 10:18 ` Kołłątaj, Remigiusz
2017-01-10 10:33 ` Marc Kleine-Budde
2017-01-10 16:54 ` Oliver Hartkopp
1 sibling, 1 reply; 10+ messages in thread
From: Kołłątaj, Remigiusz @ 2017-01-10 10:18 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Oliver Hartkopp, linux-can
Hi,
I can see three major problems with setting up predefined bitrates:
1. I think that there is no way to pass supported bitrates to
userspace (other than printk)
2. The bitrate that is passed to the driver is calculated/validated by
can_get_bittiming. This causes that driver not always get exact value
that was input by a user.
3. When CAN device is using predefined bitrates you do not have
influence or even access to bittiming parameters. As you noticed I had
to hardcode those values in mcba_usb to present them correctly to
userspace.
Based on solution created by Oliver for termination I created
something similar for predefined bitrates. It solves issue number 1
and 2. Not sure how to solve issue 3 correctly to maintain
compatibility (adding another netlink value for predefined bitrate
will break it). As for now I just zeroed values.
The patch is just to visualise my idea. It definitely needs some more work.
Inside the driver:
const u32 predef_bitrate[] = {20000, 50000, 100000};
priv->bitrate_const = predef_bitrate;
priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 8d6208c0b400..d2034a69c990 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -279,25 +279,44 @@ static int can_fixup_bittiming(struct net_device
*dev, struct can_bittiming *bt,
return 0;
}
+/*
+ * Checks the validity of predefined bitrate settings
+ */
+static int can_validate_bittiming(struct net_device *dev, struct
can_bittiming *bt)
+{
+ struct can_priv *priv = netdev_priv(dev);
+ int i;
+
+ for(i = 0; i < priv->bitrate_const_cnt &&
+ bt->bitrate != priv->bitrate_const[i]; ++i);
+
+ if (i >= priv->bitrate_const_cnt)
+ return -EINVAL;
+
+ /* Clear bittiming settings to idicate we are using predefined
bitrate */
+ memset(bt, 0, sizeof(struct can_bittiming));
+ bt->bitrate = priv->bitrate_const[i];
+
+ return 0;
+}
+
static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
const struct can_bittiming_const *btc)
{
int err;
- /* Check if the CAN device has bit-timing parameters */
- if (!btc)
- return -EOPNOTSUPP;
-
/*
* Depending on the given can_bittiming parameter structure the CAN
* timing parameters are calculated based on the provided bitrate OR
* alternatively the CAN timing parameters (tq, prop_seg, etc.) are
* provided directly which are then checked and fixed up.
*/
- if (!bt->tq && bt->bitrate)
+ if (!bt->tq && bt->bitrate && btc)
err = can_calc_bittiming(dev, bt, btc);
- else if (bt->tq && !bt->bitrate)
+ else if (bt->tq && !bt->bitrate && btc)
err = can_fixup_bittiming(dev, bt, btc);
+ else if (bt->bitrate && !btc)
+ err = can_validate_bittiming(dev, bt);
else
err = -EINVAL;
@@ -1018,7 +1037,11 @@ 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->bitrate_const &&
+ nla_put(skb, IFLA_CAN_BITRATE_CONST,
+ sizeof(u32) * priv->bitrate_const_cnt,
priv->bitrate_const)))
return -EMSGSIZE;
return 0;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5f5270941ba0..fa5e8337f0e3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -38,6 +38,8 @@ struct can_priv {
struct can_bittiming bittiming, data_bittiming;
const struct can_bittiming_const *bittiming_const,
*data_bittiming_const;
+ const u32 *bitrate_const;
+ const int bitrate_const_cnt;
struct can_clock clock;
enum can_state state;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 94ffe0c83ce7..7d1815dad72b 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -127,6 +127,7 @@ enum {
IFLA_CAN_BERR_COUNTER,
IFLA_CAN_DATA_BITTIMING,
IFLA_CAN_DATA_BITTIMING_CONST,
+ IFLA_CAN_BITRATE_CONST,
__IFLA_CAN_MAX
};
Regards,
Remik
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-10 10:18 ` Kołłątaj, Remigiusz
@ 2017-01-10 10:33 ` Marc Kleine-Budde
2017-01-10 16:42 ` Kołłątaj, Remigiusz
0 siblings, 1 reply; 10+ messages in thread
From: Marc Kleine-Budde @ 2017-01-10 10:33 UTC (permalink / raw)
To: Kołłątaj, Remigiusz; +Cc: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 1763 bytes --]
On 01/10/2017 11:18 AM, Kołłątaj, Remigiusz wrote:
> Hi,
>
> I can see three major problems with setting up predefined bitrates:
> 1. I think that there is no way to pass supported bitrates to
> userspace (other than printk)
yes
> 2. The bitrate that is passed to the driver is calculated/validated by
> can_get_bittiming. This causes that driver not always get exact value
> that was input by a user.
Have you seen my patch?
> 3. When CAN device is using predefined bitrates you do not have
> influence or even access to bittiming parameters. As you noticed I had
> to hardcode those values in mcba_usb to present them correctly to
> userspace.
With my patch, there's no need to set bittiming_const anymore.
> Based on solution created by Oliver for termination I created
> something similar for predefined bitrates. It solves issue number 1
> and 2. Not sure how to solve issue 3 correctly to maintain
> compatibility (adding another netlink value for predefined bitrate
> will break it). As for now I just zeroed values.
>
> The patch is just to visualise my idea. It definitely needs some more work.
>
> Inside the driver:
> const u32 predef_bitrate[] = {20000, 50000, 100000};
>
> priv->bitrate_const = predef_bitrate;
> priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);
The general idea makes sense, although we need support for CANFD, too.
Let's first agree on Oliver's and my patch, then yours will fit in easily.
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: 484 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-10 10:33 ` Marc Kleine-Budde
@ 2017-01-10 16:42 ` Kołłątaj, Remigiusz
0 siblings, 0 replies; 10+ messages in thread
From: Kołłątaj, Remigiusz @ 2017-01-10 16:42 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Oliver Hartkopp, linux-can
Marc,
On 10 January 2017 at 11:33, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 01/10/2017 11:18 AM, Kołłątaj, Remigiusz wrote:
>> Hi,
>>
>> I can see three major problems with setting up predefined bitrates:
>> 1. I think that there is no way to pass supported bitrates to
>> userspace (other than printk)
>
> yes
>
>> 2. The bitrate that is passed to the driver is calculated/validated by
>> can_get_bittiming. This causes that driver not always get exact value
>> that was input by a user.
>
> Have you seen my patch?
Yes, your patch solves that issue too.
>
>> 3. When CAN device is using predefined bitrates you do not have
>> influence or even access to bittiming parameters. As you noticed I had
>> to hardcode those values in mcba_usb to present them correctly to
>> userspace.
>
> With my patch, there's no need to set bittiming_const anymore.
You are right, but can_bittiming is still passed to userspace and
since devices with predefined bitrates have no interest in bittiming
parameters the struct will contain undefined values (i.e. tq,
prop_seg, phase_seg etc.).
>
>> Based on solution created by Oliver for termination I created
>> something similar for predefined bitrates. It solves issue number 1
>> and 2. Not sure how to solve issue 3 correctly to maintain
>> compatibility (adding another netlink value for predefined bitrate
>> will break it). As for now I just zeroed values.
>>
>> The patch is just to visualise my idea. It definitely needs some more work.
>>
>> Inside the driver:
>> const u32 predef_bitrate[] = {20000, 50000, 100000};
>>
>> priv->bitrate_const = predef_bitrate;
>> priv->bitrate_const_cnt = ARRAY_SIZE(predef_bitrate);
>
> The general idea makes sense, although we need support for CANFD, too.
I'm not so familiar with CANFD. Do you mean can_data_bittiming part?
> Let's first agree on Oliver's and my patch, then yours will fit in easily.
>
Ok.
Cheers,
Remik
--
____________________
Remigiusz Kołłątaj
Mobica Ltd
Technology Consultant
Skype: remigiusz.kollataj
www.mobica.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-10 8:39 ` Marc Kleine-Budde
2017-01-10 10:18 ` Kołłątaj, Remigiusz
@ 2017-01-10 16:54 ` Oliver Hartkopp
2017-01-10 19:42 ` Marc Kleine-Budde
1 sibling, 1 reply; 10+ messages in thread
From: Oliver Hartkopp @ 2017-01-10 16:54 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
On 01/10/2017 09:39 AM, Marc Kleine-Budde wrote:
>>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>>> the code IMHO quite hard to understand.
>>
>> No no no.
>>
>> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
>
> No, do_set_bittiming is not mandatory.
>
> On SoC internal CAN cores it's easier to program the bittiming
> parameters into the controller during open(). As long as the driver sets
> the bittiming_const the framework will check if the bittiming parameters
> are correct.
Oh. Didn't know that use-case for SoCs.
>> As you suggested in the answer to my RFC these checks for existing
>> functions can be moved to the beginning of each code section.
>
> That's what my original patch proposes, here the resulting code of my patch:
>
>> if (data[IFLA_CAN_BITTIMING]) {
>> struct can_bittiming bt;
>>
>> /* Do not allow changing bittiming while running */
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>>
>> /* Calculate bittiming parameters based on
>> * bittiming_const if set, otherwise pass bitrate
>> * directly via do_set_bitrate(). Bail out if neither
>> * is given.
>> */
>> if (!priv->bittiming_const && !priv->do_set_bittiming)
>> return -EOPNOTSUPP;
>>
>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>> if (priv->bittiming_const) {
>> err = can_get_bittiming(dev, &bt,
>> priv->bittiming_const);
>> if (err)
>> return err;
>> }
>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> if (priv->do_set_bittiming) {
I was stumbling upon this additional check.
But now it seems correct to me ;-)
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Thanks,
Oliver
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const
2017-01-10 16:54 ` Oliver Hartkopp
@ 2017-01-10 19:42 ` Marc Kleine-Budde
0 siblings, 0 replies; 10+ messages in thread
From: Marc Kleine-Budde @ 2017-01-10 19:42 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 2141 bytes --]
On 01/10/2017 05:54 PM, Oliver Hartkopp wrote:
> On 01/10/2017 09:39 AM, Marc Kleine-Budde wrote:
>
>>>> This "else" means an implicit "!priv->do_set_bittiming &&" which makes
>>>> the code IMHO quite hard to understand.
>>>
>>> No no no.
>>>
>>> I just meant to throw -EOPNOTSUPP if !(priv->do_set_bittiming).
>>
>> No, do_set_bittiming is not mandatory.
>>
>> On SoC internal CAN cores it's easier to program the bittiming
>> parameters into the controller during open(). As long as the driver sets
>> the bittiming_const the framework will check if the bittiming parameters
>> are correct.
>
> Oh. Didn't know that use-case for SoCs.
>
>>> As you suggested in the answer to my RFC these checks for existing
>>> functions can be moved to the beginning of each code section.
>>
>> That's what my original patch proposes, here the resulting code of my patch:
>>
>>> if (data[IFLA_CAN_BITTIMING]) {
>>> struct can_bittiming bt;
>>>
>>> /* Do not allow changing bittiming while running */
>>> if (dev->flags & IFF_UP)
>>> return -EBUSY;
>>>
>>> /* Calculate bittiming parameters based on
>>> * bittiming_const if set, otherwise pass bitrate
>>> * directly via do_set_bitrate(). Bail out if neither
>>> * is given.
>>> */
>>> if (!priv->bittiming_const && !priv->do_set_bittiming)
>>> return -EOPNOTSUPP;
>>>
>>> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>>> if (priv->bittiming_const) {
>>> err = can_get_bittiming(dev, &bt,
>>> priv->bittiming_const);
>>> if (err)
>>> return err;
>>> }
>>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>>
>>> if (priv->do_set_bittiming) {
>
> I was stumbling upon this additional check.
>
> But now it seems correct to me ;-)
>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
tnx.
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] 10+ messages in thread
end of thread, other threads:[~2017-01-10 19:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-06 12:53 [RFC, PATCH] can: dev: can_changelink: allow to set bitrate on devices not providing {data_,}bittiming_const Marc Kleine-Budde
2017-01-07 13:22 ` Oliver Hartkopp
2017-01-09 10:39 ` Marc Kleine-Budde
2017-01-09 17:23 ` Oliver Hartkopp
2017-01-10 8:39 ` Marc Kleine-Budde
2017-01-10 10:18 ` Kołłątaj, Remigiusz
2017-01-10 10:33 ` Marc Kleine-Budde
2017-01-10 16:42 ` Kołłątaj, Remigiusz
2017-01-10 16:54 ` Oliver Hartkopp
2017-01-10 19:42 ` Marc Kleine-Budde
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.