linux-kernel - Re: [PATCH v8 4/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: <bc1f5334-b0fb-4e81-979d-feb17886ac40@wanadoo.fr>
Date: Sun, 23 Feb 2025 22:11:34 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Dmitry Osipenko <dmitry.osipenko@...labora.com>,
 Shreeya Patel <shreeya.patel@...labora.com>, Heiko Stuebner
 <heiko@...ech.de>, Mauro Carvalho Chehab <mchehab@...nel.org>,
 Hans Verkuil <hverkuil@...all.nl>, 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>
Subject: Re: [PATCH v8 4/6] media: platform: synopsys: Add support for HDMI
 input driver

Le 23/02/2025 à 19:08, Dmitry Osipenko a écrit :
> On 2/23/25 20:54, Christophe JAILLET wrote:
>> Le 23/02/2025 à 18:30, Dmitry Osipenko a écrit :
>>> From: Shreeya Patel <shreeya.patel@...labora.com>
>>>
>>> Add initial support for the Synopsys DesignWare HDMI RX
>>> Controller Driver used by Rockchip RK3588. The driver
>>> supports:
>>>    - HDMI 1.4b and 2.0 modes (HDMI 4k@...z)
>>>    - RGB888, YUV422, YUV444 and YCC420 pixel formats
>>>    - CEC
>>>    - EDID configuration
>>>
>>> The hardware also has Audio and HDCP capabilities, but these are
>>> not yet supported by the driver.
>>>
>>> Co-developed-by: Dingxian Wen <shawn.wen@...k-chips.com>
>>> Signed-off-by: Dingxian Wen <shawn.wen@...k-chips.com>
>>> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
>>
>> Hi,
>>
>>> +    hdmirx_dev->dev = dev;
>>> +    dev_set_drvdata(dev, hdmirx_dev);
>>> +
>>> +    ret = hdmirx_parse_dt(hdmirx_dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = hdmirx_setup_irq(hdmirx_dev, pdev);
>>> +    if (ret)
>>> +        return ret;
>>
>>  From here, should of_reserved_mem_device_release() be called in the
>> error handling path, as done in the remove function?
> 
> Indeed, I'll make it to use devm.
> 
>>> +    hdmirx_dev->regs = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(hdmirx_dev->regs))
>>> +        return dev_err_probe(dev, PTR_ERR(hdmirx_dev->regs),
>>> +                     "failed to remap regs resource\n");
>>
>> ...
>>
>>> +static const struct of_device_id hdmirx_id[] = {
>>> +    { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
>>> +    { },
>>
>> Unneeded trailing comma after a terminator.
>>
>>> +};
>>> +MODULE_DEVICE_TABLE(of, hdmirx_id);
>>
>> ...
>>
>>> +    ret = cec_register_adapter(cec->adap, cec->dev);
>>> +    if (ret < 0) {
>>> +        dev_err(cec->dev, "cec register adapter failed\n");
>>> +        cec_unregister_adapter(cec->adap);
>>
>> Is it needed to call cec_unregister_adapter() when
>> cec_register_adapter() fails?
> 
> Yes, it's confusing, but unregister is needed to free the adapter
> properly, it's prepared to do it. Thanks for the review.
> 

I don't know this API, so you'll get the last word, but 
cec_unregister_adapter() does not seem to do that many things in such a 
case, unless I miss something. See [1].

CJ

[1]: 
https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/media/cec/core/cec-core.c#L370