Thanks Mr. Dae,

I have some points for discussion.

On 30 August 2013 14:03, Inki Dae <inki....@samsung.com> wrote:
> Hi Rahul.
>
> Thanks for your patch set.
>
> I had just quick review to all patch series. And I think we could fully hide
> hdmiphy interfaces,
> exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
> driver.
> That may be prototyped like below,
>
> at exynos_hdmi.h
>
> /* Define hdmiphy callbacks. */
> struct exynos_hdmiphy_ops {
>         void (*enable)(struct device *dev);
>         void (*disable)(struct device *dev);
>         int (*check_mode)(struct device *dev, struct drm_display_mode
> *mode);
>         int (*set_mode)(struct device *dev, struct drm_display_mode *mode);
>         int (*apply)(struct device *dev);
> };
>

Above looks fine to me. I will hide the ops as you suggested.

>
> at exynos_hdmi.c
>
> /*
>   * Add a new structure for hdmi driver data.
>   * type could be HDMI_TYPE13 or HDMI_TYPE14.
>   * i2c_hdmiphy could be true or false: true means that current hdmi device
> uses i2c
>   * based hdmiphy device, otherwise platform device based one.
>   */
> struct hdmi_drv_data {
>         unsigned int type;
>         unsigned int i2c_hdmiphy;
> };
>
> ...
>
> /* Add new members to hdmi context. */
> struct hdmi_context {
>         ...
>         struct hdmi_drv_data *drv_data;
>         struct hdmiphy_ops *ops;
>         ...
> };
>
>
> /* Add hdmi device data according Exynos SoC. */
> static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
>         .type = HDMI_TYPE14,
>         .i2c_hdmiphy = true
> };
>
> static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
>         .type = HDMI_TYPE14,
>         .i2c_hdmiphy = false
> };
>
>
> static struct of_device_id hdmi_match_types[] = {
>         {
>                 .compatible = "samsung,exynos4212-hdmi",
>                 .data           = (void *)&exynos4212_hdmi_drv_data,
>         }, {
>         ...
>
>                 .compatible = "samsung,exynos5420-hdmi",
>                 .data           = (void *)&exynos5420_hdmi_drv_data,
>         }, {
>         }
> };
>
> /* the below example function shows how hdmiphy interfaces can be hided from
> hdmi driver. */
> static void hdmi_mode_set(...)
> {
>         ...
>         hdata->ops->set_mode(hdata->hdmiphy_dev, mode);

This looks fine.

> }
>
> static int hdmi_get_phy_device(struct hdmi_context *hdata)
> {
>         struct hdmi_drv_data *data = hdata->drv_data;
>
>         ...
>         /* Register hdmiphy driver according to i2c_hdmiphy value. */
>         ret = exynos_hdmiphy_driver_register(data->i2c_hdmiphy);
>         ...
>         /* Get hdmiphy driver ops according to i2c_hdmiphy value. */
>         hdata->ops = exynos_hdmiphy_get_ops(data->i2c_hdmiphy);
>         ...
> }
>
>
> at exynos_hdmiphy.c
>
> /* Define hdmiphy ops respectively. */
> struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
>         .enable = exynos_hdmiphy_i2c_enable,
>         .disable = exynos_hdmiphy_i2c_disable,
>         ...
> };
>
> struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
>         .enable = exynos_hdmiphy_platdev_enable,
>         .disable = exynos_hdmiphy_platdev_disable,
>         ...
> };

Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata->dev),
we use i2c_master_send or writeb.

>
> struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int i2c_hdmiphy)
> {
>         /* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
>         if (i2c_hdmiphy)
>                 return &hdmiphy_i2c_ops;
>
>         return &hdmiphy_platdev_ops;
> }

We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.

>
> int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
> {
>         ...
>         /* Register hdmiphy driver appropriately according to i2c_hdmiphy.
> */
>         if (i2c_hdmiphy) {
>                 ret = i2c_add_driver(&hdmiphy_i2c_driver);
>                 ...
>         } else {
>                 ret = platform_driver_register(&hdmiphy_platform_driver);
>                 ...
>         }
>

Here i2c_hdmiphy flag helps in avoiding registering both i2c
and platform drivers for phy. But is it a concern that we should
not register 2 drivers on different buses for single hw device. I am
still not clear on this.

Otherwise we do not need to maintain i2c_hdmiphy flag.

Secondly, we always have registration of i2c driver for ddc and
hdmiphy driver in hdmi probe. It works. I have also tested above
series for 5420 and 5250 smdk board. There are no issues.

regards,
Rahul Sharma.

>         return ret;
> }
>
> Thanks,
> Inki Dae
>
>> -----Original Message-----
>> From: Rahul Sharma [mailto:rahul.sha...@samsung.com]
>> Sent: Friday, August 30, 2013 3:59 PM
>> To: linux-samsung-...@vger.kernel.org; dri-devel@lists.freedesktop.org
>> Cc: kgene....@samsung.com; sw0312....@samsung.com; inki....@samsung.com;
>> seanp...@chromium.org; l.st...@pengutronix.de; tomasz.f...@gmail.com;
>> s.nawro...@samsung.com; jo...@samsung.com; r.sh.o...@gmail.com; Rahul
>> Sharma
>> Subject: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
>> driver
>>
>> Currently, exynos hdmiphy operations and configs are kept
>> inside the hdmi driver. Hdmiphy related code is tightly
>> coupled with hdmi IP driver.
>>
>> This series also removes hdmiphy dummy clock for hdmiphy
>> and replace it with Phy PMU Control from the hdmiphy driver.
>>
>> At the end, support for exynos5420 hdmiphy is added to the
>> hdmiphy driver which is a platform device.
>>
>> Drm related paches are based on exynos-drm-next branch at
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> Arch patches are dependent on
>> http://www.mail-archive.com/linux-samsung-
>> s...@vger.kernel.org/msg22195.html
>>
>> Rahul Sharma (7):
>>   drm/exynos: move hdmiphy related code to hdmiphy driver
>>   drm/exynos: remove dummy hdmiphy clock
>>   drm/exynos: add hdmiphy pmu bit control in hdmiphy driver
>>   drm/exynos: add support for exynos5420 hdmiphy
>>   exynos/drm: fix ddc i2c device probe failure
>>   ARM: dts: update hdmiphy dt node for exynos5250
>>   ARM: dts: update hdmiphy dt node for exynos5420
>>
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |    2 +
>>  .../devicetree/bindings/video/exynos_hdmiphy.txt   |    6 +
>>  arch/arm/boot/dts/exynos5250-smdk5250.dts          |    9 +-
>>  arch/arm/boot/dts/exynos5420.dtsi                  |   12 +
>>  drivers/gpu/drm/exynos/exynos_ddc.c                |    5 +
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h           |   13 +
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |  353 ++--------
>>  drivers/gpu/drm/exynos/exynos_hdmiphy.c            |  738
>> +++++++++++++++++++-
>>  drivers/gpu/drm/exynos/regs-hdmiphy.h              |   35 +
>>  9 files changed, 868 insertions(+), 305 deletions(-)
>>  create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h
>>
>> --
>> 1.7.10.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to