Hi Inès,
On 14/4/24 15:05, Inès Varhol wrote:
This device implements the IM120417002 colors shield v1.1 for Arduino
(which relies on the DM163 8x3-channel led driving logic) and features
a simple display of an 8x8 RGB matrix. The columns of the matrix are
driven by the DM163 and the rows are driven externally.
Acked-by: Alistair Francis <alistair.fran...@wdc.com>
Signed-off-by: Arnaud Minier <arnaud.min...@telecom-paris.fr>
Signed-off-by: Inès Varhol <ines.var...@telecom-paris.fr>
---
docs/system/arm/b-l475e-iot01a.rst | 3 +-
include/hw/display/dm163.h | 58 +++++
hw/display/dm163.c | 333 +++++++++++++++++++++++++++++
hw/display/Kconfig | 3 +
hw/display/meson.build | 1 +
hw/display/trace-events | 14 ++
6 files changed, 411 insertions(+), 1 deletion(-)
create mode 100644 include/hw/display/dm163.h
create mode 100644 hw/display/dm163.c
diff --git a/include/hw/display/dm163.h b/include/hw/display/dm163.h
new file mode 100644
index 0000000000..00d0504640
--- /dev/null
+++ b/include/hw/display/dm163.h
@@ -0,0 +1,58 @@
+/*
+ * QEMU DM163 8x3-channel constant current led driver
+ * driving columns of associated 8x8 RGB matrix.
+ *
+ * Copyright (C) 2024 Samuel Tardieu <s...@rfc1149.net>
+ * Copyright (C) 2024 Arnaud Minier <arnaud.min...@telecom-paris.fr>
+ * Copyright (C) 2024 Inès Varhol <ines.var...@telecom-paris.fr>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_DISPLAY_DM163_H
+#define HW_DISPLAY_DM163_H
+
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+
+#define TYPE_DM163 "dm163"
+OBJECT_DECLARE_SIMPLE_TYPE(DM163State, DM163);
+
+#define DM163_NUM_LEDS 24
+#define RGB_MATRIX_NUM_ROWS 8
+#define RGB_MATRIX_NUM_COLS (DM163_NUM_LEDS / 3)
Maybe better as:
#define DM163_NUM_LEDS (RGB_MATRIX_NUM_COLS * RGB_MATRIX_NUM_ROWS)
+#define COLOR_BUFFER_SIZE RGB_MATRIX_NUM_ROWS
It could ease the code to define here directly as:
/* The last row is filled with 0 (turned off row) */
#define COLOR_BUFFER_SIZE (RGB_MATRIX_NUM_ROWS + 1)
+
+typedef struct DM163State {
+ DeviceState parent_obj;
+
+ /* DM163 driver */
+ uint64_t bank0_shift_register[3];
+ uint64_t bank1_shift_register[3];
+ uint16_t latched_outputs[DM163_NUM_LEDS];
+ uint16_t outputs[DM163_NUM_LEDS];
+ qemu_irq sout;
+
+ uint8_t sin;
+ uint8_t dck;
+ uint8_t rst_b;
+ uint8_t lat_b;
+ uint8_t selbk;
+ uint8_t en_b;
+
+ /* IM120417002 colors shield */
+ uint8_t activated_rows;
+
+ /* 8x8 RGB matrix */
+ QemuConsole *console;
+ uint8_t redraw;
+ /* Rows currently being displayed on the matrix. */
+ /* The last row is filled with 0 (turned off row) */
+ uint32_t buffer[COLOR_BUFFER_SIZE + 1][RGB_MATRIX_NUM_COLS];
+ uint8_t last_buffer_idx;
+ uint8_t buffer_idx_of_row[RGB_MATRIX_NUM_ROWS];
+ /* Used to simulate retinal persistence of rows */
+ uint8_t age_of_row[RGB_MATRIX_NUM_ROWS];
Maybe "row_persistence_delay"?
+} DM163State;
+
+#endif /* HW_DISPLAY_DM163_H */
+static void dm163_dck_gpio_handler(void *opaque, int line, int new_state)
+{
+ DM163State *s = DM163(opaque);
GPIO handlers are initialized in dm163_realize() where we know @dev
is already a DM163State:
static void dm163_realize(DeviceState *dev, Error **errp)
{
DM163State *s = DM163(dev);
^^^^^
qdev_init_gpio_in(dev, dm163_rows_gpio_handler, 8);
So here (and other handlers) you can avoid the QOM cast macro,
and directly use:
DM163State *s = opaque;
+
+ if (new_state && !s->dck) {
+ /*
+ * On raising dck, sample selbk to get the bank to use, and
+ * sample sin for the bit to enter into the bank shift buffer.
+ */
+ uint64_t *sb =
+ s->selbk ? s->bank1_shift_register : s->bank0_shift_register;
+ /* Output the outgoing bit on sout */
+ const bool sout = (s->selbk ? sb[2] & MAKE_64BIT_MASK(63, 1) :
+ sb[2] & MAKE_64BIT_MASK(15, 1)) != 0;
+ qemu_set_irq(s->sout, sout);
+ /* Enter sin into the shift buffer */
+ sb[2] = (sb[2] << 1) | ((sb[1] >> 63) & 1);
+ sb[1] = (sb[1] << 1) | ((sb[0] >> 63) & 1);
+ sb[0] = (sb[0] << 1) | s->sin;
+ }
+
+ s->dck = new_state;
+ trace_dm163_dck(new_state);
+}
+static void dm163_en_b_gpio_handler(void *opaque, int line, int new_state)
+{
+ DM163State *s = DM163(opaque);
+
+ s->en_b = new_state;
+ dm163_propagate_outputs(s);
+ trace_dm163_en_b(new_state);
+}
+
+static inline uint8_t dm163_bank0(const DM163State *s, uint8_t led)
No need to force the compiler to inline these methods.
+{
+ /*
+ * Bank 1 uses 6 bits per led, so a value may be stored accross
+ * two uint64_t entries.
+ */
+ const uint8_t low_bit = 6 * led;
+ const uint8_t low_word = low_bit / 64;
+ const uint8_t high_word = (low_bit + 5) / 64;
+ const uint8_t low_shift = low_bit % 64;
+
+ if (low_word == high_word) {
+ /* Simple case: the value belongs to one entry. */
+ return (s->bank0_shift_register[low_word] &
+ MAKE_64BIT_MASK(low_shift, 6)) >> low_shift;
+ }
+
+ const uint8_t bits_in_low_word = 64 - low_shift;
+ const uint8_t bits_in_high_word = 6 - bits_in_low_word;
+ return ((s->bank0_shift_register[low_word] &
+ MAKE_64BIT_MASK(low_shift, bits_in_low_word)) >>
+ low_shift) |
+ ((s->bank0_shift_register[high_word] &
+ MAKE_64BIT_MASK(0, bits_in_high_word))
+ << bits_in_low_word);
+}
+
+static inline uint8_t dm163_bank1(const DM163State *s, uint8_t led)
+{
+ const uint64_t entry = s->bank1_shift_register[led / 8];
+ const unsigned shift = 8 * (led % 8);
+ return (entry & MAKE_64BIT_MASK(shift, 8)) >> shift;
+}
+static void dm163_realize(DeviceState *dev, Error **errp)
+{
+ DM163State *s = DM163(dev);
+
+ qdev_init_gpio_in(dev, dm163_rows_gpio_handler, 8);
s/8/RGB_MATRIX_NUM_ROWS/
+ qdev_init_gpio_in(dev, dm163_sin_gpio_handler, 1);
+ qdev_init_gpio_in(dev, dm163_dck_gpio_handler, 1);
+ qdev_init_gpio_in(dev, dm163_rst_b_gpio_handler, 1);
+ qdev_init_gpio_in(dev, dm163_lat_b_gpio_handler, 1);
+ qdev_init_gpio_in(dev, dm163_selbk_gpio_handler, 1);
+ qdev_init_gpio_in(dev, dm163_en_b_gpio_handler, 1);
+ qdev_init_gpio_out_named(dev, &s->sout, "sout", 1);
+
+ s->console = graphic_console_init(dev, 0, &dm163_ops, s);
+ qemu_console_resize(s->console, RGB_MATRIX_NUM_COLS * LED_SQUARE_SIZE,
+ RGB_MATRIX_NUM_ROWS * LED_SQUARE_SIZE);
+}
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 2336a0ca15..fc7cbdcd36 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
+# dm163.c
+dm163_redraw(uint8_t redraw) "0x%02x"
+dm163_dck(int new_state) "dck : %d"
+dm163_en_b(int new_state) "en_b : %d"
+dm163_rst_b(int new_state) "rst_b : %d"
+dm163_lat_b(int new_state) "lat_b : %d"
+dm163_sin(int new_state) "sin : %d"
+dm163_selbk(int new_state) "selbk : %d"
unsigned new_state, "%u"
+dm163_activated_rows(int new_state) "Activated rows : 0x%" PRIx32 ""
+dm163_bits_ppi(unsigned dest_width) "dest_width : %u"
+dm163_leds(int led, uint32_t value) "led %d: 0x%x"
+dm163_channels(int channel, uint8_t value) "channel %d: 0x%x"
+dm163_refresh_rate(uint32_t rr) "refresh rate %d"
Minor comments, otherwise LGTM!
Regards,
Phil.
