| Message ID | 1891081c09747965173fb03a7305e1c73033dac0.1494660546.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > First, the logic for translating a register bit to the return code of > exar_get_direction and exar_get_value were wrong. And second, there was > a flip regarding the register bank in exar_get_direction. Again, I wish it was tested in the first place. After addressing below: FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > @@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) > value = readb(exar_gpio->regs + reg); > mutex_unlock(&exar_gpio->lock); > > - return !!value; > + return value; This one is correct. > @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) > addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; > val = exar_get(chip, addr) >> (offset % 8); > > - return !!val; > + return val & 1; It should be rather val = exar_get(chip, addr) & BIT(offset % 8); > } > > static int exar_get_value(struct gpio_chip *chip, unsigned int offset) > @@ -89,10 +89,10 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset) > unsigned int addr; > int val; > > - addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI; > + addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; Good catch! > val = exar_get(chip, addr) >> (offset % 8); > > - return !!val; > + return val & 1; Ditto (see above).
On 2017-05-13 15:36, Andy Shevchenko wrote: > On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> First, the logic for translating a register bit to the return code of >> exar_get_direction and exar_get_value were wrong. And second, there was >> a flip regarding the register bank in exar_get_direction. > > Again, I wish it was tested in the first place. > > After addressing below: > FWIW: > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> @@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) >> value = readb(exar_gpio->regs + reg); >> mutex_unlock(&exar_gpio->lock); >> >> - return !!value; >> + return value; > > This one is correct. > >> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) >> addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; >> val = exar_get(chip, addr) >> (offset % 8); >> >> - return !!val; >> + return val & 1; > > It should be rather > > val = exar_get(chip, addr) & BIT(offset % 8); That won't give us 0 or 1 as return value, thus would be incorrect. > >> } >> >> static int exar_get_value(struct gpio_chip *chip, unsigned int offset) >> @@ -89,10 +89,10 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset) >> unsigned int addr; >> int val; >> >> - addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI; >> + addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; > > Good catch! > >> val = exar_get(chip, addr) >> (offset % 8); >> >> - return !!val; >> + return val & 1; > > Ditto (see above). > Same here. Jan
On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2017-05-13 15:36, Andy Shevchenko wrote: >> On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> First, the logic for translating a register bit to the return code of >>> exar_get_direction and exar_get_value were wrong. And second, there was >>> a flip regarding the register bank in exar_get_direction. >> >> Again, I wish it was tested in the first place. >> >> After addressing below: >> FWIW: >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >>> @@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) >>> value = readb(exar_gpio->regs + reg); >>> mutex_unlock(&exar_gpio->lock); >>> >>> - return !!value; >>> + return value; >> >> This one is correct. >> >>> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) >>> addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; >>> val = exar_get(chip, addr) >> (offset % 8); >>> >>> - return !!val; >>> + return val & 1; >> >> It should be rather >> >> val = exar_get(chip, addr) & BIT(offset % 8); > > That won't give us 0 or 1 as return value, thus would be incorrect. Full picture: val = exar_get(chip, addr) & BIT(offset % 8); return !!val; How it could be non-(1 or 0)?
On 2017-05-18 12:11, Andy Shevchenko wrote: > On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2017-05-13 15:36, Andy Shevchenko wrote: >>> On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> First, the logic for translating a register bit to the return code of >>>> exar_get_direction and exar_get_value were wrong. And second, there was >>>> a flip regarding the register bank in exar_get_direction. >>> >>> Again, I wish it was tested in the first place. >>> >>> After addressing below: >>> FWIW: >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> >>>> @@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) >>>> value = readb(exar_gpio->regs + reg); >>>> mutex_unlock(&exar_gpio->lock); >>>> >>>> - return !!value; >>>> + return value; >>> >>> This one is correct. > >>> >>>> @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) >>>> addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; >>>> val = exar_get(chip, addr) >> (offset % 8); >>>> >>>> - return !!val; >>>> + return val & 1; >>> >>> It should be rather >>> >>> val = exar_get(chip, addr) & BIT(offset % 8); >> >> That won't give us 0 or 1 as return value, thus would be incorrect. > > Full picture: > > val = exar_get(chip, addr) & BIT(offset % 8); > > return !!val; > > How it could be non-(1 or 0)? > Right - but what is the point of that other style? Jan
On Thu, May 18, 2017 at 1:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2017-05-18 12:11, Andy Shevchenko wrote: >> On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> Full picture: >> >> val = exar_get(chip, addr) & BIT(offset % 8); >> >> return !!val; >> >> How it could be non-(1 or 0)? >> > > Right - but what is the point of that other style? gpio-exar.c is just 4th module which is using it, OTOH the rest of GPIO drivers are using return !!val style.
On 2017-05-18 12:23, Andy Shevchenko wrote: > On Thu, May 18, 2017 at 1:16 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2017-05-18 12:11, Andy Shevchenko wrote: >>> On Thu, May 18, 2017 at 8:20 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>> Full picture: >>> >>> val = exar_get(chip, addr) & BIT(offset % 8); >>> >>> return !!val; >>> >>> How it could be non-(1 or 0)? >>> >> >> Right - but what is the point of that other style? > > gpio-exar.c is just 4th module which is using it, OTOH the rest of > GPIO drivers are using return !!val style. > OK, consistency is valid point. Will adjust. Jan
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c index d6ffb3d89b9c..98bd3eb1290e 100644 --- a/drivers/gpio/gpio-exar.c +++ b/drivers/gpio/gpio-exar.c @@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg) value = readb(exar_gpio->regs + reg); mutex_unlock(&exar_gpio->lock); - return !!value; + return value; } static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) @@ -80,7 +80,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset) addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO; val = exar_get(chip, addr) >> (offset % 8); - return !!val; + return val & 1; } static int exar_get_value(struct gpio_chip *chip, unsigned int offset) @@ -89,10 +89,10 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset) unsigned int addr; int val; - addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI; + addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; val = exar_get(chip, addr) >> (offset % 8); - return !!val; + return val & 1; } static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
First, the logic for translating a register bit to the return code of exar_get_direction and exar_get_value were wrong. And second, there was a flip regarding the register bank in exar_get_direction. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- drivers/gpio/gpio-exar.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)