The goal here is to set the element bus_format in the struct panel_desc. This is an enum with the possible values defined in include/uapi/linux/media-bus-format.h.
The enum values are not constructed in a way that you could calculate the value from color channel width/shift/mapping/whatever. You rather would have to check if the combination of color channel width/shift/mapping/whatever maps to an existing value and otherwise EINVAL out.
I don't see the value in having yet another way of how this information can be specified and then having to write a more complicated parser which maps the dt data to bus_format.
On Wed, Feb 23, 2022 at 2:45 PM Marek Vasut marex@denx.de 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-electronic...
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).
I agree this doesn't look right however, some more generic color channel width/shift/mapping might be better.