Re: [PATCHv3 24/30] drm/omap: display: Add displays in sorted order to the panel_list

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2462 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel