media: bugfixes for mt9m111 driver
Subsystems (2)
v3 cover media: bugfixes for mt9m111 driver
This patch series contains four bugfixes for mt9m111 driver.
* v3
- Set initial try format with default configuration instead of current one.
* v2
- Drop patch 1/4 in v1 ("fix setting pixclk polarit") since it was wrong.
- Use format->pad for the argument of v4l2_subdev_get_try_format().
Akinobu Mita (3):
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 | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
Signature · 1 line
2.7.4
PATCH v3 1/3 media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
The 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.
---
* v3
- Set initial try format with default configuration instead of current one.
drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
drivers/media/i2c/mt9m111.c +30 -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, format->pad); + format->format = *mf; +return 0 ; +#else +return -ENOTTY ; +#endif + } + mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;
@@ -1089,6 +1099,25 @@ 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 v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width =MT9M111_MAX_WIDTH ; + format->height =MT9M111_MAX_HEIGHT ; + format->code = mt9m111_colour_fmts[0 ].code; + format->colorspace = mt9m111_colour_fmts[0 ].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) {
@@ -1114,6 +1143,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 v3 1/3] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
Hi Akinobu,
sorry for the delayed response.
Quoted 24 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 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]>---
* v3
- Set initial try format with default configuration instead of current one.drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)
drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 538
Quoted 10 lines
drivers/media/i2c/mt9m111.c +8
-0return -EINVAL ; +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 ; +#else +return -ENOTTY ; +#endif
If've checked this again and found the ov* devices do this too. IMO it's not good for other developers to check the upper layer if the '#else' path is reachable. There are also some code analyzer tools which will report this as issue/warning.
As I said, I checked the v4l2_subdev_get_try_format() usage again and found the solution made by the mt9v111.c better. Why do you don't add a dependency in the Kconfig, so we can drop this ifdef?
Regards,
Marco
Quoted 43 lines
drivers/media/i2c/mt9m111.c +22
-0+ } + mf->width = mt9m111->width; mf->height = mt9m111->height; mf->code = mt9m111->fmt->code;@@ -1089,6 +1099,25 @@ 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 v4l2_mbus_framefmt *format = +v4l2_subdev_get_try_format (sd, cfg,0 ); + + format->width =MT9M111_MAX_WIDTH ; + format->height =MT9M111_MAX_HEIGHT ; + format->code = mt9m111_colour_fmts[0 ].code; + format->colorspace = mt9m111_colour_fmts[0 ].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) {@@ -1114,6 +1143,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 v3 1/3] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
2019年1月24日(木) 0:17 Marco Felsch <[email protected]>:
Quoted 46 lines
Hi Akinobu,
sorry for the delayed response.
Quoted 24 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 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]>---
* v3
- Set initial try format with default configuration instead of current one.drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 538
Quoted 10 lines
drivers/media/i2c/mt9m111.c +8
-0return -EINVAL ; +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 ; +#else +return -ENOTTY ; +#endif If've checked this again and found the ov* devices do this too. IMO it's not good for other developers to check the upper layer if the '#else' path is reachable. There are also some code analyzer tools which will report this as issue/warning.
Quoted 3 lines
As I said, I checked the
v4l2_subdev_get_try_format()usage again and found the solution made by themt9v111.cbetter. Why do you don't add a dependency in theKconfig, so we can drop this ifdef?
I'm ok with adding CONFIG_VIDEO_V4L2_SUBDEV_API dependency to this driver, because I always enable it.
But it may cause an issue on some environments that don't require CONFIG_VIDEO_V4L2_SUBDEV_API.
Sakari, do you have any opinion?
Sakari Ailus
maintainerRe: [PATCH v3 1/3] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
Hi Mita-san, Marco,
Quoted 51 lines · Akinobu Mita
2019年1月24日(木) 0:17 Marco Felsch <[email protected]>:
Quoted 46 lines
Hi Akinobu,
sorry for the delayed response.
Quoted 24 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 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]>---
* v3
- Set initial try format with default configuration instead of current one.drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 538
Quoted 10 lines
drivers/media/i2c/mt9m111.c +8
-0return -EINVAL ; +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 ; +#else +return -ENOTTY ; +#endif If've checked this again and found the ov* devices do this too. IMO it's not good for other developers to check the upper layer if the '#else' path is reachable. There are also some code analyzer tools which will report this as issue/warning.
Quoted 3 lines
As I said, I checked the
v4l2_subdev_get_try_format()usage again and found the solution made by themt9v111.cbetter. Why do you don't add a dependency in theKconfig, so we can drop this ifdef?
Quoted 7 lines
I'm ok with adding
CONFIG_VIDEO_V4L2_SUBDEV_APIdependency to this driver, because I always enable it.But it may cause an issue on some environments that don't require
CONFIG_VIDEO_V4L2_SUBDEV_API.Sakari, do you have any opinion?
I think the dependency is just fine. There are drivers that do not support MC (and V4L2 sub-device uAPI) but if a driver does, I don't see why it couldn't depend on the related Kconfig option.
Signature · 4 lines
Regards, Sakari Ailus [email protected]
Re: [PATCH v3 1/3] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
2019年1月28日(月) 17:34 Sakari Ailus <[email protected]>:
Quoted 58 lines
Hi Mita-san, Marco,
Quoted 54 lines · Akinobu Mita
2019年1月24日(木) 0:17 Marco Felsch <[email protected]>:
Quoted 46 lines
Hi Akinobu,
sorry for the delayed response.
Quoted 24 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 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]>---
* v3
- Set initial try format with default configuration instead of current one.drivers/media/i2c/mt9m111.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)drivers/media/i2c/mt9m111.c +0
-0@@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,if (format->pad)drivers/media/i2c/mt9m111.c static int mt9m111_get_fmt(struct v4l2_subdev *sd, · line 538
Quoted 10 lines
drivers/media/i2c/mt9m111.c +8
-0return -EINVAL ; +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 ; +#else +return -ENOTTY ; +#endif If've checked this again and found the ov* devices do this too. IMO it's not good for other developers to check the upper layer if the '#else' path is reachable. There are also some code analyzer tools which will report this as issue/warning.
Quoted 3 lines
As I said, I checked the
v4l2_subdev_get_try_format()usage again and found the solution made by themt9v111.cbetter. Why do you don't add a dependency in theKconfig, so we can drop this ifdef?I'm ok with adding
CONFIG_VIDEO_V4L2_SUBDEV_APIdependency to this driver, because I always enable it.
Quoted 8 lines
Quoted 4 lines
But it may cause an issue on some environments that don't require
CONFIG_VIDEO_V4L2_SUBDEV_API.Sakari, do you have any opinion?
I think the dependency is just fine. There are drivers that do not support MC (and V4L2 sub-device uAPI) but if a driver does, I don't see why it couldn't depend on the related
Kconfigoption.
OK. I'll prepare a patch that adds the dependency and removes the ifdef. I made similar change for ov2640, so I'll do for mt9m111 and ov2640, respectively.
PATCH v3 2/3 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.
---
* No changes from v2
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 ; }
@@ -670,6 +673,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 v3 3/3 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.
---
* No changes from v2
drivers/media/i2c/mt9m111.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/media/i2c/mt9m111.c +2 -0
@@ -1310,6 +1310,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