| Message ID | 20211104110924.248444-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9271: Fix pclk_detect silent failure | expand |
Quoting Jacopo Mondi (2021-11-04 11:09:23) > Valid pixel clock detection is performed by spinning on a register read > which if repeated too frequently might fail. As the error is not fatal > ignore it instead of bailing out to continue spinning until the timeout > completion. > > Also relax the time between bus transactions and slightly increase the > wait interval to mitigate the failure risk. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> This seems good to me. In your testing did you identify how many spins/how long it usually takes before it first detects the pixel clock? I.e. - was it cutting it close at 10ms, and we should even still extend this further? (as the usleep_range means we could still loop this 10 ms) Anyway, this looks like a strong improvement. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > > v1->v2: > - Do not continue but jump to a label to respect the sleep timout after a > failed read > > Niklas I kept your tag anyway, hope it's ok. > > Thanks > j > > --- > drivers/media/i2c/max9271.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c > index ff86c8c4ea61..aa4add473716 100644 > --- a/drivers/media/i2c/max9271.c > +++ b/drivers/media/i2c/max9271.c > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val) > /* > * max9271_pclk_detect() - Detect valid pixel clock from image sensor > * > - * Wait up to 10ms for a valid pixel clock. > + * Wait up to 15ms for a valid pixel clock. > * > * Returns 0 for success, < 0 for pixel clock not properly detected > */ > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev) > unsigned int i; > int ret; > > - for (i = 0; i < 100; i++) { > + for (i = 0; i < 10; i++) { > ret = max9271_read(dev, 0x15); > if (ret < 0) > - return ret; > + goto skip; > > if (ret & MAX9271_PCLKDET) > return 0; > > - usleep_range(50, 100); > +skip: > + usleep_range(1000, 1500); > } > > dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n"); > -- > 2.33.1 >
Hi Jacopo, On Thu, Nov 4, 2021 at 12:10 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Valid pixel clock detection is performed by spinning on a register read > which if repeated too frequently might fail. As the error is not fatal > ignore it instead of bailing out to continue spinning until the timeout > completion. > > Also relax the time between bus transactions and slightly increase the > wait interval to mitigate the failure risk. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > > v1->v2: > - Do not continue but jump to a label to respect the sleep timout after a > failed read Thanks for the update! > --- a/drivers/media/i2c/max9271.c > +++ b/drivers/media/i2c/max9271.c > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val) > /* > * max9271_pclk_detect() - Detect valid pixel clock from image sensor > * > - * Wait up to 10ms for a valid pixel clock. > + * Wait up to 15ms for a valid pixel clock. > * > * Returns 0 for success, < 0 for pixel clock not properly detected > */ > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev) > unsigned int i; > int ret; > > - for (i = 0; i < 100; i++) { > + for (i = 0; i < 10; i++) { > ret = max9271_read(dev, 0x15); > if (ret < 0) > - return ret; > + goto skip; Edgar Dijkstra: Go To Statement Considered Harmful? > > if (ret & MAX9271_PCLKDET) "if (ret > 0 && (ret & MAX9271_PCLKDET))"? > return 0; > > - usleep_range(50, 100); > +skip: > + usleep_range(1000, 1500); > } > > dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n"); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert On Mon, Nov 08, 2021 at 11:45:58AM +0100, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Thu, Nov 4, 2021 at 12:10 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > > Valid pixel clock detection is performed by spinning on a register read > > which if repeated too frequently might fail. As the error is not fatal > > ignore it instead of bailing out to continue spinning until the timeout > > completion. > > > > Also relax the time between bus transactions and slightly increase the > > wait interval to mitigate the failure risk. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > > > v1->v2: > > - Do not continue but jump to a label to respect the sleep timout after a > > failed read > > Thanks for the update! > > > --- a/drivers/media/i2c/max9271.c > > +++ b/drivers/media/i2c/max9271.c > > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val) > > /* > > * max9271_pclk_detect() - Detect valid pixel clock from image sensor > > * > > - * Wait up to 10ms for a valid pixel clock. > > + * Wait up to 15ms for a valid pixel clock. > > * > > * Returns 0 for success, < 0 for pixel clock not properly detected > > */ > > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev) > > unsigned int i; > > int ret; > > > > - for (i = 0; i < 100; i++) { > > + for (i = 0; i < 10; i++) { > > ret = max9271_read(dev, 0x15); > > if (ret < 0) > > - return ret; > > + goto skip; > > Edgar Dijkstra: Go To Statement Considered Harmful? > Dijkstra would be very unpleased reading the kernel error path handling implementations. But I got your point, we're not in a cleanup path here =) > > > > if (ret & MAX9271_PCLKDET) > > "if (ret > 0 && (ret & MAX9271_PCLKDET))"? > Much better, thanks. I'll resend! Thanks j > > return 0; > > > > - usleep_range(50, 100); > > +skip: > > + usleep_range(1000, 1500); > > } > > > > dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n"); > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Kieran On Thu, Nov 04, 2021 at 11:04:57PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-11-04 11:09:23) > > Valid pixel clock detection is performed by spinning on a register read > > which if repeated too frequently might fail. As the error is not fatal > > ignore it instead of bailing out to continue spinning until the timeout > > completion. > > > > Also relax the time between bus transactions and slightly increase the > > wait interval to mitigate the failure risk. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > This seems good to me. In your testing did you identify how many > spins/how long it usually takes before it first detects the pixel clock? > > I.e. - was it cutting it close at 10ms, and we should even still extend > this further? (as the usleep_range means we could still loop this 10 ms) Thanks for asking, this turned out to be much quicker than expected with the pclk_clk detected at the first or second attempts all the times. I would not bother reducing the sleep time though. > > Anyway, this looks like a strong improvement. > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thanks j > > > --- > > > > v1->v2: > > - Do not continue but jump to a label to respect the sleep timout after a > > failed read > > > > Niklas I kept your tag anyway, hope it's ok. > > > > Thanks > > j > > > > --- > > drivers/media/i2c/max9271.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c > > index ff86c8c4ea61..aa4add473716 100644 > > --- a/drivers/media/i2c/max9271.c > > +++ b/drivers/media/i2c/max9271.c > > @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val) > > /* > > * max9271_pclk_detect() - Detect valid pixel clock from image sensor > > * > > - * Wait up to 10ms for a valid pixel clock. > > + * Wait up to 15ms for a valid pixel clock. > > * > > * Returns 0 for success, < 0 for pixel clock not properly detected > > */ > > @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev) > > unsigned int i; > > int ret; > > > > - for (i = 0; i < 100; i++) { > > + for (i = 0; i < 10; i++) { > > ret = max9271_read(dev, 0x15); > > if (ret < 0) > > - return ret; > > + goto skip; > > > > if (ret & MAX9271_PCLKDET) > > return 0; > > > > - usleep_range(50, 100); > > +skip: > > + usleep_range(1000, 1500); > > } > > > > dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n"); > > -- > > 2.33.1 > >
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c index ff86c8c4ea61..aa4add473716 100644 --- a/drivers/media/i2c/max9271.c +++ b/drivers/media/i2c/max9271.c @@ -55,7 +55,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val) /* * max9271_pclk_detect() - Detect valid pixel clock from image sensor * - * Wait up to 10ms for a valid pixel clock. + * Wait up to 15ms for a valid pixel clock. * * Returns 0 for success, < 0 for pixel clock not properly detected */ @@ -64,15 +64,16 @@ static int max9271_pclk_detect(struct max9271_device *dev) unsigned int i; int ret; - for (i = 0; i < 100; i++) { + for (i = 0; i < 10; i++) { ret = max9271_read(dev, 0x15); if (ret < 0) - return ret; + goto skip; if (ret & MAX9271_PCLKDET) return 0; - usleep_range(50, 100); +skip: + usleep_range(1000, 1500); } dev_err(&dev->client->dev, "Unable to detect valid pixel clock\n");