Re: [PATCH v5 2/2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select

[Marek Vasut <marex@denx.de>]

On 11/24/21 04:02, Marek Vasut wrote:
> On 10/24/21 1:04 AM, Marek Vasut wrote:
>> On 10/17/21 7:40 PM, Sam Ravnborg wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sun, Oct 17, 2021 at 07:29:51PM +0200, Marek Vasut wrote:
>>>> On 10/17/21 6:49 PM, Sam Ravnborg wrote:
>>>>
>>>> [...]
>>>>
>>>>>> +    /*
>>>>>> +     * Encoder might sample data on different clock edge than the 
>>>>>> display,
>>>>>> +     * for example OnSemi FIN3385 has a dedicated strapping pin 
>>>>>> to select
>>>>>> +     * the sampling edge.
>>>>>> +     */
>>>>>> +    if (lvds_codec->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>>>>>> +        !of_property_read_u32(dev->of_node, "pclk-sample", &val)) {
>>>>>> +        lvds_codec->timings.input_bus_flags = val ?
>>>>>> +            DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE :
>>>>>> +            DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
>>>>>> +    }
>>>>>> +
>>>>>>        /*
>>>>>>         * The panel_bridge bridge is attached to the panel's of_node,
>>>>>>         * but we need a bridge attached to our of_node for our user
>>>>>>         * to look up.
>>>>>>         */
>>>>>>        lvds_codec->bridge.of_node = dev->of_node;
>>>>>> +    lvds_codec->bridge.timings = &lvds_codec->timings;
>>>>> I do not understand how this will work. The only field that is set 
>>>>> is timings.input_bus_flags
>>>>> but any user will see bridge.timings is set and will think this is all
>>>>> timing info.
>>>>>
>>>>> Maybe I just misses something obvious?
>>>>
>>>> Is there anything else in those timings that should be set ? See
>>>> include/drm/drm_bridge.h around line 640
>>>>
>>>> setup_time_ps/hold_time_ps/dual_link isn't supported by this driver, 
>>>> so it
>>>> is 0 or false anyway, i.e. no change.
>>>
>>> Just me being confused with display_timings. Patch looks good.
>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>>
>>> Ping me in a few days to apply it if there is no more feedback.
>>
>> Ping I guess ... Laurent ?
> 
> Ping one more time ?

Ping yet again ?