Hi! > 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. > > Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx> Reviewed-by: Pavel Machek <pavel@xxxxxx> Some nitpicks below.. > +config VIDEO_HI846 > + tristate "Hynix Hi-846 sensor support" > + depends on I2C && VIDEO_V4L2 > + select MEDIA_CONTROLLER > + select VIDEO_V4L2_SUBDEV_API > + select V4L2_FWNODE > + help > + This is a Video4Linux2 sensor driver for the Hynix > + Hi-846 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called hi846. hi846.ko? > +/* Frame length lines / Vertical timings */ vertical > +/* > + * Long exposure time. Actually, exposure is a 20 bit value that > + * includes the lower 4 bits of 0x0073 too. only 16 bit are used > + * right now > + */ Only 16 bits now. > +struct hi846_mode { > + /* Frame width in pixels */ > + u32 width; > + > + /* Frame height in pixels */ > + u32 height; > + > + /* Horizontal timing size */ > + u32 llp; > + > + /* Link frequency needed for this resolution */ > + u8 link_freq_index; > + > + u16 fps; > + > + /* vertical timining size */ Vertical timing > + /* position inside of the 3264x2448 pixel array */ Position > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&hi846->sd); > + struct i2c_msg msgs[2]; > + u8 addr_buf[2]; > + u8 data_buf[1] = {0}; > + int ret; > + > + put_unaligned_be16(reg, addr_buf); > + msgs[0].addr = client->addr; > + msgs[0].flags = 0; > + msgs[0].len = sizeof(addr_buf); > + msgs[0].buf = addr_buf; > + msgs[1].addr = client->addr; > + msgs[1].flags = I2C_M_RD; > + msgs[1].len = 1; > + msgs[1].buf = &data_buf[0]; = data_buf; To simplify things and for consistency with above. > +static void hi846_write_reg_16(struct hi846 *hi846, u16 reg, u16 val, int *err) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&hi846->sd); > + u8 buf[6]; > + int ret; > + > + if (*err < 0) > + return; > + > + put_unaligned_be16(reg, buf); > + put_unaligned_be32(val << 8 * 2, buf + 2); Is that obfuscated way of saying put_unaligned_be16(val, buf+2); buf[3] = 0; buf[4] = 0; ? > +static int hi846_set_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct hi846 *hi846 = to_hi846(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + int ret = 0; > + > + if (hi846->streaming == enable) > + return 0; > + > + mutex_lock(&hi846->mutex); > + > + if (enable) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + goto out; > + } > + > + ret = hi846_start_streaming(hi846); > + if (ret) { > + enable = 0; > + hi846_stop_streaming(hi846); > + pm_runtime_put(&client->dev); > + } > + } else { > + hi846_stop_streaming(hi846); > + pm_runtime_put(&client->dev); > + } enable = 0 is dead code. Could this be written as } if (!enable || ret) { stop_streaming() put() } But I guess that start_streaming should really do all the cleanup itself if it fails. > + ret = hi846_identify_module(hi846); > + if (ret) > + goto probe_error_power_off; Does this go to right place? > + hi846->cur_mode = &supported_modes[0]; > + > + ret = hi846_init_controls(hi846); > + if (ret) { > + dev_err(&client->dev, "failed to init controls: %d", ret); > + return ret; > + } This should go to cleanup code somewhere. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
