Testing of the LED state showed that when the LED polarity was
set to GPIO_POLARITY_ACTIVE_LOW and a low logic value was set on
the input GPIO of the LED, the LED was being turned off when it was
expected to be turned on.

Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---
 hw/misc/led.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/led.c b/hw/misc/led.c
index f6d6d68bce..96cad7578e 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -63,7 +63,7 @@ static void led_set_state_gpio_handler(void *opaque, int line, int new_state)
     LEDState *s = LED(opaque);
 
     assert(line == 0);
-    led_set_state(s, !!new_state != s->gpio_active_high);
+    led_set_state(s, !new_state != s->gpio_active_high);
 }
 
 static void led_reset(DeviceState *dev)
-- 
2.31.1

On Tue, 24 Oct 2023 at 18:40, Glenn Miles <milesg@linux.vnet.ibm.com> wrote:
>
> Testing of the LED state showed that when the LED polarity was
> set to GPIO_POLARITY_ACTIVE_LOW and a low logic value was set on
> the input GPIO of the LED, the LED was being turned off when it was
> expected to be turned on.

It looks to me from reading the code like the bug is there
for active-high GPIO as well ?

> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
>  hw/misc/led.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/led.c b/hw/misc/led.c
> index f6d6d68bce..96cad7578e 100644
> --- a/hw/misc/led.c
> +++ b/hw/misc/led.c
> @@ -63,7 +63,7 @@ static void led_set_state_gpio_handler(void *opaque, int line, int new_state)
>      LEDState *s = LED(opaque);
>
>      assert(line == 0);
> -    led_set_state(s, !!new_state != s->gpio_active_high);
> +    led_set_state(s, !new_state != s->gpio_active_high);
>  }

Maybe "!!new_state == s->gpio_active_high" would be clearer?
Then you can see that we are (1) converting the int new_state
to a bool with the !! idiom and (2) we enable the LED for a
high input and active-high GPIO.

thanks
-- PMM

On Tue, 2023-10-24 at 18:46 +0100, Peter Maydell wrote:
> On Tue, 24 Oct 2023 at 18:40, Glenn Miles <milesg@linux.vnet.ibm.com>
> wrote:
> > Testing of the LED state showed that when the LED polarity was
> > set to GPIO_POLARITY_ACTIVE_LOW and a low logic value was set on
> > the input GPIO of the LED, the LED was being turned off when it was
> > expected to be turned on.
> 
> It looks to me from reading the code like the bug is there
> for active-high GPIO as well ?

Yes, this should fix the issue for both situations.

> 
> > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > ---
> >  hw/misc/led.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/misc/led.c b/hw/misc/led.c
> > index f6d6d68bce..96cad7578e 100644
> > --- a/hw/misc/led.c
> > +++ b/hw/misc/led.c
> > @@ -63,7 +63,7 @@ static void led_set_state_gpio_handler(void
> > *opaque, int line, int new_state)
> >      LEDState *s = LED(opaque);
> > 
> >      assert(line == 0);
> > -    led_set_state(s, !!new_state != s->gpio_active_high);
> > +    led_set_state(s, !new_state != s->gpio_active_high);
> >  }
> 
> Maybe "!!new_state == s->gpio_active_high" would be clearer?
> Then you can see that we are (1) converting the int new_state
> to a bool with the !! idiom and (2) we enable the LED for a
> high input and active-high GPIO.
> 
> thanks
> -- PMM

Yes, I agree,  that is easier to read.  Thanks!

Glenn