Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:33PM +0200, Niklas Söderlund wrote:
> The link setup callback once acted on each VIN instance, and expected to
> be called once for each VIN instance. This have changed as the driver
> grew support for later hardware generations and the callback is now
> expected to setup links for all VIN in the group.
>
> The argument to the callback have however remained a pointer to a single
> VIN instance. This pointer was then used to get the group structure. Fix
> this and pass the group as the single argument to the link setup
> callback making the expectation of the function clear.
>
> There is no intentional change in behavior.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 52 ++++++++++++-------
> .../platform/renesas/rcar-vin/rcar-vin.h | 2 +-
> 2 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index b9ea5b8db559..1be408d6c508 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -65,7 +65,7 @@ static void rvin_group_cleanup(struct rvin_group *group)
> }
>
> static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
> - int (*link_setup)(struct rvin_dev *),
> + int (*link_setup)(struct rvin_group *),
> const struct media_device_ops *ops)
> {
> struct media_device *mdev = &group->mdev;
> @@ -115,7 +115,7 @@ static void rvin_group_release(struct kref *kref)
> }
>
> static int rvin_group_get(struct rvin_dev *vin,
> - int (*link_setup)(struct rvin_dev *),
> + int (*link_setup)(struct rvin_group *),
> const struct media_device_ops *ops)
> {
> struct rvin_group *group;
> @@ -246,7 +246,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> }
> }
>
> - return vin->group->link_setup(vin);
> + return vin->group->link_setup(vin->group);
> }
>
> static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -909,35 +909,46 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
> return 0;
> }
>
> -static int rvin_csi2_setup_links(struct rvin_dev *vin)
> +static int rvin_csi2_setup_links(struct rvin_group *group)
> {
> - const struct rvin_group_route *route;
> + const struct rvin_group_route *routes, *route;
const struct rvin_group_route *routes = NULL;
const struct rvin_group_route *route;
> unsigned int id;
> int ret = -EINVAL;
>
> + /* Find any VIN in group to get route info. */
> + routes = NULL;
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
Use ARRAY_SIZE()
> + if (group->vin[i]) {
> + routes = group->vin[i]->info->routes;
> + break;
> + }
> + }
> + if (!routes)
> + return -ENODEV;
I wonder if the info pointer should also be stored in the rvin_group
structure.
> +
> /* Create all media device links between VINs and CSI-2's. */
> - mutex_lock(&vin->group->lock);
> - for (route = vin->info->routes; route->chsel; route++) {
> + mutex_lock(&group->lock);
> + for (route = routes; route->chsel; route++) {
> /* Check that VIN' master is part of the group. */
> - if (!vin->group->vin[route->master])
> + if (!group->vin[route->master])
> continue;
>
> /* Check that CSI-2 is part of the group. */
> - if (!vin->group->remotes[route->csi].subdev)
> + if (!group->remotes[route->csi].subdev)
> continue;
>
> for (id = route->master; id < route->master + 4; id++) {
> /* Check that VIN is part of the group. */
> - if (!vin->group->vin[id])
> + if (!group->vin[id])
> continue;
>
> - ret = rvin_csi2_create_link(vin->group, id, route);
> + ret = rvin_csi2_create_link(group, id, route);
> if (ret)
> goto out;
> }
> }
> out:
> - mutex_unlock(&vin->group->lock);
> + mutex_unlock(&group->lock);
>
> return ret;
> }
> @@ -991,30 +1002,33 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> * ISP
> */
>
> -static int rvin_isp_setup_links(struct rvin_dev *vin)
> +static int rvin_isp_setup_links(struct rvin_group *group)
> {
> unsigned int i;
> int ret = -EINVAL;
>
> /* Create all media device links between VINs and ISP's. */
> - mutex_lock(&vin->group->lock);
> + mutex_lock(&group->lock);
> for (i = 0; i < RCAR_VIN_NUM; i++) {
> struct media_pad *source_pad, *sink_pad;
> struct media_entity *source, *sink;
> unsigned int source_slot = i / 8;
> unsigned int source_idx = i % 8 + 1;
> + struct rvin_dev *vin;
struct rvin_dev *vin = group->vin[i];
>
> - if (!vin->group->vin[i])
> + vin = group->vin[i];
> +
> + if (!vin)
> continue;
>
> /* Check that ISP is part of the group. */
> - if (!vin->group->remotes[source_slot].subdev)
> + if (!group->remotes[source_slot].subdev)
> continue;
>
> - source = &vin->group->remotes[source_slot].subdev->entity;
> + source = &group->remotes[source_slot].subdev->entity;
> source_pad = &source->pads[source_idx];
>
> - sink = &vin->group->vin[i]->vdev.entity;
> + sink = &vin->vdev.entity;
> sink_pad = &sink->pads[0];
>
> /* Skip if link already exists. */
> @@ -1030,7 +1044,7 @@ static int rvin_isp_setup_links(struct rvin_dev *vin)
> break;
> }
> }
> - mutex_unlock(&vin->group->lock);
> + mutex_unlock(&group->lock);
>
> return ret;
> }
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index 83d1b2734c41..7d4fce248976 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -257,7 +257,7 @@ struct rvin_group {
> struct v4l2_async_notifier notifier;
> struct rvin_dev *vin[RCAR_VIN_NUM];
>
> - int (*link_setup)(struct rvin_dev *vin);
> + int (*link_setup)(struct rvin_group *group);
>
> struct {
> struct v4l2_async_connection *asc;
--
Regards,
Laurent Pinchart
