Re: [PATCH v1 1/1] drm/bridge: drop drmP.h usage

[Sam Ravnborg <sam@ravnborg.org>]

Hi Laurent

Thanks for the quick feedback.

> > @@ -15,14 +15,17 @@
> >  #include <linux/of_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
> > -#include <drm/drmP.h>
> > +
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> > +#include <drm/drm_print.h>
> >  #include <drm/drm_probe_helper.h>
> > +
> >  #include <drm/bridge/dw_mipi_dsi.h>
> 
> If you separate this file fromt he rest of the drm/ includes, you should
> do the same in drivers/gpu/drm/bridge/synopsys/dw-hdmi.c for
> consistency. I'm fine either way.
> 
> > index a20e454ddd64..170f162ffa55 100644
> > --- a/drivers/gpu/drm/bridge/tc358764.c
> > +++ b/drivers/gpu/drm/bridge/tc358764.c
> > @@ -7,18 +7,22 @@
> >   *	Maciej Purski <m.purski@samsung.com>
> >   */
> >  
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_fb_helper.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> >  #include <drm/drm_probe_helper.h>
> > -#include <drm/drmP.h>
> > -#include <linux/gpio/consumer.h>
> > -#include <linux/of_graph.h>
> > -#include <linux/regulator/consumer.h>
> > -#include <video/mipi_display.h>
> 
> Similarly, in drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c you have
> video/ after drm/, while it's the other way around. I think we should
> pick one order and stick with it.

Well done to spot this inconsistency.

I will go through the files and consistently use following order:

<linux/*>

<asm/*>

<video/*>

<drm/*>

""

This seems to match the majority but all variants can be found.
And then the principle is:
- most general includes first, with linux/* first
- drm includes second-last
- local drm includes last

> 
> With these small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks. I will post a v2 - where I can also fix so the mail is sent to
the relevant people (my cc: was missing some this time).

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