Re: [PATCH 03/16] rcar-vin: fix how pads are handled for v4l2 subdevice operations

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

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@ragnatech.se>
> > ---
> >  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