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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
