[PATCH 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats

[PATCH 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats

[Stefan Riedmueller]

From: Christian Hemp <c.hemp@phytec.de>

Aside from 12 bit monochrome or color format the sensor implicitly
supports 10 and 8 bit formats as well by simply dropping the
corresponding LSBs.

Signed-off-by: Christian Hemp <c.hemp@phytec.de>
[jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index dc23b9ed510a..0002dd299ffa 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -116,6 +116,18 @@ enum mt9p031_model {
 	MT9P031_MODEL_MONOCHROME,
 };
 
+static const u32 mt9p031_color_fmts[] = {
+	MEDIA_BUS_FMT_SGRBG8_1X8,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGRBG12_1X12,
+};
+
+static const u32 mt9p031_monochrome_fmts[] = {
+	MEDIA_BUS_FMT_Y8_1X8,
+	MEDIA_BUS_FMT_Y10_1X10,
+	MEDIA_BUS_FMT_Y12_1X12,
+};
+
 struct mt9p031 {
 	struct v4l2_subdev subdev;
 	struct media_pad pad;
@@ -138,6 +150,9 @@ struct mt9p031 {
 	struct v4l2_ctrl *blc_auto;
 	struct v4l2_ctrl *blc_offset;
 
+	const u32 *fmts;
+	int num_fmts;
+
 	/* Registers cache */
 	u16 output_control;
 	u16 mode2;
@@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
 	return container_of(sd, struct mt9p031, subdev);
 }
 
+static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
+{
+	int i;
+
+	for (i = 0; i < mt9p031->num_fmts; i++)
+		if (mt9p031->fmts[i] == code)
+			return mt9p031->fmts[i];
+
+	return mt9p031->fmts[mt9p031->num_fmts-1];
+}
+
 static int mt9p031_read(struct i2c_client *client, u8 reg)
 {
 	return i2c_smbus_read_word_swapped(client, reg);
@@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
 {
 	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
 
-	if (code->pad || code->index)
+	if (code->pad || code->index >= mt9p031->num_fmts)
 		return -EINVAL;
 
-	code->code = mt9p031->format.code;
+	code->code = mt9p031->fmts[code->index];
+
 	return 0;
 }
 
@@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
 	__format->width = __crop->width / hratio;
 	__format->height = __crop->height / vratio;
 
+	__format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
+
 	format->format = *__format;
 
 	return 0;
@@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
 
 	format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
 
-	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
-		format->code = MEDIA_BUS_FMT_Y12_1X12;
-	else
-		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
+	format->code = mt9p031_find_datafmt(mt9p031, 0);
 
 	format->width = MT9P031_WINDOW_WIDTH_DEF;
 	format->height = MT9P031_WINDOW_HEIGHT_DEF;
@@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
 	mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
 	mt9p031->crop.top = MT9P031_ROW_START_DEF;
 
-	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
-		mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
-	else
-		mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
+	if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
+		mt9p031->fmts = mt9p031_monochrome_fmts;
+		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
+	} else {
+		mt9p031->fmts = mt9p031_color_fmts;
+		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
+	}
+	mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
 
 	mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
 	mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
-- 
2.25.1

[PATCH 2/5] media: mt9p031: Read back the real clock rate

[Stefan Riedmueller]

From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

The real and requested clock can differ and because it is used to
calculate PLL values, the real clock rate should be read.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/mt9p031.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 0002dd299ffa..ce192be4531f 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -255,6 +255,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
 
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
 	struct mt9p031_platform_data *pdata = mt9p031->pdata;
+	unsigned long ext_freq;
 	int ret;
 
 	mt9p031->clk = devm_clk_get(&client->dev, NULL);
@@ -265,13 +266,15 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
 	if (ret < 0)
 		return ret;
 
+	ext_freq = clk_get_rate(mt9p031->clk);
+
 	/* If the external clock frequency is out of bounds for the PLL use the
 	 * pixel clock divider only and disable the PLL.
 	 */
-	if (pdata->ext_freq > limits.ext_clock_max) {
+	if (ext_freq > limits.ext_clock_max) {
 		unsigned int div;
 
-		div = DIV_ROUND_UP(pdata->ext_freq, pdata->target_freq);
+		div = DIV_ROUND_UP(ext_freq, pdata->target_freq);
 		div = roundup_pow_of_two(div) / 2;
 
 		mt9p031->clk_div = min_t(unsigned int, div, 64);
@@ -280,7 +283,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
 		return 0;
 	}
 
-	mt9p031->pll.ext_clock = pdata->ext_freq;
+	mt9p031->pll.ext_clock = ext_freq;
 	mt9p031->pll.pix_clock = pdata->target_freq;
 	mt9p031->use_pll = true;
 
-- 
2.25.1

[PATCH 3/5] media: mt9p031: Implement [gs]_register debug calls

[Stefan Riedmueller]

From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

Implement g_register and s_register v4l2_subdev_core_ops to access
camera register directly from userspace for debug purposes.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index ce192be4531f..f5d6a7890c47 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
 	return 0;
 }
 
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9p031_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret;
+
+	ret = mt9p031_read(client, reg->reg);
+	if (ret < 0)
+		return ret;
+
+	reg->val = ret;
+	return 0;
+}
+
+static int mt9p031_s_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register const *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return mt9p031_write(client, reg->reg, reg->val);
+}
+#endif
+
 static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9p031 *mt9p031 =
@@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
 
 static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
 	.s_power        = mt9p031_set_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.s_register	= mt9p031_s_register,
+	.g_register	= mt9p031_g_register,
+#endif
 };
 
 static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
-- 
2.25.1

[PATCH 4/5] media: mt9p031: Make pixel clock polarity configurable by DT

[Stefan Riedmueller]

From: Christian Hemp <c.hemp@phytec.de>

Evaluate the desired pixel clock polarity from the device tree.

Signed-off-by: Christian Hemp <c.hemp@phytec.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/Kconfig   |  1 +
 drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
 include/media/i2c/mt9p031.h |  1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c7ba76fee599..7c026daeacf0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
 	select MEDIA_CONTROLLER
 	select VIDEO_V4L2_SUBDEV_API
 	select VIDEO_APTINA_PLL
+	select V4L2_FWNODE
 	help
 	  This is a Video4Linux2 sensor driver for the Aptina
 	  (Micron) mt9p031 5 Mpixel camera.
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index f5d6a7890c47..8f8ee37a2dd2 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -27,6 +27,7 @@
 #include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
 #include "aptina-pll.h"
@@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
 		return ret;
 	}
 
