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#L...
The rest LGTM, so Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be
Thanks!