Hi Niklas,
Thank you for the patch.
On Fri, Jun 06, 2025 at 08:25:57PM +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
s/have/has/
> 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>
> Tested-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> * Changes since v4
> - Use the group->info structure added in previous patch instead of
> iterating over all VIN to find one that is instantiated to get the
> same information.
> - Condense variable declaration in rvin_isp_setup_links().
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 37 ++++++++++---------
> .../platform/renesas/rcar-vin/rcar-vin.h | 2 +-
> 2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index 66efe075adae..73d713868391 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;
> @@ -247,7 +247,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,
> @@ -910,35 +910,35 @@ 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;
> unsigned int id;
> int ret = -EINVAL;
>
> /* 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 = group->info->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;
> }
> @@ -992,30 +992,31 @@ 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 = group->vin[i];
>
> - if (!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. */
> @@ -1031,7 +1032,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 313703cd1103..cb8e8fa54f96 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -259,7 +259,7 @@ struct rvin_group {
> const struct rvin_info *info;
> 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
