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

[Marek Vasut]

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.

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 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.