| Message ID | 20210831134344.1673318-1-martin.kepplinger@puri.sm |
|---|---|
| Headers | show |
| Series | Add support for the Hynix Hi-846 camera | expand |
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@puri.sm> Reviewed-by: Pavel Machek <pavel@ucw.cz> 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
Am Freitag, dem 03.09.2021 um 18:17 +0200 schrieb Pavel Machek: > 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@puri.sm> > > Reviewed-by: Pavel Machek <pavel@ucw.cz> > > 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? hi Pavel, thanks a lot for taking the time! the module name doesn't include the file ending, according to all other Kconfig descriptions. the rest looks good and I'll change it according to your review. martin
Hi Pavel, Martin, On Fri, Sep 03, 2021 at 06:17:43PM +0200, Pavel Machek wrote: > > +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; ? Good catch. The buf should be only four u8's long, and you should use 16-bit variant here, too. Also the transfer should be done on sizeof(buf), not 4 (which indeed is the same, but cleaner).
hi, This series adds support for the SK Hynix Hi-846 CMOS images sensor. It includes the dt-bindings and the driver. best wishes, martin revision history ---------------- v8: (thank you Laurent) * add fwnode properties support for orientation and rotation on the board * remove the arm64 defconfig addition patch. deal with that later. v7: (thank you Sakari) * move the check for supported nr_lanes for a mode to set_format() * change get_selection() according to the Sakaris' review * check for the mipi link frequencies from DT * check for the external clock rate and add assigned-clock-rates requirement * https://lore.kernel.org/linux-media/20210712084137.3779628-1-martin.kepplinger@puri.sm/ v6: * better digital gain defaults * lane config fix found by smatch * fix regulator usage in power_on() * https://lore.kernel.org/linux-media/20210628101054.828579-1-martin.kepplinger@puri.sm/ v5: (thank you Laurent and Rob) * minor dt-bindings fixes * driver: disable lens shading correcting (no seed values yet used from "otp" for that) * add reviewed-tags * https://lore.kernel.org/linux-media/20210611101404.2553818-1-martin.kepplinger@puri.sm/ v4: (thank you Laurent, Sakari and Rob) many driver changes, see v3 review for details. they include: * add get_selection(), remove open() callback * use gpiod API * use regulator_bulk API * fix power supply timing sequence and bindings * https://lore.kernel.org/linux-media/20210607105213.1211722-1-martin.kepplinger@puri.sm/ v3: (thank you, Laurent) * use do_div() for divisions * reset-gpios DT property name instead of rst-gpios * improve the dt-bindings * add the phone-devel list * https://lore.kernel.org/linux-media/20210531120737.168496-1-martin.kepplinger@puri.sm/ v2: sent a bit early due to stupid mistakes * fix build issues * fix dtschema issues * add enable for arm64 defconfig * https://lore.kernel.org/linux-media/20210530212337.GA15366@duo.ucw.cz/T/#t v1: * https://lore.kernel.org/linux-media/20210527091221.3335998-1-martin.kepplinger@puri.sm/ Martin Kepplinger (4): dt-bindings: vendor-prefixes: Add SK Hynix Inc. dt-bindings: media: document SK Hynix Hi-846 MIPI CSI-2 8M pixel sensor media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera Documentation: i2c-cardlist: add the Hynix hi846 sensor .../admin-guide/media/i2c-cardlist.rst | 1 + .../bindings/media/i2c/hynix,hi846.yaml | 120 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 6 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/hi846.c | 2190 +++++++++++++++++ 7 files changed, 2333 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml create mode 100644 drivers/media/i2c/hi846.c