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

[Philippe Mathieu-Daudé]

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.