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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANiq72kSdjRgBN1D-de9xKQuKRm7XqX6=1tbi61oouMS6KGTbA@mail.gmail.com>
Date:   Tue, 8 Feb 2022 15:00:16 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Charles Mirabile <cmirabil@...hat.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Serge Schneider <serge@...pberrypi.org>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Nicolas Saenz Julienne <nsaenzju@...hat.com>,
        Mattias Brugger <mbrugger@...e.com>,
        linux-rpi-kernel@...ts.infradead.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        fedora-rpi@...glegroups.com, Miguel Ojeda <ojeda@...nel.org>,
        Mwesigwa Guma <mguma@...hat.com>,
        Joel Savitz <jsavitz@...hat.com>,
        Daniel Bauman <dbauman@...hat.com>
Subject: Re: [PATCH 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT
 display driver

On Thu, Feb 3, 2022 at 1:26 AM Charles Mirabile <cmirabil@...hat.com> wrote:
>
> This patch adds the driver for the 8x8 RGB LED matrix display
> on the Sense HAT. It appears as a character device named sense-hat
> in /dev/. That special file is 192 bytes large and contains one byte
> for each color channel of each pixel. The overall layout is:

A general comment about the commit message: I am not sure why the
format of the file is included here -- shouldn't this be placed in
comments in the code or in Documentation? Is it elsewhere in the
series?

>     col 1       col 8
>       |           |
>       v           v
> 0x00: R   . . .   R \
> 0x08: G   . . .   G |> row 1
> 0x10: B   . . .   B /
>       .   .       .
> ...   .     .     .
>       .       .   .
> 0xA8: R   . . .   R \
> 0xB0: G   . . .   G |> row 8
> 0xB8: G   . . .   G /

Do you mean B instead of G in row 8?

By the way, is there a particular reason for this layout vs., say, RGB tuples?

> Each channel may have any value between 0 and 31 (larger values are
> wrapped) and are translated by a gamma table before being send to the
> device to provide a linear experience of brightness (the gamma table
> can be modified or reset using an ioctl call on the file). The constants
> for the ioctl are in the sensehat.h header also added in this patch.

Even though there is a gamma table, it is unclear what the input
encoding is supposed to be. sRGB? Linear sRGB?

In any case, where the gamma tables' values come from? i.e. the
device's datasheet (or sRGB, or a combination of those), or hand-made,
...?

Also, should this conversion be handled by the driver? If yes, why not
use the full range 0..255 as input?

> +       for (i = 0; i < VMEM_SIZE; ++i)
> +               temp[i] = display->gamma[display->vmem[i] & 0x1f];

Please avoid hardcoded constants (and, in particular, this is in hex
while the related constant `GAMMA_SIZE` is in decimal, so it is even
harder to see). Or perhaps use a small helper function or macro.

> +       pos = base + offset;
> +       if (pos < 0 || pos >= VMEM_SIZE)
> +               return -EINVAL;
> +       filp->f_pos = pos;

Please check if one of the generic `llseek`'s would work here (e.g.
`fixed_size_llseek`).

> +       memcpy(sensehat_display->gamma, gamma_presets[lowlight], GAMMA_SIZE);

Here it is confusing which table you are copying due to the `lowlight`
name. I would write `lowlight ? SENSEHAT_GAMMA_LOWLIGHT :
SENSEHAT_GAMMA_DEFAULT` even if it is redundant.

(Given you created an enum for the presets, it may make sense to use
something else than a `bool`, by the way).

Cheers,
Miguel