Re: [PATCH v3 14/19] drm/bridge: starfive: Add JH7110 HDMI controller driver

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 9/7/26 05:51, Chaoyi Chen wrote:
> Hello Michal,
> 
> On 9/4/2026 9:27 PM, Michal Wilczynski wrote:
>> Add the HDMI controller (bridge) driver for the StarFive JH7110.
>>
>> This driver binds to the starfive,jh7110-inno-hdmi-controller node.
>> It gets its shared regmap from its parent and its register access,
>> module and bus clocks from voutcrg. It consumes the pixel clock and the
>> PHY from its hdmi_phy sibling.
>>
>> The driver calls the generic inno_hdmi_probe function and passes the
>> shared regmap to it, registering as a DRM bridge. The .enable hook is
>> responsible for setting the PHY's pixel clock rate via clk_set_rate()
>> and powering on the PHY via phy_power_on().
>>
>> The DC8200 has two panels, each exposing a DP and a DPI interface, and a
>> mux in the video output system controller picks which of them drives the
>> HDMI transmitter. Program that mux from the port graph rather than
>> relying on whatever the bootloader left behind, taking the panel from the
>> remote port number and the interface from the remote endpoint number.
>>
>> The generic driver holds the clock it looks up as the register access
>> clock enabled for its lifetime, and derives the DDC divider from that
>> clock's rate, so point it at the system clock. Naming the pixel clock
>> there instead would keep the PHY pre-PLL powered from probe onwards and
>> size the divider from the wrong rate.
>>
>> The PHY can only generate the discrete set of pixel clocks described by
>> its pre-PLL table, so .mode_valid rejects any mode clk_round_rate()
>> cannot satisfy. Without it such a mode would be advertised to userspace
>> and the modeset would appear to succeed while the display stayed blank.
>>
>> .enable returns early when the rate is unsupported or the PHY fails to
>> power on, so track whether the pixel clock was actually enabled and let
>> .disable tear down only what was brought up, otherwise the clock
>> refcount underflows.
>>
>> The clocks and the reset are torn down through devm rather than from
>> .remove, so that they outlive the bridge that inno_hdmi_probe() adds with
>> devm_drm_bridge_add(). Releasing them in .remove runs before devres
>> unwinds and would leave the bridge registered with its clocks already
>> gated.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig            |  11 ++
>>  drivers/gpu/drm/bridge/Makefile           |   1 +
>>  drivers/gpu/drm/bridge/jh7110-inno-hdmi.c | 318 ++++++++++++++++++++++++++++++
>>  3 files changed, 330 insertions(+)
>>

[snip]

>> +static enum drm_mode_status
>> +inno_hdmi_starfive_mode_valid(struct device *dev,
>> +			      const struct drm_display_mode *mode)
>> +{
>> +	struct stf_inno_hdmi_controller *ctrl = dev_get_drvdata(dev);
>> +	unsigned long pixelclk = mode->clock * 1000;
>> +	long rounded;
>> +
>> +	/*
>> +	 * The PHY can only generate the discrete set of pixel clocks described
>> +	 * by its pre-PLL table, and clk_round_rate() fails for anything else.
>> +	 * Reject those modes here: without this the modeset would appear to
>> +	 * succeed while the PHY never produces a signal.
>> +	 */
>> +	rounded = clk_round_rate(ctrl->clks[CLK_PCLK].clk, pixelclk);
>> +	if (rounded < 0 || rounded != pixelclk)
>> +		return MODE_NOCLOCK;
>> +
> 
> Using "if (rounded != pixelclk)" would be ok.

Thanks will fix.

> 
>> +	return MODE_OK;
>> +}
>> +

[snip]

>> +/*
>> + * The DC8200 has two panels, each exposing a DP and a DPI interface, and a mux
>> + * in dom_vout_syscon picks which of them drives the HDMI transmitter. Derive
>> + * the mux setting from the port graph: the remote port number selects the
>> + * DC8200 panel, and the remote endpoint number the interface on that panel
>> + * (0 for DPI, 1 for DP). Both drive 8-bit RGB, the only format this driver
>> + * currently produces.
>> + */
>> +static int stf_inno_hdmi_setup_mux(struct device *dev)
>> +{
>> +	struct device_node *ep, *remote;
> 
> Using "struct device_node *ep __free(device_node)" can help you simplify
> the processing of resource release.
> 

Sure, thanks !

> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>