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;
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Jagan Teki <jagan@amarulasolutions.com>
Cc:     Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-amarula@amarulasolutions.com, linux-sunxi@googlegroups.com
References: <20210322140152.101709-1-jagan@amarulasolutions.com>
 <20210322140152.101709-2-jagan@amarulasolutions.com>
 <YFpxYpA+EIZm7sOf@pendragon.ideasonboard.com>
From:   Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge
Message-ID: <f47bc0ad-dbd6-05b5-aaec-2e3256e3715a@sholland.org>
Date:   Tue, 23 Mar 2021 21:48:53 -0500
User-Agent: Mozilla/5.0 (X11; Linux ppc64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.1
MIME-Version: 1.0
In-Reply-To: <YFpxYpA+EIZm7sOf@pendragon.ideasonboard.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> Hi Jagan,
> 
> Thank you for the patch.
> 
> On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
>> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
>> for finding panel, this indeed help to find the bridge if
>> bridge support added.
>>
>> Added NULL in bridge argument, same will replace with bridge
>> parameter once bridge supported.
>>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> 
> Looks good, there should be no functional change.

Actually this breaks all existing users of this driver, see below.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>> Changes for v4, v3:
>> - none
>>
>>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> index 4f5efcace68e..2e9e7b2d4145 100644
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -21,6 +21,7 @@
>>  
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_of.h>
>>  #include <drm/drm_panel.h>
>>  #include <drm/drm_print.h>
>>  #include <drm/drm_probe_helper.h>
>> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>>  			    struct mipi_dsi_device *device)
>>  {
>>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
>> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

This is using the OF node of the DSI device, which is a direct child of
the DSI host's OF node. There is no OF graph involved.

>> +	struct drm_panel *panel;
>> +	int ret;
>> +
>> +	ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
>> +					  &panel, NULL);

However, this function expects to find the panel using OF graph. This
does not work with existing device trees (PinePhone, PineTab) which do
not use OF graph to connect the panel. And it cannot work, because the
DSI host's binding specifies a single port: the input port from the
display engine.

Regards,
Samuel

>> +	if (ret)
>> +		return ret;
>>  
>> -	if (IS_ERR(panel))
>> -		return PTR_ERR(panel);
>>  	if (!dsi->drm || !dsi->drm->registered)
>>  		return -EPROBE_DEFER;
>>  
>