Re: [PATCH 3/3] can: c_can: Add support for Bosch D_CAN controller

[Wolfgang Grandegger <wg@grandegger.com>]

On 04/24/2012 10:36 AM, Marc Kleine-Budde wrote:
> On 04/24/2012 10:31 AM, Wolfgang Grandegger wrote:
>> On 04/24/2012 10:10 AM, Marc Kleine-Budde wrote:
>>> On 04/20/2012 11:58 AM, AnilKumar Ch wrote:
>>>> This patch adds the support for D_CAN controller driver to the existing
>>>> C_CAN driver.
>>>>
>>>> Bosch D_CAN controller is a full-CAN implementation which is compliant
>>>> to CAN protocol version 2.0 part A and B. Bosch D_CAN user manual can be
>>>> obtained from: http://www.semiconductors.bosch.de/media/en/pdf/
>>>> ipmodules_1/can/d_can_users_manual_111.pdf
>>>>
>>>> The following are the design choices made while adding D_CAN to C_CAN
>>>> driver:
>>>> A new overlay structure is added for d_can controller and care is taken
>>>> to make sure its member names match with equavalent c_can structure
>>>> members (even if the d_can specification calls them slightly differently).
>>>> Note that d_can controller has more registers, so structure members of
>>>> d_can are more than those in c_can.
>>>>
>>>> A new set if read/write macros are used to access the registers common
>>>> between c_can and d_can. To get the basic d_can functionality working
>>>> it is sufficient to access just these registers.
>>>
>>> I don't like macros. I've two further possible solutions:
>>
>> Yes, I don't like that part either, also because of the
>>
>>   "if (priv->dev_type == DEV_TYPE_D_CAN)"
>>
>> for each read/write access.
>>
>>> a) Access the registers via an array. The array index is a "virtual"
>>>    register, the array's value the physical offset within the c_can or
>>>    d_can controller.
>>
>> I was thinking about that as well but using absolute addresses. This
>> would avoid further calculations for 16/32 bit aligned accesses.
> 
> Yes, this way we might get rid of this calculation, too.
> 
>>> b) AFAICS you need more than three registers to get the CAN core
>>>    working. Another possibility is to implement an accessor function
>>>   for each register.
>>
>> ... offsetof() might be useful for this approach.
> 
> Please elaborate.

#define c_can_write_reg_func(member) \
	static inline void write_##reg(struct c_can_priv *priv, u32 val) \
	{ \
		priv->write_reg(priv, offsetof(struct c_can_regs, member), val); \
	}

And similar for read and d_can. Then in the init part:

	if (priv->dev_type == DEV_TYPE_C_CAN) {
		c_can_write_reg_func(control);
		c_can_write_reg_func(status);
		...
	} else {
		d_can_write_reg_func(control);
		d_can_write_reg_func(status);
		...
	}

Maybe we could even get rid of priv->write_reg (for memory mapped i/o only!).

Wolfgang.