Re: [PATCH v6 1/2] drm/bridge: Add sii902x driver

From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>, Meng Yi <meng.yi@nxp.com>,
	Xiubo Li <lixiubo@cmss.chinamobile.com>,
	Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Alison Wang <alison.wang@nxp.com>,
	Nicolas Ferre <nicolas.ferre@atmel.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	devicetree <devicetree@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Alexandre Belloni <alexandre.belloni@free-electrons.com>,
	Kumar Gala <galak@codeaurora.org>,
	Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,
	Jianwei Wang <jianwei.wang.chn@gmail.com>
Subject: Re: [PATCH v6 1/2] drm/bridge: Add sii902x driver
Date: Wed, 8 Jun 2016 12:06:09 +0200	[thread overview]
Message-ID: <20160608120609.1e3f8e43@bbrezillon> (raw)
In-Reply-To: <CACvgo51drsxyZA1YdDx=2x-idq8CKQkJYyPyywhHrGTTjnOzJA@mail.gmail.com>

On Wed, 8 Jun 2016 10:39:11 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> Hi Boris,
> 
> On 7 June 2016 at 12:59, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Add basic support for the sii902x RGB -> HDMI bridge.
> > This driver does not support audio output yet.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> > ---
> > Hello,
> >
> > This patch is only adding basic support for the sii9022 chip.
> > As stated in the commit log, there's no audio support, but the
> > driver also hardcodes a lot of things (like the RGB input format to
> > use).
> >
> > Best Regards,
> >
> > Boris
> >
> > Changes in v6:
> > - use HDMI_INFOFRAME_SIZE(AVI)
> > - fix reset_gpio initialization  
> I'm afraid that this one is still busted :-( See below
> 
> 
> > +       sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +                                                     GPIOD_OUT_LOW);  
> Documentation states required, above we use optional.

Argh! Forgot to update the documentation.

> 
> > +       if (IS_ERR(sii902x->reset_gpio)) {  
> devm_gpiod_get_optional returns NULL on error, yet we use IS_ERR.

It returns NULL or a valid pointer on success, and an err-ptr otherwise.
Here NULL is a valid value, since the GPIO is optional, and the reset
function is just a NOP in this case.

> 
> > +               dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n",
> > +                       PTR_ERR(sii902x->reset_gpio));
> > +               return PTR_ERR(sii902x->reset_gpio);  
> If the gpio is truly optional we shouldn't return here. Alternatively
> the reset_gpio NULL check in sii902x_reset() can go.

No, if the GPIO is not defined in the DT, NULL is returned and we never
reach this place.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel