Re: [PATCH 2/2] hw/misc/pca9552: Make LEDs 13-15 also GPIOs

2020-06-16 Thread Cédric Le Goater
On 6/16/20 11:45 AM, Philippe Mathieu-Daudé wrote:
> The PCA9552 has 3 GPIOs, add them.

This is an example of an HW configuration. All pins can be used
as GPIOs. 
 
> See 'PCA9552 Product Datasheet Rev. 05 - 9 March 2006',
> chapter 6.4 'Pins used as GPIOs':
> 
>   LED pins not used to control LEDs can be used as general
>   purpose I/Os (GPIOs).
>   For use as input, set LEDn to high-impedance (01) and then
>   read the pin state via the input register.
>   For use as output, connect external pull-up resistor to the
>   pin and size it according to the DC recommended operating
>   characteristics. LED output pin is HIGH when the output is
>   programmed as high-impedance, and LOW when the output is
>   programmed LOW through the ‘LED selector’ register. The
>   output can be pulse-width controlled when PWM0 or PWM1 are
>   used.
>
>
> And chapter 8 'Application design-in information':
> 
>   LED0 to LED12 are used as LED drivers.
>   LED13 to LED15 are used as regular GPIOs.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/misc/pca9552.h |  2 ++
>  hw/misc/pca9552.c | 18 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
> index ebb43c63fe..7e47ea312d 100644
> --- a/include/hw/misc/pca9552.h
> +++ b/include/hw/misc/pca9552.h
> @@ -15,6 +15,7 @@
>  #define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
>  
>  #define PCA9552_NR_REGS 10
> +#define PCA9552_NR_GPIOS 3
>  
>  typedef struct PCA9552State {
>  /*< private >*/
> @@ -27,6 +28,7 @@ typedef struct PCA9552State {
>  uint8_t regs[PCA9552_NR_REGS];
>  uint8_t max_reg;
>  uint8_t nr_leds;
> +qemu_irq gpio[PCA9552_NR_GPIOS];
>  } PCA9552State;
>  
>  #endif
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index a3d0decbff..6ca6c0dbc2 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -12,8 +12,10 @@
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "qemu/bitops.h"
>  #include "hw/misc/pca9552.h"
>  #include "hw/misc/pca9552_regs.h"
> +#include "hw/irq.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> @@ -48,12 +50,16 @@ static void pca9552_update_pin_input(PCA9552State *s)
>  s->regs[input_reg] |= 1 << input_shift;
>  if (input_shift < s->nr_leds) {
>  trace_pca9552_led_set(input_shift, true);
> +} else {
> +qemu_set_irq(s->gpio[input_shift - s->nr_leds], 1);

It is simpler to set the irq level in pca9552_gpio_set().

>  }
>  break;
>  case PCA9552_LED_OFF:
>  s->regs[input_reg] &= ~(1 << input_shift);
>  if (input_shift < s->nr_leds) {
>  trace_pca9552_led_set(input_shift, false);
> +} else {
> +qemu_set_irq(s->gpio[input_shift - s->nr_leds], 0);
>  }
>  break;
>  case PCA9552_LED_PWM0:
> @@ -65,6 +71,16 @@ static void pca9552_update_pin_input(PCA9552State *s)
>  }
>  }
>  
> +static void pca9552_gpio_set(void *opaque, int n, int enable)
> +{
> +PCA9552State *s = opaque;
> +
> +/* LED13 to LED15 are used as regular GPIOs. */
> +s->regs[PCA9552_LS3] = deposit32(s->regs[PCA9552_LS3], n + 1, 1,
> + enable ? PCA9552_LED_ON : 
> PCA9552_LED_OFF);> +pca9552_update_pin_input(s);

I would introduce a set_pin() helper instead. See pca9552_pin_get_config().
pca9552_gpio_set() would look like :

pca9552_set_pin(s, n, enable ? PCA9552_LED_ON : PCA9552_LED_OFF);
pca9552_update_pin_input(s);
qemu_set_irq(s->gpio[n], enable);

> +}
> +
>  static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
>  {
>  switch (reg) {
> @@ -308,6 +324,8 @@ static void pca9552_initfn(Object *obj)
>  NULL, NULL);
>  g_free(name);
>  }
> +qdev_init_gpio_in(DEVICE(obj), pca9552_gpio_set, PCA9552_NR_GPIOS);
> +qdev_init_gpio_out(DEVICE(obj), s->gpio, PCA9552_NR_GPIOS);
>  }
>  
>  static void pca9552_class_init(ObjectClass *klass, void *data)
> 




