Re: [PATCH v3 3/3] media: i2c: gc2145: Galaxy Core GC2145 sensor support

[Alain Volmat]

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