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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>:
> 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 <[email protected]>
>> ---
>> 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
>
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
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
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 <[email protected]>:
> 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
>
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 <[email protected]>:
> 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 <[email protected]>:
>>
>> 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
>>
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