Re: [PATCH 1/2] misc/pca9552: Fix inverted input status

[Miles Glenn]

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