Hello,
On Fri, Aug 17, 2012 at 6:18 AM, Joonyoung Shim <dofmind@xxxxxxxxx> wrote:
>
> Hi Leela.
>
> 2012/8/16 Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>:
> > The name of the exynos drm fimd device is renamed to exynos-drm-fimd
> > and two device ids are created for exynos4-fb and exynos5-drm-fimd.
> > Also, added driver data for exynos5 to pick the fimd version at runtime
> > and
> > to choose the VIDTCON register offsets accordingly.
> >
> > Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 56
> > +++++++++++++++++++++++++++---
> > 1 files changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > index 24c0bd4..8379c59 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -57,6 +57,18 @@
> >
> > #define get_fimd_context(dev)
> > platform_get_drvdata(to_platform_device(dev))
> >
> > +enum fimd_version_type {
> > + VERSION_8, /* FIMD_VERSION8 */
> > +};
> > +
> > +struct drm_fimd_driver_data {
>
> Don't use drm_ prefix for code consistency.
>
Ok, I'll remove the drm_prefix
> > + enum fimd_version_type fimd_ver;
>
> I don't prefer to check using version, it needs many if statement.
> Refer s3c-fb driver and how about use changed base address?
>
> struct fimd_driverdata {
> unsigned int timing_base;
> };
>
> static struct fimd_driverdata exynos5_fimd_data = {
> .timing_base = 0x20000,
> };
>
Ok, If I use the above structure as driver data for exynos5, then the
register write calls look this way
writel(val, ctx->regs + driver_data->timing_base + VIDTCON1);
[driver_data->timing_base = 0x20000 in case of exynos5 and 0 in case
of exynos4]
and only one statement is enough for both exynos4 and exynos5.
I had put some effort for the patch set
http://lists.freedesktop.org/archives/dri-devel/2012-August/026076.html
that contains macro definitions for FIMD_V8_VIDCONX and is already
merged to kgene's for-next branch.
If I go with the approach that you suggested then these macros will not be used.
If other reviewers also accepted to use timing_base as driver data,
then will go for it other wise will keep the code same :) .
> > +};
> > +
> > +struct drm_fimd_driver_data exynos5_drm_fimd_driver_data = {
> > + .fimd_ver = VERSION_8,
> > +};
> > +
> > struct fimd_win_data {
> > unsigned int offset_x;
> > unsigned int offset_y;
> > @@ -91,6 +103,13 @@ struct fimd_context {
> > struct exynos_drm_panel_info *panel;
> > };
> >
> > +static inline struct drm_fimd_driver_data *drm_fimd_get_driver_data(
> > + struct platform_device *pdev)
> > +{
> > + return (struct drm_fimd_driver_data *)
> > + platform_get_device_id(pdev)->driver_data;
> > +}
> > +
> > static bool fimd_display_is_connected(struct device *dev)
> > {
> > DRM_DEBUG_KMS("%s\n", __FILE__);
> > @@ -194,32 +213,47 @@ static void fimd_commit(struct device *dev)
> > struct fimd_context *ctx = get_fimd_context(dev);
> > struct exynos_drm_panel_info *panel = ctx->panel;
> > struct fb_videomode *timing = &panel->timing;
> > + struct drm_fimd_driver_data *driver_data;
> > + struct platform_device *pdev = to_platform_device(dev);
> > u32 val;
> >
> > + driver_data = drm_fimd_get_driver_data(pdev);
> > if (ctx->suspended)
> > return;
> >
> > DRM_DEBUG_KMS("%s\n", __FILE__);
> >
> > /* setup polarity values from machine code. */
> > - writel(ctx->vidcon1, ctx->regs + VIDCON1);
> > + if (driver_data->fimd_ver == VERSION_8)
> > + writel(ctx->vidcon1, ctx->regs + FIMD_V8_VIDCON1);
> > + else
> > + writel(ctx->vidcon1, ctx->regs + VIDCON1);
> >
> > /* setup vertical timing values. */
> > val = VIDTCON0_VBPD(timing->upper_margin - 1) |
> > VIDTCON0_VFPD(timing->lower_margin - 1) |
> > VIDTCON0_VSPW(timing->vsync_len - 1);
> > - writel(val, ctx->regs + VIDTCON0);
> > + if (driver_data->fimd_ver == VERSION_8)
> > + writel(val, ctx->regs + FIMD_V8_VIDTCON0);
> > + else
> > + writel(val, ctx->regs + VIDTCON0);
> >
> > /* setup horizontal timing values. */
> > val = VIDTCON1_HBPD(timing->left_margin - 1) |
> > VIDTCON1_HFPD(timing->right_margin - 1) |
> > VIDTCON1_HSPW(timing->hsync_len - 1);
> > - writel(val, ctx->regs + VIDTCON1);
> > + if (driver_data->fimd_ver == VERSION_8)
> > + writel(val, ctx->regs + FIMD_V8_VIDTCON1);
> > + else
> > + writel(val, ctx->regs + VIDTCON1);
> >
> > /* setup horizontal and vertical display size. */
> > val = VIDTCON2_LINEVAL(timing->yres - 1) |
> > VIDTCON2_HOZVAL(timing->xres - 1);
> > - writel(val, ctx->regs + VIDTCON2);
> > + if (driver_data->fimd_ver == VERSION_8)
> > + writel(val, ctx->regs + FIMD_V8_VIDTCON2);
> > + else
> > + writel(val, ctx->regs + VIDTCON2);
> >
> > /* setup clock source, clock divider, enable dma. */
> > val = ctx->vidcon0;
> > @@ -982,6 +1016,17 @@ static int fimd_runtime_resume(struct device *dev)
> > }
> > #endif
> >
> > +static struct platform_device_id exynos_drm_fimd_driver_ids[] = {
>
> Just use "fimd_driver_ids".
>
Ok, will take care of it.
> > + {
> > + .name = "exynos4-fb",
> > + }, {
> > + .name = "exynos5-drm-fimd",
>
> I think this name should be "exynos5-fb" because exynos5 fb driver and
> clock-exynos5 also use "exynos5-fb".
>
Ok, will change it.
Thanks for reviewing the patches.
> > + .driver_data = (unsigned
> > long)&exynos5_drm_fimd_driver_data,
> > + },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(platform, exynos_drm_fimd_driver_ids);
> > +
> > static const struct dev_pm_ops fimd_pm_ops = {
> > SET_SYSTEM_SLEEP_PM_OPS(fimd_suspend, fimd_resume)
> > SET_RUNTIME_PM_OPS(fimd_runtime_suspend, fimd_runtime_resume,
> > NULL)
> > @@ -990,8 +1035,9 @@ static const struct dev_pm_ops fimd_pm_ops = {
> > struct platform_driver fimd_driver = {
> > .probe = fimd_probe,
> > .remove = __devexit_p(fimd_remove),
> > + .id_table = exynos_drm_fimd_driver_ids,
> > .driver = {
> > - .name = "exynos4-fb",
> > + .name = "exynos-drm-fimd",
> > .owner = THIS_MODULE,
> > .pm = &fimd_pm_ops,
> > },
> > --
> > 1.7.0.4
> >
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss@xxxxxxxxxxxxxxxx
> > https://lists.ozlabs.org/listinfo/devicetree-discuss
>
> Thanks.
>
> --
> - Joonyoung Shim
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@xxxxxxxxxxxxxxxx
> https://lists.ozlabs.org/listinfo/devicetree-discuss
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
