Re: [PATCH v3 2/2] CAN: CAN driver to support multiple CAN bus on SPI interface

[Stefano Babic <sbabic@denx.de>]

Hi Oliver,

On 02/04/2014 19:21, Oliver Hartkopp wrote:
> Hello Stefano,
> 
> On 02.04.2014 09:18, Stefano Babic wrote:
> 
>> +
>> +/* CFG message */
>> +struct msg_cfg_data {
>> +	u8	channel;
>> +	u8	enabled;
>> +	struct can_bittiming bt;
>> +} __packed;
> 
> 
>> +static int spi_can_probe(struct spi_device *spi)
>> +{
> 
> (..)
> 
>> +
>> +		/* Set CAN specific parameters */
>> +		priv->can.clock.freq = SLAVE_CLK_FREQ;
>> +		priv->can.bittiming_const = NULL;
>> +		priv->can.do_set_mode = spi_can_set_mode;
>> +		priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>> +			CAN_CTRLMODE_LISTENONLY	| CAN_CTRLMODE_3_SAMPLES;
> 
> This looks wrong to me.
> 
> In the one case (ctrlmode_supported) you aim to know about the SPI CAN
> controllers e.g. if they support LISTENONLY.
> 
> In the other case (bittiming_const) you don't know anything about the
> controller??

I agreee, this is wrong and must be fixed.

> 
> How do you set the bitrate without defining bittiming_const??
> And how does
> 
> 	ip -det link show can0
> 
> look like for your device then?

Only to check what is happening:

5: hcan2: <NOARP> mtu 16 qdisc noop state DOWN qlen 10
    link/can
    can state STOPPED restart-ms 0
    bitrate 0 sample-point 0.000
    tq 0 prop-seg 0 phase-seg1 0 phase-seg2 0 sjw 0
    clock 100000000

Everything is zeroed...


> 
> Setting the bitrate with the ip tools requires bittiming_const to be set:
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/can/dev.c#n252
> 

In fact I had supposed that setting the bitrate generates an OOPS in
can_calc_bittiming(). It does not, I have not checked why...in any case
the behavior must be fixed.

> IMO both ctrlmode_supported and bittiming_const need to be retrieved from
> each remote SPI attached CAN controller at startup initialization time.
> So that the configuration can be done with the standard tools.

Agree, it is the best way. I will add a SPI_MSG_GET_CFG to retrieve the
timing from the controller in probe(). In my case, the controller cannot
have different timing for each channel, but I agree the query must be
done for each channel, in case a microcontroller supports it.

Thanks for reviewing,

Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================