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

[Frieder Schrempf]

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.

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.