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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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