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

[Marek Vasut <marex@denx.de>]

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@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 ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel