| Message ID | 20231024173923.4041874-1-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc/led: LED state is set opposite of what is expected | expand |
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
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)
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(-)