i Laurent,
On Sun, 7 Jun 2020 at 00:02, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Emil,
On Fri, Jun 05, 2020 at 03:58:53PM +0100, Emil Velikov wrote:
Hi Laurent,
With the previous disclaimer in mind, the series is: Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
Sorry, which disclaimer is that ?
"I don't know much about the hardware" from earlier. Even then, the refactor that is 1-21 is spot on.
There's a small suggestion inline, for future work.
On Sat, 30 May 2020 at 04:11, Laurent Pinchart wrote:
switch (bus_format) { case MEDIA_BUS_FMT_RGB565_1X16:
reg |= CTRL_BUS_WIDTH_16;
ctrl |= CTRL_BUS_WIDTH_16; break; case MEDIA_BUS_FMT_RGB666_1X18:
reg |= CTRL_BUS_WIDTH_18;
ctrl |= CTRL_BUS_WIDTH_18; break; case MEDIA_BUS_FMT_RGB888_1X24:
reg |= CTRL_BUS_WIDTH_24;
ctrl |= CTRL_BUS_WIDTH_24; break; default: dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); break;Off the top of my head, the default case should be handled in the atomic_check() hook. That is, unless I'm missing something and one can change the bus format in between atomic_check() and atomic_enable(). Which would sound like a very odd thing to do.
I agree, this should go to the atomic check. There are still quite a few things to improve in this driver, one step at a time :-)
Agreed - simply mentioning for completeness-sake. Doing that at misc later point is perfectly fine.
HTH Emil