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: > > On Mon, May 31, 2021 at 02:07:35PM +0200, Martin Kepplinger wrote: > > > 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 :-) > > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx> > > > --- > > > 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
