On 05/11/2012 01:09 PM, AnilKumar, Chimata wrote:

On Fri, May 11, 2012 at 01:42:43, Marc Kleine-Budde wrote:

quoted

On 05/10/2012 01:34 PM, AnilKumar Ch wrote:

quoted

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.

quoted

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

quoted

quoted

 	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.

Wolfgang.