Hi Laurent,
Thanks for your review. I know this patch must not have been fun to
review, much appreciated.
On 2025-06-13 02:53:19 +0300, Laurent Pinchart wrote:
[snip]
> > @@ -290,17 +283,15 @@ static int rvin_group_notify_bound(struct
> > v4l2_async_notifier *notifier,
> > guard(mutex)(&group->lock);
> >
> > for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> struct rvin_dev *vin = &group->vin[i];
>
> will simplify the code blow.
Create idea.
[snip]
> > @@ -971,7 +873,7 @@ static int __maybe_unused rvin_resume(struct
> > device *dev)
> > * as we don't know if and in which order the master VINs will
> > * be resumed.
> > */
> > - if (vin->info->use_mc) {
> > + if (vin->info->model == RCAR_GEN3) {
>
> No need to do this for Gen4 ?
No. This section restores the routing for a set of VIN instances, a
feature only available on Gen3. On Gen4 this functionality is moved to
the Channel Selector ISP. That it previously was done on Gen4 was not
intentional.
I think this is a good example on how organic grow of this driver have
created a lot of unneeded complexity that needs to be resolved, see next
comment.
[snip]
> > @@ -1319,21 +1209,27 @@ static int rcar_vin_probe(struct
> > platform_device *pdev)
> > if (ret < 0)
> > return ret;
> >
> > - if (vin->info->use_isp) {
> > - ret = rvin_isp_init(vin);
> > - } else if (vin->info->use_mc) {
> > - ret = rvin_csi2_init(vin);
> > + switch (vin->info->model) {
> > + case RCAR_GEN3:
> > + case RCAR_GEN4:
> > + if (vin->info->use_isp) {
> > + ret = rvin_isp_init(vin);
> > + } else {
> > + ret = rvin_csi2_init(vin);
> >
> > - if (vin->info->scaler &&
> > - rvin_group_id_to_master(vin->id) == vin->id)
> > - vin->scaler = vin->info->scaler;
> > - } else {
> > - ret = rvin_group_get(vin, NULL, NULL);
> > + if (vin->info->scaler &&
> > + rvin_group_id_to_master(vin->id) == vin->id)
> > + vin->scaler = vin->info->scaler;
> > + }
> > + break;
> > + default:
> > + ret = rvin_group_get(vin, rvin_parallel_setup_links, NULL);
> > if (!ret)
> > ret = rvin_group_notifier_init(vin, 0, 0);
> >
> > if (vin->info->scaler)
> > vin->scaler = vin->info->scaler;
>
> If I'm not mistaken there's one group per VIN on Gen2, right ? If so, I
> think you could move the
>
> if (vin->info->scaler &&
> rvin_group_id_to_master(vin->id) == vin->id)
> vin->scaler = vin->info->scaler;
>
> code after the switch.
Yes and no. The function rvin_group_id_to_master() is purely a Gen3
thing. To move the check outside the switch it would need to be made
Gen2 aware. It can be kept in the Gen3+Gen4 code path as we know we
don't have any scalers on Gen4 so vin->info->scaler will always be
false.
As the previous comment hints at, this is a bit of an organic growth
mess. In my ever growing todo file to clean up this driver I hope to
rework the info structure to remove the remove the
vin->info->{model,use_isp,nv12,raw10} flags and replace them with
function pointers that do the right thing for each platform without
these special cases sprinkled all over. Too much small issues have come
up over the years using the current design.
For this reason I would like to keep this split in each case of the
switch over extending the special case handling more in the helper
functions.
[snip]
> > @@ -1011,11 +592,7 @@ static int rvin_open(struct file *file)
> > if (ret)
> > goto err_unlock;
> >
> > - if (vin->info->use_mc)
> > - ret = v4l2_pipeline_pm_get(&vin->vdev.entity);
> > - else if (v4l2_fh_is_singular_file(file))
> > - ret = rvin_power_parallel(vin, true);
> > -
> > + ret = v4l2_pipeline_pm_get(&vin->vdev.entity);
>
> Another item for your todo list, this is deprecated.
Ack, added to list. I hope I can get that list down to zero at some
point! Getting this series done would be a good step as it's blocking
many other smaller improvements. Hopefully we won't need another large
series like this to move forward.
[snip]
> > @@ -1162,13 +721,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
> > vin->format.field = RVIN_DEFAULT_FIELD;
> > vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
> >
> > - if (vin->info->use_mc) {
> > - vdev->device_caps |= V4L2_CAP_IO_MC;
> > - vdev->ioctl_ops = &rvin_mc_ioctl_ops;
> > - } else {
> > - vdev->ioctl_ops = &rvin_ioctl_ops;
> > - rvin_reset_format(vin);
> > - }
> > + vdev->device_caps |= V4L2_CAP_IO_MC;
>
> You can combine this with the line that sets device_caps.
>
> > + vdev->ioctl_ops = &rvin_mc_ioctl_ops;
>
> I would also move this above with the rest of the code that sets the
> vdev fields.
Great catch, thanks!
[snip]
--
Kind Regards,
Niklas Söderlund
