Re: [PATCH RFC 1/3] can: calc_bittiming: get rid of the incorrect "nominal" word

[Vincent Mailhol <mailhol@kernel.org>]

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