Hi Inès,
On 21/4/24 16:02, 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 | 59 +++++
hw/display/dm163.c | 334 +++++++++++++++++++++++++++++
hw/display/Kconfig | 3 +
hw/display/meson.build | 1 +
hw/display/trace-events | 14 ++
6 files changed, 413 insertions(+), 1 deletion(-)
create mode 100644 include/hw/display/dm163.h
create mode 100644 hw/display/dm163.c
+static void dm163_propagate_outputs(DM163State *s)
+{
+ s->last_buffer_idx = (s->last_buffer_idx + 1) % RGB_MATRIX_NUM_ROWS;
+ /* Values are output when reset is high and enable is low. */
+ if (s->rst_b && !s->en_b) {
+ memcpy(s->outputs, s->latched_outputs, sizeof(s->outputs));
+ } else {
+ memset(s->outputs, 0, sizeof(s->outputs));
+ }
+ for (unsigned x = 0; x < RGB_MATRIX_NUM_COLS; x++) {
+ trace_dm163_channels(3 * x, (uint8_t)(s->outputs[3 * x] >> 6));
+ trace_dm163_channels(3 * x + 1, (uint8_t)(s->outputs[3 * x + 1] >> 6));
+ trace_dm163_channels(3 * x + 2, (uint8_t)(s->outputs[3 * x + 2] >> 6));
+ s->buffer[s->last_buffer_idx][x] =
+ (s->outputs[3 * x + 2] >> 6) |
+ ((s->outputs[3 * x + 1] << 2) & 0xFF00) |
+ (((uint32_t)s->outputs[3 * x] << 10) & 0xFF0000);
Alternatively easier to review as:
uint16_t x0 = extract16(s->outputs[3 * x + 0], 6, 8);
uint16_t x1 = extract16(s->outputs[3 * x + 1], 6, 8);
uint16_t x2 = extract16(s->outputs[3 * x + 2], 6, 8);
uint32_t val = 0;
trace_dm163_channels(3 * x + 0, x0);
trace_dm163_channels(3 * x + 1, x1);
trace_dm163_channels(3 * x + 2, x2);
val = deposit32(val, 0, 8, x2);
val = deposit32(val, 8, 8, x1);
val = deposit32(val, 16, 8, x0);
s->buffer[s->last_buffer_idx][x] = val;
+ }
+ for (unsigned row = 0; row < RGB_MATRIX_NUM_ROWS; row++) {
+ if (s->activated_rows & (1 << row)) {
+ s->buffer_idx_of_row[row] = s->last_buffer_idx;
+ s->redraw |= (1 << row);
+ trace_dm163_redraw(s->redraw);
+ }
+ }
+}
+static uint8_t dm163_bank0(const DM163State *s, uint8_t led)
+{
+ /*
+ * 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;
FYI note the BIT* macros in "qemu/bitops.h" (but you'd need to
use arrays of unsigned long instead of uint64_t).
+
+ 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);
Splitting this (assigning intermediate variables) could benefit
code review (see earlier suggestion).
+}
+
+static uint8_t dm163_bank1(const DM163State *s, uint8_t led)
+{
+ const uint64_t entry = s->bank1_shift_register[led / RGB_MATRIX_NUM_COLS];
+ const unsigned shift = 8 * (led % RGB_MATRIX_NUM_COLS);
+ return (entry & MAKE_64BIT_MASK(shift, 8)) >> shift;
Or simply:
return extract64(entry, 8 * (led % RGB_MATRIX_NUM_COLS), 8);
+}
Anyhow, I only gave nitpicking comments to improve readability,
model LGTM! :)
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Regards,
Phil.
