Hi Sergei,
Thanks for your feedback.
On 2017-03-15 12:12:21 +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 3/14/2017 9:59 PM, Niklas Söderlund wrote:
>
> > The rcar-vin driver only uses one pad, pad number 0.
> >
> > - All v4l2 operations that did not check that the requested operation
> > was for pad 0 have been updated with a check to enforce this.
> >
> > - All v4l2 operations that stored (and later restore) the requested pad
>
> Restored?
Will update for v2.
>
> > before substituting it for the subdevice pad number have been updated
> > to not store the incoming pad and simply restore it to 0 after the
> > subdevice operation is complete.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 7ca27599b9982ffc..610f59e2a9142622 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -550,14 +550,16 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > struct v4l2_subdev *sd = vin_to_source(vin);
> > - int pad, ret;
> > + int ret;
> > +
> > + if (timings->pad)
> > + return -EINVAL;
> >
> > - pad = timings->pad;
> > timings->pad = vin->sink_pad_idx;
> >
> > ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>
> Does this still compile after you removed 'pad'?
Yes, the pad in v4l2_subdev_call() do not refer to the pad variable but
the pad operations of the subdevice ops struct, the macro is defined as:
#define v4l2_subdev_call(sd, o, f, args...) \
(!(sd) ? -ENODEV : (((sd)->ops->o && (sd)->ops->o->f) ? \
(sd)->ops->o->f((sd), ##args) : -ENOIOCTLCMD))
So if you expand the macro it looks like:
sd->ops->pad->enum_dv_timings(timings);
I agree it's confusing and I had the same thought the first times I
looked at it too :-)
>
> >
> > - timings->pad = pad;
> > + timings->pad = 0;
> >
> > return ret;
> > }
> > @@ -600,14 +602,16 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> > struct v4l2_subdev *sd = vin_to_source(vin);
> > - int pad, ret;
> > + int ret;
> > +
> > + if (cap->pad)
> > + return -EINVAL;
> >
> > - pad = cap->pad;
> > cap->pad = vin->sink_pad_idx;
> >
> > ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
>
> And this?
>
> >
> > - cap->pad = pad;
> > + cap->pad = 0;
> >
> > return ret;
> > }
> [...]
>
> MBR, Sergei
>
--
Regards,
Niklas Söderlund
