| Message ID | 20190712163333.231884-1-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: simple: Doxygenize 'struct panel_desc'; rename a few functions | expand |
Hi Doug. On Fri, Jul 12, 2019 at 09:33:33AM -0700, Douglas Anderson wrote: > This attempts to address outstanding review feedback from commit > b8a2948fa2b3 ("drm/panel: simple: Add ability to override typical > timing"). Specifically: > > * It was requested that I document (in the structure definition) that > the device tree override had no effect if 'struct drm_display_mode' > was used in the panel description. I have provided full Doxygen > comments for 'struct panel_desc' to accomplish that. > * panel_simple_get_fixed_modes() was thought to be a confusing name, > so it has been renamed to panel_simple_get_display_modes(). > * panel_simple_parse_override_mode() was thought to be better named as > panel_simple_parse_panel_timing_node(). > > Suggested-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> Thanks. I updated the $subject to: drm/panel: simple: document panel_desc; rename a few functions And pushed out to drm-misc-next. > - Sam said that there was still something that he didn't understand > with regards to the flags. Sam: if this is something that needs to > be addressed, please yell. Need to re-visit this later when I have familiarized myself with the new yaml syntax and what impact any potential changes may have on the panel drivers. So for now we leave it as is. Sam
Hi Doug. On Wed, Jul 17, 2019 at 07:33:17PM +0200, Sam Ravnborg wrote: > Hi Doug. > > On Fri, Jul 12, 2019 at 09:33:33AM -0700, Douglas Anderson wrote: > > This attempts to address outstanding review feedback from commit > > b8a2948fa2b3 ("drm/panel: simple: Add ability to override typical > > timing"). Specifically: > > > > * It was requested that I document (in the structure definition) that > > the device tree override had no effect if 'struct drm_display_mode' > > was used in the panel description. I have provided full Doxygen > > comments for 'struct panel_desc' to accomplish that. > > * panel_simple_get_fixed_modes() was thought to be a confusing name, > > so it has been renamed to panel_simple_get_display_modes(). > > * panel_simple_parse_override_mode() was thought to be better named as > > panel_simple_parse_panel_timing_node(). > > > > Suggested-by: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Thanks. > > I updated the $subject to: > drm/panel: simple: document panel_desc; rename a few functions > > And pushed out to drm-misc-next. I see the following error printed at each boot: /panel: could not find node 'panel-timing' The error originates from this snip (from panel-simple.c): if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) panel_simple_parse_panel_timing_node(dev, panel, &dt); of_get_display_timing() returns -2 (-ENOENT). In the setup I use there is no panel-timing node as the timing specified in panel-simple.c is fine. This is the typical setup - and there should not in the normal case be printed error messages like this during boot. Will you please take a look at this. Sam
Hi, On Sun, Jul 21, 2019 at 2:38 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Doug. > > On Wed, Jul 17, 2019 at 07:33:17PM +0200, Sam Ravnborg wrote: > > Hi Doug. > > > > On Fri, Jul 12, 2019 at 09:33:33AM -0700, Douglas Anderson wrote: > > > This attempts to address outstanding review feedback from commit > > > b8a2948fa2b3 ("drm/panel: simple: Add ability to override typical > > > timing"). Specifically: > > > > > > * It was requested that I document (in the structure definition) that > > > the device tree override had no effect if 'struct drm_display_mode' > > > was used in the panel description. I have provided full Doxygen > > > comments for 'struct panel_desc' to accomplish that. > > > * panel_simple_get_fixed_modes() was thought to be a confusing name, > > > so it has been renamed to panel_simple_get_display_modes(). > > > * panel_simple_parse_override_mode() was thought to be better named as > > > panel_simple_parse_panel_timing_node(). > > > > > > Suggested-by: Sam Ravnborg <sam@ravnborg.org> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > Thanks. > > > > I updated the $subject to: > > drm/panel: simple: document panel_desc; rename a few functions > > > > And pushed out to drm-misc-next. > > I see the following error printed at each boot: > > /panel: could not find node 'panel-timing' > > The error originates from this snip (from panel-simple.c): > > if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) > panel_simple_parse_panel_timing_node(dev, panel, &dt); > > of_get_display_timing() returns -2 (-ENOENT). > > In the setup I use there is no panel-timing node as the timing specified > in panel-simple.c is fine. > This is the typical setup - and there should not in the normal case > be printed error messages like this during boot. > > Will you please take a look at this. Breadcrumbs: series has been posted to address this. PTAL. https://lkml.kernel.org/r/20190722182439.44844-1-dianders@chromium.org -Doug
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a50871c6836b..f3c0e203f40f 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -38,6 +38,22 @@ #include <drm/drm_mipi_dsi.h> #include <drm/drm_panel.h> +/** + * @modes: Pointer to array of fixed modes appropriate for this panel. If + * only one mode then this can just be the address of this the mode. + * NOTE: cannot be used with "timings" and also if this is specified + * then you cannot override the mode in the device tree. + * @num_modes: Number of elements in modes array. + * @timings: Pointer to array of display timings. NOTE: cannot be used with + * "modes" and also these will be used to validate a device tree + * override if one is present. + * @num_timings: Number of elements in timings array. + * @bpc: Bits per color. + * @size: Structure containing the physical size of this panel. + * @delay: Structure containing various delay values for this panel. + * @bus_format: See MEDIA_BUS_FMT_... defines. + * @bus_flags: See DRM_BUS_FLAG_... defines. + */ struct panel_desc { const struct drm_display_mode *modes; unsigned int num_modes; @@ -135,7 +151,7 @@ static unsigned int panel_simple_get_timings_modes(struct panel_simple *panel) return num; } -static unsigned int panel_simple_get_fixed_modes(struct panel_simple *panel) +static unsigned int panel_simple_get_display_modes(struct panel_simple *panel) { struct drm_connector *connector = panel->base.connector; struct drm_device *drm = panel->base.drm; @@ -199,7 +215,7 @@ static int panel_simple_get_non_edid_modes(struct panel_simple *panel) */ WARN_ON(panel->desc->num_timings && panel->desc->num_modes); if (num == 0) - num = panel_simple_get_fixed_modes(panel); + num = panel_simple_get_display_modes(panel); connector->display_info.bpc = panel->desc->bpc; connector->display_info.width_mm = panel->desc->size.width; @@ -351,9 +367,9 @@ static const struct drm_panel_funcs panel_simple_funcs = { #define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \ (to_check->field.typ >= bounds->field.min && \ to_check->field.typ <= bounds->field.max) -static void panel_simple_parse_override_mode(struct device *dev, - struct panel_simple *panel, - const struct display_timing *ot) +static void panel_simple_parse_panel_timing_node(struct device *dev, + struct panel_simple *panel, + const struct display_timing *ot) { const struct panel_desc *desc = panel->desc; struct videomode vm; @@ -446,7 +462,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) } if (!of_get_display_timing(dev->of_node, "panel-timing", &dt)) - panel_simple_parse_override_mode(dev, panel, &dt); + panel_simple_parse_panel_timing_node(dev, panel, &dt); drm_panel_init(&panel->base); panel->base.dev = dev;
This attempts to address outstanding review feedback from commit b8a2948fa2b3 ("drm/panel: simple: Add ability to override typical timing"). Specifically: * It was requested that I document (in the structure definition) that the device tree override had no effect if 'struct drm_display_mode' was used in the panel description. I have provided full Doxygen comments for 'struct panel_desc' to accomplish that. * panel_simple_get_fixed_modes() was thought to be a confusing name, so it has been renamed to panel_simple_get_display_modes(). * panel_simple_parse_override_mode() was thought to be better named as panel_simple_parse_panel_timing_node(). Suggested-by: Sam Ravnborg <sam@ravnborg.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- NOTES: - I have not addressed the suggestion of doing 'bool has_override = panel->override_mode.type != 0;'. As per my reply on the mailing list I think the convention in this file is to rely on implicit conversions from int to bool. - Sam said that there was still something that he didn't understand with regards to the flags. Sam: if this is something that needs to be addressed, please yell. drivers/gpu/drm/panel/panel-simple.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)