Hi Laurent, Thanks for your feedback.
On 2018-03-02 14:00:19 +0200, Laurent Pinchart wrote: [snip] > > + > > + /* If any entity is in use don't allow link changes. */ > > + media_device_for_each_entity(entity, &group->mdev) > > + if (entity->use_count) > > + return -EBUSY; > > This means that you disallow link setup when any video node is open. > According > to the comment above, isn't it stream_count that you want to check ? If so > the > MC core does it for you (unless you create links with the > MEDIA_LNK_FL_DYNAMIC > flag set), see __media_entity_setup_link(). You are correct that the comment and code don't align. I rather update the comment and keep denying link enablement if any video devices are open. I'm sure this is not a real issue but this group concept feels a bit fragile, so better safe then sorry. Or do you feel there is a benefit for the user to be able to change the graph with video nodes open? We can always loosen the constraint later if it becomes a problem but introducing it if we would need it could be considered a regression? > > > + mutex_lock(&group->lock); > > + > > + /* > > + * Figure out which VIN the link concern's and lookup > > + * which master VIN controls the routes for that VIN. > > + */ > > Can't you simply use a container_of to cast from vdev to vin ? Thank you for this, made the code much more readable! > > > + vdev = media_entity_to_video_device(link->sink->entity); > > + for (i = 0; i < RCAR_VIN_NUM; i++) { > > + if (group->vin[i] && &group->vin[i]->vdev == vdev) { > > + vin = group->vin[i]; > > + master_id = rvin_group_id_to_master(vin->id); > > + break; > > + } > > + } [snip] -- Regards, Niklas Söderlund