diff mbox

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

Message ID 1359107722-9974-1-git-send-email-sachin.kamat@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sachin Kamat Jan. 25, 2013, 9:55 a.m. UTC
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(-)

Comments

Sylwester Nawrocki Jan. 30, 2013, 9:38 p.m. UTC | #1
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
Sachin Kamat Jan. 31, 2013, 6:29 a.m. UTC | #2
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.
diff mbox

Patch

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);
 
 	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,
+	},
+	{},
+};
+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),
 	},
 };