Re: [PATCH 4.19.y-cip 13/40] media: i2c: Add driver for Sony IMX219 sensor


Pavel Machek
 

Hi!

+/* External clock frequency is 24.0M */
+#define IMX219_XCLK_FREQ 24000000
MHz?

+/*Frame Length Line*/
Make this usual comment style.

+/*
+ * Initialisation delay between XCLR low->high and the moment when the sensor
+ * can start capture (i.e. can leave software stanby) must be not less than:
+ * t4 + max(t5, t6 + <time to initialize the sensor register over I2C>)
+ * where
+ * t4 is fixed, and is max 200uS,
+ * t5 is fixed, and is 6000uS,
...
+ * 1185 to 5333 uS, and is always less than t5.
...
+ * For this reason this is always safe to wait (t4 + t5) = 6200 uS, then
I believe this should be "us" or "usec".

+ /*
+ * Mutex for serialized access:
+ * Protect sensor module set pad format and start/stop streaming safely.
+ */
This is not really english.

+/* Read registers up to 2 at a time */
+static int imx219_read_reg(struct imx219 *imx219, u16 reg, u32 len, u32 *val)
+{
+ msgs[1].buf = &data_buf[4 - len];
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret != ARRAY_SIZE(msgs))
+ return -EIO;
+
+ *val = get_unaligned_be32(data_buf);
That's really interesting piece of code. len is in bytes, but return
value is in 32 bit values, magic with be32...

+static int imx219_write_reg(struct imx219 *imx219, u16 reg, u32 len, u32 val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+ u8 buf[6];
+
+ if (len > 4)
+ return -EINVAL;
+
+ put_unaligned_be16(reg, buf);
+ put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
Wow. When I thought above was interesting.... this is even more so.


+static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
+{
...
+ /*
+ * Applying V4L2 control value only happens
+ * when power is up for streaming
+ */
+ if (pm_runtime_get_if_in_use(&client->dev) == 0)
+ return 0;
Is this usual for other cameras? Will not it cause problems to
applications doing autogain?

+ switch (ctrl->id) {
...
+ case V4L2_CID_HFLIP:
+ case V4L2_CID_VFLIP:
+ ret = imx219_write_reg(imx219, IMX219_REG_ORIENTATION, 1,
+ imx219->hflip->val |
+ imx219->vflip->val << 1);
+ break;
ctrl->val is unused in this case?

+ break;
+ default:
+ dev_info(&client->dev,
+ "ctrl(id:0x%x,val:0x%x) is not handled\n",
+ ctrl->id, ctrl->val);
+ ret = -EINVAL;
+ break;
+ }
Rate limit this as user can trigger these?

+static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct imx219 *imx219 = to_imx219(sd);
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ int ret = 0;
+
+ mutex_lock(&imx219->mutex);
+ if (imx219->streaming == enable) {
+ mutex_unlock(&imx219->mutex);
+ return 0;
+ }
+
+ if (enable) {
+ ret = pm_runtime_get_sync(&client->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&client->dev);
+ goto err_unlock;
+ }
+
+ /*
+ * Apply default & customized values
+ * and then start streaming.
+ */
+ ret = imx219_start_streaming(imx219);
+ if (ret)
+ goto err_rpm_put;
Is it correct/vital that one error path uses put_noidle() and second
uses plain put()?

+static int __maybe_unused imx219_resume(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;
+
+ if (imx219->streaming) {
+ ret = imx219_start_streaming(imx219);
+ if (ret)
+ goto error;
+ }
+
+ return 0;
+
+error:
+ imx219_stop_streaming(imx219);
+ imx219->streaming = 0;
This error path will leave driver in inconsistent state. hflip/vflip
controls will be locked and pm_runtime_put() is not executed.

+static int imx219_check_hwcfg(struct device *dev)
+{
+ struct v4l2_fwnode_endpoint *ep_cfg;
+ struct device_node *ep;
+ int ret = -EINVAL;
+
+ ep = of_graph_get_next_endpoint(dev->of_node, NULL);
+ if (!ep) {
+ dev_err(dev, "missing endpoint node\n");
+ return -EINVAL;
+ }
+
+ ep_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(ep));
+ if (IS_ERR(ep_cfg)) {
+ dev_err(dev, "could not parse endpoint\n");
+ goto error_out;
+ }
...
+error_out:
+ v4l2_fwnode_endpoint_free(ep_cfg);
Is it correct to call endpoint_free on error pointer?

+MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
Missing ">" at end of address.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Join cip-dev@lists.cip-project.org to automatically receive all group messages.