Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1365900 > unrolled thread

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

Started byYong Li <sdliyong@gmail.com>
First post2016-03-29 08:40 +0200
Last post2016-03-30 07:20 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] gpio: pca953x: Use correct u16 value for register word write Yong Li <sdliyong@gmail.com> - 2016-03-29 08:40 +0200
    Re: [PATCH] gpio: pca953x: Use correct u16 value for register word  write Phil Reid <preid@electromag.com.au> - 2016-03-29 14:10 +0200
      Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write Yong Li <sdliyong@gmail.com> - 2016-03-29 15:00 +0200
        Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write Alexander Stein <alexander.stein@systec-electronic.com> - 2016-03-29 16:40 +0200
          Re: [PATCH] gpio: pca953x: Use correct u16 value for register word  write Phil Reid <preid@electromag.com.au> - 2016-03-29 18:40 +0200
            Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write Yong Li <sdliyong@gmail.com> - 2016-03-30 04:50 +0200
            Re: [PATCH] gpio: pca953x: Use correct u16 value for register word write Yong Li <sdliyong@gmail.com> - 2016-03-30 07:10 +0200
              Re: [PATCH] gpio: pca953x: Use correct u16 value for register word  write Phil Reid <preid@electromag.com.au> - 2016-03-30 07:20 +0200

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

FromYong Li <sdliyong@gmail.com>
Date2016-03-29 08:40 +0200
Subject[PATCH] gpio: pca953x: Use correct u16 value for register word write
Message-ID<rhVtL-2bq-1@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromPhil Reid <preid@electromag.com.au>
Date2016-03-29 14:10 +0200
SubjectRe: [PATCH] gpio: pca953x: Use correct u16 value for register word write
Message-ID<ri0D7-61I-9@gated-at.bofh.it>
In reply to#1365900
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

[toc] | [prev] | [next] | [standalone]

#1366246

FromYong Li <sdliyong@gmail.com>
Date2016-03-29 15:00 +0200
Message-ID<ri1px-6kK-31@gated-at.bofh.it>
In reply to#1366199
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
>

[toc] | [prev] | [next] | [standalone]

#1366348

FromAlexander Stein <alexander.stein@systec-electronic.com>
Date2016-03-29 16:40 +0200
Message-ID<ri2Yi-7C8-23@gated-at.bofh.it>
In reply to#1366246
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

[toc] | [prev] | [next] | [standalone]

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

FromPhil Reid <preid@electromag.com.au>
Date2016-03-29 18:40 +0200
SubjectRe: [PATCH] gpio: pca953x: Use correct u16 value for register word write
Message-ID<ri4Qq-sQ-13@gated-at.bofh.it>
In reply to#1366348
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

[toc] | [prev] | [next] | [standalone]

#1366875

FromYong Li <sdliyong@gmail.com>
Date2016-03-30 04:50 +0200
Message-ID<riemK-7jK-7@gated-at.bofh.it>
In reply to#1366451
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
>

[toc] | [prev] | [next] | [standalone]

#1366893

FromYong Li <sdliyong@gmail.com>
Date2016-03-30 07:10 +0200
Message-ID<rigye-Aq-1@gated-at.bofh.it>
In reply to#1366451
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
>>

[toc] | [prev] | [next] | [standalone]

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

FromPhil Reid <preid@electromag.com.au>
Date2016-03-30 07:20 +0200
SubjectRe: [PATCH] gpio: pca953x: Use correct u16 value for register word write
Message-ID<rigHU-Er-7@gated-at.bofh.it>
In reply to#1366893
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web