* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-12 21:44 ` Sylwester Nawrocki
0 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-12 21:44 UTC (permalink / raw)
To: Ajay Kumar
Cc: linux-samsung-soc, linux-fbdev, jg1.han, devicetree-discuss,
FlorianSchandinat, sylvester.nawrocki, tomasz.figa, thomas.ab
On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> This patch enables device tree based discovery support for DP driver.
> The driver is modified to handle platform data in both the cases:
> with DT and non-DT.
>
> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> Acked-by: Jingoo Han<jg1.han@samsung.com>
> ---
> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
> drivers/video/exynos/exynos_dp_core.h | 2 +
> 2 files changed, 149 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index d55470e..62932ba 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -18,6 +18,7 @@
> #include<linux/io.h>
> #include<linux/interrupt.h>
> #include<linux/delay.h>
> +#include<linux/of.h>
>
> #include<video/exynos_dp.h>
>
> @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> +#ifdef CONFIG_OF
> +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
> +{
> + struct device_node *dp_node = dev->of_node;
> + struct exynos_dp_platdata *pd;
> + struct video_info *dp_video_config;
> +
> + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> + if (!pd) {
> + dev_err(dev, "memory allocation for pdata failed\n");
> + return ERR_PTR(-ENOMEM);
> + }
> + dp_video_config = devm_kzalloc(dev,
> + sizeof(*dp_video_config), GFP_KERNEL);
> +
> + if (!dp_video_config) {
> + dev_err(dev, "memory allocation for video config failed\n");
> + return ERR_PTR(-ENOMEM);
> + }
> + pd->video_info = dp_video_config;
> +
> + if (of_get_property(dp_node, "hsync-active-high", NULL))
> + dp_video_config->h_sync_polarity = 1;
> +
> + if (of_get_property(dp_node, "vsync-active-high", NULL))
> + dp_video_config->v_sync_polarity = 1;
> +
> + if (of_get_property(dp_node, "interlaced", NULL))
> + dp_video_config->interlaced = 1;
of_property_read_bool() could also be used here.
> +
> + of_property_read_u32(dp_node, "samsung,color-space",
> + &dp_video_config->color_space);
> +
> + of_property_read_u32(dp_node, "samsung,dynamic-range",
> + &dp_video_config->dynamic_range);
> +
> + of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> + &dp_video_config->ycbcr_coeff);
> +
> + of_property_read_u32(dp_node, "samsung,color-depth",
> + &dp_video_config->color_depth);
> +
> + of_property_read_u32(dp_node, "samsung,link-rate",
> + &dp_video_config->link_rate);
> +
> + of_property_read_u32(dp_node, "samsung,lane-count",
> + &dp_video_config->lane_count);
Shouldn't it be checked whether all these required properties are present ?
If someone forgets to specify any one the driver will silently ignore it,
not giving a clue what's wrong.
> + return pd;
> +}
> +
> +void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
> +{
> + struct device_node *dp_phy_node;
> + u32 phy_base;
> + void *virt_phy_base;
> +
> + dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
> + if (!dp_phy_node) {
> + dp->dp_phy_addr = NULL;
Is this required ? You've allocated dp with kzalloc().
> + return;
> + }
> +
> + of_property_read_u32(dp_phy_node, "reg",&phy_base);
> + of_property_read_u32(dp_phy_node, "samsung,enable-mask",
> + &dp->enable_mask);
I think you should check the return values, these are required properties.
Some error logs might be useful here.
> + virt_phy_base = ioremap(phy_base, SZ_4);
How about assigning to dp->dp_phy_addr directly and gettting rid of
virt_phy_base ?
> + if (!virt_phy_base) {
> + dev_err(dp->dev, "failed to ioremap dp-phy\n");
> + dp->dp_phy_addr = NULL;
> + return;
> + }
> + dp->dp_phy_addr = virt_phy_base;
> +}
> +
> +void exynos_dp_phy_init(struct exynos_dp_device *dp)
> +{
> + u32 reg;
> +
> + reg = __raw_readl(dp->dp_phy_addr);
> + reg |= dp->enable_mask;
> + __raw_writel(reg, dp->dp_phy_addr);
> +}
> +
> +void exynos_dp_phy_exit(struct exynos_dp_device *dp)
> +{
> + u32 reg;
> +
> + reg = __raw_readl(dp->dp_phy_addr);
> + reg&= ~(dp->enable_mask);
> + __raw_writel(reg, dp->dp_phy_addr);
> +}
> +#endif /* CONFIG_OF */
> +
> static int __devinit exynos_dp_probe(struct platform_device *pdev)
> {
> struct resource *res;
> @@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>
> int ret = 0;
>
> - pdata = pdev->dev.platform_data;
> - if (!pdata) {
> - dev_err(&pdev->dev, "no platform data\n");
> - return -EINVAL;
> - }
> -
> dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> GFP_KERNEL);
> if (!dp) {
> @@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>
> dp->dev =&pdev->dev;
>
> + if (pdev->dev.of_node) {
> + pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> +
> + exynos_dp_dt_parse_phydata(dp);
> + } else {
> + pdata = pdev->dev.platform_data;
> + }
> +
> + if (!pdata) {
> + dev_err(&pdev->dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
This check is only reqired for non-dt case, might be worth to move
it under else statement above.
> dp->clock = devm_clk_get(&pdev->dev, "dp");
> if (IS_ERR(dp->clock)) {
> dev_err(&pdev->dev, "failed to get clock\n");
> @@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> }
>
> dp->video_info = pdata->video_info;
> - if (pdata->phy_init)
> - pdata->phy_init();
> +
> + if (pdev->dev.of_node) {
> + if (dp->dp_phy_addr)
> + exynos_dp_phy_init(dp);
> + } else {
> + if (pdata->phy_init)
> + pdata->phy_init();
> + }
>
> exynos_dp_init_dp(dp);
>
> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
This doesn't belong to your patch, but the above 2 lines could be written as:
struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
Same in functions exynos_dp_suspend() and exynos_dp_resume().
>
> - if (pdata&& pdata->phy_exit)
> - pdata->phy_exit();
> + if (pdev->dev.of_node) {
> + if (dp->dp_phy_addr)
> + exynos_dp_phy_exit(dp);
> + } else {
> + if (pdata&& pdata->phy_exit)
> + pdata->phy_exit();
> + }
It is not possible to have valid dp->dp_phy_addr pointer without
valid pdev->dev.of_node, is it ?
Can't this (and all similar occurrences) be simplified to:
if (dp->dp_phy_addr)
exynos_dp_phy_exit(dp);
else if (pdata->phy_exit)
pdata->phy_exit();
?
It just requires dp->dp_phy_addr being NULL for non-dt case.
pdata is never NULL, otherwise probe() would have already failed.
> clk_disable_unprepare(dp->clock);
>
> @@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
> - if (pdata&& pdata->phy_exit)
> - pdata->phy_exit();
> + if (dev->of_node) {
> + if (dp->dp_phy_addr)
> + exynos_dp_phy_exit(dp);
> + } else {
> + if (pdata&& pdata->phy_exit)
> + pdata->phy_exit();
> + }
>
> clk_disable_unprepare(dp->clock);
>
> @@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
> - if (pdata&& pdata->phy_init)
> - pdata->phy_init();
> + if (dev->of_node) {
> + if (dp->dp_phy_addr)
> + exynos_dp_phy_init(dp);
> + } else {
> + if (pdata&& pdata->phy_init)
> + pdata->phy_init();
> + }
>
> clk_prepare_enable(dp->clock);
>
> @@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
> };
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id exynos_dp_match[] = {
> + { .compatible = "samsung,exynos5-dp" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_dp_match);
> +#endif
> +
How about just dropping this #ifdef, since you're using of_match_ptr() ?
> static struct platform_driver exynos_dp_driver = {
> .probe = exynos_dp_probe,
> .remove = __devexit_p(exynos_dp_remove),
> @@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
> .name = "exynos-dp",
> .owner = THIS_MODULE,
> .pm =&exynos_dp_pm_ops,
> + .of_match_table = of_match_ptr(exynos_dp_match),
> },
> };
>
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 57b8a65..569858b 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -29,6 +29,8 @@ struct exynos_dp_device {
> struct clk *clock;
> unsigned int irq;
> void __iomem *reg_base;
> + void __iomem *dp_phy_addr;
Nit: "dp" is already within the structure name. How about just
naming it phy_addr ? dp->phy_addr might look better than
dp->dp_phy_addr ;)
> + unsigned int enable_mask;
>
> struct video_info *video_info;
> struct link_train link_train;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-12 21:44 ` Sylwester Nawrocki
@ 2012-10-12 21:54 ` Tomasz Figa
-1 siblings, 0 replies; 22+ messages in thread
From: Tomasz Figa @ 2012-10-12 21:54 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Ajay Kumar, linux-samsung-soc, linux-fbdev, jg1.han,
devicetree-discuss, FlorianSchandinat, thomas.ab
Dnia piątek, 12 października 2012 23:44:05 Sylwester Nawrocki pisze:
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> >
> > drivers/video/exynos/exynos_dp_core.c | 161
> > ++++++++++++++++++++++++++++++---
> > drivers/video/exynos/exynos_dp_core.h | 2 +
> > 2 files changed, 149 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c
> > b/drivers/video/exynos/exynos_dp_core.c index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> >
> > #include<linux/io.h>
> > #include<linux/interrupt.h>
> > #include<linux/delay.h>
> >
> > +#include<linux/of.h>
> >
> > #include<video/exynos_dp.h>
> >
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq,
> > void *arg)>
> > return IRQ_HANDLED;
> >
> > }
> >
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device
> > *dev)
> > +{
> > + struct device_node *dp_node = dev->of_node;
> > + struct exynos_dp_platdata *pd;
> > + struct video_info *dp_video_config;
> > +
> > + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > + if (!pd) {
> > + dev_err(dev, "memory allocation for pdata failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + dp_video_config = devm_kzalloc(dev,
> > + sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > + if (!dp_video_config) {
> > + dev_err(dev, "memory allocation for video config failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + pd->video_info = dp_video_config;
> > +
> > + if (of_get_property(dp_node, "hsync-active-high", NULL))
> > + dp_video_config->h_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "vsync-active-high", NULL))
> > + dp_video_config->v_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "interlaced", NULL))
> > + dp_video_config->interlaced = 1;
>
> of_property_read_bool() could also be used here.
Wouldn't it make the property mandatory?
I would just use of_find_property here, instead of of_get_property.
Best regards,
Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-12 21:54 ` Tomasz Figa
0 siblings, 0 replies; 22+ messages in thread
From: Tomasz Figa @ 2012-10-12 21:54 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Ajay Kumar, linux-samsung-soc, linux-fbdev, jg1.han,
devicetree-discuss, FlorianSchandinat, thomas.ab
Dnia piątek, 12 października 2012 23:44:05 Sylwester Nawrocki pisze:
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> >
> > drivers/video/exynos/exynos_dp_core.c | 161
> > ++++++++++++++++++++++++++++++---
> > drivers/video/exynos/exynos_dp_core.h | 2 +
> > 2 files changed, 149 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c
> > b/drivers/video/exynos/exynos_dp_core.c index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> >
> > #include<linux/io.h>
> > #include<linux/interrupt.h>
> > #include<linux/delay.h>
> >
> > +#include<linux/of.h>
> >
> > #include<video/exynos_dp.h>
> >
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq,
> > void *arg)>
> > return IRQ_HANDLED;
> >
> > }
> >
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device
> > *dev)
> > +{
> > + struct device_node *dp_node = dev->of_node;
> > + struct exynos_dp_platdata *pd;
> > + struct video_info *dp_video_config;
> > +
> > + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > + if (!pd) {
> > + dev_err(dev, "memory allocation for pdata failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + dp_video_config = devm_kzalloc(dev,
> > + sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > + if (!dp_video_config) {
> > + dev_err(dev, "memory allocation for video config failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + pd->video_info = dp_video_config;
> > +
> > + if (of_get_property(dp_node, "hsync-active-high", NULL))
> > + dp_video_config->h_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "vsync-active-high", NULL))
> > + dp_video_config->v_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "interlaced", NULL))
> > + dp_video_config->interlaced = 1;
>
> of_property_read_bool() could also be used here.
Wouldn't it make the property mandatory?
I would just use of_find_property here, instead of of_get_property.
Best regards,
Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-12 21:54 ` Tomasz Figa
@ 2012-10-12 22:14 ` Sylwester Nawrocki
-1 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-12 22:14 UTC (permalink / raw)
To: Tomasz Figa
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
FlorianSchandinat-Mmb7MZpHnFY,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
jg1.han-Sze3O3UU22JBDgjK7y7TUQ, thomas.ab-Sze3O3UU22JBDgjK7y7TUQ,
Sylwester Nawrocki, Ajay Kumar
On 10/12/2012 11:54 PM, Tomasz Figa wrote:
>>> + if (of_get_property(dp_node, "interlaced", NULL))
>>> + dp_video_config->interlaced = 1;
>>
>> of_property_read_bool() could also be used here.
>
> Wouldn't it make the property mandatory?
No, it wouldn't. of_property_read_bool() uses of_find_property()
internally. It just returns true if property is found or false
otherwise. Anyway, it appears of_get_property(..., NULL) pattern
is commonly used for boolean properties.
> I would just use of_find_property here, instead of of_get_property.
>
> Best regards,
> Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-12 22:14 ` Sylwester Nawrocki
0 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-12 22:14 UTC (permalink / raw)
To: Tomasz Figa
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
FlorianSchandinat-Mmb7MZpHnFY,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
jg1.han-Sze3O3UU22JBDgjK7y7TUQ, thomas.ab-Sze3O3UU22JBDgjK7y7TUQ,
Sylwester Nawrocki, Ajay Kumar
On 10/12/2012 11:54 PM, Tomasz Figa wrote:
>>> + if (of_get_property(dp_node, "interlaced", NULL))
>>> + dp_video_config->interlaced = 1;
>>
>> of_property_read_bool() could also be used here.
>
> Wouldn't it make the property mandatory?
No, it wouldn't. of_property_read_bool() uses of_find_property()
internally. It just returns true if property is found or false
otherwise. Anyway, it appears of_get_property(..., NULL) pattern
is commonly used for boolean properties.
> I would just use of_find_property here, instead of of_get_property.
>
> Best regards,
> Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-12 22:14 ` Sylwester Nawrocki
@ 2012-10-12 22:22 ` Tomasz Figa
-1 siblings, 0 replies; 22+ messages in thread
From: Tomasz Figa @ 2012-10-12 22:22 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Ajay Kumar, linux-samsung-soc, linux-fbdev, jg1.han,
devicetree-discuss, FlorianSchandinat, thomas.ab
On Saturday 13 of October 2012 00:14:39 Sylwester Nawrocki wrote:
> On 10/12/2012 11:54 PM, Tomasz Figa wrote:
> >>> + if (of_get_property(dp_node, "interlaced", NULL))
> >>> + dp_video_config->interlaced = 1;
> >>
> >> of_property_read_bool() could also be used here.
> >
> > Wouldn't it make the property mandatory?
>
> No, it wouldn't. of_property_read_bool() uses of_find_property()
> internally. It just returns true if property is found or false
> otherwise.
Right, sorry. I thought that all of_property_read_* return error in case
of missing property.
> Anyway, it appears of_get_property(..., NULL) pattern
> is commonly used for boolean properties.
I guess all three of them should be fine in this case, but since there is
a dedicated function for bool, it might be the best solution here indeed.
Best regards,
Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-12 22:22 ` Tomasz Figa
0 siblings, 0 replies; 22+ messages in thread
From: Tomasz Figa @ 2012-10-12 22:22 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Ajay Kumar, linux-samsung-soc, linux-fbdev, jg1.han,
devicetree-discuss, FlorianSchandinat, thomas.ab
On Saturday 13 of October 2012 00:14:39 Sylwester Nawrocki wrote:
> On 10/12/2012 11:54 PM, Tomasz Figa wrote:
> >>> + if (of_get_property(dp_node, "interlaced", NULL))
> >>> + dp_video_config->interlaced = 1;
> >>
> >> of_property_read_bool() could also be used here.
> >
> > Wouldn't it make the property mandatory?
>
> No, it wouldn't. of_property_read_bool() uses of_find_property()
> internally. It just returns true if property is found or false
> otherwise.
Right, sorry. I thought that all of_property_read_* return error in case
of missing property.
> Anyway, it appears of_get_property(..., NULL) pattern
> is commonly used for boolean properties.
I guess all three of them should be fine in this case, but since there is
a dedicated function for bool, it might be the best solution here indeed.
Best regards,
Tomasz Figa
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-12 21:44 ` Sylwester Nawrocki
@ 2012-10-15 8:14 ` Jingoo Han
-1 siblings, 0 replies; 22+ messages in thread
From: Jingoo Han @ 2012-10-15 8:14 UTC (permalink / raw)
To: 'Sylwester Nawrocki', 'Ajay Kumar'
Cc: linux-samsung-soc, linux-fbdev, devicetree-discuss,
FlorianSchandinat, tomasz.figa, thomas.ab, 'Jingoo Han'
On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> > drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
> > drivers/video/exynos/exynos_dp_core.h | 2 +
> > 2 files changed, 149 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> > index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> > #include<linux/io.h>
> > #include<linux/interrupt.h>
> > #include<linux/delay.h>
> > +#include<linux/of.h>
> >
> > #include<video/exynos_dp.h>
> >
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> > return IRQ_HANDLED;
> > }
> >
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
> > +{
> > + struct device_node *dp_node = dev->of_node;
> > + struct exynos_dp_platdata *pd;
> > + struct video_info *dp_video_config;
> > +
> > + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > + if (!pd) {
> > + dev_err(dev, "memory allocation for pdata failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + dp_video_config = devm_kzalloc(dev,
> > + sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > + if (!dp_video_config) {
> > + dev_err(dev, "memory allocation for video config failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + pd->video_info = dp_video_config;
> > +
> > + if (of_get_property(dp_node, "hsync-active-high", NULL))
> > + dp_video_config->h_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "vsync-active-high", NULL))
> > + dp_video_config->v_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "interlaced", NULL))
> > + dp_video_config->interlaced = 1;
>
> of_property_read_bool() could also be used here.
OK, right.
of_property_read_bool() would be better.
>
> > +
> > + of_property_read_u32(dp_node, "samsung,color-space",
> > + &dp_video_config->color_space);
> > +
> > + of_property_read_u32(dp_node, "samsung,dynamic-range",
> > + &dp_video_config->dynamic_range);
> > +
> > + of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> > + &dp_video_config->ycbcr_coeff);
> > +
> > + of_property_read_u32(dp_node, "samsung,color-depth",
> > + &dp_video_config->color_depth);
> > +
> > + of_property_read_u32(dp_node, "samsung,link-rate",
> > + &dp_video_config->link_rate);
> > +
> > + of_property_read_u32(dp_node, "samsung,lane-count",
> > + &dp_video_config->lane_count);
>
> Shouldn't it be checked whether all these required properties are present ?
> If someone forgets to specify any one the driver will silently ignore it,
> not giving a clue what's wrong.
OK, right.
>
> > + return pd;
> > +}
> > +
> > +void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
> > +{
> > + struct device_node *dp_phy_node;
> > + u32 phy_base;
> > + void *virt_phy_base;
> > +
> > + dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
> > + if (!dp_phy_node) {
> > + dp->dp_phy_addr = NULL;
>
> Is this required ? You've allocated dp with kzalloc().
OK, right.
>
> > + return;
> > + }
> > +
> > + of_property_read_u32(dp_phy_node, "reg",&phy_base);
> > + of_property_read_u32(dp_phy_node, "samsung,enable-mask",
> > + &dp->enable_mask);
>
> I think you should check the return values, these are required properties.
> Some error logs might be useful here.
OK, right.
>
> > + virt_phy_base = ioremap(phy_base, SZ_4);
>
> How about assigning to dp->dp_phy_addr directly and gettting rid of
> virt_phy_base ?
OK, right.
There is no reason, virt_phy_base should be removed.
>
> > + if (!virt_phy_base) {
> > + dev_err(dp->dev, "failed to ioremap dp-phy\n");
> > + dp->dp_phy_addr = NULL;
> > + return;
> > + }
> > + dp->dp_phy_addr = virt_phy_base;
> > +}
> > +
> > +void exynos_dp_phy_init(struct exynos_dp_device *dp)
> > +{
> > + u32 reg;
> > +
> > + reg = __raw_readl(dp->dp_phy_addr);
> > + reg |= dp->enable_mask;
> > + __raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +
> > +void exynos_dp_phy_exit(struct exynos_dp_device *dp)
> > +{
> > + u32 reg;
> > +
> > + reg = __raw_readl(dp->dp_phy_addr);
> > + reg&= ~(dp->enable_mask);
> > + __raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +#endif /* CONFIG_OF */
> > +
> > static int __devinit exynos_dp_probe(struct platform_device *pdev)
> > {
> > struct resource *res;
> > @@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> > int ret = 0;
> >
> > - pdata = pdev->dev.platform_data;
> > - if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > -
> > dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> > GFP_KERNEL);
> > if (!dp) {
> > @@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> > dp->dev =&pdev->dev;
> >
> > + if (pdev->dev.of_node) {
> > + pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
> > + if (IS_ERR(pdata))
> > + return PTR_ERR(pdata);
> > +
> > + exynos_dp_dt_parse_phydata(dp);
> > + } else {
> > + pdata = pdev->dev.platform_data;
> > + }
> > +
> > + if (!pdata) {
> > + dev_err(&pdev->dev, "no platform data\n");
> > + return -EINVAL;
> > + }
> > +
>
> This check is only reqired for non-dt case, might be worth to move
> it under else statement above.
OK, right.
>
> > dp->clock = devm_clk_get(&pdev->dev, "dp");
> > if (IS_ERR(dp->clock)) {
> > dev_err(&pdev->dev, "failed to get clock\n");
> > @@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> > }
> >
> > dp->video_info = pdata->video_info;
> > - if (pdata->phy_init)
> > - pdata->phy_init();
> > +
> > + if (pdev->dev.of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_init(dp);
> > + } else {
> > + if (pdata->phy_init)
> > + pdata->phy_init();
> > + }
> >
> > exynos_dp_init_dp(dp);
> >
> > @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
> This doesn't belong to your patch, but the above 2 lines could be written as:
>
> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>
> Same in functions exynos_dp_suspend() and exynos_dp_resume().
No, above 2 lines cannot be reduced to 1 line, as you mentioned.
This is because it makes build error.
>
> >
> > - if (pdata&& pdata->phy_exit)
> > - pdata->phy_exit();
> > + if (pdev->dev.of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_exit(dp);
> > + } else {
> > + if (pdata&& pdata->phy_exit)
> > + pdata->phy_exit();
> > + }
>
> It is not possible to have valid dp->dp_phy_addr pointer without
> valid pdev->dev.of_node, is it ?
Right, however, I prefer coding style as bellow:
if (pdev->dev.of_node)
DT
else
non-DT
>
> Can't this (and all similar occurrences) be simplified to:
>
> if (dp->dp_phy_addr)
> exynos_dp_phy_exit(dp);
> else if (pdata->phy_exit)
> pdata->phy_exit();
> ?
>
> It just requires dp->dp_phy_addr being NULL for non-dt case.
> pdata is never NULL, otherwise probe() would have already failed.
OK, right.
pdata can be removed.
>
> > clk_disable_unprepare(dp->clock);
> >
> > @@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > - if (pdata&& pdata->phy_exit)
> > - pdata->phy_exit();
> > + if (dev->of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_exit(dp);
> > + } else {
> > + if (pdata&& pdata->phy_exit)
> > + pdata->phy_exit();
> > + }
> >
> > clk_disable_unprepare(dp->clock);
> >
> > @@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > - if (pdata&& pdata->phy_init)
> > - pdata->phy_init();
> > + if (dev->of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_init(dp);
> > + } else {
> > + if (pdata&& pdata->phy_init)
> > + pdata->phy_init();
> > + }
> >
> > clk_prepare_enable(dp->clock);
> >
> > @@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
> > SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
> > };
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id exynos_dp_match[] = {
> > + { .compatible = "samsung,exynos5-dp" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos_dp_match);
> > +#endif
> > +
>
> How about just dropping this #ifdef, since you're using of_match_ptr() ?
OK, right.
>
> > static struct platform_driver exynos_dp_driver = {
> > .probe = exynos_dp_probe,
> > .remove = __devexit_p(exynos_dp_remove),
> > @@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
> > .name = "exynos-dp",
> > .owner = THIS_MODULE,
> > .pm =&exynos_dp_pm_ops,
> > + .of_match_table = of_match_ptr(exynos_dp_match),
> > },
> > };
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> > index 57b8a65..569858b 100644
> > --- a/drivers/video/exynos/exynos_dp_core.h
> > +++ b/drivers/video/exynos/exynos_dp_core.h
> > @@ -29,6 +29,8 @@ struct exynos_dp_device {
> > struct clk *clock;
> > unsigned int irq;
> > void __iomem *reg_base;
> > + void __iomem *dp_phy_addr;
>
> Nit: "dp" is already within the structure name. How about just
> naming it phy_addr ? dp->phy_addr might look better than
> dp->dp_phy_addr ;)
OK, right.
>
> > + unsigned int enable_mask;
> >
> > struct video_info *video_info;
> > struct link_train link_train;
Ajay,
When CONFIG_OF is not enabled, it makes build errors.
In addition, exynos_dp_dt_parse_phydata needs return value,
instead of returning void.
Best regards,
Jingoo Han
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-15 8:14 ` Jingoo Han
0 siblings, 0 replies; 22+ messages in thread
From: Jingoo Han @ 2012-10-15 8:14 UTC (permalink / raw)
To: 'Sylwester Nawrocki', 'Ajay Kumar'
Cc: linux-samsung-soc, linux-fbdev, devicetree-discuss,
FlorianSchandinat, tomasz.figa, thomas.ab, 'Jingoo Han'
On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> > drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
> > drivers/video/exynos/exynos_dp_core.h | 2 +
> > 2 files changed, 149 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> > index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> > #include<linux/io.h>
> > #include<linux/interrupt.h>
> > #include<linux/delay.h>
> > +#include<linux/of.h>
> >
> > #include<video/exynos_dp.h>
> >
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> > return IRQ_HANDLED;
> > }
> >
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
> > +{
> > + struct device_node *dp_node = dev->of_node;
> > + struct exynos_dp_platdata *pd;
> > + struct video_info *dp_video_config;
> > +
> > + pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > + if (!pd) {
> > + dev_err(dev, "memory allocation for pdata failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + dp_video_config = devm_kzalloc(dev,
> > + sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > + if (!dp_video_config) {
> > + dev_err(dev, "memory allocation for video config failed\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > + pd->video_info = dp_video_config;
> > +
> > + if (of_get_property(dp_node, "hsync-active-high", NULL))
> > + dp_video_config->h_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "vsync-active-high", NULL))
> > + dp_video_config->v_sync_polarity = 1;
> > +
> > + if (of_get_property(dp_node, "interlaced", NULL))
> > + dp_video_config->interlaced = 1;
>
> of_property_read_bool() could also be used here.
OK, right.
of_property_read_bool() would be better.
>
> > +
> > + of_property_read_u32(dp_node, "samsung,color-space",
> > + &dp_video_config->color_space);
> > +
> > + of_property_read_u32(dp_node, "samsung,dynamic-range",
> > + &dp_video_config->dynamic_range);
> > +
> > + of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> > + &dp_video_config->ycbcr_coeff);
> > +
> > + of_property_read_u32(dp_node, "samsung,color-depth",
> > + &dp_video_config->color_depth);
> > +
> > + of_property_read_u32(dp_node, "samsung,link-rate",
> > + &dp_video_config->link_rate);
> > +
> > + of_property_read_u32(dp_node, "samsung,lane-count",
> > + &dp_video_config->lane_count);
>
> Shouldn't it be checked whether all these required properties are present ?
> If someone forgets to specify any one the driver will silently ignore it,
> not giving a clue what's wrong.
OK, right.
>
> > + return pd;
> > +}
> > +
> > +void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
> > +{
> > + struct device_node *dp_phy_node;
> > + u32 phy_base;
> > + void *virt_phy_base;
> > +
> > + dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
> > + if (!dp_phy_node) {
> > + dp->dp_phy_addr = NULL;
>
> Is this required ? You've allocated dp with kzalloc().
OK, right.
>
> > + return;
> > + }
> > +
> > + of_property_read_u32(dp_phy_node, "reg",&phy_base);
> > + of_property_read_u32(dp_phy_node, "samsung,enable-mask",
> > + &dp->enable_mask);
>
> I think you should check the return values, these are required properties.
> Some error logs might be useful here.
OK, right.
>
> > + virt_phy_base = ioremap(phy_base, SZ_4);
>
> How about assigning to dp->dp_phy_addr directly and gettting rid of
> virt_phy_base ?
OK, right.
There is no reason, virt_phy_base should be removed.
>
> > + if (!virt_phy_base) {
> > + dev_err(dp->dev, "failed to ioremap dp-phy\n");
> > + dp->dp_phy_addr = NULL;
> > + return;
> > + }
> > + dp->dp_phy_addr = virt_phy_base;
> > +}
> > +
> > +void exynos_dp_phy_init(struct exynos_dp_device *dp)
> > +{
> > + u32 reg;
> > +
> > + reg = __raw_readl(dp->dp_phy_addr);
> > + reg |= dp->enable_mask;
> > + __raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +
> > +void exynos_dp_phy_exit(struct exynos_dp_device *dp)
> > +{
> > + u32 reg;
> > +
> > + reg = __raw_readl(dp->dp_phy_addr);
> > + reg&= ~(dp->enable_mask);
> > + __raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +#endif /* CONFIG_OF */
> > +
> > static int __devinit exynos_dp_probe(struct platform_device *pdev)
> > {
> > struct resource *res;
> > @@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> > int ret = 0;
> >
> > - pdata = pdev->dev.platform_data;
> > - if (!pdata) {
> > - dev_err(&pdev->dev, "no platform data\n");
> > - return -EINVAL;
> > - }
> > -
> > dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> > GFP_KERNEL);
> > if (!dp) {
> > @@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> > dp->dev =&pdev->dev;
> >
> > + if (pdev->dev.of_node) {
> > + pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
> > + if (IS_ERR(pdata))
> > + return PTR_ERR(pdata);
> > +
> > + exynos_dp_dt_parse_phydata(dp);
> > + } else {
> > + pdata = pdev->dev.platform_data;
> > + }
> > +
> > + if (!pdata) {
> > + dev_err(&pdev->dev, "no platform data\n");
> > + return -EINVAL;
> > + }
> > +
>
> This check is only reqired for non-dt case, might be worth to move
> it under else statement above.
OK, right.
>
> > dp->clock = devm_clk_get(&pdev->dev, "dp");
> > if (IS_ERR(dp->clock)) {
> > dev_err(&pdev->dev, "failed to get clock\n");
> > @@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> > }
> >
> > dp->video_info = pdata->video_info;
> > - if (pdata->phy_init)
> > - pdata->phy_init();
> > +
> > + if (pdev->dev.of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_init(dp);
> > + } else {
> > + if (pdata->phy_init)
> > + pdata->phy_init();
> > + }
> >
> > exynos_dp_init_dp(dp);
> >
> > @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
> This doesn't belong to your patch, but the above 2 lines could be written as:
>
> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>
> Same in functions exynos_dp_suspend() and exynos_dp_resume().
No, above 2 lines cannot be reduced to 1 line, as you mentioned.
This is because it makes build error.
>
> >
> > - if (pdata&& pdata->phy_exit)
> > - pdata->phy_exit();
> > + if (pdev->dev.of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_exit(dp);
> > + } else {
> > + if (pdata&& pdata->phy_exit)
> > + pdata->phy_exit();
> > + }
>
> It is not possible to have valid dp->dp_phy_addr pointer without
> valid pdev->dev.of_node, is it ?
Right, however, I prefer coding style as bellow:
if (pdev->dev.of_node)
DT
else
non-DT
>
> Can't this (and all similar occurrences) be simplified to:
>
> if (dp->dp_phy_addr)
> exynos_dp_phy_exit(dp);
> else if (pdata->phy_exit)
> pdata->phy_exit();
> ?
>
> It just requires dp->dp_phy_addr being NULL for non-dt case.
> pdata is never NULL, otherwise probe() would have already failed.
OK, right.
pdata can be removed.
>
> > clk_disable_unprepare(dp->clock);
> >
> > @@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > - if (pdata&& pdata->phy_exit)
> > - pdata->phy_exit();
> > + if (dev->of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_exit(dp);
> > + } else {
> > + if (pdata&& pdata->phy_exit)
> > + pdata->phy_exit();
> > + }
> >
> > clk_disable_unprepare(dp->clock);
> >
> > @@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
> > struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> > struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > - if (pdata&& pdata->phy_init)
> > - pdata->phy_init();
> > + if (dev->of_node) {
> > + if (dp->dp_phy_addr)
> > + exynos_dp_phy_init(dp);
> > + } else {
> > + if (pdata&& pdata->phy_init)
> > + pdata->phy_init();
> > + }
> >
> > clk_prepare_enable(dp->clock);
> >
> > @@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
> > SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
> > };
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id exynos_dp_match[] = {
> > + { .compatible = "samsung,exynos5-dp" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos_dp_match);
> > +#endif
> > +
>
> How about just dropping this #ifdef, since you're using of_match_ptr() ?
OK, right.
>
> > static struct platform_driver exynos_dp_driver = {
> > .probe = exynos_dp_probe,
> > .remove = __devexit_p(exynos_dp_remove),
> > @@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
> > .name = "exynos-dp",
> > .owner = THIS_MODULE,
> > .pm =&exynos_dp_pm_ops,
> > + .of_match_table = of_match_ptr(exynos_dp_match),
> > },
> > };
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> > index 57b8a65..569858b 100644
> > --- a/drivers/video/exynos/exynos_dp_core.h
> > +++ b/drivers/video/exynos/exynos_dp_core.h
> > @@ -29,6 +29,8 @@ struct exynos_dp_device {
> > struct clk *clock;
> > unsigned int irq;
> > void __iomem *reg_base;
> > + void __iomem *dp_phy_addr;
>
> Nit: "dp" is already within the structure name. How about just
> naming it phy_addr ? dp->phy_addr might look better than
> dp->dp_phy_addr ;)
OK, right.
>
> > + unsigned int enable_mask;
> >
> > struct video_info *video_info;
> > struct link_train link_train;
Ajay,
When CONFIG_OF is not enabled, it makes build errors.
In addition, exynos_dp_dt_parse_phydata needs return value,
instead of returning void.
Best regards,
Jingoo Han
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-15 8:14 ` Jingoo Han
@ 2012-10-15 21:14 ` Sylwester Nawrocki
-1 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-15 21:14 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Ajay Kumar', linux-samsung-soc, linux-fbdev,
devicetree-discuss, FlorianSchandinat, tomasz.figa, thomas.ab
On 10/15/2012 10:14 AM, Jingoo Han wrote:
> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>
>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>> This patch enables device tree based discovery support for DP driver.
>>> The driver is modified to handle platform data in both the cases:
>>> with DT and non-DT.
>>>
>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>> ---
>>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
>>> drivers/video/exynos/exynos_dp_core.h | 2 +
>>> 2 files changed, 149 insertions(+), 14 deletions(-)
...
>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>
>> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>
>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>
> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> This is because it makes build error.
Sorry, my bad. It looks fine in case of exynos_dp_remove().
But at exynos_dp_suspend/resume() there is something like:
static int exynos_dp_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
You need only pdata and dp there. I think that simpler form would
do as well:
struct exynos_dp_device *dp = dev_get_drvdata(dev);
struct exynos_dp_platdata *pdata = dev->platform_data;
Sorry, this is just a nitpicking.
BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-15 21:14 ` Sylwester Nawrocki
0 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-15 21:14 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Ajay Kumar', linux-samsung-soc, linux-fbdev,
devicetree-discuss, FlorianSchandinat, tomasz.figa, thomas.ab
On 10/15/2012 10:14 AM, Jingoo Han wrote:
> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>
>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>> This patch enables device tree based discovery support for DP driver.
>>> The driver is modified to handle platform data in both the cases:
>>> with DT and non-DT.
>>>
>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>> ---
>>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
>>> drivers/video/exynos/exynos_dp_core.h | 2 +
>>> 2 files changed, 149 insertions(+), 14 deletions(-)
...
>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>
>> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>
>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>
> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> This is because it makes build error.
Sorry, my bad. It looks fine in case of exynos_dp_remove().
But at exynos_dp_suspend/resume() there is something like:
static int exynos_dp_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
You need only pdata and dp there. I think that simpler form would
do as well:
struct exynos_dp_device *dp = dev_get_drvdata(dev);
struct exynos_dp_platdata *pdata = dev->platform_data;
Sorry, this is just a nitpicking.
BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-15 21:14 ` Sylwester Nawrocki
@ 2012-10-16 2:02 ` Jingoo Han
-1 siblings, 0 replies; 22+ messages in thread
From: Jingoo Han @ 2012-10-16 2:02 UTC (permalink / raw)
To: 'Sylwester Nawrocki'
Cc: 'Florian Tobias Schandinat', 'Ajay Kumar',
linux-samsung-soc, linux-fbdev, devicetree-discuss,
'Tomasz Figa', 'Thomas Abraham',
'Jingoo Han'
On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
>
> On 10/15/2012 10:14 AM, Jingoo Han wrote:
> > On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
> >>
> >> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> >>> This patch enables device tree based discovery support for DP driver.
> >>> The driver is modified to handle platform data in both the cases:
> >>> with DT and non-DT.
> >>>
> >>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> >>> Acked-by: Jingoo Han<jg1.han@samsung.com>
> >>> ---
> >>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
> >>> drivers/video/exynos/exynos_dp_core.h | 2 +
> >>> 2 files changed, 149 insertions(+), 14 deletions(-)
> ...
> >>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >>
> >> This doesn't belong to your patch, but the above 2 lines could be written as:
> >>
> >> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
> >>
> >> Same in functions exynos_dp_suspend() and exynos_dp_resume().
> >
> > No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> > This is because it makes build error.
>
> Sorry, my bad. It looks fine in case of exynos_dp_remove().
>
> But at exynos_dp_suspend/resume() there is something like:
>
>
> static int exynos_dp_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
>
> You need only pdata and dp there. I think that simpler form would
> do as well:
>
> struct exynos_dp_device *dp = dev_get_drvdata(dev);
> struct exynos_dp_platdata *pdata = dev->platform_data;
OK, I see.
I will accept your suggestion, because it is helpful to reduce
lines. Then, I will send v8 patch, soon.
>
>
> Sorry, this is just a nitpicking.
>
> BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?
>
>
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-16 2:02 ` Jingoo Han
0 siblings, 0 replies; 22+ messages in thread
From: Jingoo Han @ 2012-10-16 2:02 UTC (permalink / raw)
To: 'Sylwester Nawrocki'
Cc: 'Florian Tobias Schandinat', 'Ajay Kumar',
linux-samsung-soc, linux-fbdev, devicetree-discuss,
'Tomasz Figa', 'Thomas Abraham',
'Jingoo Han'
On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
>
> On 10/15/2012 10:14 AM, Jingoo Han wrote:
> > On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
> >>
> >> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> >>> This patch enables device tree based discovery support for DP driver.
> >>> The driver is modified to handle platform data in both the cases:
> >>> with DT and non-DT.
> >>>
> >>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> >>> Acked-by: Jingoo Han<jg1.han@samsung.com>
> >>> ---
> >>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
> >>> drivers/video/exynos/exynos_dp_core.h | 2 +
> >>> 2 files changed, 149 insertions(+), 14 deletions(-)
> ...
> >>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >>
> >> This doesn't belong to your patch, but the above 2 lines could be written as:
> >>
> >> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
> >>
> >> Same in functions exynos_dp_suspend() and exynos_dp_resume().
> >
> > No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> > This is because it makes build error.
>
> Sorry, my bad. It looks fine in case of exynos_dp_remove().
>
> But at exynos_dp_suspend/resume() there is something like:
>
>
> static int exynos_dp_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
>
> You need only pdata and dp there. I think that simpler form would
> do as well:
>
> struct exynos_dp_device *dp = dev_get_drvdata(dev);
> struct exynos_dp_platdata *pdata = dev->platform_data;
OK, I see.
I will accept your suggestion, because it is helpful to reduce
lines. Then, I will send v8 patch, soon.
>
>
> Sorry, this is just a nitpicking.
>
> BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?
>
>
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
2012-10-16 2:02 ` Jingoo Han
@ 2012-10-16 11:07 ` Sylwester Nawrocki
-1 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-16 11:07 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Sylwester Nawrocki', 'Florian Tobias Schandinat',
'Ajay Kumar', linux-samsung-soc, linux-fbdev,
devicetree-discuss, 'Tomasz Figa',
'Thomas Abraham'
On 10/16/2012 04:02 AM, Jingoo Han wrote:
> On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
>> On 10/15/2012 10:14 AM, Jingoo Han wrote:
>>> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>>>
>>>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>>>> This patch enables device tree based discovery support for DP driver.
>>>>> The driver is modified to handle platform data in both the cases:
>>>>> with DT and non-DT.
>>>>>
>>>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>>>> ---
>>>>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
>>>>> drivers/video/exynos/exynos_dp_core.h | 2 +
>>>>> 2 files changed, 149 insertions(+), 14 deletions(-)
>> ...
>>>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>>>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>>>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>>>
>>>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>>>
>>>> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>>>
>>>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>>>
>>> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
>>> This is because it makes build error.
>>
>> Sorry, my bad. It looks fine in case of exynos_dp_remove().
>>
>> But at exynos_dp_suspend/resume() there is something like:
>>
>>
>> static int exynos_dp_suspend(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>>
>> You need only pdata and dp there. I think that simpler form would
>> do as well:
>>
>> struct exynos_dp_device *dp = dev_get_drvdata(dev);
>> struct exynos_dp_platdata *pdata = dev->platform_data;
>
> OK, I see.
> I will accept your suggestion, because it is helpful to reduce
> lines. Then, I will send v8 patch, soon.
OK. I thought about it more as a candidate for a separate. But you seem
to have already sent new version, it's probably fine this way too.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver
@ 2012-10-16 11:07 ` Sylwester Nawrocki
0 siblings, 0 replies; 22+ messages in thread
From: Sylwester Nawrocki @ 2012-10-16 11:07 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Sylwester Nawrocki', 'Florian Tobias Schandinat',
'Ajay Kumar', linux-samsung-soc, linux-fbdev,
devicetree-discuss, 'Tomasz Figa',
'Thomas Abraham'
On 10/16/2012 04:02 AM, Jingoo Han wrote:
> On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
>> On 10/15/2012 10:14 AM, Jingoo Han wrote:
>>> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>>>
>>>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>>>> This patch enables device tree based discovery support for DP driver.
>>>>> The driver is modified to handle platform data in both the cases:
>>>>> with DT and non-DT.
>>>>>
>>>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>>>> ---
>>>>> drivers/video/exynos/exynos_dp_core.c | 161 ++++++++++++++++++++++++++++++---
>>>>> drivers/video/exynos/exynos_dp_core.h | 2 +
>>>>> 2 files changed, 149 insertions(+), 14 deletions(-)
>> ...
>>>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>>>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>>>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>>>
>>>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>>>
>>>> struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>>>
>>>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>>>
>>> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
>>> This is because it makes build error.
>>
>> Sorry, my bad. It looks fine in case of exynos_dp_remove().
>>
>> But at exynos_dp_suspend/resume() there is something like:
>>
>>
>> static int exynos_dp_suspend(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>>
>> You need only pdata and dp there. I think that simpler form would
>> do as well:
>>
>> struct exynos_dp_device *dp = dev_get_drvdata(dev);
>> struct exynos_dp_platdata *pdata = dev->platform_data;
>
> OK, I see.
> I will accept your suggestion, because it is helpful to reduce
> lines. Then, I will send v8 patch, soon.
OK. I thought about it more as a candidate for a separate. But you seem
to have already sent new version, it's probably fine this way too.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 22+ messages in thread