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

[Lachlan.Michael@xxxxxxxx]

Dear Krzysztof,

Thank-you for your review.

<snip>

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.

I will dev_dbg in v2.

>> +	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?

I will review all comments for the v2 submission and remove / reword them to be
more in line with the kernel style. 

>> +	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.

Will remove in v2.

>> +	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.

Will remove in v2.

>> +	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.

Understood. 

>> +	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?

Will remove these comments for v2.

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