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

[Stefano Babic <sbabic@denx.de>]

Hi Oliver,

On 21/03/2014 22:42, Oliver Hartkopp wrote:
> Hello Stephano,
> 
> I definitely like this driver as it implements a full featured CAN netdev over
> SPI with all the SocketCAN specific configuration stuff :-)
> 

Thanks !

> First some general comments:
> 
> There's a mixup of the driver name:
> 
> spican
> spi_can
> #define DRV_NAME		"imxhcscan"
> 
>>  drivers/net/can/Kconfig             |   11 +
>>  drivers/net/can/Makefile            |    1 +
>>  drivers/net/can/spi_can.c           | 1533 +++++++++++++++++++++++++++++++++++
>>  include/linux/can/platform/spican.h |   36 +
> 
> I would first suggest to create a new directory
> 
> linux/drivers/net/can/spi

Understood - I'll do for V2.

> 
> and move the mcp251x there (including the Kconfig stuff which is only entered
> when SPI is enabled similar to the CAN USB adapter configs).
> 
> As the driver and it's communication concept is not very specific for the
> hcs12 or imx35 (or should no be very specific) I would tend to name it
> 
> 	spi_can
> 
> and rename the 'hcs12' or 'imx' named functions to something more generic,
> e.g. spi_slave, spi_can or something better.

Right.

> 
> Maybe when there is really imx35 or hcs12 specific stuff to handle, this
> should be made clear, e.g. with a imx_spi_can driver which uses the spi_can
> driver?!?

There is nothing really specific to these two processors. I will check
all names to be consistent.

> 
> And then there has to be a Kconfig option which depends on IMX35/ARM.
> 
> 
>> +#ifndef __CAN_PLATFORM_SPICAN_H__
>> +#define __CAN_PLATFORM_SPICAN_H__
>> +
>> +#include <linux/spi/spi.h>
>> +
>> +/*
>> + * struct hcs12_imx_platform_data - i.MX to HCS12 CAN controller platform data
>> + */
>> +
>> +struct hcs12_imx_platform_data {
>> +	unsigned int can_channels;
>> +	unsigned int gpio;
>> +	unsigned int active;
>> +	unsigned int slow_freq;
>> +};
>> +
>> +#endif
>>
> 
> Is this really i.MX or HCS12 specific??

It is neither i.MX35 nor HCS12 specific. I will drop both names in next
version.

Thanks for reviewing,
Stefano



-- 
=====================================================================
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
=====================================================================