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
