lkml.org 
[lkml]   [2020]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 3/6] drm: Add SPI DBI host driver
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

\
 
 \ /
  Last update: 2020-09-09 17:11    [W:0.075 / U:27.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site