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

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

On 2/23/25 20:54, Christophe JAILLET wrote:
> Le 23/02/2025 à 18:30, Dmitry Osipenko a écrit :
>> From: Shreeya Patel <shreeya.patel@collabora.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@60Hz)
>>   - 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@rock-chips.com>
>> Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.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.

-- 
Best regards,
Dmitry

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip