Message-ID: <d6bbd959-93fc-4374-af60-310859b266dc@collabora.com>
Date: Wed, 5 Jun 2024 14:08:05 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Zhi Mao <zhi.mao@...iatek.com>, mchehab@...nel.org, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, sakari.ailus@...ux.intel.com
Cc: laurent.pinchart@...asonboard.com, shengnan.wang@...iatek.com,
 yaya.chang@...iatek.com, Project_Global_Chrome_Upstream_Group@...iatek.com,
 yunkec@...omium.org, conor+dt@...nel.org, matthias.bgg@...il.com,
 jacopo.mondi@...asonboard.com, 10572168@...com, hverkuil-cisco@...all.nl,
 heiko@...ech.de, jernej.skrabec@...il.com, macromorgan@...mail.com,
 linus.walleij@...aro.org, hdegoede@...hat.com,
 tomi.valkeinen@...asonboard.com, gerald.loacker@...fvision.net,
 andy.shevchenko@...il.com, bingbu.cao@...el.com,
 dan.scally@...asonboard.com, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v5 2/3] media: i2c: Add GC05A2 image sensor driver

Il 05/06/24 12:55, Zhi Mao ha scritto:
> Add a V4L2 sub-device driver for Galaxycore GC05A2 image sensor.
> 
> Signed-off-by: Zhi Mao <zhi.mao@...iatek.com>
> ---
>   drivers/media/i2c/Kconfig  |   10 +
>   drivers/media/i2c/Makefile |    1 +
>   drivers/media/i2c/gc05a2.c | 1361 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 1372 insertions(+)
>   create mode 100644 drivers/media/i2c/gc05a2.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c6d3ee472d81..4e7c71c95143 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -70,6 +70,16 @@ config VIDEO_GC0308
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called gc0308.
>   
> +config VIDEO_GC05A2
> +	tristate "GalaxyCore gc05a2 sensor support"
> +	select V4L2_CCI_I2C
> +	help
> +	  This is a Video4Linux2 sensor driver for the GalaxyCore gc05a2
> +	  camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called gc05a2.
> +
>   config VIDEO_GC2145
>   	select V4L2_CCI_I2C
>   	tristate "GalaxyCore GC2145 sensor support"
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index dfbe6448b549..8ed6faf0f854 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>   obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>   obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
>   obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
> +obj-$(CONFIG_VIDEO_GC05A2) += gc05a2.o
>   obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
>   obj-$(CONFIG_VIDEO_HI556) += hi556.o
>   obj-$(CONFIG_VIDEO_HI846) += hi846.o
> diff --git a/drivers/media/i2c/gc05a2.c b/drivers/media/i2c/gc05a2.c
> new file mode 100644
> index 000000000000..87a209b27fc2
> --- /dev/null
> +++ b/drivers/media/i2c/gc05a2.c
> @@ -0,0 +1,1361 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for GalaxyCore gc05a2 image sensor
> + *
> + * Copyright 2024 MediaTek
> + *
> + * Zhi Mao <zhi.mao@...iatek.com>
> + */
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define GC05A2_REG_TEST_PATTERN_EN CCI_REG8(0x008c)
> +#define GC05A2_REG_TEST_PATTERN_IDX CCI_REG8(0x008d)
> +#define GC05A2_TEST_PATTERN_EN 0x01
> +
> +#define GC05A2_STREAMING_REG CCI_REG8(0x0100)
> +
> +#define GC05A2_FLIP_REG CCI_REG8(0x0101)
> +#define GC05A2_FLIP_H_MASK BIT(0)
> +#define GC05A2_FLIP_V_MASK BIT(1)
> +
> +#define GC05A2_EXP_REG CCI_REG16(0x0202)
> +#define GC05A2_EXP_MARGIN 16
> +#define GC05A2_EXP_MIN 4
> +#define GC05A2_EXP_STEP 1
> +
> +#define GC05A2_AGAIN_REG CCI_REG16(0x0204)
> +#define GC05A2_AGAIN_MIN 1024
> +#define GC05A2_AGAIN_MAX (1024 * 16)
> +#define GC05A2_AGAIN_STEP 1
> +
> +#define GC05A2_FRAME_LENGTH_REG CCI_REG16(0x0340)
> +#define GC05A2_VTS_MAX 0xffff
> +
> +#define GC05A2_REG_CHIP_ID CCI_REG16(0x03f0)
> +#define GC05A2_CHIP_ID 0x05a2
> +
> +#define GC05A2_NATIVE_WIDTH 2592
> +#define GC05A2_NATIVE_HEIGHT 1944
> +
> +#define GC05A2_DEFAULT_CLK_FREQ (24 * HZ_PER_MHZ)
> +#define GC05A2_MBUS_CODE MEDIA_BUS_FMT_SGRBG10_1X10
> +#define GC05A2_DATA_LANES 2
> +#define GC05A2_RGB_DEPTH 10
> +#define GC05A2_SLEEP_US  (2 * USEC_PER_MSEC)
> +

..snip..

> +static int gc05a2_parse_fwnode(struct gc05a2 *gc05a2)
> +{
> +	struct fwnode_handle *endpoint;
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};
> +	int ret;
> +	struct device *dev = gc05a2->dev;
> +
> +	endpoint =
> +		fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +						FWNODE_GRAPH_ENDPOINT_NEXT);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");

