RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for BoschD_CAN controller

[Bhupesh SHARMA]

> -----Original Message-----
> From: linux-can-owner@xxxxxxxxxxxxxxx [mailto:linux-can-
> owner@xxxxxxxxxxxxxxx] On Behalf Of AnilKumar, Chimata
> Sent: Tuesday, April 03, 2012 9:28 PM
> To: Marc Kleine-Budde
> Cc: Wolfgang Grandegger; socketcan@xxxxxxxxxxxx; m.kleine-
> budde@xxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Gole, Anant; Nori, Sekhar
> Subject: RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for
> Bosch D_CAN controller
> 
> On Tue, Apr 03, 2012 at 21:03:40, Marc Kleine-Budde wrote:
> > On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote:
> > >>>> Please explain why this CAN controller cannot be handled by the
> existing
> > >>>> C_CAN driver, eventually with some extensions. The register
> layout seems
> > >>>> almost identical, at least.
> > >>>>
> > >>>> Wolfgang.
> > >>>>
> > >>>
> > >>> These are the some of the pointers I can say, why I had gone for
> separate
> > >>> file instead of existing driver:
> > >>> * In case of D_CAN driver we can see all the registers are 32bit
> length
> > >>>   but in case of C_CAN registers are in 16bit length.
> > >>
> > >> How many bits in these 32 bit registers are used?
> > >
> > > In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used
> all the
> > > bits, in some cases used few bits.
> > >
> > > Roughly I can say that its (higher 16bits) % of usages is similar
> as compare
> > > to 16bits
> > >
> > > While checking the status of TXRequest registers and INT pending
> register,
> > > which is a hot code path, we have to put if checks for register
> access.
> >
> > The c_can already has a c_can_read_reg32() function. It's for example
> > used in the rx_poll function. You can make it a function pointer
> (i.e.
> > pric->read_reg32()) for easy abstraction.
> 
> This won't fit for D_CAN case because offsets are different in c_can
> compared
> to d_can. For example if I read CONTROL_REG register (0x0) in case of
> d_can,
> which will read only control register. In case of c_can it will read
> CONTROL_REG + STATUS register values in single read

C_CAN core has both 16-bit as well as 32-bit registers.
While the registers like NEWDATA and INTPND are 32-bit the others
like Control and Status are 16-bit.

These cases are handled by the present C_CAN driver already.
If you will see the C_CAN driver then you will see that the normal
read/write operations are 16-bit whereas a special variant is provided
for 32-bit access.

Are you sure you cannot use them as it is?

> >
> > >>> * Some of the registers, bit masks are different, so we have to
> add
> > >>>   checks on every API for differentiating the kind of device
> > >>
> > >> Which registers are this? Can you give us an example?
> > >
> > > I am pointing out some of the resisters
> > > * Single registers in case of D_CAN but multiple register in case
> of C_CAN
> > >   So masks will change MASK, ARB, INTPND
> > > * D_CAN_IFCMD is the combination of COMM request and COMM mask
> registers
> >
> > Maybe you can use the read_reg32 function on both c_can and d_can.
> 
> Above comment applies here as well
> 

This can be easily handled. I have worked on several drivers that do that.
See for example the STMPE multi-function device driver (MFD), here [1]

There are various variants of STMPE devices (like STMPE801 etc..) and they
can be handled by using a common driver and passing different platform data
intelligently that provides specific handling for a specific STMPE device.

Regards,
Bhupesh

[1] http://lxr.linux.no/linux+v3.3.1/drivers/mfd/stmpe.c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/