RE: [PATCH v7 2/2] media: i2c: add driver for ITE IT6625/IT6626

[Hermes.Wu]

Hi Krzysztof,

Thanks for the review.

>On Thu, Jul 23, 2026 at 10:44:03AM +0800, Hermes Wu wrote:
>> +	mutex_init(&it6625->it6625_lock);
>> +	INIT_DELAYED_WORK(&it6625->hpd_delayed_work, it6625_hpd_delayed_work);
>> +	INIT_WORK(&it6625->polling_work, it6625_polling_work);
>> +
>> +	if (client->irq) {
>> +		err = devm_request_threaded_irq(&client->dev, client->irq,
>
>You do not have interrupts. Read your binding again. You are not allowed to sneak in any undocumented ABI.

You're right. The interrupt property is missing from the binding.
I'll add the interrupt binding in the next revision.

>> +						NULL, it6625_irq_handler,
>> +						IRQF_TRIGGER_LOW |
>> +						IRQF_ONESHOT |
>> +						IRQF_NO_AUTOEN,
>> +						"it6625", it6625);
>> +		if (err)
>> +			return err;
>> +	} else {
>> +		dev_info(it6625->dev, "no IRQ, falling back to polling");
>> +		timer_setup(&it6625->timer, it6625_irq_poll_timer, 0);
>> +	}
>> +
>> +	sd = &it6625->sd;
>> +	err = it6625_init_v4l2_subdev(it6625);
>> +	if (err)
>> +		goto err_clean_work_queues;
>> +
>> +	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
>> +	if (err)
>> +		goto err_clean_hdl;
>> +
>> +	it6625->cec_adap = cec_allocate_adapter(&it6625_cec_adap_ops,
>> +						it6625, dev_name(it6625->dev),
>> +						CEC_CAP_DEFAULTS |
>> +						CEC_CAP_MONITOR_ALL |
>> +						CEC_CAP_PHYS_ADDR,
>> +						CEC_MAX_LOG_ADDRS);
>> +	if (IS_ERR(it6625->cec_adap)) {
>> +		err = PTR_ERR(it6625->cec_adap);
>> +		dev_err(it6625->dev, "%s %d", __func__, __LINE__);
>> +		goto err_clean_hdl;
>> +	}
>> +
>> +	err = cec_register_adapter(it6625->cec_adap, &client->dev);
>> +	if (err < 0) {
>> +		dev_err(it6625->dev, "%s: failed to register the cec device", __func__);
>> +		cec_delete_adapter(it6625->cec_adap);
>> +		it6625->cec_adap = NULL;
>> +		goto err_clean_hdl;
>> +	}
>> +
>> +	it6625_debugfs_init(it6625, client);
>> +
>> +	it6625_initial_setup(it6625);
>> +	it6625_v4l2_sd_ctrl_update(sd);
>> +
>> +	err = v4l2_async_register_subdev(sd);
>> +	if (err < 0) {
>> +		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
>> +		goto err_clean_debugfs;
>> +	}
>> +
>> +	if (client->irq)
>> +		enable_irq(client->irq);
>> +	else
>> +		mod_timer(&it6625->timer, jiffies + 
>> +msecs_to_jiffies(POLL_INTERVAL_MS));
>> +
>> +	return 0;
>> +
>> +err_clean_debugfs:
>> +	debugfs_remove_recursive(it6625->debugfs_dir);
>> +	cec_unregister_adapter(it6625->cec_adap);
>> +err_clean_hdl:
>> +	media_entity_cleanup(&sd->entity);
>> +	v4l2_ctrl_handler_free(&it6625->hdl);
>> +
>> +err_clean_work_queues:
>> +	if (!client->irq)
>> +		timer_shutdown_sync(&it6625->timer);
>> +	cancel_work_sync(&it6625->polling_work);
>> +	cancel_delayed_work_sync(&it6625->hpd_delayed_work);
>> +	mutex_destroy(&it6625->it6625_lock);
>> +	return err;
>> +}
>> +
>> +static void it6625_remove(struct i2c_client *client) {
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct it6625 *it6625 = sd_to_6625(sd);
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	v4l2_device_unregister_subdev(sd);
>> +
>> +	if (client->irq)
>> +		disable_irq(client->irq);
>> +	else
>> +		timer_shutdown_sync(&it6625->timer);
>> +
>> +	cancel_work_sync(&it6625->polling_work);
>> +	cancel_delayed_work_sync(&it6625->hpd_delayed_work);
>> +	debugfs_remove_recursive(it6625->debugfs_dir);
>> +	cec_unregister_adapter(it6625->cec_adap);
>> +	mutex_destroy(&it6625->it6625_lock);
>> +	media_entity_cleanup(&sd->entity);
>> +	v4l2_ctrl_handler_free(&it6625->hdl);
>> +}
>> +
>> +static const struct i2c_device_id it6625_id[] = {
>> +	{ .name = "it6625", .driver_data = IT6625_CHIP },
>> +	{ .name = "it6626", .driver_data = IT6626_CHIP },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, it6625_id);
>> +
>> +static const struct of_device_id it6625_of_match[] = {
>> +	{ .compatible = "ite,it6625", .data = (void *)IT6625_CHIP },
>> +	{ .compatible = "ite,it6626", .data = (void *)IT6626_CHIP },
>
>
>I don't see any difference in the code between these two, so your devices are simply compatible, no?

The driver uses the compatible data to distinguish the chip variant.

The only functional difference currently is the CSI output capability:
IT6626 supports both D-PHY and C-PHY, while IT6625 supports D-PHY only.

For example:

    if (endpoint.bus_type == V4L2_MBUS_CSI2_CPHY &&
        it6625->chip_type != IT6626_CHIP)
        return -EINVAL;

So the two compatibles are required to identify the hardware capabilities.


>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, it6625_of_match);
>> +
>> +static struct i2c_driver it6625_driver = {
>> +	.driver = {
>> +		.name = "it6625",
>> +		.of_match_table = of_match_ptr(it6625_of_match),
>
>Drop of_match_ptr. You have a warning here, most likely.
>
>Best regards,
>Krzysztof
>
>