On 01/12/2011 04:30 AM, Bhupesh SHARMA wrote:
> Hi Wolfgang,
> 
>> -----Original Message-----
>> From: Wolfgang Grandegger [mailto:[email protected]]
>> Hi Bhupesh,
>>
>> some final nitpicking as you need to fix Marc remarks anyway...
>>
>> On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
>>> Bosch C_CAN controller is a full-CAN implementation which is
>> compliant
>>> to CAN protocol version 2.0 part A and B. Bosch C_CAN user manual can
>> be
>>> obtained from:
>>> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/
>>> c_can/users_manual_c_can.pdf
>>>
>>> This patch adds the support for this controller.
>>> The following are the design choices made while writing the
>> controller
>>> driver:
>>> 1. Interface Register set IF1 has be used only in the current design.
>>> 2. Out of the 32 Message objects available, 16 are kept aside for RX
>>>    purposes and the rest for TX purposes.
>>> 3. NAPI implementation is such that both the TX and RX paths function
>>>    in polling mode.
>>>
>>> Changes since V3:
>>> 1. Corrected commenting style.
>>> 2. Removing *non-required* header files from driver files.
>>> 3. Removed extra *non-required* outer brackets globally.
>>> 4. Implemented and used a inline routine to check BUSY status.
>>> 5. Added a board-specific param *priv* inside the c_can_priv struct.
>>>
>>> Signed-off-by: Bhupesh Sharma <[email protected]>
>>
>> ...
>>> +++ b/drivers/net/can/c_can/c_can.h
>>> @@ -0,0 +1,235 @@
>>> +/*
>>> + * CAN bus driver for Bosch C_CAN controller
>>> + *
>>> + * Copyright (C) 2010 ST Microelectronics
>>> + * Bhupesh Sharma <[email protected]>
>>> + *
>>> + * Borrowed heavily from the C_CAN driver originally written by:
>>> + * Copyright (C) 2007
>>> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
>> <[email protected]>
>>> + * - Simon Kallweit, intefo AG <[email protected]>
>>> + *
>>> + * TX and RX NAPI implementation has been borrowed from at91 CAN
>> driver
>>> + * written by:
>>> + * Copyright
>>> + * (C) 2007 by Hans J. Koch <[email protected]>
>>> + * (C) 2008, 2009 by Marc Kleine-Budde <[email protected]>
>>> + *
>>> + * Bosch C_CAN controller is compliant to CAN protocol version 2.0
>> part A and B.
>>> + * Bosch C_CAN user manual can be obtained from:
>>> + *
>> http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/c_can/
>>> + * users_manual_c_can.pdf
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + */
>>> +
>>> +#ifndef C_CAN_H
>>> +#define C_CAN_H
>>> +
>>> +/* control register */
>>> +#define CONTROL_TEST               BIT(7)
>>> +#define CONTROL_CCE                BIT(6)
>>> +#define CONTROL_DISABLE_AR BIT(5)
>>> +#define CONTROL_ENABLE_AR  (0 << 5)
>>> +#define CONTROL_EIE                BIT(3)
>>> +#define CONTROL_SIE                BIT(2)
>>> +#define CONTROL_IE         BIT(1)
>>> +#define CONTROL_INIT               BIT(0)
>>> +
>>> +/* test register */
>>> +#define TEST_RX                    BIT(7)
>>> +#define TEST_TX1           BIT(6)
>>> +#define TEST_TX2           BIT(5)
>>> +#define TEST_LBACK         BIT(4)
>>> +#define TEST_SILENT                BIT(3)
>>> +#define TEST_BASIC         BIT(2)
>>> +
>>> +/* status register */
>>> +#define STATUS_BOFF                BIT(7)
>>> +#define STATUS_EWARN               BIT(6)
>>> +#define STATUS_EPASS               BIT(5)
>>> +#define STATUS_RXOK                BIT(4)
>>> +#define STATUS_TXOK                BIT(3)
>>> +#define STATUS_LEC_MASK            0x07
>>> +
>>> +/* error counter register */
>>> +#define ERR_COUNTER_TEC_MASK       0xff
>>> +#define ERR_COUNTER_TEC_SHIFT      0
>>> +#define ERR_COUNTER_REC_SHIFT      8
>>> +#define ERR_COUNTER_REC_MASK       (0x7f << ERR_COUNTER_REC_SHIFT)
>>> +#define ERR_COUNTER_RP_SHIFT       15
>>> +#define ERR_COUNTER_RP_MASK        (0x1 << ERR_COUNTER_RP_SHIFT)
>>
>> s/ERR_COUNTER/ERR_CNT/ to match the member name.
> 
> Ok.
> 
>>> +
>>> +/* bit-timing register */
>>> +#define BTR_BRP_MASK               0x3f
>>> +#define BTR_BRP_SHIFT              0
>>> +#define BTR_SJW_SHIFT              6
>>> +#define BTR_SJW_MASK               (0x3 << BTR_SJW_SHIFT)
>>> +#define BTR_TSEG1_SHIFT            8
>>> +#define BTR_TSEG1_MASK             (0xf << BTR_TSEG1_SHIFT)
>>> +#define BTR_TSEG2_SHIFT            12
>>> +#define BTR_TSEG2_MASK             (0x7 << BTR_TSEG2_SHIFT)
>>> +
>>> +/* brp extension register */
>>> +#define BRP_EXT_BRPE_MASK  0x0f
>>> +#define BRP_EXT_BRPE_SHIFT 0
>>> +
>>> +/* IFx command request */
>>> +#define IF_COMR_BUSY               BIT(15)
>>> +
>>> +/* IFx command mask */
>>> +#define IF_COMM_WR         BIT(7)
>>> +#define IF_COMM_MASK               BIT(6)
>>> +#define IF_COMM_ARB                BIT(5)
>>> +#define IF_COMM_CONTROL            BIT(4)
>>> +#define IF_COMM_CLR_INT_PND        BIT(3)
>>> +#define IF_COMM_TXRQST             BIT(2)
>>> +#define IF_COMM_DATAA              BIT(1)
>>> +#define IF_COMM_DATAB              BIT(0)
>>> +#define IF_COMM_ALL                (IF_COMM_MASK | IF_COMM_ARB | \
>>> +                           IF_COMM_CONTROL | IF_COMM_TXRQST | \
>>> +                           IF_COMM_DATAA | IF_COMM_DATAB)
>>> +
>>> +/* IFx arbitration */
>>> +#define IF_ARB_MSGVAL              BIT(15)
>>> +#define IF_ARB_MSGXTD              BIT(14)
>>> +#define IF_ARB_TRANSMIT            BIT(13)
>>> +
>>> +/* IFx message control */
>>> +#define IF_MCONT_NEWDAT            BIT(15)
>>> +#define IF_MCONT_MSGLST            BIT(14)
>>> +#define IF_MCONT_CLR_MSGLST        (0 << 14)
>>> +#define IF_MCONT_INTPND            BIT(13)
>>> +#define IF_MCONT_UMASK             BIT(12)
>>> +#define IF_MCONT_TXIE              BIT(11)
>>> +#define IF_MCONT_RXIE              BIT(10)
>>> +#define IF_MCONT_RMTEN             BIT(9)
>>> +#define IF_MCONT_TXRQST            BIT(8)
>>> +#define IF_MCONT_EOB               BIT(7)
>>> +#define IF_MCONT_DLC_MASK  0xf
>>> +
>>> +/*
>>> + * IFx register masks:
>>> + * allow easy operation on 16-bit registers when the
>>> + * argument is 32-bit instead
>>> + */
>>> +#define IFX_WRITE_LOW_16BIT(x)     ((x) & 0xFFFF)
>>> +#define IFX_WRITE_HIGH_16BIT(x)    (((x) & 0xFFFF0000) >> 16)
>>> +
>>> +/* message object split */
>>> +#define C_CAN_NO_OF_OBJECTS        31
>>> +#define C_CAN_MSG_OBJ_RX_NUM       16
>>> +#define C_CAN_MSG_OBJ_TX_NUM       16
>>> +
>>> +#define C_CAN_MSG_OBJ_RX_FIRST     0
>>> +#define C_CAN_MSG_OBJ_RX_LAST      (C_CAN_MSG_OBJ_RX_FIRST + \
>>> +                           C_CAN_MSG_OBJ_RX_NUM - 1)
>>> +
>>> +#define C_CAN_MSG_OBJ_TX_FIRST     (C_CAN_MSG_OBJ_RX_LAST + 1)
>>> +#define C_CAN_MSG_OBJ_TX_LAST      (C_CAN_MSG_OBJ_TX_FIRST + \
>>> +                           C_CAN_MSG_OBJ_TX_NUM - 1)
>>> +
>>> +#define C_CAN_MSG_OBJ_RX_SPLIT     8
>>> +#define C_CAN_MSG_RX_LOW_LAST      (C_CAN_MSG_OBJ_RX_SPLIT - 1)
>>> +
>>> +#define C_CAN_NEXT_MSG_OBJ_MASK    (C_CAN_MSG_OBJ_TX_NUM - 1)
>>> +#define RECEIVE_OBJECT_BITS        0x0000ffff
>>> +
>>> +/* status interrupt */
>>> +#define STATUS_INTERRUPT   0x8000
>>> +
>>> +/* global interrupt masks */
>>> +#define ENABLE_ALL_INTERRUPTS      1
>>> +#define DISABLE_ALL_INTERRUPTS     0
>>> +
>>> +/* minimum timeout for checking BUSY status */
>>> +#define MIN_TIMEOUT_VALUE  6
>>> +
>>> +/* napi related */
>>> +#define C_CAN_NAPI_WEIGHT  C_CAN_MSG_OBJ_RX_NUM
>>> +
>>> +/* c_can IF registers */
>>> +struct c_can_if_regs {
>>> +   u16 com_reg;
>>> +   u16 com_mask;
>>> +   u16 mask1;
>>> +   u16 mask2;
>>> +   u16 arb1;
>>> +   u16 arb2;
>>> +   u16 msg_cntrl;
>>> +   u16 data[4];
>>> +   u16 _reserved[13];
>>> +};
>>> +
>>> +/* c_can hardware registers */
>>> +struct c_can_regs {
>>> +   u16 control;
>>> +   u16 status;
>>> +   u16 err_cnt;
>>> +   u16 btr;
>>> +   u16 interrupt;
>>> +   u16 test;
>>> +   u16 brp_ext;
>>> +   u16 _reserved1;
>>> +   struct c_can_if_regs intf[2]; /* [0] = IF1 and [1] = IF2 */
>>
>> I not happy with the name "intf". Sounds more like an interrupt related
>> property. Above you already use the prefix IF_ for the corresponding
>> macro definitions and somewhere else the name "iface".
> 
> I tried using the name *if* suggested by you here but the compiler will 
> complain
> considering it as a usage of if-construct. Do you have a better name that we
> can use here?

Oh, I was not aware ot that. Sorry for the noise! Then your old "ifregs"
or "iface" would be fine, I think. I just see that the pch_can uses
"ifregs" as well.

Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core

Reply via email to