On 08/09/2016 04:17 PM, Stephane Grosjean wrote:
> Hi all,
> 
> With the new way of computing bit-timings introduced in linux-can-next, 
> when CONFIG_CAN_CALC_BITTIMING is defined, I think there's a chance to 
> set 0 in "bt->sample_point" of a can device. This is a different 
> behavior than with the previous versions.
> 
> IMHO, there are cases where "can_update_sample_point()" might return 0 
> ("best_sample_point" is initialized to 0). There is no real damage in 
> the main loop of "can_calc_bittiming()": its returned value is ignored 
> (and only *sample_point_error matters). But it is not, outside this 
> loop, when setting the (real) value of the sample_point. So I think that 
> the real value of the sample_point must be computed using the final 
> values of tseg and tseg2, something like:
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 7188137..de422b3 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -200,8 +200,8 @@ static int can_calc_bittiming(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 = 1000 * (best_tseg + CAN_CALC_SYNC_SEG - tseg2) /
> +                                       (best_tseg + CAN_CALC_SYNC_SEG);

You're using tseg2 as an input, where before it was used as an output
value. As the sample point has already been calculated in the loop, why
should it not be possible to calculate it again?

> 
>          v64 = (u64)best_brp * 1000 * 1000 * 1000;
>          do_div(v64, priv->clock.freq);
> 
> The best would be to introduce a function that implements the formula...

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   |