Hi Sakari,
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Friday, December 11, 2020 11:32 AM
> To: Martina Krasteva <martinax.krasteva@xxxxxxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; daniele.alessandrelli@xxxxxxxxxxxxxxx;
> paul.j.murphy@xxxxxxxxxxxxxxx; gjorgjix.rosikopulos@xxxxxxxxxxxxxxx
> 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:
> ...
> > +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.
>
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * imx334_write_reg() - Write register
> > + * @imx334: pointer to imx334 device
> > + * @reg: register address
> > + * @len: length of bytes. Max supported bytes is 4
> > + * @val: register value
> > + *
> > + * Return: 0 if successful, error code otherwise.
> > + */
> > +static int imx334_write_reg(struct imx334 *imx334, u16 reg, u32 len,
> > +u32 val) {
> > + struct i2c_client *client = v4l2_get_subdevdata(&imx334->sd);
> > + u8 buf[6] = {0};
> > +
> > + if (WARN_ON(len > 4))
> > + return -EINVAL;
> > +
> > + put_unaligned_be16(reg, buf);
> > + put_unaligned_le32(val, buf + 2);
>
> Similar comment on this one.
>
> --
> Kind regards,
>
> Sakari Ailus
