[PATCH] gpio: pca953x: Use correct u16 value for register word write

[PATCH] gpio: pca953x: Use correct u16 value for register word write

[Yong Li]

The current implementation only uses the first byte in *val,
the second data is always 0. Change it to *(u16 *)val
to write the two bytes into the register

Signed-off-by: Yong Li <sdliyong@gmail.com>
---
 drivers/gpio/gpio-pca953x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d0d3065..cf3d410 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -159,7 +159,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 		switch (chip->chip_type) {
 		case PCA953X_TYPE:
 			ret = i2c_smbus_write_word_data(chip->client,
-							reg << 1, (u16) *val);
+							reg << 1, *(u16 *)val);
 			break;
 		case PCA957X_TYPE:
 			ret = i2c_smbus_write_byte_data(chip->client, reg << 1,
-- 
2.1.4

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Phil Reid]

G'day Yong,

One comment below.

On 29/03/2016 2:27 PM, Yong Li wrote:
> The current implementation only uses the first byte in *val,
> the second data is always 0. Change it to *(u16 *)val
> to write the two bytes into the register
>
> Signed-off-by: Yong Li <sdliyong@gmail.com>
> ---
>   drivers/gpio/gpio-pca953x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index d0d3065..cf3d410 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -159,7 +159,7 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
>   		switch (chip->chip_type) {
>   		case PCA953X_TYPE:
>   			ret = i2c_smbus_write_word_data(chip->client,
> -							reg << 1, (u16) *val);
> +							reg << 1, *(u16 *)val);
I don't think this is safe for systems that don't support unaligned memory access.


>   			break;
>   		case PCA957X_TYPE:
>   			ret = i2c_smbus_write_byte_data(chip->client, reg << 1,
>


-- 
Regards
Phil Reid

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Yong Li]

Thanks for your comment, I think I can change it to val[0] | (val[1]
<< 8), is it okay ?

2016-03-29 20:06 GMT+08:00 Phil Reid <preid@electromag.com.au>:
> G'day Yong,
>
> One comment below.
>
> On 29/03/2016 2:27 PM, Yong Li wrote:
>>
>> The current implementation only uses the first byte in *val,
>> the second data is always 0. Change it to *(u16 *)val
>> to write the two bytes into the register
>>
>> Signed-off-by: Yong Li <sdliyong@gmail.com>
>> ---
>>   drivers/gpio/gpio-pca953x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index d0d3065..cf3d410 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -159,7 +159,7 @@ static int pca953x_write_regs(struct pca953x_chip
>> *chip, int reg, u8 *val)
>>                 switch (chip->chip_type) {
>>                 case PCA953X_TYPE:
>>                         ret = i2c_smbus_write_word_data(chip->client,
>> -                                                       reg << 1, (u16)
>> *val);
>> +                                                       reg << 1, *(u16
>> *)val);
>
> I don't think this is safe for systems that don't support unaligned memory
> access.
>
>
>>                         break;
>>                 case PCA957X_TYPE:
>>                         ret = i2c_smbus_write_byte_data(chip->client, reg
>> << 1,
>>
>
>
> --
> Regards
> Phil Reid
>

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Alexander Stein]

You missed CC'ing Phil (Added for this post)

On Tuesday 29 March 2016 20:53:58, Yong Li wrote:
> Thanks for your comment, I think I can change it to val[0] | (val[1]
> << 8), is it okay ?

Mh, currently there is only one caller (device_pca953x_init) which passes only 
0, 0 or 0xff, 0xff, so endianess is irrelevant. But to be future proof this 
should be done in an endian-safe manner. Though cpu_to_le16p does not work, 
due to same alignment problem as casting to u16*.

Best regards,
Alexander

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Phil Reid]

