Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 16 Apr 2021 12:30:04 +0100
From:   Jonathan McDowell <noodles@earth.li>
To:     Heiko Stuebner <heiko@sntech.de>
Cc:     Sandy Huang <hjc@rock-chips.com>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/rockchip: Cope with endpoints that haven't been
 registered yet
Message-ID: <20210416113004.GW11733@earth.li>
References: <20210316182753.GA25685@earth.li>
 <3104631.44csPzL39Z@phil>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3104631.44csPzL39Z@phil>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Sun, Mar 21, 2021 at 07:58:13PM +0100, Heiko Stuebner wrote:
> Am Dienstag, 16. M?rz 2021, 19:27:53 CET schrieb Jonathan McDowell:
> > The Rockchip RGB CRTC output driver attempts to avoid probing Rockchip
> > subdrivers to see if they're a connected panel or bridge. However part
> > of its checks assumes that if no OF platform device is found then it
> > can't be a valid bridge or panel. This causes issues with I2C controlled
> > bridges that have not yet been registered to the point they can be
> > found.
> > 
> > Change this to return EPROBE_DEFER instead of ENODEV and don't ignore
> > such devices. The subsequent call to drm_of_find_panel_or_bridge() will
> > return EPROBE_DEFER as well if there's actually a valid device we should
> > wait for.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++--
> >  drivers/gpu/drm/rockchip/rockchip_rgb.c     | 7 ++++---
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > index 212bd87c0c4a..b0d63a566501 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > @@ -270,11 +270,15 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)
> >  	if (!node)
> >  		return -ENODEV;
> >  
> > -	/* status disabled will prevent creation of platform-devices */
> > +	/*
> > +	 * status disabled will prevent creation of platform-devices,
> > +	 * but equally we can't rely on the driver having been registered
> > +	 * yet (e.g. I2C bridges).
> > +	 */
> >  	pdev = of_find_device_by_node(node);
> >  	of_node_put(node);
> >  	if (!pdev)
> > -		return -ENODEV;
> > +		return -EPROBE_DEFER;
> 
> In general, how does that relate to i2c-bridge-drivers, as
> of_find_device_by_node supposedly only acts on platform-devices?

I think the problem here is that not finding the device node means we
return an error here, which means it's not actually possible to attach
an i2c bridge driver to the Rockchip RGB interface at present.

> Also if that points to a disabled bridge (hdmi, etc) that would likely make
> it probe-defer indefinitly, as that device will never become available?
> 
> Maybe we could do something like of_device_is_available() which checks
> the status property of the node. So something like:
> 
>   	pdev = of_find_device_by_node(node);
>   	if (!pdev) {
> 		bool avail = of_device_is_available(node);
> 
> 		of_node_put(node);
> 
> 		/* if disabled
> 		if (!avail)
> 			return -ENODEV;
> 		else
> 			return -EPROBE_DEFER;
> 	}
>   	of_node_put(node);
> 
> Though I still do not understand how that should actually pick up on
> i2c devices at all.

of_find_device_by_node will fail here, as it's not a platform device,
but then of_device_is_available should return true so I think I can
actually just return false here rather than EPROBE_DEFER - because if
it's not a platform device then it's not a subdriver, which is what
we're checking for.

I'll re-roll and test this weekend and post an updated revision. Thanks
for the pointers.

J.

-- 
101 things you can't have too much of : 3 - Sleep.