RE: [PATCH 2/5] can: bittiming: can_calc_bittiming(): make use of min3()

[<Thomas.Kopp@microchip.com>]

> > In can_calc_bittiming() there are several open coded checks to ensure
> > that SJW is within certain limits. Replace this by a single call to
> > min3().
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/can/dev/calc_bittiming.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/calc_bittiming.c
> b/drivers/net/can/dev/calc_bittiming.c
> > index d3caa040614d..ce6bef2444a2 100644
> > --- a/drivers/net/can/dev/calc_bittiming.c
> > +++ b/drivers/net/can/dev/calc_bittiming.c
> > @@ -158,12 +158,8 @@ int can_calc_bittiming(const struct net_device
> *dev, struct can_bittiming *bt,
> >         if (!bt->sjw || !btc->sjw_max) {
> >                 bt->sjw = 1;
> >         } else {
> > -               /* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
> > -               if (bt->sjw > btc->sjw_max)
> > -                       bt->sjw = btc->sjw_max;
> > -               /* bt->sjw must not be higher than tseg2 */
> > -               if (tseg2 < bt->sjw)
> > -                       bt->sjw = tseg2;
> > +               /* sjw must not be higher than sjw_max and tseg2 */
> > +               bt->sjw = min3(bt->sjw, btc->sjw_max, tseg2);
> 
> Not directly a criticism of this patch (as things were already like
> that), but if the user provides an incorrect value for SJW (or any
> other bittiming argument), wouldn't it be better to inform? Returning
> -EINVAL might be too violent. Maybe a dmesg would be good?

I'd say it would be consistent to keep returning -EINVAL (at least for the SJW>(min p1,p2)) condition. The same is done for FD Bitrate < Arbitration bitrate and both conditions have the same level of justification in the ISO 11898-1:2015
"The data bit time shall have the same length as the nominal bit time or it shall be shorter than the nominal bit time."

"In nominal bit time and in data bit time, SJW shall be less than or equal to the minimum of these two items: Phase_Seg1 and Phase_Seg2."

Shall is a binding requirement to be ISO conformant in this case.

Best Regards,
Thomas