Re: [PATCH v4 2/2] media: i2c: Add imx334 camera sensor driver

Hi Martina,

On Fri, Dec 11, 2020 at 11:51:05AM -0000, Martina Krasteva wrote:

Hi Sakari,

quoted

-----Original Message-----
From: Sakari Ailus <sakari.ailus@linux.intel.com>
Sent: Friday, December 11, 2020 11:32 AM
To: Martina Krasteva <redacted>
Cc: linux-media@vger.kernel.org; mchehab@kernel.org; robh+dt@kernel.org;
devicetree@vger.kernel.org; daniele.alessandrelli@linux.intel.com;
paul.j.murphy@linux.intel.com; gjorgjix.rosikopulos@linux.intel.com
Subject: Re: [PATCH v4 2/2] media: i2c: Add imx334 camera sensor driver

Hi Martina,

On Fri, Dec 11, 2020 at 10:56:33AM +0000, Martina Krasteva wrote:
...

quoted

+static int imx334_read_reg(struct imx334 *imx334, u16 reg, u32 len,
+u32 *val) {
+	struct i2c_client *client = v4l2_get_subdevdata(&imx334->sd);
+	struct i2c_msg msgs[2] = { 0 };
+	u8 addr_buf[2] = { 0 };
+	u8 data_buf[4] = { 0 };
+	int ret;
+
+	if (WARN_ON(len > 4))
+		return -EINVAL;
+
+	put_unaligned_be16(reg, addr_buf);
+
+	/* Write register address */
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].len = ARRAY_SIZE(addr_buf);
+	msgs[0].buf = addr_buf;
+
+	/* Read data from register */
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = len;
+	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_le32(data_buf + (4 - len));

Hmm. The device native endianness is big (on control interface) unless
something very unexpected happened in hardware development.

You also can't do this as this will overrun data_buf.

Imx334 uses little endian so I have to convert the values. 

Ack. That's very unusual, and probably the first raw camera sensor
implemented that way.

Could you fix the data_buf overrun?

-- 
Sakari Ailus