[PATCH v3 0/6] media: mt9p031: Read back the real clock rate

[PATCH v3 0/6] media: mt9p031: Read back the real clock rate

[Stefan Riedmueller]

Hi,

sorry it took me some time but this is the v3 for my previously send
patchstack:
https://lore.kernel.org/linux-media/20200930105133.139981-1-s.riedmueller@phytec.de/

Changes in v3:
 - Dropped 1/5 media: mt9p031: Add support for 8 bit and 10 bit formats
 - Dropped 3/5 media: mt9p031: Implement [gs]_register debug calls
 - Added reviewed-by from Laurent Pinchart to
   media: mt9p031: Read back the real clock rate
 - Dropped unnecessary register reads in
   media: mt9p031: Fix corrupted frame after restarting
 - Changed sorting of register bits from MSB to LSB
 - Added patch to switch to BIT macro
 - Added two additional dt-bindings patches to add missing properties
   documentation

Christian Hemp (1):
  media: mt9p031: Make pixel clock polarity configurable by DT

Dirk Bender (1):
  media: mt9p031: Fix corrupted frame after restarting stream

Enrico Scholz (1):
  media: mt9p031: Read back the real clock rate

Stefan Riedmueller (3):
  dt-bindings: media: mt9p031: Add missing required properties
  dt-bindings: media: mt9p031: add pclk-sample property
  media: mt9p031: Use BIT macro

 .../devicetree/bindings/media/i2c/mt9p031.txt | 17 +++++
 drivers/media/i2c/Kconfig                     |  1 +
 drivers/media/i2c/mt9p031.c                   | 71 +++++++++++++++----
 include/media/i2c/mt9p031.h                   |  1 +
 4 files changed, 78 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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 77567341ec98..3eaaa8d44523 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

Re: [PATCH v3 1/6] media: mt9p031: Read back the real clock rate

[Sakari Ailus]

Hi Enrico,

On Fri, Jul 02, 2021 at 11:59:17AM +0200, Stefan Riedmueller wrote:
> 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.

Do you have a system where this happens? That suggests there's a wrong
value in DT.

The preference nowadays is to rely on assigned-clock-rates, even though
it's inherently somewhat unreliable, just as clk_set_rate(). This is an
existing driver though. The old ones could be kept for compatibility with
older DT binaries.

> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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 77567341ec98..3eaaa8d44523 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;
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 1/6] media: mt9p031: Read back the real clock rate

[Stefan Riedmüller]

Hi Sakari,

On Mon, 2021-07-05 at 10:13 +0300, Sakari Ailus wrote:
> Hi Enrico,
> 
> On Fri, Jul 02, 2021 at 11:59:17AM +0200, Stefan Riedmueller wrote:
> > 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.
> 
> Do you have a system where this happens? That suggests there's a wrong
> value in DT.

The use case here is when the clock is supplied by one of the clock outputs of
a SOC which might not hit the requested frequency exactly due to internal PLL
configuration. So to get a better pixel clock the actual clock rate is read to
calculate the PLL parameters on the sensor. At least that's the idea.

Regards,
Stefan

> 
> The preference nowadays is to rely on assigned-clock-rates, even though
> it's inherently somewhat unreliable, just as clk_set_rate(). This is an
> existing driver though. The old ones could be kept for compatibility with
> older DT binaries.
> 
> > Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  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 77567341ec98..3eaaa8d44523 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;
> >  

[PATCH v3 2/6] 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 | 20 +++++++++++++++++++-
 include/media/i2c/mt9p031.h |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 588f8eb95984..1f9e98be8066 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1187,6 +1187,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 3eaaa8d44523..6a6f16df3f4a 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"
@@ -398,6 +399,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);
 }
 
@@ -1040,8 +1049,11 @@ 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 device_node *np;
+	struct v4l2_fwnode_endpoint endpoint = {
+		.bus_type = V4L2_MBUS_PARALLEL
+	};
 
 	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
 		return client->dev.platform_data;
