| Newsgroups |
org.kernel.vger.linux-media,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel |
| Message-ID |
<TY6PR01MB17839831F80F2EABFF940ED8B92D32@TY6PR01MB17839.jpnprd01.prod.outlook.com> |
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 */