| Message ID | 1490706496-4959-25-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Thank you for the patch. On Tuesday 28 Mar 2017 16:08:10 Tomi Valkeinen wrote: > From: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Keep the panel_list ordered according to aliases. The DRM connectors will > be created following the panel_list. By keeping the list ordered the DRM > connectors will be created in the same order regardless of the driver > probe order. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/dss/display.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/display.c > b/drivers/gpu/drm/omapdrm/dss/display.c index 94c012e0584b..26cb59be045e > 100644 > --- a/drivers/gpu/drm/omapdrm/dss/display.c > +++ b/drivers/gpu/drm/omapdrm/dss/display.c > @@ -83,6 +83,7 @@ static int disp_num_counter; > int omapdss_register_display(struct omap_dss_device *dssdev) > { > struct omap_dss_driver *drv = dssdev->driver; > + struct list_head *cur; > int id; > > /* > @@ -118,7 +119,14 @@ int omapdss_register_display(struct omap_dss_device > *dssdev) drv->get_timings = omapdss_default_get_timings; > > mutex_lock(&panel_list_mutex); > - list_add_tail(&dssdev->panel_list, &panel_list); > + list_for_each(cur, &panel_list) { How about using list_for_each_entry ? > + struct omap_dss_device *ldev = list_entry(cur, > + struct omap_dss_device, > + panel_list); > + if (strcmp(ldev->alias, dssdev->alias) > 0) > + break; > + } > + list_add_tail(&dssdev->panel_list, cur); What you want to do here is add the new element before cur. list_add_tail() does that, but that's not very explicit (at least to me) from the function name. I would have used list_add(new, cur->prev), but that's up to you. > mutex_unlock(&panel_list_mutex); > return 0; > }
On 29/03/17 13:08, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tuesday 28 Mar 2017 16:08:10 Tomi Valkeinen wrote: >> From: Peter Ujfalusi <peter.ujfalusi@ti.com> >> >> Keep the panel_list ordered according to aliases. The DRM connectors will >> be created following the panel_list. By keeping the list ordered the DRM >> connectors will be created in the same order regardless of the driver >> probe order. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/dss/display.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/dss/display.c >> b/drivers/gpu/drm/omapdrm/dss/display.c index 94c012e0584b..26cb59be045e >> 100644 >> --- a/drivers/gpu/drm/omapdrm/dss/display.c >> +++ b/drivers/gpu/drm/omapdrm/dss/display.c >> @@ -83,6 +83,7 @@ static int disp_num_counter; >> int omapdss_register_display(struct omap_dss_device *dssdev) >> { >> struct omap_dss_driver *drv = dssdev->driver; >> + struct list_head *cur; >> int id; >> >> /* >> @@ -118,7 +119,14 @@ int omapdss_register_display(struct omap_dss_device >> *dssdev) drv->get_timings = omapdss_default_get_timings; >> >> mutex_lock(&panel_list_mutex); >> - list_add_tail(&dssdev->panel_list, &panel_list); >> + list_for_each(cur, &panel_list) { > > How about using list_for_each_entry ? We need the list iterator after the loop to add the new node. Where would list_for_each_entry()'s 'pos' point to, if there's nothing in the list yet? I think list_for_each_entry()'s iterator is supposed to be used only inside the loop. >> + struct omap_dss_device *ldev = list_entry(cur, >> + struct > omap_dss_device, >> + panel_list); >> + if (strcmp(ldev->alias, dssdev->alias) > 0) >> + break; >> + } >> + list_add_tail(&dssdev->panel_list, cur); > > What you want to do here is add the new element before cur. list_add_tail() > does that, but that's not very explicit (at least to me) from the function > name. I would have used list_add(new, cur->prev), but that's up to you. list_add_tail's documentation says "Insert a new entry before the specified head". So while I agree that "tail" is not what comes to my mind from the description, it sounds like this is exactly the case where list_add_tail is to be used... Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/display.c b/drivers/gpu/drm/omapdrm/dss/display.c index 94c012e0584b..26cb59be045e 100644 --- a/drivers/gpu/drm/omapdrm/dss/display.c +++ b/drivers/gpu/drm/omapdrm/dss/display.c @@ -83,6 +83,7 @@ static int disp_num_counter; int omapdss_register_display(struct omap_dss_device *dssdev) { struct omap_dss_driver *drv = dssdev->driver; + struct list_head *cur; int id; /* @@ -118,7 +119,14 @@ int omapdss_register_display(struct omap_dss_device *dssdev) drv->get_timings = omapdss_default_get_timings; mutex_lock(&panel_list_mutex); - list_add_tail(&dssdev->panel_list, &panel_list); + list_for_each(cur, &panel_list) { + struct omap_dss_device *ldev = list_entry(cur, + struct omap_dss_device, + panel_list); + if (strcmp(ldev->alias, dssdev->alias) > 0) + break; + } + list_add_tail(&dssdev->panel_list, cur); mutex_unlock(&panel_list_mutex); return 0; }