Hi Paul,
I looked a bit at this patch
On Sat, Aug 22, 2020 at 6:33 PM Paul Cercueil paul@crapouillou.net wrote:
+config DRM_MIPI_DBI_SPI
tristate "SPI host support for MIPI DBI"depends on DRM && OF && SPI
I think you want to depend on SPI_HOST actually.
struct gpio_desc *dc;
This dc is very much undocumented, so I can only guess what it is for, please add some kerneldoc explaining what it is. I suppose it is in the DBI spec but I don't have it.
gpiod_set_value_cansleep(dbi->dc, 0);
Since it is optional I usually prefer to do it like this:
if (dbi->dc) gpiod_set_value_cansleep(dbi->dc, 0);
- gpiod_set_value_cansleep(dbi->dc, 1);
Since you drive this low when you assert it and high when you de-assert it, inverse this and mark the GPIO line as GPIO_ACTIVE_LOW in the device tree instead.
dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);if (IS_ERR(dbi->dc)) {dev_err(dev, "Failed to get gpio 'dc'\n");return PTR_ERR(dbi->dc);}
Currently you are requesting the line asserted according to the above logic. Is that what you want?
If you change the DT to GPIO_ACTIVE_LOW then this seems correct.
But I am overall a bit curious about this "dc" thing. What is it really? It seems suspiciously similar to a SPI chip select, and then that is something that should be handled by the SPI core and set as cs-gpios in the device tree instead. It is certainly used exactly like a chip select as far as I can tell.
Yours, Linus Walleij