@@ -1050,6 +1062,9 @@ mt9p031_get_pdata(struct i2c_client *client)
 	if (!np)
 		return NULL;
 
+	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;
@@ -1057,6 +1072,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 v3 3/6] dt-bindings: media: mt9p031: Add missing required properties

[Stefan Riedmueller]

Add missing required clocks and supply regulator properties for the
sensor input clock and vdd, vdd_io and vaa supply regulators.

Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 .../devicetree/bindings/media/i2c/mt9p031.txt        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
index cb60443ff78f..4437d0e3147d 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
@@ -9,6 +9,12 @@ Required Properties:
 	(a) "aptina,mt9p031" for mt9p031 sensor
 	(b) "aptina,mt9p031m" for mt9p031m sensor
 
+- clocks: Reference to the sensor input clock
+
+- vdd-supply: VDD supply regulator
+- vdd_io-supply: VDD_IO supply regulator
+- vaa-supply: VAA supply regulator
+
 - input-clock-frequency: Input clock frequency.
 
 - pixel-clock-frequency: Pixel clock frequency.
@@ -29,6 +35,12 @@ Example:
 			reg = <0x5d>;
 			reset-gpios = <&gpio3 30 0>;
 
+			clocks = <&sensor_clk>;
+
+			vdd-supply = <&reg_vdd>;
+			vdd_io-supply = <&reg_vdd_io>;
+			vaa-supply = <&reg_vaa>;
+
 			port {
 				mt9p031_1: endpoint {
 					input-clock-frequency = <6000000>;
-- 
2.25.1

Re: [PATCH v3 3/6] dt-bindings: media: mt9p031: Add missing required properties

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Fri, Jul 02, 2021 at 11:59:19AM +0200, Stefan Riedmueller wrote:
> Add missing required clocks and supply regulator properties for the
> sensor input clock and vdd, vdd_io and vaa supply regulators.

Can I volunteer you to convert these bindings to YAML first, and add the
properties on top ? :-)

> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
>  .../devicetree/bindings/media/i2c/mt9p031.txt        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> index cb60443ff78f..4437d0e3147d 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> @@ -9,6 +9,12 @@ Required Properties:
>  	(a) "aptina,mt9p031" for mt9p031 sensor
>  	(b) "aptina,mt9p031m" for mt9p031m sensor
>  
> +- clocks: Reference to the sensor input clock
> +
> +- vdd-supply: VDD supply regulator
> +- vdd_io-supply: VDD_IO supply regulator
> +- vaa-supply: VAA supply regulator
> +
>  - input-clock-frequency: Input clock frequency.
>  
>  - pixel-clock-frequency: Pixel clock frequency.
> @@ -29,6 +35,12 @@ Example:
>  			reg = <0x5d>;
>  			reset-gpios = <&gpio3 30 0>;
>  
> +			clocks = <&sensor_clk>;
> +
> +			vdd-supply = <&reg_vdd>;
> +			vdd_io-supply = <&reg_vdd_io>;
> +			vaa-supply = <&reg_vaa>;
> +
>  			port {
>  				mt9p031_1: endpoint {
>  					input-clock-frequency = <6000000>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/6] dt-bindings: media: mt9p031: Add missing required properties

[Stefan Riedmüller]

Hi Laurent,

On Fri, 2021-07-02 at 16:15 +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Jul 02, 2021 at 11:59:19AM +0200, Stefan Riedmueller wrote:
> > Add missing required clocks and supply regulator properties for the
> > sensor input clock and vdd, vdd_io and vaa supply regulators.
> 
> Can I volunteer you to convert these bindings to YAML first, and add the
> properties on top ? :-)

Sure, I can give it a try :-)

Regards,
Stefan