This function is used only in probe stage, and logging should be consistent.
Check below :-)

> +		return -EINVAL;
> +	}
> +

..snip..

> +
> +static int gc05a2_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct gc05a2 *gc05a2;
> +	int ret;
> +
> +	gc05a2 = devm_kzalloc(dev, sizeof(*gc05a2), GFP_KERNEL);
> +	if (!gc05a2)
> +		return -ENOMEM;
> +
> +	gc05a2->dev = dev;
> +
> +	ret = gc05a2_parse_fwnode(gc05a2);
> +	if (ret)
> +		return ret;
> +
> +	gc05a2->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(gc05a2->regmap))
> +		return dev_err_probe(dev, PTR_ERR(gc05a2->regmap),
> +				     "failed to init CCI\n");
> +
> +	gc05a2->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(gc05a2->xclk))
> +		return dev_err_probe(dev, PTR_ERR(gc05a2->xclk),
> +				     "failed to get xclk\n");
> +
> +	ret = clk_set_rate(gc05a2->xclk, GC05A2_DEFAULT_CLK_FREQ);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set xclk frequency\n");
> +
> +	ret = gc05a2_get_regulators(dev, gc05a2);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get regulators\n");
> +
> +	gc05a2->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(gc05a2->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(gc05a2->reset_gpio),
> +				     "failed to get gpio\n");
> +
> +	v4l2_i2c_subdev_init(&gc05a2->sd, client, &gc05a2_subdev_ops);
> +	gc05a2->sd.internal_ops = &gc05a2_internal_ops;
> +	gc05a2->cur_mode = &gc05a2_modes[0];
> +
> +	ret = gc05a2_init_controls(gc05a2);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to init controls\n");
> +
> +	gc05a2->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
> +	gc05a2->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	gc05a2->sd.dev = &client->dev;
> +	gc05a2->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	ret = media_entity_pads_init(&gc05a2->sd.entity, 1, &gc05a2->pad);
> +	if (ret < 0) {
> +		dev_err(dev, "could not register media entity\n");

For logging strings consistency, you should use dev_err_probe() everywhere,
at this point, because that's printing the error code.

So here it'd be

if (ret < 0) {
	dev_err_probe(dev, ret, "could not register media entity\n");
	goto err_v4l2_ctrl_handler_free;
}

> +		goto err_v4l2_ctrl_handler_free;
> +	}
> +
> +	gc05a2->sd.state_lock = gc05a2->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&gc05a2->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "v4l2 subdev init error: %d\n", ret);

same here

> +		goto err_media_entity_cleanup;
> +	}
> +
> +	pm_runtime_enable(gc05a2->dev);
> +	pm_runtime_set_autosuspend_delay(gc05a2->dev, 1000);
> +	pm_runtime_use_autosuspend(gc05a2->dev);
> +	pm_runtime_idle(gc05a2->dev);
> +
> +	ret = v4l2_async_register_subdev_sensor(&gc05a2->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "could not register v4l2 device\n");

ditto

> +		goto err_rpm;
> +	}
> +
> +	return 0;
> +
> +err_rpm:
> +	pm_runtime_disable(gc05a2->dev);
> +	v4l2_subdev_cleanup(&gc05a2->sd);
> +
> +err_media_entity_cleanup:
> +	media_entity_cleanup(&gc05a2->sd.entity);
> +
> +err_v4l2_ctrl_handler_free:
> +	v4l2_ctrl_handler_free(&gc05a2->ctrls);
> +
> +	return ret;
> +}
> +
> +static void gc05a2_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct gc05a2 *gc05a2 = to_gc05a2(sd);
> +
> +	v4l2_async_unregister_subdev(&gc05a2->sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&gc05a2->sd.entity);
> +	v4l2_ctrl_handler_free(&gc05a2->ctrls);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		gc05a2_power_off(gc05a2->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +}
> +
> +static const struct of_device_id gc05a2_of_match[] = {
> +	{ .compatible = "galaxycore,gc05a2" },
> +	{}

{ /* sentinel */ }


After which, looks good to me, so, after addressing my comments:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>

> +};
> +MODULE_DEVICE_TABLE(of, gc05a2_of_match);
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(gc05a2_pm_ops,
> +				 gc05a2_power_off,
> +				 gc05a2_power_on,
> +				 NULL);
> +
> +static struct i2c_driver gc05a2_i2c_driver = {
> +	.driver = {
> +		.of_match_table = gc05a2_of_match,
> +		.pm = pm_ptr(&gc05a2_pm_ops),
> +		.name  = "gc05a2",
> +	},
> +	.probe = gc05a2_probe,
> +	.remove = gc05a2_remove,
> +};
> +module_i2c_driver(gc05a2_i2c_driver);
> +
> +MODULE_DESCRIPTION("GalaxyCore gc05a2 Camera driver");
> +MODULE_AUTHOR("Zhi Mao <zhi.mao@...iatek.com>");
> +MODULE_LICENSE("GPL");

-- 
AngeloGioacchino Del Regno
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Powered by blists - more mailing lists