Hi Laurent
On Mon, 22 Apr 2013, Laurent Pinchart wrote:
> Hi Guennadi,
>
> Thanks for the patch.
>
> On Thursday 18 April 2013 23:35:39 Guennadi Liakhovetski wrote:
> > The mt9p031 driver first accesses the I2C device in its .registered()
> > method. While doing that it furst powers the device up, but if probing
>
> s/furst/first/
>
> > fails, it doesn't power the chip back down. This patch fixes that bug.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >
> > ---
> > drivers/media/i2c/mt9p031.c | 10 ++++++----
> > 1 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > index eb2de22..70f4525 100644
> > --- a/drivers/media/i2c/mt9p031.c
> > +++ b/drivers/media/i2c/mt9p031.c
> > @@ -844,7 +844,7 @@ static int mt9p031_registered(struct v4l2_subdev
> > *subdev) ret = mt9p031_power_on(mt9p031);
> > if (ret < 0) {
> > dev_err(&client->dev, "MT9P031 power up failed\n");
> > - return ret;
> > + goto done;
>
> Not here. If power on fails, there's no need to power off.
Oops, right.
> > }
> >
> > /* Read out the chip version register */
> > @@ -852,13 +852,15 @@ static int mt9p031_registered(struct v4l2_subdev
> > *subdev) if (data != MT9P031_CHIP_VERSION_VALUE) {
> > dev_err(&client->dev, "MT9P031 not detected, wrong version "
> > "0x%04x\n", data);
> > - return -ENODEV;
> > + ret = -ENODEV;
> > }
> >
> > +done:
> > mt9p031_power_off(mt9p031);
> >
> > - dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> > - client->addr);
> > + if (!ret)
> > + dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> > + client->addr);
> >
> > return ret;
> > }
>
> It would be easier to just move the power off line right after the
> mt9p031_read() call and leave the rest unchanged.
Sure, please, do.
Thanks
Guennadi
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 28cf95b..8de84c0 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -849,18 +849,18 @@ static int mt9p031_registered(struct v4l2_subdev
> *subdev)
>
> /* Read out the chip version register */
> data = mt9p031_read(client, MT9P031_CHIP_VERSION);
> + mt9p031_power_off(mt9p031);
> +
> if (data != MT9P031_CHIP_VERSION_VALUE) {
> dev_err(&client->dev, "MT9P031 not detected, wrong version "
> "0x%04x\n", data);
> return -ENODEV;
> }
>
> - mt9p031_power_off(mt9p031);
> -
> dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> client->addr);
>
> - return ret;
> + return 0;
> }
>
> static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh
> *fh)
>
> If you're happy with that there's no need to resubmit, I'll apply the patch to
> my tree for v3.11.
>
> --
> Regards,
>
> Laurent Pinchart
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
