1. Patchew
  2. QEMU
  3. View series

[PATCH] misc/pca9552: Fix inverted input status

Glenn Miles posted 1 patch 7 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230927203221.3286895-1-milesg@linux.vnet.ibm.com
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/misc/pca9552.c          | 13 +++++++++----
tests/qtest/pca9552-test.c |  6 +++---
2 files changed, 12 insertions(+), 7 deletions(-)
[PATCH] misc/pca9552: Fix inverted input status
Posted by Glenn Miles 7 months, 1 week ago 
The pca9552 INPUT0 and INPUT1 registers are supposed to
hold the logical values of the LED pins.  A logical 0
should be seen in the INPUT0/1 registers for a pin when
its corresponding LSn bits are set to 0, which is also
the state needed for turning on an LED in a typical
usage scenario.  Existing code was doing the opposite
and setting INPUT0/1 bit to a 1 when the LSn bit was
set to 0, so this commit fixes that.

Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---
 hw/misc/pca9552.c          | 13 +++++++++----
 tests/qtest/pca9552-test.c |  6 +++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index fff19e369a..ad811fb249 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -112,13 +112,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
 
         switch (config) {
         case PCA9552_LED_ON:
-            qemu_set_irq(s->gpio[i], 1);
-            s->regs[input_reg] |= 1 << input_shift;
-            break;
-        case PCA9552_LED_OFF:
+            /* Pin is set to 0V to turn on LED */
             qemu_set_irq(s->gpio[i], 0);
             s->regs[input_reg] &= ~(1 << input_shift);
             break;
+        case PCA9552_LED_OFF:
+            /*
+             * Pin is set to Hi-Z to turn off LED and
+             * pullup sets it to a logical 1.
+             */
+            qemu_set_irq(s->gpio[i], 1);
+            s->regs[input_reg] |= 1 << input_shift;
+            break;
         case PCA9552_LED_PWM0:
         case PCA9552_LED_PWM1:
             /* TODO */
diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
index d80ed93cd3..ccca2b3d91 100644
--- a/tests/qtest/pca9552-test.c
+++ b/tests/qtest/pca9552-test.c
@@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x55);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x0);
+    g_assert_cmphex(value, ==, 0xFF);
 
     pca9552_init(i2cdev);
 
@@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT0);
-    g_assert_cmphex(value, ==, 0x01);
+    g_assert_cmphex(value, ==, 0xFE);
 
     value = i2c_get8(i2cdev, PCA9552_LS3);
     g_assert_cmphex(value, ==, 0x54);
 
     value = i2c_get8(i2cdev, PCA9552_INPUT1);
-    g_assert_cmphex(value, ==, 0x10);
+    g_assert_cmphex(value, ==, 0xEF);
 }
 
 static void pca9552_register_nodes(void)
-- 
2.31.1
Re: [PATCH] misc/pca9552: Fix inverted input status
Posted by Cédric Le Goater 6 months, 3 weeks ago 
On 9/27/23 22:32, Glenn Miles wrote:
> The pca9552 INPUT0 and INPUT1 registers are supposed to
> hold the logical values of the LED pins.  A logical 0
> should be seen in the INPUT0/1 registers for a pin when
> its corresponding LSn bits are set to 0, which is also
> the state needed for turning on an LED in a typical
> usage scenario.  Existing code was doing the opposite
> and setting INPUT0/1 bit to a 1 when the LSn bit was
> set to 0, so this commit fixes that.
> 
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>

Looks OK to me.

