Re: [PATCH v2] can: add Renesas R-Car CAN driver

[Wolfgang Grandegger <wg@grandegger.com>]

On 11/01/2013 11:24 PM, Sergei Shtylyov wrote:
> On 10/25/2013 11:28 PM, Wolfgang Grandegger wrote:
> 
>>> Add support for the CAN controller found in Renesas R-Car SoCs.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
>>> Index: linux-can-next/drivers/net/can/rcar_can.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-can-next/drivers/net/can/rcar_can.c
>>> @@ -0,0 +1,920 @@
> [...]
>>> +static bool autorecovery;
>>> +module_param(autorecovery, bool, 0644);
>>> +MODULE_PARM_DESC(autorecovery, "Automatic hardware recovery from
>>> bus-off");
> 
>> Software recovery is the preferred solution. No need to support
>> automatic recovery by the hardware.
> 
>    OK, removed it.
> 
>>> +/* Mailbox registers structure */
>>> +struct rcar_can_mbox_regs {
>>> +    u32 id;        /* IDE and RTR bits, SID and EID */
>>> +    u8 stub;    /* Not used */
>>> +    u8 dlc;        /* Data Length Code - bits [0..3] */
>>> +    u8 data[8];    /* Data Bytes */
>>> +    u8 tsh;        /* Time Stamp Higher Byte */
>>> +    u8 tsl;        /* Time Stamp Lower Byte */
> 
>> I would add padding bytes here to ensure alignment.
> 
>    What padding? This is how the hardware registers are laid out. I
> think I should rather add __packed after }.

OK, I was confused that data does not start on a 4 byte boundary.


>>> +            rcar_can_writeb(priv, RCAR_CAN_ECSR, (u8)~ECSR_ADEF);
> 
>> Please avoid casts here and below.
> 
>    These casts help avoid compiler warnings.

Well instead of using casts the declaration of ECSR_ADEF and otheres
should be fixed. I think the problem is that BIT is declared as shown below:

 #define BIT(nr)                 (1UL << (nr))

Using BIT seems not appropriate here.

Wolfgang,