linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: ov5640: fix MIPI power sequence
@ 2020-10-07 14:43 Hugues Fruchet
  2020-10-07 14:43 ` Hugues Fruchet
  0 siblings, 1 reply; 5+ messages in thread
From: Hugues Fruchet @ 2020-10-07 14:43 UTC (permalink / raw)
  To: Lad Prabhakar, Jacopo Mondi, Steve Longerbeam, Sakari Ailus,
	Hans Verkuil, Mauro Carvalho Chehab
  Cc: linux-media, linux-stm32, Hugues Fruchet, Alain Volmat

fixes: b1751ae652fb ("media: i2c: ov5640: Separate out mipi configuration from s_power")

Fix ov5640_write_reg()return value unchecked at power off.
Reformat code to keep register access below the register description.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8d0254d..f34e62e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1940,14 +1940,6 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
-	if (!on) {
-		/* Reset MIPI bus settings to their default values. */
-		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
-		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
-		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
-		return 0;
-	}
-
 	/*
 	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
 	 *
@@ -1958,7 +1950,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [3] = 0	: Power up MIPI LS Rx
 	 * [2] = 0	: MIPI interface disabled
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+			       on ? 0x40 : 0x58);
 	if (ret)
 		return ret;
 
@@ -1969,7 +1962,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [5] = 1	: Gate clock when 'no packets'
 	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
+	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00,
+			       on ? 0x24 : 0x04);
 	if (ret)
 		return ret;
 
@@ -1981,7 +1975,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
 	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
+			       on ? 0x70 : 0x00);
 	if (ret)
 		return ret;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] media: ov5640: fix MIPI power sequence
  2020-10-07 14:43 [PATCH] media: ov5640: fix MIPI power sequence Hugues Fruchet
@ 2020-10-07 14:43 ` Hugues Fruchet
  2020-10-07 15:01   ` Sakari Ailus
  0 siblings, 1 reply; 5+ messages in thread
From: Hugues Fruchet @ 2020-10-07 14:43 UTC (permalink / raw)
  To: Lad Prabhakar, Jacopo Mondi, Steve Longerbeam, Sakari Ailus,
	Hans Verkuil, Mauro Carvalho Chehab
  Cc: linux-media, linux-stm32, Hugues Fruchet, Alain Volmat

fixes: b1751ae652fb ("media: i2c: ov5640: Separate out mipi configuration from s_power")

Fix ov5640_write_reg()return value unchecked at power off.
Reformat code to keep register access below the register description.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
Change-Id: I12db67c416c3d63eadee400a3c89aaf48c5b1469
---
 drivers/media/i2c/ov5640.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8d0254d..f34e62e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1940,14 +1940,6 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
-	if (!on) {
-		/* Reset MIPI bus settings to their default values. */
-		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
-		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
-		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
-		return 0;
-	}
-
 	/*
 	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
 	 *
@@ -1958,7 +1950,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [3] = 0	: Power up MIPI LS Rx
 	 * [2] = 0	: MIPI interface disabled
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+			       on ? 0x40 : 0x58);
 	if (ret)
 		return ret;
 
@@ -1969,7 +1962,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [5] = 1	: Gate clock when 'no packets'
 	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
+	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00,
+			       on ? 0x24 : 0x04);
 	if (ret)
 		return ret;
 
@@ -1981,7 +1975,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
 	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
+			       on ? 0x70 : 0x00);
 	if (ret)
 		return ret;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov5640: fix MIPI power sequence
  2020-10-07 14:43 ` Hugues Fruchet
@ 2020-10-07 15:01   ` Sakari Ailus
  2020-10-07 15:09     ` Jacopo Mondi
  0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2020-10-07 15:01 UTC (permalink / raw)
  To: Hugues Fruchet
  Cc: Lad Prabhakar, Jacopo Mondi, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, linux-stm32, Alain Volmat

Hi Hugues,

