[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 9 Sep 2020 13:57:48 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Paul Cercueil <paul@...pouillou.net>
Cc: Thierry Reding <thierry.reding@...il.com>,
Sam Ravnborg <sam@...nborg.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Rob Herring <robh+dt@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Noralf Tronnes <noralf@...nnes.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
od@...c.me,
"open list:DRM PANEL DRIVERS" <dri-devel@...ts.freedesktop.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>
Subject: Re: [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@...pouillou.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
Powered by blists - more mailing lists