Re: [PATCH v3 17/25] dt-bindings: panel: Add Bananapi S070WV20-CT16 ICN6211 MIPI-DSI to RGB bridge

[Andrzej Hajda <a.hajda@samsung.com>]

On 10.11.2018 08:32, Jagan Teki wrote:
> On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 06.11.2018 19:08, Jagan Teki wrote:
>>> On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 31.10.2018 09:58, Chen-Yu Tsai wrote:
>>>>> On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 26.10.2018 16:43, Jagan Teki wrote:
>>>>>>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB
>>>>>>> bridge panel, which is available on same PCB with 24-bit RGB interface.
>>>>>>>
>>>>>>> So, this patch adds DSI specific binding details on existing
>>>>>>> dt-bindings file.
>>>>>>>
>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>> ---
>>>>>>> Changes for v3:
>>>>>>> - Use existing binding doc and update dsi details
>>>>>>> Changes for v2:
>>>>>>> - none
>>>>>>>
>>>>>>>  .../display/panel/bananapi,s070wv20-ct16.txt  | 31 +++++++++++++++++--
>>>>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> index 35bc0c839f49..b7855dc7c66f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt
>>>>>>> @@ -1,12 +1,39 @@
>>>>>>>  Banana Pi 7" (S070WV20-CT16) TFT LCD Panel
>>>>>>>
>>>>>>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.
>>>>>>> +
>>>>>>> +Depending on the variant, the PCB attached to the panel module either
>>>>>>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
>>>>>>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
>>>>>>> +itself
>>>>>> As I understand this is display board, which contains 'pure' RGB panel
>>>>>> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge.
>>>>>> These are separate devices, just connected by vendor to simplify its
>>>>>> assembly. Why don't you create then bridge driver for ICN6211 and RGB
>>>>>> panel driver for S070WV20-CT16 - it looks more generic.
>>>>>> Then you can describe both in dts and voila.
>>>>>> Creating drivers for every combo of devices (panel + bridge), just
>>>>>> because some vendor sells them together seems incorrect - we have
>>>>>> devicetree for it.
>>>>> Rob suggested this, and also the opposite: using the same
>>>>> "bananapi,s070wv20-ct16"
>>>>> compatible string for both types of connections, and have the driver deal with
>>>>> detecting the bus type.
>>>>>
>>>>> The thing about the bridge chip is that there's no available datasheet that
>>>>> describes all the parts of the init sequence, in fact none at all. I managed
>>>>> to work out some bits, but the others remain a mystery and must be hard-coded
>>>>> to match the panel. That would work against having a generic bridge driver.
>>>> But it is common for many chips - 1st version of the driver is developed
>>>> on one platform and it supports only one configuration, if next platform
>>>> with the same cheap appears the driver is augmented if necessary.
>>> At-least few of the commands from panel initialization code, the
>>> respective opcode data values are based on panel timings and even
>>> clock value is different in DSI. I think it look hard to try bridge
>>> driver for these restrictions, do you have any suggestions?
>>
>> Where do you see an issue? Since panel is RGB it should have no
>> initialization sequence (beside regulator/gpio power on/off), so the
>> only thing to do is to figure out which regulators/gpios belongs to
>> which component - with publicly available specs it should be doable.
>>
>> The whole initialization sequence is for the bridge, so you put it into
>> bridge driver, for starters it can be hardcoded.
> Yes, I understand we can move regulators/gpio setup separately and
> though we hardcode the init sequence there is difference  in clock for
> DSI(which I mentioned in previous mail). DSI panel can't work with
> clock used by RGB panel-simple.


If you mean pixel clock from timings in next patch it seems incorrect.
Pixel clock should be always

htotal * vtotal * vrefresh, in case of drm_display_mode result should be
divided by 1000 (as .clock is in kHz).

With timings provided there you have: 928*525*60 = 29232000

So pixel clock should be 29232, if other timings are correct. DSI clock
is a different thing and it is private thing of DSI bridge/panel it
should not be exposed via drm_display_mode.


Regards

Andrzej


>
>> Then you can:
>>
>> 1. Try to find other users of this ICN6211 chip and compare
>> initialization sequences to guess purpose of registers.
>>
>> 2. Try to get specs of the chip (ask vendor, distributor, grep Internet).
> As we mentioned (even Chen-Yu), we are unable to find the proper spec
> for this panel, all we taken reference from AW BSP code.
>
>> 3. Do nothing - if there will be other users of the bridge they will do
>> this work.
> Don't know how we can go with generic bridge driver irrespective of
> these particular wrinkles, let me know if you have any suggestions.
>
>