Re: [PATCH v2 3/4] can: c_can: Move overlay structure to array with offset as index

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

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

On 05/11/2012 05:23 PM, AnilKumar, Chimata wrote:
> On Fri, May 11, 2012 at 20:10:48, Wolfgang Grandegger wrote:
>> On 05/11/2012 01:09 PM, AnilKumar, Chimata wrote:
>>> On Fri, May 11, 2012 at 01:42:43, Marc Kleine-Budde wrote:
>>>> On 05/10/2012 01:34 PM, AnilKumar Ch wrote:
>>>>> c_can uses overlay structure for accessing c_can module registers.
>>>>> With this kind of implementation it is difficult to add one more ip
>>>>> which is similar to c_can in functionality but different register
>>>>> offsets.
>>>>>
>>>>> This patch changes the overlay structure implementation to an array
>>>>> with register offset as index. This way we can overcome the above
>>>>> limitation.
>>>>
>>>> The array index implementation looks very nice.
>>>>
>>>> I suggest to use the enum instead of a plain "int reg" in the
>>>> c_can_read_* function arguments.
>>>
>>> That is better compared to int reg, I will change.
>>>
>>>>
>>>> General question: What happend to "iface", like in this hunk below?
>>>
>>> In entire driver iface is used as "0" means IF register bank one. So I defined
>>> enum like "C_CAN_IF1_*". If IF register bank 2 is used in the driver at that
>>> time we can define "C_CAN_IF2_*" and used appropriately.
>>
>> But switching IF1->IF2 would then be awkward. Anyway, at the moment
>> switching seems not required and therefore there is no need to introduce
>> another array but...
>>
>>>
>>>>
>>>>>  	while (count && priv->read_reg(priv,
>>>>> -				&priv->regs->ifregs[iface].com_req) &
>>>>> +				C_CAN_IF1_COMREQ_REG) &
>>>>>  				IF_COMR_BUSY) {
>>
>> ... what is then "iface" still good for? I see that the functions still
>> have that argument.
>>
>> Apart from that the patch series look quite good now.
> 
> How about this implementation?
> 
> 1. C_CAN_IF2_* will be added to enum next to C_CAN_IF1_* register flags
> 
> 2. Add array fields for C_CAN_IF2_* with coresponding offsets.
> 
> 3. #define IF_ENUM_REG_LEN	11 (eleven interface registers are present)
> 
> 4. priv->read_reg(priv, C_CAN_IF1_COMREQ_REG + iface * IF_ENUM_REG_LEN);
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

make this a MACRO, taking iface as an argument

> Ultimately 11 * 3 + 1 + few more lines will be added to the patch.

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