On Wed, Oct 07, 2020 at 04:43:18PM +0200, Hugues Fruchet wrote:
> fixes: b1751ae652fb ("media: i2c: ov5640: Separate out mipi configuration from s_power")

Fixes: ...

And preferrably before Sob line.

> 
> Fix ov5640_write_reg()return value unchecked at power off.
> Reformat code to keep register access below the register description.

If there's an error, I wouldn't stop turning things off, but just proceed.
The caller will ignore the error in that case anyway. This changes with the
patch.

> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> Change-Id: I12db67c416c3d63eadee400a3c89aaf48c5b1469

This was probably not intended to be here.

> ---
>  drivers/media/i2c/ov5640.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 8d0254d..f34e62e 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1940,14 +1940,6 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>  {
>  	int ret;
>  
> -	if (!on) {
> -		/* Reset MIPI bus settings to their default values. */
> -		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> -		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> -		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> -		return 0;
> -	}
> -
>  	/*
>  	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
>  	 *
> @@ -1958,7 +1950,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>  	 * [3] = 0	: Power up MIPI LS Rx
>  	 * [2] = 0	: MIPI interface disabled
>  	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
> +			       on ? 0x40 : 0x58);
>  	if (ret)
>  		return ret;
>  
> @@ -1969,7 +1962,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>  	 * [5] = 1	: Gate clock when 'no packets'
>  	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
>  	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00,
> +			       on ? 0x24 : 0x04);
>  	if (ret)
>  		return ret;
>  
> @@ -1981,7 +1975,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>  	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
>  	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
>  	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> +			       on ? 0x70 : 0x00);
>  	if (ret)
>  		return ret;
>  

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov5640: fix MIPI power sequence
  2020-10-07 15:01   ` Sakari Ailus
@ 2020-10-07 15:09     ` Jacopo Mondi
  2020-10-08  8:58       ` Hugues FRUCHET
  0 siblings, 1 reply; 5+ messages in thread
From: Jacopo Mondi @ 2020-10-07 15:09 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Hugues Fruchet, Lad Prabhakar, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, linux-stm32, Alain Volmat

Hi Hugues, Sakari,

On Wed, Oct 07, 2020 at 06:01:03PM +0300, Sakari Ailus wrote:
> Hi Hugues,
>
> On Wed, Oct 07, 2020 at 04:43:18PM +0200, Hugues Fruchet wrote:
> > fixes: b1751ae652fb ("media: i2c: ov5640: Separate out mipi configuration from s_power")
>
> Fixes: ...
>
> And preferrably before Sob line.
>
> >
> > Fix ov5640_write_reg()return value unchecked at power off.
> > Reformat code to keep register access below the register description.
>
> If there's an error, I wouldn't stop turning things off, but just proceed.
> The caller will ignore the error in that case anyway. This changes with the
> patch.

Agreed, I think it's best to continue instead of bailing out as we're
in a power-off path

>
> >
> > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > Change-Id: I12db67c416c3d63eadee400a3c89aaf48c5b1469
>
> This was probably not intended to be here.
>
> > ---
> >  drivers/media/i2c/ov5640.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 8d0254d..f34e62e 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -1940,14 +1940,6 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> >  {
> >  	int ret;
> >
> > -	if (!on) {
> > -		/* Reset MIPI bus settings to their default values. */
> > -		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> > -		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
> > -		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
> > -		return 0;
> > -	}
> > -
> >  	/*
> >  	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> >  	 *
> > @@ -1958,7 +1950,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> >  	 * [3] = 0	: Power up MIPI LS Rx
> >  	 * [2] = 0	: MIPI interface disabled
> >  	 */
> > -	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > +	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
> > +			       on ? 0x40 : 0x58);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -1969,7 +1962,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> >  	 * [5] = 1	: Gate clock when 'no packets'
> >  	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> >  	 */
> > -	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> > +	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00,
> > +			       on ? 0x24 : 0x04);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -1981,7 +1975,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> >  	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> >  	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> >  	 */
> > -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> > +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> > +			       on ? 0x70 : 0x00);
> >  	if (ret)
> >  		return ret;
> >
>
> --
> Kind regards,
>
> Sakari Ailus

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov5640: fix MIPI power sequence
  2020-10-07 15:09     ` Jacopo Mondi
@ 2020-10-08  8:58       ` Hugues FRUCHET
  0 siblings, 0 replies; 5+ messages in thread
