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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Sorry, which disclaimer is that ?

> 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 :-)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux