From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755897AbbIDAls (ORCPT <rfc822;w@1wt.eu>);
	Thu, 3 Sep 2015 20:41:48 -0400
Received: from mailout1.w1.samsung.com ([210.118.77.11]:29230 "EHLO
	mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751103AbbIDAlp (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 3 Sep 2015 20:41:45 -0400
MIME-version: 1.0
Content-type: text/plain; charset=utf-8
X-AuditID: cbfec7f5-f794b6d000001495-6c-55e8e8c5277a
Content-transfer-encoding: 8BIT
Subject: Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver
 to bridge dir
To: Yakir Yang <ykk@rock-chips.com>, Heiko Stuebner <heiko@sntech.de>,
        Thierry Reding <treding@nvidia.com>, Jingoo Han <jingoohan1@gmail.com>,
        Inki Dae <inki.dae@samsung.com>, joe@perches.com,
        Kukjin Kim <kgene@kernel.org>, Mark Yao <mark.yao@rock-chips.com>
References: <1441086371-24838-1-git-send-email-ykk@rock-chips.com>
 <1441086598-24995-1-git-send-email-ykk@rock-chips.com>
 <55E79B41.6010301@samsung.com> <55E7DAE1.6070308@rock-chips.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>, djkurtz@chromium.com,
        dianders@chromium.com, seanpaul@chromium.com, ajaynumb@gmail.com,
        Andrzej Hajda <a.hajda@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        David Airlie <airlied@linux.ie>,
        Gustavo Padovan <gustavo.padovan@collabora.co.uk>,
        Andy Yan <andy.yan@rock-chips.com>, Kumar Gala <galak@codeaurora.org>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        Kishon Vijay Abraham I <kishon@ti.com>, architt@codeaurora.org,
        robherring2@gmail.com, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org
From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Message-id: <55E8E8BE.8010300@samsung.com>
Date: Fri, 04 Sep 2015 09:41:34 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.2.0
In-reply-to: <55E7DAE1.6070308@rock-chips.com>
X-Brightmail-Tracker: H4sIAAAAAAAAA02SbUhTYRiGec85e8+ZNTlNZy8paANJpOZHXy8REkR0fgRKNYqI6qQHlTmV
	LSX7USuNbKZbKakz7FOsaVkaaKZLbSp9ODVrlpITR2w1y1DTvtSaI+zf9Tz3zXPfPx6GlBaI
	VjGp6ccETTqfJod+1Iv5bvu6LrdbGV0WhIfu2US40PaMwPfHr5PY0PsO4DP5n0X4qvWv0O7O
	gzh/1kDg198mIDaMXxfhyZqzNF4Y84iw7dMdgC85jBS+PV1O4wqHk8Iedww2OD0k7vtwAeKe
	M+M0rnfaRXig+QrEk6MLJC7rtRC4qaSNwMbSuxRu+PoR4rOtVhrPzsxA3H/TQePhX/7Ypium
	t4VxtZW1gMvLvQA5V7OH4AaKCgluzjVIcY9M72nuTvU05OrN5yHXODMq4hwF3QTXcOsUV5j7
	BXLTZjvJzZmeUFzRQzPgGu2VZIL0gN/WJCEtNVvQRMUd8UuZnXtHZ449AMe7a2qBDuj0QA/E
	DGI3oJahj9DHQahvpG6RpWwVQO1XT3hZwq5A34tHKD1gGJINRdZXKh+uQSUlaj3w++ueBmiy
	9TTltQewh5CjzrJ4PpDNJ9DjwZ0+kwWgPFcZ7R1IthyiHzWDiy7IrkcN1begLywSlYwW016m
	2HDUVvplcS9j96PnPZ+hN1nMKpBrXmMErOm/eqaleqaletcAaQYyISsxU3s0WR2r0PJqbVZ6
	siIxQ10PfC/yrQlUdW3pACwD5MslxgqXUiris7U56g6AGFIeKJG1uZVSSRKfc0LQZBzWZKUJ
	2g4QzFDylZLy5om9UjaZPyaoBCFT0PxTCUa8SgdiLoYSb6NyI5yKXVI6on3HPskNS2cu/F6Q
	8ME4tTpoKvpkDv+2My7lBj4X3nIoac9QZPjX3gC3ynaqPLjnXMzk602KN8t04Xv3334piV8b
	G2d3rYuXSXj/H9Gj+qdt20N2p20gW62bS8MeT/QzG1WXD6qGxUrlz9IQj1wWMf7rt5zSpvAx
	kaRGy/8B6gtIbR4DAAA=
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 03.09.2015 14:30, Yakir Yang wrote:
> Hi Krzysztof,
> 
> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
>> On 01.09.2015 14:49, Yakir Yang wrote:
>>> Split the dp core driver from exynos directory to bridge
>>> directory, and rename the core driver to analogix_dp_*,
>>> leave the platform code to analogix_dp-exynos.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> Changes in v4:
>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>> propery.
>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>> "exynos_dp.c"
>>> - Take Archit suggest, create a separate folder for analogix code in
>>> bridge/
>>>
>>> Changes in v3:
>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>>    to analogix_dp-exynos platform driver, add get_modes method
>>>    to struct analogix_dp_plat_data.
>>> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>>>
>>> Changes in v2:
>>> - Take Jingoo Han suggest, remove new copyright
>>> - Fix compiled failed dut to analogix_dp_device misspell
>>>
>>>   drivers/gpu/drm/bridge/Kconfig                     |    2 +
>>>   drivers/gpu/drm/bridge/Makefile                    |    1 +
>>>   drivers/gpu/drm/bridge/analogix/Kconfig            |    4 +
>>>   drivers/gpu/drm/bridge/analogix/Makefile           |    1 +
>>>   .../analogix/analogix_dp_core.c}                   |  817
>>> ++++++-------
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  283 +++++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 1265
>>> ++++++++++++++++++++
>>>   .../analogix/analogix_dp_reg.h}                    |  258 ++--
>>>   drivers/gpu/drm/exynos/Kconfig                     |    3 +-
>>>   drivers/gpu/drm/exynos/Makefile                    |    2 +-
>>>   drivers/gpu/drm/exynos/exynos_dp.c                 |  306 +++++
>>>   drivers/gpu/drm/exynos/exynos_dp_core.h            |  282 -----
>>>   drivers/gpu/drm/exynos/exynos_dp_reg.c             | 1259
>>> -------------------
>>>   include/drm/bridge/analogix_dp.h                   |   24 +
>>>   14 files changed, 2357 insertions(+), 2150 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/Kconfig
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/Makefile
>>>   rename drivers/gpu/drm/{exynos/exynos_dp_core.c =>
>>> bridge/analogix/analogix_dp_core.c} (50%)
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>   rename drivers/gpu/drm/{exynos/exynos_dp_reg.h =>
>>> bridge/analogix/analogix_dp_reg.h} (64%)
>>>   create mode 100644 drivers/gpu/drm/exynos/exynos_dp.c
>>>   delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_core.h
>>>   delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_reg.c
>>>   create mode 100644 include/drm/bridge/analogix_dp.h
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>> b/drivers/gpu/drm/bridge/Kconfig
>>> index 2de52a5..7b5b77a 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -29,4 +29,6 @@ config DRM_PARADE_PS8622
>>>       ---help---
>>>         Parade eDP-LVDS bridge chip driver.
>>>   +source "drivers/gpu/drm/bridge/analogix/Kconfig"
>>> +
>>>   endmenu
>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index e2eef1c..5366c6b 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -3,3 +3,4 @@ ccflags-y := -Iinclude/drm
>>>   obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
>>>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
>>> b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> new file mode 100644
>>> index 0000000..5ff6551
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> @@ -0,0 +1,4 @@
>>> +config DRM_ANALOGIX_DP
>>> +    tristate
>>> +    depends on DRM
>>> +    select DRM_KMS_HELPER
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
>>> b/drivers/gpu/drm/bridge/analogix/Makefile
>>> new file mode 100644
>>> index 0000000..9107b86
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp_core.o analogix_dp_reg.o
>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> similarity index 50%
>>> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
>>> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index bed0252..7d62f22 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -1,14 +1,14 @@
>>>   /*
>>> - * Samsung SoC DP (Display Port) interface driver.
>>> - *
>>> - * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> - * Author: Jingoo Han <jg1.han@samsung.com>
>>> - *
>>> - * This program is free software; you can redistribute it and/or
>>> modify it
>>> - * under the terms of the GNU General Public License as published by
>>> the
>>> - * Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> - * option) any later version.
>>> - */
>>> +* Analogix DP (Display Port) core interface driver.
>>> +*
>>> +* Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> +* Author: Jingoo Han <jg1.han@samsung.com>
>>> +*
>>> +* This program is free software; you can redistribute it and/or
>>> modify it
>>> +* under the terms of the GNU General Public License as published by the
>>> +* Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> +* option) any later version.
>>> +*/
>>>     #include <linux/module.h>
>>>   #include <linux/platform_device.h>
>>> @@ -18,12 +18,9 @@
>>>   #include <linux/interrupt.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_gpio.h>
>>> -#include <linux/of_graph.h>
>>>   #include <linux/gpio.h>
>>>   #include <linux/component.h>
>>>   #include <linux/phy/phy.h>
>>> -#include <video/of_display_timing.h>
>>> -#include <video/of_videomode.h>
>>>     #include <drm/drmP.h>
>>>   #include <drm/drm_crtc.h>
>>> @@ -31,52 +28,42 @@
>>>   #include <drm/drm_atomic_helper.h>
>>>   #include <drm/drm_panel.h>
>>>   -#include "exynos_dp_core.h"
>>> -#include "exynos_drm_crtc.h"
>>> +#include <drm/bridge/analogix_dp.h>
>>>   -#define ctx_from_connector(c)    container_of(c, struct
>>> exynos_dp_device, \
>>> -                    connector)
>>> +#include "analogix_dp_core.h"
>>>   -static inline struct exynos_drm_crtc *dp_to_crtc(struct
>>> exynos_dp_device *dp)
>>> -{
>>> -    return to_exynos_crtc(dp->encoder.crtc);
>>> -}
>>> -
>>> -static inline struct exynos_dp_device *encoder_to_dp(
>>> -                        struct drm_encoder *e)
>>> -{
>>> -    return container_of(e, struct exynos_dp_device, encoder);
>>> -}
>>> +#define connector_to_dp(c) \
>>> +    container_of(c, struct analogix_dp_device, connector)
>>>     struct bridge_init {
>>>       struct i2c_client *client;
>>>       struct device_node *node;
>>>   };
>>>   -static void exynos_dp_init_dp(struct exynos_dp_device *dp)
>>> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>>>   {
>>> -    exynos_dp_reset(dp);
>>> +    analogix_dp_reset(dp);
>>>   -    exynos_dp_swreset(dp);
>>> +    analogix_dp_swreset(dp);
>>>   -    exynos_dp_init_analog_param(dp);
>>> -    exynos_dp_init_interrupt(dp);
>>> +    analogix_dp_init_analog_param(dp);
>>> +    analogix_dp_init_interrupt(dp);
>>>         /* SW defined function Normal operation */
>>> -    exynos_dp_enable_sw_function(dp);
>>> +    analogix_dp_enable_sw_function(dp);
>>>   -    exynos_dp_config_interrupt(dp);
>>> -    exynos_dp_init_analog_func(dp);
>>> +    analogix_dp_config_interrupt(dp);
>>> +    analogix_dp_init_analog_func(dp);
>>>   -    exynos_dp_init_hpd(dp);
>>> -    exynos_dp_init_aux(dp);
>>> +    analogix_dp_init_hpd(dp);
>>> +    analogix_dp_init_aux(dp);
>>>   }
>>>   -static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
>>> +static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>>   {
>>>       int timeout_loop = 0;
>>>   -    while (exynos_dp_get_plug_in_status(dp) != 0) {
>>> +    while (analogix_dp_get_plug_in_status(dp) != 0) {
>>>           timeout_loop++;
>>>           if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>>>               dev_err(dp->dev, "failed to get hpd plug status\n");
>>> @@ -88,7 +75,7 @@ static int exynos_dp_detect_hpd(struct
>>> exynos_dp_device *dp)
>>>       return 0;
>>>   }
>>>   -static unsigned char exynos_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>> +static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>>   {
>>>       int i;
>>>       unsigned char sum = 0;
>>> @@ -99,7 +86,7 @@ static unsigned char
>>> exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
>>>       return sum;
>>>   }
>>>   -static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>>> +static int analogix_dp_read_edid(struct analogix_dp_device *dp)
>>>   {
>>>       unsigned char edid[EDID_BLOCK_LENGTH * 2];
>>>       unsigned int extend_block = 0;
>>> @@ -114,9 +101,9 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>>        */
>>>         /* Read Extension Flag, Number of 128-byte EDID extension
>>> blocks */
>>> -    retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> -                          EDID_EXTENSION_FLAG,
>>> -                          &extend_block);
>>> +    retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> +                        EDID_EXTENSION_FLAG,
>>> +                        &extend_block);
>> Hi,
>>
>> What was the point of patch 1 (checkpatch -f --subjective) if you
>> have to re-align the parameters again? It's meaningless.
>>
>> I could understand that idea if this patch touched only first
>> line - name of the function. This would result in small
>> and effective diff, like:
>>
>> -    retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> +    retval = SAME_LEGNTH____byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>                             EDID_EXTENSION_FLAG,
>>
>> But this is not the case. The reviewer have to look at 6 lines
>> of changes instead of two. You fixed the alignment just for two
>> patches and then you re-written it. It's meaningless.
> 
> Yes, this is the place that crazy me too. After rename "exynos"
> to "analogix", there would bring some "over-80-char" problem :(

If you can't avoid it then it's okay. When you rename the function this
could happen and then you just re-align the block.

> 
>> Nope. Please drop all changes from patch 1 which:
>> (all conditions apply):
>> 1. change alignment,
>> 2. are immediately changed in next patches (e.g. like here),
>>
>> and fix the alignment while renaming the function.
>>
>> This is actually another point for not accepting commits where
>> the reason is "checkpatch told me".
> 
> Okay, so there would be two patches to fix the alignment, done.
> 
> Thanks for your careful remind.

Two patches but fixing alignment in different places. Do not fix the
alignment in 1/16 and then re-align the same line at 3/16.


> 
>>
>>
>>>       if (retval)
>>>           return retval;
>>>   @@ -124,7 +111,7 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>>           dev_dbg(dp->dev, "EDID data includes a single extension!\n");
>>>             /* Read EDID data */
>>> -        retval = exynos_dp_read_bytes_from_i2c(
>>> +        retval = analogix_dp_read_bytes_from_i2c(
>>>                       dp, I2C_EDID_DEVICE_ADDR,
>>>                       EDID_HEADER_PATTERN,
>>>                       EDID_BLOCK_LENGTH,
>>> @@ -133,14 +120,14 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>> (...)
>>
>>> @@ -871,218 +862,204 @@ static irqreturn_t exynos_dp_irq_handler(int
>>> irq, void *arg)
>>>       return IRQ_HANDLED;
>>>   }
>>>   -static void exynos_dp_hotplug(struct work_struct *work)
>>> +static void analogix_dp_hotplug(struct work_struct *work)
>>>   {
>>> -    struct exynos_dp_device *dp;
>>> +    struct analogix_dp_device *dp;
>>>   -    dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>> +    dp = container_of(work, struct analogix_dp_device, hotplug_work);
>>>         if (dp->drm_dev)
>>>           drm_helper_hpd_irq_event(dp->drm_dev);
>>>   }
>>>   -static void exynos_dp_commit(struct drm_encoder *encoder)
>>> +static void analogix_dp_commit(struct analogix_dp_device *dp)
>>>   {
>>> -    struct exynos_dp_device *dp = encoder_to_dp(encoder);
>>>       int ret;
>>>         /* Keep the panel disabled while we configure video */
>>> -    if (dp->panel) {
>>> -        if (drm_panel_disable(dp->panel))
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        if (drm_panel_disable(dp->plat_data->panel))
>>>               DRM_ERROR("failed to disable the panel\n");
>>>       }
>>>   -    ret = exynos_dp_detect_hpd(dp);
>>> +    ret = analogix_dp_detect_hpd(dp);
>>>       if (ret) {
>>>           /* Cable has been disconnected, we're done */
>>>           return;
>>>       }
>>>   -    ret = exynos_dp_handle_edid(dp);
>>> +    ret = analogix_dp_handle_edid(dp);
>>>       if (ret) {
>>>           dev_err(dp->dev, "unable to handle edid\n");
>>>           return;
>>>       }
>>>   -    ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
>>> -                       dp->video_info->link_rate);
>>> +    ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
>>> +                     dp->video_info->link_rate);
>>>       if (ret) {
>>>           dev_err(dp->dev, "unable to do link train\n");
>>>           return;
>>>       }
>>>   -    exynos_dp_enable_scramble(dp, 1);
>>> -    exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> -    exynos_dp_enable_enhanced_mode(dp, 1);
>>> +    analogix_dp_enable_scramble(dp, 1);
>>> +    analogix_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> +    analogix_dp_enable_enhanced_mode(dp, 1);
>>>   -    exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> -    exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>> +    analogix_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> +    analogix_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>>   -    exynos_dp_init_video(dp);
>>> -    ret = exynos_dp_config_video(dp);
>>> +    analogix_dp_init_video(dp);
>>> +    ret = analogix_dp_config_video(dp);
>>>       if (ret)
>>>           dev_err(dp->dev, "unable to config video\n");
>>>         /* Safe to enable the panel now */
>>> -    if (dp->panel) {
>>> -        if (drm_panel_enable(dp->panel))
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        if (drm_panel_enable(dp->plat_data->panel))
>>>               DRM_ERROR("failed to enable the panel\n");
>>>       }
>>>         /* Enable video */
>>> -    exynos_dp_start_video(dp);
>>> +    analogix_dp_start_video(dp);
>>>   }
>>>     static enum drm_connector_status
>>> -exynos_dp_detect(struct drm_connector *connector, bool force)
>>> +analogix_dp_detect(struct drm_connector *connector, bool force)
>>>   {
>>>       return connector_status_connected;
>>>   }
>>>   -static void exynos_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> +static void analogix_dp_connector_destroy(struct drm_connector
>>> *connector)
>>>   {
>>>       drm_connector_unregister(connector);
>>>       drm_connector_cleanup(connector);
>>>   }
>>>   -static struct drm_connector_funcs exynos_dp_connector_funcs = {
>>> +static struct drm_connector_funcs analogix_dp_connector_funcs = {
>>>       .dpms = drm_atomic_helper_connector_dpms,
>>>       .fill_modes = drm_helper_probe_single_connector_modes,
>>> -    .detect = exynos_dp_detect,
>>> -    .destroy = exynos_dp_connector_destroy,
>>> +    .detect = analogix_dp_detect,
>>> +    .destroy = analogix_dp_connector_destroy,
>>>       .reset = drm_atomic_helper_connector_reset,
>>>       .atomic_duplicate_state =
>>> drm_atomic_helper_connector_duplicate_state,
>>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>   };
>>>   -static int exynos_dp_get_modes(struct drm_connector *connector)
>>> +static int analogix_dp_get_modes(struct drm_connector *connector)
>>>   {
>>> -    struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> -    struct drm_display_mode *mode;
>>> -
>>> -    if (dp->panel)
>>> -        return drm_panel_get_modes(dp->panel);
>>> +    struct analogix_dp_device *dp = connector_to_dp(connector);
>>> +    struct analogix_dp_plat_data *plat_data = dp->plat_data;
>>> +    int num_modes = 0;
>>>   -    mode = drm_mode_create(connector->dev);
>>> -    if (!mode) {
>>> -        DRM_ERROR("failed to create a new display mode.\n");
>>> -        return 0;
>>> -    }
>>> -
>>> -    drm_display_mode_from_videomode(&dp->priv.vm, mode);
>>> -    mode->width_mm = dp->priv.width_mm;
>>> -    mode->height_mm = dp->priv.height_mm;
>>> -    connector->display_info.width_mm = mode->width_mm;
>>> -    connector->display_info.height_mm = mode->height_mm;
>>> +    if (plat_data && plat_data->panel)
>>> +        num_modes += drm_panel_get_modes(plat_data->panel);
>>>   -    mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>> -    drm_mode_set_name(mode);
>>> -    drm_mode_probed_add(connector, mode);
>>> +    if (plat_data && plat_data->get_modes)
>>> +         num_modes += plat_data->get_modes(plat_data, connector);
>>>   -    return 1;
>>> +    return num_modes;
>>>   }
>>>     static struct drm_encoder *
>>> -exynos_dp_best_encoder(struct drm_connector *connector)
>>> +analogix_dp_best_encoder(struct drm_connector *connector)
>>>   {
>>> -    struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> +    struct analogix_dp_device *dp = connector_to_dp(connector);
>>>   -    return &dp->encoder;
>>> +    return dp->encoder;
>>>   }
>>>   -static struct drm_connector_helper_funcs
>>> exynos_dp_connector_helper_funcs = {
>>> -    .get_modes = exynos_dp_get_modes,
>>> -    .best_encoder = exynos_dp_best_encoder,
>>> +static struct drm_connector_helper_funcs
>>> analogix_dp_connector_helper_funcs = {
>>> +    .get_modes = analogix_dp_get_modes,
>>> +    .best_encoder = analogix_dp_best_encoder,
>>>   };
>>>   -/* returns the number of bridges attached */
>>> -static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>>> -                    struct drm_encoder *encoder)
>>> -{
>>> -    int ret;
>>> -
>>> -    encoder->bridge->next = dp->ptn_bridge;
>>> -    dp->ptn_bridge->encoder = encoder;
>>> -    ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
>>> -    if (ret) {
>>> -        DRM_ERROR("Failed to attach bridge to drm\n");
>>> -        return ret;
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>>   {
>>> -    struct exynos_dp_device *dp = bridge->driver_private;
>>> -    struct drm_encoder *encoder = &dp->encoder;
>>> +    struct analogix_dp_device *dp = bridge->driver_private;
>>> +    struct drm_encoder *encoder = dp->encoder;
>>>       struct drm_connector *connector = &dp->connector;
>>>       int ret;
>>>   -    /* Pre-empt DP connector creation if there's a bridge */
>>> -    if (dp->ptn_bridge) {
>>> -        ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>> -        if (!ret)
>>> -            return 0;
>>> +    if (!bridge->encoder) {
>>> +        DRM_ERROR("Parent encoder object not found");
>>> +        return -ENODEV;
>>>       }
>>>   +    encoder->bridge = bridge;
>>> +
>>>       connector->polled = DRM_CONNECTOR_POLL_HPD;
>>>         ret = drm_connector_init(dp->drm_dev, connector,
>>> -                 &exynos_dp_connector_funcs,
>>> +                 &analogix_dp_connector_funcs,
>>>                    DRM_MODE_CONNECTOR_eDP);
>>>       if (ret) {
>>>           DRM_ERROR("Failed to initialize connector with drm\n");
>>>           return ret;
>>>       }
>>>   -    drm_connector_helper_add(connector,
>>> &exynos_dp_connector_helper_funcs);
>>> +    drm_connector_helper_add(connector,
>>> +                 &analogix_dp_connector_helper_funcs);
>>>       drm_connector_register(connector);
>>>       drm_mode_connector_attach_encoder(connector, encoder);
>>>   -    if (dp->panel)
>>> -        ret = drm_panel_attach(dp->panel, &dp->connector);
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>> +        if (ret) {
>>> +            DRM_ERROR("Failed to attach panel\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    /*
>>> +     * This should be the end of attach function, caused
>>> +     * we should ensure dp bridge could attach first.
>>> +     */
>>> +     if (dp->plat_data && dp->plat_data->attach) {
>>> +         ret = dp->plat_data->attach(dp->plat_data, bridge);
>>> +         if (ret) {
>>> +             DRM_ERROR("Failed at platform attch func\n");
>> Two new error paths appeared here and above. Don't you have to
>> cleanup something? I don't know, just wondering...
> 
> Hmm... I think both panel & platform_attch need ERROR remind when
> it failed. But if it still need clean, I though it is tge platform attch
> error,
> this is not relate to DRM directly, just analogix driver logic, so code
> would
> like.
> 
> -    if (dp->panel)
> -        ret = drm_panel_attach(dp->panel, &dp->connector);
> +    if (dp->plat_data && dp->plat_data->panel) {
> +        ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
> +        if (ret) {
> +            DRM_ERROR("Failed to attach panel\n");
> +            return ret;
> +        }
> +    }
> 
> +    /*
> +     * This should be the end of attach function, caused
> +     * we should ensure dp bridge could attach first.
> +     */
> +     if (dp->plat_data && dp->plat_data->attach) {
> +         ret = dp->plat_data->attach(dp->plat_data, bridge);
> 
>           return ret;

I am lost... the code looks the same. What did you change?

Best regards,
Krzysztof

From mboxrd@z Thu Jan  1 00:00:00 1970
From: k.kozlowski@samsung.com (Krzysztof Kozlowski)
Date: Fri, 04 Sep 2015 09:41:34 +0900
Subject: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver
 to bridge dir
In-Reply-To: <55E7DAE1.6070308@rock-chips.com>
References: <1441086371-24838-1-git-send-email-ykk@rock-chips.com>
 <1441086598-24995-1-git-send-email-ykk@rock-chips.com>
 <55E79B41.6010301@samsung.com> <55E7DAE1.6070308@rock-chips.com>
Message-ID: <55E8E8BE.8010300@samsung.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

On 03.09.2015 14:30, Yakir Yang wrote:
> Hi Krzysztof,
> 
> ? 09/03/2015 08:58 AM, Krzysztof Kozlowski ??:
>> On 01.09.2015 14:49, Yakir Yang wrote:
>>> Split the dp core driver from exynos directory to bridge
>>> directory, and rename the core driver to analogix_dp_*,
>>> leave the platform code to analogix_dp-exynos.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> Changes in v4:
>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>> propery.
>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>> "exynos_dp.c"
>>> - Take Archit suggest, create a separate folder for analogix code in
>>> bridge/
>>>
>>> Changes in v3:
>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>>    to analogix_dp-exynos platform driver, add get_modes method
>>>    to struct analogix_dp_plat_data.
>>> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>>>
>>> Changes in v2:
>>> - Take Jingoo Han suggest, remove new copyright
>>> - Fix compiled failed dut to analogix_dp_device misspell
>>>
>>>   drivers/gpu/drm/bridge/Kconfig                     |    2 +
>>>   drivers/gpu/drm/bridge/Makefile                    |    1 +
>>>   drivers/gpu/drm/bridge/analogix/Kconfig            |    4 +
>>>   drivers/gpu/drm/bridge/analogix/Makefile           |    1 +
>>>   .../analogix/analogix_dp_core.c}                   |  817
>>> ++++++-------
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  283 +++++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 1265
>>> ++++++++++++++++++++
>>>   .../analogix/analogix_dp_reg.h}                    |  258 ++--
>>>   drivers/gpu/drm/exynos/Kconfig                     |    3 +-
>>>   drivers/gpu/drm/exynos/Makefile                    |    2 +-
>>>   drivers/gpu/drm/exynos/exynos_dp.c                 |  306 +++++
>>>   drivers/gpu/drm/exynos/exynos_dp_core.h            |  282 -----
>>>   drivers/gpu/drm/exynos/exynos_dp_reg.c             | 1259
>>> -------------------
>>>   include/drm/bridge/analogix_dp.h                   |   24 +
>>>   14 files changed, 2357 insertions(+), 2150 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/Kconfig
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/Makefile
>>>   rename drivers/gpu/drm/{exynos/exynos_dp_core.c =>
>>> bridge/analogix/analogix_dp_core.c} (50%)
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>   create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>   rename drivers/gpu/drm/{exynos/exynos_dp_reg.h =>
>>> bridge/analogix/analogix_dp_reg.h} (64%)
>>>   create mode 100644 drivers/gpu/drm/exynos/exynos_dp.c
>>>   delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_core.h
>>>   delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_reg.c
>>>   create mode 100644 include/drm/bridge/analogix_dp.h
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>> b/drivers/gpu/drm/bridge/Kconfig
>>> index 2de52a5..7b5b77a 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -29,4 +29,6 @@ config DRM_PARADE_PS8622
>>>       ---help---
>>>         Parade eDP-LVDS bridge chip driver.
>>>   +source "drivers/gpu/drm/bridge/analogix/Kconfig"
>>> +
>>>   endmenu
>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index e2eef1c..5366c6b 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -3,3 +3,4 @@ ccflags-y := -Iinclude/drm
>>>   obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
>>>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
>>> b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> new file mode 100644
>>> index 0000000..5ff6551
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> @@ -0,0 +1,4 @@
>>> +config DRM_ANALOGIX_DP
>>> +    tristate
>>> +    depends on DRM
>>> +    select DRM_KMS_HELPER
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
>>> b/drivers/gpu/drm/bridge/analogix/Makefile
>>> new file mode 100644
>>> index 0000000..9107b86
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp_core.o analogix_dp_reg.o
>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> similarity index 50%
>>> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
>>> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index bed0252..7d62f22 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -1,14 +1,14 @@
>>>   /*
>>> - * Samsung SoC DP (Display Port) interface driver.
>>> - *
>>> - * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> - * Author: Jingoo Han <jg1.han@samsung.com>
>>> - *
>>> - * This program is free software; you can redistribute it and/or
>>> modify it
>>> - * under the terms of the GNU General Public License as published by
>>> the
>>> - * Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> - * option) any later version.
>>> - */
>>> +* Analogix DP (Display Port) core interface driver.
>>> +*
>>> +* Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> +* Author: Jingoo Han <jg1.han@samsung.com>
>>> +*
>>> +* This program is free software; you can redistribute it and/or
>>> modify it
>>> +* under the terms of the GNU General Public License as published by the
>>> +* Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> +* option) any later version.
>>> +*/
>>>     #include <linux/module.h>
>>>   #include <linux/platform_device.h>
>>> @@ -18,12 +18,9 @@
>>>   #include <linux/interrupt.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_gpio.h>
>>> -#include <linux/of_graph.h>
>>>   #include <linux/gpio.h>
>>>   #include <linux/component.h>
>>>   #include <linux/phy/phy.h>
>>> -#include <video/of_display_timing.h>
>>> -#include <video/of_videomode.h>
>>>     #include <drm/drmP.h>
>>>   #include <drm/drm_crtc.h>
>>> @@ -31,52 +28,42 @@
>>>   #include <drm/drm_atomic_helper.h>
>>>   #include <drm/drm_panel.h>
>>>   -#include "exynos_dp_core.h"
>>> -#include "exynos_drm_crtc.h"
>>> +#include <drm/bridge/analogix_dp.h>
>>>   -#define ctx_from_connector(c)    container_of(c, struct
>>> exynos_dp_device, \
>>> -                    connector)
>>> +#include "analogix_dp_core.h"
>>>   -static inline struct exynos_drm_crtc *dp_to_crtc(struct
>>> exynos_dp_device *dp)
>>> -{
>>> -    return to_exynos_crtc(dp->encoder.crtc);
>>> -}
>>> -
>>> -static inline struct exynos_dp_device *encoder_to_dp(
>>> -                        struct drm_encoder *e)
>>> -{
>>> -    return container_of(e, struct exynos_dp_device, encoder);
>>> -}
>>> +#define connector_to_dp(c) \
>>> +    container_of(c, struct analogix_dp_device, connector)
>>>     struct bridge_init {
>>>       struct i2c_client *client;
>>>       struct device_node *node;
>>>   };
>>>   -static void exynos_dp_init_dp(struct exynos_dp_device *dp)
>>> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>>>   {
>>> -    exynos_dp_reset(dp);
>>> +    analogix_dp_reset(dp);
>>>   -    exynos_dp_swreset(dp);
>>> +    analogix_dp_swreset(dp);
>>>   -    exynos_dp_init_analog_param(dp);
>>> -    exynos_dp_init_interrupt(dp);
>>> +    analogix_dp_init_analog_param(dp);
>>> +    analogix_dp_init_interrupt(dp);
>>>         /* SW defined function Normal operation */
>>> -    exynos_dp_enable_sw_function(dp);
>>> +    analogix_dp_enable_sw_function(dp);
>>>   -    exynos_dp_config_interrupt(dp);
>>> -    exynos_dp_init_analog_func(dp);
>>> +    analogix_dp_config_interrupt(dp);
>>> +    analogix_dp_init_analog_func(dp);
>>>   -    exynos_dp_init_hpd(dp);
>>> -    exynos_dp_init_aux(dp);
>>> +    analogix_dp_init_hpd(dp);
>>> +    analogix_dp_init_aux(dp);
>>>   }
>>>   -static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
>>> +static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>>   {
>>>       int timeout_loop = 0;
>>>   -    while (exynos_dp_get_plug_in_status(dp) != 0) {
>>> +    while (analogix_dp_get_plug_in_status(dp) != 0) {
>>>           timeout_loop++;
>>>           if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>>>               dev_err(dp->dev, "failed to get hpd plug status\n");
>>> @@ -88,7 +75,7 @@ static int exynos_dp_detect_hpd(struct
>>> exynos_dp_device *dp)
>>>       return 0;
>>>   }
>>>   -static unsigned char exynos_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>> +static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>>   {
>>>       int i;
>>>       unsigned char sum = 0;
>>> @@ -99,7 +86,7 @@ static unsigned char
>>> exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
>>>       return sum;
>>>   }
>>>   -static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>>> +static int analogix_dp_read_edid(struct analogix_dp_device *dp)
>>>   {
>>>       unsigned char edid[EDID_BLOCK_LENGTH * 2];
>>>       unsigned int extend_block = 0;
>>> @@ -114,9 +101,9 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>>        */
>>>         /* Read Extension Flag, Number of 128-byte EDID extension
>>> blocks */
>>> -    retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> -                          EDID_EXTENSION_FLAG,
>>> -                          &extend_block);
>>> +    retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> +                        EDID_EXTENSION_FLAG,
>>> +                        &extend_block);
>> Hi,
>>
>> What was the point of patch 1 (checkpatch -f --subjective) if you
>> have to re-align the parameters again? It's meaningless.
>>
>> I could understand that idea if this patch touched only first
>> line - name of the function. This would result in small
>> and effective diff, like:
>>
>> -    retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> +    retval = SAME_LEGNTH____byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>                             EDID_EXTENSION_FLAG,
>>
>> But this is not the case. The reviewer have to look at 6 lines
>> of changes instead of two. You fixed the alignment just for two
>> patches and then you re-written it. It's meaningless.
> 
> Yes, this is the place that crazy me too. After rename "exynos"
> to "analogix", there would bring some "over-80-char" problem :(

If you can't avoid it then it's okay. When you rename the function this
could happen and then you just re-align the block.

> 
>> Nope. Please drop all changes from patch 1 which:
>> (all conditions apply):
>> 1. change alignment,
>> 2. are immediately changed in next patches (e.g. like here),
>>
>> and fix the alignment while renaming the function.
>>
>> This is actually another point for not accepting commits where
>> the reason is "checkpatch told me".
> 
> Okay, so there would be two patches to fix the alignment, done.
> 
> Thanks for your careful remind.

Two patches but fixing alignment in different places. Do not fix the
alignment in 1/16 and then re-align the same line at 3/16.


> 
>>
>>
>>>       if (retval)
>>>           return retval;
>>>   @@ -124,7 +111,7 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>>           dev_dbg(dp->dev, "EDID data includes a single extension!\n");
>>>             /* Read EDID data */
>>> -        retval = exynos_dp_read_bytes_from_i2c(
>>> +        retval = analogix_dp_read_bytes_from_i2c(
>>>                       dp, I2C_EDID_DEVICE_ADDR,
>>>                       EDID_HEADER_PATTERN,
>>>                       EDID_BLOCK_LENGTH,
>>> @@ -133,14 +120,14 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>> (...)
>>
>>> @@ -871,218 +862,204 @@ static irqreturn_t exynos_dp_irq_handler(int
>>> irq, void *arg)
>>>       return IRQ_HANDLED;
>>>   }
>>>   -static void exynos_dp_hotplug(struct work_struct *work)
>>> +static void analogix_dp_hotplug(struct work_struct *work)
>>>   {
>>> -    struct exynos_dp_device *dp;
>>> +    struct analogix_dp_device *dp;
>>>   -    dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>> +    dp = container_of(work, struct analogix_dp_device, hotplug_work);
>>>         if (dp->drm_dev)
>>>           drm_helper_hpd_irq_event(dp->drm_dev);
>>>   }
>>>   -static void exynos_dp_commit(struct drm_encoder *encoder)
>>> +static void analogix_dp_commit(struct analogix_dp_device *dp)
>>>   {
>>> -    struct exynos_dp_device *dp = encoder_to_dp(encoder);
>>>       int ret;
>>>         /* Keep the panel disabled while we configure video */
>>> -    if (dp->panel) {
>>> -        if (drm_panel_disable(dp->panel))
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        if (drm_panel_disable(dp->plat_data->panel))
>>>               DRM_ERROR("failed to disable the panel\n");
>>>       }
>>>   -    ret = exynos_dp_detect_hpd(dp);
>>> +    ret = analogix_dp_detect_hpd(dp);
>>>       if (ret) {
>>>           /* Cable has been disconnected, we're done */
>>>           return;
>>>       }
>>>   -    ret = exynos_dp_handle_edid(dp);
>>> +    ret = analogix_dp_handle_edid(dp);
>>>       if (ret) {
>>>           dev_err(dp->dev, "unable to handle edid\n");
>>>           return;
>>>       }
>>>   -    ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
>>> -                       dp->video_info->link_rate);
>>> +    ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
>>> +                     dp->video_info->link_rate);
>>>       if (ret) {
>>>           dev_err(dp->dev, "unable to do link train\n");
>>>           return;
>>>       }
>>>   -    exynos_dp_enable_scramble(dp, 1);
>>> -    exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> -    exynos_dp_enable_enhanced_mode(dp, 1);
>>> +    analogix_dp_enable_scramble(dp, 1);
>>> +    analogix_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> +    analogix_dp_enable_enhanced_mode(dp, 1);
>>>   -    exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> -    exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>> +    analogix_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> +    analogix_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>>   -    exynos_dp_init_video(dp);
>>> -    ret = exynos_dp_config_video(dp);
>>> +    analogix_dp_init_video(dp);
>>> +    ret = analogix_dp_config_video(dp);
>>>       if (ret)
>>>           dev_err(dp->dev, "unable to config video\n");
>>>         /* Safe to enable the panel now */
>>> -    if (dp->panel) {
>>> -        if (drm_panel_enable(dp->panel))
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        if (drm_panel_enable(dp->plat_data->panel))
>>>               DRM_ERROR("failed to enable the panel\n");
>>>       }
>>>         /* Enable video */
>>> -    exynos_dp_start_video(dp);
>>> +    analogix_dp_start_video(dp);
>>>   }
>>>     static enum drm_connector_status
>>> -exynos_dp_detect(struct drm_connector *connector, bool force)
>>> +analogix_dp_detect(struct drm_connector *connector, bool force)
>>>   {
>>>       return connector_status_connected;
>>>   }
>>>   -static void exynos_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> +static void analogix_dp_connector_destroy(struct drm_connector
>>> *connector)
>>>   {
>>>       drm_connector_unregister(connector);
>>>       drm_connector_cleanup(connector);
>>>   }
>>>   -static struct drm_connector_funcs exynos_dp_connector_funcs = {
>>> +static struct drm_connector_funcs analogix_dp_connector_funcs = {
>>>       .dpms = drm_atomic_helper_connector_dpms,
>>>       .fill_modes = drm_helper_probe_single_connector_modes,
>>> -    .detect = exynos_dp_detect,
>>> -    .destroy = exynos_dp_connector_destroy,
>>> +    .detect = analogix_dp_detect,
>>> +    .destroy = analogix_dp_connector_destroy,
>>>       .reset = drm_atomic_helper_connector_reset,
>>>       .atomic_duplicate_state =
>>> drm_atomic_helper_connector_duplicate_state,
>>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>   };
>>>   -static int exynos_dp_get_modes(struct drm_connector *connector)
>>> +static int analogix_dp_get_modes(struct drm_connector *connector)
>>>   {
>>> -    struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> -    struct drm_display_mode *mode;
>>> -
>>> -    if (dp->panel)
>>> -        return drm_panel_get_modes(dp->panel);
>>> +    struct analogix_dp_device *dp = connector_to_dp(connector);
>>> +    struct analogix_dp_plat_data *plat_data = dp->plat_data;
>>> +    int num_modes = 0;
>>>   -    mode = drm_mode_create(connector->dev);
>>> -    if (!mode) {
>>> -        DRM_ERROR("failed to create a new display mode.\n");
>>> -        return 0;
>>> -    }
>>> -
>>> -    drm_display_mode_from_videomode(&dp->priv.vm, mode);
>>> -    mode->width_mm = dp->priv.width_mm;
>>> -    mode->height_mm = dp->priv.height_mm;
>>> -    connector->display_info.width_mm = mode->width_mm;
>>> -    connector->display_info.height_mm = mode->height_mm;
>>> +    if (plat_data && plat_data->panel)
>>> +        num_modes += drm_panel_get_modes(plat_data->panel);
>>>   -    mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>> -    drm_mode_set_name(mode);
>>> -    drm_mode_probed_add(connector, mode);
>>> +    if (plat_data && plat_data->get_modes)
>>> +         num_modes += plat_data->get_modes(plat_data, connector);
>>>   -    return 1;
>>> +    return num_modes;
>>>   }
>>>     static struct drm_encoder *
>>> -exynos_dp_best_encoder(struct drm_connector *connector)
>>> +analogix_dp_best_encoder(struct drm_connector *connector)
>>>   {
>>> -    struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> +    struct analogix_dp_device *dp = connector_to_dp(connector);
>>>   -    return &dp->encoder;
>>> +    return dp->encoder;
>>>   }
>>>   -static struct drm_connector_helper_funcs
>>> exynos_dp_connector_helper_funcs = {
>>> -    .get_modes = exynos_dp_get_modes,
>>> -    .best_encoder = exynos_dp_best_encoder,
>>> +static struct drm_connector_helper_funcs
>>> analogix_dp_connector_helper_funcs = {
>>> +    .get_modes = analogix_dp_get_modes,
>>> +    .best_encoder = analogix_dp_best_encoder,
>>>   };
>>>   -/* returns the number of bridges attached */
>>> -static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>>> -                    struct drm_encoder *encoder)
>>> -{
>>> -    int ret;
>>> -
>>> -    encoder->bridge->next = dp->ptn_bridge;
>>> -    dp->ptn_bridge->encoder = encoder;
>>> -    ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
>>> -    if (ret) {
>>> -        DRM_ERROR("Failed to attach bridge to drm\n");
>>> -        return ret;
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>>   {
>>> -    struct exynos_dp_device *dp = bridge->driver_private;
>>> -    struct drm_encoder *encoder = &dp->encoder;
>>> +    struct analogix_dp_device *dp = bridge->driver_private;
>>> +    struct drm_encoder *encoder = dp->encoder;
>>>       struct drm_connector *connector = &dp->connector;
>>>       int ret;
>>>   -    /* Pre-empt DP connector creation if there's a bridge */
>>> -    if (dp->ptn_bridge) {
>>> -        ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>> -        if (!ret)
>>> -            return 0;
>>> +    if (!bridge->encoder) {
>>> +        DRM_ERROR("Parent encoder object not found");
>>> +        return -ENODEV;
>>>       }
>>>   +    encoder->bridge = bridge;
>>> +
>>>       connector->polled = DRM_CONNECTOR_POLL_HPD;
>>>         ret = drm_connector_init(dp->drm_dev, connector,
>>> -                 &exynos_dp_connector_funcs,
>>> +                 &analogix_dp_connector_funcs,
>>>                    DRM_MODE_CONNECTOR_eDP);
>>>       if (ret) {
>>>           DRM_ERROR("Failed to initialize connector with drm\n");
>>>           return ret;
>>>       }
>>>   -    drm_connector_helper_add(connector,
>>> &exynos_dp_connector_helper_funcs);
>>> +    drm_connector_helper_add(connector,
>>> +                 &analogix_dp_connector_helper_funcs);
>>>       drm_connector_register(connector);
>>>       drm_mode_connector_attach_encoder(connector, encoder);
>>>   -    if (dp->panel)
>>> -        ret = drm_panel_attach(dp->panel, &dp->connector);
>>> +    if (dp->plat_data && dp->plat_data->panel) {
>>> +        ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>> +        if (ret) {
>>> +            DRM_ERROR("Failed to attach panel\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    /*
>>> +     * This should be the end of attach function, caused
>>> +     * we should ensure dp bridge could attach first.
>>> +     */
>>> +     if (dp->plat_data && dp->plat_data->attach) {
>>> +         ret = dp->plat_data->attach(dp->plat_data, bridge);
>>> +         if (ret) {
>>> +             DRM_ERROR("Failed at platform attch func\n");
>> Two new error paths appeared here and above. Don't you have to
>> cleanup something? I don't know, just wondering...
> 
> Hmm... I think both panel & platform_attch need ERROR remind when
> it failed. But if it still need clean, I though it is tge platform attch
> error,
> this is not relate to DRM directly, just analogix driver logic, so code
> would
> like.
> 
> -    if (dp->panel)
> -        ret = drm_panel_attach(dp->panel, &dp->connector);
> +    if (dp->plat_data && dp->plat_data->panel) {
> +        ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
> +        if (ret) {
> +            DRM_ERROR("Failed to attach panel\n");
> +            return ret;
> +        }
> +    }
> 
> +    /*
> +     * This should be the end of attach function, caused
> +     * we should ensure dp bridge could attach first.
> +     */
> +     if (dp->plat_data && dp->plat_data->attach) {
> +         ret = dp->plat_data->attach(dp->plat_data, bridge);
> 
>           return ret;

I am lost... the code looks the same. What did you change?

Best regards,
Krzysztof