Re: [PATCH v7 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver

[Miguel Ojeda]

On Thu, Mar 3, 2022 at 6:40 PM Charles Mirabile <cmirabil@xxxxxxxxxx> wrote:
>
> +       u8 temp[8][3][8];
> +
> +       for (i = 0; i < 8; ++i) {
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][0][j] = display->vmem[i][j].r;
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][1][j] = display->vmem[i][j].g;
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][2][j] = display->vmem[i][j].b;
> +       }

I assume the hardware uses only rgb555, right? I think this is OK, but
please consider if it would make sense to read a 8x8x3 bytes matrix
anyway, even if some bits/values are ignored/discarded.

> +       struct sensehat_display *sensehat_display =
> +               devm_kmalloc(&pdev->dev, sizeof(*sensehat_display), GFP_KERNEL);

Missing some error handling, e.g. here and in `dev_get_regmap`.

Also, if `sensehat_update_display` could return an optional error, you
could also fail if the first update did not work (assuming it is
supposed to work), or maybe warn (if it can fail for some reason but
it does not mean future updates would fail).

> +MODULE_AUTHOR("Serge Schneider <serge@xxxxxxxxxxxxxxx>");

Please consider if you should/want to appear here alongside Serge.

Cheers,
Miguel