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

[Christophe JAILLET]

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@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> > > Signed-off-by: Dingxian Wen <shawn.wen@xxxxxxxxxxxxxx>
> > > Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> >
> > 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