[RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 17:39, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote:
>> On 2/23/22 15:37, Maxime Ripard wrote:
>>> On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
>>>> On 2/23/22 14:47, Maxime Ripard wrote:
>>>>> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>>>>>> On 2/23/22 14:41, Maxime Ripard wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>>>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>>>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>>>>>
>>>>>>>> Signed-off-by: Max Krummenacher <max.krummenacher at toradex.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>>>>>
>>>>>>>> Max
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>>>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>>>>>      	int ret;
>>>>>>>>      	np = dev->of_node;
>>>>>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>>>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>>>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>>>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>>>>> +	} else {
>>>>>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>>>>>> +			np);
>>>>>>>> +		return -EINVAL;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>>>>>> from the compatible? I'd expect two panels that don't have the same bus
>>>>>>> format to not be claimed as compatible.
>>>>>>
>>>>>> Which compatible ?
>>>>>>
>>>>>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>>>>>> specified in DT (and needs bus format specified there too).
>>>>>
>>>>> panel-dpi is supposed to have two compatibles, the panel-specific one
>>>>> and panel-dpi. This would obviously be tied to the panel-specific one.
>>>>
>>>> This whole discussion is about the one which only has 'panel-dpi' compatible
>>>> and describes the panel in DT completely. The specific compatible is not
>>>> present in DT when this patch is needed.
>>>
>>>   From the panel-dpi DT binding:
>>>
>>> properties:
>>>     compatible:
>>>       description:
>>>         Shall contain a panel specific compatible and "panel-dpi"
>>>         in that order.
>>>       items:
>>>         - {}
>>>         - const: panel-dpi
>>>
>>> The panel-specific compatible is mandatory, whether you like it or not.
>>
>> It doesn't seem to me that's the intended use per panel-simple.c , so maybe
>> the bindings need to be fixed too ?
> 
> It's not clear to me why this has anything to do with panel-simple, but
> the binding is correct, and that requirement is fairly standard. We have
> the same thing with panel-lvds for example.

I think this patch is related to this patch, which was not mentioned in 
the commit message:

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

(unless I am confused)

With LVDS the situation is simpler, you have three formats and that's it 
(18bpp and 2 24bpp), with DPI it is more complex, since you need to deal 
with color channel width (888, 666 and even 565 and other oddities), 
then with mapping (RGB, BGR, etc), and then you can have panels with 
only 18bpp inputs wired to 24bpp DPI bus and vice versa which you also 
have to somehow describe.