May be you could mention that the ON/OFF values are reversed on
the pca953x family. This is good to know if one wants to model
these devices one day.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   hw/misc/pca9552.c          | 13 +++++++++----
>   tests/qtest/pca9552-test.c |  6 +++---
>   2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index fff19e369a..ad811fb249 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -112,13 +112,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
>   
>           switch (config) {
>           case PCA9552_LED_ON:
> -            qemu_set_irq(s->gpio[i], 1);
> -            s->regs[input_reg] |= 1 << input_shift;
> -            break;
> -        case PCA9552_LED_OFF:
> +            /* Pin is set to 0V to turn on LED */
>               qemu_set_irq(s->gpio[i], 0);
>               s->regs[input_reg] &= ~(1 << input_shift);
>               break;
> +        case PCA9552_LED_OFF:
> +            /*
> +             * Pin is set to Hi-Z to turn off LED and
> +             * pullup sets it to a logical 1.
> +             */
> +            qemu_set_irq(s->gpio[i], 1);
> +            s->regs[input_reg] |= 1 << input_shift;
> +            break;
>           case PCA9552_LED_PWM0:
>           case PCA9552_LED_PWM1:
>               /* TODO */
> diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
> index d80ed93cd3..ccca2b3d91 100644
> --- a/tests/qtest/pca9552-test.c
> +++ b/tests/qtest/pca9552-test.c
> @@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
>       g_assert_cmphex(value, ==, 0x55);
>   
>       value = i2c_get8(i2cdev, PCA9552_INPUT0);
> -    g_assert_cmphex(value, ==, 0x0);
> +    g_assert_cmphex(value, ==, 0xFF);
>   
>       pca9552_init(i2cdev);
>   
> @@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data, QGuestAllocator *alloc)
>       g_assert_cmphex(value, ==, 0x54);
>   
>       value = i2c_get8(i2cdev, PCA9552_INPUT0);
> -    g_assert_cmphex(value, ==, 0x01);
> +    g_assert_cmphex(value, ==, 0xFE);
>   
>       value = i2c_get8(i2cdev, PCA9552_LS3);
>       g_assert_cmphex(value, ==, 0x54);
>   
>       value = i2c_get8(i2cdev, PCA9552_INPUT1);
> -    g_assert_cmphex(value, ==, 0x10);
> +    g_assert_cmphex(value, ==, 0xEF);
>   }
>   
>   static void pca9552_register_nodes(void)
Re: [PATCH] misc/pca9552: Fix inverted input status
Posted by Miles Glenn 6 months, 3 weeks ago 
On Wed, 2023-10-11 at 16:27 +0200, Cédric Le Goater wrote:
> On 9/27/23 22:32, Glenn Miles wrote:
> > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > hold the logical values of the LED pins.  A logical 0
> > should be seen in the INPUT0/1 registers for a pin when
> > its corresponding LSn bits are set to 0, which is also
> > the state needed for turning on an LED in a typical
> > usage scenario.  Existing code was doing the opposite
> > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > set to 0, so this commit fixes that.
> > 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> 
> Looks OK to me.
> 
> May be you could mention that the ON/OFF values are reversed on
> the pca953x family. This is good to know if one wants to model
> these devices one day.
> 

Yep, I will do that.

Thanks,

Glenn

> Reviewed-by: C޸dric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
> > ---
> >   hw/misc/pca9552.c          | 13 +++++++++----
> >   tests/qtest/pca9552-test.c |  6 +++---
> >   2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index fff19e369a..ad811fb249 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -112,13 +112,18 @@ static void
> > pca955x_update_pin_input(PCA955xState *s)
> >   
> >           switch (config) {
> >           case PCA9552_LED_ON:
> > -            qemu_set_irq(s->gpio[i], 1);
> > -            s->regs[input_reg] |= 1 << input_shift;
> > -            break;
> > -        case PCA9552_LED_OFF:
> > +            /* Pin is set to 0V to turn on LED */
> >               qemu_set_irq(s->gpio[i], 0);
> >               s->regs[input_reg] &= ~(1 << input_shift);
> >               break;
> > +        case PCA9552_LED_OFF:
> > +            /*
> > +             * Pin is set to Hi-Z to turn off LED and
> > +             * pullup sets it to a logical 1.
> > +             */
> > +            qemu_set_irq(s->gpio[i], 1);
> > +            s->regs[input_reg] |= 1 << input_shift;
> > +            break;
> >           case PCA9552_LED_PWM0:
> >           case PCA9552_LED_PWM1:
> >               /* TODO */
> > diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-
> > test.c
> > index d80ed93cd3..ccca2b3d91 100644
> > --- a/tests/qtest/pca9552-test.c
> > +++ b/tests/qtest/pca9552-test.c
> > @@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void
> > *data, QGuestAllocator *alloc)
> >       g_assert_cmphex(value, ==, 0x55);
> >   
> >       value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -    g_assert_cmphex(value, ==, 0x0);
> > +    g_assert_cmphex(value, ==, 0xFF);
> >   
> >       pca9552_init(i2cdev);
> >   
> > @@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void
> > *data, QGuestAllocator *alloc)
> >       g_assert_cmphex(value, ==, 0x54);
> >   
> >       value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -    g_assert_cmphex(value, ==, 0x01);
> > +    g_assert_cmphex(value, ==, 0xFE);
> >   
> >       value = i2c_get8(i2cdev, PCA9552_LS3);
> >       g_assert_cmphex(value, ==, 0x54);
> >   
> >       value = i2c_get8(i2cdev, PCA9552_INPUT1);
> > -    g_assert_cmphex(value, ==, 0x10);
> > +    g_assert_cmphex(value, ==, 0xEF);
> >   }
> >   
> >   static void pca9552_register_nodes(void)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.