On 29/03/2016 10:39 PM, Alexander Stein wrote:
> You missed CC'ing Phil (Added for this post)
>
> On Tuesday 29 March 2016 20:53:58, Yong Li wrote:
>> Thanks for your comment, I think I can change it to val[0] | (val[1]
>> << 8), is it okay ?
>
> Mh, currently there is only one caller (device_pca953x_init) which passes only
> 0, 0 or 0xff, 0xff, so endianess is irrelevant. But to be future proof this
> should be done in an endian-safe manner. Though cpu_to_le16p does not work,
> due to same alignment problem as casting to u16*.
>

I think get_unaligned((u16 *) val) should do the job.
There's also get_unaligned_le* get_unaligned_be*

-- 
Regards
Phil Reid

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Yong Li]

If use the get_unaligned, below is the code example, but we cannot
detect if it is big endian or little endian. I would like to use the
same write logic as PCA957X_TYPE: use the i2c_smbus_write_byte_data
API to write two times. How do you think about it?

if (big_endian)
value = get_unaligned_be16(buf);
else
value = get_unaligned_le16(buf);

Thanks,
Yong Li

2016-03-30 0:33 GMT+08:00 Phil Reid <preid@electromag.com.au>:
> On 29/03/2016 10:39 PM, Alexander Stein wrote:
>>
>> You missed CC'ing Phil (Added for this post)
>>
>> On Tuesday 29 March 2016 20:53:58, Yong Li wrote:
>>>
>>> Thanks for your comment, I think I can change it to val[0] | (val[1]
>>> << 8), is it okay ?
>>
>>
>> Mh, currently there is only one caller (device_pca953x_init) which passes
>> only
>> 0, 0 or 0xff, 0xff, so endianess is irrelevant. But to be future proof
>> this
>> should be done in an endian-safe manner. Though cpu_to_le16p does not
>> work,
>> due to same alignment problem as casting to u16*.
>>
>
> I think get_unaligned((u16 *) val) should do the job.
> There's also get_unaligned_le* get_unaligned_be*
>
> --
> Regards
> Phil Reid
>

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Yong Li]

Or another method is using the below to convert the u8 to u16:
cpu_to_le16(get_unaligned((u16 *) val)), compared with the
i2c_smbus_write_byte_data method, which one is better?

Thanks,
Yong

2016-03-30 10:43 GMT+08:00 Yong Li <sdliyong@gmail.com>:
> If use the get_unaligned, below is the code example, but we cannot detect if
> it is big endian or little endian. I would like to use the same write logic
> as PCA957X_TYPE: use the i2c_smbus_write_byte_data API to write two times.
> How do you think about it?
>
> if (big_endian)
> value = get_unaligned_be16(buf);
> else
> value = get_unaligned_le16(buf);
>
> Thanks,
> Yong Li
> 2016-03-30 0:33 GMT+08:00 Phil Reid <preid@electromag.com.au>:
>>
>> On 29/03/2016 10:39 PM, Alexander Stein wrote:
>>>
>>> You missed CC'ing Phil (Added for this post)
>>>
>>> On Tuesday 29 March 2016 20:53:58, Yong Li wrote:
>>>>
>>>> Thanks for your comment, I think I can change it to val[0] | (val[1]
>>>> << 8), is it okay ?
>>>
>>>
>>> Mh, currently there is only one caller (device_pca953x_init) which passes
>>> only
>>> 0, 0 or 0xff, 0xff, so endianess is irrelevant. But to be future proof
>>> this
>>> should be done in an endian-safe manner. Though cpu_to_le16p does not
>>> work,
>>> due to same alignment problem as casting to u16*.
>>>
>>
>> I think get_unaligned((u16 *) val) should do the job.
>> There's also get_unaligned_le* get_unaligned_be*
>>
>> --
>> Regards
>> Phil Reid
>>

Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write

[Phil Reid]

On 30/03/2016 1:01 PM, Yong Li wrote:
> Or another method is using the below to convert the u8 to u16:
> cpu_to_le16(get_unaligned((u16 *) val)), compared with the
> i2c_smbus_write_byte_data method, which one is better?
>
>

G'day Yong,

I'd go with the cpu_to_le16(get_unaligned((u16 *) val))



-- 
Regards
Phil Reid