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 <mil...@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