The functions can_update_sample_point() and can_calc_bittiming() are
generic and meant to be used for both the nominal and the data
bittiming calculation.
However, those functions use terminologies such as "bitrate nominal"
or "sample point nominal". This is a leftover from when only Classical
CAN was supported and now became incorrect.
Remove or replace any occurrences of the word "nominal" with something
more accurate.
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 268ec6fa7c49..222117596704 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -24,7 +24,7 @@
*/
static int
can_update_sample_point(const struct can_bittiming_const *btc,
- const unsigned int sample_point_nominal, const unsigned int tseg,
+ unsigned int sp_origin, unsigned int tseg,
unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
unsigned int *sample_point_error_ptr)
{
@@ -35,8 +35,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
for (i = 0; i <= 1; i++) {
tseg2 = tseg + CAN_SYNC_SEG -
- (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
- 1000 - i;
+ (sp_origin * (tseg + CAN_SYNC_SEG)) / 1000 - i;
tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
tseg1 = tseg - tseg2;
if (tseg1 > btc->tseg1_max) {
@@ -46,9 +45,9 @@ can_update_sample_point(const struct can_bittiming_const *btc,
sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
(tseg + CAN_SYNC_SEG);
- sample_point_error = abs(sample_point_nominal - sample_point);
+ sample_point_error = abs(sp_origin - sample_point);
- if (sample_point <= sample_point_nominal &&
+ if (sample_point <= sp_origin &&
sample_point_error < best_sample_point_error) {
best_sample_point = sample_point;
best_sample_point_error = sample_point_error;
@@ -68,11 +67,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
{
struct can_priv *priv = netdev_priv(dev);
unsigned int bitrate; /* current bitrate */
- unsigned int bitrate_error; /* difference between current and nominal value */
+ unsigned int bitrate_error; /* difference between current and calculated value */
unsigned int best_bitrate_error = UINT_MAX;
- unsigned int sample_point_error; /* difference between current and nominal value */
+ unsigned int sample_point_error; /* difference between current and calculated value */
unsigned int best_sample_point_error = UINT_MAX;
- unsigned int sample_point_nominal; /* nominal sample point */
+ unsigned int sample_point;
unsigned int best_tseg = 0; /* current best value for tseg */
unsigned int best_brp = 0; /* current best value for brp */
unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
@@ -81,14 +80,14 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
/* Use CiA recommended sample points */
if (bt->sample_point) {
- sample_point_nominal = bt->sample_point;
+ sample_point = bt->sample_point;
} else {
if (bt->bitrate > 800 * KILO /* BPS */)
- sample_point_nominal = 750;
+ sample_point = 750;
else if (bt->bitrate > 500 * KILO /* BPS */)
- sample_point_nominal = 800;
+ sample_point = 800;
else
- sample_point_nominal = 875;
+ sample_point = 875;
}
/* tseg even = round down, odd = round up */
@@ -115,7 +114,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
if (bitrate_error < best_bitrate_error)
best_sample_point_error = UINT_MAX;
- can_update_sample_point(btc, sample_point_nominal, tseg / 2,
+ can_update_sample_point(btc, sample_point, tseg / 2,
&tseg1, &tseg2, &sample_point_error);
if (sample_point_error >= best_sample_point_error)
continue;
@@ -146,9 +145,8 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
}
/* real sample point */
- bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
- best_tseg, &tseg1, &tseg2,
- NULL);
+ bt->sample_point = can_update_sample_point(btc, sample_point, best_tseg,
+ &tseg1, &tseg2, NULL);
v64 = (u64)best_brp * 1000 * 1000 * 1000;
do_div(v64, priv->clock.freq);
--
2.51.0On 02.11.2025 23:01:22, Vincent Mailhol wrote:
> The functions can_update_sample_point() and can_calc_bittiming() are
> generic and meant to be used for both the nominal and the data
> bittiming calculation.
""There are 2 hard problems in computer science: cache invalidation,
naming things, and off-by-1 errors.""
Here it's naming things. Back in the days, in commit 7da29f97d6c8 ("can:
dev: can-calc-bit-timing(): better sample point calculation"), I wanted
to distinguish between the sample point the user requested and the
current sample point.
I was thinking about the signal that goes into a control loops, but at
university the lecture was in German, so I picked the wrong term. I
think "set point" or "reference value" are better terms.
> However, those functions use terminologies such as "bitrate nominal"
> or "sample point nominal". This is a leftover from when only Classical
> CAN was supported and now became incorrect.
>
> Remove or replace any occurrences of the word "nominal" with something
> more accurate.
What about replacing "nominal" with "reference"
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> ---
> drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 268ec6fa7c49..222117596704 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c
> @@ -24,7 +24,7 @@
> */
> static int
> can_update_sample_point(const struct can_bittiming_const *btc,
> - const unsigned int sample_point_nominal, const unsigned int tseg,
> + unsigned int sp_origin, unsigned int tseg,
Please don't remove the "const".
> unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
> unsigned int *sample_point_error_ptr)
> {
> @@ -35,8 +35,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>
> for (i = 0; i <= 1; i++) {
> tseg2 = tseg + CAN_SYNC_SEG -
> - (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
> - 1000 - i;
> + (sp_origin * (tseg + CAN_SYNC_SEG)) / 1000 - i;
> tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
> tseg1 = tseg - tseg2;
> if (tseg1 > btc->tseg1_max) {
> @@ -46,9 +45,9 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>
> sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
> (tseg + CAN_SYNC_SEG);
> - sample_point_error = abs(sample_point_nominal - sample_point);
> + sample_point_error = abs(sp_origin - sample_point);
>
> - if (sample_point <= sample_point_nominal &&
> + if (sample_point <= sp_origin &&
> sample_point_error < best_sample_point_error) {
> best_sample_point = sample_point;
> best_sample_point_error = sample_point_error;
> @@ -68,11 +67,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
> {
> struct can_priv *priv = netdev_priv(dev);
> unsigned int bitrate; /* current bitrate */
> - unsigned int bitrate_error; /* difference between current and nominal value */
> + unsigned int bitrate_error; /* difference between current and calculated value */
What about: "difference between reference and calculated value"
> unsigned int best_bitrate_error = UINT_MAX;
> - unsigned int sample_point_error; /* difference between current and nominal value */
> + unsigned int sample_point_error; /* difference between current and calculated value */
> unsigned int best_sample_point_error = UINT_MAX;
> - unsigned int sample_point_nominal; /* nominal sample point */
> + unsigned int sample_point;
> unsigned int best_tseg = 0; /* current best value for tseg */
> unsigned int best_brp = 0; /* current best value for brp */
> unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
> @@ -81,14 +80,14 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>
> /* Use CiA recommended sample points */
> if (bt->sample_point) {
> - sample_point_nominal = bt->sample_point;
> + sample_point = bt->sample_point;
> } else {
> if (bt->bitrate > 800 * KILO /* BPS */)
> - sample_point_nominal = 750;
> + sample_point = 750;
> else if (bt->bitrate > 500 * KILO /* BPS */)
> - sample_point_nominal = 800;
> + sample_point = 800;
> else
> - sample_point_nominal = 875;
> + sample_point = 875;
> }
>
> /* tseg even = round down, odd = round up */
> @@ -115,7 +114,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
> if (bitrate_error < best_bitrate_error)
> best_sample_point_error = UINT_MAX;
>
> - can_update_sample_point(btc, sample_point_nominal, tseg / 2,
> + can_update_sample_point(btc, sample_point, tseg / 2,
> &tseg1, &tseg2, &sample_point_error);
> if (sample_point_error >= best_sample_point_error)
> continue;
> @@ -146,9 +145,8 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
> }
>
> /* real sample point */
> - bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
> - best_tseg, &tseg1, &tseg2,
> - NULL);
> + bt->sample_point = can_update_sample_point(btc, sample_point, best_tseg,
> + &tseg1, &tseg2, NULL);
>
> v64 = (u64)best_brp * 1000 * 1000 * 1000;
> do_div(v64, priv->clock.freq);
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 12/11/2025 at 10:08, Marc Kleine-Budde wrote:
> On 02.11.2025 23:01:22, Vincent Mailhol wrote:
>> The functions can_update_sample_point() and can_calc_bittiming() are
>> generic and meant to be used for both the nominal and the data
>> bittiming calculation.
>
> ""There are 2 hard problems in computer science: cache invalidation,
> naming things, and off-by-1 errors.""
:D
> Here it's naming things. Back in the days, in commit 7da29f97d6c8 ("can:
> dev: can-calc-bit-timing(): better sample point calculation"), I wanted
> to distinguish between the sample point the user requested and the
> current sample point.
>
> I was thinking about the signal that goes into a control loops, but at
> university the lecture was in German, so I picked the wrong term. I
> think "set point" or "reference value" are better terms.
OK. Thanks for the clarification, now it makes more sense.
>> However, those functions use terminologies such as "bitrate nominal"
>> or "sample point nominal". This is a leftover from when only Classical
>> CAN was supported and now became incorrect.
>>
>> Remove or replace any occurrences of the word "nominal" with something
>> more accurate.
>
> What about replacing "nominal" with "reference"
Ack. I will also fully rewrite the patch description. The new title will become:
can: calc_bittiming: get rid of the confusing "nominal" wording
>> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
>> ---
>> drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++----------------
>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
>> index 268ec6fa7c49..222117596704 100644
>> --- a/drivers/net/can/dev/calc_bittiming.c
>> +++ b/drivers/net/can/dev/calc_bittiming.c
>> @@ -24,7 +24,7 @@
>> */
>> static int
>> can_update_sample_point(const struct can_bittiming_const *btc,
>> - const unsigned int sample_point_nominal, const unsigned int tseg,
>> + unsigned int sp_origin, unsigned int tseg,
>
> Please don't remove the "const".
I always considered it silly to tag a scalar parameter as const. Because C
passes the function argument by value, it is pretty meaningless. But I guess
this change is out of scope. I will restore this in the next version.
>> unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
>> unsigned int *sample_point_error_ptr)
>> {
>> @@ -35,8 +35,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>>
>> for (i = 0; i <= 1; i++) {
>> tseg2 = tseg + CAN_SYNC_SEG -
>> - (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
>> - 1000 - i;
>> + (sp_origin * (tseg + CAN_SYNC_SEG)) / 1000 - i;
>> tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
>> tseg1 = tseg - tseg2;
>> if (tseg1 > btc->tseg1_max) {
>> @@ -46,9 +45,9 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>>
>> sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
>> (tseg + CAN_SYNC_SEG);
>> - sample_point_error = abs(sample_point_nominal - sample_point);
>> + sample_point_error = abs(sp_origin - sample_point);
>>
>> - if (sample_point <= sample_point_nominal &&
>> + if (sample_point <= sp_origin &&
>> sample_point_error < best_sample_point_error) {
>> best_sample_point = sample_point;
>> best_sample_point_error = sample_point_error;
>> @@ -68,11 +67,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>> {
>> struct can_priv *priv = netdev_priv(dev);
>> unsigned int bitrate; /* current bitrate */
>> - unsigned int bitrate_error; /* difference between current and nominal value */
>> + unsigned int bitrate_error; /* difference between current and calculated value */
>
> What about: "difference between reference and calculated value"
Ack. Applied to next version.
Yours sincerely,
Vincent Mailhol
Hello again Vincent, (Sorry all, I can only slowly work through the list of emails from this weekend.) > >> static int > >> can_update_sample_point(const struct can_bittiming_const *btc, > >> - const unsigned int sample_point_nominal, const unsigned int > >> tseg, > >> + unsigned int sp_origin, unsigned int tseg, > > > > Please don't remove the "const". > I always considered it silly to tag a scalar parameter as const. > Because C > passes the function argument by value, it is pretty meaningless. The "const" attribute means that the parameter cannot be used on the left side of an assignment in its block. Even if this parameter is not an input/output parameter, without “const” it can be modified within the function, just like a local variable (which it is, since it is theoretically also declared on the stack). Explicitly stating “const” is a strong indicator that the value cannot be modified in the block imho. > Yours sincerely, > Vincent Mailhol > Best regards, -- Stéphane
On 17.11.2025 16:32:31, Stéphane Grosjean wrote: > > >> static int > > >> can_update_sample_point(const struct can_bittiming_const *btc, > > >> - const unsigned int sample_point_nominal, const unsigned int > > >> tseg, > > >> + unsigned int sp_origin, unsigned int tseg, > > > > > > Please don't remove the "const". > > I always considered it silly to tag a scalar parameter as const. > > Because C > > passes the function argument by value, it is pretty meaningless. > > The "const" attribute means that the parameter cannot be used on the > left side of an assignment in its block. Even if this parameter is not > an input/output parameter, without “const” it can be modified within > the function, just like a local variable (which it is, since it is > theoretically also declared on the stack). Explicitly stating “const” > is a strong indicator that the value cannot be modified in the block > imho. ...and this is exactly what I want to express, because it's the reference value. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 15.11.2025 12:14:31, Vincent Mailhol wrote: > >> --- > >> drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++---------------- > >> 1 file changed, 14 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c > >> index 268ec6fa7c49..222117596704 100644 > >> --- a/drivers/net/can/dev/calc_bittiming.c > >> +++ b/drivers/net/can/dev/calc_bittiming.c > >> @@ -24,7 +24,7 @@ > >> */ > >> static int > >> can_update_sample_point(const struct can_bittiming_const *btc, > >> - const unsigned int sample_point_nominal, const unsigned int tseg, > >> + unsigned int sp_origin, unsigned int tseg, > > > > Please don't remove the "const". > > I always considered it silly to tag a scalar parameter as const. Because C > passes the function argument by value, it is pretty meaningless. But I guess > this change is out of scope. I will restore this in the next version. Here they are both the reference values in the right format. I know I don't want to change them. So I've marked them as const, to document this. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
© 2016 - 2026 Red Hat, Inc.