> 
> > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> > ---
> >  .../devicetree/bindings/media/i2c/mt9p031.txt        | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > index cb60443ff78f..4437d0e3147d 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > @@ -9,6 +9,12 @@ Required Properties:
> >  	(a) "aptina,mt9p031" for mt9p031 sensor
> >  	(b) "aptina,mt9p031m" for mt9p031m sensor
> >  
> > +- clocks: Reference to the sensor input clock
> > +
> > +- vdd-supply: VDD supply regulator
> > +- vdd_io-supply: VDD_IO supply regulator
> > +- vaa-supply: VAA supply regulator
> > +
> >  - input-clock-frequency: Input clock frequency.
> >  
> >  - pixel-clock-frequency: Pixel clock frequency.
> > @@ -29,6 +35,12 @@ Example:
> >  			reg = <0x5d>;
> >  			reset-gpios = <&gpio3 30 0>;
> >  
> > +			clocks = <&sensor_clk>;
> > +
> > +			vdd-supply = <&reg_vdd>;
> > +			vdd_io-supply = <&reg_vdd_io>;
> > +			vaa-supply = <&reg_vaa>;
> > +
> >  			port {
> >  				mt9p031_1: endpoint {
> >  					input-clock-frequency = <6000000>;

[PATCH v3 4/6] dt-bindings: media: mt9p031: add pclk-sample property

[Stefan Riedmueller]

Add the pclk-sample property to the list of optional endpoint
properties for the mt9p031 camera sensor.

Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 Documentation/devicetree/bindings/media/i2c/mt9p031.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
index 4437d0e3147d..17e44fc6dc66 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
@@ -22,6 +22,10 @@ Required Properties:
 Optional Properties:
 - reset-gpios: Chip reset GPIO
 
+Optional endpoint properties:
+- pclk-sample: For information see ../video-interfaces.txt. The value is set to
+  0 if it isn't specified.
+
 For further reading on port node refer to
 Documentation/devicetree/bindings/media/video-interfaces.txt.
 
@@ -45,6 +49,7 @@ Example:
 				mt9p031_1: endpoint {
 					input-clock-frequency = <6000000>;
 					pixel-clock-frequency = <96000000>;
+					pclk-sample = <1>;
 				};
 			};
 		};
-- 
2.25.1

[PATCH v3 5/6] 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 its
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 | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 6a6f16df3f4a..3511c4ff350d 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -79,7 +79,9 @@
 #define		MT9P031_PIXEL_CLOCK_INVERT		(1 << 15)
 #define		MT9P031_PIXEL_CLOCK_SHIFT(n)		((n) << 8)
 #define		MT9P031_PIXEL_CLOCK_DIVIDE(n)		((n) << 0)
-#define MT9P031_FRAME_RESTART				0x0b
+#define MT9P031_RESTART					0x0b
+#define		MT9P031_FRAME_PAUSE_RESTART		(1 << 1)
+#define		MT9P031_FRAME_RESTART			(1 << 0)
 #define MT9P031_SHUTTER_DELAY				0x0c
 #define MT9P031_RST					0x0d
 #define		MT9P031_RST_ENABLE			1
@@ -482,9 +484,23 @@ 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) {
+		/* enable pause restart */
+		val = MT9P031_FRAME_PAUSE_RESTART;
+		ret = mt9p031_write(client, MT9P031_RESTART, val);
+		if (ret < 0)
+			return ret;
+
+		/* enable restart + keep pause restart set */
+		val |= MT9P031_FRAME_RESTART;
+		ret = mt9p031_write(client, MT9P031_RESTART, val);
+		if (ret < 0)
+			return ret;
+
 		/* Stop sensor readout */
 		ret = mt9p031_set_output_control(mt9p031,
 						 MT9P031_OUTPUT_CONTROL_CEN, 0);
@@ -504,6 +520,16 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * - clear pause restart
+	 * - don't clear restart as clearing restart manually can cause
+	 *   undefined behavior
+	 */
+	val = MT9P031_FRAME_RESTART;
+	ret = mt9p031_write(client, MT9P031_RESTART, val);
+	if (ret < 0)
+		return ret;
+
 	return mt9p031_pll_enable(mt9p031);
 }
 
-- 
2.25.1

[PATCH v3 6/6] media: mt9p031: Use BIT macro

[Stefan Riedmueller]

