Message-ID: <55E7DAE1.6070308@rock-chips.com>
Date:	Thu, 03 Sep 2015 13:30:09 +0800
From:	Yakir Yang <ykk@...k-chips.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Thierry Reding <treding@...dia.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Inki Dae <inki.dae@...sung.com>, joe@...ches.com,
	Kukjin Kim <kgene@...nel.org>,
	Mark Yao <mark.yao@...k-chips.com>
CC:	Russell King <rmk+kernel@....linux.org.uk>, djkurtz@...omium.com,
	dianders@...omium.com, seanpaul@...omium.com, ajaynumb@...il.com,
	Andrzej Hajda <a.hajda@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	David Airlie <airlied@...ux.ie>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	Andy Yan <andy.yan@...k-chips.com>,
	Kumar Gala <galak@...eaurora.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Kishon Vijay Abraham I <kishon@...com>,
	architt@...eaurora.org, robherring2@...il.com,
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver
 to bridge dir

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@...k-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@...sung.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@...sung.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 :(

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

>
>
>>   	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;
> (...)
>
>> @@ -1345,22 +1286,9 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>>   	disable_irq(dp->irq);
>>   
>>   	dp->drm_dev = drm_dev;
>> +	dp->encoder = encoder;
>>   
>> -	pipe = exynos_drm_crtc_get_pipe_from_type(drm_dev,
>> -						  EXYNOS_DISPLAY_TYPE_LCD);
>> -	if (pipe < 0)
>> -		return pipe;
>> -
>> -	encoder->possible_crtcs = 1 << pipe;
>> -
>> -	DRM_DEBUG_KMS("possible_crtcs = 0x%x\n", encoder->possible_crtcs);
>> -
>> -	drm_encoder_init(drm_dev, encoder, &exynos_dp_encoder_funcs,
>> -			 DRM_MODE_ENCODER_TMDS);
>> -
>> -	drm_encoder_helper_add(encoder, &exynos_dp_encoder_helper_funcs);
>> -
>> -	ret = exynos_dp_create_connector(encoder);
>> +	ret = analogix_dp_create_bridge(drm_dev, dp);
>>   	if (ret) {
>>   		DRM_ERROR("failed to create connector ret = %d\n", ret);
>>   		drm_encoder_cleanup(encoder);
>> @@ -1369,103 +1297,36 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(analogix_dp_bind);
> Ooo, a new export symbol. I suppose it is necessary for the split
> but the commit message does not mention exporting
> anything nor the reason for doing it. Could you extend the commit
> message describing more tha changes here and rationale for it?

Okay, got it.

> (...)
>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> new file mode 100644
>> index 0000000..b879d8c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -0,0 +1,1265 @@
>> +/*
>> + * Analogix DP (Display port) core register interface driver.
>> + *
>> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>> + * Author: Jingoo Han <jg1.han@...sung.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/device.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio.h>
>> +
>> +#include "analogix_dp_core.h"
>> +#include "analogix_dp_reg.h"
>> +
>> +#define COMMON_INT_MASK_1	0
>> +#define COMMON_INT_MASK_2	0
>> +#define COMMON_INT_MASK_3	0
>> +#define COMMON_INT_MASK_4	(HOTPLUG_CHG | HPD_LOST | PLUG)
>> +#define INT_STA_MASK		INT_HPD
>> +
>> +void analogix_dp_enable_video_mute(struct analogix_dp_device *dp, bool enable)
>> +{
>> +	u32 reg;
>> +
>> +	if (enable) {
>> +		reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +		reg |= HDCP_VIDEO_MUTE;
>> +		writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +	} else {
>> +		reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +		reg &= ~HDCP_VIDEO_MUTE;
>> +		writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +	}
>> +}
>> +
>> +void analogix_dp_stop_video(struct analogix_dp_device *dp)
>> +{
>> +	u32 reg;
>> +
>> +	reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +	reg &= ~VIDEO_EN;
>> +	writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +}
>> +
>> +void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
>> +{
>> +	u32 reg;
>> +
>> +	if (enable)
>> +		reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
>> +			LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
>> +	else
>> +		reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
>> +			LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> Here and in few other places with new code: no alignment?
>
> You spent some time fixing these in patch 1, so I would assume
> that new code already comes after "checkpatch -f --subjective".
> That's actually the only valid reason for using checkpatch - new code.
> However my mail reader could corrupt the output so maybe this is aligned?

This is another mistaken I make, and surprise by that checkpatch
fine with this no-alignment style. Anyway I would just do rename
change in this split patch, and move the style change to another
separate patch (maybe I should adjust the order, correct style
problem after this patch).


> Code seems fine after looking at it but review was rather shallow.
> I don't know the Exynos DP driver.

I really appreciate for your careful review  ;)

- Yakir
> Best regards,
> Krzysztof
>
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists