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

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>

---
Changes for v2:
- Move byte_order variable to struct pmic
---
 drivers/misc/pmic_i2c.c |   31 ++++++++++++++++++++++++-------
 include/pmic.h          |    2 ++
 2 files changed, 26 insertions(+), 7 deletions(-)

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

If this is correct, then I think we need a parameter in the structure to
indicate the endianess, and instead using custom way you could use the
cpu_to_le or cpu_to_be functions.



> ---
>  drivers/misc/pmic_i2c.c |   31 ++++++++++++++++++++++++-------
>  include/pmic.h          |    2 ++
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/pmic_i2c.c b/drivers/misc/pmic_i2c.c
> index e74c372..6b1487b 100644
> --- a/drivers/misc/pmic_i2c.c
> +++ b/drivers/misc/pmic_i2c.c
> @@ -40,13 +40,24 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>  
>  	switch (pmic_i2c_tx_num) {
>  	case 3:
> -		buf[0] = (val >> 16) & 0xff;
> -		buf[1] = (val >> 8) & 0xff;
> -		buf[2] = val & 0xff;
> +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL) {
> +			buf[2] = (val >> 16) & 0xff;
> +			buf[1] = (val >> 8) & 0xff;
> +			buf[0] = val & 0xff;
> +		} else {
> +			buf[0] = (val >> 16) & 0xff;
> +			buf[1] = (val >> 8) & 0xff;
> +			buf[2] = val & 0xff;
> +		}
>  		break;
>  	case 2:
> -		buf[0] = (val >> 8) & 0xff;
> -		buf[1] = val & 0xff;
> +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL) {
> +			buf[1] = (val >> 8) & 0xff;
> +			buf[0] = val & 0xff;
> +		} else {
> +			buf[0] = (val >> 8) & 0xff;
> +			buf[1] = val & 0xff;
> +		}
>  		break;
>  	case 1:
>  		buf[0] = val & 0xff;
> @@ -75,10 +86,16 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>  
>  	switch (pmic_i2c_tx_num) {
>  	case 3:
> -		ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
> +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL)
> +			ret_val = buf[2] << 16 | buf[1] << 8 | buf[0];
> +		else
> +			ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
>  		break;
>  	case 2:
> -		ret_val = buf[0] << 8 | buf[1];
> +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL)
> +			ret_val = buf[1] << 8 | buf[0];
> +		else
> +			ret_val = buf[0] << 8 | buf[1];
>  		break;
>  	case 1:
>  		ret_val = buf[0];
> diff --git a/include/pmic.h b/include/pmic.h
> index 6a05b40..71b7d13 100644
> --- a/include/pmic.h
> +++ b/include/pmic.h
> @@ -27,6 +27,7 @@
>  enum { PMIC_I2C, PMIC_SPI, };
>  enum { I2C_PMIC, I2C_NUM, };
>  enum { PMIC_READ, PMIC_WRITE, };
> +enum { PMIC_BYTE_ORDER_REVERSED, PMIC_BYTE_ORDER_NORMAL, };
>  
>  struct p_i2c {
>  	unsigned char addr;
> @@ -47,6 +48,7 @@ struct pmic {
>  	const char *name;
>  	unsigned char bus;
>  	unsigned char interface;
> +	unsigned char byte_order;
>  	unsigned char number_of_regs;
>  	union hw {
>  		struct p_i2c i2c;
> 

Best regards,
Stefano Babic

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

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.

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

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)

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

> and instead using custom way you could use
> the cpu_to_le or cpu_to_be functions.
> 
> 
> 
> > ---
> >  drivers/misc/pmic_i2c.c |   31 ++++++++++++++++++++++++-------
> >  include/pmic.h          |    2 ++
> >  2 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/misc/pmic_i2c.c b/drivers/misc/pmic_i2c.c
> > index e74c372..6b1487b 100644
> > --- a/drivers/misc/pmic_i2c.c
> > +++ b/drivers/misc/pmic_i2c.c
> > @@ -40,13 +40,24 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> > val) 
> >  	switch (pmic_i2c_tx_num) {
> >  	case 3:
> > -		buf[0] = (val >> 16) & 0xff;
> > -		buf[1] = (val >> 8) & 0xff;
> > -		buf[2] = val & 0xff;
> > +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL) {
> > +			buf[2] = (val >> 16) & 0xff;
> > +			buf[1] = (val >> 8) & 0xff;
> > +			buf[0] = val & 0xff;
> > +		} else {
> > +			buf[0] = (val >> 16) & 0xff;
> > +			buf[1] = (val >> 8) & 0xff;
> > +			buf[2] = val & 0xff;
> > +		}
> >  		break;
> >  	case 2:
> > -		buf[0] = (val >> 8) & 0xff;
> > -		buf[1] = val & 0xff;
> > +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL) {
> > +			buf[1] = (val >> 8) & 0xff;
> > +			buf[0] = val & 0xff;
> > +		} else {
> > +			buf[0] = (val >> 8) & 0xff;
> > +			buf[1] = val & 0xff;
> > +		}
> >  		break;
> >  	case 1:
> >  		buf[0] = val & 0xff;
> > @@ -75,10 +86,16 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32
> > *val) 
> >  	switch (pmic_i2c_tx_num) {
> >  	case 3:
> > -		ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
> > +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL)
> > +			ret_val = buf[2] << 16 | buf[1] << 8 |
> > buf[0];
> > +		else
> > +			ret_val = buf[0] << 16 | buf[1] << 8 |
> > buf[2]; break;
> >  	case 2:
> > -		ret_val = buf[0] << 8 | buf[1];
> > +		if (p->byte_order == PMIC_BYTE_ORDER_NORMAL)
> > +			ret_val = buf[1] << 8 | buf[0];
> > +		else
> > +			ret_val = buf[0] << 8 | buf[1];
> >  		break;
> >  	case 1:
> >  		ret_val = buf[0];
> > diff --git a/include/pmic.h b/include/pmic.h
> > index 6a05b40..71b7d13 100644
> > --- a/include/pmic.h
> > +++ b/include/pmic.h
> > @@ -27,6 +27,7 @@
> >  enum { PMIC_I2C, PMIC_SPI, };
> >  enum { I2C_PMIC, I2C_NUM, };
> >  enum { PMIC_READ, PMIC_WRITE, };
> > +enum { PMIC_BYTE_ORDER_REVERSED, PMIC_BYTE_ORDER_NORMAL, };
> >  
> >  struct p_i2c {
> >  	unsigned char addr;
> > @@ -47,6 +48,7 @@ struct pmic {
> >  	const char *name;
> >  	unsigned char bus;
> >  	unsigned char interface;
> > +	unsigned char byte_order;
> >  	unsigned char number_of_regs;
> >  	union hw {
> >  		struct p_i2c i2c;
> > 
> 
> Best regards,
> Stefano Babic
>

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