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

[Emil Velikov]

On 8 June 2016 at 11:06, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Wed, 8 Jun 2016 10:39:11 +0100
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> Hi Boris,
>>
>> On 7 June 2016 at 12:59, Boris Brezillon
>> <boris.brezillon at 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 at free-electrons.com>
>> > Tested-by: Nicolas Ferre <nicolas.ferre at 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.
>
Indeed. This behaviour always got be confused :-\

Thanks for covering my comments/suggestions.
Emil