Re: [PATCH v3] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Ramesh,

two more questions from my side:

On 03/15/2016 10:48 AM, Ramesh Shanmugasundaram wrote:

> +config CAN_RCAR_CANFD
> +	tristate "Renesas R-Car CAN FD controller"
> +	depends on ARCH_RENESAS || ARM
> +	---help---
> +	  Say Y here if you want to use CAN FD controller found on
> +	  Renesas R-Car SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called rcar_canfd.

Would it make sense to add some more documentation here that this 
controller is a CANFD-only CAN controller which (of course) can cope 
with CAN2.0B frames but has no dedicated CAN2.0B mode?

(..)

> +static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch)
> +{

(..)

> +
> +	priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
> +	priv->can.data_bittiming_const =
> +		&rcar_canfd_data_bittiming_const;
> +
> +	/* Controller starts in CAN FD only mode. In this mode, to interoperate
> +	 * with classical CAN only nodes, data bitrate should be configured the
> +	 * same as nominal bitrate.
> +	 */

'Should' sounds like a pretty soft requirement here - and this info 
should not hide deep in the code IMO.

What happens if the user only defines:

	"ip link set can0 up type can bitrate 1000000"

instead of

	"ip link set can0 up type can bitrate 1000000 dbitrate 1000000"

??

Would it make sense to set the data bitrate according to the arbitration 
bitrate in rcar_canfd_set_bittiming() or sanitize this at some other 
place when not data bitrate is given by the netlink command?

Regards,
Oliver