| Message ID | 1507721021-28174-2-git-send-email-LW@KARO-electronics.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@KARO-electronics.de> > --- > 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. Thierry
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@KARO-electronics.de> > > --- > > 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. Lothar Waßmann
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@KARO-electronics.de> > > > --- > > > 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
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, \ + } + static const struct drm_display_mode ampire_am800480r3tmqwa1h_mode = { .clock = 33333, .hdisplay = 800,
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@KARO-electronics.de> --- drivers/gpu/drm/panel/panel-simple.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)