Re: [PATCH v5 1/5] hw/display : Add device DM163

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.francis@wdc.com>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@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é <philmd@linaro.org>

Regards,

Phil.