| Message ID | 51B5A9E6.20903@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/10/2013 12:26 PM, Sylwester Nawrocki wrote: > Hi Sakari, > > On 06/10/2013 12:15 AM, Sakari Ailus wrote: >> Sylwester Nawrocki wrote: >> ... >>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c >>>>> index e1cd132..bd85dc3 100644 >>>>> --- a/drivers/media/media-entity.c >>>>> +++ b/drivers/media/media-entity.c >>>>> @@ -429,6 +429,57 @@ media_entity_create_link(struct media_entity >>>>> *source, u16 source_pad, >>>>> } >>>>> EXPORT_SYMBOL_GPL(media_entity_create_link); >>>>> >>>>> +void __media_entity_remove_links(struct media_entity *entity) >>>>> +{ >>>>> + int i, r; >>>> >>>> u16? r can be defined inside the loop. >>> >>> I would argue 'unsigned int' would be more optimal type for i in most >>> cases. Will move r inside the loop. >>> >>>>> + for (i = 0; i< entity->num_links; i++) { >>>>> + struct media_link *link =&entity->links[i]; >>>>> + struct media_entity *remote; >>>>> + int num_links; >>>> >>>> num_links is u16 in struct media_entity. I'd use the same type. >>> >>> I would go with 'unsigned int', as a more natural type for the CPU in >>> most cases. It's a minor issue, but I can't see how u16 would have been >>> more optimal than unsigned int for a local variable like this, while >>> this code is mostly used on 32-bit systems at least. >>> >>>>> + if (link->source->entity == entity) >>>>> + remote = link->sink->entity; >>>>> + else >>>>> + remote = link->source->entity; >>>>> + >>>>> + num_links = remote->num_links; >>>>> + >>>>> + for (r = 0; r< num_links; r++) { >>>> >>>> Is caching remote->num_links needed, or do I miss something? >>> >>> Yes, it is needed, remote->num_links is decremented inside the loop. >> >> Oh, forgot this one... yes, remote->num_links is decremented, but so is >> r it's compared to. So I don't think it's necessary to cache it, but >> it's neither an error to do so. > > Indeed, it seems more correct to not cache it, thanks. > >>>>> + struct media_link *rlink =&remote->links[r]; >>>>> + >>>>> + if (rlink != link->reverse) >>>>> + continue; >>>>> + >>>>> + if (link->source->entity == entity) >>>>> + remote->num_backlinks--; >>>>> + >>>>> + remote->num_links--; >>>>> + >>>>> + if (remote->num_links< 1) >>>> >>>> How about: if (!remote->num_links) ? >>> >>> Hmm, perhaps squashing the above two lines to: >>> >>> if (--remote->num_links == 0) >>> >>> ? >> >> I'm not too fond of --/++ operators as part of more complex structures >> so I'd keep it separate. Fewer lines doesn't always equate to more >> readable code. :-) > > In general I agree, however it's quite simple statement in this case - > only decrement and test, it's often only one instruction even in the > assembly language... > > I'm going to squash following to this patch: > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c > index f5ea82e..1fb619d 100644 > --- a/drivers/media/media-entity.c > +++ b/drivers/media/media-entity.c > @@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity) > for (i = 0; i < entity->num_links; i++) { > struct media_link *link = &entity->links[i]; > struct media_entity *remote; > - unsigned int r, num_links; > + unsigned int r; Should have been: unsigned int r = 0; Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index f5ea82e..1fb619d 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity) for (i = 0; i < entity->num_links; i++) { struct media_link *link = &entity->links[i]; struct media_entity *remote; - unsigned int r, num_links; + unsigned int r; if (link->source->entity == entity) remote = link->sink->entity; else remote = link->source->entity; - num_links = remote->num_links; - - for (r = 0; r < num_links; r++) { - if (remote->links[r] != link->reverse) + while (r < remote->num_links; r++) { + if (remote->links[r] != link->reverse) { + r++; continue; + } if (link->source->entity == entity) remote->num_backlinks--; @@ -456,7 +456,7 @@ void __media_entity_remove_links(struct media_entity *entity) break; /* Insert last entry in place of the dropped link. */ - remote->links[r--] = remote->links[remote->num_links]; + remote->links[r] = remote->links[remote->num_links]; } }