* [PATCH v3 0/2] Add support for Renesas RZ/G3L LVDS encoder
@ 2026-05-26 7:47 Biju
2026-05-26 7:47 ` [PATCH v3 1/2] dt-bindings: display: bridge: Document " Biju
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Biju @ 2026-05-26 7:47 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, David Airlie,
Simona Vetter, Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm
Cc: Biju Das, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Luca Ceresoli, Tommaso Merciai, dri-devel, devicetree,
linux-kernel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das
From: Biju Das <biju.das.jz@bp.renesas.com>
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.
v2->v3:
* Collected tags.
v2->v2[1]:
* Dropped patch#1 as it is accepted.
* Replace drm_atomic_state with drm_atomic_commit in
rzg3l_lvds_atomic_{en,dis}able().
* Drop local variable ret and dev_err() messages in
rzg3l_lvds_atomic_enable(); use WARN_ON() instead to
capture unexpected failures since atomic_enable should not fail.
* Drop local variable next_bridge from rzg3l_lvds_probe().
[1] https://lore.kernel.org/all/20260524194457.479681-1-biju.das.jz@bp.renesas.com/
v1->v2:
* Collected the tags for binding patches.
* Dropped unused function rzg3l_lvds_is_connected() and removed the
corresponding header file rzg3l_lvds.h
* Dropped next_bridge from struct rzg3l_lvds instead using bridge's
next_bridge.
* Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as
atomic_enable doesn't fail and for each enable there always will be an
atomic_disable() call.
* Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback.
* Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe()
* Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend,
resume}().
Biju Das (2):
dt-bindings: display: bridge: Document Renesas RZ/G3L LVDS encoder
drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder
.../bridge/renesas,r9a08g046-lvds.yaml | 128 ++++++++
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 | 277 ++++++++++++++++++
.../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++
5 files changed, 445 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml
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
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v3 1/2] dt-bindings: display: bridge: Document Renesas RZ/G3L LVDS encoder 2026-05-26 7:47 [PATCH v3 0/2] Add support for Renesas RZ/G3L LVDS encoder Biju @ 2026-05-26 7:47 ` Biju 2026-05-27 5:11 ` Claude review: " Claude Code Review Bot 2026-05-26 7:47 ` [PATCH v3 2/2] drm: renesas: rz-du: Add support for " Biju 2026-05-27 5:11 ` Claude review: Add support for Renesas " Claude Code Review Bot 2 siblings, 1 reply; 8+ messages in thread From: Biju @ 2026-05-26 7:47 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven, Magnus Damm Cc: Biju Das, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Tommaso Merciai, dri-devel, devicetree, linux-kernel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das, Conor Dooley From: Biju Das <biju.das.jz@bp.renesas.com> Document the LVDS encoder IP found on the RZ/G3L SoC. It supports single-link mode. LVDS and the DSI interface share a peripheral clock and the MIPI_DSI_PRESET_N reset signal. However, the LVDS module cannot be used at the same time as MIPI-DSI. Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Collected tag. v2->v2[1]: * No change. [1] https://lore.kernel.org/all/20260524195829.960401F000E9@smtp.kernel.org/ v1->v2: * Collected tag. --- .../bridge/renesas,r9a08g046-lvds.yaml | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml diff --git a/Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml new file mode 100644 index 000000000000..b1f6d020ae7b --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml @@ -0,0 +1,128 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/renesas,r9a08g046-lvds.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Renesas RZ/G3L LVDS Encoder + +maintainers: + - Biju Das <biju.das.jz@bp.renesas.com> + - Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> + +description: | + This binding describe the LVDS encoder embedded in the Renesas RZ/G3L + SoC. The encoder can operate in LVDS Single-link mode with 4 lanes + (Data) + 1 lane (Clock). + +properties: + compatible: + const: renesas,r9a08g046-lvds + + reg: + maxItems: 1 + + clocks: + items: + - description: Peripheral clock + - description: PHY clock + - description: Dot clock + + clock-names: + items: + - const: pclk + - const: phyclk + - const: dotclk + + resets: + items: + - description: LVDS_RESET_N + - description: MIPI_DSI_PRESET_N + - description: MIPI_DSI_CMN_RSTB + - description: MIPI_DSI_ARESET_N + + reset-names: + items: + - const: lvdrst + - const: prst + - const: rst + - const: arst + + power-domains: + maxItems: 1 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: Input channel, directly connected to the Display Unit. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: | + Output channel, directly connected to the LVDS panel or bridge. + + required: + - port@0 + - port@1 + +required: + - compatible + - reg + - clocks + - clock-names + - resets + - reset-names + - power-domains + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/renesas,r9a08g046-cpg.h> + + lvds-cmn@108a0000 { + compatible = "renesas,r9a08g046-lvds-cmn", + "simple-mfd", "syscon"; + reg = <0x108a0000 0x10000>; + #address-cells = <1>; + #size-cells = <1>; + + lvds0: lvds@10 { + compatible = "renesas,r9a08g046-lvds"; + reg = <0x10 0x8>; + clocks = <&cpg CPG_MOD R9A08G046_MIPI_DSI_PCLK>, + <&cpg CPG_MOD R9A08G046_LVDS_PLLCLK>, + <&cpg CPG_MOD R9A08G046_LVDS_CLK_DOT0>; + clock-names = "pclk", "phyclk", "dotclk"; + resets = <&cpg R9A08G046_LVDS_RESET_N>, + <&cpg R9A08G046_MIPI_DSI_PRESET_N>, + <&cpg R9A08G046_MIPI_DSI_CMN_RSTB>, + <&cpg R9A08G046_MIPI_DSI_ARESET_N>; + reset-names = "lvdrst", "prst", "rst", "arst"; + power-domains = <&cpg>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + lvds0_in: endpoint { + remote-endpoint = <&du_out_lvds0>; + }; + }; + + port@1 { + reg = <1>; + lvds0_out: endpoint { + remote-endpoint = <&panel_in>; + }; + }; + }; + }; + }; +... -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Claude review: dt-bindings: display: bridge: Document Renesas RZ/G3L LVDS encoder 2026-05-26 7:47 ` [PATCH v3 1/2] dt-bindings: display: bridge: Document " Biju @ 2026-05-27 5:11 ` Claude Code Review Bot 0 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-05-27 5:11 UTC (permalink / raw) To: dri-devel-reviews Patch Review This patch looks correct. The binding is well-structured, with clear property descriptions and a complete example. **Minor nit (description text):** ```yaml description: | This binding describe the LVDS encoder embedded in the Renesas RZ/G3L ``` Grammatically should be "describes" (third person singular). **No functional issues.** The binding correctly documents the 3 clocks, 4 resets, power domain, and port graph with input (port@0 from DU) and output (port@1 to panel/bridge). The example is complete and self-consistent. --- --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-05-26 7:47 [PATCH v3 0/2] Add support for Renesas RZ/G3L LVDS encoder Biju 2026-05-26 7:47 ` [PATCH v3 1/2] dt-bindings: display: bridge: Document " Biju @ 2026-05-26 7:47 ` Biju 2026-05-27 5:11 ` Claude review: " Claude Code Review Bot 2026-05-27 5:11 ` Claude review: Add support for Renesas " Claude Code Review Bot 2 siblings, 1 reply; 8+ messages in thread From: Biju @ 2026-05-26 7:47 UTC (permalink / raw) To: Biju Das, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Philipp Zabel, Geert Uytterhoeven, Magnus Damm Cc: linux-kernel, dri-devel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das, Tommaso Merciai From: Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Collected tags. v2->v2[1]: * Replace drm_atomic_state with drm_atomic_commit in rzg3l_lvds_atomic_{en,dis}able(). * Drop local variable ret and dev_err() messages in rzg3l_lvds_atomic_enable(); use WARN_ON() instead to capture unexpected failures since atomic_enable should not fail. * Drop local variable next_bridge from rzg3l_lvds_probe(). [1] https://lore.kernel.org/all/20260524194457.479681-3-biju.das.jz@bp.renesas.com/ v1->v2: * Dropped unused function rzg3l_lvds_is_connected() and removed the corresponding header file rzg3l_lvds.h * Dropped next_bridge from struct rzg3l_lvds instead using bridge's next_bridge. * Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as atomic_enable doesn't fail and for each enable there always will be an atomic_disable() call. * Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback. * Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe() * Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend, resume}() --- 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 | 277 ++++++++++++++++++ .../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++ 4 files changed, 317 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/Kconfig b/drivers/gpu/drm/renesas/rz-du/Kconfig index 7f2ef7137ae5..cbfc7b6bccb8 100644 --- a/drivers/gpu/drm/renesas/rz-du/Kconfig +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig @@ -26,3 +26,16 @@ config DRM_RZG2L_MIPI_DSI def_tristate DRM_RZG2L_DU depends on DRM_RZG2L_USE_MIPI_DSI select DRM_MIPI_DSI + +config DRM_RZG3L_USE_LVDS + bool "RZ/G3L DU LVDS Encoder Support" + depends on DRM_BRIDGE && OF + default DRM_RZG2L_DU + help + Enable support for the RZ/G3L Display Unit embedded LVDS encoders. + +config DRM_RZG3L_LVDS + def_tristate DRM_RZG2L_DU + depends on DRM_RZG3L_USE_LVDS + select DRM_KMS_HELPER + select DRM_PANEL diff --git a/drivers/gpu/drm/renesas/rz-du/Makefile b/drivers/gpu/drm/renesas/rz-du/Makefile index 2987900ea6b6..46decb7ac4f1 100644 --- a/drivers/gpu/drm/renesas/rz-du/Makefile +++ b/drivers/gpu/drm/renesas/rz-du/Makefile @@ -8,3 +8,4 @@ rzg2l-du-drm-$(CONFIG_VIDEO_RENESAS_VSP1) += rzg2l_du_vsp.o obj-$(CONFIG_DRM_RZG2L_DU) += rzg2l-du-drm.o obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o +obj-$(CONFIG_DRM_RZG3L_LVDS) += rzg3l_lvds.o 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..a51c3e5a2efe --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c @@ -0,0 +1,277 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * RZ/G3L LVDS Encoder Driver + * + * Copyright (C) 2026 Renesas Electronics Corporation + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/media-bus-format.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_probe_helper.h> + +#include "rzg3l_lvds_regs.h" + +enum rzg3l_lvds_mode { + RZG3L_LVDS_MODE_JEIDA = 0, + RZG3L_LVDS_MODE_JEIDA_MIRROR = 1, + RZG3L_LVDS_MODE_MODE2 = 2, + RZG3L_LVDS_MODE_MODE2_MIRROR = 3, + RZG3L_LVDS_MODE_VESA = 4, + RZG3L_LVDS_MODE_VESA_MIRROR = 5, + RZG3L_LVDS_MODE_MODE6 = 6, + RZG3L_LVDS_MODE_MODE6_MIRROR = 7, +}; + +struct rzg3l_lvds { + struct device *dev; + struct reset_control *prstc; + struct reset_control *lvd_rstc; + struct regmap *regmap; + struct drm_bridge bridge; +}; + +#define bridge_to_rzg3l_lvds(b) \ + container_of(b, struct rzg3l_lvds, bridge) + +/* ----------------------------------------------------------------------------- + * Bridge + */ + +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge, + struct drm_atomic_commit *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + const struct drm_bridge_state *bridge_state; + u32 fmt; + + /* Get the LVDS format from the bridge state. */ + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + if (WARN_ON(!bridge_state)) + return; + + switch (bridge_state->output_bus_cfg.format) { + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: + fmt = RZG3L_LVDS_MODE_JEIDA; + break; + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: + fmt = RZG3L_LVDS_MODE_VESA; + break; + default: + fmt = RZG3L_LVDS_MODE_VESA; + dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n", + bridge_state->output_bus_cfg.format); + break; + } + + if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) + return; + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR); + fsleep(20); + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, LVDS_0_PHY_CH_EN_LDO); + fsleep(10); + + regmap_write(lvds->regmap, LVDS_CMN, LVDS_CMN_RST_PHY0_SEL); + regmap_update_bits(lvds->regmap, LVDS_0_CTL_OFFSET, + LVDS_0_CTL_FMT_SEL_MSK, + FIELD_PREP(LVDS_0_CTL_FMT_SEL_MSK, fmt)); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, LVDS_0_PHY_CH_IO_EN); + regmap_write(lvds->regmap, LVDS_CMN, + LVDS_CMN_RST_PHY0_SEL | LVDS_CMN_PHY_RESET); + fsleep(100); +} + +static void rzg3l_lvds_atomic_disable(struct drm_bridge *bridge, + struct drm_atomic_commit *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + regmap_update_bits(lvds->regmap, LVDS_CMN, LVDS_CMN_PHY_RESET, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, 0); + + pm_runtime_put(lvds->dev); +} + +static int rzg3l_lvds_attach(struct drm_bridge *bridge, + struct drm_encoder *encoder, + enum drm_bridge_attach_flags flags) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + if (!lvds->bridge.next_bridge) + return 0; + + return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags); +} + +static enum drm_mode_status +rzg3l_lvds_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock > 87000) + return MODE_CLOCK_HIGH; + + if (mode->clock < 25000) + return MODE_CLOCK_LOW; + + return MODE_OK; +} + +static const struct drm_bridge_funcs rzg3l_lvds_bridge_ops = { + .attach = rzg3l_lvds_attach, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_enable = rzg3l_lvds_atomic_enable, + .atomic_disable = rzg3l_lvds_atomic_disable, + .mode_valid = rzg3l_lvds_bridge_mode_valid, +}; + +/* ----------------------------------------------------------------------------- + * Power Management + */ + +static int rzg3l_lvds_pm_runtime_suspend(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->lvd_rstc }, + { .rstc = lvds->prstc }, + }; + + return reset_control_bulk_assert(ARRAY_SIZE(resets), resets); +} + +static int rzg3l_lvds_pm_runtime_resume(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->lvd_rstc }, + { .rstc = lvds->prstc }, + }; + + return reset_control_bulk_deassert(ARRAY_SIZE(resets), resets); +} + +static DEFINE_RUNTIME_DEV_PM_OPS(rzg3l_lvds_pm_ops, + rzg3l_lvds_pm_runtime_suspend, + rzg3l_lvds_pm_runtime_resume, NULL); + +/* ----------------------------------------------------------------------------- + * Probe & Remove + */ + +static int rzg3l_lvds_probe(struct platform_device *pdev) +{ + struct reset_control *rstc, *arstc; + struct device *dev = &pdev->dev; + struct rzg3l_lvds *lvds; + 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; + + lvds->regmap = syscon_node_to_regmap(dev->of_node->parent); + if (IS_ERR(lvds->regmap)) + return PTR_ERR(lvds->regmap); + + rstc = devm_reset_control_get_optional_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_optional_exclusive(dev, "arst"); + if (IS_ERR(arstc)) + return dev_err_probe(dev, PTR_ERR(arstc), + "failed to get arst\n"); + + lvds->prstc = devm_reset_control_get_shared(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_shared(dev, "lvdrst"); + if (IS_ERR(lvds->lvd_rstc)) + return dev_err_probe(dev, PTR_ERR(lvds->lvd_rstc), + "failed to get core reset\n"); + + platform_set_drvdata(pdev, lvds); + ret = devm_pm_runtime_enable(dev); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable Runtime PM\n"); + + lvds->bridge.next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(lvds->bridge.next_bridge)) + return dev_err_probe(dev, PTR_ERR(lvds->bridge.next_bridge), + "failed to get next bridge\n"); + + ret = reset_control_assert(rstc); + if (ret < 0) + return ret; + + ret = reset_control_assert(arstc); + if (ret < 0) + return ret; + + ret = devm_drm_bridge_add(dev, &lvds->bridge); + if (ret) + return dev_err_probe(dev, ret, + "Failed to register drm bridge\n"); + + return ret; +} + +static const struct of_device_id rzg3l_lvds_of_table[] = { + { .compatible = "renesas,r9a08g046-lvds" }, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, rzg3l_lvds_of_table); + +static struct platform_driver rzg3l_lvds_platform_driver = { + .probe = rzg3l_lvds_probe, + .driver = { + .name = "rzg3l-lvds", + .pm = pm_ptr(&rzg3l_lvds_pm_ops), + .of_match_table = rzg3l_lvds_of_table, + }, +}; + +module_platform_driver(rzg3l_lvds_platform_driver); + +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>"); +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>"); +MODULE_DESCRIPTION("Renesas RZ/G3L LVDS Encoder Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h new file mode 100644 index 000000000000..281b7648f168 --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * RZ/G3L LVDS Interface Registers Definitions + * + * Copyright (C) 2026 Renesas Electronics Corporation + * + */ + +#ifndef __RZG3L_LVDS_REGS_H__ +#define __RZG3L_LVDS_REGS_H__ + +#define LVDS_CMN 0x00 +#define LVDS_CMN_RST_PHY0_SEL (1 << 24) +#define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) +#define LVDS_CMN_PHY_RESET (1 << 0) + +#define LVDS_0_PHY_OFFSET 0x10 +#define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) +#define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) +#define LVDS_0_PHY_CH_EN_BGR BIT(8) +#define LVDS_0_PHY_CH_EN_LDO BIT(9) + +#define LVDS_0_CTL_OFFSET 0x14 +#define LVDS_0_CTL_FMT_SEL_MSK GENMASK(23, 20) + +#endif /* __RZG3L_LVDS_REGS_H__ */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Claude review: drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-05-26 7:47 ` [PATCH v3 2/2] drm: renesas: rz-du: Add support for " Biju @ 2026-05-27 5:11 ` Claude Code Review Bot 0 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-05-27 5:11 UTC (permalink / raw) To: dri-devel-reviews Patch Review Overall a solid driver. A few items to flag: **1. Potential refcount issue with `bridge.next_bridge` assignment (medium concern):** ```c lvds->bridge.next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); ``` The `next_bridge` field is documented as "automatically put when this bridge is freed" — i.e., `drm_bridge_put()` is called on it at destroy time. However, `devm_drm_of_get_bridge()` returns either a devm-managed panel bridge (refcount 1, also devm-cleaned) or an unrefcounted bridge pointer from `of_drm_find_bridge()`. Storing it directly into `next_bridge` without calling `drm_bridge_get()` risks a refcount mismatch — the auto-put at bridge destruction would underflow. All existing in-tree drivers that store into `bridge.next_bridge` use `of_drm_find_and_get_bridge()`, `of_drm_get_bridge_by_endpoint()`, or `drm_bridge_get()` — functions that increment the refcount. Drivers that use `devm_drm_of_get_bridge()` store the result in a private struct field instead (like `rzg2l_mipi_dsi.c` does with `dsi->next_bridge`). Consider either storing in a private `lvds->next_bridge` field (matching the pattern in `rzg2l_mipi_dsi.c`), or using `of_drm_get_bridge_by_endpoint()` with a `drm_bridge_get()` for the `bridge.next_bridge` assignment. **2. Unnecessary include:** ```c #include <linux/of_device.h> ``` This header is not needed — the driver doesn't use any `of_device_*` functions. The sibling `rzg2l_mipi_dsi.c` driver only includes `<linux/of.h>` and `<linux/of_graph.h>`. This include can be dropped. **3. Register header: inconsistent bit-field style (`rzg3l_lvds_regs.h`):** The header mixes `(1 << N)` with `BIT()`: ```c #define LVDS_CMN_RST_PHY0_SEL (1 << 24) #define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) #define LVDS_CMN_PHY_RESET (1 << 0) ... #define LVDS_0_PHY_CH_EN_BGR BIT(8) #define LVDS_0_PHY_CH_EN_LDO BIT(9) ``` Prefer `BIT()` throughout for consistency with kernel style. **4. Register header: redundant definitions:** `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are both defined as `(1 << 24)` — they are identical. Only `LVDS_CMN_RST_PHY0_SEL` is used in the driver; `LVDS_CMN_RST_PHY0_SEL_CH0` is dead code. Similarly, `LVDS_0_PHY_CH_IO_EN` is defined as `(LVDS_0_PHY_CH_IO_EN_MSK << 0)` which equals `LVDS_0_PHY_CH_IO_EN_MSK` — the `<< 0` is a no-op and the two defines have the same value. This is confusing since one is used as a mask and the other as a value, but they're identical. Consider clarifying the intent or consolidating. **5. Kconfig: `select DRM_PANEL` and `select DRM_KMS_HELPER`:** ``` config DRM_RZG3L_LVDS def_tristate DRM_RZG2L_DU depends on DRM_RZG3L_USE_LVDS select DRM_KMS_HELPER select DRM_PANEL ``` `DRM_KMS_HELPER` is already selected by `DRM_RZG2L_DU` (through `DRM_GEM_DMA_HELPER` dependencies). And `DRM_PANEL` — verify this is actually needed. The driver doesn't directly use `drm_panel_*` APIs; it uses `devm_drm_of_get_bridge()` which internally handles panels via panel-bridge. The `DRM_PANEL` select may be needed transitively for `devm_drm_of_get_bridge()` to find panels, but it would be worth confirming. **6. Minor: `return ret` at end of probe:** ```c ret = devm_drm_bridge_add(dev, &lvds->bridge); if (ret) return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); return ret; ``` Since the error is already handled, `return ret` here is always `return 0`. Could be simplified to `return 0` for clarity, though functionally identical. **7. The `attach` callback accesses `next_bridge` through the bridge struct:** ```c static int rzg3l_lvds_attach(struct drm_bridge *bridge, struct drm_encoder *encoder, enum drm_bridge_attach_flags flags) { struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); if (!lvds->bridge.next_bridge) return 0; return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags); } ``` This is correct for the `bridge.next_bridge` approach, but note that returning 0 when there's no next bridge means the encoder pipeline would be incomplete (no connector). This only works if the bridge connector infrastructure handles it. If you switch to a private `next_bridge` field per item 1 above, this would need adjustment too. **No issues with:** - The `drm_atomic_commit` type usage (matches current in-tree bridge callback signatures) - The `pm_runtime_get_sync` + `WARN_ON` pattern (appropriate for atomic enable which shouldn't fail) - The reset handling (optional `rst`/`arst` asserted-and-left, shared `prst`/`lvdrst` toggled via PM runtime) - The `devm_drm_bridge_alloc` usage (correct modern API) - The mode_valid clock range checking (25-87 MHz matching the binding description) --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
* Claude review: Add support for Renesas RZ/G3L LVDS encoder 2026-05-26 7:47 [PATCH v3 0/2] Add support for Renesas RZ/G3L LVDS encoder Biju 2026-05-26 7:47 ` [PATCH v3 1/2] dt-bindings: display: bridge: Document " Biju 2026-05-26 7:47 ` [PATCH v3 2/2] drm: renesas: rz-du: Add support for " Biju @ 2026-05-27 5:11 ` Claude Code Review Bot 2 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-05-27 5:11 UTC (permalink / raw) To: dri-devel-reviews Overall Series Review Subject: Add support for Renesas RZ/G3L LVDS encoder Author: Biju <biju.das.au@gmail.com> Patches: 3 Reviewed: 2026-05-27T15:11:40.495706 --- This is a clean, well-structured 2-patch series adding LVDS encoder support for the Renesas RZ/G3L SoC. The driver follows established patterns from the existing `rzg2l_mipi_dsi.c` driver in the same directory and uses modern DRM bridge APIs (`devm_drm_bridge_alloc`, `devm_drm_bridge_add`). The DT binding is straightforward and already has Acked-by from a DT maintainer. The series is in good shape overall for v3. There are a few minor issues worth flagging — one potential refcount concern with storing `devm_drm_of_get_bridge()` result directly into `bridge.next_bridge`, an unnecessary include, and some register header cleanup opportunities — but nothing that blocks merging. --- --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 0/2] Add support for Renesas RZ/G3L LVDS encoder
@ 2026-05-24 19:44 Biju
2026-05-24 19:44 ` [PATCH v2 2/2] drm: renesas: rz-du: Add support for " Biju
0 siblings, 1 reply; 8+ messages in thread
From: Biju @ 2026-05-24 19:44 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, David Airlie,
Simona Vetter, Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm
Cc: Biju Das, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Luca Ceresoli, Tommaso Merciai, dri-devel, devicetree,
linux-kernel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das
From: Biju Das <biju.das.jz@bp.renesas.com>
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.
v2->v3:
* Dropped patch#1 as it is accepted.
* Replace drm_atomic_state with drm_atomic_commit in
rzg3l_lvds_atomic_{en,dis}able().
* Drop local variable ret and dev_err() messages in
rzg3l_lvds_atomic_enable(); use WARN_ON() instead to
capture unexpected failures since atomic_enable should not fail.
* Drop local variable next_bridge from rzg3l_lvds_probe().
v1->v2:
* Collected the tags for binding patches.
* Dropped unused function rzg3l_lvds_is_connected() and removed the
corresponding header file rzg3l_lvds.h
* Dropped next_bridge from struct rzg3l_lvds instead using bridge's
next_bridge.
* Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as
atomic_enable doesn't fail and for each enable there always will be an
atomic_disable() call.
* Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback.
* Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe()
* Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend,
resume}().
Biju Das (2):
dt-bindings: display: bridge: Document Renesas RZ/G3L LVDS encoder
drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder
.../bridge/renesas,r9a08g046-lvds.yaml | 128 ++++++++
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 | 277 ++++++++++++++++++
.../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++
5 files changed, 445 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml
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
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 2/2] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-05-24 19:44 [PATCH v2 0/2] " Biju @ 2026-05-24 19:44 ` Biju 2026-05-25 6:56 ` Claude review: " Claude Code Review Bot 0 siblings, 1 reply; 8+ messages in thread From: Biju @ 2026-05-24 19:44 UTC (permalink / raw) To: Biju Das, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Philipp Zabel, Geert Uytterhoeven, Magnus Damm Cc: linux-kernel, dri-devel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das, Tommaso Merciai From: Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Replace drm_atomic_state with drm_atomic_commit in rzg3l_lvds_atomic_{en,dis}able(). * Drop local variable ret and dev_err() messages in rzg3l_lvds_atomic_enable(); use WARN_ON() instead to capture unexpected failures since atomic_enable should not fail. * Drop local variable next_bridge from rzg3l_lvds_probe(). v1->v2: * Dropped unused function rzg3l_lvds_is_connected() and removed the corresponding header file rzg3l_lvds.h * Dropped next_bridge from struct rzg3l_lvds instead using bridge's next_bridge. * Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as atomic_enable doesn't fail and for each enable there always will be an atomic_disable() call. * Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback. * Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe() * Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend, resume}() --- 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 | 277 ++++++++++++++++++ .../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++ 4 files changed, 317 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/Kconfig b/drivers/gpu/drm/renesas/rz-du/Kconfig index 7f2ef7137ae5..cbfc7b6bccb8 100644 --- a/drivers/gpu/drm/renesas/rz-du/Kconfig +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig @@ -26,3 +26,16 @@ config DRM_RZG2L_MIPI_DSI def_tristate DRM_RZG2L_DU depends on DRM_RZG2L_USE_MIPI_DSI select DRM_MIPI_DSI + +config DRM_RZG3L_USE_LVDS + bool "RZ/G3L DU LVDS Encoder Support" + depends on DRM_BRIDGE && OF + default DRM_RZG2L_DU + help + Enable support for the RZ/G3L Display Unit embedded LVDS encoders. + +config DRM_RZG3L_LVDS + def_tristate DRM_RZG2L_DU + depends on DRM_RZG3L_USE_LVDS + select DRM_KMS_HELPER + select DRM_PANEL diff --git a/drivers/gpu/drm/renesas/rz-du/Makefile b/drivers/gpu/drm/renesas/rz-du/Makefile index 2987900ea6b6..46decb7ac4f1 100644 --- a/drivers/gpu/drm/renesas/rz-du/Makefile +++ b/drivers/gpu/drm/renesas/rz-du/Makefile @@ -8,3 +8,4 @@ rzg2l-du-drm-$(CONFIG_VIDEO_RENESAS_VSP1) += rzg2l_du_vsp.o obj-$(CONFIG_DRM_RZG2L_DU) += rzg2l-du-drm.o obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o +obj-$(CONFIG_DRM_RZG3L_LVDS) += rzg3l_lvds.o 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..a51c3e5a2efe --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c @@ -0,0 +1,277 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * RZ/G3L LVDS Encoder Driver + * + * Copyright (C) 2026 Renesas Electronics Corporation + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/media-bus-format.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_probe_helper.h> + +#include "rzg3l_lvds_regs.h" + +enum rzg3l_lvds_mode { + RZG3L_LVDS_MODE_JEIDA = 0, + RZG3L_LVDS_MODE_JEIDA_MIRROR = 1, + RZG3L_LVDS_MODE_MODE2 = 2, + RZG3L_LVDS_MODE_MODE2_MIRROR = 3, + RZG3L_LVDS_MODE_VESA = 4, + RZG3L_LVDS_MODE_VESA_MIRROR = 5, + RZG3L_LVDS_MODE_MODE6 = 6, + RZG3L_LVDS_MODE_MODE6_MIRROR = 7, +}; + +struct rzg3l_lvds { + struct device *dev; + struct reset_control *prstc; + struct reset_control *lvd_rstc; + struct regmap *regmap; + struct drm_bridge bridge; +}; + +#define bridge_to_rzg3l_lvds(b) \ + container_of(b, struct rzg3l_lvds, bridge) + +/* ----------------------------------------------------------------------------- + * Bridge + */ + +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge, + struct drm_atomic_commit *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + const struct drm_bridge_state *bridge_state; + u32 fmt; + + /* Get the LVDS format from the bridge state. */ + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + if (WARN_ON(!bridge_state)) + return; + + switch (bridge_state->output_bus_cfg.format) { + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: + fmt = RZG3L_LVDS_MODE_JEIDA; + break; + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: + fmt = RZG3L_LVDS_MODE_VESA; + break; + default: + fmt = RZG3L_LVDS_MODE_VESA; + dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n", + bridge_state->output_bus_cfg.format); + break; + } + + if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) + return; + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR); + fsleep(20); + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, LVDS_0_PHY_CH_EN_LDO); + fsleep(10); + + regmap_write(lvds->regmap, LVDS_CMN, LVDS_CMN_RST_PHY0_SEL); + regmap_update_bits(lvds->regmap, LVDS_0_CTL_OFFSET, + LVDS_0_CTL_FMT_SEL_MSK, + FIELD_PREP(LVDS_0_CTL_FMT_SEL_MSK, fmt)); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, LVDS_0_PHY_CH_IO_EN); + regmap_write(lvds->regmap, LVDS_CMN, + LVDS_CMN_RST_PHY0_SEL | LVDS_CMN_PHY_RESET); + fsleep(100); +} + +static void rzg3l_lvds_atomic_disable(struct drm_bridge *bridge, + struct drm_atomic_commit *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + regmap_update_bits(lvds->regmap, LVDS_CMN, LVDS_CMN_PHY_RESET, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, 0); + + pm_runtime_put(lvds->dev); +} + +static int rzg3l_lvds_attach(struct drm_bridge *bridge, + struct drm_encoder *encoder, + enum drm_bridge_attach_flags flags) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + if (!lvds->bridge.next_bridge) + return 0; + + return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags); +} + +static enum drm_mode_status +rzg3l_lvds_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock > 87000) + return MODE_CLOCK_HIGH; + + if (mode->clock < 25000) + return MODE_CLOCK_LOW; + + return MODE_OK; +} + +static const struct drm_bridge_funcs rzg3l_lvds_bridge_ops = { + .attach = rzg3l_lvds_attach, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_enable = rzg3l_lvds_atomic_enable, + .atomic_disable = rzg3l_lvds_atomic_disable, + .mode_valid = rzg3l_lvds_bridge_mode_valid, +}; + +/* ----------------------------------------------------------------------------- + * Power Management + */ + +static int rzg3l_lvds_pm_runtime_suspend(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->lvd_rstc }, + { .rstc = lvds->prstc }, + }; + + return reset_control_bulk_assert(ARRAY_SIZE(resets), resets); +} + +static int rzg3l_lvds_pm_runtime_resume(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->lvd_rstc }, + { .rstc = lvds->prstc }, + }; + + return reset_control_bulk_deassert(ARRAY_SIZE(resets), resets); +} + +static DEFINE_RUNTIME_DEV_PM_OPS(rzg3l_lvds_pm_ops, + rzg3l_lvds_pm_runtime_suspend, + rzg3l_lvds_pm_runtime_resume, NULL); + +/* ----------------------------------------------------------------------------- + * Probe & Remove + */ + +static int rzg3l_lvds_probe(struct platform_device *pdev) +{ + struct reset_control *rstc, *arstc; + struct device *dev = &pdev->dev; + struct rzg3l_lvds *lvds; + 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; + + lvds->regmap = syscon_node_to_regmap(dev->of_node->parent); + if (IS_ERR(lvds->regmap)) + return PTR_ERR(lvds->regmap); + + rstc = devm_reset_control_get_optional_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_optional_exclusive(dev, "arst"); + if (IS_ERR(arstc)) + return dev_err_probe(dev, PTR_ERR(arstc), + "failed to get arst\n"); + + lvds->prstc = devm_reset_control_get_shared(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_shared(dev, "lvdrst"); + if (IS_ERR(lvds->lvd_rstc)) + return dev_err_probe(dev, PTR_ERR(lvds->lvd_rstc), + "failed to get core reset\n"); + + platform_set_drvdata(pdev, lvds); + ret = devm_pm_runtime_enable(dev); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable Runtime PM\n"); + + lvds->bridge.next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(lvds->bridge.next_bridge)) + return dev_err_probe(dev, PTR_ERR(lvds->bridge.next_bridge), + "failed to get next bridge\n"); + + ret = reset_control_assert(rstc); + if (ret < 0) + return ret; + + ret = reset_control_assert(arstc); + if (ret < 0) + return ret; + + ret = devm_drm_bridge_add(dev, &lvds->bridge); + if (ret) + return dev_err_probe(dev, ret, + "Failed to register drm bridge\n"); + + return ret; +} + +static const struct of_device_id rzg3l_lvds_of_table[] = { + { .compatible = "renesas,r9a08g046-lvds" }, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, rzg3l_lvds_of_table); + +static struct platform_driver rzg3l_lvds_platform_driver = { + .probe = rzg3l_lvds_probe, + .driver = { + .name = "rzg3l-lvds", + .pm = pm_ptr(&rzg3l_lvds_pm_ops), + .of_match_table = rzg3l_lvds_of_table, + }, +}; + +module_platform_driver(rzg3l_lvds_platform_driver); + +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>"); +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>"); +MODULE_DESCRIPTION("Renesas RZ/G3L LVDS Encoder Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h new file mode 100644 index 000000000000..281b7648f168 --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * RZ/G3L LVDS Interface Registers Definitions + * + * Copyright (C) 2026 Renesas Electronics Corporation + * + */ + +#ifndef __RZG3L_LVDS_REGS_H__ +#define __RZG3L_LVDS_REGS_H__ + +#define LVDS_CMN 0x00 +#define LVDS_CMN_RST_PHY0_SEL (1 << 24) +#define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) +#define LVDS_CMN_PHY_RESET (1 << 0) + +#define LVDS_0_PHY_OFFSET 0x10 +#define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) +#define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) +#define LVDS_0_PHY_CH_EN_BGR BIT(8) +#define LVDS_0_PHY_CH_EN_LDO BIT(9) + +#define LVDS_0_CTL_OFFSET 0x14 +#define LVDS_0_CTL_FMT_SEL_MSK GENMASK(23, 20) + +#endif /* __RZG3L_LVDS_REGS_H__ */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Claude review: drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-05-24 19:44 ` [PATCH v2 2/2] drm: renesas: rz-du: Add support for " Biju @ 2026-05-25 6:56 ` Claude Code Review Bot 0 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-05-25 6:56 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Issues:** **(1) `rst` and `arst` reset controls are fire-and-forget (medium severity)** In `rzg3l_lvds_probe()`, the `rstc` and `arstc` resets are acquired, asserted, and then the local variables go out of scope. They are never stored in `struct rzg3l_lvds`, so they can never be deasserted: ```c struct reset_control *rstc, *arstc; ... rstc = devm_reset_control_get_optional_exclusive(dev, "rst"); ... arstc = devm_reset_control_get_optional_exclusive(dev, "arst"); ... ret = reset_control_assert(rstc); ... ret = reset_control_assert(arstc); ``` Per the cover letter, these resets "must be asserted before using the LVDS module" and should stay asserted. If this is intentional (assert once, never deassert), it works because `devm_*` will release the reset control on driver remove, but a comment would clarify the intent. Additionally, using `_exclusive` reset controls that are only asserted and never deasserted is unusual — if they truly must stay asserted for the lifetime of the device, consider whether `_shared` would be more appropriate if the DSI driver also needs to manipulate them. **(2) `pm_runtime_get_sync` error path leaks PM refcount (medium severity)** In `rzg3l_lvds_atomic_enable()`: ```c if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) return; ``` `pm_runtime_get_sync()` increments the reference count even on failure. On error, you must call `pm_runtime_put_noidle()` or `pm_runtime_put()` to avoid a PM refcount leak. The matching `pm_runtime_put()` in `rzg3l_lvds_atomic_disable()` will never be called if enable bailed out. Consider: ```c if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) { pm_runtime_put_noidle(lvds->dev); return; } ``` Or alternatively, use `pm_runtime_resume_and_get()` which doesn't increment the refcount on failure. **(3) Unused includes (minor)** Several headers appear unnecessary: - `linux/of_device.h` — nothing from this header is used (no `of_device_get_match_data()` etc.) - `linux/of_graph.h` — no `of_graph_*` functions are called - `linux/bitfield.h` — wait, `FIELD_PREP` is used, so this one is needed - `linux/mfd/syscon.h` — `syscon_node_to_regmap()` is used, so needed - `linux/regmap.h` — `regmap_*` functions used, needed - `linux/media-bus-format.h` — `MEDIA_BUS_FMT_*` used, needed - `drm/drm_panel.h` — no panel functions called directly, but `DRM_PANEL` is selected in Kconfig. This include is not needed in this source file. - `drm/drm_probe_helper.h` — no probe helper functions called. **(4) Register header mixes `BIT()` and raw shifts (minor style)** In `rzg3l_lvds_regs.h`: ```c #define LVDS_CMN_RST_PHY0_SEL (1 << 24) #define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) #define LVDS_CMN_PHY_RESET (1 << 0) ``` These should use `BIT()` for consistency with the rest of the file which uses `BIT(8)`, `BIT(9)`, `GENMASK()`: ```c #define LVDS_0_PHY_CH_EN_BGR BIT(8) #define LVDS_0_PHY_CH_EN_LDO BIT(9) ``` Also, `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are defined to the same value `(1 << 24)` — one of them appears redundant. **(5) `LVDS_0_PHY_CH_IO_EN` is redundant (minor)** ```c #define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) #define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) ``` `LVDS_0_PHY_CH_IO_EN` is just `LVDS_0_PHY_CH_IO_EN_MSK << 0`, which is the same value. Having both is confusing. This could be simplified, or at least the `<< 0` should be removed. **(6) `select DRM_PANEL` in Kconfig may be unnecessary (minor)** The driver doesn't use any panel API directly — it uses `devm_drm_of_get_bridge()` which handles panel wrapping via `drm_panel_bridge`. The `DRM_PANEL` symbol is typically pulled in transitively by the bridge infrastructure. Check whether `DRM_PANEL` is really needed here. **(7) `select DRM_KMS_HELPER` — check necessity (minor)** Similar to above, `DRM_KMS_HELPER` is often pulled in by `DRM_BRIDGE`. Verify it's needed as a direct dependency. **(8) The `return ret;` at the end of probe could be `return 0;` (nitpick)** ```c ret = devm_drm_bridge_add(dev, &lvds->bridge); if (ret) return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); return ret; ``` At this point `ret` is always 0 (the error case already returned). Using `return 0;` is clearer. **(9) Version numbering confusion (meta)** The cover letter changelog says "v2->v3" but the subject lines say `[PATCH v2 0/2]`. This appears to actually be v3 based on the changelog content. **(10) Bridge attach returns 0 when there's no next_bridge (needs verification)** ```c static int rzg3l_lvds_attach(...) { struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); if (!lvds->bridge.next_bridge) return 0; return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags); } ``` The `next_bridge` is set from `devm_drm_of_get_bridge()` in probe, which returns an error if not found. So `next_bridge` should always be non-NULL if probe succeeded. This NULL check is harmless defensive code but shouldn't be reachable. Not a bug, but the check is dead code given the current probe logic. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 0/3] Add support for Renesas RZ/G3L LVDS encoder
@ 2026-04-21 17:29 Biju
2026-04-21 17:29 ` [PATCH v2 3/3] drm: renesas: rz-du: Add support for " Biju
0 siblings, 1 reply; 8+ messages in thread
From: Biju @ 2026-04-21 17:29 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Philipp Zabel, Geert Uytterhoeven, Magnus Damm
Cc: Biju Das, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Tommaso Merciai, dri-devel, devicetree, linux-kernel,
linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das
From: Biju Das <biju.das.jz@bp.renesas.com>
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.
v1->v2:
* Collected the tags for binding patches.
* Dropped unused function rzg3l_lvds_is_connected() and removed the
corresponding header file rzg3l_lvds.h
* Dropped next_bridge from struct rzg3l_lvds instead using bridge's
next_bridge.
* Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as
atomic_enable doesn't fail and for each enable there always will be an
atomic_disable() call.
* Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback.
* Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe()
* Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend,
resume}().
Biju Das (3):
dt-bindings: mfd: syscon: Document the LVDS_CMN syscon for the RZ/G3L
dt-bindings: display: bridge: Document Renesas RZ/G3L LVDS encoder
drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder
.../bridge/renesas,r9a08g046-lvds.yaml | 128 ++++++++
.../devicetree/bindings/mfd/syscon.yaml | 1 +
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 | 285 ++++++++++++++++++
.../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++
6 files changed, 454 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,r9a08g046-lvds.yaml
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
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 3/3] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-04-21 17:29 [PATCH v2 0/3] Add support for Renesas " Biju @ 2026-04-21 17:29 ` Biju 2026-04-22 22:20 ` Claude review: " Claude Code Review Bot 0 siblings, 1 reply; 8+ messages in thread From: Biju @ 2026-04-21 17:29 UTC (permalink / raw) To: Biju Das, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Philipp Zabel, Geert Uytterhoeven, Magnus Damm Cc: linux-kernel, dri-devel, linux-renesas-soc, Prabhakar Mahadev Lad, Biju Das, Tommaso Merciai From: Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * Dropped unused function rzg3l_lvds_is_connected() and removed the corresponding header file rzg3l_lvds.h * Dropped next_bridge from struct rzg3l_lvds instead using bridge's next_bridge. * Replaced pm_runtime_resume_and_get()->pm_runtime_get_sync() as atomic_enable doesn't fail and for each enable there always will be an atomic_disable() call. * Started using DEFINE_RUNTIME_DEV_PM_OPS for PM callback. * Replaced rzg3l_lvds_parse_dt() with devm_drm_of_get_bridge() in probe() * Started using reset_control_bulk_*() in rzg3l_lvds_pm_runtime_{suspend, resume}() --- 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 | 285 ++++++++++++++++++ .../gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h | 26 ++ 4 files changed, 325 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/Kconfig b/drivers/gpu/drm/renesas/rz-du/Kconfig index 7f2ef7137ae5..cbfc7b6bccb8 100644 --- a/drivers/gpu/drm/renesas/rz-du/Kconfig +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig @@ -26,3 +26,16 @@ config DRM_RZG2L_MIPI_DSI def_tristate DRM_RZG2L_DU depends on DRM_RZG2L_USE_MIPI_DSI select DRM_MIPI_DSI + +config DRM_RZG3L_USE_LVDS + bool "RZ/G3L DU LVDS Encoder Support" + depends on DRM_BRIDGE && OF + default DRM_RZG2L_DU + help + Enable support for the RZ/G3L Display Unit embedded LVDS encoders. + +config DRM_RZG3L_LVDS + def_tristate DRM_RZG2L_DU + depends on DRM_RZG3L_USE_LVDS + select DRM_KMS_HELPER + select DRM_PANEL diff --git a/drivers/gpu/drm/renesas/rz-du/Makefile b/drivers/gpu/drm/renesas/rz-du/Makefile index 2987900ea6b6..46decb7ac4f1 100644 --- a/drivers/gpu/drm/renesas/rz-du/Makefile +++ b/drivers/gpu/drm/renesas/rz-du/Makefile @@ -8,3 +8,4 @@ rzg2l-du-drm-$(CONFIG_VIDEO_RENESAS_VSP1) += rzg2l_du_vsp.o obj-$(CONFIG_DRM_RZG2L_DU) += rzg2l-du-drm.o obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o +obj-$(CONFIG_DRM_RZG3L_LVDS) += rzg3l_lvds.o 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..60fd63b7be93 --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds.c @@ -0,0 +1,285 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * RZ/G3L LVDS Encoder Driver + * + * Copyright (C) 2026 Renesas Electronics Corporation + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/media-bus-format.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_probe_helper.h> + +#include "rzg3l_lvds_regs.h" + +enum rzg3l_lvds_mode { + RZG3L_LVDS_MODE_JEIDA = 0, + RZG3L_LVDS_MODE_JEIDA_MIRROR = 1, + RZG3L_LVDS_MODE_MODE2 = 2, + RZG3L_LVDS_MODE_MODE2_MIRROR = 3, + RZG3L_LVDS_MODE_VESA = 4, + RZG3L_LVDS_MODE_VESA_MIRROR = 5, + RZG3L_LVDS_MODE_MODE6 = 6, + RZG3L_LVDS_MODE_MODE6_MIRROR = 7, +}; + +struct rzg3l_lvds { + struct device *dev; + struct reset_control *prstc; + struct reset_control *lvd_rstc; + struct regmap *regmap; + struct drm_bridge bridge; +}; + +#define bridge_to_rzg3l_lvds(b) \ + container_of(b, struct rzg3l_lvds, bridge) + +/* ----------------------------------------------------------------------------- + * Bridge + */ + +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + const struct drm_bridge_state *bridge_state; + int ret; + u32 fmt; + + /* Get the LVDS format from the bridge state. */ + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + if (!bridge_state) { + dev_err(lvds->dev, "failed to get bridge state\n"); + return; + } + + switch (bridge_state->output_bus_cfg.format) { + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: + fmt = RZG3L_LVDS_MODE_JEIDA; + break; + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: + fmt = RZG3L_LVDS_MODE_VESA; + break; + default: + fmt = RZG3L_LVDS_MODE_VESA; + dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n", + bridge_state->output_bus_cfg.format); + break; + } + + ret = pm_runtime_get_sync(lvds->dev); + if (ret < 0) { + dev_err(lvds->dev, "pm_runtime_get_sync error\n"); + return; + } + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, LVDS_0_PHY_CH_EN_BGR); + fsleep(20); + + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, LVDS_0_PHY_CH_EN_LDO); + fsleep(10); + + regmap_write(lvds->regmap, LVDS_CMN, LVDS_CMN_RST_PHY0_SEL); + regmap_update_bits(lvds->regmap, LVDS_0_CTL_OFFSET, + LVDS_0_CTL_FMT_SEL_MSK, + FIELD_PREP(LVDS_0_CTL_FMT_SEL_MSK, fmt)); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, LVDS_0_PHY_CH_IO_EN); + regmap_write(lvds->regmap, LVDS_CMN, + LVDS_CMN_RST_PHY0_SEL | LVDS_CMN_PHY_RESET); + fsleep(100); +} + +static void rzg3l_lvds_atomic_disable(struct drm_bridge *bridge, + struct drm_atomic_state *state) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + regmap_update_bits(lvds->regmap, LVDS_CMN, LVDS_CMN_PHY_RESET, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_IO_EN_MSK, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_LDO, 0); + regmap_update_bits(lvds->regmap, LVDS_0_PHY_OFFSET, + LVDS_0_PHY_CH_EN_BGR, 0); + + pm_runtime_put(lvds->dev); +} + +static int rzg3l_lvds_attach(struct drm_bridge *bridge, + struct drm_encoder *encoder, + enum drm_bridge_attach_flags flags) +{ + struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge); + + if (!lvds->bridge.next_bridge) + return 0; + + return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags); +} + +static enum drm_mode_status +rzg3l_lvds_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock > 87000) + return MODE_CLOCK_HIGH; + + if (mode->clock < 25000) + return MODE_CLOCK_LOW; + + return MODE_OK; +} + +static const struct drm_bridge_funcs rzg3l_lvds_bridge_ops = { + .attach = rzg3l_lvds_attach, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_enable = rzg3l_lvds_atomic_enable, + .atomic_disable = rzg3l_lvds_atomic_disable, + .mode_valid = rzg3l_lvds_bridge_mode_valid, +}; + +/* ----------------------------------------------------------------------------- + * Power Management + */ + +static int rzg3l_lvds_pm_runtime_suspend(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->lvd_rstc }, + { .rstc = lvds->prstc }, + }; + + return reset_control_bulk_assert(ARRAY_SIZE(resets), resets); +} + +static int rzg3l_lvds_pm_runtime_resume(struct device *dev) +{ + struct rzg3l_lvds *lvds = dev_get_drvdata(dev); + struct reset_control_bulk_data resets[] = { + { .rstc = lvds->prstc }, + { .rstc = lvds->lvd_rstc }, + }; + + return reset_control_bulk_deassert(ARRAY_SIZE(resets), resets); +} + +static DEFINE_RUNTIME_DEV_PM_OPS(rzg3l_lvds_pm_ops, + rzg3l_lvds_pm_runtime_suspend, + rzg3l_lvds_pm_runtime_resume, NULL); + +/* ----------------------------------------------------------------------------- + * Probe & Remove + */ + +static int rzg3l_lvds_probe(struct platform_device *pdev) +{ + struct reset_control *rstc, *arstc; + struct device *dev = &pdev->dev; + struct drm_bridge *next_bridge; + struct rzg3l_lvds *lvds; + 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; + + lvds->regmap = syscon_node_to_regmap(dev->of_node->parent); + if (IS_ERR(lvds->regmap)) + return PTR_ERR(lvds->regmap); + + rstc = devm_reset_control_get_optional_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_optional_exclusive(dev, "arst"); + if (IS_ERR(arstc)) + return dev_err_probe(dev, PTR_ERR(arstc), + "failed to get arst\n"); + + lvds->prstc = devm_reset_control_get_shared(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_shared(dev, "lvdrst"); + if (IS_ERR(lvds->lvd_rstc)) + return dev_err_probe(dev, PTR_ERR(lvds->lvd_rstc), + "failed to get core reset\n"); + + platform_set_drvdata(pdev, lvds); + ret = devm_pm_runtime_enable(dev); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable Runtime PM\n"); + + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(next_bridge)) + return dev_err_probe(dev, PTR_ERR(next_bridge), + "failed to get next bridge\n"); + + lvds->bridge.next_bridge = next_bridge; + ret = reset_control_assert(rstc); + if (ret < 0) + return ret; + + ret = reset_control_assert(arstc); + if (ret < 0) + return ret; + + ret = devm_drm_bridge_add(dev, &lvds->bridge); + if (ret) + return dev_err_probe(dev, ret, + "Failed to register drm bridge\n"); + + return ret; +} + +static const struct of_device_id rzg3l_lvds_of_table[] = { + { .compatible = "renesas,r9a08g046-lvds" }, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, rzg3l_lvds_of_table); + +static struct platform_driver rzg3l_lvds_platform_driver = { + .probe = rzg3l_lvds_probe, + .driver = { + .name = "rzg3l-lvds", + .pm = pm_ptr(&rzg3l_lvds_pm_ops), + .of_match_table = rzg3l_lvds_of_table, + }, +}; + +module_platform_driver(rzg3l_lvds_platform_driver); + +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>"); +MODULE_AUTHOR("Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>"); +MODULE_DESCRIPTION("Renesas RZ/G3L LVDS Encoder Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h new file mode 100644 index 000000000000..281b7648f168 --- /dev/null +++ b/drivers/gpu/drm/renesas/rz-du/rzg3l_lvds_regs.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * RZ/G3L LVDS Interface Registers Definitions + * + * Copyright (C) 2026 Renesas Electronics Corporation + * + */ + +#ifndef __RZG3L_LVDS_REGS_H__ +#define __RZG3L_LVDS_REGS_H__ + +#define LVDS_CMN 0x00 +#define LVDS_CMN_RST_PHY0_SEL (1 << 24) +#define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) +#define LVDS_CMN_PHY_RESET (1 << 0) + +#define LVDS_0_PHY_OFFSET 0x10 +#define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) +#define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) +#define LVDS_0_PHY_CH_EN_BGR BIT(8) +#define LVDS_0_PHY_CH_EN_LDO BIT(9) + +#define LVDS_0_CTL_OFFSET 0x14 +#define LVDS_0_CTL_FMT_SEL_MSK GENMASK(23, 20) + +#endif /* __RZG3L_LVDS_REGS_H__ */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Claude review: drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder 2026-04-21 17:29 ` [PATCH v2 3/3] drm: renesas: rz-du: Add support for " Biju @ 2026-04-22 22:20 ` Claude Code Review Bot 0 siblings, 0 replies; 8+ messages in thread From: Claude Code Review Bot @ 2026-04-22 22:20 UTC (permalink / raw) To: dri-devel-reviews Patch Review This is the main driver patch. Several items to address: **1. Unnecessary includes** (`rzg3l_lvds.c` lines 9, 12, 16, 17, 27, 28): ```c #include <linux/clk.h> #include <linux/io.h> #include <linux/of_device.h> #include <linux/of_graph.h> ... #include <drm/drm_panel.h> #include <drm/drm_probe_helper.h> ``` - `clk.h` — no `clk_*` calls anywhere in the driver (clocks managed by pm_runtime/syscon parent). - `io.h` — no `ioread`/`iowrite`/`readl`/`writel` calls; registers accessed via regmap. - `of_device.h` — no `of_match_device()` or other `of_device` APIs used. The sibling `rzg2l_mipi_dsi.c` doesn't include this. - `of_graph.h` — no `of_graph_*` APIs called directly. - `drm_panel.h` — no `drm_panel_*` functions called anywhere. - `drm_probe_helper.h` — no probe helper functions called. Please drop these unused includes. **2. Register header: redundant and inconsistent definitions** (`rzg3l_lvds_regs.h`): ```c #define LVDS_CMN_RST_PHY0_SEL (1 << 24) #define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) #define LVDS_CMN_PHY_RESET (1 << 0) ``` `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` have the same value and `_CH0` is never used. Drop `LVDS_CMN_RST_PHY0_SEL_CH0`. Also, the file mixes `(1 << N)` with `BIT(N)` (used for bits 8 and 9). Use `BIT()` consistently throughout: ```c #define LVDS_CMN_RST_PHY0_SEL BIT(24) #define LVDS_CMN_PHY_RESET BIT(0) ``` Similarly: ```c #define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) #define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) ``` `LVDS_0_PHY_CH_IO_EN` is `0x1f << 0` which equals `LVDS_0_PHY_CH_IO_EN_MSK`. These are the same value, and the `<< 0` is a no-op. Should be `GENMASK(4, 0)` for the mask, and `LVDS_0_PHY_CH_IO_EN` can be dropped (just use the mask directly, since the value written is the mask itself). **3. `pm_runtime_get_sync` error handling** (`rzg3l_lvds.c` lines 87-91): ```c ret = pm_runtime_get_sync(lvds->dev); if (ret < 0) { dev_err(lvds->dev, "pm_runtime_get_sync error\n"); return; } ``` The v2 changelog explains the switch from `pm_runtime_resume_and_get()` to `pm_runtime_get_sync()` because "atomic_enable doesn't fail and for each enable there always will be an atomic_disable() call." This rationale is valid — the DRM framework always pairs enable/disable. However, `pm_runtime_get_sync()` increments the usage count even on failure. If it fails and `atomic_disable` later calls `pm_runtime_put`, the count rebalances but the hardware was never properly initialized, and `atomic_disable` will write teardown register sequences to a device that never fully resumed. Consider either: - Using `pm_runtime_resume_and_get()` for cleaner semantics (the extra `pm_runtime_put` in disable on failure path is harmless since it just decrements to 0), or - Dropping the error check entirely if this truly can't fail (since `atomic_enable` returns void and can't propagate errors anyway). The current code is a hybrid that checks for errors but can't properly handle them. **4. Probe ordering** (`rzg3l_lvds.c` lines 242-254): ```c next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(next_bridge)) return dev_err_probe(dev, PTR_ERR(next_bridge), "failed to get next bridge\n"); lvds->bridge.next_bridge = next_bridge; ret = reset_control_assert(rstc); ... ret = reset_control_assert(arstc); ``` The `rst` and `arst` resets (MIPI_DSI_CMN_RSTB and MIPI_DSI_ARESET_N) must be asserted as a precondition for LVDS operation. Currently they're asserted *after* `devm_drm_of_get_bridge`, which can return `-EPROBE_DEFER`. Consider moving the reset assertions earlier (right after obtaining them), so they're asserted regardless of deferred probe. This avoids repeated assert/defer/assert cycles and ensures the hardware precondition is established as early as possible. **5. Dead code in attach** (`rzg3l_lvds.c` lines 134-135): ```c if (!lvds->bridge.next_bridge) return 0; ``` Since `probe()` returns an error if `devm_drm_of_get_bridge()` fails, `next_bridge` is always non-NULL by the time the bridge is registered and `attach` can be called. This check is dead code. It's a common defensive pattern and not harmful, but worth noting. **6. Kconfig: `select DRM_PANEL` likely unnecessary**: ``` config DRM_RZG3L_LVDS def_tristate DRM_RZG2L_DU depends on DRM_RZG3L_USE_LVDS select DRM_KMS_HELPER select DRM_PANEL ``` The driver doesn't call any `drm_panel_*` functions. Panel support is handled transparently through the bridge chain (via `devm_drm_of_get_bridge` which wraps panels as bridges). `select DRM_PANEL` and the `drm_panel.h` include appear to be unnecessary. `select DRM_KMS_HELPER` is also already selected by `DRM_RZG2L_DU` (which this depends on transitively via `def_tristate DRM_RZG2L_DU`), though being explicit isn't wrong. **7. Minor: `return ret` at end of probe** (`rzg3l_lvds.c` line 261): ```c ret = devm_drm_bridge_add(dev, &lvds->bridge); if (ret) return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); return ret; ``` At this point `ret` is always 0. Clearer to write `return 0;`. **Summary for Patch 3:** The driver is functionally correct and follows established patterns. The main items to address are: cleaning up the unused includes, making the register definitions consistent (use `BIT()`/`GENMASK()`, drop redundant defs), and reconsidering the `pm_runtime_get_sync` error handling approach. The probe ordering of reset assertions could also be improved. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-05-27 5:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-26 7:47 [PATCH v3 0/2] Add support for Renesas RZ/G3L LVDS encoder Biju 2026-05-26 7:47 ` [PATCH v3 1/2] dt-bindings: display: bridge: Document " Biju 2026-05-27 5:11 ` Claude review: " Claude Code Review Bot 2026-05-26 7:47 ` [PATCH v3 2/2] drm: renesas: rz-du: Add support for " Biju 2026-05-27 5:11 ` Claude review: " Claude Code Review Bot 2026-05-27 5:11 ` Claude review: Add support for Renesas " Claude Code Review Bot -- strict thread matches above, loose matches on Subject: below -- 2026-05-24 19:44 [PATCH v2 0/2] " Biju 2026-05-24 19:44 ` [PATCH v2 2/2] drm: renesas: rz-du: Add support for " Biju 2026-05-25 6:56 ` Claude review: " Claude Code Review Bot 2026-04-21 17:29 [PATCH v2 0/3] Add support for Renesas " Biju 2026-04-21 17:29 ` [PATCH v2 3/3] drm: renesas: rz-du: Add support for " Biju 2026-04-22 22:20 ` Claude review: " Claude Code Review Bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox