On Wed, 23 May 2012, Laurent Pinchart wrote:
> Several client drivers access the hardware at probe time, for instance
> to read the probe chip ID. Such chips need to be powered up when being
> probed.
>
> soc-camera handles this by powering chips up in the soc-camera probe
> implementation. However, this will break with non soc-camera hosts that
> don't perform the same operations.
>
> Fix the problem by pushing the power up/down from the soc-camera core
> down to individual drivers on a needs basis.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/video/imx074.c | 21 ++++++++--
> drivers/media/video/mt9m001.c | 17 +++++++-
> drivers/media/video/mt9m111.c | 80 +++++++++++++++++++++----------------
> drivers/media/video/mt9t031.c | 18 ++++++---
> drivers/media/video/mt9t112.c | 12 +++++-
> drivers/media/video/mt9v022.c | 5 ++
> drivers/media/video/ov2640.c | 11 ++++-
> drivers/media/video/ov5642.c | 21 ++++++++--
> drivers/media/video/ov6650.c | 19 ++++++---
> drivers/media/video/ov772x.c | 14 ++++++-
> drivers/media/video/ov9640.c | 17 ++++++--
> drivers/media/video/ov9740.c | 23 +++++++----
> drivers/media/video/rj54n1cb0c.c | 18 ++++++--
> drivers/media/video/soc_camera.c | 14 -------
> drivers/media/video/tw9910.c | 12 +++++-
> 15 files changed, 201 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/media/video/imx074.c b/drivers/media/video/imx074.c
> index 1166c89..fc86e68 100644
> --- a/drivers/media/video/imx074.c
> +++ b/drivers/media/video/imx074.c
> @@ -313,26 +313,33 @@ static struct v4l2_subdev_ops imx074_subdev_ops = {
>
> static int imx074_video_probe(struct i2c_client *client)
> {
> + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> int ret;
> u16 id;
>
> + ret = imx074_s_power(subdev, 1);
> + if (ret < 0)
> + return ret;
> +
> /* Read sensor Model ID */
> ret = reg_read(client, 0);
> if (ret < 0)
> - return ret;
> + goto done;
>
> id = ret << 8;
>
> ret = reg_read(client, 1);
> if (ret < 0)
> - return ret;
> + goto done;
>
> id |= ret;
>
> dev_info(&client->dev, "Chip ID 0x%04x detected\n", id);
>
> - if (id != 0x74)
> - return -ENODEV;
> + if (id != 0x74) {
> + ret = -ENODEV;
> + goto done;
> + }
>
> /* PLL Setting EXTCLK=24MHz, 22.5times */
> reg_write(client, PLL_MULTIPLIER, 0x2D);
> @@ -414,7 +421,11 @@ static int imx074_video_probe(struct i2c_client *client)
>
> reg_write(client, GROUPED_PARAMETER_HOLD, 0x00); /* off */
Looking at this and other soc-camera sensor drivers, most of them do some
initialisation during probe(), that is not automatically re-applied at any
point during operation. This means, the current power switching in
soc-camera core, turning clients off directly after probe() and after each
last close() only works with "soft" power off variants, e.g., where the
board only switches off analog parts of a camera sensor and preserves
register contents. There are indeed multiple boards currently in the
mainline, implementing the soc_camera_link::power() callback. This means,
they all either only do the soft power-off, or have been lucky to not need
any of the lost register contents.
The v4l2_subdev_core_ops::s_power() operation is documented as
s_power: puts subdevice in power saving mode (on == 0) or normal operation
mode (on == 1).
"power saving mode" means pretty much the same to me - switch off power
consuming parts, but keep register contents. So, I think, we're fine here
just "mechanically" bringing over the power switching functionality into
client drivers, we might only want to improve struct soc_camera_link
documentation :-)
>
> - return 0;
> + ret = 0;
> +
> +done:
> + imx074_s_power(subdev, 0);
> + return ret;
> }
>
> static int imx074_probe(struct i2c_client *client,
[snip]
> @@ -940,6 +905,51 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
> .video = &mt9m111_subdev_video_ops,
> };
>
> +/*
> + * Interface active, can use i2c. If it fails, it can indeed mean, that
> + * this wasn't our capture interface, so, we wait for the right one
> + */
> +static int mt9m111_video_probe(struct i2c_client *client)
> +{
> + struct mt9m111 *mt9m111 = to_mt9m111(client);
> + s32 data;
> + int ret;
> +
> + ret = mt9m111_s_power(&mt9m111->subdev, 1);
> + if (ret < 0)
> + return ret;
> +
> + data = reg_read(CHIP_VERSION);
> +
> + switch (data) {
> + case 0x143a: /* MT9M111 or MT9M131 */
> + mt9m111->model = V4L2_IDENT_MT9M111;
> + dev_info(&client->dev,
> + "Detected a MT9M111/MT9M131 chip ID %x\n", data);
> + break;
> + case 0x148c: /* MT9M112 */
> + mt9m111->model = V4L2_IDENT_MT9M112;
> + dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
> + break;
> + default:
> + dev_err(&client->dev,
> + "No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
> + data);
> + ret = -ENODEV;
> + goto done;
> + }
> +
> + ret = mt9m111_init(mt9m111);
> + if (ret)
> + goto done;
> +
> + ret = v4l2_ctrl_handler_setup(&mt9m111->hdl);
You're losing this return code...
> +
> +done:
> + ret = mt9m111_s_power(&mt9m111->subdev, 0);
> + return ret;
return mt9m111_s_power(&mt9m111->subdev, 0);
But in mt9m001 you discard return code from *_s_power(0) and return the
error from v4l2_ctrl_handler_setup(). Better be consistent, IMHO.
> +}
> +
> static int mt9m111_probe(struct i2c_client *client,
> const struct i2c_device_id *did)
> {
> diff --git a/drivers/media/video/mt9t031.c b/drivers/media/video/mt9t031.c
> index 9666e20..56dd31c 100644
> --- a/drivers/media/video/mt9t031.c
> +++ b/drivers/media/video/mt9t031.c
> @@ -643,6 +643,12 @@ static int mt9t031_video_probe(struct i2c_client *client)
> s32 data;
> int ret;
>
> + ret = mt9t031_s_power(&mt9t031->subdev, 1);
> + if (ret < 0)
> + return ret;
> +
> + mt9t031_idle(client);
> +
There's one more call to mt9t031_idle() a couple of lines down in
mt9t031_video_probe()... I think, the latter one can be dropped
together with...
> /* Enable the chip */
> data = reg_write(client, MT9T031_CHIP_ENABLE, 1);
the one above - it starts the read-out, which we don't necessarily want
immediately after probe(), and it is anyway disabled again a few lines
further down in mt9t031_disable().
Thanks
Guennadi
---
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
