Re: [PATCH 1/9] drm/panel: simple: add a macro for defining display modes in a simpler and less error prone way

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2913 bytes --]

On Tue, Oct 17, 2017 at 02:13:37PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 17 Oct 2017 14:08:18 +0200 Thierry Reding wrote:
> > On Wed, Oct 11, 2017 at 01:23:33PM +0200, Lothar Waßmann wrote:
> > > Create a macro that eases the definition of display mode parameters by
> > > accecpting the parameters:
> > > freq, hactive, hfront-porch, hsynclen, hback-porch,
> > > vactive, vfront-porch, vsynclen, vback-porch, vrefresh
> > > that can be usually directly taken from an LCD datasheet.
> > > 
> > > Put the calculations that are now open coded repeating the same
> > > parameters multiple times into the macro expansion.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > ---
> > >  drivers/gpu/drm/panel/panel-simple.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 474fa75..dec639d 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -411,6 +411,20 @@ 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,							\
> > > +	}
> > 
> > I don't think this simplifies anything. It's now completely non-obvious
> > which parameter is which, so you actually have to go look at the macro
> > definition when you add a new mode to make sure you get them right.
> > 
> In the original code you have to repeat the same parameters (e.g.
> vertical front porch (vfp)) in multiple places which is error prone.

I don't think this is an issue. The definitions initialize the fields in
the natural order, so the repetitions are where very localized and quite
difficult to actually get wrong. I've never seen anyone get it wrong for
any panel.

Also note that the above notation is only out of convenience to make it
clearer what the active, front-porch, sync and back-porch values are.
You can easily just collapse them into the sum if you're worried about
the repetition.

Alternatively, are you aware of struct display_timing? It's a different
way to describe the timings which doesn't require repeating the values.
simple-panel already supports those as an alternative to the struct
drm_display_mode.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]