+	/* Configure the pixel clock polarity */
+	if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
+		ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
+				MT9P031_PIXEL_CLOCK_INVERT);
+		if (ret < 0)
+			return ret;
+	}
+
 	return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
 }
 
@@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
 static struct mt9p031_platform_data *
 mt9p031_get_pdata(struct i2c_client *client)
 {
-	struct mt9p031_platform_data *pdata;
+	struct mt9p031_platform_data *pdata = NULL;
+	struct v4l2_fwnode_endpoint endpoint;
 	struct device_node *np;
 
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
@@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
 	if (!np)
 		return NULL;
 
+	endpoint.bus_type = V4L2_MBUS_UNKNOWN;
+	if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
+		goto done;
+
 	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		goto done;
@@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
 	of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
 	of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
 
+	pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
+			       V4L2_MBUS_PCLK_SAMPLE_RISING);
+
 done:
 	of_node_put(np);
 	return pdata;
diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
index 7c29c53aa988..f933cd0be8e5 100644
--- a/include/media/i2c/mt9p031.h
+++ b/include/media/i2c/mt9p031.h
@@ -10,6 +10,7 @@ struct v4l2_subdev;
  * @target_freq: Pixel clock frequency
  */
 struct mt9p031_platform_data {
+	unsigned int pixclk_pol:1;
 	int ext_freq;
 	int target_freq;
 };
-- 
2.25.1

[PATCH 5/5] media: mt9p031: Fix corrupted frame after restarting stream

[Stefan Riedmueller]

From: Dirk Bender <d.bender@phytec.de>

To prevent corrupted frames after starting and stopping the sensor it's
datasheet specifies a specific pause sequence to follow:

