[PATCH v2 20/22] drm: mxsfb: Merge mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt()

[Emil Velikov]

i Laurent,

On Sun, 7 Jun 2020 at 00:02, Laurent Pinchart
<laurent.pinchart at 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 at 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