Re: [PATCH 2/2] media: i2c: Add imx283 camera sensor driver

On 15/02/2024 21:44, Umang Jain wrote:

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Add a v4l2 subdevice driver for the Sony IMX283 image sensor.

The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with
Square Pixel for Color Cameras.

...

+
+static int imx283_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx283 *imx283 = to_imx283(sd);
+
+	gpiod_set_value_cansleep(imx283->reset_gpio, 0);

That's odd. It is reset GPIO, so 0 means deassert means "stop resetting
device". You have wrong handling of the GPIO.

+	regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
+	clk_disable_unprepare(imx283->xclk);
+
+	return 0;
+}
+
+static int imx283_get_regulators(struct imx283 *imx283)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++)
+		imx283->supplies[i].supply = imx283_supply_name[i];
+
+	return devm_regulator_bulk_get(imx283->dev,
+				       ARRAY_SIZE(imx283_supply_name),
+				       imx283->supplies);
+}

...

+static const struct of_device_id imx283_dt_ids[] = {
+	{ .compatible = "sony,imx283", },
+	{ /* sentinel */ }
+};

This goes next to driver structure. You also miss module device table.

+
+static int imx283_parse_endpoint(struct imx283 *imx283)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(imx283->dev);
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	struct fwnode_handle *ep;
+	int ret;
+
+	if (!fwnode)
+		return -ENXIO;
+
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep) {
+		dev_err(imx283->dev, "Failed to get next endpoint\n");
+		return -ENXIO;
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		return ret;
+
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+		dev_err(imx283->dev,
+			"number of CSI2 data lanes %d is not supported\n",
+			bus_cfg.bus.mipi_csi2.num_data_lanes);
+		ret = -EINVAL;
+		goto done_endpoint_free;
+	}
+
+	ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       link_frequencies, ARRAY_SIZE(link_frequencies),
+				       &imx283->link_freq_bitmap);
+
+done_endpoint_free:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+
+	return ret;
+};
+
+static int imx283_probe(struct i2c_client *client)
+{
+	struct imx283 *imx283;
+	int ret;
+	unsigned int i;
+	unsigned int xclk_freq;
+
+	imx283 = devm_kzalloc(&client->dev, sizeof(*imx283), GFP_KERNEL);
+	if (!imx283)
+		return -ENOMEM;
+
+	imx283->dev = &client->dev;
+
+	v4l2_i2c_subdev_init(&imx283->sd, client, &imx283_subdev_ops);
+
+	imx283->cci = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(imx283->cci)) {
+		ret = PTR_ERR(imx283->cci);
+		dev_err(imx283->dev, "failed to initialize CCI: %d\n", ret);
+		return ret;
+	}
+
+	/* Get system clock (xclk) */
+	imx283->xclk = devm_clk_get(imx283->dev, NULL);
+	if (IS_ERR(imx283->xclk)) {
+		dev_err(imx283->dev, "failed to get xclk\n");

Syntax is: return dev_err_probe()

+		return PTR_ERR(imx283->xclk);
+	}
+
+	xclk_freq = clk_get_rate(imx283->xclk);
+	for (i = 0; i < ARRAY_SIZE(imx283_frequencies); i++) {
+		if (xclk_freq == imx283_frequencies[i].mhz) {
+			imx283->freq = &imx283_frequencies[i];
+			break;
+		}
+	}
+	if (!imx283->freq) {
+		dev_err(imx283->dev, "xclk frequency unsupported: %d Hz\n", xclk_freq);
+		return -EINVAL;
+	}
+
+	ret = imx283_get_regulators(imx283);
+	if (ret) {
+		dev_err(imx283->dev, "failed to get regulators\n");
+		return ret;

return dev_err_probe()

+	}
+
+	ret = imx283_parse_endpoint(imx283);
+	if (ret) {
+		dev_err(imx283->dev, "failed to parse endpoint configuration\n");
+		return ret;
+	}
+
+	/* Request optional enable pin */
+	imx283->reset_gpio = devm_gpiod_get_optional(imx283->dev, "reset",
+						     GPIOD_OUT_HIGH);

No, that's not allowed in your binding. Don't add undocumented properties.

+
+	mutex_init(&imx283->mutex);
+
+	/*
+	 * The sensor must be powered for imx283_identify_module()
+	 * to be able to read the CHIP_ID register
+	 */
+	ret = imx283_power_on(imx283->dev);
+	if (ret)
+		return ret;
+

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel