Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:34PM +0200, Niklas Söderlund wrote:
> Prepare to move Gen2 and earlier models to media controller by
> generating a unique VIN group id for each VIN instance. On Gen3 and Gen4
> it is important to have a specific id in the group as media graph routes
> depend on this. On Gen2 and earlier models all that will matter is to
> have a unique id in the range.
>
> Break out the id generation to a own function keeping the logic for Gen3
> and Gen4 while generating a sequential id for Gen2 models.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> * Changes since v1
> - Move ID allocation to probe.
> - Use ida_alloc_range() instead of implementing our own schema by
> counting DT nodes.
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 78 ++++++++++++++-----
> 1 file changed, 59 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 1be408d6c508..d9ad56fb2aa9 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -10,6 +10,7 @@
> * Based on the soc-camera rcar_vin driver
> */
>
> +#include <linux/idr.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_graph.h>
> @@ -55,6 +56,7 @@
> * be only one group for all instances.
> */
>
> +static DEFINE_IDA(rvin_ida);
> static DEFINE_MUTEX(rvin_group_lock);
> static struct rvin_group *rvin_group_data;
>
> @@ -119,23 +121,8 @@ static int rvin_group_get(struct rvin_dev *vin,
> const struct media_device_ops *ops)
> {
> struct rvin_group *group;
> - u32 id;
> int ret;
>
> - /* Make sure VIN id is present and sane */
> - ret = of_property_read_u32(vin->dev->of_node, "renesas,id", &id);
> - if (ret) {
> - vin_err(vin, "%pOF: No renesas,id property found\n",
> - vin->dev->of_node);
> - return -EINVAL;
> - }
> -
> - if (id >= RCAR_VIN_NUM) {
> - vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
> - vin->dev->of_node, id);
> - return -EINVAL;
> - }
> -
> /* Join or create a VIN group */
> mutex_lock(&rvin_group_lock);
> if (rvin_group_data) {
> @@ -164,16 +151,15 @@ static int rvin_group_get(struct rvin_dev *vin,
> /* Add VIN to group */
> mutex_lock(&group->lock);
>
> - if (group->vin[id]) {
> - vin_err(vin, "Duplicate renesas,id property value %u\n", id);
> + if (group->vin[vin->id]) {
> + vin_err(vin, "Duplicate renesas,id property value %u\n", vin->id);
> mutex_unlock(&group->lock);
> kref_put(&group->refcount, rvin_group_release);
> return -EINVAL;
> }
>
> - group->vin[id] = vin;
> + group->vin[vin->id] = vin;
>
> - vin->id = id;
> vin->group = group;
> vin->v4l2_dev.mdev = &group->mdev;
>
> @@ -1375,6 +1361,54 @@ static const struct of_device_id rvin_of_id_table[] = {
> };
> MODULE_DEVICE_TABLE(of, rvin_of_id_table);
>
> +static int rvin_id_get(struct rvin_dev *vin)
> +{
> + u32 oid;
> + int id;
> +
> + switch (vin->info->model) {
> + case RCAR_GEN3:
> + case RCAR_GEN4:
> + if (of_property_read_u32(vin->dev->of_node, "renesas,id", &oid))
I would keep the original error message here:
vin_err(vin, "%pOF: No renesas,id property found\n",
vin->dev->of_node);
> + break;
And you can return -EINVAL directly.
> +
> + if (oid < 0 || oid >= RCAR_VIN_NUM) {
> + vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
> + vin->dev->of_node, oid);
> + return -EINVAL;
> + }
> +
> + vin->id = oid;
> +
> + return 0;
> + default:
> + id = ida_alloc_range(&rvin_ida, 0, RCAR_VIN_NUM - 1,
> + GFP_KERNEL);
> + if (id < 0)
> + break;
Same here, I'd add a specific error message and return:
vin_err(vin, "Can't allocate group ID\n");
return -EINVAL;
> +
> + vin->id = id;
> +
> + return 0;
> + }
> +
> + vin_err(vin, "Can't figure out VIN id\n");
> +
> + return -EINVAL;
And you can drop this.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> +}
> +
> +static void rvin_id_put(struct rvin_dev *vin)
> +{
> + switch (vin->info->model) {
> + case RCAR_GEN3:
> + case RCAR_GEN4:
> + break;
> + default:
> + ida_free(&rvin_ida, vin->id);
> + break;
> + }
> +}
> +
> static int rcar_vin_probe(struct platform_device *pdev)
> {
> struct rvin_dev *vin;
> @@ -1402,6 +1436,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, vin);
>
> + if (rvin_id_get(vin))
> + return -EINVAL;
> +
> if (vin->info->use_isp) {
> ret = rvin_isp_init(vin);
> } else if (vin->info->use_mc) {
> @@ -1419,6 +1456,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
> if (ret) {
> rvin_dma_unregister(vin);
> + rvin_id_put(vin);
> return ret;
> }
>
> @@ -1443,6 +1481,8 @@ static void rcar_vin_remove(struct platform_device *pdev)
> else
> rvin_parallel_cleanup(vin);
>
> + rvin_id_put(vin);
> +
> rvin_dma_unregister(vin);
> }
>
--
Regards,
Laurent Pinchart
