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


Lad Prabhakar
 

Hi Pavel,

Thank you for the review.

-----Original Message-----
From: Pavel Machek <pavel@denx.de>
Sent: 09 March 2021 20:37
To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek
<pavel@denx.de>; Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH 4.19.y-cip 13/40] media: i2c: Add driver for Sony IMX219 sensor

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?
Above check is to see if the camera is turned ON, so the application doing autogain should be fine.

+ 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?
Yes, for the H/V flip it depends on the format code set.

+ 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?
Yes can be triggered.

+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()?
As per my reading https://patchwork.kernel.org/project/linux-omap/patch/20180518173008.73291-2-tony@atomide.com/ its OK.

+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.
Good catch, this needs fixing in upstream.

+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?
Yes the core handles it.

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

Cheers,
Prabhakar

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.