[PATCH 0/4] media: bugfixes for mt9m111 driver

[PATCH 0/4] media: bugfixes for mt9m111 driver

[Akinobu Mita]

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(+)

Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: Marco Felsch <m.felsch@pengutronix.de>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
-- 
2.7.4

[PATCH 1/4] media: mt9m111: fix setting pixclk polarity

[Akinobu Mita]

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.

Fixes: 98480d65c48c ("media: mt9m111: allow to setup pixclk polarity")
Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: Marco Felsch <m.felsch@pengutronix.de>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/mt9m111.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index d639b9b..f0e47fd 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -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 |
-- 
2.7.4

[PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY

[Akinobu Mita]

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.

Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index f0e47fd..acb4dee 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -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,
-- 
2.7.4

[PATCH 3/4] media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls

[Akinobu Mita]

This 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.

Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/mt9m111.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index acb4dee..465e920 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -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;
-- 
2.7.4

[PATCH 4/4] media: mt9m111: set initial frame size other than 0x0

[Akinobu Mita]

This 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.

Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/mt9m111.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 465e920..638236f 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -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);
-- 
2.7.4

Re: [PATCH 1/4] media: mt9m111: fix setting pixclk polarity

[Marco Felsch]

Hi Akinobu,

On 18-12-30 02:07, Akinobu Mita wrote:
> 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.

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

> 
> Fixes: 98480d65c48c ("media: mt9m111: allow to setup pixclk polarity")
> Cc: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Cc: Marco Felsch <m.felsch@pengutronix.de>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/mt9m111.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index d639b9b..f0e47fd 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -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 |
> -- 
> 2.7.4
> 
> 

-- 
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 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY

[Marco Felsch]

On 18-12-30 02:07, Akinobu Mita wrote:
> 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.
> 
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index f0e47fd..acb4dee 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -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.

> +		mf = v4l2_subdev_get_try_format(sd, cfg, 0);

I would use format->pad instead of the static 0.

> +		format->format = *mf;

Is this correct? I tought v4l2_subdev_get_try_format() will return the
try_pad which we need to fill.

> +		return 0;
> +#else
> +		return -ENOTTY;

Return this error is not specified in the API-Doc.

> +#endif
> +	}
> +

If I understood it right, your patch should look like:

> +	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.

>  	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)

Is this related to the patch description? I would split this into a
seperate patch.

> +{
> +#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 1/4] media: mt9m111: fix setting pixclk polarity

[Akinobu Mita]

2018年12月31日(月) 19:30 Marco Felsch <m.felsch@pengutronix.de>:
>
> Hi Akinobu,
>
> On 18-12-30 02:07, Akinobu Mita wrote:
> > 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.
>
> 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?

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.

> Can you cc me the other patches too, so I can keep track of it easier?

OK.  I'll do from v2.

Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY

[Akinobu Mita]

2018年12月31日(月) 19:54 Marco Felsch <m.felsch@pengutronix.de>:
>
> On 18-12-30 02:07, Akinobu Mita wrote:
> > 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.
> >
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index f0e47fd..acb4dee 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -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.

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/linux-media@vger.kernel.org/msg137098.html
v2: https://www.mail-archive.com/linux-media@vger.kernel.org/msg141735.html

> > +             mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>
> I would use format->pad instead of the static 0.

OK.

> > +             format->format = *mf;
>
> Is this correct? I tought v4l2_subdev_get_try_format() will return the
> try_pad which we need to fill.

I think this is correct.  Other sensor drivers are doing the same thing in
get_fmt() callback.

> > +             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.

> > +#endif
> > +     }
> > +
>
> If I understood it right, your patch should look like:
>
> > +     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.
>
> >       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)
>
> 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.

> > +{
> > +#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

[Marco Felsch]

On 19-01-01 02:07, Akinobu Mita wrote:
> 2018年12月31日(月) 19:54 Marco Felsch <m.felsch@pengutronix.de>:
> >
> > On 18-12-30 02:07, Akinobu Mita wrote:
> > > 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.
> > >
> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index f0e47fd..acb4dee 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -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.
> 
> 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/linux-media@vger.kernel.org/msg137098.html
> v2: https://www.mail-archive.com/linux-media@vger.kernel.org/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

> 
> > > +             mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> >
> > I would use format->pad instead of the static 0.
> 
> OK.
> 
> > > +             format->format = *mf;
> >
> > Is this correct? I tought v4l2_subdev_get_try_format() will return the
> > try_pad which 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.

> > > +             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.

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?

> 
> > > +#endif
> > > +     }
> > > +
> >
> > If I understood it right, your patch should look like:
> >
> > > +     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.
> >
> > >       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)
> >
> > 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.

Okay, I would split this but it's my personal opinion.

Kind regards,
Marco

> > > +{
> > > +#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

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

Hello,
  sorry to jump in

On Thu, Jan 03, 2019 at 02:47:04PM +0100, Marco Felsch wrote:
> On 19-01-01 02:07, Akinobu Mita wrote:
> > 2018年12月31日(月) 19:54 Marco Felsch <m.felsch@pengutronix.de>:
> > >
> > > On 18-12-30 02:07, Akinobu Mita wrote:
> > > > 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.
> > > >
> > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > > ---
> > > >  drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index f0e47fd..acb4dee 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -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.
> >
> > 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/linux-media@vger.kernel.org/msg137098.html
> > v2: https://www.mail-archive.com/linux-media@vger.kernel.org/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
>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY

[Akinobu Mita]

2019年1月3日(木) 22:47 Marco Felsch <m.felsch@pengutronix.de>:
>
> On 19-01-01 02:07, Akinobu Mita wrote:
> > 2018年12月31日(月) 19:54 Marco Felsch <m.felsch@pengutronix.de>:
> > >
> > > On 18-12-30 02:07, Akinobu Mita wrote:
> > > > 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.
> > > >
> > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > > ---
> > > >  drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index f0e47fd..acb4dee 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -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.
> >
> > 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/linux-media@vger.kernel.org/msg137098.html
> > v2: https://www.mail-archive.com/linux-media@vger.kernel.org/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
>
> >
> > > > +             mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> > >
> > > I would use format->pad instead of the static 0.
> >
> > OK.
> >
> > > > +             format->format = *mf;
> > >
> > > Is this correct? I tought v4l2_subdev_get_try_format() will return the
> > > try_pad which 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.
>
> > > > +             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.
>
> 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?

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
        }