Stopping:
	Set Pause_Restart Bit -> Set Restart Bit -> Set Chip_Enable Off

Restarting:
	Set Chip_Enable On -> Clear Pause_Restart Bit

The Restart Bit is cleared automatically and must not be cleared
manually as this would cause undefined behavior.

Signed-off-by: Dirk Bender <d.bender@phytec.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 8f8ee37a2dd2..2f2daf95dcd3 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -80,6 +80,8 @@
 #define		MT9P031_PIXEL_CLOCK_SHIFT(n)		((n) << 8)
 #define		MT9P031_PIXEL_CLOCK_DIVIDE(n)		((n) << 0)
 #define MT9P031_FRAME_RESTART				0x0b
+#define		MT9P031_FRAME_RESTART_SET		(1 << 0)
+#define		MT9P031_FRAME_PAUSE_RESTART_SET		(1 << 1)
 #define MT9P031_SHUTTER_DELAY				0x0c
 #define MT9P031_RST					0x0d
 #define		MT9P031_RST_ENABLE			1
@@ -483,9 +485,25 @@ static int mt9p031_set_params(struct mt9p031 *mt9p031)
 static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
+	int val;
 	int ret;
 
 	if (!enable) {
+		val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+
+		/* enable pause restart */
+		val |= MT9P031_FRAME_PAUSE_RESTART_SET;
+		ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+		if (ret < 0)
+			return ret;
+
+		/* enable restart + keep pause restart set */
+		val |= MT9P031_FRAME_RESTART_SET;
+		ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+		if (ret < 0)
+			return ret;
+
 		/* Stop sensor readout */
 		ret = mt9p031_set_output_control(mt9p031,
 						 MT9P031_OUTPUT_CONTROL_CEN, 0);
@@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
 	if (ret < 0)
 		return ret;
 
+	val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+	/* disable reset + pause restart */
+	val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
+	ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+	if (ret < 0)
+		return ret;
+
 	return mt9p031_pll_enable(mt9p031);
 }
 
-- 
2.25.1

Re: [PATCH 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats

[kernel test robot]

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

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.9-rc6 next-20200924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Stefan-Riedmueller/media-mt9p031-Add-support-for-8-bit-and-10-bit-formats/20200925-155251
base:   git://linuxtv.org/media_tree.git master
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1863bce6d7b4791c048a25ec07ff8aec8b4847cf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Stefan-Riedmueller/media-mt9p031-Add-support-for-8-bit-and-10-bit-formats/20200925-155251
        git checkout 1863bce6d7b4791c048a25ec07ff8aec8b4847cf
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/mt9p031.c:166:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
     166 | static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
         |        ^~~~~

vim +166 drivers/media/i2c/mt9p031.c

   165	
 > 166	static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
   167	{
   168		int i;
   169	
   170		for (i = 0; i < mt9p031->num_fmts; i++)
   171			if (mt9p031->fmts[i] == code)
   172				return mt9p031->fmts[i];
   173	
   174		return mt9p031->fmts[mt9p031->num_fmts-1];
   175	}
   176	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65321 bytes --]

Re: [PATCH 4/5] media: mt9p031: Make pixel clock polarity configurable by DT

[Sakari Ailus]

Hi Stefan,

Thanks for the patchset.

