Re: [PATCH v10 3/6] media: platform: synopsys: Add support for HDMI input driver

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 2/26/25 11:47, Hans Verkuil wrote:
...
>> +static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
>> +			       struct platform_device *pdev)
>> +{
>> +	struct device *dev = hdmirx_dev->dev;
>> +	struct hdmirx_cec_data cec_data;
>> +	int irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, "cec");
>> +	if (irq < 0) {
>> +		dev_err_probe(dev, irq, "failed to get cec irq\n");
>> +		return irq;
>> +	}
>> +
>> +	hdmirx_dev->cec_notifier = cec_notifier_conn_register(dev, NULL, NULL);
> 
> I never really paid attention to this, but you don't need to use the cec-notifier
> API. The notifier API is needed if an independent CEC device has to be associated
> with an independent HDMI interface, and needs to be notified whenever the
> physical address changes.
> 
> This is a single IP combining both HDMI and CEC, so the HDMI part calls
> cec_s_phys_addr_from_edid()/cec_phys_addr_invalidate() directly whenever the EDID
> changes.
> 
> So just drop the include of cec-notifier.h and anything related to this.

Ack

...
>> +	hdmirx_dev->infoframes = v4l2_debugfs_if_alloc(hdmirx_dev->debugfs_dir,
>> +						       V4L2_DEBUGFS_IF_AVI, hdmirx_dev,
>> +						       hdmirx_debugfs_if_read);
>> +
>> +	return 0;
>> +
>> +err_unreg_video_dev:
>> +	video_unregister_device(&hdmirx_dev->stream.vdev);
> 
> Replace this with vb2_video_unregister_device(). Sorry, I missed this in my previous reviews.
> 
>> +err_unreg_v4l2_dev:
>> +	v4l2_device_unregister(&hdmirx_dev->v4l2_dev);
>> +err_hdl:
>> +	v4l2_ctrl_handler_free(&hdmirx_dev->hdl);
>> +err_pm:
>> +	hdmirx_disable(dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static void hdmirx_remove(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
>> +
>> +	v4l2_debugfs_if_free(hdmirx_dev->infoframes);
>> +	debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
>> +
>> +	snps_hdmirx_cec_unregister(hdmirx_dev->cec);
>> +	cec_notifier_conn_unregister(hdmirx_dev->cec_notifier);
>> +
>> +	hdmirx_disable_irq(dev);
>> +
>> +	video_unregister_device(&hdmirx_dev->stream.vdev);
> 
> vb2_video_unregister_device().
> 
> This function ensures that, if streaming was in progress and the driver was forcibly
> removed, streaming is stopped gracefully and safely.

Ack

-- 
Best regards,
Dmitry