[PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
Rahul Sharma
r.sh.open at gmail.com
Thu Mar 14 02:40:39 PDT 2013
On Tue, Mar 12, 2013 at 5:37 PM, Inki Dae <inki.dae at samsung.com> wrote:
> 2013/3/12 Rahul Sharma <r.sh.open at gmail.com>:
>> On Mon, Mar 4, 2013 at 7:35 PM, Rahul Sharma <r.sh.open at gmail.com> wrote:
>>> Thanks Sean,
>>>
>>> On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul <seanpaul at google.com> wrote:
>>>> On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma <rahul.sharma at samsung.com> wrote:
>>>>> Right now hdmiphy operations and configs are kept inside hdmi driver. hdmiphy related
>>>>> code is tightly coupled with hdmi ip driver. Physicaly they are different devices and
>>>>
>>>> s/Physicaly/Physically/
>>>>
>>>>> should be instantiated independently.
>>>>>
>>>>> In terms of versions/mapping/configurations Hdmi and hdmiphy are independent of each
>>>>> other. It is preferred to isolate them and maintained independently.
>>>>>
>>>>> This implementations is tested for:
>>>>> 1) Resolutions supported by exynos4 and 5 hdmi.
>>>>> 2) Runtime PM and S2R scenarions for exynos5.
>>>>>
>>>>
>>>> I don't like the idea of spawning off yet another driver in here. It
>>>> adds more globals, more suspend/resume ordering issues, and more
>>>> implicit dependencies. I understand, however, that this is the Chosen
>>>> Way for the exynos driver, so I will save my rant.
>>>>
>>>
>>> I agree to it. splitting phy to a new driver will complicate the power related
>>> scenarios. But in upcoming SoC,s, Phy is changing considerably in terms of
>>> config values, mapping (i2c/platform bus) etc. Handling this diversity
>>> inside hdmi driver is complicating it with unrelated changes.
>>>
>>> I have tested this RFC for Runtime PM / S2R. But if we see any major roadblock
>>> we should re-factor this by explicitly calling power related callbacks
>>> of mixer, phy,
>>> hdmi drivers in a required order. We can call them from exynos-drm-hdmi plf
>>> device. AFAIR something like this is already in place in chrome-kernel.
>>>
>>>> I've made some comments below.
>>>>
>>>>> This patch is dependent on
>>>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34733.html
>>>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34861.html
>>>>> http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34862.html
>>>>>
>>>>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>>>>> ---
>>>>> It is based on exynos-drm-next-todo branch at
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>
>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 +
>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 +
>>>>> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 58 ++-
>>>>> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 +
>>>>> drivers/gpu/drm/exynos/exynos_hdmi.c | 375 ++------------------
>>>>> drivers/gpu/drm/exynos/exynos_hdmi.h | 1 -
>>>>> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 586 ++++++++++++++++++++++++++++++-
>>>>> drivers/gpu/drm/exynos/regs-hdmiphy.h | 61 ++++
>>>>> 8 files changed, 738 insertions(+), 368 deletions(-)
>>>>> create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>>> index 3da5c2d..03acb6b 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>>>> @@ -334,6 +334,11 @@ static int __init exynos_drm_init(void)
>>>>> ret = platform_driver_register(&hdmi_driver);
>>>>> if (ret < 0)
>>>>> goto out_hdmi;
>>>>> +
>>>>> + ret = exynos_hdmiphy_driver_register();
>>>>
>>>> Not sure why you need to obfuscate the type of driver here. All the
>>>> other ones are directly registered as platform drivers, I don't see
>>>> the harm in just doing the i2c_add_driver directly here.
>>>>
>>>
>>> AIMB, Phy is likely to get mapped as a platform device in later soc's.
>>> I want to make driver registration (inside exynos_drm_drv.c) independent
>>> of this changes. We can handle this inside exynos_hdmiphy_driver_register().
>>>
>>>>> + if (ret < 0)
>>>>> + goto out_hdmiphy;
>>>>> +
>>>>> ret = platform_driver_register(&mixer_driver);
>>>>> if (ret < 0)
>>>>> goto out_mixer;
>>>>
>>>>
>>>> I think this ordering is how you plan on enforcing the suspend/resume
>>>> order for hdmi/mixer/hdmiphy. In that case, however, I don't think it
>>>> makes sense to suspend/resume hdmiphy in between mixer and hdmi.
>>>> Ideally, hdmiphy should go down first and come up last.
>>>>
>>>
>>> I just wanted to keep, hdmi and hdmiphy things together. But what you said
>>> above makes sense. I will do that change.
>>>
>>>>
>>>>> @@ -436,6 +441,8 @@ out_common_hdmi_dev:
>>>>> out_common_hdmi:
>>>>> platform_driver_unregister(&mixer_driver);
>>>>> out_mixer:
>>>>> + exynos_hdmiphy_driver_unregister();
>>>>> +out_hdmiphy:
>>>>> platform_driver_unregister(&hdmi_driver);
>>>>> out_hdmi:
>>>>> #endif
>>>>> @@ -479,6 +486,7 @@ static void __exit exynos_drm_exit(void)
>>>>> exynos_platform_device_hdmi_unregister();
>>>>> platform_driver_unregister(&exynos_drm_common_hdmi_driver);
>>>>> platform_driver_unregister(&mixer_driver);
>>>>> + exynos_hdmiphy_driver_unregister();
>>>>> platform_driver_unregister(&hdmi_driver);
>>>>> #endif
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>>>> index 4606fac..17c53e3 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>>>> @@ -325,6 +325,12 @@ void exynos_drm_subdrv_close(struct drm_device *dev, struct drm_file *file);
>>>>> extern int exynos_platform_device_hdmi_register(void);
>>>>>
>>>>> /*
>>>>> + * these functions registers/unregisters exynos drm hdmiphy driver.
>>>>> + */
>>>>> +extern int exynos_hdmiphy_driver_register(void);
>>>>> +extern void exynos_hdmiphy_driver_unregister(void);
>>>>> +
>>>>> +/*
>>>>> * this function unregisters exynos drm hdmi platform device if it exists.
>>>>> */
>>>>> void exynos_platform_device_hdmi_unregister(void);
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>>>>> index 5dc956b..45ef331 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>>>>> @@ -32,19 +32,22 @@
>>>>> /* platform device pointer for common drm hdmi device. */
>>>>> static struct platform_device *exynos_drm_hdmi_pdev;
>>>>>
>>>>> -/* Common hdmi subdrv needs to access the hdmi and mixer though context.
>>>>> -* These should be initialied by the repective drivers */
>>>>> +/* Common hdmi subdrv needs to access the hdmi, hdmiphy and mixer though
>>>>> +* context. These should be initialied by the repective drivers */
>>>>> static struct exynos_drm_hdmi_context *hdmi_ctx;
>>>>> +static struct exynos_drm_hdmi_context *hdmiphy_ctx;
>>>>> static struct exynos_drm_hdmi_context *mixer_ctx;
>>>>>
>>>>> /* these callback points shoud be set by specific drivers. */
>>>>> static struct exynos_hdmi_ops *hdmi_ops;
>>>>> +static struct exynos_hdmiphy_ops *hdmiphy_ops;
>>>>> static struct exynos_mixer_ops *mixer_ops;
>>>>>
>>>>
>>>> This stuff really sets my teeth on edge. I can't stand these global
>>>> variables and the set-here, set-there, set-here dance that is done on
>>>> probe.
>>>>
>>>> IMO, this and the suspend/resume ordering should be enough motivation
>>>> to avoid separating everything out into their own drivers.
>>>>
>>>> Instead of adding more kruft to this list, we should be working to remove it.
>>>>
>>>> /rant
>>>>
>>>
>>> Let me think over it. Please suggest me something if you have any idea to
>>> correct this.
>>>
>>
>> Hi all,
>>
>> I replaced these variables with a global LIST of subdrv's with associated
>> type (HDMI / PHY / MIXER) and ops struct.
>>
>> drm-common-hdmi callback will parse this list and based on the subdrv
>> type calls subdrv functions, but this adds overhead of parsing the list
>> multiple times.
>>
>> If I cache the subdrvs in drm-common-hdmi context and parsing of the list
>> can be avoided.
>>
>> What you think about above approach?
>>
>
> I think it's good to use existing exynos drm core framework. But I'm
> anxious about probing point and existing subdrv callbacks with this.
> Could you share that patch as RFC?
Mr. Dae,
Drm sub-drivers including drm-fimd, drm-common-hdmi will continue using existing
exynos drm framework. If I use same for HDMI, PHY and MIXER drivers, I will be
bypassing the followed hierarchy i.e.
DRM--> EXYNOS-DRM --> Drm-common-hdmi --> (hdmi, mixer, phy).
Secondly we need to invoke callbacks in sequence for above three. That
condition can
not be met if moved to exynos-drm.
I modified drm-common-hdmi to create provision for HDMI, PHY and MIXER
sub-driver registrations. Then we don't need to maintain so many
global variables.
If you feel this approach is reasonable I will post the RFC patch.
regards,
Rahul Sharma.
>
> Thanks,
> Inki Dae
>
>> regards,
>> Rahul Sharma.
>>
>>>>> struct drm_hdmi_context {
>>>>> struct exynos_drm_subdrv subdrv;
>>>>> struct exynos_drm_hdmi_context *hdmi_ctx;
>>>>> struct exynos_drm_hdmi_context *mixer_ctx;
>>>>> + struct exynos_drm_hdmi_context *hdmiphy_ctx;
>>>>>
>>>>> bool enabled[MIXER_WIN_NR];
>>>>> };
>>>>> @@ -74,6 +77,12 @@ void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx)
>>>>> hdmi_ctx = ctx;
>>>>> }
>>>>>
>>>>> +void exynos_hdmiphy_drv_attach(struct exynos_drm_hdmi_context *ctx)
>>>>> +{
>>>>> + if (ctx)
>>>>> + hdmiphy_ctx = ctx;
>>>>> +}
>>>>> +
>>>>> void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx)
>>>>> {
>>>>> if (ctx)
>>>>> @@ -88,6 +97,14 @@ void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops)
>>>>> hdmi_ops = ops;
>>>>> }
>>>>>
>>>>> +void exynos_hdmiphy_ops_register(struct exynos_hdmiphy_ops *ops)
>>>>> +{
>>>>> + DRM_DEBUG_KMS("%s\n", __FILE__);
>>>>
>>>> Listing __FILE__ in these DRM_DEBUG_KMS traces is pretty useless. I'm
>>>> sure we can determine the file from the function name. It would be
>>>> slightly more useful to list __LINE__, but even more useful if you
>>>> actually printed state relevant to the current function.
>>>>
>>>
>>> OK. I will correct this.
>>>
>>>>> +
>>>>> + if (ops)
>>>>> + hdmiphy_ops = ops;
>>>>> +}
>>>>> +
>>>>> void exynos_mixer_ops_register(struct exynos_mixer_ops *ops)
>>>>> {
>>>>> DRM_DEBUG_KMS("%s\n", __FILE__);
>>>>> @@ -121,7 +138,7 @@ struct edid *drm_hdmi_get_edid(struct device *dev,
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> -static int drm_hdmi_check_timing(struct device *dev, void *timing)
>>>>> +static int drm_hdmi_check_timing(struct device *dev, void *mode)
>>>>> {
>>>>> struct drm_hdmi_context *ctx = to_context(dev);
>>>>> int ret = 0;
>>>>> @@ -129,18 +146,24 @@ static int drm_hdmi_check_timing(struct device *dev, void *timing)
>>>>> DRM_DEBUG_KMS("%s\n", __FILE__);
>>>>>
>>>>> /*
>>>>> - * Both, mixer and hdmi should be able to handle the requested mode.
>>>>> - * If any of the two fails, return mode as BAD.
>>>>> + * Mixer, hdmi and hdmiphy should be able to handle the requested mode.
>>>>> + * If any of the them fails, return mode as BAD.
>>>>> */
>>>>>
>>>>> if (mixer_ops && mixer_ops->check_timing)
>>>>> - ret = mixer_ops->check_timing(ctx->mixer_ctx->ctx, timing);
>>>>> + ret = mixer_ops->check_timing(ctx->mixer_ctx->ctx, mode);
>>>>>
>>>>> if (ret)
>>>>> return ret;
>>>>>
>>>>> if (hdmi_ops && hdmi_ops->check_timing)
>>>>> - return hdmi_ops->check_timing(ctx->hdmi_ctx->ctx, timing);
>>>>> + ret = hdmi_ops->check_timing(ctx->hdmi_ctx->ctx, mode);
>>>>> +
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + if (hdmiphy_ops && hdmiphy_ops->check_timing)
>>>>> + return hdmiphy_ops->check_timing(ctx->hdmiphy_ctx->ctx, mode);
>>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -254,6 +277,9 @@ static void drm_hdmi_mode_set(struct device *subdrv_dev, void *mode)
>>>>>
>>>>> if (hdmi_ops && hdmi_ops->mode_set)
>>>>> hdmi_ops->mode_set(ctx->hdmi_ctx->ctx, mode);
>>>>> +
>>>>> + if (hdmiphy_ops && hdmiphy_ops->mode_set)
>>>>> + hdmiphy_ops->mode_set(ctx->hdmiphy_ctx->ctx, mode);
>>>>> }
>>>>>
>>>>> static void drm_hdmi_get_max_resol(struct device *subdrv_dev,
>>>>> @@ -273,6 +299,15 @@ static void drm_hdmi_commit(struct device *subdrv_dev)
>>>>>
>>>>> DRM_DEBUG_KMS("%s\n", __FILE__);
>>>>>
>>>>> + if (hdmiphy_ops && hdmiphy_ops->prepare)
>>>>> + hdmiphy_ops->prepare(ctx->hdmiphy_ctx->ctx);
>>>>> +
>>>>> + if (hdmi_ops && hdmi_ops->prepare)
>>>>> + hdmi_ops->prepare(ctx->hdmi_ctx->ctx);
>>>>> +
>>>>
>>>> Why are you calling these prepare functions in commit? Isn't that
>>>> precisely the reason drm has a prepare callback?
>>>
>>> True. Currently exynos_drm_encoder_prepare is blank. I will move this to
>>> prepare callback.
>>>
>>>>
>>>>> + if (hdmiphy_ops && hdmiphy_ops->config_apply)
>>>>> + hdmiphy_ops->config_apply(ctx->hdmiphy_ctx->ctx);
>>>>> +
>>>>> if (hdmi_ops && hdmi_ops->commit)
>>>>> hdmi_ops->commit(ctx->hdmi_ctx->ctx);
>>>>> }
>>>>> @@ -288,6 +323,9 @@ static void drm_hdmi_dpms(struct device *subdrv_dev, int mode)
>>>>>
>>>>> if (hdmi_ops && hdmi_ops->dpms)
>>>>> hdmi_ops->dpms(ctx->hdmi_ctx->ctx, mode);
>>>>> +
>>>>> + if (hdmiphy_ops && hdmiphy_ops->dpms)
>>>>> + hdmiphy_ops->dpms(ctx->hdmiphy_ctx->ctx, mode);
>>>>
>>>> I think you have to be more careful about the ordering of things here
>>>> since you'll want to power on in a different order than you power off.
>>>>
>>>>> }
>>>>>
>>>>> static void drm_hdmi_apply(struct device *subdrv_dev)
>>>>> @@ -393,6 +431,11 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
>>>>> return -EFAULT;
>>>>> }
>>>>>
>>>>> + if (!hdmiphy_ctx) {
>>>>> + DRM_ERROR("hdmiphy context not initialized.\n");
>>>>> + return -EFAULT;
>>>>> + }
>>>>> +
>>>>> if (!mixer_ctx) {
>>>>> DRM_ERROR("mixer context not initialized.\n");
>>>>> return -EFAULT;
>>>>> @@ -406,6 +449,7 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
>>>>> }
>>>>>
>>>>> ctx->hdmi_ctx = hdmi_ctx;
>>>>> + ctx->hdmiphy_ctx = hdmiphy_ctx;
>>>>> ctx->mixer_ctx = mixer_ctx;
>>>>>
>>>>> ctx->hdmi_ctx->drm_dev = drm_dev;
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
>>>>> index fd2ff9f..10db62e 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
>>>>> @@ -39,10 +39,19 @@ struct exynos_hdmi_ops {
>>>>> void (*mode_set)(void *ctx, struct drm_display_mode *mode);
>>>>> void (*get_max_resol)(void *ctx, unsigned int *width,
>>>>> unsigned int *height);
>>>>> + void (*prepare)(void *ctx);
>>>>> void (*commit)(void *ctx);
>>>>> void (*dpms)(void *ctx, int mode);
>>>>> };
>>>>>
>>>>> +struct exynos_hdmiphy_ops {
>>>>> + int (*check_timing)(void *ctx, struct drm_display_mode *mode);
>>>>> + void (*mode_set)(void *ctx, struct drm_display_mode *mode);
>>>>> + void (*prepare)(void *ctx);
>>>>> + int (*config_apply)(void *ctx);
>>>>> + void (*dpms)(void *ctx, int mode);
>>>>> +};
>>>>> +
>>>>> struct exynos_mixer_ops {
>>>>> /* manager */
>>>>> int (*iommu_on)(void *ctx, bool enable);
>>>>> @@ -61,7 +70,9 @@ struct exynos_mixer_ops {
>>>>> };
>>>>>
>>>>> void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx);
>>>>> +void exynos_hdmiphy_drv_attach(struct exynos_drm_hdmi_context *ctx);
>>>>> void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx);
>>>>> void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops);
>>>>> +void exynos_hdmiphy_ops_register(struct exynos_hdmiphy_ops *ops);
>>>>> void exynos_mixer_ops_register(struct exynos_mixer_ops *ops);
>>>>> #endif
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>>>> index a5ca2cc..7ee350d 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>>>> @@ -194,7 +194,6 @@ struct hdmi_context {
>>>>> int internal_irq;
>>>>>
>>>>> struct i2c_client *ddc_port;
>>>>> - struct i2c_client *hdmiphy_port;
>>>>>
>>>>> /* current hdmiphy conf regs */
>>>>> struct hdmi_conf_regs mode_conf;
>>>>> @@ -206,180 +205,6 @@ struct hdmi_context {
>>>>> enum hdmi_type type;
>>>>> };
>>>>>
>>>>> -struct hdmiphy_config {
>>>>> - int pixel_clock;
>>>>> - u8 conf[32];
>>>>> -};
>>>>> -
>>>>> -/* list of phy config settings */
>>>>> -static const struct hdmiphy_config hdmiphy_v13_configs[] = {
>>>>> - {
>>>>> - .pixel_clock = 27000000,
>>>>> - .conf = {
>>>>> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30, 0x40,
>>>>> - 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54, 0x87,
>>>>> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> - 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 27027000,
>>>>> - .conf = {
>>>>> - 0x01, 0x05, 0x00, 0xD4, 0x10, 0x9C, 0x09, 0x64,
>>>>> - 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54, 0x87,
>>>>> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> - 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 74176000,
>>>>> - .conf = {
>>>>> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xef, 0x5B,
>>>>> - 0x6D, 0x10, 0x01, 0x51, 0xef, 0xF3, 0x54, 0xb9,
>>>>> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> - 0x22, 0x40, 0xa5, 0x26, 0x01, 0x00, 0x00, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 74250000,
>>>>> - .conf = {
>>>>> - 0x01, 0x05, 0x00, 0xd8, 0x10, 0x9c, 0xf8, 0x40,
>>>>> - 0x6a, 0x10, 0x01, 0x51, 0xff, 0xf1, 0x54, 0xba,
>>>>> - 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10, 0xe0,
>>>>> - 0x22, 0x40, 0xa4, 0x26, 0x01, 0x00, 0x00, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 148500000,
>>>>> - .conf = {
>>>>> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xf8, 0x40,
>>>>> - 0x6A, 0x18, 0x00, 0x51, 0xff, 0xF1, 0x54, 0xba,
>>>>> - 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> - 0x22, 0x40, 0xa4, 0x26, 0x02, 0x00, 0x00, 0x00,
>>>>> - },
>>>>> - },
>>>>> -};
>>>>> -
>>>>> -static const struct hdmiphy_config hdmiphy_v14_configs[] = {
>>>>> - {
>>>>> - .pixel_clock = 25200000,
>>>>> - .conf = {
>>>>> - 0x01, 0x51, 0x2A, 0x75, 0x40, 0x01, 0x00, 0x08,
>>>>> - 0x82, 0x80, 0xfc, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xf4, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 27000000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x22, 0x51, 0x40, 0x08, 0xfc, 0x20,
>>>>> - 0x98, 0xa0, 0xcb, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x06, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xe4, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 27027000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x2d, 0x72, 0x40, 0x64, 0x12, 0x08,
>>>>> - 0x43, 0xa0, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xe3, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 36000000,
>>>>> - .conf = {
>>>>> - 0x01, 0x51, 0x2d, 0x55, 0x40, 0x01, 0x00, 0x08,
>>>>> - 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xab, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 40000000,
>>>>> - .conf = {
>>>>> - 0x01, 0x51, 0x32, 0x55, 0x40, 0x01, 0x00, 0x08,
>>>>> - 0x82, 0x80, 0x2c, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0x9a, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 65000000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x36, 0x34, 0x40, 0x1e, 0x0a, 0x08,
>>>>> - 0x82, 0xa0, 0x45, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xbd, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 74176000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x3e, 0x35, 0x40, 0x5b, 0xde, 0x08,
>>>>> - 0x82, 0xa0, 0x73, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x56, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xa6, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 74250000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x1f, 0x10, 0x40, 0x40, 0xf8, 0x08,
>>>>> - 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xa5, 0x24, 0x01, 0x00, 0x00, 0x01, 0x00,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 83500000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x23, 0x11, 0x40, 0x0c, 0xfb, 0x08,
>>>>> - 0x85, 0xa0, 0xd1, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0x93, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 106500000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x2c, 0x12, 0x40, 0x0c, 0x09, 0x08,
>>>>> - 0x84, 0xa0, 0x0a, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0x73, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 108000000,
>>>>> - .conf = {
>>>>> - 0x01, 0x51, 0x2d, 0x15, 0x40, 0x01, 0x00, 0x08,
>>>>> - 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0xc7, 0x25, 0x03, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 146250000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x3d, 0x15, 0x40, 0x18, 0xfd, 0x08,
>>>>> - 0x83, 0xa0, 0x6e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0x50, 0x25, 0x03, 0x00, 0x00, 0x01, 0x80,
>>>>> - },
>>>>> - },
>>>>> - {
>>>>> - .pixel_clock = 148500000,
>>>>> - .conf = {
>>>>> - 0x01, 0xd1, 0x1f, 0x00, 0x40, 0x40, 0xf8, 0x08,
>>>>> - 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> - 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> - 0x54, 0x4b, 0x25, 0x03, 0x00, 0x00, 0x01, 0x00,
>>>>> - },
>>>>> - },
>>>>> -};
>>>>> -
>>>>> struct hdmi_infoframe {
>>>>> enum HDMI_PACKET_TYPE type;
>>>>> u8 ver;
>>>>> @@ -774,46 +599,6 @@ static struct edid *hdmi_get_edid(void *ctx, struct drm_connector *connector)
>>>>> return raw_edid;
>>>>> }
>>>>>
>>>>> -static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
>>>>> -{
>>>>> - const struct hdmiphy_config *confs;
>>>>> - int count, i;
>>>>> -
>>>>> - DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> -
>>>>> - if (hdata->type == HDMI_TYPE13) {
>>>>> - confs = hdmiphy_v13_configs;
>>>>> - count = ARRAY_SIZE(hdmiphy_v13_configs);
>>>>> - } else if (hdata->type == HDMI_TYPE14) {
>>>>> - confs = hdmiphy_v14_configs;
>>>>> - count = ARRAY_SIZE(hdmiphy_v14_configs);
>>>>> - } else
>>>>> - return -EINVAL;
>>>>> -
>>>>> - for (i = 0; i < count; i++)
>>>>> - if (confs[i].pixel_clock == pixel_clock)
>>>>> - return i;
>>>>> -
>>>>> - DRM_DEBUG_KMS("Could not find phy config for %d\n", pixel_clock);
>>>>> - return -EINVAL;
>>>>> -}
>>>>> -
>>>>> -static int hdmi_check_timing(void *ctx, struct drm_display_mode *mode)
>>>>> -{
>>>>> - struct hdmi_context *hdata = ctx;
>>>>> - int ret;
>>>>> -
>>>>> - DRM_DEBUG_KMS("%s : xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n",
>>>>> - __func__, mode->hdisplay, mode->vdisplay,
>>>>> - mode->vrefresh, (mode->flags & DRM_MODE_FLAG_INTERLACE) ? true :
>>>>> - false, mode->clock * 1000);
>>>>> -
>>>>> - ret = hdmi_find_phy_conf(hdata, mode->clock * 1000);
>>>>> - if (ret < 0)
>>>>> - return ret;
>>>>> - return 0;
>>>>> -}
>>>>> -
>>>>> static void hdmi_set_acr(u32 freq, u8 *acr)
>>>>> {
>>>>> u32 n, cts;
>>>>> @@ -1309,20 +1094,12 @@ static void hdmi_timing_apply(struct hdmi_context *hdata)
>>>>>
>>>>> static void hdmiphy_conf_reset(struct hdmi_context *hdata)
>>>>> {
>>>>> - u8 buffer[2];
>>>>> u32 reg;
>>>>>
>>>>> clk_disable(hdata->res.sclk_hdmi);
>>>>> clk_set_parent(hdata->res.sclk_hdmi, hdata->res.sclk_pixel);
>>>>> clk_enable(hdata->res.sclk_hdmi);
>>>>>
>>>>> - /* operation mode */
>>>>> - buffer[0] = 0x1f;
>>>>> - buffer[1] = 0x00;
>>>>> -
>>>>> - if (hdata->hdmiphy_port)
>>>>> - i2c_master_send(hdata->hdmiphy_port, buffer, 2);
>>>>> -
>>>>> if (hdata->type == HDMI_TYPE13)
>>>>> reg = HDMI_V13_PHY_RSTOUT;
>>>>> else
>>>>> @@ -1335,102 +1112,6 @@ static void hdmiphy_conf_reset(struct hdmi_context *hdata)
>>>>> usleep_range(10000, 12000);
>>>>> }
>>>>>
>>>>> -static void hdmiphy_poweron(struct hdmi_context *hdata)
>>>>> -{
>>>>> - DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> -
>>>>> - if (hdata->type == HDMI_TYPE14)
>>>>> - hdmi_reg_writemask(hdata, HDMI_PHY_CON_0, 0,
>>>>> - HDMI_PHY_POWER_OFF_EN);
>>>>> -}
>>>>> -
>>>>> -static void hdmiphy_poweroff(struct hdmi_context *hdata)
>>>>> -{
>>>>> - DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> -
>>>>> - if (hdata->type == HDMI_TYPE14)
>>>>> - hdmi_reg_writemask(hdata, HDMI_PHY_CON_0, ~0,
>>>>> - HDMI_PHY_POWER_OFF_EN);
>>>>
>>>> I assume these register writes are replaced with i2c operations in the
>>>> new phy driver?
>>>
>>> It is done for exynos5 phy. I am not having the sequence for exynos4. I
>>> will add it later.
>>>
>>>>
>>>>> -}
>>>>> -
>>>>> -static void hdmiphy_conf_apply(struct hdmi_context *hdata)
>>>>> -{
>>>>> - const u8 *hdmiphy_data;
>>>>> - u8 buffer[32];
>>>>> - u8 operation[2];
>>>>> - u8 read_buffer[32] = {0, };
>>>>> - int ret;
>>>>> - int i;
>>>>> -
>>>>> - if (!hdata->hdmiphy_port) {
>>>>> - DRM_ERROR("hdmiphy is not attached\n");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - /* pixel clock */
>>>>> - i = hdmi_find_phy_conf(hdata, hdata->mode_conf.pixel_clock);
>>>>> - if (i < 0) {
>>>>> - DRM_ERROR("failed to find hdmiphy conf\n");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - if (hdata->type == HDMI_TYPE13) {
>>>>> - hdmiphy_data = hdmiphy_v13_configs[i].conf;
>>>>> - } else {
>>>>> - hdmiphy_data = hdmiphy_v14_configs[i].conf;
>>>>> - }
>>>>> -
>>>>> - memcpy(buffer, hdmiphy_data, 32);
>>>>> - ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
>>>>> - if (ret != 32) {
>>>>> - DRM_ERROR("failed to configure HDMIPHY via I2C\n");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - usleep_range(10000, 12000);
>>>>> -
>>>>> - /* operation mode */
>>>>> - operation[0] = 0x1f;
>>>>> - operation[1] = 0x80;
>>>>> -
>>>>> - ret = i2c_master_send(hdata->hdmiphy_port, operation, 2);
>>>>> - if (ret != 2) {
>>>>> - DRM_ERROR("failed to enable hdmiphy\n");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - ret = i2c_master_recv(hdata->hdmiphy_port, read_buffer, 32);
>>>>> - if (ret < 0) {
>>>>> - DRM_ERROR("failed to read hdmiphy config\n");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> - for (i = 0; i < ret; i++)
>>>>> - DRM_DEBUG_KMS("hdmiphy[0x%02x] write[0x%02x] - "
>>>>> - "recv [0x%02x]\n", i, buffer[i], read_buffer[i]);
>>>>> -}
>>>>> -
>>>>> -static void hdmi_conf_apply(struct hdmi_context *hdata)
>>>>> -{
>>>>> - DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> -
>>>>> - hdmiphy_conf_reset(hdata);
>>>>> - hdmiphy_conf_apply(hdata);
>>>>> -
>>>>> - mutex_lock(&hdata->hdmi_mutex);
>>>>> - hdmi_conf_reset(hdata);
>>>>> - hdmi_conf_init(hdata);
>>>>> - mutex_unlock(&hdata->hdmi_mutex);
>>>>> -
>>>>> - hdmi_audio_init(hdata);
>>>>> -
>>>>> - /* setting core registers */
>>>>> - hdmi_timing_apply(hdata);
>>>>> - hdmi_audio_control(hdata, true);
>>>>> -
>>>>> - hdmi_regs_dump(hdata, "start");
>>>>> -}
>>>>> -
>>>>> static void hdmi_set_reg(u8 *reg_pair, int num_bytes, u32 value)
>>>>> {
>>>>> int i;
>>>>> @@ -1448,7 +1129,6 @@ static void hdmi_v13_mode_set(struct hdmi_context *hdata,
>>>>>
>>>>> hdata->mode_conf.cea_video_id =
>>>>> drm_match_cea_mode((struct drm_display_mode *)m);
>>>>> - hdata->mode_conf.pixel_clock = m->clock * 1000;
>>>>
>>>> I don't think you use pixel_clock in mode_conf any longer, please
>>>> remove it from the struct.
>>>>
>>>
>>> Correct. I will remove pixel_clock from struct hdmi_conf_regs.
>>>
>>>>>
>>>>> hdmi_set_reg(core->h_blank, 2, m->htotal - m->hdisplay);
>>>>> hdmi_set_reg(core->h_v_line, 3, (m->htotal << 12) | m->vtotal);
>>>>> @@ -1544,7 +1224,6 @@ static void hdmi_v14_mode_set(struct hdmi_context *hdata,
>>>>>
>>>>> hdata->mode_conf.cea_video_id =
>>>>> drm_match_cea_mode((struct drm_display_mode *)m);
>>>>> - hdata->mode_conf.pixel_clock = m->clock * 1000;
>>>>>
>>>>> hdmi_set_reg(core->h_blank, 2, m->htotal - m->hdisplay);
>>>>> hdmi_set_reg(core->v_line, 2, m->vtotal);
>>>>> @@ -1670,13 +1349,32 @@ static void hdmi_get_max_resol(void *ctx, unsigned int *width,
>>>>> *height = MAX_HEIGHT;
>>>>> }
>>>>>
>>>>> +static void hdmi_commit_prepare(void *ctx)
>>>>
>>>> I don't know why the "_commit" is there, just hdmi_prepare will be fine.
>>>>
>>> Ok.
>>>
>>>>> +{
>>>>> + struct hdmi_context *hdata = ctx;
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + hdmiphy_conf_reset(hdata);
>>>>
>>>> There's still phy-specific kruft laying around the driver. There are
>>>> now a bunch of implicit dependencies between the hdmi driver and the
>>>> hdmiphy driver.
>>>>
>>>
>>> I need to follow this sequence:
>>>
>>> phy off (hdmiphy_prepare) -> phy reset using HDMI IP REG (hdmi_prepare) ->
>>> phy config (hdmiphy_config_apply) -> hdmi ip config (hdmi_commit)
>>>
>>> Phy reset needs to be done by toggling Phy_reset bit in HDMI IP register which
>>> cannot be accessed in hdmiphy driver. I didn't see any better way.
>>>
>>>> Another example is the sclk_hdmiphy clock, which is still controlled
>>>> by the hdmi driver, it should be controlled by the phy.
>>>>
>>>
>>> sclk_hdmiphy is one of the source clock for hdmi IP. HDMI IP needs to change the
>>> source between sclk_hdmiphy (only if stable) else pixel clock. Above
>>> code looks ok
>>> to me. Please explain a bit mode on this.
>>>
>>>>> +}
>>>>> +
>>>>> static void hdmi_commit(void *ctx)
>>>>> {
>>>>> struct hdmi_context *hdata = ctx;
>>>>>
>>>>> DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>>
>>>>> - hdmi_conf_apply(hdata);
>>>>> + mutex_lock(&hdata->hdmi_mutex);
>>>>> + hdmi_conf_reset(hdata);
>>>>> + hdmi_conf_init(hdata);
>>>>> + mutex_unlock(&hdata->hdmi_mutex);
>>>>> +
>>>>> + hdmi_audio_init(hdata);
>>>>> +
>>>>> + /* setting core registers */
>>>>> + hdmi_timing_apply(hdata);
>>>>> + hdmi_audio_control(hdata, true);
>>>>> +
>>>>> + hdmi_regs_dump(hdata, "start");
>>>>> }
>>>>>
>>>>> static void hdmi_poweron(struct hdmi_context *hdata)
>>>>> @@ -1699,8 +1397,6 @@ static void hdmi_poweron(struct hdmi_context *hdata)
>>>>> clk_enable(res->hdmiphy);
>>>>> clk_enable(res->hdmi);
>>>>> clk_enable(res->sclk_hdmi);
>>>>> -
>>>>> - hdmiphy_poweron(hdata);
>>>>> }
>>>>>
>>>>> static void hdmi_poweroff(struct hdmi_context *hdata)
>>>>> @@ -1719,7 +1415,6 @@ static void hdmi_poweroff(struct hdmi_context *hdata)
>>>>> * its reset state seems to meet the condition.
>>>>> */
>>>>> hdmiphy_conf_reset(hdata);
>>>>> - hdmiphy_poweroff(hdata);
>>>>>
>>>>> clk_disable(res->sclk_hdmi);
>>>>> clk_disable(res->hdmi);
>>>>> @@ -1761,11 +1456,11 @@ static struct exynos_hdmi_ops hdmi_ops = {
>>>>> /* display */
>>>>> .is_connected = hdmi_is_connected,
>>>>> .get_edid = hdmi_get_edid,
>>>>> - .check_timing = hdmi_check_timing,
>>>>>
>>>>> /* manager */
>>>>> .mode_set = hdmi_mode_set,
>>>>> .get_max_resol = hdmi_get_max_resol,
>>>>> + .prepare = hdmi_commit_prepare,
>>>>> .commit = hdmi_commit,
>>>>> .dpms = hdmi_dpms,
>>>>> };
>>>>> @@ -1878,7 +1573,7 @@ fail:
>>>>> return -ENODEV;
>>>>> }
>>>>>
>>>>> -static struct i2c_client *hdmi_ddc, *hdmi_hdmiphy;
>>>>> +static struct i2c_client *hdmi_ddc;
>>>>>
>>>>> void hdmi_attach_ddc_client(struct i2c_client *ddc)
>>>>> {
>>>>> @@ -1886,12 +1581,6 @@ void hdmi_attach_ddc_client(struct i2c_client *ddc)
>>>>> hdmi_ddc = ddc;
>>>>> }
>>>>>
>>>>> -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
>>>>> -{
>>>>> - if (hdmiphy)
>>>>> - hdmi_hdmiphy = hdmiphy;
>>>>> -}
>>>>> -
>>>>> #ifdef CONFIG_OF
>>>>> static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>>>>> (struct device *dev)
>>>>> @@ -2051,27 +1740,18 @@ static int hdmi_probe(struct platform_device *pdev)
>>>>>
>>>>> hdata->ddc_port = hdmi_ddc;
>>>>>
>>>>> - /* hdmiphy i2c driver */
>>>>> - if (i2c_add_driver(&hdmiphy_driver)) {
>>>>> - DRM_ERROR("failed to register hdmiphy i2c driver\n");
>>>>> - ret = -ENOENT;
>>>>> - goto err_ddc;
>>>>> - }
>>>>> -
>>>>> - hdata->hdmiphy_port = hdmi_hdmiphy;
>>>>> -
>>>>> hdata->external_irq = gpio_to_irq(hdata->hpd_gpio);
>>>>> if (hdata->external_irq < 0) {
>>>>> DRM_ERROR("failed to get GPIO external irq\n");
>>>>> ret = hdata->external_irq;
>>>>> - goto err_hdmiphy;
>>>>> + goto err_ddc;
>>>>> }
>>>>>
>>>>> hdata->internal_irq = platform_get_irq(pdev, 0);
>>>>> if (hdata->internal_irq < 0) {
>>>>> DRM_ERROR("failed to get platform internal irq\n");
>>>>> ret = hdata->internal_irq;
>>>>> - goto err_hdmiphy;
>>>>> + goto err_ddc;
>>>>> }
>>>>>
>>>>> hdata->hpd = gpio_get_value(hdata->hpd_gpio);
>>>>> @@ -2082,7 +1762,7 @@ static int hdmi_probe(struct platform_device *pdev)
>>>>> "hdmi_external", drm_hdmi_ctx);
>>>>> if (ret) {
>>>>> DRM_ERROR("failed to register hdmi external interrupt\n");
>>>>> - goto err_hdmiphy;
>>>>> + goto err_ddc;
>>>>> }
>>>>>
>>>>> ret = request_threaded_irq(hdata->internal_irq, NULL,
>>>>> @@ -2105,8 +1785,6 @@ static int hdmi_probe(struct platform_device *pdev)
>>>>>
>>>>> err_free_irq:
>>>>> free_irq(hdata->external_irq, drm_hdmi_ctx);
>>>>> -err_hdmiphy:
>>>>> - i2c_del_driver(&hdmiphy_driver);
>>>>> err_ddc:
>>>>> i2c_del_driver(&ddc_driver);
>>>>> return ret;
>>>>> @@ -2125,9 +1803,6 @@ static int hdmi_remove(struct platform_device *pdev)
>>>>> free_irq(hdata->internal_irq, hdata);
>>>>> free_irq(hdata->external_irq, hdata);
>>>>>
>>>>> -
>>>>> - /* hdmiphy i2c driver */
>>>>> - i2c_del_driver(&hdmiphy_driver);
>>>>> /* DDC i2c driver */
>>>>> i2c_del_driver(&ddc_driver);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.h b/drivers/gpu/drm/exynos/exynos_hdmi.h
>>>>> index 0ddf395..6595d7b 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.h
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.h
>>>>> @@ -15,7 +15,6 @@
>>>>> #define _EXYNOS_HDMI_H_
>>>>>
>>>>> void hdmi_attach_ddc_client(struct i2c_client *ddc);
>>>>> -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy);
>>>>>
>>>>> extern struct i2c_driver hdmiphy_driver;
>>>>> extern struct i2c_driver ddc_driver;
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmiphy.c b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
>>>>> index ea49d13..0d47302 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_hdmiphy.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
>>>>> @@ -16,33 +16,437 @@
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/i2c.h>
>>>>> #include <linux/module.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> +#include <linux/clk.h>
>>>>>
>>>>> #include "exynos_drm_drv.h"
>>>>> +#include "exynos_drm_hdmi.h"
>>>>> #include "exynos_hdmi.h"
>>>>>
>>>>> +#include "regs-hdmiphy.h"
>>>>>
>>>>> -static int hdmiphy_probe(struct i2c_client *client,
>>>>> - const struct i2c_device_id *id)
>>>>> +#define HDMIPHY_REG_COUNT (32)
>>>>
>>>> No need to include parentheses.
>>>
>>> Ok.
>>>>
>>>>> +
>>>>> +enum hdmiphy_type {
>>>>> + HDMIPHY_EXYNOS4210,
>>>>> + HDMIPHY_EXYNOS4212,
>>>>> +};
>>>>> +
>>>>> +struct hdmiphy_context {
>>>>> + struct device *dev;
>>>>> + bool powered;
>>>>> + void *parent_ctx;
>>>>> + enum hdmiphy_type type;
>>>>> + const struct hdmiphy_config *conf;
>>>>> + struct clk *hdmiphy;
>>>>> +};
>>>>> +
>>>>> +struct hdmiphy_config {
>>>>> + int pixel_clock;
>>>>> + u8 conf[HDMIPHY_REG_COUNT];
>>>>> +};
>>>>> +
>>>>> +/* list of all required phy config settings */
>>>>> +static const struct hdmiphy_config hdmiphy_4212_configs[] = {
>>>>> + {
>>>>> + .pixel_clock = 25200000,
>>>>> + .conf = {
>>>>> + 0x01, 0x51, 0x2A, 0x75, 0x40, 0x01, 0x00, 0x08,
>>>>> + 0x82, 0x80, 0xfc, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xf4, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 27000000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x22, 0x51, 0x40, 0x08, 0xfc, 0x20,
>>>>> + 0x98, 0xa0, 0xcb, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x06, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xe4, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 27027000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x2d, 0x72, 0x40, 0x64, 0x12, 0x08,
>>>>> + 0x43, 0xa0, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xe3, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 36000000,
>>>>> + .conf = {
>>>>> + 0x01, 0x51, 0x2d, 0x55, 0x40, 0x01, 0x00, 0x08,
>>>>> + 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xab, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 40000000,
>>>>> + .conf = {
>>>>> + 0x01, 0x51, 0x32, 0x55, 0x40, 0x01, 0x00, 0x08,
>>>>> + 0x82, 0x80, 0x2c, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0x9a, 0x24, 0x00, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 65000000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x36, 0x34, 0x40, 0x1e, 0x0a, 0x08,
>>>>> + 0x82, 0xa0, 0x45, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xbd, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 74176000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x3e, 0x35, 0x40, 0x5b, 0xde, 0x08,
>>>>> + 0x82, 0xa0, 0x73, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x56, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xa6, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 74250000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x1f, 0x10, 0x40, 0x40, 0xf8, 0x08,
>>>>> + 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xa5, 0x24, 0x01, 0x00, 0x00, 0x01, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 83500000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x23, 0x11, 0x40, 0x0c, 0xfb, 0x08,
>>>>> + 0x85, 0xa0, 0xd1, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0x93, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 106500000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x2c, 0x12, 0x40, 0x0c, 0x09, 0x08,
>>>>> + 0x84, 0xa0, 0x0a, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0x73, 0x24, 0x01, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 108000000,
>>>>> + .conf = {
>>>>> + 0x01, 0x51, 0x2d, 0x15, 0x40, 0x01, 0x00, 0x08,
>>>>> + 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0xc7, 0x25, 0x03, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 146250000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x3d, 0x15, 0x40, 0x18, 0xfd, 0x08,
>>>>> + 0x83, 0xa0, 0x6e, 0xd9, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0x50, 0x25, 0x03, 0x00, 0x00, 0x01, 0x80,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 148500000,
>>>>> + .conf = {
>>>>> + 0x01, 0xd1, 0x1f, 0x00, 0x40, 0x40, 0xf8, 0x08,
>>>>> + 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac, 0x80,
>>>>> + 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44, 0x86,
>>>>> + 0x54, 0x4b, 0x25, 0x03, 0x00, 0x00, 0x01, 0x00,
>>>>> + },
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +static const struct hdmiphy_config hdmiphy_4210_configs[] = {
>>>>> + {
>>>>> + .pixel_clock = 27000000,
>>>>> + .conf = {
>>>>> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30, 0x40,
>>>>> + 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54, 0x87,
>>>>> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> + 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 27027000,
>>>>> + .conf = {
>>>>> + 0x01, 0x05, 0x00, 0xD4, 0x10, 0x9C, 0x09, 0x64,
>>>>> + 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54, 0x87,
>>>>> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> + 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 74176000,
>>>>> + .conf = {
>>>>> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xef, 0x5B,
>>>>> + 0x6D, 0x10, 0x01, 0x51, 0xef, 0xF3, 0x54, 0xb9,
>>>>> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> + 0x22, 0x40, 0xa5, 0x26, 0x01, 0x00, 0x00, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 74250000,
>>>>> + .conf = {
>>>>> + 0x01, 0x05, 0x00, 0xd8, 0x10, 0x9c, 0xf8, 0x40,
>>>>> + 0x6a, 0x10, 0x01, 0x51, 0xff, 0xf1, 0x54, 0xba,
>>>>> + 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10, 0xe0,
>>>>> + 0x22, 0x40, 0xa4, 0x26, 0x01, 0x00, 0x00, 0x00,
>>>>> + },
>>>>> + },
>>>>> + {
>>>>> + .pixel_clock = 148500000,
>>>>> + .conf = {
>>>>> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xf8, 0x40,
>>>>> + 0x6A, 0x18, 0x00, 0x51, 0xff, 0xF1, 0x54, 0xba,
>>>>> + 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10, 0xE0,
>>>>> + 0x22, 0x40, 0xa4, 0x26, 0x02, 0x00, 0x00, 0x00,
>>>>> + },
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +static const struct hdmiphy_config *hdmiphy_find_conf(void *ctx,
>>>>> + const struct drm_display_mode *mode)
>>>>> +{
>>>>> + struct hdmiphy_context *hdata = ctx;
>>>>> + const struct hdmiphy_config *confs;
>>>>> + int count, i;
>>>>> +
>>>>> + switch (hdata->type) {
>>>>> + case HDMIPHY_EXYNOS4212:
>>>>> + confs = hdmiphy_4212_configs;
>>>>> + count = ARRAY_SIZE(hdmiphy_4212_configs);
>>>>> + break;
>>>>> + case HDMIPHY_EXYNOS4210:
>>>>> + confs = hdmiphy_4210_configs;
>>>>> + count = ARRAY_SIZE(hdmiphy_4210_configs);
>>>>> + break;
>>>>> + default:
>>>>> + return NULL;
>>>>
>>>> Should probably DRM_ERROR here.
>>>>
>>> ok. I will add that.
>>>
>>>>> + }
>>>>> +
>>>>> + for (i = 0; i < count; i++)
>>>>> + if (confs[i].pixel_clock == (mode->clock * 1000))
>>>>> + return &confs[i];
>>>>> +
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_check_timing(void *ctx, struct drm_display_mode *mode)
>>>>> {
>>>>> - hdmi_attach_hdmiphy_client(client);
>>>>> + const struct hdmiphy_config *conf;
>>>>> + DRM_DEBUG_KMS("%s : xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n",
>>>>> + __func__, mode->hdisplay, mode->vdisplay,
>>>>> + mode->vrefresh, (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>>>> + ? true : false, mode->clock * 1000);
>>>>
>>>> No need to print __func__ here, DRM_DEBUG_KMS will do that for you. I
>>>> think you should also parse the flags to a string instead of a bool
>>>> cast to an int.
>>>>
>>> ok.
>>>
>>>>>
>>>>> - dev_info(&client->adapter->dev, "attached s5p_hdmiphy "
>>>>> - "into i2c adapter successfully\n");
>>>>> + conf = hdmiphy_find_conf(ctx, mode);
>>>>> + if (!conf) {
>>>>> + DRM_DEBUG_KMS("Display Mode is not supported.\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int hdmiphy_remove(struct i2c_client *client)
>>>>> +static void hdmiphy_mode_set(void *ctx, struct drm_display_mode *mode)
>>>>> +{
>>>>> + struct hdmiphy_context *hdata = ctx;
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>
>>>> No need to print __func__ here, DRM_DEBUG_KMS will do that for you.
>>>> This comment stand for the rest of the calls, as well.
>>>>
>>>>> +
>>>>> + hdata->conf = hdmiphy_find_conf(ctx, mode);
>>>>> +}
>>>>> +
>>>>> +static void hdmiphy_config_prepare(void *ctx)
>>>>> +{
>>>>> + struct hdmiphy_context *hdata = ctx;
>>>>> + const struct i2c_client *client = to_i2c_client(hdata->dev);
>>>>> + u8 buffer[2];
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + /* operation mode */
>>>>> + buffer[0] = HDMIPHY_MODE_SET_DONE;
>>>>> + buffer[1] = 0x00;
>>>>> +
>>>>> + if (client)
>>>>> + i2c_master_send(client, buffer, 2);
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_config_apply(void *ctx)
>>>>> {
>>>>> - dev_info(&client->adapter->dev, "detached s5p_hdmiphy "
>>>>> - "from i2c adapter successfully\n");
>>>>> + struct hdmiphy_context *hdata = ctx;
>>>>> + struct i2c_client *client = to_i2c_client(hdata->dev);
>>>>> + const u8 *hdmiphy_data;
>>>>> + u8 buffer[HDMIPHY_REG_COUNT];
>>>>> + u8 operation[2];
>>>>> + u8 read_buffer[HDMIPHY_REG_COUNT] = {0, };
>>>>> + int ret;
>>>>> + int i;
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + /* pixel clock */
>>>>> + if (hdata->conf && client)
>>>>> + hdmiphy_data = hdata->conf->conf;
>>>>> + else
>>>>> + return -EINVAL;
>>>>> +
>>>>> + memcpy(buffer, hdmiphy_data, HDMIPHY_REG_COUNT);
>>>>> +
>>>>> + ret = i2c_master_send(client, buffer, HDMIPHY_REG_COUNT);
>>>>> + if (ret != HDMIPHY_REG_COUNT) {
>>>>> + DRM_ERROR("failed to configure HDMIPHY via I2C\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + usleep_range(10000, 12000);
>>>>> +
>>>>> + /* operation mode */
>>>>> + operation[0] = HDMIPHY_MODE_SET_DONE;
>>>>> + operation[1] = HDMIPHY_MODE_EN;
>>>>> +
>>>>> + ret = i2c_master_send(client, operation, 2);
>>>>> + if (ret != 2) {
>>>>> + DRM_ERROR("failed to enable hdmiphy\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = i2c_master_recv(client, read_buffer, HDMIPHY_REG_COUNT);
>>>>> + if (ret < 0) {
>>>>> + DRM_ERROR("failed to read hdmiphy config\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + for (i = 0; i < ret; i++)
>>>>> + DRM_DEBUG_KMS("hdmiphy[0x%02x] wr[0x%02x], rd[0x%02x]\n",
>>>>> + i, buffer[i], read_buffer[i]);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void hdmiphy_dpms(void *ctx, int mode)
>>>>> +{
>>>>> + struct hdmiphy_context *hdata = ctx;
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s mode %d\n", __LINE__, __func__, mode);
>>>>> +
>>>>> + switch (mode) {
>>>>> + case DRM_MODE_DPMS_ON:
>>>>> + if (pm_runtime_suspended(hdata->dev))
>>>>> + pm_runtime_get_sync(hdata->dev);
>>>>> + break;
>>>>> + case DRM_MODE_DPMS_STANDBY:
>>>>> + case DRM_MODE_DPMS_SUSPEND:
>>>>> + case DRM_MODE_DPMS_OFF:
>>>>> + if (!pm_runtime_suspended(hdata->dev))
>>>>> + pm_runtime_put_sync(hdata->dev);
>>>>> + break;
>>>>> + default:
>>>>> + DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
>>>>> + break;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_update_bits(struct i2c_client *client, u8 *reg_cache,
>>>>> + u8 reg, u8 mask, u8 val)
>>>>> +{
>>>>> + int ret;
>>>>> + u8 buffer[2];
>>>>> +
>>>>> + buffer[0] = reg;
>>>>> + buffer[1] = (reg_cache[reg] & ~mask) | (val & mask);
>>>>> + reg_cache[reg] = buffer[1];
>>>>> +
>>>>> + ret = i2c_master_send(client, buffer, 2);
>>>>> + if (ret != 2)
>>>>> + return -EIO;
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static int hdmiphy_4412_turn_on(struct i2c_client *client, bool on)
>>>>
>>>> This name is misleading. Please change it to something that doesn't
>>>> convey a power state, such as hdmiphy_4412_power
>>>>
>>> ok. I will change that.
>>>
>>>>> +{
>>>>> + u8 reg_cache[HDMIPHY_REG_COUNT] = { 0 };
>>>>> + u8 buffer[2];
>>>>> + int ret;
>>>>> +
>>>>> + DRM_DEBUG_KMS("%s: hdmiphy is %s\n", __func__, on ? "on" : "off");
>>>>> +
>>>>> + /* Cache all hdmi-phy registers to make the code below faster */
>>>>> + buffer[0] = 0x0;
>>>>> + ret = i2c_master_send(client, buffer, 1);
>>>>> + if (ret != 1) {
>>>>> + ret = -EIO;
>>>>> + goto exit;
>>>>> + }
>>>>> + ret = i2c_master_recv(client, reg_cache, HDMIPHY_REG_COUNT);
>>>>> + if (ret != HDMIPHY_REG_COUNT) {
>>>>> + ret = -EIO;
>>>>> + goto exit;
>>>>> + }
>>>>> +
>>>>> + /* Change to/from configuration from/to operation mode */
>>>>> + ret = hdmiphy_update_bits(client, reg_cache, HDMIPHY_MODE_SET_DONE,
>>>>> + HDMIPHY_MODE_EN, on ? ~0 : 0);
>>>>> + if (ret)
>>>>> + goto exit;
>>>>> +
>>>>> + /*
>>>>> + * Turn off "oscpad" if !on; it turns on again in
>>>>> + * during phy-configuration.
>>>>> + */
>>>>> + if (!on)
>>>>> + ret = hdmiphy_update_bits(client, reg_cache,
>>>>> + HDMIPHY_4212_OSC_PAD_CON, HDMIPHY_OSC_PAD_EN, 0);
>>>>> + if (ret)
>>>>> + goto exit;
>>>>> +
>>>>> + /* Disable powerdown if on; enable if !on */
>>>>
>>>> This comment really isn't useful, it's just describing the code.
>>>
>>> I will remove this comment.
>>>
>>>>
>>>>> + ret = hdmiphy_update_bits(client, reg_cache, HDMIPHY_4212_PD_CON,
>>>>> + HDMIPHY_PDEN, on ? 0 : ~0);
>>>>> + if (ret)
>>>>> + goto exit;
>>>>> + ret = hdmiphy_update_bits(client, reg_cache, HDMIPHY_4212_PD_CON,
>>>>> + HDMIPHY_PD_ALL, on ? 0 : ~0);
>>>>> + if (ret)
>>>>> + goto exit;
>>>>> +
>>>>> + /* Disable pixel clock generator block if !on */
>>>>> + if (!on)
>>>>> + ret = hdmiphy_update_bits(client, reg_cache,
>>>>> + HDMIPHY_4212_PCG_CON, HDMIPHY_PCG_RESET_EN, 0);
>>>>> + if (ret)
>>>>> + goto exit;
>>>>> +
>>>>> +exit:
>>>>> + /* Don't expect any errors so just do a single warn */
>>>>> + WARN_ON(ret);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static struct exynos_hdmiphy_ops hdmiphy_ops = {
>>>>> + .check_timing = hdmiphy_check_timing,
>>>>> + .mode_set = hdmiphy_mode_set,
>>>>> + .prepare = hdmiphy_config_prepare,
>>>>> + .config_apply = hdmiphy_config_apply,
>>>>> + .dpms = hdmiphy_dpms,
>>>>> +};
>>>>> +
>>>>> static const struct i2c_device_id hdmiphy_id[] = {
>>>>> - { "s5p_hdmiphy", 0 },
>>>>> - { "exynos5-hdmiphy", 0 },
>>>>> + { "s5p_hdmiphy", HDMIPHY_EXYNOS4210 },
>>>>> + { "exynos5-hdmiphy", HDMIPHY_EXYNOS4212 },
>>>>> { },
>>>>> };
>>>>>
>>>>> @@ -50,21 +454,183 @@ static const struct i2c_device_id hdmiphy_id[] = {
>>>>> static struct of_device_id hdmiphy_match_types[] = {
>>>>> {
>>>>> .compatible = "samsung,exynos5-hdmiphy",
>>>>> + .data = (void *)HDMIPHY_EXYNOS4212,
>>>>> }, {
>>>>> /* end node */
>>>>> }
>>>>> };
>>>>> #endif
>>>>>
>>>>> +static int hdmiphy_probe(struct i2c_client *client,
>>>>> + const struct i2c_device_id *id)
>>>>> +{
>>>>> + struct device *dev = &client->dev;
>>>>> + struct hdmiphy_context *hdata;
>>>>> + struct exynos_drm_hdmi_context *drm_hdmi_ctx;
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + drm_hdmi_ctx = devm_kzalloc(dev, sizeof(*drm_hdmi_ctx),
>>>>> + GFP_KERNEL);
>>>>> + if (!drm_hdmi_ctx) {
>>>>> + DRM_ERROR("failed to allocate common hdmi context.\n");
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> +
>>>>> + hdata = devm_kzalloc(dev, sizeof(*hdata), GFP_KERNEL);
>>>>> + if (!hdata) {
>>>>> + DRM_ERROR("failed to allocate hdmiphy context.\n");
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> +
>>>>> + if (dev->of_node) {
>>>>> + const struct of_device_id *match;
>>>>> + match = of_match_node(of_match_ptr(hdmiphy_match_types),
>>>>> + dev->of_node);
>>>>> + if (match == NULL)
>>>>> + return -ENODEV;
>>>>> + hdata->type = (enum hdmiphy_type)match->data;
>>>>> + } else {
>>>>> + hdata->type = (enum hdmiphy_type)id->driver_data;
>>>>> + }
>>>>> +
>>>>> + drm_hdmi_ctx->ctx = (void *)hdata;
>>>>> + hdata->parent_ctx = (void *)drm_hdmi_ctx;
>>>>> + hdata->dev = dev;
>>>>> +
>>>>> + hdata->hdmiphy = devm_clk_get(dev, "hdmiphy");
>>>>
>>>> I don't see this call removed in the hdmi driver, am I missing something?
>>>>
>>>
>>> I took it wrong. I thought hdmi IP is clocked by hdmiphy clock. Actually it is
>>> sclk_hdmiphy clock. I will remove this from hdmi driver.
>>>
>>>>> + if (IS_ERR_OR_NULL(hdata->hdmiphy)) {
>>>>> + DRM_ERROR("failed to get clock 'hdmiphy'\n");
>>>>> + return PTR_ERR(hdata->hdmiphy);
>>>>> + }
>>>>> +
>>>>> + i2c_set_clientdata(client, hdata);
>>>>> +
>>>>> + /* Attach HDMI-PHY Driver to common hdmi. */
>>>>> + exynos_hdmiphy_drv_attach(drm_hdmi_ctx);
>>>>> +
>>>>> + /* register specific callbacks to common hdmi. */
>>>>> + exynos_hdmiphy_ops_register(&hdmiphy_ops);
>>>>> +
>>>>> + pm_runtime_enable(dev);
>>>>> +
>>>>> + dev_info(&client->adapter->dev,
>>>>> + "attached s5p_hdmiphy into i2c adapter successfully\n");
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_remove(struct i2c_client *client)
>>>>> +{
>>>>> + dev_info(&client->adapter->dev,
>>>>> + "detached s5p_hdmiphy from i2c adapter successfully\n");
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void hdmiphy_poweroff(struct device *dev)
>>>>> +{
>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>> + struct hdmiphy_context *hdata = i2c_get_clientdata(client);
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + if (hdata->type == HDMIPHY_EXYNOS4212)
>>>>> + hdmiphy_4412_turn_on(client, 0);
>>>>> +
>>>>> + clk_disable(hdata->hdmiphy);
>>>>> +}
>>>>> +
>>>>> +static void hdmiphy_poweron(struct device *dev)
>>>>> +{
>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>> + struct hdmiphy_context *hdata = i2c_get_clientdata(client);
>>>>> +
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + clk_enable(hdata->hdmiphy);
>>>>> +
>>>>> + if (hdata->type == HDMIPHY_EXYNOS4212)
>>>>> + hdmiphy_4412_turn_on(client, 1);
>>>>> +}
>>>>> +
>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>> +static int hdmiphy_suspend(struct device *dev)
>>>>> +{
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + if (pm_runtime_suspended(dev)) {
>>>>> + DRM_DEBUG_KMS("%s: already runtime-suspended.\n",
>>>>> + __func__);
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + hdmiphy_poweroff(dev);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_resume(struct device *dev)
>>>>> +{
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + if (pm_runtime_suspended(dev)) {
>>>>> + /* dpms callback should resume the mixer. */
>>>>> + DRM_DEBUG_KMS("%s: already runtime-suspended.\n",
>>>>> + __func__);
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + hdmiphy_poweron(dev);
>>>>> + return 0;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +
>>>>> +#ifdef CONFIG_PM_RUNTIME
>>>>> +static int hdmiphy_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + hdmiphy_poweroff(dev);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int hdmiphy_runtime_resume(struct device *dev)
>>>>> +{
>>>>> + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>>> +
>>>>> + hdmiphy_poweron(dev);
>>>>> + return 0;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +static const struct dev_pm_ops hdmiphy_pm_ops = {
>>>>> + SET_SYSTEM_SLEEP_PM_OPS(hdmiphy_suspend, hdmiphy_resume)
>>>>> + SET_RUNTIME_PM_OPS(hdmiphy_runtime_suspend,
>>>>> + hdmiphy_runtime_resume, NULL)
>>>>> +};
>>>>> +
>>>>> struct i2c_driver hdmiphy_driver = {
>>>>> .driver = {
>>>>> .name = "exynos-hdmiphy",
>>>>> .owner = THIS_MODULE,
>>>>> .of_match_table = of_match_ptr(hdmiphy_match_types),
>>>>> + .pm = &hdmiphy_pm_ops,
>>>>> },
>>>>> .id_table = hdmiphy_id,
>>>>> .probe = hdmiphy_probe,
>>>>> .remove = hdmiphy_remove,
>>>>> .command = NULL,
>>>>> };
>>>>> +
>>>>> +extern int exynos_hdmiphy_driver_register(void)
>>>>> +{
>>>>> + return i2c_add_driver(&hdmiphy_driver);
>>>>> +}
>>>>> +
>>>>> +extern void exynos_hdmiphy_driver_unregister(void)
>>>>> +{
>>>>> + i2c_del_driver(&hdmiphy_driver);
>>>>> +}
>>>>> +
>>>>> EXPORT_SYMBOL(hdmiphy_driver);
>>>>> diff --git a/drivers/gpu/drm/exynos/regs-hdmiphy.h b/drivers/gpu/drm/exynos/regs-hdmiphy.h
>>>>> new file mode 100644
>>>>> index 0000000..8f906c6
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/exynos/regs-hdmiphy.h
>>>>> @@ -0,0 +1,61 @@
>>>>> +/*
>>>>> + *
>>>>> + * regs-hdmiphy.h
>>>>> + *
>>>>> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
>>>>> + * http://www.samsung.com/
>>>>> + *
>>>>> + * HDMI-PHY register header file for Samsung TVOUT driver
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> +*/
>>>>> +
>>>>> +#ifndef SAMSUNG_REGS_HDMIPHY_H
>>>>> +#define SAMSUNG_REGS_HDMIPHY_H
>>>>> +
>>>>> +/*
>>>>> + * Register part
>>>>> +*/
>>>>> +
>>>>> +/* HDMI PHY Version Common */
>>>>> +#define HDMIPHY_MODE_SET_DONE (0x1f)
>>>>> +
>>>>> +/* HDMI PHY Version 4212 */
>>>>> +#define HDMIPHY_4212_PCG_CON (0x04)
>>>>> +#define HDMIPHY_4212_OSC_PAD_CON (0x0b)
>>>>> +#define HDMIPHY_4212_PD_CON (0x1d)
>>>>> +
>>>>> +/*
>>>>> + * Bit definition part
>>>>> + */
>>>>> +
>>>>> +/* HDMIPHY_MODE_SET_DONE */
>>>>> +#define HDMIPHY_MODE_EN (1 << 7)
>>>>> +
>>>>> +/* HDMIPHY_4212_PCG_CON */
>>>>> +#define HDMIPHY_PCG_RESET_EN (1 << 3)
>>>>> +
>>>>> +/* HDMIPHY_4212_OSC_PAD_CON */
>>>>> +#define HDMIPHY_OSC_PAD_EN (3 << 6)
>>>>> +
>>>>> +/* HDMIPHY_4212_PD_CON */
>>>>> +#define HDMIPHY_PDEN (1 << 7)
>>>>> +
>>>>> +#define HDMIPHY_PLL_PD (1 << 6)
>>>>> +#define HDMIPHY_CLKSER_PD (1 << 5)
>>>>> +#define HDMIPHY_CLKDRV_PD (1 << 4)
>>>>> +
>>>>> +#define HDMIPHY_DRV_PD (1 << 2)
>>>>> +#define HDMIPHY_SER_PD (1 << 1)
>>>>> +#define HDMIPHY_ICLK_PD (1 << 0)
>>>>> +
>>>>> +#define HDMIPHY_PD_ALL (HDMIPHY_PLL_PD |\
>>>>> + HDMIPHY_CLKSER_PD |\
>>>>> + HDMIPHY_CLKDRV_PD|\
>>>>> + HDMIPHY_DRV_PD|\
>>>>> + HDMIPHY_SER_PD|\
>>>>> + HDMIPHY_ICLK_PD)
>>>>> +
>>>>> +#endif /* SAMSUNG_REGS_HDMIPHY_H */
>>>>> --
>>>>> 1.8.0
>>>>>
>>>
>>> --
>>> Regards,
>>> Rahul Sharma,
>>> email to: rahul.sharma at samsung.com
>>> Samsung India.
>>
>>
>>
>> --
>> Regards,
>> Rahul Sharma,
>> email to: rahul.sharma at samsung.com
>> Samsung India.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Regards,
Rahul Sharma,
email to: rahul.sharma at samsung.com
Samsung India.
More information about the dri-devel
mailing list