linux-kernel - Re: [PATCH v4 5/5] drm/solomon: Add SSD130x OLED displays SPI support

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: <ef51a171-790e-1c13-fc9a-a4c9a2ab5863@redhat.com>
Date:   Tue, 19 Apr 2022 10:02:33 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Chen-Yu Tsai <wens@...nel.org>, Rob Herring <robh@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Chen-Yu Tsai <wens@...e.org>, Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Maxime Ripard <maxime@...no.tech>,
        YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH v4 5/5] drm/solomon: Add SSD130x OLED displays SPI support

Hello Geert,

Thanks a lot for your feedback.

On 4/19/22 09:52, Geert Uytterhoeven wrote:

[snip]

>> +static int ssd130x_spi_write(void *context, const void *data, size_t count)
>> +{
>> +       struct ssd130x_spi_transport *t = context;
>> +       struct spi_device *spi = t->spi;
>> +       const u8 *reg = data;
>> +
>> +       if (*reg == SSD130X_COMMAND)
>> +               gpiod_set_value_cansleep(t->dc, 0);
>> +
>> +       if (*reg == SSD130X_DATA)
>> +               gpiod_set_value_cansleep(t->dc, 1);
>> +
>> +       /* Remove the control byte since is not used by the 4-wire SPI */
>> +       return spi_write(spi, ((u8 *)data) + 1, count - 1);
> 
> As I don't like casts, perhaps
> 
>     spi_write(spi, reg + 1, count - 1);
> 
> ? But this is up to you.
>

It's true that is easier to read. I just wanted to make it clear that we
were removing one byte from the data but I believe the comment is enough.

Andy also pointed out an unnecessary blank line in patch 4/5, so I think
these two changes + your R-b warrants a v5. I will post one later today.
 
[snip]

>> +static const struct spi_device_id ssd130x_spi_table[] = {
>> +       { "sh1106",  SH1106_ID },
>> +       { "ssd1305", SSD1305_ID },
>> +       { "ssd1306", SSD1306_ID },
>> +       { "ssd1307", SSD1307_ID },
>> +       { "ssd1309", SSD1309_ID },
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ssd130x_spi_table);
> 
> I'm not sure about the need for this part, but as Mark provided his
> Ac-ed--by, I assume it's correct.
>

Right, I'm quite sure about this. See for example [0] vs [1]. The latter
does of_device_uevent_modalias(dev, env) while the former always does
add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias)
even for devices registered through OF.

Also, commits 3ce6c9e2617e ("spi: add of_device_uevent_modalias support")
and 96c8395e2166 ("spi: Revert modalias changes") have some more context.

[0]: https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L360
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L139
 
> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>
>

Thanks!

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat