Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir

From: Krzysztof Kozlowski
Date: Thu Sep 03 2015 - 20:42:17 EST

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@xxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxx>
>>> - *
>>> - * 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@xxxxxxxxxxx>
>>> +*
>>> +* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/