[RFC][PATCH] Revert "drm/panel-simple: drop use of data-mapping property"

[Marek Vasut]

On 2/8/22 22:27, Christoph Niedermaier wrote:
> From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com]
> Sent: Thursday, February 3, 2022 12:46 AM
>>
>> Hi Christoph,
>>
> 
> Hi Laurent,
> 
>> On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
>>> Without the data-mapping devicetree property my display won't
>>> work properly. It is flickering, because the bus flags won't
>>> be assigned without a defined bus format by the imx parallel
>>> display driver. There was a discussion about the removal [1]
>>> and an agreement that a better solution is needed, but it is
>>> missing so far. So what would be the better approach?
>>>
>>> [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
>>>
>>> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
>>>
>>> Signed-off-by: Christoph Niedermaier <cniedermaier at dh-electronics.com>
>>> Cc: Marek Vasut <marex at denx.de>
>>> Cc: Sam Ravnborg <sam at ravnborg.org>
>>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> Cc: Maxime Ripard <mripard at kernel.org>
>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>>> Cc: David Airlie <airlied at linux.ie>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>> Cc: Sascha Hauer <s.hauer at pengutronix.de>
>>> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
>>> Cc: Fabio Estevam <festevam at gmail.com>
>>> Cc: NXP Linux Team <linux-imx at nxp.com>
>>> Cc: linux-arm-kernel at lists.infradead.org
>>> To: dri-devel at lists.freedesktop.org
>>> ---
>>>   drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>> index 3c08f9827acf..2c683d94a3f3 100644
>>> --- a/drivers/gpu/drm/panel/panel-simple.c
>>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
>>>        struct panel_desc *desc;
>>>        unsigned int bus_flags;
>>>        struct videomode vm;
>>> +     const char *mapping;
>>>        int ret;
>>>
>>>        np = dev->of_node;
>>> @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
>>>        of_property_read_u32(np, "width-mm", &desc->size.width);
>>>        of_property_read_u32(np, "height-mm", &desc->size.height);
>>>
>>> +     of_property_read_string(np, "data-mapping", &mapping);
>>> +     if (!strcmp(mapping, "rgb24"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>> +     else if (!strcmp(mapping, "rgb565"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>> +     else if (!strcmp(mapping, "bgr666"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>> +     else if (!strcmp(mapping, "lvds666"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>
>> You're right that there's an issue, but a revert isn't the right option.
>> The commit you're reverting never made it in a stable release, because
>> it was deemed to not be a good enough option.
>>
>> First of all, any attempt to fix this should include an update to the DT
>> binding. Second, as this is about DPI panels, the LVDS option should be
>> dropped. Finally, I've shared some initial thoughts in [1], maybe you
>> can reply to that e-mail to continue the discussion there ?
> 
> According to your thoughts in [1] you mean that the bus format should be
> build out of the devicetree properties bus-width and data-shift. It would
> be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
> but what about a bus like RGB565_1X16, where each color has different
> bus width. Also the order of the colors should be defined to differ
> between busses like RGB888_1X24 and GBR888_1X24.
> Are there any ideas how can this be covered?

Maybe with props like these ?

channel-width -- width of each color channel
channel-shift -- shift of each color channel
channel-map -- mapping of each color channel

So for RGB888
channel-width = <8 8 8>;
channel-shift = <0 0 0>;
channel-map = "RGB"; // or something ?

For BGR565 panel connected to RGB24 scanout
channel-width = <5 6 5>;
channel-shift = <3 2 3>;
channel-map = "BGR"; // or something ?

For BGR565 panel connected to RGB565 scanout
channel-width = <5 6 5>;
channel-shift = <0 0 0>;
channel-map = "BGR"; // or something ?