From: Hugues FRUCHET @ 2020-10-08  8:58 UTC (permalink / raw)
  To: Jacopo Mondi, Sakari Ailus
  Cc: Lad Prabhakar, Steve Longerbeam, Hans Verkuil,
	Mauro Carvalho Chehab, linux-media, linux-stm32, Alain VOLMAT

Hi Jacopo, Sakari,

As you want, let's drop this patch.

BR,
Hugues.

On 10/7/20 5:09 PM, Jacopo Mondi wrote:
> Hi Hugues, Sakari,
> 
> On Wed, Oct 07, 2020 at 06:01:03PM +0300, Sakari Ailus wrote:
>> Hi Hugues,
>>
>> On Wed, Oct 07, 2020 at 04:43:18PM +0200, Hugues Fruchet wrote:
>>> fixes: b1751ae652fb ("media: i2c: ov5640: Separate out mipi configuration from s_power")
>>
>> Fixes: ...
>>
>> And preferrably before Sob line.
>>
>>>
>>> Fix ov5640_write_reg()return value unchecked at power off.
>>> Reformat code to keep register access below the register description.
>>
>> If there's an error, I wouldn't stop turning things off, but just proceed.
>> The caller will ignore the error in that case anyway. This changes with the
>> patch.
> 
> Agreed, I think it's best to continue instead of bailing out as we're
> in a power-off path
> 
>>
>>>
>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>> Change-Id: I12db67c416c3d63eadee400a3c89aaf48c5b1469
>>
>> This was probably not intended to be here.
>>
>>> ---
>>>   drivers/media/i2c/ov5640.c | 17 ++++++-----------
>>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>> index 8d0254d..f34e62e 100644
>>> --- a/drivers/media/i2c/ov5640.c
>>> +++ b/drivers/media/i2c/ov5640.c
>>> @@ -1940,14 +1940,6 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>>>   {
>>>   	int ret;
>>>
>>> -	if (!on) {
>>> -		/* Reset MIPI bus settings to their default values. */
>>> -		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
>>> -		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
>>> -		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
>>> -		return 0;
>>> -	}
>>> -
>>>   	/*
>>>   	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
>>>   	 *
>>> @@ -1958,7 +1950,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>>>   	 * [3] = 0	: Power up MIPI LS Rx
>>>   	 * [2] = 0	: MIPI interface disabled
>>>   	 */
>>> -	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
>>> +	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
>>> +			       on ? 0x40 : 0x58);
>>>   	if (ret)
>>>   		return ret;
>>>
>>> @@ -1969,7 +1962,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>>>   	 * [5] = 1	: Gate clock when 'no packets'
>>>   	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
>>>   	 */
>>> -	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
>>> +	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00,
>>> +			       on ? 0x24 : 0x04);
>>>   	if (ret)
>>>   		return ret;
>>>
>>> @@ -1981,7 +1975,8 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>>>   	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
>>>   	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
>>>   	 */
>>> -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
>>> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
>>> +			       on ? 0x70 : 0x00);
>>>   	if (ret)
>>>   		return ret;
>>>
>>
>> --
>> Kind regards,
>>
>> Sakari Ailus

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-08  8:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 14:43 [PATCH] media: ov5640: fix MIPI power sequence Hugues Fruchet
2020-10-07 14:43 ` Hugues Fruchet
2020-10-07 15:01   ` Sakari Ailus
2020-10-07 15:09     ` Jacopo Mondi
2020-10-08  8:58       ` Hugues FRUCHET

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).