| Message ID | 20180214140257.1bfd266f@vento.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/02/18 17:03, Mauro Carvalho Chehab wrote: > Em Mon, 22 Jan 2018 13:31:18 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> Convert all g/s_parm calls to g/s_frame_interval. This allows us >> to remove the g/s_parm ops since those are a duplicate of >> g/s_frame_interval. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> drivers/media/i2c/mt9v011.c | 31 +++++++------------- >> drivers/media/i2c/ov6650.c | 35 +++++++++------------- >> drivers/media/i2c/ov7670.c | 24 +++++++-------- >> drivers/media/i2c/ov7740.c | 31 +++++++------------- >> drivers/media/i2c/tvp514x.c | 39 +++++++++---------------- >> drivers/media/i2c/vs6624.c | 29 +++++++----------- >> drivers/media/platform/atmel/atmel-isc.c | 10 ++----- >> drivers/media/platform/atmel/atmel-isi.c | 12 ++------ >> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------ >> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++---- >> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++--- >> drivers/media/platform/via-camera.c | 4 +-- >> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++---- >> 13 files changed, 122 insertions(+), 165 deletions(-) >> >> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c >> index 5e29064fae91..3e23c5b0de1f 100644 >> --- a/drivers/media/i2c/mt9v011.c >> +++ b/drivers/media/i2c/mt9v011.c >> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd, >> return 0; >> } >> >> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd, >> + struct v4l2_subdev_frame_interval *ival) >> { >> - struct v4l2_captureparm *cp = &parms->parm.capture; >> - >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >> - return -EINVAL; >> - >> - memset(cp, 0, sizeof(struct v4l2_captureparm)); >> - cp->capability = V4L2_CAP_TIMEPERFRAME; >> + memset(ival->reserved, 0, sizeof(ival->reserved)); > > Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing, > the best would be to do, instead: g_frame_interval is called by bridge drivers through the subdev ops. So that path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in v4l2-subdev.c. That doesn't mean it shouldn't be there as well. I believe my MC patch series actually adds the memset in subdev_do_ioctl. > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index c5639817db34..b18b418c080f 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > if (fi->pad >= sd->entity.num_pads) > return -EINVAL; > > + memset(fi->reserved, 0, sizeof(ival->reserved)); > return v4l2_subdev_call(sd, video, g_frame_interval, arg); > } > > (same applies to s_frame_interval). > > >> calc_fps(sd, >> - &cp->timeperframe.numerator, >> - &cp->timeperframe.denominator); >> + &ival->interval.numerator, >> + &ival->interval.denominator); >> >> return 0; >> } >> >> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd, >> + struct v4l2_subdev_frame_interval *ival) >> { >> - struct v4l2_captureparm *cp = &parms->parm.capture; >> - struct v4l2_fract *tpf = &cp->timeperframe; >> + struct v4l2_fract *tpf = &ival->interval; >> u16 speed; >> >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >> - return -EINVAL; >> - if (cp->extendedmode != 0) >> - return -EINVAL; >> - > > Hmm... why are you removing those sanity checks everywhere? > The core doesn't do it. > > All the above comments also apply to the other files modified by > this patch. struct v4l2_subdev_frame_interval has neither type nor extendedmode. The check for type is done in the v4l2_g/s_parm_cap helpers instead. And extendedmode is always set to 0. > >> + memset(ival->reserved, 0, sizeof(ival->reserved)); >> speed = calc_speed(sd, tpf->numerator, tpf->denominator); >> >> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed); >> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = { >> }; >> >> static const struct v4l2_subdev_video_ops mt9v011_video_ops = { >> - .g_parm = mt9v011_g_parm, >> - .s_parm = mt9v011_s_parm, >> + .g_frame_interval = mt9v011_g_frame_interval, >> + .s_frame_interval = mt9v011_s_frame_interval, >> }; >> >> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = { >> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c >> index 8975d16b2b24..3f962dae7534 100644 >> --- a/drivers/media/i2c/ov6650.c >> +++ b/drivers/media/i2c/ov6650.c >> @@ -201,7 +201,7 @@ struct ov6650 { >> struct v4l2_rect rect; /* sensor cropping window */ >> unsigned long pclk_limit; /* from host */ >> unsigned long pclk_max; /* from resolution and format */ >> - struct v4l2_fract tpf; /* as requested with s_parm */ >> + struct v4l2_fract tpf; /* as requested with s_frame_interval */ >> u32 code; >> enum v4l2_colorspace colorspace; >> }; >> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd, >> return 0; >> } >> >> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd, >> + struct v4l2_subdev_frame_interval *ival) >> { >> struct i2c_client *client = v4l2_get_subdevdata(sd); >> struct ov6650 *priv = to_ov6650(client); >> - struct v4l2_captureparm *cp = &parms->parm.capture; >> >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >> - return -EINVAL; >> - >> - memset(cp, 0, sizeof(*cp)); >> - cp->capability = V4L2_CAP_TIMEPERFRAME; >> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, >> + memset(ival->reserved, 0, sizeof(ival->reserved)); >> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, >> priv->pclk_limit, priv->pclk_max)); >> - cp->timeperframe.denominator = FRAME_RATE_MAX; >> + ival->interval.denominator = FRAME_RATE_MAX; >> >> dev_dbg(&client->dev, "Frame interval: %u/%u s\n", >> - cp->timeperframe.numerator, cp->timeperframe.denominator); >> + ival->interval.numerator, ival->interval.denominator); > > Hmm... not sure if a debug is needed here. Yet, if this is needed, > IMHO, it would make mroe sense to move it to the core. The core doesn't see this if this subdev op is called from a bridge driver. Regards, Hans
Em Wed, 14 Feb 2018 17:34:17 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 14/02/18 17:03, Mauro Carvalho Chehab wrote: > > Em Mon, 22 Jan 2018 13:31:18 +0100 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> Convert all g/s_parm calls to g/s_frame_interval. This allows us > >> to remove the g/s_parm ops since those are a duplicate of > >> g/s_frame_interval. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> drivers/media/i2c/mt9v011.c | 31 +++++++------------- > >> drivers/media/i2c/ov6650.c | 35 +++++++++------------- > >> drivers/media/i2c/ov7670.c | 24 +++++++-------- > >> drivers/media/i2c/ov7740.c | 31 +++++++------------- > >> drivers/media/i2c/tvp514x.c | 39 +++++++++---------------- > >> drivers/media/i2c/vs6624.c | 29 +++++++----------- > >> drivers/media/platform/atmel/atmel-isc.c | 10 ++----- > >> drivers/media/platform/atmel/atmel-isi.c | 12 ++------ > >> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------ > >> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++---- > >> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++--- > >> drivers/media/platform/via-camera.c | 4 +-- > >> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++---- > >> 13 files changed, 122 insertions(+), 165 deletions(-) > >> > >> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c > >> index 5e29064fae91..3e23c5b0de1f 100644 > >> --- a/drivers/media/i2c/mt9v011.c > >> +++ b/drivers/media/i2c/mt9v011.c > >> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd, > >> return 0; > >> } > >> > >> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_frame_interval *ival) > >> { > >> - struct v4l2_captureparm *cp = &parms->parm.capture; > >> - > >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >> - return -EINVAL; > >> - > >> - memset(cp, 0, sizeof(struct v4l2_captureparm)); > >> - cp->capability = V4L2_CAP_TIMEPERFRAME; > >> + memset(ival->reserved, 0, sizeof(ival->reserved)); > > > > Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing, > > the best would be to do, instead: > > g_frame_interval is called by bridge drivers through the subdev ops. So that > path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in > v4l2-subdev.c. True, but you could also do the same for v4l2 ioctl() handling logic. That would mean just two places with memset() instead of repeating the same pattern everywhere. > That doesn't mean it shouldn't be there as well. I believe my MC patch series > actually adds the memset in subdev_do_ioctl. > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index c5639817db34..b18b418c080f 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > if (fi->pad >= sd->entity.num_pads) > > return -EINVAL; > > > > + memset(fi->reserved, 0, sizeof(ival->reserved)); > > return v4l2_subdev_call(sd, video, g_frame_interval, arg); > > } > > > > (same applies to s_frame_interval). > > > > > >> calc_fps(sd, > >> - &cp->timeperframe.numerator, > >> - &cp->timeperframe.denominator); > >> + &ival->interval.numerator, > >> + &ival->interval.denominator); > >> > >> return 0; > >> } > >> > >> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_frame_interval *ival) > >> { > >> - struct v4l2_captureparm *cp = &parms->parm.capture; > >> - struct v4l2_fract *tpf = &cp->timeperframe; > >> + struct v4l2_fract *tpf = &ival->interval; > >> u16 speed; > >> > >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >> - return -EINVAL; > >> - if (cp->extendedmode != 0) > >> - return -EINVAL; > >> - > > > > Hmm... why are you removing those sanity checks everywhere? > > The core doesn't do it. > > > > All the above comments also apply to the other files modified by > > this patch. > > struct v4l2_subdev_frame_interval has neither type nor extendedmode. > > The check for type is done in the v4l2_g/s_parm_cap helpers instead. Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it. > And extendedmode is always set to 0. > > > > >> + memset(ival->reserved, 0, sizeof(ival->reserved)); > >> speed = calc_speed(sd, tpf->numerator, tpf->denominator); > >> > >> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed); > >> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = { > >> }; > >> > >> static const struct v4l2_subdev_video_ops mt9v011_video_ops = { > >> - .g_parm = mt9v011_g_parm, > >> - .s_parm = mt9v011_s_parm, > >> + .g_frame_interval = mt9v011_g_frame_interval, > >> + .s_frame_interval = mt9v011_s_frame_interval, > >> }; > >> > >> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = { > >> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > >> index 8975d16b2b24..3f962dae7534 100644 > >> --- a/drivers/media/i2c/ov6650.c > >> +++ b/drivers/media/i2c/ov6650.c > >> @@ -201,7 +201,7 @@ struct ov6650 { > >> struct v4l2_rect rect; /* sensor cropping window */ > >> unsigned long pclk_limit; /* from host */ > >> unsigned long pclk_max; /* from resolution and format */ > >> - struct v4l2_fract tpf; /* as requested with s_parm */ > >> + struct v4l2_fract tpf; /* as requested with s_frame_interval */ > >> u32 code; > >> enum v4l2_colorspace colorspace; > >> }; > >> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd, > >> return 0; > >> } > >> > >> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_frame_interval *ival) > >> { > >> struct i2c_client *client = v4l2_get_subdevdata(sd); > >> struct ov6650 *priv = to_ov6650(client); > >> - struct v4l2_captureparm *cp = &parms->parm.capture; > >> > >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >> - return -EINVAL; > >> - > >> - memset(cp, 0, sizeof(*cp)); > >> - cp->capability = V4L2_CAP_TIMEPERFRAME; > >> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, > >> + memset(ival->reserved, 0, sizeof(ival->reserved)); > >> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, > >> priv->pclk_limit, priv->pclk_max)); > >> - cp->timeperframe.denominator = FRAME_RATE_MAX; > >> + ival->interval.denominator = FRAME_RATE_MAX; > >> > >> dev_dbg(&client->dev, "Frame interval: %u/%u s\n", > >> - cp->timeperframe.numerator, cp->timeperframe.denominator); > >> + ival->interval.numerator, ival->interval.denominator); > > > > Hmm... not sure if a debug is needed here. Yet, if this is needed, > > IMHO, it would make mroe sense to move it to the core. > > The core doesn't see this if this subdev op is called from a bridge driver. True, but, when calling via a bridge driver, there's already a way to enable such kind debug. Thanks, Mauro
On 14/02/18 18:02, Mauro Carvalho Chehab wrote: > Em Wed, 14 Feb 2018 17:34:17 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> On 14/02/18 17:03, Mauro Carvalho Chehab wrote: >>> Em Mon, 22 Jan 2018 13:31:18 +0100 >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >>> >>>> From: Hans Verkuil <hans.verkuil@cisco.com> >>>> >>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us >>>> to remove the g/s_parm ops since those are a duplicate of >>>> g/s_frame_interval. >>>> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >>>> --- >>>> drivers/media/i2c/mt9v011.c | 31 +++++++------------- >>>> drivers/media/i2c/ov6650.c | 35 +++++++++------------- >>>> drivers/media/i2c/ov7670.c | 24 +++++++-------- >>>> drivers/media/i2c/ov7740.c | 31 +++++++------------- >>>> drivers/media/i2c/tvp514x.c | 39 +++++++++---------------- >>>> drivers/media/i2c/vs6624.c | 29 +++++++----------- >>>> drivers/media/platform/atmel/atmel-isc.c | 10 ++----- >>>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------ >>>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------ >>>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++---- >>>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++--- >>>> drivers/media/platform/via-camera.c | 4 +-- >>>> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++---- >>>> 13 files changed, 122 insertions(+), 165 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c >>>> index 5e29064fae91..3e23c5b0de1f 100644 >>>> --- a/drivers/media/i2c/mt9v011.c >>>> +++ b/drivers/media/i2c/mt9v011.c >>>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd, >>>> return 0; >>>> } >>>> >>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_frame_interval *ival) >>>> { >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; >>>> - >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >>>> - return -EINVAL; >>>> - >>>> - memset(cp, 0, sizeof(struct v4l2_captureparm)); >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME; >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); >>> >>> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing, >>> the best would be to do, instead: >> >> g_frame_interval is called by bridge drivers through the subdev ops. So that >> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in >> v4l2-subdev.c. > > True, but you could also do the same for v4l2 ioctl() handling logic. > > That would mean just two places with memset() instead of repeating the same > pattern everywhere. > >> That doesn't mean it shouldn't be there as well. I believe my MC patch series >> actually adds the memset in subdev_do_ioctl. What could be done is that this patch https://patchwork.linuxtv.org/patch/46955/ is applied first. After that these memsets can be removed since internally we don't need to touch them. >> >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>> index c5639817db34..b18b418c080f 100644 >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >>> if (fi->pad >= sd->entity.num_pads) >>> return -EINVAL; >>> >>> + memset(fi->reserved, 0, sizeof(ival->reserved)); >>> return v4l2_subdev_call(sd, video, g_frame_interval, arg); >>> } >>> >>> (same applies to s_frame_interval). >>> >>> >>>> calc_fps(sd, >>>> - &cp->timeperframe.numerator, >>>> - &cp->timeperframe.denominator); >>>> + &ival->interval.numerator, >>>> + &ival->interval.denominator); >>>> >>>> return 0; >>>> } >>>> >>>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >>>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_frame_interval *ival) >>>> { >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; >>>> - struct v4l2_fract *tpf = &cp->timeperframe; >>>> + struct v4l2_fract *tpf = &ival->interval; >>>> u16 speed; >>>> >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >>>> - return -EINVAL; >>>> - if (cp->extendedmode != 0) >>>> - return -EINVAL; >>>> - >>> >>> Hmm... why are you removing those sanity checks everywhere? >>> The core doesn't do it. >>> >>> All the above comments also apply to the other files modified by >>> this patch. >> >> struct v4l2_subdev_frame_interval has neither type nor extendedmode. >> >> The check for type is done in the v4l2_g/s_parm_cap helpers instead. > > Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it. ???? Are you confusing struct v4l2_streamparm with struct v4l2_subdev_frame_interval? v4l2_subdev.c deals with the latter, and struct v4l2_subdev_frame_interval has no type field. There is nothing to check. 'type' makes no sense in subdev drivers anyway since it refers to a buffer type and subdevs do not deal with buffers. > > >> And extendedmode is always set to 0. >> >>> >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); >>>> speed = calc_speed(sd, tpf->numerator, tpf->denominator); >>>> >>>> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed); >>>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = { >>>> }; >>>> >>>> static const struct v4l2_subdev_video_ops mt9v011_video_ops = { >>>> - .g_parm = mt9v011_g_parm, >>>> - .s_parm = mt9v011_s_parm, >>>> + .g_frame_interval = mt9v011_g_frame_interval, >>>> + .s_frame_interval = mt9v011_s_frame_interval, >>>> }; >>>> >>>> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = { >>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c >>>> index 8975d16b2b24..3f962dae7534 100644 >>>> --- a/drivers/media/i2c/ov6650.c >>>> +++ b/drivers/media/i2c/ov6650.c >>>> @@ -201,7 +201,7 @@ struct ov6650 { >>>> struct v4l2_rect rect; /* sensor cropping window */ >>>> unsigned long pclk_limit; /* from host */ >>>> unsigned long pclk_max; /* from resolution and format */ >>>> - struct v4l2_fract tpf; /* as requested with s_parm */ >>>> + struct v4l2_fract tpf; /* as requested with s_frame_interval */ >>>> u32 code; >>>> enum v4l2_colorspace colorspace; >>>> }; >>>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd, >>>> return 0; >>>> } >>>> >>>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) >>>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd, >>>> + struct v4l2_subdev_frame_interval *ival) >>>> { >>>> struct i2c_client *client = v4l2_get_subdevdata(sd); >>>> struct ov6650 *priv = to_ov6650(client); >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; >>>> >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) >>>> - return -EINVAL; >>>> - >>>> - memset(cp, 0, sizeof(*cp)); >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME; >>>> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); >>>> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, >>>> priv->pclk_limit, priv->pclk_max)); >>>> - cp->timeperframe.denominator = FRAME_RATE_MAX; >>>> + ival->interval.denominator = FRAME_RATE_MAX; >>>> >>>> dev_dbg(&client->dev, "Frame interval: %u/%u s\n", >>>> - cp->timeperframe.numerator, cp->timeperframe.denominator); >>>> + ival->interval.numerator, ival->interval.denominator); >>> >>> Hmm... not sure if a debug is needed here. Yet, if this is needed, >>> IMHO, it would make mroe sense to move it to the core. >> >> The core doesn't see this if this subdev op is called from a bridge driver. > > True, but, when calling via a bridge driver, there's already a way to > enable such kind debug. It can debug VIDIOC_G/S_PARM, not the g_frame_interval op. Also, when called via a v4l-subdev device node there is currently NO core logging. For the record, I don't really care about this debug statement myself one way or another, but changing this one way or another doesn't belong in this patch series. Regards, Hans
Em Wed, 14 Feb 2018 18:16:55 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 14/02/18 18:02, Mauro Carvalho Chehab wrote: > > Em Wed, 14 Feb 2018 17:34:17 +0100 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> On 14/02/18 17:03, Mauro Carvalho Chehab wrote: > >>> Em Mon, 22 Jan 2018 13:31:18 +0100 > >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >>> > >>>> From: Hans Verkuil <hans.verkuil@cisco.com> > >>>> > >>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us > >>>> to remove the g/s_parm ops since those are a duplicate of > >>>> g/s_frame_interval. > >>>> > >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>> --- > >>>> drivers/media/i2c/mt9v011.c | 31 +++++++------------- > >>>> drivers/media/i2c/ov6650.c | 35 +++++++++------------- > >>>> drivers/media/i2c/ov7670.c | 24 +++++++-------- > >>>> drivers/media/i2c/ov7740.c | 31 +++++++------------- > >>>> drivers/media/i2c/tvp514x.c | 39 +++++++++---------------- > >>>> drivers/media/i2c/vs6624.c | 29 +++++++----------- > >>>> drivers/media/platform/atmel/atmel-isc.c | 10 ++----- > >>>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------ > >>>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------ > >>>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++---- > >>>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++--- > >>>> drivers/media/platform/via-camera.c | 4 +-- > >>>> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++---- > >>>> 13 files changed, 122 insertions(+), 165 deletions(-) > >>>> > >>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c > >>>> index 5e29064fae91..3e23c5b0de1f 100644 > >>>> --- a/drivers/media/i2c/mt9v011.c > >>>> +++ b/drivers/media/i2c/mt9v011.c > >>>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd, > >>>> return 0; > >>>> } > >>>> > >>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_frame_interval *ival) > >>>> { > >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; > >>>> - > >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >>>> - return -EINVAL; > >>>> - > >>>> - memset(cp, 0, sizeof(struct v4l2_captureparm)); > >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME; > >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); > >>> > >>> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing, > >>> the best would be to do, instead: > >> > >> g_frame_interval is called by bridge drivers through the subdev ops. So that > >> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in > >> v4l2-subdev.c. > > > > True, but you could also do the same for v4l2 ioctl() handling logic. > > > > That would mean just two places with memset() instead of repeating the same > > pattern everywhere. > > > >> That doesn't mean it shouldn't be there as well. I believe my MC patch series > >> actually adds the memset in subdev_do_ioctl. > > What could be done is that this patch https://patchwork.linuxtv.org/patch/46955/ > is applied first. After that these memsets can be removed since internally we > don't need to touch them. Works for me. > >> > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>> index c5639817db34..b18b418c080f 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > >>> if (fi->pad >= sd->entity.num_pads) > >>> return -EINVAL; > >>> > >>> + memset(fi->reserved, 0, sizeof(ival->reserved)); > >>> return v4l2_subdev_call(sd, video, g_frame_interval, arg); > >>> } > >>> > >>> (same applies to s_frame_interval). > >>> > >>> > >>>> calc_fps(sd, > >>>> - &cp->timeperframe.numerator, > >>>> - &cp->timeperframe.denominator); > >>>> + &ival->interval.numerator, > >>>> + &ival->interval.denominator); > >>>> > >>>> return 0; > >>>> } > >>>> > >>>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >>>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_frame_interval *ival) > >>>> { > >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; > >>>> - struct v4l2_fract *tpf = &cp->timeperframe; > >>>> + struct v4l2_fract *tpf = &ival->interval; > >>>> u16 speed; > >>>> > >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >>>> - return -EINVAL; > >>>> - if (cp->extendedmode != 0) > >>>> - return -EINVAL; > >>>> - > >>> > >>> Hmm... why are you removing those sanity checks everywhere? > >>> The core doesn't do it. > >>> > >>> All the above comments also apply to the other files modified by > >>> this patch. > >> > >> struct v4l2_subdev_frame_interval has neither type nor extendedmode. > >> > >> The check for type is done in the v4l2_g/s_parm_cap helpers instead. > > > > Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it. > > ???? > > Are you confusing struct v4l2_streamparm with struct v4l2_subdev_frame_interval? > > v4l2_subdev.c deals with the latter, and struct v4l2_subdev_frame_interval has > no type field. There is nothing to check. > > 'type' makes no sense in subdev drivers anyway since it refers to a buffer type > and subdevs do not deal with buffers. Yeah, you're right: those checks can be removed. > >> And extendedmode is always set to 0. > >> > >>> > >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); > >>>> speed = calc_speed(sd, tpf->numerator, tpf->denominator); > >>>> > >>>> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed); > >>>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = { > >>>> }; > >>>> > >>>> static const struct v4l2_subdev_video_ops mt9v011_video_ops = { > >>>> - .g_parm = mt9v011_g_parm, > >>>> - .s_parm = mt9v011_s_parm, > >>>> + .g_frame_interval = mt9v011_g_frame_interval, > >>>> + .s_frame_interval = mt9v011_s_frame_interval, > >>>> }; > >>>> > >>>> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = { > >>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c > >>>> index 8975d16b2b24..3f962dae7534 100644 > >>>> --- a/drivers/media/i2c/ov6650.c > >>>> +++ b/drivers/media/i2c/ov6650.c > >>>> @@ -201,7 +201,7 @@ struct ov6650 { > >>>> struct v4l2_rect rect; /* sensor cropping window */ > >>>> unsigned long pclk_limit; /* from host */ > >>>> unsigned long pclk_max; /* from resolution and format */ > >>>> - struct v4l2_fract tpf; /* as requested with s_parm */ > >>>> + struct v4l2_fract tpf; /* as requested with s_frame_interval */ > >>>> u32 code; > >>>> enum v4l2_colorspace colorspace; > >>>> }; > >>>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd, > >>>> return 0; > >>>> } > >>>> > >>>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms) > >>>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_frame_interval *ival) > >>>> { > >>>> struct i2c_client *client = v4l2_get_subdevdata(sd); > >>>> struct ov6650 *priv = to_ov6650(client); > >>>> - struct v4l2_captureparm *cp = &parms->parm.capture; > >>>> > >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > >>>> - return -EINVAL; > >>>> - > >>>> - memset(cp, 0, sizeof(*cp)); > >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME; > >>>> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, > >>>> + memset(ival->reserved, 0, sizeof(ival->reserved)); > >>>> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, > >>>> priv->pclk_limit, priv->pclk_max)); > >>>> - cp->timeperframe.denominator = FRAME_RATE_MAX; > >>>> + ival->interval.denominator = FRAME_RATE_MAX; > >>>> > >>>> dev_dbg(&client->dev, "Frame interval: %u/%u s\n", > >>>> - cp->timeperframe.numerator, cp->timeperframe.denominator); > >>>> + ival->interval.numerator, ival->interval.denominator); > >>> > >>> Hmm... not sure if a debug is needed here. Yet, if this is needed, > >>> IMHO, it would make mroe sense to move it to the core. > >> > >> The core doesn't see this if this subdev op is called from a bridge driver. > > > > True, but, when calling via a bridge driver, there's already a way to > > enable such kind debug. > > It can debug VIDIOC_G/S_PARM, not the g_frame_interval op. Also, when called > via a v4l-subdev device node there is currently NO core logging. > > For the record, I don't really care about this debug statement myself one > way or another, but changing this one way or another doesn't belong in this > patch series. Ok. Yet, IMHO, it is probably safe to just remove the debug statements on the above driver, as I fail to see that just ov6650 driver would need this. > > Regards, > > Hans Thanks, Mauro
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index c5639817db34..b18b418c080f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) if (fi->pad >= sd->entity.num_pads) return -EINVAL; + memset(fi->reserved, 0, sizeof(ival->reserved)); return v4l2_subdev_call(sd, video, g_frame_interval, arg); }