linux-kernel - Re: [PATCH] Add support for TCA6424

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 25 Apr 2012 14:47:09 +0200
From:	Andreas Schallenberg <Andreas.Schallenberg@...itytechnica.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"linus.walleij@...ricsson.com" <linus.walleij@...ricsson.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Add support for TCA6424

On 25.04.2012 13:02, Linus Walleij wrote:
> On Tue, Apr 24, 2012 at 3:48 PM, Andreas Schallenberg
> <Andreas.Schallenberg@...itytechnica.com>  wrote:
>
>> From: Andreas Schallenberg<aschallenberg@...026Linux.(none)>
> Some more detailed commit blurb here detailing some TC6424 characteristics
> maybe?
Yes, I'll add it.

>>   struct pca953x_chip {
>>         unsigned gpio_start;
>> -       uint16_t reg_output;
>> -       uint16_t reg_direction;
>> +       uint reg_output;
>> +       uint reg_direction;
>>         struct mutex i2c_lock;
> So ther are not 16bit?
>
> Suggest using the kernel-internal u16 otherwise.
> Or u32 if they really are 32bit. Which I suspect they are.
>
> Or if they are 24bit as I see in some below code,
Yes, it is a 24 bit I/O expander.


>   cosider:
>
> u32 foo:24;
Nice. Haven't seen this before.

>> -static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint16_t val)
>> +static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint val)
> u32?
Changing to u32.
>>         if (chip->gpio_chip.ngpio<= 8)
>>                 ret = i2c_smbus_write_byte_data(chip->client, reg, val);
>> +       else if (chip->gpio_chip.ngpio>  16) {
>> +               ret = i2c_smbus_write_word_data(chip->client,
>> +                                               (reg<<  2) | REG_ADDR_AI,
>> +                                               val&  0xffff);
>> +               ret = i2c_smbus_write_byte_data(chip->client,
>> +                                               (reg<<  2) + 2,
>> +                                               (val&  0xff0000)>>  16);
>> +       }
>>         else {
>>                 switch (chip->chip_type) {
>>                 case PCA953X_TYPE:
> And you are positively certain that this does not create a problem
> on other chips in this familym or is this the only chip with
>> 16 IRQs?
All the other chips supported by this driver have 8 or 16 GPIOs. Its the 
only one with 24.

>> @@ -121,12 +131,17 @@ static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint16_t val)
>>         return 0;
>>   }
>>
>> -static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint16_t *val)
>> +static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint *val)
> u32?
Changing to u32.

>>   {
>>         int ret;
>>
>>         if (chip->gpio_chip.ngpio<= 8)
>>                 ret = i2c_smbus_read_byte_data(chip->client, reg);
>> +       else if (chip->gpio_chip.ngpio == 24) {
> So is the comparison going to be>  16 (as above) or ==24 as here?
> Decide for *one* way of detecting.
Switching to == 24 in both places.
>> @@ -135,14 +150,14 @@ static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint16_t *val)
>>                 return ret;
>>         }
>>
>> -       *val = (uint16_t)ret;
>> +       *val = (uint)ret;
> u32?
>
Changing to u32.
>>   static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
>>   {
>>         struct pca953x_chip *chip;
>> -       uint16_t reg_val;
>> +       uint reg_val;
> u32?
Changing to u32.
>> @@ -173,7 +188,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
>>                 unsigned off, int val)
>>   {
>>         struct pca953x_chip *chip;
>> -       uint16_t reg_val;
>> +       uint reg_val;
> u32?
Changing to u32.
>> @@ -223,7 +238,7 @@ exit:
>>   static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
>>   {
>>         struct pca953x_chip *chip;
>> -       uint16_t reg_val;
>> +       uint reg_val;
> u32?
Changing to u32.
>> @@ -253,7 +268,7 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
>>   static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
>>   {
>>         struct pca953x_chip *chip;
>> -       uint16_t reg_val;
>> +       uint reg_val;
> u32?
Changing to u32.

>> @@ -386,7 +401,7 @@ static struct irq_chip pca953x_irq_chip = {
>>
>>   static uint16_t pca953x_irq_pending(struct pca953x_chip *chip)
>>   {
>> -       uint16_t cur_stat;
>> +       uint     cur_stat;
> u32?
Changing to u32.
>> @@ -449,6 +464,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>>   {
>>         struct i2c_client *client = chip->client;
>>         int ret, offset = 0;
>> +       uint temporary;
> u32?
Changing to u32.
>> @@ -606,7 +623,7 @@ out:
>>   static int __devinit device_pca957x_init(struct pca953x_chip *chip, int invert)
>>   {
>>         int ret;
>> -       uint16_t val = 0;
>> +       uint val = 0;
> u32?
Changing to u32.

Thanks for the review, will send an updated patch.

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/