media: bugfixes for mt9m111 driver
Subsystems (2)
cover media: bugfixes for mt9m111 driver
This patch series contains four bugfixes for mt9m111 driver. The first patch fixes the recent change for v4.21.
Akinobu Mita (4):
media: mt9m111: fix setting pixclk polarity
media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with
V4L2_SUBDEV_FORMAT_TRY
media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls
media: mt9m111: set initial frame size other than 0x0
drivers/media/i2c/mt9m111.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
Signature · 1 line
2.7.4
PATCH 1/4 media: mt9m111: fix setting pixclk polarity
Since commit 98480d65c48c ("media: mt9m111: allow to setup pixclk polarity"), the MT9M111_OUTFMT_INV_PIX_CLOCK bit in the output format control 2 register has to be changed depending on the pclk-sample property setting.
Without this change, the MT9M111_OUTFMT_INV_PIX_CLOCK bit is unchanged.
--- drivers/media/i2c/mt9m111.c | 1 + 1 file changed, 1 insertion(+)
drivers/media/i2c/mt9m111.c +1 -0
@@ -542,6 +542,7 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111, {struct i2c_client *client =v4l2_get_subdevdata (&mt9m111->subdev);u16 data_outfmt2, mask_outfmt2 =MT9M111_OUTFMT_PROCESSED_BAYER | +MT9M111_OUTFMT_INV_PIX_CLOCK |MT9M111_OUTFMT_BYPASS_IFP |MT9M111_OUTFMT_RGB |MT9M111_OUTFMT_RGB565 |MT9M111_OUTFMT_RGB555 | MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
Signature · 1 line
2.7.4
Re: [PATCH 1/4] media: mt9m111: fix setting pixclk polarity
Hi Akinobu,
Quoted 6 lines · Akinobu Mita
Since commit
98480d65c48c("media: mt9m111: allow to setup pixclk polarity"), theMT9M111_OUTFMT_INV_PIX_CLOCKbit in the output format control 2 register has to be changed depending on the pclk-sample property setting.Without this change, the
MT9M111_OUTFMT_INV_PIX_CLOCKbit is unchanged.
I don't know what you mean, it will get applied depending on the property.
8<------------------------------------------------------------------------ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
u32 code)
{
... /* receiver samples on falling edge, chip-hw default is rising */ if (mt9m111->pclk_sample == 0) mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK; ... }
8<------------------------------------------------------------------------
Isn't this right?
Can you cc me the other patches too, so I can keep track of it easier?
Regards,
Marco
Quoted 28 lines
Fixes 98480d65c48c ("media: mt9m111: allow to setup pixclk polarity")Cc Enrico Scholz <[email protected]>Cc Michael Grzeschik <[email protected]>Cc Marco Felsch <[email protected]>Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 1 + 1 file changed, 1 insertion(+)drivers/media/i2c/mt9m111.c +1
-0@@ -542,6 +542,7 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111, {struct i2c_client *client =v4l2_get_subdevdata (&mt9m111->subdev);u16 data_outfmt2, mask_outfmt2 =MT9M111_OUTFMT_PROCESSED_BAYER | +MT9M111_OUTFMT_INV_PIX_CLOCK |MT9M111_OUTFMT_BYPASS_IFP |MT9M111_OUTFMT_RGB |MT9M111_OUTFMT_RGB565 |MT9M111_OUTFMT_RGB555 | MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |Signature · 1 line
2.7.4
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Re: [PATCH 1/4] media: mt9m111: fix setting pixclk polarity
2018年12月31日(月) 19:30 Marco Felsch <[email protected]>:
Quoted 28 lines
Hi Akinobu,
Quoted 6 lines · Akinobu Mita
Since commit
98480d65c48c("media: mt9m111: allow to setup pixclk polarity"), theMT9M111_OUTFMT_INV_PIX_CLOCKbit in the output format control 2 register has to be changed depending on the pclk-sample property setting.Without this change, the
MT9M111_OUTFMT_INV_PIX_CLOCKbit is unchanged.I don't know what you mean, it will get applied depending on the property.
8<------------------------------------------------------------------------ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
u32 code){
... /* receiver samples on falling edge, chip-hw default is rising */ if (mt9m111->pclk_sample == 0) mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK; ... }
Quoted 3 lines
8<------------------------------------------------------------------------
Isn't this right?
You are right. I misread and thought the commit sets the MT9M111_OUTFMT_INV_PIX_CLOCK bit in 'data_outfmt2' instead of 'mask_outfmt2'.
This patch will be dropped from this series in the next version.
Quoted 1 line
Can you cc me the other patches too, so I can keep track of it easier?
OK. I'll do from v2.
PATCH 2/4 media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
In reply to Akinobu MitaThe VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE is specified.
--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
drivers/media/i2c/mt9m111.c +31 -0
@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ; +if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + mf =v4l2_subdev_get_try_format (sd, cfg,0 ); + format->format = *mf; +return 0 ; +#else +return -ENOTTY ; +#endif + } + mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;
@@ -1090,6 +1100,26 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)return 0 ; } +static int mt9m111_init_cfg (struct v4l2_subdev *sd, +struct v4l2_subdev_pad_config *cfg) +{ +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); +struct v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width = mt9m111->width; + format->height = mt9m111->height; + format->code = mt9m111->fmt->code; + format->colorspace = mt9m111->fmt->colorspace; + format->field =V4L2_FIELD_NONE ; + format->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + format->quantization =V4L2_QUANTIZATION_DEFAULT ; + format->xfer_func =V4L2_XFER_FUNC_DEFAULT ; +#endif +return 0 ; +} +static int mt9m111_g_mbus_config (struct v4l2_subdev *sd,struct v4l2_mbus_config *cfg) {
@@ -1115,6 +1145,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { };static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { + .init_cfg = mt9m111_init_cfg, .enum_mbus_code = mt9m111_enum_mbus_code, .get_selection = mt9m111_get_selection, .set_selection = mt9m111_set_selection,
Signature · 1 line
2.7.4
Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
Quoted 16 lines · Akinobu Mita
The
VIDIOC_SUBDEV_G_FMTioctl for this driver doesn't recognizeV4L2_SUBDEV_FORMAT_TRYand always works as ifV4L2_SUBDEV_FORMAT_ACTIVEis specified.Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/media/i2c/mt9m111.c +0
-0
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 532
Quoted 5 lines
drivers/media/i2c/mt9m111.c +2
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ; +if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
This ifdef is made in the include/media/v4l2-subdev.h, so I would drop it.
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 533
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ mf =v4l2_subdev_get_try_format (sd, cfg,0 );
I would use format->pad instead of the static 0.
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 534
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ format->format = *mf;
Is this correct? I tought v4l2_subdev_get_try_format() will return the try_pad which we need to fill.
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 537
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+return 0 ; +#else +return -ENOTTY ;
Return this error is not specified in the API-Doc.
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 540
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+#endif + } +
If I understood it right, your patch should look like:
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 542
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) + mf =v4l2_subdev_get_try_format (sd, cfg, format->pad);
Sakari please correct me if it's wrong.
drivers/media/i2c/mt9m111.c static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) · line 1104
Quoted 9 lines
drivers/media/i2c/mt9m111.c +2
-0mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;@@ -1090,6 +1100,26 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)return 0 ; } +static int mt9m111_init_cfg (struct v4l2_subdev *sd, +struct v4l2_subdev_pad_config *cfg)
Is this related to the patch description? I would split this into a seperate patch.
Quoted 33 lines
drivers/media/i2c/mt9m111.c +19
-0+{ +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); +struct v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width = mt9m111->width; + format->height = mt9m111->height; + format->code = mt9m111->fmt->code; + format->colorspace = mt9m111->fmt->colorspace; + format->field =V4L2_FIELD_NONE ; + format->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + format->quantization =V4L2_QUANTIZATION_DEFAULT ; + format->xfer_func =V4L2_XFER_FUNC_DEFAULT ; +#endif +return 0 ; +} +static int mt9m111_g_mbus_config (struct v4l2_subdev *sd,struct v4l2_mbus_config *cfg) {@@ -1115,6 +1145,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { };static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { + .init_cfg = mt9m111_init_cfg, .enum_mbus_code = mt9m111_enum_mbus_code, .get_selection = mt9m111_get_selection, .set_selection = mt9m111_set_selection,Signature · 1 line
2.7.4
Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
2018年12月31日(月) 19:54 Marco Felsch <[email protected]>:
Quoted 21 lines · Akinobu Mita
Quoted 19 lines
The
VIDIOC_SUBDEV_G_FMTioctl for this driver doesn't recognizeV4L2_SUBDEV_FORMAT_TRYand always works as ifV4L2_SUBDEV_FORMAT_ACTIVEis specified.Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ;
Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 532
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API This ifdef is made in the
include/media/v4l2-subdev.h, so I would drop it.
I sent similar fix for ov2640 driver and kerel test robot reported build test failure. So I think this ifdef is necessary.
v1: https://www.mail-archive.com/[email protected]/msg137098.html
v2: https://www.mail-archive.com/[email protected]/msg141735.html
Quoted 3 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 533
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ mf =v4l2_subdev_get_try_format (sd, cfg,0 );I would use
format->padinstead of the static 0.
OK.
Quoted 4 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 534
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ format->format = *mf;Is this correct? I tought
v4l2_subdev_get_try_format()will return thetry_padwhich we need to fill.
I think this is correct. Other sensor drivers are doing the same thing in get_fmt() callback.
Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 537
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+return 0 ; +#else +return -ENOTTY ;Return this error is not specified in the API-Doc.
I think this 'return -ENOTTY' is not reachable even if CONFIG_VIDEO_V4L2_SUBDEV_API is not set, and can be replaced with any return value.
Quoted 18 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 540
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+#endif + } +If I understood it right, your patch should look like:
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 542
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) + mf =v4l2_subdev_get_try_format (sd, cfg, format->pad);Sakari please correct me if it's wrong.
Quoted 7 lines
drivers/media/i2c/mt9m111.c +0
-0mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;@@ -1090,6 +1100,26 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)return 0 ; }
Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) · line 1104
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+static int mt9m111_init_cfg (struct v4l2_subdev *sd, +struct v4l2_subdev_pad_config *cfg)Is this related to the patch description? I would split this into a seperate patch.
The mt9m111_init_cfg() initializes the try format with default setting. So this is required in case get_fmt() with V4L2_SUBDEV_FORMAT_TRY is called before set_fmt() with V4L2_SUBDEV_FORMAT_TRY is called.
Quoted 33 lines
Quoted 33 lines
drivers/media/i2c/mt9m111.c +19
-1+{ +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); +struct v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width = mt9m111->width; + format->height = mt9m111->height; + format->code = mt9m111->fmt->code; + format->colorspace = mt9m111->fmt->colorspace; + format->field =V4L2_FIELD_NONE ; + format->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + format->quantization =V4L2_QUANTIZATION_DEFAULT ; + format->xfer_func =V4L2_XFER_FUNC_DEFAULT ; +#endif +return 0 ; +} +static int mt9m111_g_mbus_config (struct v4l2_subdev *sd,struct v4l2_mbus_config *cfg) {@@ -1115,6 +1145,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { };static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { + .init_cfg = mt9m111_init_cfg, .enum_mbus_code = mt9m111_enum_mbus_code, .get_selection = mt9m111_get_selection, .set_selection = mt9m111_set_selection, --2.7.4
Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
Quoted 28 lines · Akinobu Mita
2018年12月31日(月) 19:54 Marco Felsch <[email protected]>:
Quoted 21 lines · Akinobu Mita
Quoted 19 lines
The
VIDIOC_SUBDEV_G_FMTioctl for this driver doesn't recognizeV4L2_SUBDEV_FORMAT_TRYand always works as ifV4L2_SUBDEV_FORMAT_ACTIVEis specified.Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ;Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 532
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API This ifdef is made in the
include/media/v4l2-subdev.h, so I would drop it.
Quoted 5 lines
I sent similar fix for ov2640 driver and kerel test robot reported build test failure. So I think this ifdef is necessary.
v1: https://www.mail-archive.com/[email protected]/msg137098.html
v2: https://www.mail-archive.com/[email protected]/msg141735.html
You are absolutely true, sorry my mistake.. Unfortunately my patch [1] wasn't applied which fixes it commonly. This patch will avoid the 2nd ifdef in init_cfg() too.
[1] https://www.spinics.net/lists/linux-media/msg138940.html
Quoted 7 lines
Quoted 4 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 533
Quoted 2 lines
drivers/media/i2c/mt9m111.c +1
-0+ mf =v4l2_subdev_get_try_format (sd, cfg,0 );I would use
format->padinstead of the static 0.OK.
Quoted 7 lines
Quoted 4 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 534
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ format->format = *mf;Is this correct? I tought
v4l2_subdev_get_try_format()will return thetry_padwhich we need to fill.I think this is correct. Other sensor drivers are doing the same thing in
get_fmt()callback.
Yes, you're right.
Quoted 9 lines
Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 537
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+return 0 ; +#else +return -ENOTTY ;Return this error is not specified in the API-Doc.
I think this 'return
-ENOTTY' is not reachable even ifCONFIG_VIDEO_V4L2_SUBDEV_APIis not set, and can be replaced with any return value.
Sorry I didn't catched this. When it's not reachable why is it there and why isn't it reachable? If the format->which = V4L2_SUBDEV_FORMAT_TRY and we don't configure CONFIG_VIDEO_V4L2_SUBDEV_API, then this path will be reached, or overlooked I something?
Quoted 25 lines
Quoted 19 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 540
Quoted 4 lines
drivers/media/i2c/mt9m111.c +3
-0+#endif + } +If I understood it right, your patch should look like:
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 542
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) + mf =v4l2_subdev_get_try_format (sd, cfg, format->pad);Sakari please correct me if it's wrong.
Quoted 7 lines
drivers/media/i2c/mt9m111.c +0
-0mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;@@ -1090,6 +1100,26 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)return 0 ; }Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) · line 1104
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+static int mt9m111_init_cfg (struct v4l2_subdev *sd, +struct v4l2_subdev_pad_config *cfg)Is this related to the patch description? I would split this into a seperate patch.
Quoted 3 lines
The
mt9m111_init_cfg()initializes the try format with default setting. So this is required in caseget_fmt()withV4L2_SUBDEV_FORMAT_TRYis called beforeset_fmt()withV4L2_SUBDEV_FORMAT_TRYis called.
Okay, I would split this but it's my personal opinion.
Kind regards,
Marco
Quoted 34 lines
Quoted 33 lines
Quoted 33 lines
drivers/media/i2c/mt9m111.c +19
-1+{ +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); +struct v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width = mt9m111->width; + format->height = mt9m111->height; + format->code = mt9m111->fmt->code; + format->colorspace = mt9m111->fmt->colorspace; + format->field =V4L2_FIELD_NONE ; + format->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + format->quantization =V4L2_QUANTIZATION_DEFAULT ; + format->xfer_func =V4L2_XFER_FUNC_DEFAULT ; +#endif +return 0 ; +} +static int mt9m111_g_mbus_config (struct v4l2_subdev *sd,struct v4l2_mbus_config *cfg) {@@ -1115,6 +1145,7 @@ static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = { };static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = { + .init_cfg = mt9m111_init_cfg, .enum_mbus_code = mt9m111_enum_mbus_code, .get_selection = mt9m111_get_selection, .set_selection = mt9m111_set_selection, --2.7.4
Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
Hello,
sorry to jump in
Quoted 32 lines · Marco Felsch
Quoted 31 lines · Akinobu Mita
2018年12月31日(月) 19:54 Marco Felsch <[email protected]>:
Quoted 21 lines · Akinobu Mita
Quoted 19 lines
The
VIDIOC_SUBDEV_G_FMTioctl for this driver doesn't recognizeV4L2_SUBDEV_FORMAT_TRYand always works as ifV4L2_SUBDEV_FORMAT_ACTIVEis specified.Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ;Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 532
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API This ifdef is made in the
include/media/v4l2-subdev.h, so I would drop it.I sent similar fix for ov2640 driver and kerel test robot reported build test failure. So I think this ifdef is necessary.
Quoted 9 lines
Quoted 2 lines
v1: https://www.mail-archive.com/[email protected]/msg137098.html
v2: https://www.mail-archive.com/[email protected]/msg141735.htmlYou are absolutely true, sorry my mistake.. Unfortunately my patch [1] wasn't applied which fixes it commonly. This patch will avoid the 2nd ifdef in
init_cfg()too.[1] https://www.spinics.net/lists/linux-media/msg138940.html
There have been recents attempts to do the same, please see:
https://lkml.org/lkml/2018/11/28/1080
and Hans' attempt at
https://patchwork.kernel.org/patch/10717699/
Unfortunately seems like we're gonna live with those ifdefs
Thanks
j
Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
In reply to Marco Felsch2019年1月3日(木) 22:47 Marco Felsch <[email protected]>:
Quoted 65 lines · Akinobu Mita
Quoted 28 lines
2018年12月31日(月) 19:54 Marco Felsch <[email protected]>:
Quoted 21 lines · Akinobu Mita
Quoted 19 lines
The
VIDIOC_SUBDEV_G_FMTioctl for this driver doesn't recognizeV4L2_SUBDEV_FORMAT_TRYand always works as ifV4L2_SUBDEV_FORMAT_ACTIVEis specified.Cc Sakari Ailus <[email protected]>Cc Mauro Carvalho Chehab <[email protected]>Signed-off-by Akinobu Mita <[email protected]>--- drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)return -EINVAL ;Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 532
Quoted 2 lines
drivers/media/i2c/mt9m111.c +2
-0+if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API This ifdef is made in the
include/media/v4l2-subdev.h, so I would drop it.Quoted 5 lines
I sent similar fix for ov2640 driver and kerel test robot reported build test failure. So I think this ifdef is necessary.
v1: https://www.mail-archive.com/[email protected]/msg137098.html
v2: https://www.mail-archive.com/[email protected]/msg141735.htmlYou are absolutely true, sorry my mistake.. Unfortunately my patch [1] wasn't applied which fixes it commonly. This patch will avoid the 2nd ifdef in
init_cfg()too.[1] https://www.spinics.net/lists/linux-media/msg138940.html
Quoted 7 lines
Quoted 4 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 533
Quoted 2 lines
drivers/media/i2c/mt9m111.c +1
-0+ mf =v4l2_subdev_get_try_format (sd, cfg,0 );I would use
format->padinstead of the static 0.OK.
Quoted 7 lines
Quoted 4 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 534
Quoted 1 line
drivers/media/i2c/mt9m111.c +1
-0+ format->format = *mf;Is this correct? I tought
v4l2_subdev_get_try_format()will return thetry_padwhich we need to fill.I think this is correct. Other sensor drivers are doing the same thing in
get_fmt()callback.Yes, you're right.
Quoted 6 lines
Quoted 5 lines
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 537
Quoted 3 lines
drivers/media/i2c/mt9m111.c +3
-0+return 0 ; +#else +return -ENOTTY ;Return this error is not specified in the API-Doc.
Quoted 8 lines
Quoted 3 lines
I think this 'return
-ENOTTY' is not reachable even ifCONFIG_VIDEO_V4L2_SUBDEV_APIis not set, and can be replaced with any return value.Sorry I didn't catched this. When it's not reachable why is it there and why isn't it reachable? If the
format->which=V4L2_SUBDEV_FORMAT_TRYand we don't configureCONFIG_VIDEO_V4L2_SUBDEV_API, then this path will be reached, or overlooked I something?
As far as I can see, when CONFIG_VIDEO_V4L2_SUBDEV_API is not defined, the get_fmt() callback is always called with
'format->which == V4L2_SUBDEV_FORMAT_ACTIVE'.
There is only one call site that the get_fmt() is called with 'format->which == V4L2_SUBDEV_FORMAT_TRY' in drivers/media/v4l2-core/v4l2-subdev.c: subdev_do_ioctl(). But the call site is enclosed by ifdef CONFIG_VIDEO_V4L2_SUBDEV_API.
So the hunk of the patch can be changed to:
if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
mf = v4l2_subdev_get_try_format(sd, cfg, format->pad);
format->format = *mf;
return 0;
#endif
}PATCH 3/4 media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls
In reply to Akinobu MitaThis driver doesn't set all members of mbus format field when the VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.
This is detected by v4l2-compliance.
--- drivers/media/i2c/mt9m111.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/media/i2c/mt9m111.c +7 -0
@@ -543,6 +543,9 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd, mf->code = mt9m111->fmt->code; mf->colorspace = mt9m111->fmt->colorspace; mf->field =V4L2_FIELD_NONE ; + mf->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + mf->quantization =V4L2_QUANTIZATION_DEFAULT ; + mf->xfer_func =V4L2_XFER_FUNC_DEFAULT ;return 0 ; }
@@ -671,6 +674,10 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd, mf->code = fmt->code; mf->colorspace = fmt->colorspace; + mf->field =V4L2_FIELD_NONE ; + mf->ycbcr_enc =V4L2_YCBCR_ENC_DEFAULT ; + mf->quantization =V4L2_QUANTIZATION_DEFAULT ; + mf->xfer_func =V4L2_XFER_FUNC_DEFAULT ;if (format->which ==V4L2_SUBDEV_FORMAT_TRY ) { cfg->try_fmt = *mf;
Signature · 1 line
2.7.4
PATCH 4/4 media: mt9m111: set initial frame size other than 0x0
In reply to Akinobu MitaThis driver sets initial frame width and height to 0x0, which is invalid. So set it to selection rectangle bounds instead.
This is detected by v4l2-compliance detected.
--- drivers/media/i2c/mt9m111.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/media/i2c/mt9m111.c +2 -0
@@ -1312,6 +1312,8 @@ static int mt9m111_probe(struct i2c_client *client, mt9m111->rect.top =MT9M111_MIN_DARK_ROWS ; mt9m111->rect.width =MT9M111_MAX_WIDTH ; mt9m111->rect.height =MT9M111_MAX_HEIGHT ; + mt9m111->width = mt9m111->rect.width; + mt9m111->height = mt9m111->rect.height; mt9m111->fmt = &mt9m111_colour_fmts[0 ]; mt9m111->lastpage =-1 ;mutex_init (&mt9m111->power_lock);
Signature · 1 line
2.7.4