All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jacopo Mondi <jacopo@jmondi.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
	niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop
Date: Mon, 22 Feb 2021 16:06:43 +0100	[thread overview]
Message-ID: <20210222150643.cuv6uye3wpxaykim@uno.localdomain> (raw)
In-Reply-To: <YDMDPymgU/N5wd/i@pendragon.ideasonboard.com>

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

  reply	other threads:[~2021-02-22 15:08 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-16 17:41 [PATCH 00/16] media: i2c: GMSL reliability improvements Jacopo Mondi
2021-02-16 17:41 ` [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity Jacopo Mondi
2021-02-17 12:55   ` Kieran Bingham
2021-02-22  0:49     ` Laurent Pinchart
2021-02-22 14:59       ` Jacopo Mondi
2021-02-24 20:35         ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field Jacopo Mondi
2021-02-17 12:56   ` Kieran Bingham
2021-02-22  0:58   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop Jacopo Mondi
2021-02-17 13:01   ` Kieran Bingham
2021-02-22  1:05     ` Laurent Pinchart
2021-02-22 15:06       ` Jacopo Mondi [this message]
2021-02-24 20:27         ` Laurent Pinchart
2021-02-25  8:56           ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 04/16] media: i2c: rdacm20: Report camera module name Jacopo Mondi
2021-02-17 13:02   ` Kieran Bingham
2021-02-22  1:06   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 05/16] media: i2c: rdacm20: Check return values Jacopo Mondi
2021-02-17 13:08   ` Kieran Bingham
2021-02-22  1:09   ` Laurent Pinchart
2021-02-22 15:08     ` Jacopo Mondi
2021-02-16 17:41 ` [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset Jacopo Mondi
2021-02-17 13:22   ` Kieran Bingham
2021-02-22  1:15   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay Jacopo Mondi
2021-02-17  8:02   ` Geert Uytterhoeven
2021-02-17 13:33   ` Kieran Bingham
2021-02-22  1:18     ` Laurent Pinchart
2021-02-22 15:11       ` Jacopo Mondi
2021-02-24 20:29         ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 08/16] media: i2c: max9286: Adjust parameters indent Jacopo Mondi
2021-02-17 13:36   ` Kieran Bingham
2021-02-22  1:20   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization Jacopo Mondi
2021-02-17  8:06   ` Geert Uytterhoeven
2021-02-17 13:55   ` Kieran Bingham
2021-02-22  1:27   ` Laurent Pinchart
2021-02-22 15:19     ` Jacopo Mondi
2021-02-24 20:30       ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv Jacopo Mondi
2021-02-17  8:07   ` Geert Uytterhoeven
2021-02-18 15:06   ` Kieran Bingham
2021-02-22  1:29   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 11/16] media: i2c: max9286: Cache channel amplitude Jacopo Mondi
2021-02-17  8:08   ` Geert Uytterhoeven
2021-02-18 15:39   ` Kieran Bingham
2021-02-22  1:33   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 12/16] media: i2c: max9286: Define high " Jacopo Mondi
2021-02-18 15:39   ` Kieran Bingham
2021-02-22  1:35   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op Jacopo Mondi
2021-02-17  8:17   ` Geert Uytterhoeven
2021-02-18 16:13   ` Kieran Bingham
2021-02-22  1:52     ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound Jacopo Mondi
2021-02-17  8:19   ` Geert Uytterhoeven
2021-02-18 16:00   ` Kieran Bingham
2021-02-22  2:03   ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 15/16] media: i2c: max9286: Rework comments in .bound() Jacopo Mondi
2021-02-18 16:03   ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate Jacopo Mondi
2021-02-17  8:19   ` Geert Uytterhoeven
2021-02-18 16:07   ` Kieran Bingham
2021-02-22  2:06     ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210222150643.cuv6uye3wpxaykim@uno.localdomain \
    --to=jacopo@jmondi.org \
    --cc=geert@linux-m68k.org \
    --cc=jacopo+renesas@jmondi.org \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --subject='Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.