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

[김승우]

On 2013년 03월 07일 15:45, Rahul Sharma wrote:
> Thanks Seung Woo, Mr. Dae,
> 
> On Thu, Mar 7, 2013 at 10:13 AM, Inki Dae <inki.dae at samsung.com> wrote:
>> 2013/3/7 김승우 <sw0312.kim at 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 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.
>>>
>>> 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.

Considering this, exynos_drm_hdmi_probe() is more proper position.

> 
> 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).

Yes, it makes sense. All real hw blocks for hdmi including mixer, hdmi,
and hdmiphy shoulde be registered in exynos_drm_hdmi (drm-common-hdmi
for exynos).

Thanks and Regards,
- Seung-Woo Kim

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

-- 
Seung-Woo Kim
Samsung Software R&D Center
--