mbox series

[v3,0/3] media: i2c: gc2145: GC2145 sensor support

Message ID 20231107081345.3172392-1-alain.volmat@foss.st.com
Headers show
Series media: i2c: gc2145: GC2145 sensor support | expand

Message

Alain Volmat Nov. 7, 2023, 8:13 a.m. UTC 
This serie adds support for the GalaxyCore GC2145 sensor.
Initialization is based on scripts provided by GalaxyCore,
allowing 3 fixed configurations supported for the time being.

Minimum controls have been added in order to allow it to
be successfully used with libcamera and dcmipp driver (under
review) on STM32MP13 platform.

The sensor also supports Bayer formats however I removed
them for the time being since they would require more
controls to be exposed.  They will be added again later on.

v3: - fix copyright year
    - add more gc2145_ func or variable prefixes
    - removal of colorspace within struct gc2145_format
    - use 1X16 formats since driver is supporting CSI-2 interface only
    - use msleep instead of usleep_range
    - add pm_runtime_ last_busy & autosuspend handling
    - only start streaming AFTER applying ctrls
    - correct RGB565 format to generate _LE instead of _BE
    - perform pm_runtime configuration prior to v4l2_async_register_subdev_sensor
    - remove frame_interval handling and expose HBLANK/VBLANK ctrls
      instead

v2: - split vendor-prefixes update into a separate commit
    - correction into dt-bindings (description, const i2c address,
      lowcase of supplies, node naming
    - correct KConfig (avoid VIDEO_V4L2_SUBDEV_API, V4L2_FWNODE and add
      V4L2_CCI_I2C)
    - usage of v4l2-cci framework for accessing to device registers
    - correction in power-on / power-off sequences and update
      usleep_range delay
    - detail MIPI configuration and stop register access to stop
      streaming
    - removal of SYSTEM_SLEEP_PM_OPS (and streaming variable)
    - various small fixes, typo, oneline functions
    - removal of useless HBLANK control considereing that RAW isn't
      supported for the time being
    - removal of RAW parts
    - usage of dev_err_probe

Alain Volmat (3):
  dt-bindings: vendor-prefixes: Add prefix for GalaxyCore Inc.
  dt-bindings: media: i2c: add galaxycore,gc2145 dt-bindings
  media: i2c: gc2145: Galaxy Core GC2145 sensor support

 .../bindings/media/i2c/galaxycore,gc2145.yaml |  104 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 MAINTAINERS                                   |    8 +
 drivers/media/i2c/Kconfig                     |   10 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/gc2145.c                    | 1404 +++++++++++++++++
 6 files changed, 1529 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
 create mode 100644 drivers/media/i2c/gc2145.c

Comments

Pavel Machek Nov. 11, 2023, 7:06 p.m. UTC | #1 
Hi!

> This serie adds support for the GalaxyCore GC2145 sensor.
> Initialization is based on scripts provided by GalaxyCore,
> allowing 3 fixed configurations supported for the time being.

There is another version of the driver, from Ondrej Jirman, floating
around. See mail "gc2145 camera driver (front camera on
PinePhone)". I've added some cc-s in case people wanted to comment on
it.

Best regards,
								Pavel
Pavel Machek Nov. 11, 2023, 7:22 p.m. UTC | #2 
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
Alain Volmat Nov. 17, 2023, 9:02 a.m. UTC | #3 
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@foss.st.com");
> 
> ">" 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
Pavel Machek Dec. 16, 2023, 1:41 p.m. UTC | #4 
Hi!

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

Thanks for all the fixes and all the work you are putting to this.

Best regards,
								Pavel