* [PATCH] misc/pca9552: Fix inverted input status
@ 2023-09-27 20:32 Glenn Miles
2023-10-11 14:27 ` Cédric Le Goater
0 siblings, 1 reply; 3+ messages in thread
From: Glenn Miles @ 2023-09-27 20:32 UTC (permalink / raw)
To: qemu-devel
Cc: Glenn Miles, qemu-arm, Cédric Le Goater, andrew,
Joel Stanley
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>
---
hw/misc/pca9552.c | 13 +++++++++----
tests/qtest/pca9552-test.c | 6 +++---
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index fff19e369a..ad811fb249 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -112,13 +112,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)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] misc/pca9552: Fix inverted input status
2023-09-27 20:32 [PATCH] misc/pca9552: Fix inverted input status Glenn Miles
@ 2023-10-11 14:27 ` Cédric Le Goater
2023-10-11 19:26 ` Miles Glenn
0 siblings, 1 reply; 3+ messages in thread
From: Cédric Le Goater @ 2023-10-11 14:27 UTC (permalink / raw)
To: Glenn Miles, qemu-devel; +Cc: qemu-arm, andrew, Joel Stanley
On 9/27/23 22:32, 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>
Looks OK to me.
May be you could mention that the ON/OFF values are reversed on
the pca953x family. This is good to know if one wants to model
these devices one day.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/misc/pca9552.c | 13 +++++++++----
> tests/qtest/pca9552-test.c | 6 +++---
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index fff19e369a..ad811fb249 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -112,13 +112,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)
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] misc/pca9552: Fix inverted input status
2023-10-11 14:27 ` Cédric Le Goater
@ 2023-10-11 19:26 ` Miles Glenn
0 siblings, 0 replies; 3+ messages in thread
From: Miles Glenn @ 2023-10-11 19:26 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel; +Cc: qemu-arm, andrew, Joel Stanley
On Wed, 2023-10-11 at 16:27 +0200, Cédric Le Goater wrote:
> On 9/27/23 22:32, 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>
>
> Looks OK to me.
>
> May be you could mention that the ON/OFF values are reversed on
> the pca953x family. This is good to know if one wants to model
> these devices one day.
>
Yep, I will do that.
Thanks,
Glenn
> Reviewed-by: Cdric Le Goater <clg@kaod.org>
>
> Thanks,
>
> C.
>
>
> > ---
> > hw/misc/pca9552.c | 13 +++++++++----
> > tests/qtest/pca9552-test.c | 6 +++---
> > 2 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index fff19e369a..ad811fb249 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -112,13 +112,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)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-11 19:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-27 20:32 [PATCH] misc/pca9552: Fix inverted input status Glenn Miles
2023-10-11 14:27 ` Cédric Le Goater
2023-10-11 19:26 ` Miles Glenn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).