Re: [PATCH v5 08/21] drm/bridge: inno-hdmi: Split probe out of bind

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

On 9/30/26 12:02, Heiko Stübner wrote:
> Am Dienstag, 29. September 2026, 12:31:05 Mitteleuropäische Sommerzeit schrieb Michal Wilczynski:
>> inno_hdmi_bind() both sets up the bridge and attaches it to a DRM
>> encoder. A platform whose HDMI controller is a child of a larger device
>> needs the first half without the second, since it registers as its own
>> platform driver and lets the DRM core bind the bridge later.
>>
>> Move the setup into a new exported inno_hdmi_probe(), and reduce
>> inno_hdmi_bind() to a wrapper around it.
>>
>> No functional change intended.
>>
>> Reviewed-by: Joshua Peisach <jpeisach@ubuntu.com>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/bridge/inno-hdmi.c | 49 +++++++++++++++++++++++++++++++++-----
>>  include/drm/bridge/inno_hdmi.h     |  4 ++++
>>  2 files changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/inno-hdmi.c b/drivers/gpu/drm/bridge/inno-hdmi.c
>> index d7c33b92e2ddf4ced84d17304463f44be52270b8..7a9f54dce2647f0320d77629a28154eade14385f 100644
>> --- a/drivers/gpu/drm/bridge/inno-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/inno-hdmi.c
>> @@ -929,7 +929,14 @@ static irqreturn_t inno_hdmi_irq(int irq, void *dev_id)
>>  {
>>  	struct inno_hdmi *hdmi = dev_id;
>>  
>> -	drm_helper_hpd_irq_event(hdmi->bridge.dev);
>> +	/*
>> +	 * The interrupt is requested in probe, but bridge.dev is only set once
>> +	 * the DRM master binds and attaches the bridge, which may never happen.
>> +	 * Drop hotplug events that arrive before then rather than dereference a
>> +	 * NULL drm_device.
>> +	 */
>> +	if (hdmi->bridge.dev)
>> +		drm_helper_hpd_irq_event(hdmi->bridge.dev);
> 
> This is not part of the commit description and while true, should likely
> be its own patch with its own description.


It only made sense with this patch, hence why it's included. Happy to
split it.

> 
> 
>>  
>>  	return IRQ_HANDLED;
>>  }
>> @@ -1061,11 +1068,24 @@ static struct i2c_adapter *inno_hdmi_i2c_adapter(struct inno_hdmi *hdmi)
>>  	return adap;
>>  }
>>  
>> -struct inno_hdmi *inno_hdmi_bind(struct device *dev,
>> -				 struct drm_encoder *encoder,
>> -				 const struct inno_hdmi_plat_data *plat_data)
>> +/**
>> + * inno_hdmi_probe - Internal helper to perform common setup
>> + * @pdev: platform device
>> + * @plat_data: SoC-specific platform data
>> + *
>> + * This function handles all the common hardware setup: allocating the main
>> + * struct, mapping registers, getting clocks, initializing the hardware,
>> + * setting up the IRQ, and initializing the DDC adapter and bridge struct.
>> + * It returns a pointer to the inno_hdmi struct on success, or an ERR_PTR
>> + * on failure.
>> + *
>> + * This function is used by modern, decoupled MFD/glue drivers. It registers
>> + * the bridge but does not attach it.
>> + */
>> +struct inno_hdmi *inno_hdmi_probe(struct platform_device *pdev,
>> +				  const struct inno_hdmi_plat_data *plat_data)
>>  {
>> -	struct platform_device *pdev = to_platform_device(dev);
>> +	struct device *dev = &pdev->dev;
>>  	struct inno_hdmi *hdmi;
>>  	int irq;
>>  	int ret;
>> @@ -1128,7 +1148,24 @@ struct inno_hdmi *inno_hdmi_bind(struct device *dev,
>>  	if (ret)
>>  		return ERR_PTR(ret);
>>  
>> -	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +	return hdmi;
>> +}
>> +EXPORT_SYMBOL_GPL(inno_hdmi_probe);
>> +
>> +struct inno_hdmi *inno_hdmi_bind(struct device *dev,
>> +				 struct drm_encoder *encoder,
>> +				 const struct inno_hdmi_plat_data *plat_data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct inno_hdmi *hdmi;
>> +	int ret;
>> +
>> +	hdmi = inno_hdmi_probe(pdev, plat_data);
>> +	if (IS_ERR(hdmi))
>> +		return hdmi;
>> +
>> +	ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL,
>> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  	if (ret)
>>  		return ERR_PTR(ret);
> 
> This clutters up the structure even more than it is right now. From "bind"
> back to "probe" and "dev" back to "pdev" and calling a function called
> "probe" from the bind callback.
> 
> You are right, that all the resource allocation can (and probably should)
> live in the drivers probe-path, but then please call that probe function
> from the actual probe path.
> 
> For example you could look at how all the Synopsys bridges do that.

That is what the new jh7110 driver does - it is a plain platform driver
and calls inno_hdmi_probe() from its own probe no component framework.
Same shape as th1520-dw-hdmi.c which calls dw_hdmi_probe() from its
platform probe and never calls dw_hdmi_bind().

I kept inno_hdmi_bind() as a wrapper on purpose so the rockchip probe/bind
flow stays exactly as it is today. I do not have any rockchip hardware and
moving resource acquisition out of bind and into probe there changes when the
bridge is added, which I did not want to do build tested only. dw-hdmi keeps
dw_hdmi_bind() for the same kind of consumer since dw_hdmi-rockchip.c is
still component based.

I am happy to convert inno_hdmi-rockchip.c so probe acquires the
resources and bind only attaches - that would let inno_hdmi_bind() go
away entirely - if someone with rockchip hardware can test it. Otherwise
I would rather send it separately where it can get proper testing, as
this series is at 21 patches already.

You are right about pdev - inno_hdmi_bind() will take a platform_device like
dw_hdmi_bind() does so the to_platform_device() goes away.

> 
> 
> Thanks
> Heiko
> 
> 
> 

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