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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3700 bytes --]

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"?

> coming from Galaxy Core and for that purpose only 3 static

"and so"?

> resolutions are supported.

"supported:"?

>  - 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"?

Link to some kind of datasheet / documentation /... would be welcome
here.

> +/**
> + * struct gc2145_mode - GC2145 mode description
> + * @width: frame width (in pixel)
> + * @height: frame height (in pixel)

"in pixels".

> +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?

> +/* 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?

> +	 * 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?

> +	/*
> +	 * Set the fifo gate mode / MIPI wdiv set:
> +	 * 0xf1 in case of RAW mode and 0xf0 otherwise
> +	 */

fifo -> FIFO?

> +	/*
> +	 * Datasheet doesn't mention timing between PWDN/RESETB control and
> +	 * i2c access however experimentation shows that a rather big delay is
> +	 * needed
> +	 */

"however," "needed."

> +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?

> +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.

> +MODULE_AUTHOR("Alain Volmat <alain.volmat@foss.st.com");

">" is missing at the end of address.

The driver looks good, thank you!

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]