[PATCH v1] drm/tegra: dpaux: Fix always-failing probing of the driver

[Dmitry Osipenko]

On 9/24/18 5:04 PM, Thierry Reding wrote:
> On Mon, Sep 24, 2018 at 03:36:29PM +0200, Thierry Reding wrote:
>> On Mon, Sep 24, 2018 at 03:40:35PM +0300, Dmitry Osipenko wrote:
>>> On 9/24/18 3:32 PM, Jon Hunter wrote:
>>>>
>>>> On 24/09/18 12:59, Thierry Reding wrote:
>>>>> On Fri, Sep 21, 2018 at 02:42:41PM +0300, Dmitry Osipenko wrote:
>>>>>> Some of definitions in the code changed the meaning, unfortunately one
>>>>>> place missed the change.
>>>>>>
>>>>>> Fixes: 0751bb5c44fe ("drm/tegra: dpaux: Add pinctrl support")
>>>>>> Cc: <stable at vger.kernel.org> # v4.8+
>>>>>> Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
>>>>>> ---
>>>>>>
>>>>>> I don't have HW to test DPAUX driver, apparently it has been broken for
>>>>>> 2+ years now. There is also a known issue on with the DPAUX driver that
>>>>>> prevents it from probing, that was discussed on the #tegra IRC. Thierry,
>>>>>> please take a closer look at this driver and test it thoroughly, it has
>>>>>> some obvious problems.
>>>>>>
>>>>>>   drivers/gpu/drm/tegra/dpaux.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> It's odd that you claim that the driver is always failing probe and at
>>>>> the same time you say that you don't have hardware to test the driver.
>>>>> =)
>>>>>
>>>>> I know for a fact that this driver does not usually fail because it is
>>>>> required on all recent chips (Tegra210 and later) to drive HDMI, which
>>>>> we support on all boards, so it is indeed thoroughly tested.
>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
>>>>>> index d84e81ff36ad..ba5681fab73b 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dpaux.c
>>>>>> +++ b/drivers/gpu/drm/tegra/dpaux.c
>>>>>> @@ -521,7 +521,7 @@ static int tegra_dpaux_probe(struct platform_device *pdev)
>>>>>>   	 * is no possibility to perform the I2C mode configuration in the
>>>>>>   	 * HDMI path.
>>>>>>   	 */
>>>>>> -	err = tegra_dpaux_pad_config(dpaux, DPAUX_HYBRID_PADCTL_MODE_I2C);
>>>>>> +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_I2C);
>>>>>>   	if (err < 0)
>>>>>>   		return err;
>>>>>
>>>>> If you look at the definitions of both DPAUX_HYBRID_PADCTL_MODE_I2C and
>>>>> DPAUX_PADCTL_FUNC_I2C, you'll see that both are actually the same, which
>>>>> is a good explanation for why the driver performs flawlessly.
>>>>>
>>>>> That said, your change is obviously correct. I've applied it, but since
>>>>> it doesn't actually fix anything, and doesn't change anything from a
>>>>> binary point of view, I've removed the Fixes: and Cc: stable tags.
>>>>
>>>> Did you change the subject for the patch because as you mentioned it
>>>> does not seem related to the change?
>>>>
>>>> Otherwise for the fix you can have my ...
>>>>
>>>> Acked-by: Jon Hunter <jonathanh at nvidia.com>
>>>>
>>>> The dpaux driver has been working fine for me on Tegra210 Smaug when
>>>> using the pins for I2C and I have not seen any probing problems.
>>>
>>> Guy with nickname "vlado" said on the IRC that the panel stopped to work on
>>> T124 chromebook since 4.16 kernel, reverting commit [0] helps.
>>>
>>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/drm_dp_helper.c?id=17ab7806de0c10d27cdbda8ef57ca680bdd24315
>>
>> I'm pretty sure that's something that we fixed, but I'll have to check.
> 
> Looks like we didn't fix that after all. Going through the patchwork log
> linked to from that commit message I thought there wasn't actually an
> issue with the code, but it turns out that I was wrong. The problem is
> that for HDMI we don't use DPAUX in I2C mode, so we don't need to look
> up the corresponding I2C controller and we just need the DPAUX driver to
> program the pins into I2C mode, then use a real I2C controller for
> communication.
> 
> What's special about Nyan is that it uses eDP and for eDP we use the
> DPAUX for access to the DPCD *and* for DDC. eDP panels will look up the
> phandle to the I2C controller used for EDID (DDC) via the ddc-i2c-bus
> property. In order for that to work, we need to register the DPAUX I2C
> controller with its OF node set to that of the DPAUX device.
> 
> Technically I don't think we need to query the EDID because we always
> have at least one hard-coded mode in panel-simple. However, if the panel
> has a slightly different mode from that, it'd still be good if the
> driver used that instead of the built-in mode.

That also could be bad. The panels EDID mode worked fine in the past,
but it didn't work when I tried it last time. The timing was incorrect
or something like that, maybe something is wrong with the mode parser
(or HW).