On Fri, Sep 25, 2020 at 09:50:28AM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <c.hemp@phytec.de>
> 
> Evaluate the desired pixel clock polarity from the device tree.
> 
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
>  drivers/media/i2c/Kconfig   |  1 +
>  drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
>  include/media/i2c/mt9p031.h |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c7ba76fee599..7c026daeacf0 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select VIDEO_APTINA_PLL
> +	select V4L2_FWNODE
>  	help
>  	  This is a Video4Linux2 sensor driver for the Aptina
>  	  (Micron) mt9p031 5 Mpixel camera.
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index f5d6a7890c47..8f8ee37a2dd2 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -27,6 +27,7 @@
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
>  #include "aptina-pll.h"
> @@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
>  		return ret;
>  	}
>  
> +	/* Configure the pixel clock polarity */
> +	if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
> +		ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
> +				MT9P031_PIXEL_CLOCK_INVERT);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
>  }
>  
> @@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
>  static struct mt9p031_platform_data *
>  mt9p031_get_pdata(struct i2c_client *client)
>  {
> -	struct mt9p031_platform_data *pdata;
> +	struct mt9p031_platform_data *pdata = NULL;
> +	struct v4l2_fwnode_endpoint endpoint;

Could you initialise the bus_type field to a valid value? I suppose this
sensor only supports one of them? That way you'll also initialise the rest
of the struct fields to zero.

>  	struct device_node *np;
>  
>  	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> @@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
>  	if (!np)
>  		return NULL;
>  
> +	endpoint.bus_type = V4L2_MBUS_UNKNOWN;
> +	if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
> +		goto done;
> +
>  	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		goto done;
> @@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
>  	of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
>  	of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
>  
> +	pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
> +			       V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
>  done:
>  	of_node_put(np);
>  	return pdata;
> diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
> index 7c29c53aa988..f933cd0be8e5 100644
> --- a/include/media/i2c/mt9p031.h
> +++ b/include/media/i2c/mt9p031.h
> @@ -10,6 +10,7 @@ struct v4l2_subdev;
>   * @target_freq: Pixel clock frequency
>   */
>  struct mt9p031_platform_data {
> +	unsigned int pixclk_pol:1;
>  	int ext_freq;
>  	int target_freq;
>  };

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats

[Sakari Ailus]

Hi Stefan,

On Fri, Sep 25, 2020 at 09:50:25AM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <c.hemp@phytec.de>
> 
> Aside from 12 bit monochrome or color format the sensor implicitly
> supports 10 and 8 bit formats as well by simply dropping the
> corresponding LSBs.
> 
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
>  drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index dc23b9ed510a..0002dd299ffa 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -116,6 +116,18 @@ enum mt9p031_model {
>  	MT9P031_MODEL_MONOCHROME,
>  };
>  
> +static const u32 mt9p031_color_fmts[] = {
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +};
> +
> +static const u32 mt9p031_monochrome_fmts[] = {
> +	MEDIA_BUS_FMT_Y8_1X8,
> +	MEDIA_BUS_FMT_Y10_1X10,
> +	MEDIA_BUS_FMT_Y12_1X12,
> +};
> +
>  struct mt9p031 {
>  	struct v4l2_subdev subdev;
>  	struct media_pad pad;
> @@ -138,6 +150,9 @@ struct mt9p031 {
>  	struct v4l2_ctrl *blc_auto;
>  	struct v4l2_ctrl *blc_offset;
>  
> +	const u32 *fmts;
> +	int num_fmts;

Unsigned int, please.

> +
>  	/* Registers cache */
>  	u16 output_control;
>  	u16 mode2;
> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
>  	return container_of(sd, struct mt9p031, subdev);
>  }
>  
> +static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
> +{
> +	int i;

Same here.

> +
> +	for (i = 0; i < mt9p031->num_fmts; i++)
> +		if (mt9p031->fmts[i] == code)
> +			return mt9p031->fmts[i];
> +
> +	return mt9p031->fmts[mt9p031->num_fmts-1];
> +}
> +
>  static int mt9p031_read(struct i2c_client *client, u8 reg)
>  {
>  	return i2c_smbus_read_word_swapped(client, reg);
> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
>  {
>  	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>  
> -	if (code->pad || code->index)
> +	if (code->pad || code->index >= mt9p031->num_fmts)
>  		return -EINVAL;
>  
> -	code->code = mt9p031->format.code;
> +	code->code = mt9p031->fmts[code->index];
> +
>  	return 0;
>  }
>  
> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>  	__format->width = __crop->width / hratio;
>  	__format->height = __crop->height / vratio;
>  
> +	__format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
> +
>  	format->format = *__format;
>  
>  	return 0;
> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>  
>  	format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>  
> -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> -		format->code = MEDIA_BUS_FMT_Y12_1X12;
> -	else
> -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> +	format->code = mt9p031_find_datafmt(mt9p031, 0);
>  
>  	format->width = MT9P031_WINDOW_WIDTH_DEF;
>  	format->height = MT9P031_WINDOW_HEIGHT_DEF;
> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
>  	mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
>  	mt9p031->crop.top = MT9P031_ROW_START_DEF;
>  
> -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> -		mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
> -	else
> -		mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
> +	if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
> +		mt9p031->fmts = mt9p031_monochrome_fmts;
> +		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
> +	} else {
> +		mt9p031->fmts = mt9p031_color_fmts;
> +		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
> +	}
> +	mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>  
>  	mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
>  	mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 4/5] media: mt9p031: Make pixel clock polarity configurable by DT

[Stefan Riedmüller]

Hi Sakari,

thanks for your review.

On 25.09.20 22:07, Sakari Ailus wrote:
> Hi Stefan,
> 
> Thanks for the patchset.
> 
> On Fri, Sep 25, 2020 at 09:50:28AM +0200, Stefan Riedmueller wrote:
>> From: Christian Hemp <c.hemp@phytec.de>
>>
>> Evaluate the desired pixel clock polarity from the device tree.
>>
>> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>>   drivers/media/i2c/Kconfig   |  1 +
>>   drivers/media/i2c/mt9p031.c | 19 ++++++++++++++++++-
>>   include/media/i2c/mt9p031.h |  1 +
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index c7ba76fee599..7c026daeacf0 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1103,6 +1103,7 @@ config VIDEO_MT9P031
>>   	select MEDIA_CONTROLLER
>>   	select VIDEO_V4L2_SUBDEV_API
>>   	select VIDEO_APTINA_PLL
>> +	select V4L2_FWNODE
>>   	help
>>   	  This is a Video4Linux2 sensor driver for the Aptina
>>   	  (Micron) mt9p031 5 Mpixel camera.
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index f5d6a7890c47..8f8ee37a2dd2 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -27,6 +27,7 @@
>>   #include <media/v4l2-async.h>
>>   #include <media/v4l2-ctrls.h>
>>   #include <media/v4l2-device.h>
>> +#include <media/v4l2-fwnode.h>
>>   #include <media/v4l2-subdev.h>
>>   
>>   #include "aptina-pll.h"
>> @@ -399,6 +400,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
>>   		return ret;
>>   	}
>>   
>> +	/* Configure the pixel clock polarity */
>> +	if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
>> +		ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
>> +				MT9P031_PIXEL_CLOCK_INVERT);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>   	return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
>>   }
>>   
>> @@ -1062,7 +1071,8 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
>>   static struct mt9p031_platform_data *
>>   mt9p031_get_pdata(struct i2c_client *client)
>>   {
>> -	struct mt9p031_platform_data *pdata;
>> +	struct mt9p031_platform_data *pdata = NULL;
>> +	struct v4l2_fwnode_endpoint endpoint;
> 
> Could you initialise the bus_type field to a valid value? I suppose this
> sensor only supports one of them? That way you'll also initialise the rest
> of the struct fields to zero.

Yes, I'll do that and send a v2.

Thanks,
Stefan

> 
>>   	struct device_node *np;
>>   
>>   	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
>> @@ -1072,6 +1082,10 @@ mt9p031_get_pdata(struct i2c_client *client)
>>   	if (!np)
>>   		return NULL;
>>   
>> +	endpoint.bus_type = V4L2_MBUS_UNKNOWN;
>> +	if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
>> +		goto done;
>> +
>>   	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   	if (!pdata)
>>   		goto done;
>> @@ -1079,6 +1093,9 @@ mt9p031_get_pdata(struct i2c_client *client)
>>   	of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
>>   	of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
>>   
>> +	pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
>> +			       V4L2_MBUS_PCLK_SAMPLE_RISING);
>> +
>>   done:
>>   	of_node_put(np);
>>   	return pdata;
>> diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
>> index 7c29c53aa988..f933cd0be8e5 100644
>> --- a/include/media/i2c/mt9p031.h
>> +++ b/include/media/i2c/mt9p031.h
>> @@ -10,6 +10,7 @@ struct v4l2_subdev;
>>    * @target_freq: Pixel clock frequency
>>    */
>>   struct mt9p031_platform_data {
>> +	unsigned int pixclk_pol:1;
>>   	int ext_freq;
>>   	int target_freq;
>>   };
> 

