Re: [PATCH v1 2/2] media/i2c: Add a driver for the Sony IMX708 image sensor

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/01/2023 16:05, Naushir Patuck wrote:
> From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
> 
> The imx708 is a 12MP MIPI sensor with a 16:9 aspect ratio, here using
> two CSI-2 lanes. It is a "quad Bayer" sensor with all 3 modes offering
> 10-bit output:
> 
> 12MP: 4608x2592 up to 14.35fps (full FoV)
> 1080p: 2304x1296 up to 56.02fps (full FoV)
> 720p: 1536x864 up to 120.12fps (cropped)
> 

Thank you for your patch. There is something to discuss/improve.

> +
> +static int imx708_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct imx708 *imx708;
> +	int ret;
> +
> +	imx708 = devm_kzalloc(&client->dev, sizeof(*imx708), GFP_KERNEL);
> +	if (!imx708)
> +		return -ENOMEM;
> +
> +	v4l2_i2c_subdev_init(&imx708->sd, client, &imx708_subdev_ops);
> +
> +	/* Check the hardware configuration in device tree */
> +	if (imx708_check_hwcfg(dev))
> +		return -EINVAL;
> +
> +	/* Get system clock (xclk) */
> +	imx708->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(imx708->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");

return dev_err_probe().

> +		return PTR_ERR(imx708->xclk);
> +	}
> +
> +	imx708->xclk_freq = clk_get_rate(imx708->xclk);
> +	if (imx708->xclk_freq != IMX708_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx708->xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx708_get_regulators(imx708);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");

return dev_err_probe().

> +		return ret;
> +	}
> +
> +	/* Request optional enable pin */
> +	imx708->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx708_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx708_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx708_identify_module(imx708);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize default format */
> +	imx708_set_default_format(imx708);
> +
> +	/*
> +	 * Enable runtime PM with autosuspend. As the device has been powered
> +	 * manually, mark it as active, and increase the usage count without
> +	 * resuming the device.
> +	 */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	/* This needs the pm runtime to be registered. */
> +	ret = imx708_init_controls(imx708);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	imx708->sd.internal_ops = &imx708_internal_ops;
> +	imx708->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
> +	imx708->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	imx708->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&imx708->sd.entity, 1, &imx708->pad);
> +	if (ret) {
> +		dev_err(dev, "failed to init entity pads: %d\n", ret);

return dev_err_probe().

> +		goto error_handler_free;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor(&imx708->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);

return dev_err_probe().


Best regards,
Krzysztof