* [PATCH 0/4] media: bugfixes for mt9m111 driver
@ 2018-12-29 17:07 Akinobu Mita
2018-12-29 17:07 ` [PATCH 1/4] media: mt9m111: fix setting pixclk polarity Akinobu Mita
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Akinobu Mita @ 2018-12-29 17:07 UTC (permalink / raw)
To: linux-media
Cc: Akinobu Mita, Enrico Scholz, Michael Grzeschik, Marco Felsch,
Sakari Ailus, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/4] media: mt9m111: fix setting pixclk polarity 2018-12-29 17:07 [PATCH 0/4] media: bugfixes for mt9m111 driver Akinobu Mita @ 2018-12-29 17:07 ` Akinobu Mita 2018-12-31 10:30 ` Marco Felsch 2018-12-29 17:07 ` [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Akinobu Mita @ 2018-12-29 17:07 UTC (permalink / raw) To: linux-media Cc: Akinobu Mita, Enrico Scholz, Michael Grzeschik, Marco Felsch, Sakari Ailus, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] media: mt9m111: fix setting pixclk polarity 2018-12-29 17:07 ` [PATCH 1/4] media: mt9m111: fix setting pixclk polarity Akinobu Mita @ 2018-12-31 10:30 ` Marco Felsch 2018-12-31 16:57 ` Akinobu Mita 0 siblings, 1 reply; 12+ messages in thread From: Marco Felsch @ 2018-12-31 10:30 UTC (permalink / raw) To: Akinobu Mita Cc: linux-media, Enrico Scholz, Michael Grzeschik, Sakari Ailus, Mauro Carvalho Chehab 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 | ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] media: mt9m111: fix setting pixclk polarity 2018-12-31 10:30 ` Marco Felsch @ 2018-12-31 16:57 ` Akinobu Mita 0 siblings, 0 replies; 12+ messages in thread From: Akinobu Mita @ 2018-12-31 16:57 UTC (permalink / raw) To: Marco Felsch Cc: linux-media, Enrico Scholz, Michael Grzeschik, Sakari Ailus, Mauro Carvalho Chehab 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2018-12-29 17:07 [PATCH 0/4] media: bugfixes for mt9m111 driver Akinobu Mita 2018-12-29 17:07 ` [PATCH 1/4] media: mt9m111: fix setting pixclk polarity Akinobu Mita @ 2018-12-29 17:07 ` Akinobu Mita 2018-12-31 10:54 ` Marco Felsch 2018-12-29 17:07 ` [PATCH 3/4] media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita 2018-12-29 17:07 ` [PATCH 4/4] media: mt9m111: set initial frame size other than 0x0 Akinobu Mita 3 siblings, 1 reply; 12+ messages in thread From: Akinobu Mita @ 2018-12-29 17:07 UTC (permalink / raw) To: linux-media; +Cc: Akinobu Mita, Sakari Ailus, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2018-12-29 17:07 ` [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita @ 2018-12-31 10:54 ` Marco Felsch 2018-12-31 17:07 ` Akinobu Mita 0 siblings, 1 reply; 12+ messages in thread From: Marco Felsch @ 2018-12-31 10:54 UTC (permalink / raw) To: Akinobu Mita; +Cc: linux-media, Sakari Ailus, Mauro Carvalho Chehab 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2018-12-31 10:54 ` Marco Felsch @ 2018-12-31 17:07 ` Akinobu Mita 2019-01-03 13:47 ` Marco Felsch 0 siblings, 1 reply; 12+ messages in thread From: Akinobu Mita @ 2018-12-31 17:07 UTC (permalink / raw) To: Marco Felsch; +Cc: linux-media, Sakari Ailus, Mauro Carvalho Chehab 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 > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2018-12-31 17:07 ` Akinobu Mita @ 2019-01-03 13:47 ` Marco Felsch 2019-01-04 13:35 ` Jacopo Mondi 2019-01-05 14:52 ` Akinobu Mita 0 siblings, 2 replies; 12+ messages in thread From: Marco Felsch @ 2019-01-03 13:47 UTC (permalink / raw) To: Akinobu Mita; +Cc: linux-media, Sakari Ailus, Mauro Carvalho Chehab 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 > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2019-01-03 13:47 ` Marco Felsch @ 2019-01-04 13:35 ` Jacopo Mondi 2019-01-05 14:52 ` Akinobu Mita 1 sibling, 0 replies; 12+ messages in thread From: Jacopo Mondi @ 2019-01-04 13:35 UTC (permalink / raw) To: Marco Felsch Cc: Akinobu Mita, linux-media, Sakari Ailus, Mauro Carvalho Chehab [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY 2019-01-03 13:47 ` Marco Felsch 2019-01-04 13:35 ` Jacopo Mondi @ 2019-01-05 14:52 ` Akinobu Mita 1 sibling, 0 replies; 12+ messages in thread From: Akinobu Mita @ 2019-01-05 14:52 UTC (permalink / raw) To: Marco Felsch; +Cc: linux-media, Sakari Ailus, Mauro Carvalho Chehab 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 } ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls 2018-12-29 17:07 [PATCH 0/4] media: bugfixes for mt9m111 driver Akinobu Mita 2018-12-29 17:07 ` [PATCH 1/4] media: mt9m111: fix setting pixclk polarity Akinobu Mita 2018-12-29 17:07 ` [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita @ 2018-12-29 17:07 ` Akinobu Mita 2018-12-29 17:07 ` [PATCH 4/4] media: mt9m111: set initial frame size other than 0x0 Akinobu Mita 3 siblings, 0 replies; 12+ messages in thread From: Akinobu Mita @ 2018-12-29 17:07 UTC (permalink / raw) To: linux-media; +Cc: Akinobu Mita, Sakari Ailus, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] media: mt9m111: set initial frame size other than 0x0 2018-12-29 17:07 [PATCH 0/4] media: bugfixes for mt9m111 driver Akinobu Mita ` (2 preceding siblings ...) 2018-12-29 17:07 ` [PATCH 3/4] media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita @ 2018-12-29 17:07 ` Akinobu Mita 3 siblings, 0 replies; 12+ messages in thread From: Akinobu Mita @ 2018-12-29 17:07 UTC (permalink / raw) To: linux-media; +Cc: Akinobu Mita, Sakari Ailus, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-01-05 14:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-29 17:07 [PATCH 0/4] media: bugfixes for mt9m111 driver Akinobu Mita 2018-12-29 17:07 ` [PATCH 1/4] media: mt9m111: fix setting pixclk polarity Akinobu Mita 2018-12-31 10:30 ` Marco Felsch 2018-12-31 16:57 ` Akinobu Mita 2018-12-29 17:07 ` [PATCH 2/4] media: mt9m111: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita 2018-12-31 10:54 ` Marco Felsch 2018-12-31 17:07 ` Akinobu Mita 2019-01-03 13:47 ` Marco Felsch 2019-01-04 13:35 ` Jacopo Mondi 2019-01-05 14:52 ` Akinobu Mita 2018-12-29 17:07 ` [PATCH 3/4] media: mt9m111: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita 2018-12-29 17:07 ` [PATCH 4/4] media: mt9m111: set initial frame size other than 0x0 Akinobu Mita
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.