Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200822163250.63664-1-paul@crapouillou.net> <20200822163250.63664-4-paul@crapouillou.net>
In-Reply-To: <20200822163250.63664-4-paul@crapouillou.net>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Wed, 9 Sep 2020 13:57:48 +0200
Message-ID: <CACRpkdaN7S9PoYd2d-hxUNq9NXA52P0TugwRzyFuCTCc6Onp=Q@mail.gmail.com>
Subject: Re: [PATCH v2 3/6] drm: Add SPI DBI host driver
To:     Paul Cercueil <paul@crapouillou.net>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Noralf Tronnes <noralf@tronnes.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        od@zcrc.me,
        "open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Mark Brown <broonie@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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