Hi Pavel, On Sat, Nov 11, 2023 at 08:22:07PM +0100, Pavel Machek wrote: > Hi! > > > Addition of support for the Galaxy Core GC2145 XVGA sensor. > > The sensor supports both DVP and CSI-2 interfaces however for > > the time being only CSI-2 is implemented. > > > > Configurations is currently based on initialization scripts > > "are"? Fixed > > > coming from Galaxy Core and for that purpose only 3 static > > "and so"? Fixed > > > resolutions are supported. > > "supported:"? Fixed > > > - 640x480 > > - 1280x720 > > - 1600x1200 > > > > --- /dev/null > > +++ b/drivers/media/i2c/gc2145.c > > @@ -0,0 +1,1404 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * A V4L2 driver for Galaxycore GC2145 camera. > > + * Copyright (C) 2023, STMicroelectronics SA > > + * > > + * Inspired from the imx219.c driver > > "by the"? Fixed > > Link to some kind of datasheet / documentation /... would be welcome > here. Seems an old version of the datasheet is available on pine64.org so I guess I could add a link to this one. http://files.pine64.org/doc/datasheet/PinebookPro/GC2145%20CSP%20DataSheet%20release%20V1.0_20131201.pdf > > > +/** > > + * struct gc2145_mode - GC2145 mode description > > + * @width: frame width (in pixel) > > + * @height: frame height (in pixel) > > "in pixels". Ok > > > +static const struct gc2145_mode supported_modes[] = { > ... > > + { > > + /* 1280x720 30fps mode */ > > + .width = 1280, > > + .height = 720, > > + .reg_seq = gc2145_mode_1280_720_regs, > > + .reg_seq_size = ARRAY_SIZE(gc2145_mode_1280_720_regs), > > + .pixel_rate = GC2145_1280_720_PIXELRATE, > > + .crop = { > > + .top = 160, > > + .left = 240, > > + .width = 1280, > > + .height = 720, > > + }, > > + .hblank = GC2145_1280_720_HBLANK, > > + .vblank = GC2145_1280_720_VBLANK, > > + }, > > Won't this result in 1120x480 mode due to crop? The crop struct indicates the top left corner and width/height so this leads to 720p mode. > > > +/* All supported formats */ > > +static const struct gc2145_format supported_formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > + .code = MEDIA_BUS_FMT_VYUY8_1X16, > > + .code = MEDIA_BUS_FMT_YUYV8_1X16, > > + .code = MEDIA_BUS_FMT_YVYU8_1X16, > > + .code = MEDIA_BUS_FMT_RGB565_1X16, > > +}; > > So ... the hardware can do 10bit ADC, but we don't actually have a > mode exposing that? We don't have YET (in the driver). Choice is to have this first serie with only non-RAW modes. RAW8/10 will be added later on. > > > + * Adjust the MIPI buffer settings. > > + * For YUV/RGB, LWC = image width * 2 > > + * For RAW8, LWC = image width > > + * For RAW10, LWC = image width * 1.25 > > + */ > > + lwc = gc2145->mode->width * 2; > > + cci_write(gc2145->regmap, GC2145_REG_LWC_HIGH, lwc >> 8, &ret); > > + cci_write(gc2145->regmap, GC2145_REG_LWC_LOW, lwc & 0xff, &ret); > > + > > + /* > > + * Adjust the MIPI Fifo Full Level > > Fifo -> FIFO? Ok > > > + /* > > + * Set the fifo gate mode / MIPI wdiv set: > > + * 0xf1 in case of RAW mode and 0xf0 otherwise > > + */ > > fifo -> FIFO? Ok > > > + /* > > + * Datasheet doesn't mention timing between PWDN/RESETB control and > > + * i2c access however experimentation shows that a rather big delay is > > + * needed > > + */ > > "however," "needed." Ok > > > +static const struct v4l2_ctrl_ops gc2145_ctrl_ops = { > > + .s_ctrl = gc2145_s_ctrl, > > +}; > > + > > +/* Initialize control handlers */ > > +static int gc2145_init_controls(struct gc2145 *gc2145) > > +{ > > + ret = v4l2_ctrl_handler_init(hdl, 12); > > + if (ret) > > + return ret; > > + > > + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, > > + GC2145_640_480_PIXELRATE, > > + GC2145_1280_720_PIXELRATE, 1, > > Should the second pixelrate be one from 1600x1200? Indeed. This will actually evolve in the v4 since I implemented instead the V4L2_CID_LINK_FREQ control. > > > +static int gc2145_check_hwcfg(struct device *dev) > > +{ > > + struct fwnode_handle *endpoint; > > + struct v4l2_fwnode_endpoint ep_cfg = { > > + .bus_type = V4L2_MBUS_CSI2_DPHY > > + }; > > + int ret = -EINVAL; > > This "ret" value is unused. Not sure if something will warn about this. Corrected. > > > +MODULE_AUTHOR("Alain Volmat <alain.volmat@xxxxxxxxxxx"); > > ">" is missing at the end of address. Done. > > The driver looks good, thank you! > > Best regards, > Pavel > -- > People of Russia, stop Putin before his war on Ukraine escalates. Regards, Alain
