| Message ID | 20231019204011.3174115-2-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc/pca9552: Changes to support powernv10 | expand |
On Thu, 2023-10-19 at 15:40 -0500, 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> > > --- > > > > Changes from prior version: > > - Added comment regarding pca953X > > - Added cover letter > > > > hw/misc/pca9552.c | 18 +++++++++++++----- > > tests/qtest/pca9552-test.c | 6 +++--- > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index fff19e369a..445f56a9e8 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass; > > > > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, > > TYPE_PCA955X) > > - > > +/* > > + * Note: The LED_ON and LED_OFF configuration values for the PCA955X > > + * chips are the reverse of the PCA953X family of chips. > > + */ > > #define PCA9552_LED_ON 0x0 > > #define PCA9552_LED_OFF 0x1 > > #define PCA9552_LED_PWM0 0x2 > > @@ -112,13 +115,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; So the witherspoon-bmc machine was a user of the PCA9552 outputs as LEDs. I guess its LEDs were in the wrong state the whole time? That looks like the only user though, and shouldn't be negatively affected. Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> > > 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)
On 20/10/23 04:51, Andrew Jeffery wrote: > On Thu, 2023-10-19 at 15:40 -0500, 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> >>> --- >>> >>> Changes from prior version: >>> - Added comment regarding pca953X >>> - Added cover letter >>> >>> hw/misc/pca9552.c | 18 +++++++++++++----- >>> tests/qtest/pca9552-test.c | 6 +++--- >>> 2 files changed, 16 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c >>> index fff19e369a..445f56a9e8 100644 >>> --- a/hw/misc/pca9552.c >>> +++ b/hw/misc/pca9552.c >>> @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass; >>> >>> DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, >>> TYPE_PCA955X) >>> - >>> +/* >>> + * Note: The LED_ON and LED_OFF configuration values for the PCA955X >>> + * chips are the reverse of the PCA953X family of chips. >>> + */ >>> #define PCA9552_LED_ON 0x0 >>> #define PCA9552_LED_OFF 0x1 >>> #define PCA9552_LED_PWM0 0x2 >>> @@ -112,13 +115,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; > > So the witherspoon-bmc machine was a user of the PCA9552 outputs as > LEDs. I guess its LEDs were in the wrong state the whole time? That > looks like the only user though, and shouldn't be negatively affected. Usually GPIO polarity is a machine/board property, not a device one. We could use the LED API (hw/misc/led.h) which initialize each output with GpioPolarity.
On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote: > On 20/10/23 04:51, Andrew Jeffery wrote: > > On Thu, 2023-10-19 at 15:40 -0500, 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> > > > > --- > > > > > > > > Changes from prior version: > > > > - Added comment regarding pca953X > > > > - Added cover letter > > > > > > > > hw/misc/pca9552.c | 18 +++++++++++++----- > > > > tests/qtest/pca9552-test.c | 6 +++--- > > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > > > index fff19e369a..445f56a9e8 100644 > > > > --- a/hw/misc/pca9552.c > > > > +++ b/hw/misc/pca9552.c > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass; > > > > > > > > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, > > > > TYPE_PCA955X) > > > > - > > > > +/* > > > > + * Note: The LED_ON and LED_OFF configuration values for the > > > > PCA955X > > > > + * chips are the reverse of the PCA953X family of > > > > chips. > > > > + */ > > > > #define PCA9552_LED_ON 0x0 > > > > #define PCA9552_LED_OFF 0x1 > > > > #define PCA9552_LED_PWM0 0x2 > > > > @@ -112,13 +115,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; > > > > So the witherspoon-bmc machine was a user of the PCA9552 outputs as > > LEDs. I guess its LEDs were in the wrong state the whole time? That > > looks like the only user though, and shouldn't be negatively > > affected. > > Usually GPIO polarity is a machine/board property, not a device > one. > > We could use the LED API (hw/misc/led.h) which initialize each > output with GpioPolarity. > Thanks for your comments! This piqued my curiosity so I decided to run a test with the witherspoon-bmc machine. Without my changes, I ran the following command to turn off LED13 on the pca9552 which I had previously set to "on": qom-set /machine/unattached/device[18] led13 off I had GDB connected at the time with a breakpoint set on led_set_state() so that I could see what was happening. Due to the inversion bug, I expected the pca9552 code to set the pin low and also set the irq low, which it did. The connected LED's on this pca9552 were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that setting the irq state low would actually turn on the LED. Instead it turned off the LED. Reviewing the LED code, I believe the problem lies here: 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); } In my test, new_state was 0 and gpio_active_high was 0, resulting in the boolean expression of ( 0 != 0 ) which is false and results in turning off the LED. So, this looks like a bug to me. For another simpler example, if the LED polarity was set to GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be 1. Then, if we set the irq line to 1, wouldn't we expect the LED to turn on? However, as the code stands, it would actually turn the LED off. So, I think we can remove one of the "!"'s from in front of new_state. Then, if the LED is active high and the irq line is set high, it would turn on the LED. Correct? Thanks, Glenn
On Fri, 2023-10-20 at 11:32 -0500, Miles Glenn wrote: > On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote: > > On 20/10/23 04:51, Andrew Jeffery wrote: > > > On Thu, 2023-10-19 at 15:40 -0500, 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> > > > > > --- > > > > > > > > > > Changes from prior version: > > > > > - Added comment regarding pca953X > > > > > - Added cover letter > > > > > > > > > > hw/misc/pca9552.c | 18 +++++++++++++----- > > > > > tests/qtest/pca9552-test.c | 6 +++--- > > > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > > > > index fff19e369a..445f56a9e8 100644 > > > > > --- a/hw/misc/pca9552.c > > > > > +++ b/hw/misc/pca9552.c > > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass; > > > > > > > > > > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, > > > > > TYPE_PCA955X) > > > > > - > > > > > +/* > > > > > + * Note: The LED_ON and LED_OFF configuration values for the > > > > > PCA955X > > > > > + * chips are the reverse of the PCA953X family of > > > > > chips. > > > > > + */ > > > > > #define PCA9552_LED_ON 0x0 > > > > > #define PCA9552_LED_OFF 0x1 > > > > > #define PCA9552_LED_PWM0 0x2 > > > > > @@ -112,13 +115,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; > > > > > > So the witherspoon-bmc machine was a user of the PCA9552 outputs as > > > LEDs. I guess its LEDs were in the wrong state the whole time? That > > > looks like the only user though, and shouldn't be negatively > > > affected. > > > > Usually GPIO polarity is a machine/board property, not a device > > one. > > > > We could use the LED API (hw/misc/led.h) which initialize each > > output with GpioPolarity. > > > > Thanks for your comments! This piqued my curiosity so I decided to > run a test with the witherspoon-bmc machine. Without my changes, I ran > the following command to turn off LED13 on the pca9552 which I had > previously set to "on": > > qom-set /machine/unattached/device[18] led13 off > > I had GDB connected at the time with a breakpoint set on > led_set_state() so that I could see what was happening. Due to the > inversion bug, I expected the pca9552 code to set the pin low and also > set the irq low, which it did. The connected LED's on this pca9552 > were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that > setting the irq state low would actually turn on the LED. Instead it > turned off the LED. > > Reviewing the LED code, I believe the problem lies here: > > 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); > } > > > In my test, new_state was 0 and gpio_active_high was 0, resulting in > the boolean expression of ( 0 != 0 ) which is false and results in > turning off the LED. So, this looks like a bug to me. > > For another simpler example, if the LED polarity was set to > GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be 1. Then, > if we set the irq line to 1, wouldn't we expect the LED to turn on? > However, as the code stands, it would actually turn the LED off. So, I > think we can remove one of the "!"'s from in front of new_state. Then, > if the LED is active high and the irq line is set high, it would turn > on the LED. Correct? Seems reasonable to me. Looks like Philippe added the LED behaviour in Fddb67f6402b8 ("hw/misc/led: Allow connecting from GPIO output"), so his input would be helpful. Perhaps send a fix for review? Andrew
On Tue, 2023-10-24 at 10:04 +1030, Andrew Jeffery wrote: > On Fri, 2023-10-20 at 11:32 -0500, Miles Glenn wrote: > > On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote: > > > On 20/10/23 04:51, Andrew Jeffery wrote: > > > > On Thu, 2023-10-19 at 15:40 -0500, 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> > > > > > > --- > > > > > > > > > > > > Changes from prior version: > > > > > > - Added comment regarding pca953X > > > > > > - Added cover letter > > > > > > > > > > > > hw/misc/pca9552.c | 18 +++++++++++++----- > > > > > > tests/qtest/pca9552-test.c | 6 +++--- > > > > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > > > > > index fff19e369a..445f56a9e8 100644 > > > > > > --- a/hw/misc/pca9552.c > > > > > > +++ b/hw/misc/pca9552.c > > > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass > > > > > > PCA955xClass; > > > > > > > > > > > > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, > > > > > > TYPE_PCA955X) > > > > > > - > > > > > > +/* > > > > > > + * Note: The LED_ON and LED_OFF configuration values for > > > > > > the > > > > > > PCA955X > > > > > > + * chips are the reverse of the PCA953X family of > > > > > > chips. > > > > > > + */ > > > > > > #define PCA9552_LED_ON 0x0 > > > > > > #define PCA9552_LED_OFF 0x1 > > > > > > #define PCA9552_LED_PWM0 0x2 > > > > > > @@ -112,13 +115,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; > > > > > > > > So the witherspoon-bmc machine was a user of the PCA9552 > > > > outputs as > > > > LEDs. I guess its LEDs were in the wrong state the whole time? > > > > That > > > > looks like the only user though, and shouldn't be negatively > > > > affected. > > > > > > Usually GPIO polarity is a machine/board property, not a device > > > one. > > > > > > We could use the LED API (hw/misc/led.h) which initialize each > > > output with GpioPolarity. > > > > > > > Thanks for your comments! This piqued my curiosity so I decided > > to > > run a test with the witherspoon-bmc machine. Without my changes, I > > ran > > the following command to turn off LED13 on the pca9552 which I had > > previously set to "on": > > > > qom-set /machine/unattached/device[18] led13 off > > > > I had GDB connected at the time with a breakpoint set on > > led_set_state() so that I could see what was happening. Due to the > > inversion bug, I expected the pca9552 code to set the pin low and > > also > > set the irq low, which it did. The connected LED's on this pca9552 > > were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that > > setting the irq state low would actually turn on the LED. Instead > > it > > turned off the LED. > > > > Reviewing the LED code, I believe the problem lies here: > > > > 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); > > } > > > > > > In my test, new_state was 0 and gpio_active_high was 0, resulting > > in > > the boolean expression of ( 0 != 0 ) which is false and results in > > turning off the LED. So, this looks like a bug to me. > > > > For another simpler example, if the LED polarity was set to > > GPIO_POLARITY_ACTIVE_HIGH, then s->gpio_active_high would be > > 1. Then, > > if we set the irq line to 1, wouldn't we expect the LED to turn > > on? > > However, as the code stands, it would actually turn the LED > > off. So, I > > think we can remove one of the "!"'s from in front of > > new_state. Then, > > if the LED is active high and the irq line is set high, it would > > turn > > on the LED. Correct? > > Seems reasonable to me. Looks like Philippe added the LED behaviour > in > Fddb67f6402b8 ("hw/misc/led: Allow connecting from GPIO output"), so > his input would be helpful. Perhaps send a fix for review? > > Andrew Sounds good. I'll do that. Glenn
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c index fff19e369a..445f56a9e8 100644 --- a/hw/misc/pca9552.c +++ b/hw/misc/pca9552.c @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass; DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X, TYPE_PCA955X) - +/* + * Note: The LED_ON and LED_OFF configuration values for the PCA955X + * chips are the reverse of the PCA953X family of chips. + */ #define PCA9552_LED_ON 0x0 #define PCA9552_LED_OFF 0x1 #define PCA9552_LED_PWM0 0x2 @@ -112,13 +115,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)
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> --- Changes from prior version: - Added comment regarding pca953X - Added cover letter hw/misc/pca9552.c | 18 +++++++++++++----- tests/qtest/pca9552-test.c | 6 +++--- 2 files changed, 16 insertions(+), 8 deletions(-)