Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220303173935.100622-1-cmirabil@redhat.com> <20220303173935.100622-4-cmirabil@redhat.com>
In-Reply-To: <20220303173935.100622-4-cmirabil@redhat.com>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Thu, 3 Mar 2022 21:19:35 +0100
Message-ID: <CANiq72=vTm-_MwPMREr5Z61022AEQzcOs+kTgdtz6zgoFpazGA@mail.gmail.com>
Subject: Re: [PATCH v7 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense
 HAT display driver
To:     Charles Mirabile <cmirabil@redhat.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Serge Schneider <serge@raspberrypi.org>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Nicolas Saenz Julienne <nsaenzju@redhat.com>,
        Mattias Brugger <mbrugger@suse.com>,
        linux-rpi-kernel@lists.infradead.org,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        fedora-rpi@googlegroups.com, Miguel Ojeda <ojeda@kernel.org>,
        Daniel Bauman <dbauman@redhat.com>,
        Mwesigwa Guma <mguma@redhat.com>,
        Joel Savitz <jsavitz@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 3, 2022 at 6:40 PM Charles Mirabile <cmirabil@redhat.com> 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@raspberrypi.org>");

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

Cheers,
Miguel