Re: [PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver

From: Rahul Sharma <r.sh.open@gmail.com>
To: Inki Dae <inki.dae@samsung.com>
Cc: sw0312.kim@samsung.com, linux-samsung-soc@vger.kernel.org,
	Sean Paul <seanpaul@google.com>, sunil joshi <joshi@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Rahul Sharma <rahul.sharma@samsung.com>
Subject: Re: [PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
Date: Thu, 7 Mar 2013 12:15:15 +0530	[thread overview]
Message-ID: <CAPdUM4MhLaw=c_MKedHzSJ4irk1d_dXcmdfi15GSRSmhvH4_TQ@mail.gmail.com> (raw)
In-Reply-To: <CAAQKjZMSqaD60vqM-wjuLU8O=uPsDkFKGugbOc-uKdF6QfmA7w@mail.gmail.com>

Thanks Seung Woo, Mr. Dae,

On Thu, Mar 7, 2013 at 10:13 AM, Inki Dae <inki.dae@samsung.com> wrote:
> 2013/3/7 김승우 <sw0312.kim@samsung.com>:
>>
>>
>> On 2013년 03월 04일 23:05, Rahul Sharma wrote:
>>> Thanks Sean,
>>>
>>> On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul <seanpaul@google.com> wrote:
>>>> On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma <rahul.sharma@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.
>>
>> Basically, I agree with the idea to split hdmiphy from hdmi. And it
>> seems that already existing hdmiphy i2c device is just reused and
>> hdmiphy_power_on is reorganized to hdmiphy dpms operation: even calling
>> flow of power operations is reordered.
>>
>> But I'm not sure exynos_hdmiphy_driver_register() really need to be
>> called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough to
>> call exynos_hdmiphy_driver_register() from hdmi_probe() because hdmiphy
>> is only used from hdmi.
>>
>
> I agree with Seung-Woo. The hdmiphy is just one part of HDMI subsystem.
>

I agree to the Seung Woo's point that hdmi-phy used to be solely accessed by
hdmi driver.  But in this RFC, hdmi-phy is not called by hdmi driver
anymore. Phy
ops will be called from drm-common-hdmi platform driver along with mixer and
hdmi ops.

The rational behind my implementation is that I am projecting hdmi-phy as
a device which is peer to hdmi ip and mixer. These 3 devices together makes
DRM HDMI subsystem.

Even physically hdmi-phy doesn't seems to be a part of hdmi ip though
configurations are listed under hdmi ip user manual. It looks like a
isolated module accessed by i2c.

Though I don't find anything wrong with Seung Woo suggestion but above
placement of hdmi-phy (parallel to hdmi and mixer) makes more sense
to me.

Please have a another look at it and let me know your opinion.

Another things which bothers me is registering mixer, hdmi driver inside
exynos_drm_init(). If we strictly follow the hierarchy inside drm,
exynos_drm_init()
should register drm-common-hdmi only. drm-common-hdmi should register
mixer and hdmi (or hdmi-phy as well).

regards,
Rahul Sharma.

> Thanks,
> Inki Dae
>
>> Thanks and Regards,
>> - Seung-Woo Kim
>>
>>>
>>> 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@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
>>>>>
>>
>> <snip>
>>
>> --
>> Seung-Woo Kim
>> Samsung Software R&D Center
>> --
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Regards,
Rahul Sharma,
email to: rahul.sharma@samsung.com
Samsung India.