diff mbox series

[03/16] media: i2c: rdacm20: Replace goto with a loop

Message ID 20210216174146.106639-4-jacopo+renesas@jmondi.org
State New
Headers show
Series [01/16] media: i2c: rdacm20: Enable noise immunity | expand

Commit Message

Jacopo Mondi Feb. 16, 2021, 5:41 p.m. UTC
During the camera module initialization the image sensor PID is read to
verify it can correctly be identified. The current implementation is
rather confused and uses a loop implemented with a label and a goto.

Replace it with a more compact for() loop.

No functional changes intended.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

Comments

Kieran Bingham Feb. 17, 2021, 1:01 p.m. UTC | #1
Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to

> verify it can correctly be identified. The current implementation is

> rather confused and uses a loop implemented with a label and a goto.

> 

> Replace it with a more compact for() loop.

> 

> No functional changes intended.


I think there is a functional change in here, but I almost like it.

Before, if the read was successful, it would check to see if the
OV10635_PID == OV10635_VERSION, and if not it would print that the read
was successful but a mismatch.

Now - it will retry again instead, and if at the end of the retries it
still fails then it's a failure.

This means we perhaps don't get told if the device id is not correct in
the same way, but it also means that if the VERSION was not correct
because of a read error (which I believe i've seen occur), it will retry.

Because there is a functional change you might want to update the
commit, but I still think this is a good change overall.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> 

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> ---

>  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------

>  1 file changed, 10 insertions(+), 17 deletions(-)

> 

> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> index 4d9bac87cba8..6504ed0bd3bc 100644

> --- a/drivers/media/i2c/rdacm20.c

> +++ b/drivers/media/i2c/rdacm20.c

> @@ -59,6 +59,8 @@

>   */

>  #define OV10635_PIXEL_RATE		(44000000)

>  

> +#define OV10635_PID_TIMEOUT		3

> +

>  static const struct ov10635_reg {

>  	u16	reg;

>  	u8	val;

> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {

>  

>  static int rdacm20_initialize(struct rdacm20_device *dev)

>  {

> -	unsigned int retry = 3;

> +	unsigned int i;

>  	int ret;

>  

>  	/* Verify communication with the MAX9271: ping to wakeup. */

> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

>  		return ret;

>  	usleep_range(10000, 15000);

>  

> -again:

> -	ret = ov10635_read16(dev, OV10635_PID);

> -	if (ret < 0) {

> -		if (retry--)

> -			goto again;

> -

> -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",

> -			ret);

> -		return -ENXIO;

> +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

> +		ret = ov10635_read16(dev, OV10635_PID);

> +		if (ret == OV10635_VERSION)

> +			break;

> +		usleep_range(1000, 2000);

>  	}

> -

> -	if (ret != OV10635_VERSION) {

> -		if (retry--)

> -			goto again;

> -

> -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",

> -			ret);

> +	if (i == OV10635_PID_TIMEOUT) {

> +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);

>  		return -ENXIO;

>  	}

>  

>
Laurent Pinchart Feb. 22, 2021, 1:05 a.m. UTC | #2
Hi Jacopo,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:

> > During the camera module initialization the image sensor PID is read to

> > verify it can correctly be identified. The current implementation is

> > rather confused and uses a loop implemented with a label and a goto.

> > 

> > Replace it with a more compact for() loop.

> > 

> > No functional changes intended.

> 

> I think there is a functional change in here, but I almost like it.

> 

> Before, if the read was successful, it would check to see if the

> OV10635_PID == OV10635_VERSION, and if not it would print that the read

> was successful but a mismatch.

> 

> Now - it will retry again instead, and if at the end of the retries it

> still fails then it's a failure.

> 

> This means we perhaps don't get told if the device id is not correct in

> the same way, but it also means that if the VERSION was not correct

> because of a read error (which I believe i've seen occur), it will retry.


I was going to ask about that, whether we can have a successful I2C read
operation that would return incorrect data. If we do, aren't we screwed
? If there's a non-negligible probability that reads will return
incorrect data without any way to know about it (for other registers
than the version register of course), then I would consider that writes
could fail the same way, and that would mean an unusable device,
wouldn't it ?

If, on the other hand, read failures can always (or nearly always,
ignoring space neutrinos and similar niceties) be detected, then I think
we should avoid the functional change.

> Because there is a functional change you might want to update the

> commit, but I still think this is a good change overall.

> 

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > ---

> >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------

> >  1 file changed, 10 insertions(+), 17 deletions(-)

> > 

> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > index 4d9bac87cba8..6504ed0bd3bc 100644

> > --- a/drivers/media/i2c/rdacm20.c

> > +++ b/drivers/media/i2c/rdacm20.c

> > @@ -59,6 +59,8 @@

> >   */

> >  #define OV10635_PIXEL_RATE		(44000000)

> >  

> > +#define OV10635_PID_TIMEOUT		3

> > +

> >  static const struct ov10635_reg {

> >  	u16	reg;

> >  	u8	val;

> > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {

> >  

> >  static int rdacm20_initialize(struct rdacm20_device *dev)

> >  {

> > -	unsigned int retry = 3;

> > +	unsigned int i;

> >  	int ret;

> >  

> >  	/* Verify communication with the MAX9271: ping to wakeup. */

> > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> >  		return ret;

> >  	usleep_range(10000, 15000);

> >  

> > -again:

> > -	ret = ov10635_read16(dev, OV10635_PID);

> > -	if (ret < 0) {

> > -		if (retry--)

> > -			goto again;

> > -

> > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",

> > -			ret);

> > -		return -ENXIO;

> > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

> > +		ret = ov10635_read16(dev, OV10635_PID);

> > +		if (ret == OV10635_VERSION)

> > +			break;

> > +		usleep_range(1000, 2000);

> >  	}

> > -

> > -	if (ret != OV10635_VERSION) {

> > -		if (retry--)

> > -			goto again;

> > -

> > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",

> > -			ret);

> > +	if (i == OV10635_PID_TIMEOUT) {

> > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);

> >  		return -ENXIO;

> >  	}

> >  


-- 
Regards,

Laurent Pinchart
Jacopo Mondi Feb. 22, 2021, 3:06 p.m. UTC | #3
Hi,

On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,

>

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>

> On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:

> > On 16/02/2021 17:41, Jacopo Mondi wrote:

> > > During the camera module initialization the image sensor PID is read to

> > > verify it can correctly be identified. The current implementation is

> > > rather confused and uses a loop implemented with a label and a goto.

> > >

> > > Replace it with a more compact for() loop.

> > >

> > > No functional changes intended.

> >

> > I think there is a functional change in here, but I almost like it.

> >

> > Before, if the read was successful, it would check to see if the

> > OV10635_PID == OV10635_VERSION, and if not it would print that the read

> > was successful but a mismatch.

> >

> > Now - it will retry again instead, and if at the end of the retries it

> > still fails then it's a failure.

> >

> > This means we perhaps don't get told if the device id is not correct in

> > the same way, but it also means that if the VERSION was not correct

> > because of a read error (which I believe i've seen occur), it will retry.

>

> I was going to ask about that, whether we can have a successful I2C read

> operation that would return incorrect data. If we do, aren't we screwed

> ? If there's a non-negligible probability that reads will return

> incorrect data without any way to know about it (for other registers

> than the version register of course), then I would consider that writes

> could fail the same way, and that would mean an unusable device,

> wouldn't it ?

>

> If, on the other hand, read failures can always (or nearly always,

> ignoring space neutrinos and similar niceties) be detected, then I think

> we should avoid the functional change.

>

> > Because there is a functional change you might want to update the

> > commit, but I still think this is a good change overall.

> >


I'm not sure I got your concerns to be honest :/
yes before the code flow was like

        ret = ov10635_read();
        if (ret < 0) {

        }

        if (ret != PID) {

        }

But the condition ret != PID implied ret < 0 so I don't really get
what changes, apart from the fact that in the previous version we
could have had two different error messages for the same issue, and yes,
I saw ID mistmatch happening but the value of knowing the i2c read
didn't fail but the read data was garbage (usually it's 0x01 when it
fails iirc) is, well, questionable.

I'm sorry I didn't fully get this comment.


> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> >

> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > > ---

> > >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------

> > >  1 file changed, 10 insertions(+), 17 deletions(-)

> > >

> > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > > index 4d9bac87cba8..6504ed0bd3bc 100644

> > > --- a/drivers/media/i2c/rdacm20.c

> > > +++ b/drivers/media/i2c/rdacm20.c

> > > @@ -59,6 +59,8 @@

> > >   */

> > >  #define OV10635_PIXEL_RATE		(44000000)

> > >

> > > +#define OV10635_PID_TIMEOUT		3

> > > +

> > >  static const struct ov10635_reg {

> > >  	u16	reg;

> > >  	u8	val;

> > > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {

> > >

> > >  static int rdacm20_initialize(struct rdacm20_device *dev)

> > >  {

> > > -	unsigned int retry = 3;

> > > +	unsigned int i;

> > >  	int ret;

> > >

> > >  	/* Verify communication with the MAX9271: ping to wakeup. */

> > > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> > >  		return ret;

> > >  	usleep_range(10000, 15000);

> > >

> > > -again:

> > > -	ret = ov10635_read16(dev, OV10635_PID);

> > > -	if (ret < 0) {

> > > -		if (retry--)

> > > -			goto again;

> > > -

> > > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",

> > > -			ret);

> > > -		return -ENXIO;

> > > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

> > > +		ret = ov10635_read16(dev, OV10635_PID);

> > > +		if (ret == OV10635_VERSION)

> > > +			break;

> > > +		usleep_range(1000, 2000);

> > >  	}

> > > -

> > > -	if (ret != OV10635_VERSION) {

> > > -		if (retry--)

> > > -			goto again;

> > > -

> > > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",

> > > -			ret);

> > > +	if (i == OV10635_PID_TIMEOUT) {

> > > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);

> > >  		return -ENXIO;

> > >  	}

> > >

>

> --

> Regards,

>

> Laurent Pinchart
Laurent Pinchart Feb. 24, 2021, 8:27 p.m. UTC | #4
Hi Jacopo,

On Mon, Feb 22, 2021 at 04:06:43PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:

> > Hi Jacopo,

> >

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >

> > On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:

> > > On 16/02/2021 17:41, Jacopo Mondi wrote:

> > > > During the camera module initialization the image sensor PID is read to

> > > > verify it can correctly be identified. The current implementation is

> > > > rather confused and uses a loop implemented with a label and a goto.

> > > >

> > > > Replace it with a more compact for() loop.

> > > >

> > > > No functional changes intended.

> > >

> > > I think there is a functional change in here, but I almost like it.

> > >

> > > Before, if the read was successful, it would check to see if the

> > > OV10635_PID == OV10635_VERSION, and if not it would print that the read

> > > was successful but a mismatch.

> > >

> > > Now - it will retry again instead, and if at the end of the retries it

> > > still fails then it's a failure.

> > >

> > > This means we perhaps don't get told if the device id is not correct in

> > > the same way, but it also means that if the VERSION was not correct

> > > because of a read error (which I believe i've seen occur), it will retry.

> >

> > I was going to ask about that, whether we can have a successful I2C read

> > operation that would return incorrect data. If we do, aren't we screwed

> > ? If there's a non-negligible probability that reads will return

> > incorrect data without any way to know about it (for other registers

> > than the version register of course), then I would consider that writes

> > could fail the same way, and that would mean an unusable device,

> > wouldn't it ?

> >

> > If, on the other hand, read failures can always (or nearly always,

> > ignoring space neutrinos and similar niceties) be detected, then I think

> > we should avoid the functional change.

> >

> > > Because there is a functional change you might want to update the

> > > commit, but I still think this is a good change overall.

> 

> I'm not sure I got your concerns to be honest :/

> yes before the code flow was like

> 

>         ret = ov10635_read();

>         if (ret < 0) {

> 

>         }

> 

>         if (ret != PID) {

> 

>         }

> 

> But the condition ret != PID implied ret < 0 so I don't really get

> what changes, apart from the fact that in the previous version we

> could have had two different error messages for the same issue, and yes,

> I saw ID mistmatch happening but the value of knowing the i2c read

> didn't fail but the read data was garbage (usually it's 0x01 when it

> fails iirc) is, well, questionable.


That's worrying :-S May we should add a warning message when the read
succeeds but the ID doesn't match, to at least have a way to track the
issue, and see if other changes get rid of this problem ?

> I'm sorry I didn't fully get this comment.


You're right, I had missed that the current code retried in case of a
version number mismatch. There's no functional change.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> > >

> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > > > ---

> > > >  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------

> > > >  1 file changed, 10 insertions(+), 17 deletions(-)

> > > >

> > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > > > index 4d9bac87cba8..6504ed0bd3bc 100644

> > > > --- a/drivers/media/i2c/rdacm20.c

> > > > +++ b/drivers/media/i2c/rdacm20.c

> > > > @@ -59,6 +59,8 @@

> > > >   */

> > > >  #define OV10635_PIXEL_RATE		(44000000)

> > > >

> > > > +#define OV10635_PID_TIMEOUT		3

> > > > +

> > > >  static const struct ov10635_reg {

> > > >  	u16	reg;

> > > >  	u8	val;

> > > > @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {

> > > >

> > > >  static int rdacm20_initialize(struct rdacm20_device *dev)

> > > >  {

> > > > -	unsigned int retry = 3;

> > > > +	unsigned int i;

> > > >  	int ret;

> > > >

> > > >  	/* Verify communication with the MAX9271: ping to wakeup. */

> > > > @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> > > >  		return ret;

> > > >  	usleep_range(10000, 15000);

> > > >

> > > > -again:

> > > > -	ret = ov10635_read16(dev, OV10635_PID);

> > > > -	if (ret < 0) {

> > > > -		if (retry--)

> > > > -			goto again;

> > > > -

> > > > -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",

> > > > -			ret);

> > > > -		return -ENXIO;

> > > > +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

> > > > +		ret = ov10635_read16(dev, OV10635_PID);

> > > > +		if (ret == OV10635_VERSION)

> > > > +			break;

> > > > +		usleep_range(1000, 2000);

> > > >  	}

> > > > -

> > > > -	if (ret != OV10635_VERSION) {

> > > > -		if (retry--)

> > > > -			goto again;

> > > > -

> > > > -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",

> > > > -			ret);

> > > > +	if (i == OV10635_PID_TIMEOUT) {

> > > > +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);

> > > >  		return -ENXIO;

> > > >  	}

> > > >


-- 
Regards,

Laurent Pinchart
Kieran Bingham Feb. 25, 2021, 8:56 a.m. UTC | #5
On 24/02/2021 20:27, Laurent Pinchart wrote:
> Hi Jacopo,

> 

> On Mon, Feb 22, 2021 at 04:06:43PM +0100, Jacopo Mondi wrote:

>> On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:

>>> Hi Jacopo,

>>>

>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>>>

>>> On Wed, Feb 17, 2021 at 01:01:26PM +0000, Kieran Bingham wrote:

>>>> On 16/02/2021 17:41, Jacopo Mondi wrote:

>>>>> During the camera module initialization the image sensor PID is read to

>>>>> verify it can correctly be identified. The current implementation is

>>>>> rather confused and uses a loop implemented with a label and a goto.

>>>>>

>>>>> Replace it with a more compact for() loop.

>>>>>

>>>>> No functional changes intended.

>>>>

>>>> I think there is a functional change in here, but I almost like it.

>>>>

>>>> Before, if the read was successful, it would check to see if the

>>>> OV10635_PID == OV10635_VERSION, and if not it would print that the read

>>>> was successful but a mismatch.

>>>>

>>>> Now - it will retry again instead, and if at the end of the retries it

>>>> still fails then it's a failure.

>>>>

>>>> This means we perhaps don't get told if the device id is not correct in

>>>> the same way, but it also means that if the VERSION was not correct

>>>> because of a read error (which I believe i've seen occur), it will retry.


So - to be clear here, I meant a 'read error', as in perhaps a
one-bit-flip or something else, not an error detected and propogated by
the I2C controllers.

I.e. ... something happening on the bus that gives a different result
but the 'read' was successful.... it's just that it returns a different
value than expected.

Given our noisy bus, not certain bus speeds, etc etc, I believe this can
happen.


>>>

>>> I was going to ask about that, whether we can have a successful I2C read

>>> operation that would return incorrect data. If we do, aren't we screwed

>>> ? If there's a non-negligible probability that reads will return

>>> incorrect data without any way to know about it (for other registers

>>> than the version register of course), then I would consider that writes

>>> could fail the same way, and that would mean an unusable device,

>>> wouldn't it ?

>>>

>>> If, on the other hand, read failures can always (or nearly always,

>>> ignoring space neutrinos and similar niceties) be detected, then I think

>>> we should avoid the functional change.

>>>

>>>> Because there is a functional change you might want to update the

>>>> commit, but I still think this is a good change overall.

>>

>> I'm not sure I got your concerns to be honest :/

>> yes before the code flow was like

>>

>>         ret = ov10635_read();

>>         if (ret < 0) {

>>

>>         }

>>

>>         if (ret != PID) {

>>


And so here you might have had a 'successful' read of the wrong value,
which means that ret > 0 but != PID.

>>         }

>>

>> But the condition ret != PID implied ret < 0 so I don't really get

>> what changes, apart from the fact that in the previous version we

>> could have had two different error messages for the same issue, and yes,

>> I saw ID mistmatch happening but the value of knowing the i2c read

>> didn't fail but the read data was garbage (usually it's 0x01 when it

>> fails iirc) is, well, questionable.

> 

> That's worrying :-S May we should add a warning message when the read

> succeeds but the ID doesn't match, to at least have a way to track the

> issue, and see if other changes get rid of this problem ?

> 


Ok, now I'm confused, that's what I was talking about!

Before we did do this, and now we don't. Ergo - functional change.


>> I'm sorry I didn't fully get this comment.

> 

> You're right, I had missed that the current code retried in case of a

> version number mismatch. There's no functional change.


I still think there's a functional change, but I'm not all too worried
about it.

As I said before, I think it's worth the retry in that event, which
didn't happen before, so my tag still holds.


> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 

>>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

>>>>

>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

>>>>> ---

>>>>>  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------

>>>>>  1 file changed, 10 insertions(+), 17 deletions(-)

>>>>>

>>>>> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

>>>>> index 4d9bac87cba8..6504ed0bd3bc 100644

>>>>> --- a/drivers/media/i2c/rdacm20.c

>>>>> +++ b/drivers/media/i2c/rdacm20.c

>>>>> @@ -59,6 +59,8 @@

>>>>>   */

>>>>>  #define OV10635_PIXEL_RATE		(44000000)

>>>>>

>>>>> +#define OV10635_PID_TIMEOUT		3

>>>>> +

>>>>>  static const struct ov10635_reg {

>>>>>  	u16	reg;

>>>>>  	u8	val;

>>>>> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {

>>>>>

>>>>>  static int rdacm20_initialize(struct rdacm20_device *dev)

>>>>>  {

>>>>> -	unsigned int retry = 3;

>>>>> +	unsigned int i;

>>>>>  	int ret;

>>>>>

>>>>>  	/* Verify communication with the MAX9271: ping to wakeup. */

>>>>> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

>>>>>  		return ret;

>>>>>  	usleep_range(10000, 15000);

>>>>>

>>>>> -again:

>>>>> -	ret = ov10635_read16(dev, OV10635_PID);

>>>>> -	if (ret < 0) {

>>>>> -		if (retry--)

>>>>> -			goto again;

>>>>> -

>>>>> -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",

>>>>> -			ret);

>>>>> -		return -ENXIO;

>>>>> +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

>>>>> +		ret = ov10635_read16(dev, OV10635_PID);

>>>>> +		if (ret == OV10635_VERSION)

>>>>> +			break;

>>>>> +		usleep_range(1000, 2000);

>>>>>  	}

>>>>> -

>>>>> -	if (ret != OV10635_VERSION) {

>>>>> -		if (retry--)

>>>>> -			goto again;

>>>>> -

>>>>> -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",

>>>>> -			ret);

>>>>> +	if (i == OV10635_PID_TIMEOUT) {

>>>>> +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);

>>>>>  		return -ENXIO;

>>>>>  	}

>>>>>

>
diff mbox series

Patch

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 4d9bac87cba8..6504ed0bd3bc 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -59,6 +59,8 @@ 
  */
 #define OV10635_PIXEL_RATE		(44000000)
 
+#define OV10635_PID_TIMEOUT		3
+
 static const struct ov10635_reg {
 	u16	reg;
 	u8	val;
@@ -452,7 +454,7 @@  static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
 
 static int rdacm20_initialize(struct rdacm20_device *dev)
 {
-	unsigned int retry = 3;
+	unsigned int i;
 	int ret;
 
 	/* Verify communication with the MAX9271: ping to wakeup. */
@@ -501,23 +503,14 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 		return ret;
 	usleep_range(10000, 15000);
 
-again:
-	ret = ov10635_read16(dev, OV10635_PID);
-	if (ret < 0) {
-		if (retry--)
-			goto again;
-
-		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
-			ret);
-		return -ENXIO;
+	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
+		ret = ov10635_read16(dev, OV10635_PID);
+		if (ret == OV10635_VERSION)
+			break;
+		usleep_range(1000, 2000);
 	}
-
-	if (ret != OV10635_VERSION) {
-		if (retry--)
-			goto again;
-
-		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
-			ret);
+	if (i == OV10635_PID_TIMEOUT) {
+		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
 		return -ENXIO;
 	}