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

[Jyri Sarha <jsarha@ti.com>]

On 03/02/15 18:01, Russell King - ARM Linux wrote:
> 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.
>

Will remove.

>> +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...
>

That is otherwise fine, but with the match code it not possible to 
implement a master that may not have any components.

Would it be Ok to add a check that master->ops->add_components is 
defined, before calling it in find_componets() 
(drivers/base/component.c:120) and return 0 if it is not?

Best regards,
Jyri

>> @@ -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.
>