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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
>

Thanks!

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux