Re: [PATCH v5 2/3] media: i2c: Add GC05A2 image sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
---
  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@xxxxxxxxxxxx>
+ */
+#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@xxxxxxxxxxxxx>

+};
+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@xxxxxxxxxxxx>");
+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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux