Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2019-12-27 at 15:21 +0300, Andrey Konovalov wrote:
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> 
> Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
> Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
> currently only supports 2 lanes.
> 8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
> @ 30fps are currently supported.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@xxxxxxxxxx>
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1253 insertions(+)
>  create mode 100644 drivers/media/i2c/imx219.c
> 
> 
[..]
> +
> +static int imx219_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct imx219 *imx219;
> +	int ret;
> +
> +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> +	if (!imx219)
> +		return -ENOMEM;
> +
> +	imx219->dev = dev;
> +
> +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> +
> +	/* Get CSI2 bus config */
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +						  NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "Could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	/* Get system clock (xclk) */
> +	imx219->xclk = devm_clk_get(dev, "xclk");
> +	if (IS_ERR(imx219->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(imx219->xclk);
> +	}
> +
> +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret)
> +		return ret;
> +

I think printing an error if you can't regulators
(as the core will stay silent), is a good idea.

Just got bitten by this while trying the driver with
a RPI camera module v2, and the RKISP1 patches that
we recently posted on this mailing list.

(did _very_ limited testing, but working nicely so far).

> +	/* Request optional enable pin */
> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> +						    GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	imx219->sd.internal_ops = &imx219_internal_ops;
> +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> +	if (ret)
> +		goto error_handler_free;
> +
> +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> +	if (ret < 0)
> +		goto error_media_entity;
> +

Ditto on these last two, it's possible to get silent
failures here, which are a bit annoying to debug.

Thanks,
Ezequiel




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux