dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Frieder Schrempf <frieder.schrempf@kontron.de>
To: Marek Vasut <marex@denx.de>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Loic Poulain <loic.poulain@linaro.org>
Cc: ch@denx.de, Douglas Anderson <dianders@chromium.org>,
	dri-devel@lists.freedesktop.org,
	Stephen Boyd <swboyd@chromium.org>,
	Philippe Schenker <philippe.schenker@toradex.com>,
	Jagan Teki <jagan@amarulasolutions.com>,
	Valentin Raevsky <valentin@compulab.co.il>,
	Sam Ravnborg <sam@ravnborg.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH V2 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver
Date: Thu, 29 Apr 2021 18:27:13 +0200	[thread overview]
Message-ID: <b1a58421-5032-09d2-c95d-5ac6fd38c118@kontron.de> (raw)
In-Reply-To: <a91a379a-2d3a-fe31-98b5-194bf648bc44@denx.de>

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

  reply	other threads:[~2021-04-29 16:27 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-21 22:31 [PATCH V2 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 bindings Marek Vasut
2021-04-21 22:31 ` [PATCH V2 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver Marek Vasut
2021-04-23 16:03   ` Loic Poulain
2021-04-28  7:51   ` Frieder Schrempf
2021-04-28  8:13     ` Frieder Schrempf
2021-04-28  9:26       ` Loic Poulain
2021-04-28  9:24         ` Neil Armstrong
2021-04-28  9:49           ` Jagan Teki
2021-04-28 14:18             ` Marek Vasut
2021-04-28 14:16           ` Marek Vasut
2021-04-29 16:27             ` Frieder Schrempf [this message]
2021-04-30  8:34               ` Neil Armstrong
2021-04-21 22:56 ` [PATCH V2 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 bindings Laurent Pinchart
2021-04-22  8:38 ` Neil Armstrong
2021-04-22 16:16   ` Marek Vasut
2021-04-22  8:43 ` Jagan Teki
2021-04-22 16:22   ` Marek Vasut
2021-04-28  7:56 ` Frieder Schrempf
2021-04-28 14:19   ` Marek Vasut
2021-04-27 12:15 [PATCH V2 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver Felix Radensky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b1a58421-5032-09d2-c95d-5ac6fd38c118@kontron.de \
    --to=frieder.schrempf@kontron.de \
    --cc=ch@denx.de \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jagan@amarulasolutions.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=loic.poulain@linaro.org \
    --cc=marex@denx.de \
    --cc=narmstrong@baylibre.com \
    --cc=philippe.schenker@toradex.com \
    --cc=sam@ravnborg.org \
    --cc=swboyd@chromium.org \
    --cc=valentin@compulab.co.il \
    --subject='Re: [PATCH V2 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).