Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver

[Krzysztof Kozlowski]

On Thu, Jul 30, 2026 at 11:15:25AM +0900, Lachlan Michael wrote:
> +	msleep(24); /* Regulator stabilization after standby cancel. */
> +
> +	ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
> +	if (ret || err)
> +		return ret ? ret : err;
> +
> +	chip_id = val;
> +	dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);

Drivers should be silent on success. Drop or dev_dbg.

> +
> +	if (chip_id != IMX908_CHIP_ID) {
> +		dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
> +			chip_id, IMX908_CHIP_ID);
> +		return -ENXIO;
> +	}
> +
> +	/* Set to standby mode */
> +	ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
> +	if (ret)
> +		dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
> +
> +	return 0;
> +}
> +
> +static int imx908_probe(struct i2c_client *client)
> +{
> +	struct imx908 *imx;
> +	int ret;
> +
> +	/* Allocate Memory */

Really?

> +	imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
> +	if (!imx)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				     "failed to allocate IMX908 device structure\n");
> +	imx->dev = &client->dev;
> +
> +	/* Initialize V4L2 subdevice */

Obvious.

> +	v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
> +	imx->sd.internal_ops = &imx908_internal_ops;
> +
> +	/* Register access initialization. Set 2-byte (16-bit) addresses */
> +	imx->cci = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(imx->cci))
> +		return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
> +				     "CCI regmap init failed\n");
> +
> +	/* Get mandatory input clock from DT (INCK) */

Obvious.

> +	imx->xclk = devm_clk_get(imx->dev, "xclk");
> +	if (IS_ERR(imx->xclk))
> +		return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
> +
> +	/* Get clock frequency and check against acceptable HW values */
> +	imx->xclk_freq = clk_get_rate(imx->xclk);
> +	ret = imx908_get_inck_sel(imx);
> +	if (ret)
> +		return ret;
> +
> +	/* GPIO reset acquisition */

Please drop obvious comments. Can devm_gpiod_get_optional() be anything
else than GPIO reset acquisition? No.

Redundant comments bloat the code and make it more difficult to actually
spot important things.

> +	imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
> +						  GPIOD_OUT_HIGH);
> +
> +	if (IS_ERR(imx->reset_gpio))
> +		return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
> +				     "reset gpio\n");
> +
> +	/* Link to power supplies */
> +	ret = imx908_get_regulators(imx);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "regulator get failed\n");
> +
> +	/* Parse Device Tree endpoint */

Obvious

> +	ret = imx908_parse_fwnode(imx);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "device tree parse failed\n");
> +
> +	/* Power on IMX908 image sensor */

What if imx908_power_on() does power off of the sensor?

> +	ret = imx908_power_on(imx);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "power-on failed\n");
> +
> +	/* Read IMX908 device ID */

Best regards,
Krzysztof