Re: [PATCH] [RFC] video: exynos dp: Making Exynos DP Compliant with CDF

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Sean,
Thanks for reviewing the patch.

On Sat, Jan 12, 2013 at 1:30 AM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> On Fri, Jan 11, 2013 at 5:35 AM, Leela Krishna Amudala
> <l.krishna@xxxxxxxxxxx> wrote:
>> The Exynos DP transmitter is treated as an end entity in the display pipeline
>> and made this RFC patch compliant with CDF.
>>
>> Any suggestions are welcome.
>>
>
> A few comments below. It's hard to get too much of an appreciation for
> what you're trying to do since a bunch of the interesting parts are
> stubbed out.
>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>
>> ---
>>  drivers/video/display/display-core.c  |  2 +-
>>  drivers/video/exynos/exynos_dp_core.c | 88 +++++++++++++++++++++++++++++++++++
>>  drivers/video/exynos/exynos_dp_core.h |  6 +++
>>  3 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/display/display-core.c b/drivers/video/display/display-core.c
>> index 5f8be30..dbad7e9 100644
>> --- a/drivers/video/display/display-core.c
>> +++ b/drivers/video/display/display-core.c
>> @@ -15,7 +15,7 @@
>>  #include <linux/list.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> -#include <linux/videomode.h>
>> +#include <video/videomode.h>
>>
>>  #include <video/display.h>
>>
>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>> index 4ef18e2..0f8de27b 100644
>> --- a/drivers/video/exynos/exynos_dp_core.c
>> +++ b/drivers/video/exynos/exynos_dp_core.c
>> @@ -23,6 +23,9 @@
>>  #include <video/exynos_dp.h>
>>
>>  #include "exynos_dp_core.h"
>> +#include <video/videomode.h>
>> +#include <video/display.h>
>> +#define to_panel(p) container_of(p, struct exynos_dp_device, entity)
>>
>>  static int exynos_dp_init_dp(struct exynos_dp_device *dp)
>>  {
>> @@ -1033,6 +1036,81 @@ static void exynos_dp_phy_exit(struct exynos_dp_device *dp)
>>  }
>>  #endif /* CONFIG_OF */
>>
>> +static int exynos_dp_power_on(struct exynos_dp_device *dp)
>> +{
>> +       struct platform_device *pdev = to_platform_device(dp->dev);
>> +       struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>> +
>> +       if (dp->dev->of_node) {
>> +               if (dp->phy_addr)
>> +                       exynos_dp_phy_init(dp);
>> +       } else {
>> +               if (pdata->phy_init)
>> +                       pdata->phy_init();
>> +       }
>> +
>> +       clk_prepare_enable(dp->clock);
>> +       exynos_dp_init_dp(dp);
>> +       enable_irq(dp->irq);
>> +
>> +       return 0;
>> +}
>> +
>> +static int dp_set_state(struct display_entity *entity,
>> +                       enum display_entity_state state)
>> +{
>> +       struct exynos_dp_device *dp = to_panel(entity);
>> +       struct platform_device *pdev = to_platform_device(dp->dev);
>> +       int ret = 0;
>> +
>> +       switch (state) {
>> +       case DISPLAY_ENTITY_STATE_OFF:
>> +       case DISPLAY_ENTITY_STATE_STANDBY:
>> +               ret = exynos_dp_remove(pdev);
>
> This is incorrect, that is the module remove function. It seems like
> it works right now since there's nothing permanent happening (like
> platform data being freed), but there's no guarantee that this will
> remain like that in the future.
>
> IMO, you should factor out the common bits from remove and suspend
> into a new function which is called from all three.
>

Yes, I used the module remove function because it works fine with its
current state.
I'll factor out the common things and will create a common function.

>> +               break;
>> +       case DISPLAY_ENTITY_STATE_ON:
>> +               ret = exynos_dp_power_on(dp);
>> +               break;
>> +       }
>> +       return ret;
>> +}
>> +
>> +static int dp_get_modes(struct display_entity *entity,
>> +                       const struct videomode **modes)
>> +{
>> +       /* Rework has to be done here*/
>> +       return 1;
>
> Returning 1 here is pretty risky since you didn't actually allocate or
> populate a mode. I'm surprised this isn't causing some weird
> side-effects for you.
>

This current code is a dummy function, That is the reason to mention
"Rework has to be done here"
The current DP driver is not receiving any video mode properties. So I
have to think out how to get the same and will do the implementation.

>> +}
>> +
>> +static int dp_get_size(struct display_entity *entity,
>> +                       unsigned int *width, unsigned int *height)
>> +{
>> +       struct exynos_dp_device *dp = to_panel(entity);
>> +       struct platform_device *pdev = to_platform_device(dp->dev);
>> +       /*getting pdata in older way, rework has to be done  here to
>> +         parse it from dt node */
>> +       struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>> +
>> +       /*Rework has to be done here */
>> +       *width = 1280;
>> +       *height = 800;
>> +       return 0;
>> +}
>> +
>> +static int dp_update(struct display_entity *entity,
>> +               void (*callback)(int, void *), void *data)
>> +{
>> +       /*Rework has to be done here*/
>> +       return 0;
>> +}
>> +
>> +static const struct display_entity_control_ops dp_control_ops = {
>> +       .set_state = dp_set_state,
>> +       .get_modes = dp_get_modes,
>> +       .get_size = dp_get_size,
>> +       .update = dp_update,
>> +};
>> +
>>  static int exynos_dp_probe(struct platform_device *pdev)
>>  {
>>         struct resource *res;
>> @@ -1111,6 +1189,16 @@ static int exynos_dp_probe(struct platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, dp);
>>
>> +       /* setup panel entity */
>> +       dp->entity.dev = &pdev->dev;
>> +       dp->entity.release = exynos_dp_remove;
>> +       dp->entity.ops = &dp_control_ops;
>> +
>> +       ret = display_entity_register(&dp->entity);
>> +       if (ret < 0) {
>> +               pr_err("failed to register display entity\n");
>> +               return ret;
>> +       }
>>         return 0;
>>  }
>>
>> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
>> index 6c567bb..eb18c10 100644
>> --- a/drivers/video/exynos/exynos_dp_core.h
>> +++ b/drivers/video/exynos/exynos_dp_core.h
>> @@ -13,6 +13,8 @@
>>  #ifndef _EXYNOS_DP_CORE_H
>>  #define _EXYNOS_DP_CORE_H
>>
>> +#include <video/display.h>
>> +
>>  enum dp_irq_type {
>>         DP_IRQ_TYPE_HP_CABLE_IN,
>>         DP_IRQ_TYPE_HP_CABLE_OUT,
>> @@ -42,6 +44,7 @@ struct exynos_dp_device {
>>         struct video_info       *video_info;
>>         struct link_train       link_train;
>>         struct work_struct      hotplug_work;
>> +       struct display_entity   entity;
>>  };
>>
>>  /* exynos_dp_reg.c */
>> @@ -133,6 +136,9 @@ void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp);
>>  void exynos_dp_enable_scrambling(struct exynos_dp_device *dp);
>>  void exynos_dp_disable_scrambling(struct exynos_dp_device *dp);
>>
>> +static int exynos_dp_power_on(struct exynos_dp_device *dp);
>> +static int exynos_dp_remove(struct platform_device *pdev);
>> +
>
> You can just move the functions around the file as needed and avoid
> the forward declaration.
>

Yes accepted, will do this.

Best Wishes,
Leela Krishna Amudala.

>>  /* I2C EDID Chip ID, Slave Address */
>>  #define I2C_EDID_DEVICE_ADDR                   0x50
>>  #define I2C_E_EDID_DEVICE_ADDR                 0x30
>> --
>> 1.8.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux