Hi Martin,

On Wed, Jun 02, 2021 at 02:00:11PM +0200, Martin Kepplinger wrote:

Am Dienstag, dem 01.06.2021 um 03:14 +0300 schrieb Laurent Pinchart:

quoted

On Mon, May 31, 2021 at 02:07:35PM +0200, Martin Kepplinger wrote:

quoted

The SK Hynix Hi-846 is a 1/4" 8M Pixel CMOS Image Sensor. It supports
usual features like I2C control, CSI-2 for frame data, digital/analog
gain control or test patterns.

This driver supports the 640x480, 1280x720 and 1632x1224 resolution
modes. It supports runtime PM in order not to draw any unnecessary
power.

The part is also called YACG4D0C9SHC and a datasheet can be found at
https://product.skhynix.com/products/cis/cis.go

The large sets of partly undocumented register values are for example
found when searching for the hi846_mipi_raw_Sensor.c Android driver.

A common story, unfortunately :-S

I've done an initial review, I'll likely have more comments on v4, but
you should have quite a few things to address already :-)

quoted

Signed-off-by: Martin Kepplinger <redacted>
---
 MAINTAINERS                |    6 +
 drivers/media/i2c/Kconfig  |   13 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/hi846.c  | 2138 ++++++++++++++++++++++++++++++++++++
 4 files changed, 2158 insertions(+)
 create mode 100644 drivers/media/i2c/hi846.c

[snip]

Thank you, Laurent for that wonderful review. It made me rework/fix the
power supply interface + sequencing in the driver (and even better
understand how it's supplied on my board).

I want to take all your review into account for a next revision, except
for the additional bits for the register definitions, that should
encode the length, if that's ok. We can choose whether to write 1 or 2
bytes at a given address and it just looks nice and simple to me as it
is.

I won't push strongly, but in my experience it's error-prone, as it's
easy to select the incorrect number of bytes. That's what led me to
create this mechanism to bundle register addresses and sizes, it has
simplified my life when writing drivers. I think it should actually be
turned into a helper, possibly provided by regmap.

-- 
Regards,

Laurent Pinchart