[PATCH 1/2] [media] s5p-g2d: Add DT based discovery support

[Sachin Kamat]

Hi Sylwester.

Thank you for the review.

On 31 January 2013 03:08, Sylwester Nawrocki
<sylvester.nawrocki at gmail.com> wrote:
> Hi Sachin,
>
>
> On 01/25/2013 10:55 AM, Sachin Kamat wrote:
>>
>> This patch adds device tree based discovery support to G2D driver
>>
>> Signed-off-by: Sachin Kamat<sachin.kamat at linaro.org>
>> ---
>
> Don' you need something like:
>
>         else {
>                 of_id = of_match_node(exynos_g2d_match, pdev->dev.of_node);
>                 if (!of_id)
>                         return -ENODEV;
>                 dev->variant = (struct g2d_variant *)of_id->data;
>         }
> ?
>
> Otherwise dev->variant is left uninitialized...?

Exactly. The above code is very much required. Not sure how I missed it :(

>
>
>>         return 0;
>>
>> @@ -844,6 +846,18 @@ static struct g2d_variant g2d_drvdata_v4x = {
>>         .hw_rev = TYPE_G2D_4X, /* Revision 4.1 for Exynos4X12 and Exynos5
>> */
>>   };
>>
>> +static const struct of_device_id exynos_g2d_match[] = {
>> +       {
>> +               .compatible = "samsung,g2d-v3",
>> +               .data =&g2d_drvdata_v3x,
>> +       }, {
>> +               .compatible = "samsung,g2d-v41",
>> +               .data =&g2d_drvdata_v4x,
>
>
> Didn't you consider adding "exynos" to these compatible strings ?
> I'm afraid g2d may be too generic.

Choosing the right compatible string seems to be the biggest challenge :)
I did consider adding "exynos" to the compatible strings, but then MFC
used it as "mfc-v5" and I followed the same example. Prepending exynos
makes it more specific and should be added (even to MFC) IMO too.
We need to arrive at a consensus about the bindings (right now for
g2d) as they would be common irrespective of DRM or V4L2 framework.
Please let me know your opinion about Inki's suggestion to use version
property instead.

>
>
>> +       },
>> +       {},
>> +               .of_match_table = of_match_ptr(exynos_g2d_match),
>
>
> of_match_ptr() could be dropped, since exynos_g2d_match[] is
> always compiled in.

OK.

Once I get confirmation about the compatible strings, I will resend
this patch with other suggested updates.

-- 
With warm regards,
Sachin