[U-Boot] [PATCH v2 02/21] pmic:i2c: Add I2C byte order to PMIC framework

[Stefano Babic <sbabic@denx.de>]

On 09/10/2012 11:52, Lukasz Majewski wrote:
> Hi Stefano,
> 
>> On 05/10/2012 10:16, Lukasz Majewski wrote:
>>> Since the pmic_reg_read is the u32 value, the order in which bytes
>>> are placed to form u32 value is important.
>>>
>>> This commit adds the reverse (which is default) and normal byte
>>> order.
>>>
>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>
>>> ---
>>
>> Hi Lucasz,
>>
>>> Changes for v2:
>>> - Move byte_order variable to struct pmic
>>
>> Is the byte reversal you introduce here only another way for the
>> endianess ?
>>
>> It seems to me you have a PMIC whose endianess is different as the
>> SOC's endianess, and that must be changed.
>>
>> I can suppose that "normal byte" and "reverse byte" works only with
>> ARM SOC, as they are (normally, but not always) little endian, but
>> not for example on PowerPC (big endian).
> 
> This procedure was necessary since, some sensor produces output of 2B
> and those bytes are in a big endian.

Right. So we have the endianess of the SOC and the endianess of the
PMIC, and they can differ.

> For simplicity reason I decided to make the switch on received bytes.
> 
> I suppose that witch some luck :-), proper cpu_to_le16|be16 (le32|be32)
> functions can be used to avoid repetition.

I hope so.

> 
> To make the code working at both ARM and PPC we shall use the 
> __BYTE_ORDER == __LITTLE_ENDIAN check, which is CPU dependent. Hence,
> the interpretation of stored data would be consistent from CPU point of
> view.
> 
> To sum up - two options possible:
> 1. Use cpu_to_le|be functions (hack cpu_to_le32 to handle 3B input) 
> 2. Perform switch (as it was done in this patch) with explicit check of 
> __BYTE_ORDER == __LITTLE_ENDIAN.
> 
> I would opt for 1 here.

I vote for 1, too. I think generally in u-boot and kernel there is no
use of the explicit check, but I have not grepped in code.

> 
>>
>> If this is correct, then I think we need a parameter in the structure
>> to indicate the endianess, 
> 
> We need also the endiness of data coming out from sensor as it is
> defined in this patch:
> enum { PMIC_BYTE_ORDER_REVERSED, PMIC_BYTE_ORDER_NORMAL, };
> I admit, that name could be more appropriate here (like
> SENSOR_BYTE_ORDER).

Yes. The name is misleading. Which is "normal" nowadays ?


> 
> In this patch I've added a "byte_order" variable to struct pmic to
> indicate the endiantess of device (I2C connected) to which we are
> talking to. It is per sensor defined (as the struct pmic is)

I think the procedure is correct. The endianess is stored in the pmic
structure, and then calling appropriate cpu_to_le/be function we should
have the bytes in the right order.

> 
> Please feel free to comment other patches, so I could prepare v3 of
> this series :-). 

I will do..

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================