Re: [PATCH 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats

[Stefan Riedmüller]

Hi Sakari,

On 25.09.20 22:11, Sakari Ailus wrote:
> Hi Stefan,
> 
> On Fri, Sep 25, 2020 at 09:50:25AM +0200, Stefan Riedmueller wrote:
>> From: Christian Hemp <c.hemp@phytec.de>
>>
>> Aside from 12 bit monochrome or color format the sensor implicitly
>> supports 10 and 8 bit formats as well by simply dropping the
>> corresponding LSBs.
>>
>> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
>> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>>   drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
>>   1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index dc23b9ed510a..0002dd299ffa 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -116,6 +116,18 @@ enum mt9p031_model {
>>   	MT9P031_MODEL_MONOCHROME,
>>   };
>>   
>> +static const u32 mt9p031_color_fmts[] = {
>> +	MEDIA_BUS_FMT_SGRBG8_1X8,
>> +	MEDIA_BUS_FMT_SGRBG10_1X10,
>> +	MEDIA_BUS_FMT_SGRBG12_1X12,
>> +};
>> +
>> +static const u32 mt9p031_monochrome_fmts[] = {
>> +	MEDIA_BUS_FMT_Y8_1X8,
>> +	MEDIA_BUS_FMT_Y10_1X10,
>> +	MEDIA_BUS_FMT_Y12_1X12,
>> +};
>> +
>>   struct mt9p031 {
>>   	struct v4l2_subdev subdev;
>>   	struct media_pad pad;
>> @@ -138,6 +150,9 @@ struct mt9p031 {
>>   	struct v4l2_ctrl *blc_auto;
>>   	struct v4l2_ctrl *blc_offset;
>>   
>> +	const u32 *fmts;
>> +	int num_fmts;
> 
> Unsigned int, please.
> 
>> +
>>   	/* Registers cache */
>>   	u16 output_control;
>>   	u16 mode2;
>> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
>>   	return container_of(sd, struct mt9p031, subdev);
>>   }
>>   
>> +static const u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
>> +{
>> +	int i;
> 
> Same here.

I'll fix it in v2.

Thanks,
Stefan

> 
>> +
>> +	for (i = 0; i < mt9p031->num_fmts; i++)
>> +		if (mt9p031->fmts[i] == code)
>> +			return mt9p031->fmts[i];
>> +
>> +	return mt9p031->fmts[mt9p031->num_fmts-1];
>> +}
>> +
>>   static int mt9p031_read(struct i2c_client *client, u8 reg)
>>   {
>>   	return i2c_smbus_read_word_swapped(client, reg);
>> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
>>   {
>>   	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>>   
>> -	if (code->pad || code->index)
>> +	if (code->pad || code->index >= mt9p031->num_fmts)
>>   		return -EINVAL;
>>   
>> -	code->code = mt9p031->format.code;
>> +	code->code = mt9p031->fmts[code->index];
>> +
>>   	return 0;
>>   }
>>   
>> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>>   	__format->width = __crop->width / hratio;
>>   	__format->height = __crop->height / vratio;
>>   
>> +	__format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
>> +
>>   	format->format = *__format;
>>   
>>   	return 0;
>> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>   
>>   	format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>>   
>> -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> -		format->code = MEDIA_BUS_FMT_Y12_1X12;
>> -	else
>> -		format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> +	format->code = mt9p031_find_datafmt(mt9p031, 0);
>>   
>>   	format->width = MT9P031_WINDOW_WIDTH_DEF;
>>   	format->height = MT9P031_WINDOW_HEIGHT_DEF;
>> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
>>   	mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
>>   	mt9p031->crop.top = MT9P031_ROW_START_DEF;
>>   
>> -	if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> -		mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
>> -	else
>> -		mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> +	if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
>> +		mt9p031->fmts = mt9p031_monochrome_fmts;
>> +		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
>> +	} else {
>> +		mt9p031->fmts = mt9p031_color_fmts;
>> +		mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
>> +	}
>> +	mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>>   
>>   	mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
>>   	mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
>