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
