linux-kernel - Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1c68fa67-c4c4-ea78-bfe4-6344799bbf46@collabora.com>
Date:   Mon, 23 Dec 2019 15:27:54 +0100
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Nicolas Boichat <drinkcat@...omium.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Collabora Kernel ML <kernel@...labora.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Jitao Shi <jitao.shi@...iatek.com>,
        Daniel Kurtz <djkurtz@...omium.org>,
        Ulrich Hecht <uli@...nd.eu>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Jonas Karlman <jonas@...boo.se>,
        dri-devel@...ts.freedesktop.org,
        Neil Armstrong <narmstrong@...libre.com>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        David Airlie <airlied@...ux.ie>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640
 bridge

Hi Nicolas,

On 23/12/19 10:14, Nicolas Boichat wrote:
> On Mon, Dec 23, 2019 at 3:10 PM Enric Balletbo i Serra
> <enric.balletbo@...labora.com> wrote:
>>
>> Hi Nicolas,
>>
>> Many thanks for you review. Just preparing a new version with your comments
>> addressed.
>>
>> On 20/12/19 9:44, Nicolas Boichat wrote:
>>> On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra
>>> <enric.balletbo@...labora.com> wrote:
>>>>
>>>> From: Jitao Shi <jitao.shi@...iatek.com>
>>>>
>>>> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
>>>>
>>>> Signed-off-by: Jitao Shi <jitao.shi@...iatek.com>
>>>> Reviewed-by: Daniel Kurtz <djkurtz@...omium.org>
>>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>>>> [uli: followed API changes, removed FW update feature]
>>>> Signed-off-by: Ulrich Hecht <uli@...nd.eu>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>>>> ---
>> [snip]
>>>> +       ret = i2c_smbus_write_byte_data(client, PAGE2_MCS_EN,
>>>> +                                       status & ~MCS_EN);
>>>> +       if (ret < 0) {
>>>> +               DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
>>>> +               goto err_regulators_disable;
>>>> +       }
>>>> +
>>>> +       ret = ps8640_bridge_unmute(ps_bridge);
>>>> +       if (ret)
>>>> +               DRM_ERROR("failed to enable unmutevideo: %d\n", ret);
>>>
>>> failed to unmute? Or failed to enable?
>>>
>>
>> failed to unmute sound more clear to me.
> 
> I may be wrong, but I have the feeling that the functions
> "mute/unmute" video/display, actually... And that the function naming
> is strange...
> 

Yes, that's strange.

> You could just try to remove the calls, as there is no audio on the
> board you have (elm), so if video still works, maybe this is actually
> audio ,-)
> 

And without those the display doesn't work. So I suspect that what is wrong and
confusing is the message, instead of mute/unmute, and based on the register
names this looks more like an internal regulator that you need to enable and
disable, so I'll change the error message accordingly.

Thanks,
 Enric

> Thanks,
>