Re: [PATCH 2/9] drm/panel: simple: simplify display_mode definitions by using macro

[Thierry Reding]

On Tue, Oct 17, 2017 at 03:05:16PM +0200, Lothar WaÃmann wrote:
> Hi,
> 
> On Tue, 17 Oct 2017 14:09:37 +0200 Thierry Reding wrote:
> > On Wed, Oct 11, 2017 at 01:23:34PM +0200, Lothar WaÃmann wrote:
> > > Use the newly defined macro to generate the display_mode data entries
> > > for all panels. This reduces the code size significantly and makes the
> > > code more readable.
> > > 
> > > Signed-off-by: Lothar WaÃmann <LW@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/panel/panel-simple.c | 799 ++++++-----------------------------
> > >  1 file changed, 134 insertions(+), 665 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index dec639d..fde9c41 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -89,6 +89,20 @@ struct panel_simple {
> > >  	struct gpio_desc *enable_gpio;
> > >  };
> > >  
> > > +#define SP_DISPLAY_MODE(freq, ha, hfp, hs, hbp, va, vfp, vs, vbp, vr, flgs) { \
> > > +	.clock = freq,							\
> > > +	.hdisplay = ha,							\
> > > +	.hsync_start = (ha) + (hfp),					\
> > > +	.hsync_end = (ha) + (hfp) + (hs),				\
> > > +	.htotal = (ha) + (hfp) + (hs) + (hbp),				\
> > > +	.vdisplay = (va),						\
> > > +	.vsync_start = (va) + (vfp),					\
> > > +	.vsync_end = (va) + (vfp) + (vs),				\
> > > +	.vtotal = (va) + (vfp) + (vs) + (vbp),				\
> > > +	.vrefresh = vr,							\
> > > +	.flags = flgs,							\
> > > +}
> > > +
> > >  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> > >  {
> > >  	return container_of(panel, struct panel_simple, base);
> > [...]
> > > @@ -411,33 +415,9 @@ static const struct panel_desc ampire_am_480272h3tmqw_t01h = {
> > >  	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> > >  };
> > >  
> > > -#define SP_DISPLAY_MODE(freq, ha, hfp, hs, hbp, va, vfp, vs, vbp, vr, flgs) { \
> > > -	.clock = freq,							\
> > > -	.hdisplay = ha,							\
> > > -	.hsync_start = (ha) + (hfp),					\
> > > -	.hsync_end = (ha) + (hfp) + (hs),				\
> > > -	.htotal = (ha) + (hfp) + (hs) + (hbp),				\
> > > -	.vdisplay = (va),						\
> > > -	.vsync_start = (va) + (vfp),					\
> > > -	.vsync_end = (va) + (vfp) + (vs),				\
> > > -	.vtotal = (va) + (vfp) + (vs) + (vbp),				\
> > > -	.vrefresh = vr,							\
> > > -	.flags = flgs,							\
> > > -	}
> > 
> > Your first patch should put this in the right place to begin with so
> > that this patch is really just the conversion.
> > 
> > Again, I don't think this macro actually improves the way modes are
> > defined.
> > 
> I'm not happy with this panel driver stuff anyway. With the legacy
> 'display-timings' node that provided the timing data directly in the
> DTB, every bootloader could pick up the timing data and feed it to
> whatever driver it used for the display.
> With the panel driver stuff the whole Linux driver has to be replicated
> in the boot loader in order to be able to use the same DTB as Linux for
> its HW configuration.
> And adding a new panel involves recompiling the kernel and the boot
> loader, rather than adding the timing data from the panel's datasheet
> into the DTB.

This isn't the first time I've heard this. Please read this for more
background information on why the situation is what it is:

	http://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html

Thierry

Attachment: signature.asc
Description: PGP signature