[Freedreno] [DPU PATCH 2/3] drm/msm/dp: add displayPort driver support
chandanu at codeaurora.org
chandanu at codeaurora.org
Sun Mar 10 03:52:22 UTC 2019
On 2019-02-14 04:28, chandanu at codeaurora.org wrote:
Hello Sean
I had few more queries regarding your comments. Can you please provide
your feedback? (Queries/responses are present below)
thanks
Chandan
> On 2018-10-23 09:28, Sean Paul wrote:
>> On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote:
>>> Add the needed displayPort files to enable DP driver
>>> on msm target.
>>>
>>> "dp_display" module is the main module that calls into
>>> other sub-modules. "dp_drm" file represents the interface
>>> between DRM framework and DP driver.
>>>
>>
>
> Hello Sean,
> I have responded to all of your comments and queries. I have specified
> the comments addressed
> in V2 patch series and what I have missed that will be fixed in V3.
>
>
>> Hi Chandan,
>> A few themes which I've pointed out below, but want to call out at the
>> top so
>> it's obvious.
>>
>> - Please remove all superfluous NULL checks at the beginning of
>> functions.
>> Included in this is reflowing initialization of variables and
>> checking that
>> return types still make sense (ie: if a function's only error path
>> is the NULL
>> check, make it void).
>> - Please remove all of the new callbacks and just call functions
>> directly. This
>> should also get rid of the _get/_put functions that allocate/set
>> them up
>> - Please consolidate state structs into one struct with a few
>> substructs,
>> there's too much state right now.
>> - Please #define hardcoded values
>> - Instead of sprinkling wmb() everywhere, use readl/writel instead of
>> *_relaxed
>> - There are a bunch of races that I found, please audit the code and
>> remove
>> races with locks as necessary
>> - I tried to catch as much dead code as possible, please audit the
>> rest for
>> unused structs/variables/functions/etc
>> - Use drm dp helper structs for link parameter storage
>>
>>
>>> Signed-off-by: Chandan Uddaraju <chandanu at codeaurora.org>
>>> ---
>>> drivers/gpu/drm/msm/Kconfig | 9 +
>>> drivers/gpu/drm/msm/Makefile | 15 +-
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 ++++
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 +
>>> drivers/gpu/drm/msm/dp/dp_aux.c | 570 ++++++++++
>>> drivers/gpu/drm/msm/dp/dp_aux.h | 44 +
>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 1188
>>> ++++++++++++++++++++
>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 144 +++
>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475
>>> +++++++++++++++++++++++++
>>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 +
>>> drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++++
>>> drivers/gpu/drm/msm/dp/dp_debug.h | 81 ++
>>> drivers/gpu/drm/msm/dp/dp_display.c | 977 +++++++++++++++++
>>> drivers/gpu/drm/msm/dp/dp_display.h | 55 +
>>> drivers/gpu/drm/msm/dp/dp_drm.c | 542 ++++++++++
>>> drivers/gpu/drm/msm/dp/dp_drm.h | 52 +
>>> drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++++
>>> drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++
>>> drivers/gpu/drm/msm/dp/dp_link.c | 1549
>>> +++++++++++++++++++++++++++
>>> drivers/gpu/drm/msm/dp/dp_link.h | 184 ++++
>>> drivers/gpu/drm/msm/dp/dp_panel.c | 624 +++++++++++
>>> drivers/gpu/drm/msm/dp/dp_panel.h | 121 +++
>>> drivers/gpu/drm/msm/dp/dp_parser.c | 679 ++++++++++++
>>> drivers/gpu/drm/msm/dp/dp_parser.h | 205 ++++
>>> drivers/gpu/drm/msm/dp/dp_power.c | 599 +++++++++++
>>> drivers/gpu/drm/msm/dp/dp_power.h | 57 +
>>> drivers/gpu/drm/msm/dp/dp_reg.h | 357 ++++++
>>> drivers/gpu/drm/msm/msm_drv.c | 2 +
>>> drivers/gpu/drm/msm/msm_drv.h | 22 +
>>> include/drm/drm_dp_helper.h | 19 +
>>> 30 files changed, 10887 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h
>>> create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h
>>>
>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> new file mode 100644
>>> index 0000000..5818612
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> @@ -0,0 +1,1188 @@
>>> +/*
>>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights
>>> reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License version 2
>>> and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
>>> +
>>> +#include <linux/delay.h>
>>> +#include <drm/drm_dp_helper.h>
>>> +
>>> +#include "dp_catalog.h"
>>> +#include "dp_reg.h"
>>> +
>>> +#define DP_GET_MSB(x) (x >> 8)
>>> +#define DP_GET_LSB(x) (x & 0xff)
>>
>> These aren't used anywhere
>>
> Fixed in V2.
>
>>> +
>>> +#define dp_read(offset) readl_relaxed((offset))
>>> +#define dp_write(offset, data) writel_relaxed((data), (offset))
>>
>> These macros aren't useful. I think you'd be much better having a
>> read_/write_
>> for the different io types, ie:
>>
>> static inline u32 dp_read_audio(struct dp_catalog *catalog, u32
>> offset)
>> {
>> return readl_relaxed(catalog->io->dp_link.base + offset);
>> }
>>
>> Then you can remove a ton of boilerplate code below getting base;
>
> Fixed in V2.
>
>>
>>> +
>>> +#define dp_catalog_get_priv(x) { \
>>
>> static inline struct dp_catalog_private *dp_catalog_get_priv(...)
>> {
>> ...
>> }
>>
> For inline, we have to use 3 different functions (one each for aux,
> ctrl and panel).
> Looking at "type-checking" w.r.t macro, container_of will through an
> error if its not valid pointer.
> Please let me know if you still want to change this macro to inline
> func.
>
/snip
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> new file mode 100644
>>> index 0000000..58951df
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> @@ -0,0 +1,144 @@
>>> +/*
>>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights
>>> reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License version 2
>>> and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#ifndef _DP_CATALOG_H_
>>> +#define _DP_CATALOG_H_
>>> +
>>> +#include "dp_parser.h"
>>> +
>>> +/* interrupts */
>>> +#define DP_INTR_HPD BIT(0)
>>> +#define DP_INTR_AUX_I2C_DONE BIT(3)
>>> +#define DP_INTR_WRONG_ADDR BIT(6)
>>> +#define DP_INTR_TIMEOUT BIT(9)
>>> +#define DP_INTR_NACK_DEFER BIT(12)
>>> +#define DP_INTR_WRONG_DATA_CNT BIT(15)
>>> +#define DP_INTR_I2C_NACK BIT(18)
>>> +#define DP_INTR_I2C_DEFER BIT(21)
>>> +#define DP_INTR_PLL_UNLOCKED BIT(24)
>>> +#define DP_INTR_AUX_ERROR BIT(27)
>>> +
>>> +#define DP_INTR_READY_FOR_VIDEO BIT(0)
>>> +#define DP_INTR_IDLE_PATTERN_SENT BIT(3)
>>> +#define DP_INTR_FRAME_END BIT(6)
>>> +#define DP_INTR_CRC_UPDATED BIT(9)
>>> +
>>> +struct dp_catalog_aux {
>>> + u32 data;
>>> + u32 isr;
>>> +
>>> + u32 (*read_data)(struct dp_catalog_aux *aux);
>>> + int (*write_data)(struct dp_catalog_aux *aux);
>>> + int (*write_trans)(struct dp_catalog_aux *aux);
>>> + int (*clear_trans)(struct dp_catalog_aux *aux, bool read);
>>> + void (*reset)(struct dp_catalog_aux *aux);
>>> + void (*enable)(struct dp_catalog_aux *aux, bool enable);
>>> + void (*update_aux_cfg)(struct dp_catalog_aux *aux,
>>> + struct dp_aux_cfg *cfg, enum dp_phy_aux_config_type type);
>>> + void (*setup)(struct dp_catalog_aux *aux,
>>> + struct dp_aux_cfg *aux_cfg);
>>> + void (*get_irq)(struct dp_catalog_aux *aux, bool cmd_busy);
>>> +};
>>> +
>>> +struct dp_catalog_ctrl {
>>> + u32 dp_tu;
>>> + u32 valid_boundary;
>>> + u32 valid_boundary2;
>>> + u32 isr;
>>> +
>>> + void (*state_ctrl)(struct dp_catalog_ctrl *ctrl, u32 state);
>>> + void (*config_ctrl)(struct dp_catalog_ctrl *ctrl, u32 config);
>>> + void (*lane_mapping)(struct dp_catalog_ctrl *ctrl);
>>> + void (*mainlink_ctrl)(struct dp_catalog_ctrl *ctrl, bool enable);
>>> + void (*config_misc)(struct dp_catalog_ctrl *ctrl, u32 cc, u32 tb);
>>> + void (*config_msa)(struct dp_catalog_ctrl *ctrl, u32 rate,
>>> + u32 stream_rate_khz, bool fixed_nvid);
>>> + void (*set_pattern)(struct dp_catalog_ctrl *ctrl, u32 pattern);
>>> + void (*reset)(struct dp_catalog_ctrl *ctrl);
>>> + void (*usb_reset)(struct dp_catalog_ctrl *ctrl, bool flip);
>>> + bool (*mainlink_ready)(struct dp_catalog_ctrl *ctrl);
>>> + void (*enable_irq)(struct dp_catalog_ctrl *ctrl, bool enable);
>>> + void (*hpd_config)(struct dp_catalog_ctrl *ctrl, bool enable);
>>> + void (*phy_reset)(struct dp_catalog_ctrl *ctrl);
>>> + void (*phy_lane_cfg)(struct dp_catalog_ctrl *ctrl, bool flipped,
>>> + u8 lane_cnt);
>>> + void (*update_vx_px)(struct dp_catalog_ctrl *ctrl, u8 v_level,
>>> + u8 p_level);
>>> + void (*get_interrupt)(struct dp_catalog_ctrl *ctrl);
>>> + void (*update_transfer_unit)(struct dp_catalog_ctrl *ctrl);
>>> + void (*send_phy_pattern)(struct dp_catalog_ctrl *ctrl,
>>> + u32 pattern);
>>> + u32 (*read_phy_pattern)(struct dp_catalog_ctrl *ctrl);
>>> +};
>>> +
>>> +enum dp_catalog_audio_sdp_type {
>>> + DP_AUDIO_SDP_STREAM,
>>> + DP_AUDIO_SDP_TIMESTAMP,
>>> + DP_AUDIO_SDP_INFOFRAME,
>>> + DP_AUDIO_SDP_COPYMANAGEMENT,
>>> + DP_AUDIO_SDP_ISRC,
>>> + DP_AUDIO_SDP_MAX,
>>> +};
>>> +
>>> +enum dp_catalog_audio_header_type {
>>> + DP_AUDIO_SDP_HEADER_1,
>>> + DP_AUDIO_SDP_HEADER_2,
>>> + DP_AUDIO_SDP_HEADER_3,
>>> + DP_AUDIO_SDP_HEADER_MAX,
>>> +};
>>> +
>>> +struct dp_catalog_audio {
>>> + enum dp_catalog_audio_sdp_type sdp_type;
>>> + enum dp_catalog_audio_header_type sdp_header;
>>> + u32 data;
>>> +
>>> + void (*init)(struct dp_catalog_audio *audio);
>>> + void (*enable)(struct dp_catalog_audio *audio);
>>> + void (*config_acr)(struct dp_catalog_audio *audio);
>>> + void (*config_sdp)(struct dp_catalog_audio *audio);
>>> + void (*set_header)(struct dp_catalog_audio *audio);
>>> + void (*get_header)(struct dp_catalog_audio *audio);
>>> + void (*safe_to_exit_level)(struct dp_catalog_audio *audio);
>>
>> None of these are called? So get rid of this entire structure, the
>> functions
>> that aren't called, and the audio enums above.
>>
> Removed all the Audio related APIs in V2.
>
>>> +};
>>> +
>>> +struct dp_catalog_panel {
>>> + u32 total;
>>> + u32 sync_start;
>>> + u32 width_blanking;
>>> + u32 dp_active;
>>> +
>>> + /* TPG */
>>> + u32 hsync_period;
>>> + u32 vsync_period;
>>> + u32 display_v_start;
>>> + u32 display_v_end;
>>> + u32 v_sync_width;
>>> + u32 hsync_ctl;
>>> + u32 display_hctl;
>>
>> All of this information is already available from drm core, so it can
>> go.
>
> Will address this in V3.
>
/snip
>>> +
>>> +static void dp_ctrl_setup_tr_unit(struct dp_ctrl_private *ctrl)
>>> +{
>>> + u32 dp_tu = 0x0;
>>> + u32 valid_boundary = 0x0;
>>> + u32 valid_boundary2 = 0x0;
>>> + struct dp_vc_tu_mapping_table tu_calc_table;
>>> +
>>> + dp_ctrl_calc_tu_parameters(ctrl, &tu_calc_table);
>>> +
>>> + dp_tu |= tu_calc_table.tu_size_minus1;
>>> + valid_boundary |= tu_calc_table.valid_boundary_link;
>>> + valid_boundary |= (tu_calc_table.delay_start_link << 16);
>>> +
>>> + valid_boundary2 |= (tu_calc_table.valid_lower_boundary_link << 1);
>>> + valid_boundary2 |= (tu_calc_table.upper_boundary_count << 16);
>>> + valid_boundary2 |= (tu_calc_table.lower_boundary_count << 20);
>>> +
>>> + if (tu_calc_table.boundary_moderation_en)
>>> + valid_boundary2 |= BIT(0);
>>> +
>>> + pr_debug("dp_tu=0x%x, valid_boundary=0x%x, valid_boundary2=0x%x\n",
>>> + dp_tu, valid_boundary, valid_boundary2);
>>> +
>>> + ctrl->catalog->dp_tu = dp_tu;
>>> + ctrl->catalog->valid_boundary = valid_boundary;
>>> + ctrl->catalog->valid_boundary2 = valid_boundary2;
>>> +
>>> + ctrl->catalog->update_transfer_unit(ctrl->catalog);
>>
>> Again here, if you just call the function directly, you can avoid the
>> callback.
>> If you add function arguments, you can avoid having to store
>> dp_tu/valid_boundary/
>> valid_boundary2 entirely.
>>
> Will remove the dp_tu/valid_boundary/valid_boundary2 in V3.
>>> +}
After removing all the function ptrs in each structure and removing
dp_tu/valid_boundary/valid_boundary2 for dp_catalog_ctrl structure,
we can get rid of dp_catalog_ctrl structure. Similarly, even the other
aux and panel structures have minimal data that can be removed.
If we remove these structures and get read of dp_catalog structure, we
have to find a different way to access dp_catalog_private structure
present in dp_catalog.c file. Having "static dp_catalog_private
*dp_catalog_private_ptr" is acceptable in upstream? Please share if you
have
any better suggestion?
>>> +
>>> +static int dp_ctrl_wait4video_ready(struct dp_ctrl_private *ctrl)
>>> +{
>>> + int ret = 0;
>>> +
>>> + ret = wait_for_completion_timeout(&ctrl->video_comp, HZ / 2);
>>
>> Break out the timeout into a #define
>
> Fixed in V2.
>
>>
>>> + if (ret <= 0) {
>>> + pr_err("Link Train timedout\n");
>>> + ret = -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>
>> You're not checking the return value at the callsite.
>>
> Fixed in V2.
>
>>> +}
/snip
>>> +
>>> +static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel,
>>> + u32 mode_edid_bpp, u32 mode_pclk_khz)
>>> +{
>>> + struct drm_dp_link *link_info;
>>> + const u32 max_supported_bpp = 30, min_supported_bpp = 18;
>>> + u32 bpp = 0, data_rate_khz = 0;
>>> +
>>> + bpp = min_t(u32, mode_edid_bpp, max_supported_bpp);
>>> +
>>> + link_info = &dp_panel->link_info;
>>> + data_rate_khz = link_info->num_lanes * link_info->rate * 8;
>>> +
>>> + while (bpp > min_supported_bpp) {
>>> + if (mode_pclk_khz * bpp <= data_rate_khz)
>>> + break;
>>> + bpp -= 6;
>>
>> Watch out for underflow here.
>>
> Will add check in V3 patch series.
>
Since min_supported_bpp is 18, bpp can never go below 12. I don't think
we need to check underflow here.
>>> + }
>>> +
>>> + return bpp;
>>> +}
>>> +
>>> +static u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel,
>>> + u32 mode_edid_bpp, u32 mode_pclk_khz)
>>> +{
>>> + struct dp_panel_private *panel;
>>> + u32 bpp = mode_edid_bpp;
>>> +
>>> + if (!dp_panel || !mode_edid_bpp || !mode_pclk_khz) {
>>> + pr_err("invalid input\n");
>>> + return 0;
>>> + }
>>> +
>>> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>> +
>>> + if (dp_panel->video_test)
>>> + bpp = dp_link_bit_depth_to_bpp(
>>> + panel->link->test_video.test_bit_depth);
>>> + else
>>> + bpp = dp_panel_get_supported_bpp(dp_panel, mode_edid_bpp,
>>> + mode_pclk_khz);
>>> +
>>> + return bpp;
>>> +}
>>> +
>>> +static void dp_panel_set_test_mode(struct dp_panel_private *panel,
>>> + struct dp_display_mode *mode)
>>> +{
>>> + struct dp_panel_info *pinfo = NULL;
>>
/snip
>>> +
>>> +struct dp_display_mode {
>>> + struct dp_panel_info timing;
>>> + u32 capabilities;
>>> +};
>>> +
>>> +struct dp_panel_in {
>>> + struct device *dev;
>>> + struct dp_aux *aux;
>>> + struct dp_link *link;
>>> + struct dp_catalog_panel *catalog;
>>> +};
>>> +
>>> +struct dp_panel {
>>> + /* dpcd raw data */
>>> + u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
>>
>> Why + 1?
>>
> Will follow up on this in V3.
DP_RECIEVER_CAP_SIZE is 0xf. We need 16 bytes for dpcd read.
>
>>> + u8 ds_ports[DP_MAX_DOWNSTREAM_PORTS];
>>> +
>>> + struct drm_dp_link link_info;
>>> + struct edid *edid;
>>> + struct drm_connector *connector;
>>> + struct dp_panel_info pinfo;
>>> + bool video_test;
>>> +
>>> + u32 vic;
>>> + u32 max_pclk_khz;
>>> +
>>> + u32 max_bw_code;
>>> + int (*init_info)(struct dp_panel *dp_panel);
>>> + int (*deinit)(struct dp_panel *dp_panel);
>>> + int (*timing_cfg)(struct dp_panel *dp_panel);
>>> + int (*read_sink_caps)(struct dp_panel *dp_panel,
>>> + struct drm_connector *connector);
>>> + u32 (*get_min_req_link_rate)(struct dp_panel *dp_panel);
>>
>> This function isn't called anywhere
>>
> Removed in V2.
>
/Snip
>
>>
>>> +
>>> +static int dp_power_pinctrl_set(struct dp_power_private *power, bool
>>> active)
>>> +{
>>> + int rc = -EFAULT;
>>> + struct pinctrl_state *pin_state;
>>> + struct dp_parser *parser;
>>> +
>>> + parser = power->parser;
>>> +
>>> + if (IS_ERR_OR_NULL(parser->pinctrl.pin))
>>> + return PTR_ERR(parser->pinctrl.pin);
>>> +
>>> + pin_state = active ? parser->pinctrl.state_active
>>> + : parser->pinctrl.state_suspend;
>>> + if (!IS_ERR_OR_NULL(pin_state)) {
>>
>> How is this not possible? You check these on initialization.
>>
> Will remove this in V3.
GPIO/pin_ctrl entries are optional entries in dtsi for DP since we don't
need them for specific target. During init, we allow
the probe to go through even though we initialize pin_state or not. So,
we have to check this again here.
>>> + rc = pinctrl_select_state(parser->pinctrl.pin,
>>> + pin_state);
>>
>> Combine on 1 line
>>
> Fixed in V2.
>
>>> + if (rc)
>>> + pr_err("can not set %s pins\n",
>>> + active ? "dp_active"
>>> + : "dp_sleep");
>>
>>
>> Combine on one line. Also return immediately if this is the case.
>>
> Fixed in V2.
>
>>> + } else {
>>> + pr_err("invalid '%s' pinstate\n",
>>> + active ? "dp_active"
>>> + : "dp_sleep");
>>> + }
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +static int dp_power_clk_init(struct dp_power_private *power, bool
>>> enable)
>>
>> Split this into init/deinit, the paths don't share code.
>>
> Fixed in V2.
>
/snip
>>> #define DP_TEST_SINK 0x270
>>> # define DP_TEST_SINK_START (1 << 0)
>>> +#define DP_TEST_AUDIO_MODE 0x271
>>> +#define DP_TEST_AUDIO_PATTERN_TYPE 0x272
>>> +#define DP_TEST_AUDIO_PERIOD_CH1 0x273
>>> +#define DP_TEST_AUDIO_PERIOD_CH2 0x274
>>> +#define DP_TEST_AUDIO_PERIOD_CH3 0x275
>>> +#define DP_TEST_AUDIO_PERIOD_CH4 0x276
>>> +#define DP_TEST_AUDIO_PERIOD_CH5 0x277
>>> +#define DP_TEST_AUDIO_PERIOD_CH6 0x278
>>> +#define DP_TEST_AUDIO_PERIOD_CH7 0x279
>>> +#define DP_TEST_AUDIO_PERIOD_CH8 0x27A
>>>
>>> #define DP_FEC_STATUS 0x280 /* 1.4 */
>>> # define DP_FEC_DECODE_EN_DETECTED (1 << 0)
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>>
>>> _______________________________________________
>>> Freedreno mailing list
>>> Freedreno at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/freedreno
More information about the dri-devel
mailing list