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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf4c855b-6b1f-4a91-b292-5635ee4f7d2e@collabora.com>
Date: Wed, 26 Feb 2025 12:24:22 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Hans Verkuil <hverkuil@...all.nl>,
 Shreeya Patel <shreeya.patel@...labora.com>, Heiko Stuebner
 <heiko@...ech.de>, Mauro Carvalho Chehab <mchehab@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, jose.abreu@...opsys.com,
 nelson.costa@...opsys.com, shawn.wen@...k-chips.com,
 nicolas.dufresne@...labora.com,
 Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: kernel@...labora.com, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, Tim Surber <me@...surber.de>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH v10 3/6] media: platform: synopsys: Add support for HDMI
 input driver

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