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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

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.

>> Other, hopefully better, solutions are open for discussion.
> 
> The solutions above are not really elegant, but so far I do not have
> better ideas.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]