Hi Sakari,
Sorry for delayed reply..
(your email got into a wrong folder, and I might not find it there if Ezequiel
did not warn me that I didn't address the comments from your review)
On 27.12.2019 17:55, Sakari Ailus wrote:
Hi Andrey,
On Fri, Dec 27, 2019 at 03:21:14PM +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
<snip>
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
new file mode 100644
index 000000000000..28b55c61cd77
--- /dev/null
+++ b/drivers/media/i2c/imx219.c
<snip>
+/* Power/clock management functions */
+static int imx219_power_on(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx219 *imx219 = to_imx219(sd);
+ int ret;
+
+ ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
+ imx219->supplies);
+ if (ret) {
+ dev_err(&client->dev, "%s: failed to enable regulators\n",
+ __func__);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(imx219->xclk);
+ if (ret) {
+ dev_err(&client->dev, "%s: failed to enable clock\n",
+ __func__);
+ goto reg_off;
+ }
+
+ gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
+ msleep(IMX219_XCLR_DELAY_MS);
I guess 10 ms is ok albeit it'd be nicer if you calculated the required
delay instead.
I think this 10 ms delay actually serves two purposes here. It is
not only the delay after XCLR pin is set high (reset de-asserted),
but it also lets the camera power supplies voltages to settle after
regulator_bulk_enable() called few lines above.
So I would make the delay a sum of 1) the value which depends on
input clock frequency (the driver currently supports 24MHz only)
and 2) a fixed value of 8 ms or so to account for the power supplies
settle time. So that the sum would be the same 10 ms for 24MHz input
clock.
Does it makes sense?
<snip>
+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;
Instead of putting a dev field to struct imx219, you can find the device in
struct i2c_client.dev, which you can get by:
v4l2_get_subdevdata(imx219->sd)
+
+ 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;
+ }
Could you also check the link frequencies (the link-frequencies property
that also should be added to DT bindings) matches with what is possible
with the given xclk frequency? Please see e.g. imx319 driver for an
example.
The driver only supports single xclk frequency and single link-frequency
(hardcoded in the registers value tables). So the check will be more like
the one in imx290 driver (error out if the link-frequency property isn't
equal to the pre#define-d default value).
+
+ ret = imx219_get_regulators(imx219);
+ if (ret)
+ return ret;
+
+ /* 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;
+
+ /* Enable runtime PM and turn off the device */
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_runtime_idle(dev);
+
+ return 0;
+
+error_media_entity:
+ media_entity_cleanup(&imx219->sd.entity);
+
+error_handler_free:
+ imx219_free_controls(imx219);
+
+error_power_off:
+ imx219_power_off(dev);
+
+ return ret;
+}
+
+static int imx219_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx219 *imx219 = to_imx219(sd);
+
+ v4l2_async_unregister_subdev(sd);
+ media_entity_cleanup(&sd->entity);
+ imx219_free_controls(imx219);
+
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
imx219_power_off(), if the sensor is powered on here. Please see e.g. the
smiapp driver regarding this.
It should be powered off here.
The sensor is powered on when streaming is started, and is powered off when
it is stopped: if the enable argument is false, imx219_set_stream() calls
pm_runtime_put().
IOW, it follows the imx319 driver as the example of power management.
+
+ return 0;
+}
+
+static const struct of_device_id imx219_dt_ids[] = {
+ { .compatible = "sony,imx219" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx219_dt_ids);
+
+static const struct dev_pm_ops imx219_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
+ SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
+};
+
+static struct i2c_driver imx219_i2c_driver = {
+ .driver = {
+ .name = "imx219",
+ .of_match_table = imx219_dt_ids,
+ .pm = &imx219_pm_ops,
+ },
+ .probe = imx219_probe,
Could you use .probe_new, and remove the i2c_device_id argument?
I'll fix this and all the other issues I didn't comment on in this email
in the v4 of the patch set.
Thanks,
Andrey
+ .remove = imx219_remove,
+};
+
+module_i2c_driver(imx219_i2c_driver);
+
+MODULE_AUTHOR("Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx");
+MODULE_DESCRIPTION("Sony IMX219 sensor driver");
+MODULE_LICENSE("GPL v2");
