RE: [PATCH v3 07/13] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder

[Biju Das]

Hi Philipp,

Thanks for the feedback.

> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: 31 August 2026 10:17
> Subject: Re: [PATCH v3 07/13] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder
> 
> On Mi, 2026-08-26 at 18:48 +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Add support for the RZ/G3L LVDS encoder driver. It operates in
> > single-link mode with 4 lanes (Data) + 1 lane (Clock) and supports
> > pixel clock rates from 25 to 87 MHz. The LVDS module cannot be used at
> > the same time as MIPI-DSI. However, LVDS and the DSI interface share a
> > peripheral clock and the MIPI_DSI_PRESET_N reset signal. Also, the
> > MIPI_DSI_CMN_RSTB and MIPI_DSI_ARESET_N reset signals must be asserted
> > before using the LVDS module.
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > Tested-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> [...]
> > ---
> >  drivers/gpu/drm/renesas/rz-du/Kconfig         |  13 +
> >  drivers/gpu/drm/renesas/rz-du/Makefile        |   1 +
> >  drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c    | 295 ++++++++++++++++++
> >  .../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h   |  25 ++
> >  4 files changed, 334 insertions(+)
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h
> >
> [...]
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
> > new file mode 100644
> > index 000000000000..2a7ae67ff08d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c
> > @@ -0,0 +1,295 @@
> [...]
> > +static int rzg3l_lvds_probe(struct platform_device *pdev) {
> > +	struct reset_control *rstc, *arstc;
> > +	struct device *dev = &pdev->dev;
> > +	struct rzg3l_lvds *lvds;
> > +	void __iomem *base;
> > +	int ret;
> > +
> > +	lvds = devm_drm_bridge_alloc(dev, struct rzg3l_lvds, bridge,
> > +				     &rzg3l_lvds_bridge_ops);
> > +	if (IS_ERR(lvds))
> > +		return PTR_ERR(lvds);
> > +
> > +	lvds->dev = dev;
> > +	lvds->bridge.of_node = pdev->dev.of_node;
> > +
> > +	base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	lvds->regmap = devm_regmap_init_mmio(dev, base, &rzg3l_lvds_regmap_config);
> > +	if (IS_ERR(lvds->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(lvds->regmap),
> > +				     "failed to init regmap\n");
> > +
> > +	lvds->dotclk = devm_clk_get(dev, "dotclk");
> > +	if (IS_ERR(lvds->dotclk))
> > +		return PTR_ERR(lvds->dotclk);
> > +
> > +	rstc = devm_reset_control_get_exclusive(dev, "rst");
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get rst\n");
> > +
> > +	arstc = devm_reset_control_get_exclusive(dev, "arst");
> > +	if (IS_ERR(arstc))
> > +		return dev_err_probe(dev, PTR_ERR(arstc),
> > +				     "failed to get arst\n");
> 
> It would be nice to have a comment about the two MIPI DSI resets having to be asserted here as well.

Ok.

> 
> Also, it looks like you could pair these two ...

Ok.

> 
> > +
> > +	lvds->prstc = devm_reset_control_get_exclusive(dev, "prst");
> > +	if (IS_ERR(lvds->prstc))
> > +		return dev_err_probe(dev, PTR_ERR(lvds->prstc),
> > +				     "failed to get prst\n");
> > +
> > +	lvds->lvd_rstc = devm_reset_control_get_exclusive(dev, "lvdrst");
> > +	if (IS_ERR(lvds->lvd_rstc))
> > +		return dev_err_probe(dev, PTR_ERR(lvds->lvd_rstc),
> > +				     "failed to get core reset\n");
> 
> ... and these two into a devm_reset_control_bulk_get_exclusive() each.

Agreed, will fix this in next version.

Cheers,
Biju