Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings

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

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

On 11/09/2010 01:26 PM, Tomoya MORINAGA wrote:
>> It's just a question if the driver for an Intel Chipset should/could ignore
>> the endian problematic.
>>
>> If this is intended the driver should only appear in Kconfig depending on X86
>> or little endian systems.
> 
> This driver is for only x86 processor.
> I have no intention to support processor except x86.

Fair enough, if you do the endianess conversion correct, either with my
proposed cpu_to_be16 or with the simpler and probably unnoticeable
slower version from Oliver. With Olivers version it's a bit harder to
build a loop that just copies the number of bytes specified in dlc.

>> Besides this remark, the struct pch_can_regs could also be defined in a way
>> that every single CAN payload data byte could be accessed directly to allow
>> e.g. this code in pch_can_rx_normal():
>>
>> cf->data[0] = ioread8(&priv->regs->if1_data0);
>> cf->data[1] = ioread8(&priv->regs->if1_data1);
>> cf->data[2] = ioread8(&priv->regs->if1_data2);
>> (..)
>> cf->data[6] = ioread8(&priv->regs->if1_data6);
>> cf->data[7] = ioread8(&priv->regs->if1_data7);
>>
>> This is easy to understand and additionally endian aware.
> 
> I agree. Thease codes are very simple.
> I will modify like above.
> 
>>
>> In opposite to this:
>>
>> + if (cf->can_dlc > 2) {
>> + u32 data1 = *((u16 *)&cf->data[2]);
>> + iowrite32(data1, &priv->regs->if2_dataa2);
>> + }
>>
>> Which puts a little? endian u16 into the big endian register layout.
>> Sorry i'm just getting curious on this register access implementation.
> 
> I think cf->data is like below
> MSB----------LSB
> D3-D2-D1-D0
> D7-D6-D5-D4

No, cf->data is an array of 8 u8, so it looks this way:

D0 D1 D2 D3  D4 D5 D6 D7

> *((u16 *)&cf->data[2]) means "D3-D2".

No, it's D2 D3. This is why you need the endianess conversion.

> This order coprresponds to register order.
> data register is like below
> MSB----------LSB
> **-**-D3-D2
> 
> ** means reserved area.

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