Make use of the BIT macro for setting individual bits. This improves
readability and safety with respect to shifts.

Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
 drivers/media/i2c/mt9p031.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 3511c4ff350d..0a5bcbebe55f 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -76,39 +76,39 @@
 #define	MT9P031_PLL_CONFIG_1				0x11
 #define	MT9P031_PLL_CONFIG_2				0x12
 #define MT9P031_PIXEL_CLOCK_CONTROL			0x0a
-#define		MT9P031_PIXEL_CLOCK_INVERT		(1 << 15)
+#define		MT9P031_PIXEL_CLOCK_INVERT		BIT(15)
 #define		MT9P031_PIXEL_CLOCK_SHIFT(n)		((n) << 8)
 #define		MT9P031_PIXEL_CLOCK_DIVIDE(n)		((n) << 0)
 #define MT9P031_RESTART					0x0b
-#define		MT9P031_FRAME_PAUSE_RESTART		(1 << 1)
-#define		MT9P031_FRAME_RESTART			(1 << 0)
+#define		MT9P031_FRAME_PAUSE_RESTART		BIT(1)
+#define		MT9P031_FRAME_RESTART			BIT(0)
 #define MT9P031_SHUTTER_DELAY				0x0c
 #define MT9P031_RST					0x0d
 #define		MT9P031_RST_ENABLE			1
 #define		MT9P031_RST_DISABLE			0
 #define MT9P031_READ_MODE_1				0x1e
 #define MT9P031_READ_MODE_2				0x20
-#define		MT9P031_READ_MODE_2_ROW_MIR		(1 << 15)
-#define		MT9P031_READ_MODE_2_COL_MIR		(1 << 14)
-#define		MT9P031_READ_MODE_2_ROW_BLC		(1 << 6)
+#define		MT9P031_READ_MODE_2_ROW_MIR		BIT(15)
+#define		MT9P031_READ_MODE_2_COL_MIR		BIT(14)
+#define		MT9P031_READ_MODE_2_ROW_BLC		BIT(6)
 #define MT9P031_ROW_ADDRESS_MODE			0x22
 #define MT9P031_COLUMN_ADDRESS_MODE			0x23
 #define MT9P031_GLOBAL_GAIN				0x35
 #define		MT9P031_GLOBAL_GAIN_MIN			8
 #define		MT9P031_GLOBAL_GAIN_MAX			1024
 #define		MT9P031_GLOBAL_GAIN_DEF			8
-#define		MT9P031_GLOBAL_GAIN_MULT		(1 << 6)
+#define		MT9P031_GLOBAL_GAIN_MULT		BIT(6)
 #define MT9P031_ROW_BLACK_TARGET			0x49
 #define MT9P031_ROW_BLACK_DEF_OFFSET			0x4b
 #define MT9P031_GREEN1_OFFSET				0x60
 #define MT9P031_GREEN2_OFFSET				0x61
 #define MT9P031_BLACK_LEVEL_CALIBRATION			0x62
-#define		MT9P031_BLC_MANUAL_BLC			(1 << 0)
+#define		MT9P031_BLC_MANUAL_BLC			BIT(0)
 #define MT9P031_RED_OFFSET				0x63
 #define MT9P031_BLUE_OFFSET				0x64
 #define MT9P031_TEST_PATTERN				0xa0
 #define		MT9P031_TEST_PATTERN_SHIFT		3
-#define		MT9P031_TEST_PATTERN_ENABLE		(1 << 0)
+#define		MT9P031_TEST_PATTERN_ENABLE		BIT(0)
 #define		MT9P031_TEST_PATTERN_DISABLE		(0 << 0)
 #define MT9P031_TEST_PATTERN_GREEN			0xa1
 #define MT9P031_TEST_PATTERN_RED			0xa2
-- 
2.25.1