[PATCH 2/2] hw/misc/pca9552: Make LEDs 13-15 also GPIOs

2020-06-16 Thread Philippe Mathieu-Daudé
The PCA9552 has 3 GPIOs, add them.

See 'PCA9552 Product Datasheet Rev. 05 - 9 March 2006',
chapter 6.4 'Pins used as GPIOs':

  LED pins not used to control LEDs can be used as general
  purpose I/Os (GPIOs).
  For use as input, set LEDn to high-impedance (01) and then
  read the pin state via the input register.
  For use as output, connect external pull-up resistor to the
  pin and size it according to the DC recommended operating
  characteristics. LED output pin is HIGH when the output is
  programmed as high-impedance, and LOW when the output is
  programmed LOW through the ‘LED selector’ register. The
  output can be pulse-width controlled when PWM0 or PWM1 are
  used.

And chapter 8 'Application design-in information':

  LED0 to LED12 are used as LED drivers.
  LED13 to LED15 are used as regular GPIOs.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/misc/pca9552.h |  2 ++
 hw/misc/pca9552.c | 18 ++
 2 files changed, 20 insertions(+)

diff --git a/include/hw/misc/pca9552.h b/include/hw/misc/pca9552.h
index ebb43c63fe..7e47ea312d 100644
--- a/include/hw/misc/pca9552.h
+++ b/include/hw/misc/pca9552.h
@@ -15,6 +15,7 @@
 #define PCA9552(obj) OBJECT_CHECK(PCA9552State, (obj), TYPE_PCA9552)
 
 #define PCA9552_NR_REGS 10
+#define PCA9552_NR_GPIOS 3
 
 typedef struct PCA9552State {
 /*< private >*/
@@ -27,6 +28,7 @@ typedef struct PCA9552State {
 uint8_t regs[PCA9552_NR_REGS];
 uint8_t max_reg;
 uint8_t nr_leds;
+qemu_irq gpio[PCA9552_NR_GPIOS];
 } PCA9552State;
 
 #endif
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index a3d0decbff..6ca6c0dbc2 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -12,8 +12,10 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qemu/bitops.h"
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
+#include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
@@ -48,12 +50,16 @@ static void pca9552_update_pin_input(PCA9552State *s)
 s->regs[input_reg] |= 1 << input_shift;
 if (input_shift < s->nr_leds) {
 trace_pca9552_led_set(input_shift, true);
+} else {
+qemu_set_irq(s->gpio[input_shift - s->nr_leds], 1);
 }
 break;
 case PCA9552_LED_OFF:
 s->regs[input_reg] &= ~(1 << input_shift);
 if (input_shift < s->nr_leds) {
 trace_pca9552_led_set(input_shift, false);
+} else {
+qemu_set_irq(s->gpio[input_shift - s->nr_leds], 0);
 }
 break;
 case PCA9552_LED_PWM0:
@@ -65,6 +71,16 @@ static void pca9552_update_pin_input(PCA9552State *s)
 }
 }
 
+static void pca9552_gpio_set(void *opaque, int n, int enable)
+{
+PCA9552State *s = opaque;
+
+/* LED13 to LED15 are used as regular GPIOs. */
+s->regs[PCA9552_LS3] = deposit32(s->regs[PCA9552_LS3], n + 1, 1,
+ enable ? PCA9552_LED_ON : 
PCA9552_LED_OFF);
+pca9552_update_pin_input(s);
+}
+
 static uint8_t pca9552_read(PCA9552State *s, uint8_t reg)
 {
 switch (reg) {
@@ -308,6 +324,8 @@ static void pca9552_initfn(Object *obj)
 NULL, NULL);
 g_free(name);
 }
+qdev_init_gpio_in(DEVICE(obj), pca9552_gpio_set, PCA9552_NR_GPIOS);
+qdev_init_gpio_out(DEVICE(obj), s->gpio, PCA9552_NR_GPIOS);
 }
 
 static void pca9552_class_init(ObjectClass *klass, void *data)
-- 
2.21.3