[PATCH V2 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver

[Neil Armstrong]

On 29/04/2021 18:27, Frieder Schrempf wrote:
> On 28.04.21 16:16, Marek Vasut wrote:
>> On 4/28/21 11:24 AM, Neil Armstrong wrote:
>> [...]
>>
>>>>>>> +static int sn65dsi83_probe(struct i2c_client *client,
>>>>>>> +               const struct i2c_device_id *id)
>>>>>>> +{
>>>>>>> +    struct device *dev = &client->dev;
>>>>>>> +    enum sn65dsi83_model model;
>>>>>>> +    struct sn65dsi83 *ctx;
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>>>>> +    if (!ctx)
>>>>>>> +        return -ENOMEM;
>>>>>>> +
>>>>>>> +    ctx->dev = dev;
>>>>>>> +
>>>>>>> +    if (dev->of_node)
>>>>>>> +        model = (enum sn65dsi83_model)of_device_get_match_data(dev);
>>>>>>> +    else
>>>>>>> +        model = id->driver_data;
>>>>>>> +
>>>>>>> +    /* Default to dual-link LVDS on all but DSI83. */
>>>>>>> +    if (model != MODEL_SN65DSI83)
>>>>>>> +        ctx->lvds_dual_link = true;
>>>>>>
>>>>>> What if I use the DSI84 with a single link LVDS? I can't see any way to
>>>>>> configure that right now.
>>>>
>>>> I assume the simplest way would be to use the "ti,sn65dsi83"
>>>> compatible string in your dts, since the way you wired it is
>>>> 'compatible' with sn65dsi83, right?
>>>
>>> No this isn't the right way to to, if sn65dsi84 is supported and the bindings only support single lvds link,
>>> the driver must only support single link on sn65dsi84, or add the dual link lvds in the bindings only for sn65dsi84.
>>
>> The driver has a comment about what is supported and tested, as Frieder already pointed out:
>>
>> Currently supported:
>> - SN65DSI83
>>    = 1x Single-link DSI ~ 1x Single-link LVDS
>>    - Supported
>>    - Single-link LVDS mode tested
>> - SN65DSI84
>>    = 1x Single-link DSI ~ 2x Single-link or 1x Dual-link LVDS
>>    - Supported
>>    - Dual-link LVDS mode tested
>>    - 2x Single-link LVDS mode unsupported
>>      (should be easy to add by someone who has the HW)
>> - SN65DSI85
>>    = 2x Single-link or 1x Dual-link DSI ~ 2x Single-link or 1x Dual-link LVDS
>>    - Unsupported
>>      (should be easy to add by someone who has the HW)
>>
>> So,
>> DSI83 is always single-link DSI, single-link LVDS.
>> DSI84 is always single-link DSI, and currently always dual-link LVDS.
>>
>> The DSI83 can do one thing on the LVDS end:
>> - 1x single link lVDS
>>
>> The DSI84 can do two things on the LVDS end:
>> - 1x single link lVDS
>> - 1x dual link LVDS
>>
>> There is also some sort of mention in the DSI84 datasheet about 2x single link LVDS, but I suspect that might be copied from DSI85 datasheet instead, which would make sense. The other option is that it behaves as a mirror (i.e. same pixels are scanned out of LVDS channel A and B). Either option can be added by either adding a DT property which would enable the mirror mode, or new port linking the LVDS endpoint to the same panel twice, and/or two new ports for DSI85, so there is no problem to extend the bindings without breaking them. So for now I would ignore this mode.
> 
> Agreed.
> 
>>
>> So ultimately, what we have to sort out is the 1x single / 1x dual link LVDS mode setting on DSI84. Frieder already pointed out how the driver can be tweaked to support the single-link mode on DSI84, so now we need to tie it into DT bindings.
>>
>> Currently, neither the LVDS panels in upstream in panel-simple nor lvds.yaml provide any indication that the panel is single-link or dual-link. Those dual-link LVDS panels seem to always set 2x pixel clock and let the bridge somehow sort it out.
>>
>> Maybe that isn't always the best approach, maybe we should add a new DRM_BUS_FLAG for those panels and handle the flag in the bridge driver ? Such a new flag could be added over time to panels where applicable, so old setups won't be broken by that either, they will just ignore the new flag and work as before.
> 
> I agree that the panel driver should report whether the LVDS input is expected to be single or dual link. So introducing a DRM_BUS_FLAG for this seems like a good solution.
> 
> This wouldn't reflect the physical ports, but I don't really know how we could use two ports connected to the same panel for this case, as all the existing dual link panels don't follow this scheme.

A dual-link LVDS panel should need 2 ports, because each LVDS link could come from different controllers, here by the same but simply connect the 2 panel ports to the 2 controller's ports.

Neil

> 
> I would suggest, that the driver defaults to single link for the DSI84 and then switches to dual link if the panel requests it using the flag.
> 
>>
>>>>> I just saw the note in the header of the driver that says that single
>>>>> link mode is unsupported for the DSI84.
>>>>>
>>>>> I have hardware with a single link display and if I set
>>>>> ctx->lvds_dual_link = false it works just fine.
>>>>>
>>>>> How is this supposed to be selected? Does it need an extra devicetree
>>>>> property? And would you mind adding single-link support in the next
>>>>> version or do you prefer adding it in a follow-up patch?
>>>>
>>>> If this has to be supported I think the proper way would be to support
>>>> two output ports in the dts (e.g. lvds0_out, lvds1_out), in the same
>>>> way as supported by the 'advantech,idk-2121wr' panel.
>>>
>>> Yes, this is why I asked to have the dual-link lvds in the bindings.
>>
>> Maybe it shouldn't really be in the bindings, see above.