Re: [PATCH v3 6/6] media: mt9p031: Use BIT macro

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Fri, Jul 02, 2021 at 11:59:22AM +0200, Stefan Riedmueller wrote:
> Make use of the BIT macro for setting individual bits. This improves
> readability and safety with respect to shifts.
> 
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
>  drivers/media/i2c/mt9p031.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 3511c4ff350d..0a5bcbebe55f 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -76,39 +76,39 @@
>  #define	MT9P031_PLL_CONFIG_1				0x11
>  #define	MT9P031_PLL_CONFIG_2				0x12
>  #define MT9P031_PIXEL_CLOCK_CONTROL			0x0a
> -#define		MT9P031_PIXEL_CLOCK_INVERT		(1 << 15)
> +#define		MT9P031_PIXEL_CLOCK_INVERT		BIT(15)
>  #define		MT9P031_PIXEL_CLOCK_SHIFT(n)		((n) << 8)
>  #define		MT9P031_PIXEL_CLOCK_DIVIDE(n)		((n) << 0)
>  #define MT9P031_RESTART					0x0b
> -#define		MT9P031_FRAME_PAUSE_RESTART		(1 << 1)
> -#define		MT9P031_FRAME_RESTART			(1 << 0)
> +#define		MT9P031_FRAME_PAUSE_RESTART		BIT(1)
> +#define		MT9P031_FRAME_RESTART			BIT(0)
>  #define MT9P031_SHUTTER_DELAY				0x0c
>  #define MT9P031_RST					0x0d
>  #define		MT9P031_RST_ENABLE			1

This could also be turned into BIT(0).

>  #define		MT9P031_RST_DISABLE			0

This should then be dropped.

>  #define MT9P031_READ_MODE_1				0x1e
>  #define MT9P031_READ_MODE_2				0x20
> -#define		MT9P031_READ_MODE_2_ROW_MIR		(1 << 15)
> -#define		MT9P031_READ_MODE_2_COL_MIR		(1 << 14)
> -#define		MT9P031_READ_MODE_2_ROW_BLC		(1 << 6)
> +#define		MT9P031_READ_MODE_2_ROW_MIR		BIT(15)
> +#define		MT9P031_READ_MODE_2_COL_MIR		BIT(14)
> +#define		MT9P031_READ_MODE_2_ROW_BLC		BIT(6)
>  #define MT9P031_ROW_ADDRESS_MODE			0x22
>  #define MT9P031_COLUMN_ADDRESS_MODE			0x23
>  #define MT9P031_GLOBAL_GAIN				0x35
>  #define		MT9P031_GLOBAL_GAIN_MIN			8
>  #define		MT9P031_GLOBAL_GAIN_MAX			1024
>  #define		MT9P031_GLOBAL_GAIN_DEF			8
> -#define		MT9P031_GLOBAL_GAIN_MULT		(1 << 6)
> +#define		MT9P031_GLOBAL_GAIN_MULT		BIT(6)
>  #define MT9P031_ROW_BLACK_TARGET			0x49
>  #define MT9P031_ROW_BLACK_DEF_OFFSET			0x4b
>  #define MT9P031_GREEN1_OFFSET				0x60
>  #define MT9P031_GREEN2_OFFSET				0x61
>  #define MT9P031_BLACK_LEVEL_CALIBRATION			0x62
> -#define		MT9P031_BLC_MANUAL_BLC			(1 << 0)
> +#define		MT9P031_BLC_MANUAL_BLC			BIT(0)
>  #define MT9P031_RED_OFFSET				0x63
>  #define MT9P031_BLUE_OFFSET				0x64
>  #define MT9P031_TEST_PATTERN				0xa0
>  #define		MT9P031_TEST_PATTERN_SHIFT		3
> -#define		MT9P031_TEST_PATTERN_ENABLE		(1 << 0)
> +#define		MT9P031_TEST_PATTERN_ENABLE		BIT(0)
>  #define		MT9P031_TEST_PATTERN_DISABLE		(0 << 0)

Similarly here, MT9P031_TEST_PATTERN_DISABLE should be dropped.

>  #define MT9P031_TEST_PATTERN_GREEN			0xa1
>  #define MT9P031_TEST_PATTERN_RED			0xa2

-- 
Regards,

Laurent Pinchart