Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder - Russell King

From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Jyri Sarha <jsarha@ti.com>
Cc: devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	detheridge@ti.com, tony@atomide.com, tomi.valkeinen@ti.com,
	bcousson@baylibre.com, linux-omap@vger.kernel.org
Subject: Re: [PATCH RFC 3/6] drm/tilcdc: Add support for external compontised DRM encoder
Date: Mon, 2 Mar 2015 16:01:35 +0000	[thread overview]
Message-ID: <20150302160135.GA29584@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <92792bb4f64fb1b7f26e687ec159d870d0e7a81a.1424961754.git.jsarha@ti.com>

On Thu, Feb 26, 2015 at 04:55:32PM +0200, Jyri Sarha wrote:
> +	ret = component_bind_all(dev->dev, dev);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "Binding subcomponents failed: %d\n", ret);

Do you need to print this?  The component helper is already fairly
verbose about what succeeds and fails.

> +static const struct component_master_ops tilcdc_comp_ops = {
> +	.add_components = tilcdc_add_external_components,

I'd much rather you used the new matching support rather than using the
old .add_components.  The new matching support is more efficient as you
only have to scan DT once, rather than each time we try to probe.  That
will mean...

> @@ -613,12 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
>  		return -ENXIO;
>  	}

You need to build a struct component_match array here using
component_match_add()...

>  
> -	return drm_platform_init(&tilcdc_driver, pdev);
> +	return component_master_add(&pdev->dev, &tilcdc_comp_ops);

and then finally register the ops with component_master_add_with_match().

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel