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
