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?
Thanks,
Glenn
