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

This patch adds device tree based discovery support to G2D driver

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/s5p-g2d/g2d.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

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@linaro.org>
> ---
>   drivers/media/platform/s5p-g2d/g2d.c |   17 ++++++++++++++++-
>   1 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
> index 7e41529..210e142 100644
> --- a/drivers/media/platform/s5p-g2d/g2d.c
> +++ b/drivers/media/platform/s5p-g2d/g2d.c
> @@ -18,6 +18,7 @@
>   #include<linux/slab.h>
>   #include<linux/clk.h>
>   #include<linux/interrupt.h>
> +#include<linux/of.h>
>
>   #include<linux/platform_device.h>
>   #include<media/v4l2-mem2mem.h>
> @@ -796,7 +797,8 @@ static int g2d_probe(struct platform_device *pdev)
>   	}
>
>   	def_frame.stride = (def_frame.width * def_frame.fmt->depth)>>  3;
> -	dev->variant = g2d_get_drv_data(pdev);
> +	if (!pdev->dev.of_node)
> +		dev->variant = g2d_get_drv_data(pdev);

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

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

> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_g2d_match);
> +
>   static struct platform_device_id g2d_driver_ids[] = {
>   	{
>   		.name = "s5p-g2d",
> @@ -863,6 +877,7 @@ static struct platform_driver g2d_pdrv = {
>   	.driver		= {
>   		.name = G2D_NAME,
>   		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(exynos_g2d_match),

of_match_ptr() could be dropped, since exynos_g2d_match[] is
always compiled in.

--

Thanks,
Sylwester

Hi Sylwester.

Thank you for the review.

On 31 January 2013 03:08, Sylwester Nawrocki
<sylvester.nawrocki@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@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.