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
+
+#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;
+
+#define dp_catalog_get_priv(x) { \
static inline struct dp_catalog_private *dp_catalog_get_priv(...)
{
...
}
+ struct dp_catalog *dp_catalog; \
+ dp_catalog = container_of(x, struct dp_catalog, x); \
+ catalog = container_of(dp_catalog, struct dp_catalog_private, \
+ dp_catalog); \
+}
+
+#define DP_INTERRUPT_STATUS1 \
+ (DP_INTR_AUX_I2C_DONE| \
+ DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
+ DP_INTR_NACK_DEFER | DP_INTR_WRONG_DATA_CNT | \
+ DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER | \
+ DP_INTR_PLL_UNLOCKED | DP_INTR_AUX_ERROR)
+
+#define DP_INTR_MASK1 (DP_INTERRUPT_STATUS1 << 2)
+
+#define DP_INTERRUPT_STATUS2 \
+ (DP_INTR_READY_FOR_VIDEO | DP_INTR_IDLE_PATTERN_SENT | \
+ DP_INTR_FRAME_END | DP_INTR_CRC_UPDATED)
+
+#define DP_INTR_MASK2 (DP_INTERRUPT_STATUS2 << 2)
Can you please rename these to be consistent with each other?
DP_INTERRUPT_STATUS2_MASK perhaps?
+
+static u8 const vm_pre_emphasis[4][4] = {
+ {0x00, 0x0B, 0x12, 0xFF}, /* pe0, 0 db */
+ {0x00, 0x0A, 0x12, 0xFF}, /* pe1, 3.5 db */
+ {0x00, 0x0C, 0xFF, 0xFF}, /* pe2, 6.0 db */
+ {0xFF, 0xFF, 0xFF, 0xFF} /* pe3, 9.5 db */
+};
+
+/* voltage swing, 0.2v and 1.0v are not support */
+static u8 const vm_voltage_swing[4][4] = {
+ {0x07, 0x0F, 0x14, 0xFF}, /* sw0, 0.4v */
+ {0x11, 0x1D, 0x1F, 0xFF}, /* sw1, 0.6 v */
+ {0x18, 0x1F, 0xFF, 0xFF}, /* sw1, 0.8 v */
+ {0xFF, 0xFF, 0xFF, 0xFF} /* sw1, 1.2 v, optional */
+};
+
+/* audio related catalog functions */
+struct dp_catalog_private {
+ struct device *dev;
+ struct dp_io *io;
+
+ u32 (*audio_map)[DP_AUDIO_SDP_HEADER_MAX];
+ struct dp_catalog dp_catalog;
+};
+
+/* aux related catalog functions */
+static u32 dp_catalog_aux_read_data(struct dp_catalog_aux *aux)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
And another logging type, please remove pr_* and use a
DPU_/DRM_/DRM_DEV_
variant.
+ goto end;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ return dp_read(base + DP_AUX_DATA);
+end:
+ return 0;
... or you could just return directly in the if above and remove the
label :)
Same comment applies below.
Also, probably shouldn't return 0 in an error case.
Then again, I'm guessing hitting the NULL case is likely impossible, so
you can
probably remove all of the machinery around checking !aux.
+}
+
+static int dp_catalog_aux_write_data(struct dp_catalog_aux *aux)
+{
+ int rc = 0;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ dp_write(base + DP_AUX_DATA, aux->data);
+end:
+ return rc;
+}
+
+static int dp_catalog_aux_write_trans(struct dp_catalog_aux *aux)
+{
+ int rc = 0;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ dp_write(base + DP_AUX_TRANS_CTRL, aux->data);
+end:
+ return rc;
+}
+
+static int dp_catalog_aux_clear_trans(struct dp_catalog_aux *aux,
bool read)
+{
+ int rc = 0;
+ u32 data = 0;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ if (read) {
+ data = dp_read(base + DP_AUX_TRANS_CTRL);
+ data &= ~BIT(9);
+ dp_write(base + DP_AUX_TRANS_CTRL, data);
+ } else {
+ dp_write(base + DP_AUX_TRANS_CTRL, 0);
+ }
+end:
+ return rc;
+}
+
+static void dp_catalog_aux_reset(struct dp_catalog_aux *aux)
+{
+ u32 aux_ctrl;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ aux_ctrl = dp_read(base + DP_AUX_CTRL);
+
+ aux_ctrl |= BIT(1);
What's BIT(1)? Can you please #define it?
+ dp_write(base + DP_AUX_CTRL, aux_ctrl);
+ usleep_range(1000, 1010); /* h/w recommended delay */
10us diff is right on the line of enough delay to make usleep_range
worth it.
I'd recommend increasing to 100us to increase the chance of the
scheduler being
able to batch this wakeup with another.
+
+ aux_ctrl &= ~BIT(1);
+ dp_write(base + DP_AUX_CTRL, aux_ctrl);
+ wmb();
Can you just use writel for these cases instead of using _relaxed and
then
wmb()? This applies elsewhere too.
+}
+
+static void dp_catalog_aux_enable(struct dp_catalog_aux *aux, bool
enable)
+{
+ u32 aux_ctrl;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(aux);
+ base = catalog->io->dp_aux.base;
+
+ aux_ctrl = dp_read(base + DP_AUX_CTRL);
+
+ if (enable) {
+ dp_write(base + DP_TIMEOUT_COUNT, 0xffff);
+ dp_write(base + DP_AUX_LIMITS, 0xffff);
+ aux_ctrl |= BIT(0);
Please #define BIT(0)
+ } else {
+ aux_ctrl &= ~BIT(0);
+ }
+
+ dp_write(base + DP_AUX_CTRL, aux_ctrl);
+}
+
+static void dp_catalog_aux_update_cfg(struct dp_catalog_aux *aux,
+ struct dp_aux_cfg *cfg, enum dp_phy_aux_config_type type)
+{
+ struct dp_catalog_private *catalog;
+ u32 new_index = 0, current_index = 0;
+
+ if (!aux || !cfg || (type >= PHY_AUX_CFG_MAX)) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(aux);
+
+ current_index = cfg[type].current_index;
+ new_index = (current_index + 1) % cfg[type].cfg_cnt;
+ pr_debug("Updating %s from 0x%08x to 0x%08x\n",
+ dp_phy_aux_config_type_to_string(type),
+ cfg[type].lut[current_index], cfg[type].lut[new_index]);
+
+ dp_write(catalog->io->phy_io.base + cfg[type].offset,
+ cfg[type].lut[new_index]);
+ cfg[type].current_index = new_index;
+}
+
+static void dp_catalog_aux_setup(struct dp_catalog_aux *aux,
+ struct dp_aux_cfg *cfg)
+{
+ struct dp_catalog_private *catalog;
+ int i = 0;
+
+ if (!aux || !cfg) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(aux);
+
+ dp_write(catalog->io->phy_io.base + DP_PHY_PD_CTL, 0x65);
What's 0x65? Please #define
+ wmb(); /* make sure PD programming happened */
+
+ /* Turn on BIAS current for PHY/PLL */
+ dp_write(catalog->io->dp_pll_io.base +
+ QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x1b);
Same here for 0x1b. I'm going to stop mentioning this, please audit the
driver
for other instances of hardcoded values (like this and the one at the
end of
this function) and break them out into meaningful #defines.
+
+ /* DP AUX CFG register programming */
+ for (i = 0; i < PHY_AUX_CFG_MAX; i++) {
+ pr_debug("%s: offset=0x%08x, value=0x%08x\n",
+ dp_phy_aux_config_type_to_string(i),
+ cfg[i].offset, cfg[i].lut[cfg[i].current_index]);
+ dp_write(catalog->io->phy_io.base + cfg[i].offset,
+ cfg[i].lut[cfg[i].current_index]);
+ }
+
+ dp_write(catalog->io->phy_io.base + DP_PHY_AUX_INTERRUPT_MASK,
0x1F);
+}
+
+static void dp_catalog_aux_get_irq(struct dp_catalog_aux *aux, bool
cmd_busy)
+{
+ u32 ack;
+ struct dp_catalog_private *catalog;
+ void __iomem *ahb_base;
+
+ if (!aux) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(aux);
+ ahb_base = catalog->io->dp_ahb.base;
+
+ aux->isr = dp_read(ahb_base + DP_INTR_STATUS);
+ aux->isr &= ~DP_INTR_MASK1;
+ ack = aux->isr & DP_INTERRUPT_STATUS1;
+ ack <<= 1;
+ ack |= DP_INTR_MASK1;
+ dp_write(ahb_base + DP_INTR_STATUS, ack);
+}
+
+/* controller related catalog functions */
+static void dp_catalog_ctrl_update_transfer_unit(struct
dp_catalog_ctrl *ctrl)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ dp_write(base + DP_VALID_BOUNDARY, ctrl->valid_boundary);
+ dp_write(base + DP_TU, ctrl->dp_tu);
+ dp_write(base + DP_VALID_BOUNDARY_2, ctrl->valid_boundary2);
+}
+
+static void dp_catalog_ctrl_state_ctrl(struct dp_catalog_ctrl *ctrl,
u32 state)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ dp_write(base + DP_STATE_CTRL, state);
+}
+
+static void dp_catalog_ctrl_config_ctrl(struct dp_catalog_ctrl *ctrl,
u32 cfg)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *link_base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ link_base = catalog->io->dp_link.base;
+
+ pr_debug("DP_CONFIGURATION_CTRL=0x%x\n", cfg);
+
+ dp_write(link_base + DP_CONFIGURATION_CTRL, cfg);
+}
+
+static void dp_catalog_ctrl_lane_mapping(struct dp_catalog_ctrl
*ctrl)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ dp_write(base + DP_LOGICAL2PHYSICAL_LANE_MAPPING, 0xe4);
+}
+
+static void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog_ctrl
*ctrl,
+ bool enable)
+{
+ u32 mainlink_ctrl;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ if (enable) {
+ dp_write(base + DP_MAINLINK_CTRL, 0x02000000);
+ wmb(); /* make sure mainlink is turned off before reset */
+ dp_write(base + DP_MAINLINK_CTRL, 0x02000002);
+ wmb(); /* make sure mainlink entered reset */
+ dp_write(base + DP_MAINLINK_CTRL, 0x02000000);
+ wmb(); /* make sure mainlink reset done */
+ dp_write(base + DP_MAINLINK_CTRL, 0x02000001);
+ wmb(); /* make sure mainlink turned on */
Same comments here re: readl vs readl_relaxed + wmb
Please #define the bits you're setting.
+ } else {
+ mainlink_ctrl = dp_read(base + DP_MAINLINK_CTRL);
+ mainlink_ctrl &= ~BIT(0);
+ dp_write(base + DP_MAINLINK_CTRL, mainlink_ctrl);
+ }
+}
+
+static void dp_catalog_ctrl_config_misc(struct dp_catalog_ctrl *ctrl,
+ u32 cc, u32 tb)
+{
+ u32 misc_val;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ misc_val = dp_read(base + DP_MISC1_MISC0);
+ misc_val |= cc;
Should you mask this so it doesn't stomp areas it shouldn't?
+ misc_val |= (tb << 5);
Parentheses are unnecessary
+ misc_val |= BIT(0); /* Configure clock to synchronous mode */
+
+ pr_debug("misc settings = 0x%x\n", misc_val);
+ dp_write(base + DP_MISC1_MISC0, misc_val);
+}
+
+static void dp_catalog_ctrl_config_msa(struct dp_catalog_ctrl *ctrl,
+ u32 rate, u32 stream_rate_khz,
+ bool fixed_nvid)
+{
+ u32 pixel_m, pixel_n;
+ u32 mvid, nvid;
+ u64 mvid_calc;
+ u32 const nvid_fixed = 0x8000;
You should pull this out into a #define in drm_dp_helper.h and use the
new
define in the i915 driver (in a separate patch) as well as here.
+ u32 const link_rate_hbr2 = 540000;
+ u32 const link_rate_hbr3 = 810000;
+ struct dp_catalog_private *catalog;
+ void __iomem *base_cc, *base_ctrl;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ if (fixed_nvid) {
+ pr_debug("use fixed NVID=0x%x\n", nvid_fixed);
+ nvid = nvid_fixed;
+
+ pr_debug("link rate=%dkbps, stream_rate_khz=%uKhz",
+ rate, stream_rate_khz);
+
+ /*
+ * For intermediate results, use 64 bit arithmetic to avoid
+ * loss of precision.
+ */
+ mvid_calc = (u64) stream_rate_khz * nvid;
+ mvid_calc = div_u64(mvid_calc, rate);
+
+ /*
+ * truncate back to 32 bits as this final divided value will
+ * always be within the range of a 32 bit unsigned int.
+ */
+ mvid = (u32) mvid_calc;
+ } else {
+ base_cc = catalog->io->dp_cc_io.base;
+
+ pixel_m = dp_read(base_cc + MMSS_DP_PIXEL_M);
+ pixel_n = dp_read(base_cc + MMSS_DP_PIXEL_N);
+ pr_debug("pixel_m=0x%x, pixel_n=0x%x\n", pixel_m, pixel_n);
+
+ mvid = (pixel_m & 0xFFFF) * 5;
+ nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF);
+
+ pr_debug("rate = %d\n", rate);
+
+ if (link_rate_hbr2 == rate)
+ nvid *= 2;
+
+ if (link_rate_hbr3 == rate)
+ nvid *= 3;
switch (drm_dp_bw_code_to_link_rate(rate)) {
case DP_LINK_BW_5_4:
nvid *= 2;
break;
case DP_LINK_BW_8_1:
nvid *= 3;
break;
default:
break;
}
And remove link_rate_hbr* above.
+ }
+
+ base_ctrl = catalog->io->dp_link.base;
+ pr_debug("mvid=0x%x, nvid=0x%x\n", mvid, nvid);
+ dp_write(base_ctrl + DP_SOFTWARE_MVID, mvid);
+ dp_write(base_ctrl + DP_SOFTWARE_NVID, nvid);
+}
+
+static void dp_catalog_ctrl_set_pattern(struct dp_catalog_ctrl *ctrl,
+ u32 pattern)
+{
+ int bit, cnt = 10;
+ u32 data;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ bit = 1;
+ bit <<= (pattern - 1);
bit = BIT(pattern - 1);
+ pr_debug("hw: bit=%d train=%d\n", bit, pattern);
+ dp_write(base + DP_STATE_CTRL, bit);
+
+ bit = 8;
+ bit <<= (pattern - 1);
bit = BIT(pattern - 1) << 3;
+
+ while (cnt--) {
+ data = dp_read(base + DP_MAINLINK_READY);
+ if (data & bit)
+ break;
Should there be a sleep in here? Also, consider using
readx_poll_timeout instead
of rolling your own polling.
+ }
+
+ if (cnt == 0)
+ pr_err("set link_train=%d failed\n", pattern);
This function should return an int and report this failure instead of
ignoring
it.
+}
+
+static void dp_catalog_ctrl_usb_reset(struct dp_catalog_ctrl *ctrl,
bool flip)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+
+ base = catalog->io->usb3_dp_com.base;
+
+ dp_write(base + USB3_DP_COM_RESET_OVRD_CTRL, 0x0a);
Again, all of these magic values need to be pulled out into #defines so
we know
what the values mean.
+ dp_write(base + USB3_DP_COM_PHY_MODE_CTRL, 0x02);
+ dp_write(base + USB3_DP_COM_SW_RESET, 0x01);
+ /* make sure usb3 com phy software reset is done */
+ wmb();
Same comments here wrt wmb()
+
+ if (!flip) /* CC1 */
+ dp_write(base + USB3_DP_COM_TYPEC_CTRL, 0x02);
+ else /* CC2 */
+ dp_write(base + USB3_DP_COM_TYPEC_CTRL, 0x03);
u32 val;
...
val = SOME_DEFINE_THAT_MAKES_SENSE;
if (flip)
val |= ANOTHER_DEFINE_THAT_MEANS_FLIP;
dp_io_write(USB3_DP_COM_TYPEC_CTRL, val);
+
+ dp_write(base + USB3_DP_COM_SWI_CTRL, 0x00);
+ dp_write(base + USB3_DP_COM_SW_RESET, 0x00);
+ /* make sure the software reset is done */
+ wmb();
+
+ dp_write(base + USB3_DP_COM_POWER_DOWN_CTRL, 0x01);
+ dp_write(base + USB3_DP_COM_RESET_OVRD_CTRL, 0x00);
+ /* make sure phy is brought out of reset */
+ wmb();
+
+}
+
+static void dp_catalog_panel_tpg_cfg(struct dp_catalog_panel *panel,
+ bool enable)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!panel) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(panel);
+ base = catalog->io->dp_p0.base;
+
+ if (!enable) {
+ dp_write(base + MMSS_DP_TPG_MAIN_CONTROL, 0x0);
+ dp_write(base + MMSS_DP_BIST_ENABLE, 0x0);
+ dp_write(base + MMSS_DP_TIMING_ENGINE_EN, 0x0);
+ wmb(); /* ensure Timing generator is turned off */
+ return;
+ }
There's really no reason to have both the enable + disable path in the
same
function, they don't share any code (aside from writing a few different
values
to the same registers). Could you please split this function into a
timing_generator_disable and timing_generator_enable?
+
+ dp_write(base + MMSS_DP_INTF_CONFIG, 0x0);
+ dp_write(base + MMSS_DP_INTF_HSYNC_CTL, panel->hsync_ctl);
+ dp_write(base + MMSS_DP_INTF_VSYNC_PERIOD_F0, panel->vsync_period *
+ panel->hsync_period);
+ dp_write(base + MMSS_DP_INTF_VSYNC_PULSE_WIDTH_F0,
panel->v_sync_width *
+ panel->hsync_period);
+ dp_write(base + MMSS_DP_INTF_VSYNC_PERIOD_F1, 0);
+ dp_write(base + MMSS_DP_INTF_VSYNC_PULSE_WIDTH_F1, 0);
+ dp_write(base + MMSS_DP_INTF_DISPLAY_HCTL, panel->display_hctl);
+ dp_write(base + MMSS_DP_INTF_ACTIVE_HCTL, 0);
+ dp_write(base + MMSS_INTF_DISPLAY_V_START_F0,
panel->display_v_start);
+ dp_write(base + MMSS_DP_INTF_DISPLAY_V_END_F0,
panel->display_v_end);
+ dp_write(base + MMSS_INTF_DISPLAY_V_START_F1, 0);
+ dp_write(base + MMSS_DP_INTF_DISPLAY_V_END_F1, 0);
+ dp_write(base + MMSS_DP_INTF_ACTIVE_V_START_F0, 0);
+ dp_write(base + MMSS_DP_INTF_ACTIVE_V_END_F0, 0);
+ dp_write(base + MMSS_DP_INTF_ACTIVE_V_START_F1, 0);
+ dp_write(base + MMSS_DP_INTF_ACTIVE_V_END_F1, 0);
+ dp_write(base + MMSS_DP_INTF_POLARITY_CTL, 0);
+ wmb(); /* ensure TPG registers are programmed */
+
+ dp_write(base + MMSS_DP_TPG_MAIN_CONTROL, 0x100);
+ dp_write(base + MMSS_DP_TPG_VIDEO_CONFIG, 0x5);
+ wmb(); /* ensure TPG config is programmed */
+ dp_write(base + MMSS_DP_BIST_ENABLE, 0x1);
+ dp_write(base + MMSS_DP_TIMING_ENGINE_EN, 0x1);
+ wmb(); /* ensure Timing generator is turned on */
+ pr_debug("%s: enabled tpg\n", __func__);
+}
+
+static void dp_catalog_ctrl_reset(struct dp_catalog_ctrl *ctrl)
+{
+ u32 sw_reset;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_ahb.base;
+
+ sw_reset = dp_read(base + DP_SW_RESET);
+
+ sw_reset |= BIT(0);
Should be a #define
+ dp_write(base + DP_SW_RESET, sw_reset);
+ usleep_range(1000, 1010); /* h/w recommended delay */
Same comment here regarding the 10us range
+
+ sw_reset &= ~BIT(0);
+ dp_write(base + DP_SW_RESET, sw_reset);
+}
+
+static bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog_ctrl
*ctrl)
+{
+ u32 data;
+ int cnt = 10;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ goto end;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_link.base;
+
+ while (--cnt) {
+ /* DP_MAINLINK_READY */
+ data = dp_read(base + DP_MAINLINK_READY);
+ if (data & BIT(0))
+ return true;
+
+ usleep_range(1000, 1010); /* 1ms wait before next reg read */
Same comment here regarding using readx_poll_timeout wrapped in a
helper, also
10us comment.
+ }
+ pr_err("mainlink not ready\n");
+end:
+ return false;
+}
+
+static void dp_catalog_ctrl_enable_irq(struct dp_catalog_ctrl *ctrl,
+ bool enable)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_ahb.base;
+
+ if (enable) {
+ dp_write(base + DP_INTR_STATUS, DP_INTR_MASK1);
+ dp_write(base + DP_INTR_STATUS2, DP_INTR_MASK2);
+ } else {
+ dp_write(base + DP_INTR_STATUS, 0x00);
+ dp_write(base + DP_INTR_STATUS2, 0x00);
+ }
+}
+
+static void dp_catalog_ctrl_hpd_config(struct dp_catalog_ctrl *ctrl,
bool en)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_aux.base;
+
+ if (en) {
+ u32 reftimer = dp_read(base + DP_DP_HPD_REFTIMER);
+
+ dp_write(base + DP_DP_HPD_INT_ACK, 0xF);
+ dp_write(base + DP_DP_HPD_INT_MASK, 0xF);
+
+ /* Enabling REFTIMER */
+ reftimer |= BIT(16);
You don't use this anywhere?
+ dp_write(base + DP_DP_HPD_REFTIMER, 0xF);
+ /* Enable HPD */
+ dp_write(base + DP_DP_HPD_CTRL, 0x1);
+ } else {
+ /*Disable HPD */
Space after *
+ dp_write(base + DP_DP_HPD_CTRL, 0x0);
+ }
+}
+
+static void dp_catalog_ctrl_get_interrupt(struct dp_catalog_ctrl
*ctrl)
+{
+ u32 ack = 0;
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_ahb.base;
+
+ ctrl->isr = dp_read(base + DP_INTR_STATUS2);
This value should just get returned instead of being stored.
+ ctrl->isr &= ~DP_INTR_MASK2;
Unmasking the interrupts should not be returned from this function, it
should
be part of the ack below.
+ ack = ctrl->isr & DP_INTERRUPT_STATUS2;
And this does the above unmask already, so you can just turf it.
+ ack <<= 1;
+ ack |= DP_INTR_MASK2;
Instead of rolling the ack code yourself, I'd prefer what you did with
_MASK,
where you define DP_INTERRUPT_STATUS2_ACK above and just do a
bitwise-OR to add
the acks here. Without the register map in front of me, it would take a
while
to figure out what you're trying to accomplish.
+ dp_write(base + DP_INTR_STATUS2, ack);
+}
+
+static void dp_catalog_ctrl_phy_reset(struct dp_catalog_ctrl *ctrl)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base = catalog->io->dp_ahb.base;
+
+ dp_write(base + DP_PHY_CTRL, 0x5); /* bit 0 & 2 */
The comment doesn't help here, please #define these bits
+ usleep_range(1000, 1010); /* h/w recommended delay */
Same comment re: delay range
+ dp_write(base + DP_PHY_CTRL, 0x0);
+ wmb(); /* make sure PHY reset done */
Same comment re: wmb
+}
+
+static void dp_catalog_ctrl_phy_lane_cfg(struct dp_catalog_ctrl
*ctrl,
+ bool flipped, u8 ln_cnt)
+{
+ u32 info = 0x0;
+ struct dp_catalog_private *catalog;
+ u8 orientation = BIT(!!flipped);
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+
+ info |= (ln_cnt & 0x0F);
Remove the | and just make this an assignment (and you can remove the
initialization to 0 above)
+ info |= ((orientation & 0x0F) << 4);
+ pr_debug("Shared Info = 0x%x\n", info);
+
+ dp_write(catalog->io->phy_io.base + DP_PHY_SPARE0, info);
+}
+
+static void dp_catalog_ctrl_update_vx_px(struct dp_catalog_ctrl
*ctrl,
+ u8 v_level, u8 p_level)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base0, *base1;
+ u8 value0, value1;
Can we please pick better names for these?!
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+ base0 = catalog->io->ln_tx0_io.base;
+ base1 = catalog->io->ln_tx1_io.base;
+
+ pr_debug("hw: v=%d p=%d\n", v_level, p_level);
+
+ value0 = vm_voltage_swing[v_level][p_level];
+ value1 = vm_pre_emphasis[v_level][p_level];
+
+ /* program default setting first */
Why do you need to do this?
+ dp_write(base0 + TXn_TX_DRV_LVL, 0x2A);
+ dp_write(base1 + TXn_TX_DRV_LVL, 0x2A);
+ dp_write(base0 + TXn_TX_EMP_POST1_LVL, 0x20);
+ dp_write(base1 + TXn_TX_EMP_POST1_LVL, 0x20);
#define these magic values
+
+ /* Enable MUX to use Cursor values from these registers */
+ value0 |= BIT(5);
+ value1 |= BIT(5);
#define these bits please
+
+ /* Configure host and panel only if both values are allowed */
+ if (value0 != 0xFF && value1 != 0xFF) {
You can check this a lot earlier in the function.
+ dp_write(base0 + TXn_TX_DRV_LVL, value0);
+ dp_write(base1 + TXn_TX_DRV_LVL, value0);
+ dp_write(base0 + TXn_TX_EMP_POST1_LVL, value1);
+ dp_write(base1 + TXn_TX_EMP_POST1_LVL, value1);
+
+ pr_debug("hw: vx_value=0x%x px_value=0x%x\n",
+ value0, value1);
+ } else {
+ pr_err("invalid vx (0x%x=0x%x), px (0x%x=0x%x\n",
+ v_level, value0, p_level, value1);
Seems like returning an error in this case would be useful
+ }
+}
+
+static void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog_ctrl
*ctrl,
+ u32 pattern)
+{
+ struct dp_catalog_private *catalog;
+ u32 value = 0x0;
+ void __iomem *base = NULL;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp_catalog_get_priv(ctrl);
+
+ base = catalog->io->dp_link.base;
+
+ dp_write(base + DP_STATE_CTRL, 0x0);
Do you really need to do this?
+
+ switch (pattern) {
+ case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING:
+ dp_write(base + DP_STATE_CTRL, 0x1);
+ break;
+ case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT:
+ value &= ~(1 << 16);
+ dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
+ value |= 0xFC;
+ dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
+ dp_write(base + DP_MAINLINK_LEVELS, 0x2);
+ dp_write(base + DP_STATE_CTRL, 0x10);
+ break;
+ case DP_TEST_PHY_PATTERN_PRBS7:
+ dp_write(base + DP_STATE_CTRL, 0x20);
+ break;
+ case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN:
+ dp_write(base + DP_STATE_CTRL, 0x40);
+ /* 00111110000011111000001111100000 */
+ dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG0, 0x3E0F83E0);
+ /* 00001111100000111110000011111000 */
+ dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG1, 0x0F83E0F8);
+ /* 1111100000111110 */
+ dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG2, 0x0000F83E);
+ break;
+ case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN:
+ value = BIT(16);
+ dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
+ value |= 0xFC;
+ dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
+ dp_write(base + DP_MAINLINK_LEVELS, 0x2);
+ dp_write(base + DP_STATE_CTRL, 0x10);
This code is the exact same as the case for
DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT, aside from BIT(16) vs
~BIT(16). Can you please combine these cases
and share code?
+ break;
+ default:
+ pr_debug("No valid test pattern requested: 0x%x\n", pattern);
+ return;
+ }
All of the DP_STATE_CTRL values should be #defined, and you can move
the write
down here instead of duplicating it everywhere (with
DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN writing a 0).
+
+ /* Make sure the test pattern is programmed in the hardware */
+ wmb();
+}
+
+static u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog_ctrl
*ctrl)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base = NULL;
+
+ if (!ctrl) {
+ pr_err("invalid input\n");
+ return 0;
+ }
+
+ dp_catalog_get_priv(ctrl);
+
+ base = catalog->io->dp_link.base;
+
+ return dp_read(base + DP_MAINLINK_READY);
+}
+
+/* panel related catalog functions */
+static int dp_catalog_panel_timing_cfg(struct dp_catalog_panel
*panel)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+
+ if (!panel) {
+ pr_err("invalid input\n");
+ goto end;
+ }
+
+ dp_catalog_get_priv(panel);
+ base = catalog->io->dp_link.base;
+
+ dp_write(base + DP_TOTAL_HOR_VER, panel->total);
+ dp_write(base + DP_START_HOR_VER_FROM_SYNC, panel->sync_start);
+ dp_write(base + DP_HSYNC_VSYNC_WIDTH_POLARITY,
panel->width_blanking);
+ dp_write(base + DP_ACTIVE_HOR_VER, panel->dp_active);
+end:
useless label, but it'll probably go away when you strip out all of the
unnecessary NULL checks from the driver.
+ return 0;
+}
+
+static void dp_catalog_audio_init(struct dp_catalog_audio *audio)
+{
+ struct dp_catalog_private *catalog;
+ static u32 sdp_map[][DP_AUDIO_SDP_HEADER_MAX] = {
+ {
+ MMSS_DP_AUDIO_STREAM_0,
+ MMSS_DP_AUDIO_STREAM_1,
+ MMSS_DP_AUDIO_STREAM_1,
+ },
+ {
+ MMSS_DP_AUDIO_TIMESTAMP_0,
+ MMSS_DP_AUDIO_TIMESTAMP_1,
+ MMSS_DP_AUDIO_TIMESTAMP_1,
+ },
+ {
+ MMSS_DP_AUDIO_INFOFRAME_0,
+ MMSS_DP_AUDIO_INFOFRAME_1,
+ MMSS_DP_AUDIO_INFOFRAME_1,
+ },
+ {
+ MMSS_DP_AUDIO_COPYMANAGEMENT_0,
+ MMSS_DP_AUDIO_COPYMANAGEMENT_1,
+ MMSS_DP_AUDIO_COPYMANAGEMENT_1,
+ },
+ {
+ MMSS_DP_AUDIO_ISRC_0,
+ MMSS_DP_AUDIO_ISRC_1,
+ MMSS_DP_AUDIO_ISRC_1,
+ },
+ };
+
+ if (!audio)
+ return;
+
+ dp_catalog_get_priv(audio);
+
+ catalog->audio_map = sdp_map;
Hmm, this is a bit spooky, we're storing a (non-const) pointer to a
static
stack variable. Fortunately this looks like it'll be easy to fix since
this
data is only accessed in this file. So move the map into a static const
global
and reference it directly when needed (and drop audio_map).
+}
+
+static void dp_catalog_audio_config_sdp(struct dp_catalog_audio
*audio)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+ u32 sdp_cfg = 0;
+ u32 sdp_cfg2 = 0;
+
+ if (!audio)
+ return;
+
+ dp_catalog_get_priv(audio);
+ base = catalog->io->dp_link.base;
+
+ sdp_cfg = dp_read(base + MMSS_DP_SDP_CFG);
+
+ /* AUDIO_TIMESTAMP_SDP_EN */
+ sdp_cfg |= BIT(1);
+ /* AUDIO_STREAM_SDP_EN */
+ sdp_cfg |= BIT(2);
+ /* AUDIO_COPY_MANAGEMENT_SDP_EN */
+ sdp_cfg |= BIT(5);
+ /* AUDIO_ISRC_SDP_EN */
+ sdp_cfg |= BIT(6);
+ /* AUDIO_INFOFRAME_SDP_EN */
+ sdp_cfg |= BIT(20);
#define the values and combine the assignment
+
+ pr_debug("sdp_cfg = 0x%x\n", sdp_cfg);
+ dp_write(base + MMSS_DP_SDP_CFG, sdp_cfg);
+
+ sdp_cfg2 = dp_read(base + MMSS_DP_SDP_CFG2);
+ /* IFRM_REGSRC -> Do not use reg values */
+ sdp_cfg2 &= ~BIT(0);
+ /* AUDIO_STREAM_HB3_REGSRC-> Do not use reg values */
+ sdp_cfg2 &= ~BIT(1);
The reset value of this register is 0x4, so how can the bits in
position 0/1
get flipped? We shouldn't need this code, afaict.
If we do, the same comments regarding #define and combination as above.
+
+ pr_debug("sdp_cfg2 = 0x%x\n", sdp_cfg2);
+ dp_write(base + MMSS_DP_SDP_CFG2, sdp_cfg2);
+}
+
+static void dp_catalog_audio_get_header(struct dp_catalog_audio
*audio)
+{
+ struct dp_catalog_private *catalog;
+ u32 (*sdp_map)[DP_AUDIO_SDP_HEADER_MAX];
+ void __iomem *base;
+ enum dp_catalog_audio_sdp_type sdp;
+ enum dp_catalog_audio_header_type header;
+
+ if (!audio)
+ return;
+
+ dp_catalog_get_priv(audio);
+
+ base = catalog->io->dp_link.base;
+ sdp_map = catalog->audio_map;
+ sdp = audio->sdp_type;
+ header = audio->sdp_header;
+
+ audio->data = dp_read(base + sdp_map[sdp][header]);
+}
This function is unused, please remove.
+
+static void dp_catalog_audio_set_header(struct dp_catalog_audio
*audio)
+{
+ struct dp_catalog_private *catalog;
+ u32 (*sdp_map)[DP_AUDIO_SDP_HEADER_MAX];
+ void __iomem *base;
+ enum dp_catalog_audio_sdp_type sdp;
+ enum dp_catalog_audio_header_type header;
+ u32 data;
+
+ if (!audio)
+ return;
+
+ dp_catalog_get_priv(audio);
+
+ base = catalog->io->dp_link.base;
+ sdp_map = catalog->audio_map;
+ sdp = audio->sdp_type;
+ header = audio->sdp_header;
+ data = audio->data;
+
+ dp_write(base + sdp_map[sdp][header], data);
+}
This function is unused, please remove.
+
+static void dp_catalog_audio_config_acr(struct dp_catalog_audio
*audio)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+ u32 acr_ctrl, select;
+
+ dp_catalog_get_priv(audio);
+
+ select = audio->data;
+ base = catalog->io->dp_link.base;
+
+ acr_ctrl = select << 4 | BIT(31) | BIT(8) | BIT(14);
+
+ pr_debug("select = 0x%x, acr_ctrl = 0x%x\n", select, acr_ctrl);
+
+ dp_write(base + MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl);
+}
This function is unused, please remove.
+
+static void dp_catalog_audio_safe_to_exit_level(struct
dp_catalog_audio *audio)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+ u32 mainlink_levels, safe_to_exit_level;
+
+ dp_catalog_get_priv(audio);
+
+ base = catalog->io->dp_link.base;
+ safe_to_exit_level = audio->data;
+
+ mainlink_levels = dp_read(base + DP_MAINLINK_LEVELS);
+ mainlink_levels &= 0xFE0;
+ mainlink_levels |= safe_to_exit_level;
+
+ pr_debug("mainlink_level = 0x%x, safe_to_exit_level = 0x%x\n",
+ mainlink_levels, safe_to_exit_level);
+
+ dp_write(base + DP_MAINLINK_LEVELS, mainlink_levels);
+}
This function is unused, please remove.
+
+static void dp_catalog_audio_enable(struct dp_catalog_audio *audio)
+{
+ struct dp_catalog_private *catalog;
+ void __iomem *base;
+ bool enable;
+ u32 audio_ctrl;
+
+ dp_catalog_get_priv(audio);
+
+ base = catalog->io->dp_link.base;
+ enable = !!audio->data;
+
+ audio_ctrl = dp_read(base + MMSS_DP_AUDIO_CFG);
+
+ if (enable)
+ audio_ctrl |= BIT(0);
+ else
+ audio_ctrl &= ~BIT(0);
+
+ pr_debug("dp_audio_cfg = 0x%x\n", audio_ctrl);
+ dp_write(base + MMSS_DP_AUDIO_CFG, audio_ctrl);
+
+ /* make sure audio engine is disabled */
+ wmb();
+}
This function is unused, please remove.
+
+struct dp_catalog *dp_catalog_get(struct device *dev, struct dp_io
*io)
+{
+ int rc = 0;
+ struct dp_catalog *dp_catalog;
+ struct dp_catalog_private *catalog;
+ struct dp_catalog_aux aux = {
+ .read_data = dp_catalog_aux_read_data,
+ .write_data = dp_catalog_aux_write_data,
+ .write_trans = dp_catalog_aux_write_trans,
+ .clear_trans = dp_catalog_aux_clear_trans,
+ .reset = dp_catalog_aux_reset,
+ .update_aux_cfg = dp_catalog_aux_update_cfg,
+ .enable = dp_catalog_aux_enable,
+ .setup = dp_catalog_aux_setup,
+ .get_irq = dp_catalog_aux_get_irq,
Out of these, it seems like write_trans, clear_trans, update_aux_cfg,
setup and
get_irq are all unused. Please remove them.
Please also check the other functions to ensure they're used.
+ };
+ struct dp_catalog_ctrl ctrl = {
+ .state_ctrl = dp_catalog_ctrl_state_ctrl,
+ .config_ctrl = dp_catalog_ctrl_config_ctrl,
+ .lane_mapping = dp_catalog_ctrl_lane_mapping,
+ .mainlink_ctrl = dp_catalog_ctrl_mainlink_ctrl,
+ .config_misc = dp_catalog_ctrl_config_misc,
+ .config_msa = dp_catalog_ctrl_config_msa,
+ .set_pattern = dp_catalog_ctrl_set_pattern,
+ .reset = dp_catalog_ctrl_reset,
+ .usb_reset = dp_catalog_ctrl_usb_reset,
+ .mainlink_ready = dp_catalog_ctrl_mainlink_ready,
+ .enable_irq = dp_catalog_ctrl_enable_irq,
+ .hpd_config = dp_catalog_ctrl_hpd_config,
+ .phy_reset = dp_catalog_ctrl_phy_reset,
+ .phy_lane_cfg = dp_catalog_ctrl_phy_lane_cfg,
+ .update_vx_px = dp_catalog_ctrl_update_vx_px,
+ .get_interrupt = dp_catalog_ctrl_get_interrupt,
+ .update_transfer_unit = dp_catalog_ctrl_update_transfer_unit,
+ .send_phy_pattern = dp_catalog_ctrl_send_phy_pattern,
+ .read_phy_pattern = dp_catalog_ctrl_read_phy_pattern,
So, what's the benefit of adding the catalog abstractions? Do you ever
have
multiple/alternate implementations of a catalog? It doesn't seem like
it. You
should just remove all of the catalog overhead and call the functions
directly.
+
+ ln_cnt = ctrl->link->link_params.lane_count;
+
+ bpp = pinfo->bpp;
+ lwidth = pinfo->h_active;
+ h_blank = pinfo->h_back_porch + pinfo->h_front_porch +
+ pinfo->h_sync_width;
+ pclk = pinfo->pixel_clk_khz * 1000;
+
+ boundary_moderation_en = 0;
+ upper_bdry_cnt = 0;
+ lower_bdry_cnt = 0;
+ i_upper_bdry_cnt = 0;
+ i_lower_bdry_cnt = 0;
+ valid_lower_boundary_link = 0;
+ even_distribution_bf = 0;
+ even_distribution_legacy = 0;
+ even_distribution = 0;
+ min_hblank = 0;
+
+ lclk = drm_dp_bw_code_to_link_rate(
+ ctrl->link->link_params.bw_code) * DP_KHZ_TO_HZ;
+
+ pr_debug("pclk=%lld, active_width=%d, h_blank=%d\n",
+ pclk, lwidth, h_blank);
+ pr_debug("lclk = %lld, ln_cnt = %d\n", lclk, ln_cnt);
+ ratio = div64_u64_rem(pclk * bpp * multiplier,
+ 8 * ln_cnt * lclk, &reminder);
+ ratio = div64_u64((pclk * bpp * multiplier), (8 * ln_cnt * lclk));
+ original_ratio = ratio;
+
+ extra_buffer_margin = roundup_u64(div64_u64(extra_pclk_cycle_delay
+ * lclk * multiplier, pclk), multiplier);
+ extra_buffer_margin = div64_u64(extra_buffer_margin, multiplier);
+
+ /* To deal with cases where lines are not distributable */
+ if (((lwidth % ln_cnt) != 0) && ratio < multiplier) {
+ ratio = ratio * ratio_scale;
+ ratio = ratio < (1000 * multiplier)
+ ? ratio : (1000 * multiplier);
+ }
+ pr_debug("ratio = %lld\n", ratio);
+
+ for (tu_size = 32; tu_size <= 64; tu_size++) {
+ temp = ratio * tu_size;
+ temp2 = ((temp / multiplier) + 1) * multiplier;
+ n_err = roundup_u64(temp, multiplier) - temp;
+
+ if (n_err < err) {
+ err = n_err;
+ tu_size_desired = tu_size;
+ }
+ }
+ pr_debug("Info: tu_size_desired = %d\n", tu_size_desired);
+
+ tu_size_minus1 = tu_size_desired - 1;
+
+ valid_boundary_link = roundup_u64(ratio * tu_size_desired,
multiplier);
+ valid_boundary_link /= multiplier;
+ n_tus = rounddown((lwidth * bpp * multiplier)
+ / (8 * valid_boundary_link), multiplier) / multiplier;
+ even_distribution_legacy = n_tus % ln_cnt == 0 ? 1 : 0;
+ pr_debug("Info: n_symbol_per_tu=%d, number_of_tus=%d\n",
+ valid_boundary_link, n_tus);
+
+ extra_bytes = roundup_u64((n_tus + 1)
+ * ((valid_boundary_link * multiplier)
+ - (original_ratio * tu_size_desired)), multiplier);
+ extra_bytes /= multiplier;
+ extra_pclk_cycles = roundup(extra_bytes * 8 * multiplier / bpp,
+ multiplier);
+ extra_pclk_cycles /= multiplier;
+ extra_pclk_cycles_in_link_clk =
roundup_u64(div64_u64(extra_pclk_cycles
+ * lclk * multiplier, pclk), multiplier);
+ extra_pclk_cycles_in_link_clk /= multiplier;
+ filler_size = roundup_u64((tu_size_desired - valid_boundary_link)
+ * multiplier, multiplier);
+ filler_size /= multiplier;
+ ratio_by_tu = div64_u64(ratio * tu_size_desired, multiplier);
+
+ pr_debug("extra_pclk_cycles_in_link_clk=%d, extra_bytes=%d\n",
+ extra_pclk_cycles_in_link_clk, extra_bytes);
+ pr_debug("extra_pclk_cycles_in_link_clk=%d\n",
+ extra_pclk_cycles_in_link_clk);
+ pr_debug("filler_size=%d, extra_buffer_margin=%lld\n",
+ filler_size, extra_buffer_margin);
+
+ delay_start_link = ((extra_bytes > extra_pclk_cycles_in_link_clk)
+ ? extra_bytes
+ : extra_pclk_cycles_in_link_clk)
+ + filler_size + extra_buffer_margin;
+ resulting_valid = valid_boundary_link;
+ pr_debug("Info: delay_start_link=%d, filler_size=%d\n",
+ delay_start_link, filler_size);
+ pr_debug("valid_boundary_link=%d ratio_by_tu=%lld\n",
+ valid_boundary_link, ratio_by_tu);
+
+ diff_abs = (resulting_valid >= ratio_by_tu)
+ ? (resulting_valid - ratio_by_tu)
+ : (ratio_by_tu - resulting_valid);
+
+ if (err != 0 && ((diff_abs > brute_force_threshold)
+ || (even_distribution_legacy == 0)
+ || (dp_brute_force == 1))) {
+ err = multiplier;
+ for (tu_size = 32; tu_size <= 64; tu_size++) {
+ for (i_upper_bdry_cnt = 1; i_upper_bdry_cnt <= 15;
+ i_upper_bdry_cnt++) {
+ for (i_lower_bdry_cnt = 1;
+ i_lower_bdry_cnt <= 15;
+ i_lower_bdry_cnt++) {
+ new_valid_boundary_link =
+ roundup_u64(ratio
+ * tu_size, multiplier);
+ average_valid2 = (i_upper_bdry_cnt
+ * new_valid_boundary_link
+ + i_lower_bdry_cnt
+ * (new_valid_boundary_link
+ - multiplier))
+ / (i_upper_bdry_cnt
+ + i_lower_bdry_cnt);
+ n_tus = rounddown_u64(div64_u64(lwidth
+ * multiplier * multiplier
+ * (bpp / 8), average_valid2),
+ multiplier);
+ n_tus /= multiplier;
+ n_tus_per_lane
+ = rounddown(n_tus
+ * multiplier
+ / ln_cnt, multiplier);
+ n_tus_per_lane /= multiplier;
+ paired_tus =
+ rounddown((n_tus_per_lane)
+ * multiplier
+ / (i_upper_bdry_cnt
+ + i_lower_bdry_cnt),
+ multiplier);
+ paired_tus /= multiplier;
+ remainder_tus = n_tus_per_lane
+ - paired_tus
+ * (i_upper_bdry_cnt
+ + i_lower_bdry_cnt);
+ if ((remainder_tus
+ - i_upper_bdry_cnt) > 0) {
+ remainder_tus_upper
+ = i_upper_bdry_cnt;
+ remainder_tus_lower =
+ remainder_tus
+ - i_upper_bdry_cnt;
+ } else {
+ remainder_tus_upper
+ = remainder_tus;
+ remainder_tus_lower = 0;
+ }
+ total_valid = paired_tus
+ * (i_upper_bdry_cnt
+ * new_valid_boundary_link
+ + i_lower_bdry_cnt
+ * (new_valid_boundary_link
+ - multiplier))
+ + (remainder_tus_upper
+ * new_valid_boundary_link)
+ + (remainder_tus_lower
+ * (new_valid_boundary_link
+ - multiplier));
+ n_err_neg = nn_err_neg = false;
+ effective_valid
+ = div_u64(total_valid,
+ n_tus_per_lane);
+ n_n_err = (effective_valid
+ >= (ratio * tu_size))
+ ? (effective_valid
+ - (ratio * tu_size))
+ : ((ratio * tu_size)
+ - effective_valid);
+ if (effective_valid < (ratio * tu_size))
+ nn_err_neg = true;
+ n_err = (average_valid2
+ >= (ratio * tu_size))
+ ? (average_valid2
+ - (ratio * tu_size))
+ : ((ratio * tu_size)
+ - average_valid2);
+ if (average_valid2 < (ratio * tu_size))
+ n_err_neg = true;
+ even_distribution =
+ n_tus % ln_cnt == 0 ? 1 : 0;
+ diff_abs =
+ resulting_valid >= ratio_by_tu
+ ? (resulting_valid
+ - ratio_by_tu)
+ : (ratio_by_tu
+ - resulting_valid);
+
+ resulting_valid_tmp = div64_u64(
+ (i_upper_bdry_cnt
+ * new_valid_boundary_link
+ + i_lower_bdry_cnt
+ * (new_valid_boundary_link
+ - multiplier)),
+ (i_upper_bdry_cnt
+ + i_lower_bdry_cnt));
+ ratio_by_tu_tmp =
+ original_ratio * tu_size;
+ ratio_by_tu_tmp /= multiplier;
+ n_tus_tmp = rounddown_u64(
+ div64_u64(lwidth
+ * multiplier * multiplier
+ * bpp / 8,
+ resulting_valid_tmp),
+ multiplier);
+ n_tus_tmp /= multiplier;
+
+ temp3 = (resulting_valid_tmp
+ >= (original_ratio * tu_size))
+ ? (resulting_valid_tmp
+ - original_ratio * tu_size)
+ : (original_ratio * tu_size)
+ - resulting_valid_tmp;
+ temp3 = (n_tus_tmp + 1) * temp3;
+ temp4 = (new_valid_boundary_link
+ >= (original_ratio * tu_size))
+ ? (new_valid_boundary_link
+ - original_ratio
+ * tu_size)
+ : (original_ratio * tu_size)
+ - new_valid_boundary_link;
+ temp4 = (i_upper_bdry_cnt
+ * ln_cnt * temp4);
+
+ temp3 = roundup_u64(temp3, multiplier);
+ temp4 = roundup_u64(temp4, multiplier);
+ dp_ctrl_get_extra_req_bytes
+ (resulting_valid_tmp,
+ new_valid_boundary_link,
+ temp3, temp4,
+ &extra_req_bytes_is_neg,
+ &result,
+ (original_ratio * tu_size));
+ extra_req_bytes_new_tmp
+ = div64_ul(result, multiplier);
+ if ((extra_req_bytes_is_neg)
+ && (extra_req_bytes_new_tmp
+ > 1))
+ extra_req_bytes_new_tmp
+ = extra_req_bytes_new_tmp - 1;
+ if (extra_req_bytes_new_tmp == 0)
+ extra_req_bytes_new_tmp = 1;
+ extra_pclk_cycles_tmp =
+ (u64)(extra_req_bytes_new_tmp
+ * 8 * multiplier) / bpp;
+ extra_pclk_cycles_tmp /= multiplier;
+
+ if (extra_pclk_cycles_tmp <= 0)
+ extra_pclk_cycles_tmp = 1;
+ extra_pclk_cycles_in_lclk_tmp =
+ roundup_u64(div64_u64(
+ extra_pclk_cycles_tmp
+ * lclk * multiplier,
+ pclk), multiplier);
+ extra_pclk_cycles_in_lclk_tmp
+ /= multiplier;
+ filler_size_tmp = roundup_u64(
+ (tu_size * multiplier *
+ new_valid_boundary_link),
+ multiplier);
+ filler_size_tmp /= multiplier;
+ lower_filler_size_tmp =
+ filler_size_tmp + 1;
+ if (extra_req_bytes_is_neg)
+ temp3 = (extra_req_bytes_new_tmp
+ > extra_pclk_cycles_in_lclk_tmp
+ ? extra_pclk_cycles_in_lclk_tmp
+ : extra_req_bytes_new_tmp);
+ else
+ temp3 = (extra_req_bytes_new_tmp
+ > extra_pclk_cycles_in_lclk_tmp
+ ? extra_req_bytes_new_tmp :
+ extra_pclk_cycles_in_lclk_tmp);
+
+ temp4 = lower_filler_size_tmp
+ + extra_buffer_margin;
+ if (extra_req_bytes_is_neg)
+ delay_start_link_tmp
+ = (temp3 >= temp4)
+ ? (temp3 - temp4)
+ : (temp4 - temp3);
+ else
+ delay_start_link_tmp
+ = temp3 + temp4;
+
+ min_hblank_tmp = (int)div64_u64(
+ roundup_u64(
+ div64_u64(delay_start_link_tmp
+ * pclk * multiplier, lclk),
+ multiplier), multiplier)
+ + hblank_margin;
+
+ if (((even_distribution == 1)
+ || ((even_distribution_bf == 0)
+ && (even_distribution_legacy
+ == 0)))
+ && !n_err_neg && !nn_err_neg
+ && n_n_err < err
+ && (n_n_err < diff_abs
+ || (dp_brute_force == 1))
+ && (new_valid_boundary_link
+ - 1) > 0
+ && (h_blank >=
+ (u32)min_hblank_tmp)) {
+ upper_bdry_cnt =
+ i_upper_bdry_cnt;
+ lower_bdry_cnt =
+ i_lower_bdry_cnt;
+ err = n_n_err;
+ boundary_moderation_en = 1;
+ tu_size_desired = tu_size;
+ valid_boundary_link =
+ new_valid_boundary_link;
+ effective_valid_recorded
+ = effective_valid;
+ delay_start_link
+ = delay_start_link_tmp;
+ filler_size = filler_size_tmp;
+ min_hblank = min_hblank_tmp;
+ n_tus = n_tus_tmp;
+ even_distribution_bf = 1;
+
+ pr_debug("upper_bdry_cnt=%d, lower_boundary_cnt=%d, err=%lld,
tu_size_desired=%d, valid_boundary_link=%d, effective_valid=%lld\n",
+ upper_bdry_cnt,
+ lower_bdry_cnt, err,
+ tu_size_desired,
+ valid_boundary_link,
+ effective_valid);
+ }
+ }
+ }
+ }
+
+ if (boundary_moderation_en == 1) {
+ resulting_valid = (u64)(upper_bdry_cnt
+ *valid_boundary_link + lower_bdry_cnt
+ * (valid_boundary_link - 1))
+ / (upper_bdry_cnt + lower_bdry_cnt);
+ ratio_by_tu = original_ratio * tu_size_desired;
+ valid_lower_boundary_link =
+ (valid_boundary_link / multiplier) - 1;
+
+ tu_size_minus1 = tu_size_desired - 1;
+ even_distribution_bf = 1;
+ valid_boundary_link /= multiplier;
+ pr_debug("Info: Boundary_moderation enabled\n");
+ }
+ }
+
+ min_hblank = ((int) roundup_u64(div64_u64(delay_start_link * pclk
+ * multiplier, lclk), multiplier))
+ / multiplier + hblank_margin;
+ if (h_blank < (u32)min_hblank) {
+ pr_debug(" WARNING: run_idx=%d Programmed h_blank %d is smaller
than the min_hblank %d supported.\n",
+ run_idx, h_blank, min_hblank);
+ }
+
+ if (fifo_empty) {
+ tu_size_minus1 = 31;
+ valid_boundary_link = 32;
+ delay_start_link = 0;
+ boundary_moderation_en = 0;
+ }
+
+ pr_debug("tu_size_minus1=%d valid_boundary_link=%d
delay_start_link=%d boundary_moderation_en=%d\n upper_boundary_cnt=%d
lower_boundary_cnt=%d valid_lower_boundary_link=%d min_hblank=%d\n",
+ tu_size_minus1, valid_boundary_link, delay_start_link,
+ boundary_moderation_en, upper_bdry_cnt, lower_bdry_cnt,
+ valid_lower_boundary_link, min_hblank);
+
+ tu_table->valid_boundary_link = valid_boundary_link;
+ tu_table->delay_start_link = delay_start_link;
+ tu_table->boundary_moderation_en = boundary_moderation_en;
+ tu_table->valid_lower_boundary_link = valid_lower_boundary_link;
+ tu_table->upper_boundary_count = upper_bdry_cnt;
+ tu_table->lower_boundary_count = lower_bdry_cnt;
+ tu_table->tu_size_minus1 = tu_size_minus1;
+}
+
+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.
+}
+
+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
+ if (ret <= 0) {
+ pr_err("Link Train timedout\n");
+ ret = -EINVAL;
+ }
+
+ return ret;
You're not checking the return value at the callsite.
+}
+
+static int dp_ctrl_update_sink_vx_px(struct dp_ctrl_private *ctrl,
+ u32 voltage_level, u32 pre_emphasis_level)
I don't think this needs to be a function, just move it all into
dp_ctrl_update_vx_px.
+{
+ int i;
+ u8 buf[4];
+ u32 max_level_reached = 0;
+
+ if (voltage_level == DP_LINK_VOLTAGE_MAX) {
+ pr_debug("max. voltage swing level reached %d\n",
+ voltage_level);
+ max_level_reached |= BIT(2);
DP_TRAIN_MAX_SWING_REACHED
+ }
+
+ if (pre_emphasis_level == DP_LINK_PRE_EMPHASIS_MAX) {
+ pr_debug("max. pre-emphasis level reached %d\n",
+ pre_emphasis_level);
+ max_level_reached |= BIT(5);
DP_TRAIN_MAX_PRE_EMPHASIS_REACHED
+ }
+
+ pre_emphasis_level <<= 3;
This is DP_TRAIN_PRE_EMPHASIS_SHIFT, but I'm guessing you aren't using
the
DP_TRAIN_PRE_EMPH_LEVEL_* values when you set p_level. So the right way
to fix
this is to use those #defines and get rid of this shift.
+
+ for (i = 0; i < 4; i++)
You shouldn't assume 4 lanes here, use drm_dp_link->num_lanes
+ buf[i] = voltage_level | pre_emphasis_level | max_level_reached;
+
+ pr_debug("sink: p|v=0x%x\n", voltage_level | pre_emphasis_level);
+ return drm_dp_dpcd_write(ctrl->aux->drm_aux, 0x103, buf, 4);
s/0x103/DP_TRAINING_LANE0_SET/ and s/4/drm_dp_link->num_lanes/
Also, this will return number of bytes transmitted, not (0 || -errno).
+}
+
+static void dp_ctrl_update_vx_px(struct dp_ctrl_private *ctrl)
+{
+ struct dp_link *link = ctrl->link;
+
+ ctrl->catalog->update_vx_px(ctrl->catalog,
+ link->phy_params.v_level, link->phy_params.p_level);
+
+ dp_ctrl_update_sink_vx_px(ctrl, link->phy_params.v_level,
+ link->phy_params.p_level);
Fortunately (or unfortunately), the return value isn't checked. So
please check
the return value.
+}
+
+static void dp_ctrl_train_pattern_set(struct dp_ctrl_private *ctrl,
+ u8 pattern)
+{
+ u8 buf[4];
Why 4?
+
+ pr_debug("sink: pattern=%x\n", pattern);
+
+ buf[0] = pattern;
+ drm_dp_dpcd_write(ctrl->aux->drm_aux, DP_TRAINING_PATTERN_SET, buf,
1);
Check the return value of this and alll drm_dp_dpcd_(read|write) calls.
+}
+
+static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
+ u8 *link_status)
+{
+ int ret = 0, len;
+ u32 const offset = DP_LANE_ALIGN_STATUS_UPDATED - DP_LANE0_1_STATUS;
+ u32 link_status_read_max_retries = 100;
+
+ while (--link_status_read_max_retries) {
+ len = drm_dp_dpcd_read_link_status(ctrl->aux->drm_aux,
+ link_status);
+ if (len != DP_LINK_STATUS_SIZE) {
+ pr_err("DP link status read failed, err: %d\n", len);
+ ret = len;
+ break;
Just return directly here. You can also just drop len and use ret (or
drop ret
and use len).
+ }
+
+ if (!(link_status[offset] & DP_LINK_STATUS_UPDATED))
+ break;
And return 0 directly here
+ }
+
+ return ret;
Here you're returning successful in the timeout case :( Once you
convert the above
to direct returns, this can become return -ETIMEDOUT;
I think you should also drop the number of retries drastically.
+}
+
+static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl)
+{
+ int tries, old_v_level, ret = 0;
+ u8 link_status[DP_LINK_STATUS_SIZE];
+ int const maximum_retries = 5;
+
+ dp_ctrl_state_ctrl(ctrl, 0);
+ /* Make sure to clear the current pattern before starting a new one
*/
+ wmb();
+
+ ctrl->catalog->set_pattern(ctrl->catalog, 0x01);
#define that bit please
+ dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_1 |
+ DP_LINK_SCRAMBLING_DISABLE); /* train_1 */
the train_1 comment isn't needed.
+ dp_ctrl_update_vx_px(ctrl);
+
+ tries = 0;
+ old_v_level = ctrl->link->phy_params.v_level;
+ while (1) {
Instead of unbounded, this should be:
for (tries = 0; tries < maximum_retries; tries++) {
You'll need to s/break/return ret/ in the rest of the loop and move the
timeout
error to the bottom of the function
+ drm_dp_link_train_clock_recovery_delay(ctrl->panel->dpcd);
+
+ ret = dp_ctrl_read_link_status(ctrl, link_status);
+ if (ret)
+ break;
+
+ if (drm_dp_clock_recovery_ok(link_status,
+ ctrl->link->link_params.lane_count)) {
+ break;
+ }
+
+ if (ctrl->link->phy_params.v_level == DP_LINK_VOLTAGE_MAX) {
Since DP_LINK_VOLTAGE_MAX == DP_LINK_VOLTAGE_LEVEL_2, you'll end up not
retrying
LEVEL_2 at all.
+ pr_err_ratelimited("max v_level reached\n");
+ ret = -EAGAIN;
+ break;
+ }
+
+ if (old_v_level == ctrl->link->phy_params.v_level) {
+ tries++;
+ if (tries >= maximum_retries) {
+ pr_err("max tries reached\n");
+ ret = -ETIMEDOUT;
+ break;
+ }
+ } else {
+ tries = 0;
+ old_v_level = ctrl->link->phy_params.v_level;
+ }
+
+ pr_debug("clock recovery not done, adjusting vx px\n");
+
+ ctrl->link->adjust_levels(ctrl->link, link_status);
+ dp_ctrl_update_vx_px(ctrl);
+ }
+
+ return ret;
+}
+
+static int dp_ctrl_link_rate_down_shift(struct dp_ctrl_private *ctrl)
+{
+ int ret = 0;
+
+ if (!ctrl)
+ return -EINVAL;
+
+ switch (ctrl->link->link_params.bw_code) {
+ case DP_LINK_BW_8_1:
+ ctrl->link->link_params.bw_code = DP_LINK_BW_5_4;
+ break;
+ case DP_LINK_BW_5_4:
+ ctrl->link->link_params.bw_code = DP_LINK_BW_2_7;
+ break;
+ case DP_LINK_BW_2_7:
+ case DP_LINK_BW_1_62:
+ default:
+ ctrl->link->link_params.bw_code = DP_LINK_BW_1_62;
+ break;
+ };
+
+ pr_debug("new bw code=0x%x\n", ctrl->link->link_params.bw_code);
+
+ return ret;
+}
+
+static void dp_ctrl_clear_training_pattern(struct dp_ctrl_private
*ctrl)
+{
+ dp_ctrl_train_pattern_set(ctrl, 0);
DP_TRAINING_PATTERN_DISABLE
+ drm_dp_link_train_channel_eq_delay(ctrl->panel->dpcd);
+}
+
+static int dp_ctrl_link_training_2(struct dp_ctrl_private *ctrl)
+{
+ int tries = 0, ret = 0;
+ char pattern;
+ int const maximum_retries = 5;
+ u8 link_status[DP_LINK_STATUS_SIZE];
+
+ dp_ctrl_state_ctrl(ctrl, 0);
+ /* Make sure to clear the current pattern before starting a new one
*/
+ wmb();
+
+ if (drm_dp_tps3_supported(ctrl->panel->dpcd))
+ pattern = DP_TRAINING_PATTERN_3;
+ else
+ pattern = DP_TRAINING_PATTERN_2;
+
+ dp_ctrl_update_vx_px(ctrl);
+ ctrl->catalog->set_pattern(ctrl->catalog, pattern);
+ dp_ctrl_train_pattern_set(ctrl, pattern |
DP_RECOVERED_CLOCK_OUT_EN);
+
+ do {
for (tries = 0; tries < maximum_retries; tries++)
+ drm_dp_link_train_channel_eq_delay(ctrl->panel->dpcd);
+
+ ret = dp_ctrl_read_link_status(ctrl, link_status);
+ if (ret)
+ break;
+
+ if (drm_dp_channel_eq_ok(link_status,
+ ctrl->link->link_params.lane_count))
+ break;
+
+ if (tries > maximum_retries) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ tries++;
+
+ ctrl->link->adjust_levels(ctrl->link, link_status);
+ dp_ctrl_update_vx_px(ctrl);
+ } while (1);
+
+ return ret;
+}
+
+static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl)
+{
+ int ret = 0;
+ u8 encoding = 0x1;
DP_SET_ANSI_8B10B
+ struct drm_dp_link link_info = {0};
+
+ ctrl->link->phy_params.p_level = 0;
+ ctrl->link->phy_params.v_level = 0;
+
+ dp_ctrl_config_ctrl(ctrl);
+
+ link_info.num_lanes = ctrl->link->link_params.lane_count;
+ link_info.rate = drm_dp_bw_code_to_link_rate(
+ ctrl->link->link_params.bw_code);
+ link_info.capabilities = ctrl->panel->link_info.capabilities;
Drop the hand-rolled ctrl->panel->link_info and use drm_dp_link_probe
to fill in
drm_dp_link.
+
+ drm_dp_link_configure(ctrl->aux->drm_aux, &link_info);
+ drm_dp_dpcd_write(ctrl->aux->drm_aux,
DP_MAIN_LINK_CHANNEL_CODING_SET,
+ &encoding, 1);
+
+ ret = dp_ctrl_link_train_1(ctrl);
+ if (ret) {
+ pr_err("link training #1 failed\n");
print ret
+ goto end;
+ }
+
+ /* print success info as this is a result of user initiated action
*/
+ pr_info("link training #1 successful\n");
this should be debug loglevel
+
+ ret = dp_ctrl_link_training_2(ctrl);
+ if (ret) {
+ pr_err("link training #2 failed\n");
print ret
+ goto end;
+ }
+
+ /* print success info as this is a result of user initiated action
*/
+ pr_info("link training #2 successful\n");
debug loglevel
+
+end:
+ dp_ctrl_state_ctrl(ctrl, 0);
+ /* Make sure to clear the current pattern before starting a new one
*/
+ wmb();
Same comments re: wmb
+
+ dp_ctrl_clear_training_pattern(ctrl);
+ return ret;
+}
+
+static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl, bool
train)
+{
+ bool mainlink_ready = false;
+ int ret = 0;
+
+ ctrl->catalog->mainlink_ctrl(ctrl->catalog, true);
+
+ ret = ctrl->link->psm_config(ctrl->link,
+ &ctrl->panel->link_info, false);
This can fit on one line
+ if (ret)
+ goto end;
All of the "goto end"'s can be replaced with "return ret", then delete
the end
label.
+
+ if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
You should really protect sink_request with a lock, it's racy. I'm
trying
to catch the race conditions while I review, but I'd urge you to audit
the
driver for other places that need locking.
+ goto end;
+
+ if (!train)
+ goto send_video;
Instead of adding the label, just wrap the reset and train calls in an
if (train) {} block.
+
+ /*
+ * As part of previous calls, DP controller state might have
+ * transitioned to PUSH_IDLE. In order to start transmitting a link
+ * training pattern, we have to first to a DP software reset.
"we have to first _do_ a..."
+ */
+ ctrl->catalog->reset(ctrl->catalog);
+
+ ret = dp_ctrl_link_train(ctrl);
+ if (ret)
+ goto end;
+
+send_video:
+ /*
+ * Set up transfer unit values and set controller state to send
+ * video.
+ */
+ dp_ctrl_setup_tr_unit(ctrl);
+ ctrl->catalog->state_ctrl(ctrl->catalog, ST_SEND_VIDEO);
+
+ dp_ctrl_wait4video_ready(ctrl);
Check return value please
+ mainlink_ready = ctrl->catalog->mainlink_ready(ctrl->catalog);
+ pr_debug("mainlink %s\n", mainlink_ready ? "READY" : "NOT READY");
+end:
+ return ret;
+}
+
+static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
+ char *name, u32 rate)
+{
+ u32 num = ctrl->parser->mp[DP_CTRL_PM].num_clk;
+ struct dss_clk *cfg = ctrl->parser->mp[DP_CTRL_PM].clk_config;
+
+ while (num && strcmp(cfg->clk_name, name)) {
+ num--;
+ cfg++;
+ }
+
+ pr_debug("setting rate=%d on clk=%s\n", rate, name);
+
+ if (num)
+ cfg->rate = rate;
+ else
+ pr_err("%s clock could not be set with rate %d\n", name, rate);
+}
Instead of doing all of this work, just store pointers to the link and
pixel
clock when you parse the dt. That way you can just access them directly
in
dp_ctrl_enable_mainlink_clocks.
+
+static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private
*ctrl)
+{
+ int ret = 0;
+
+ ctrl->power->set_pixel_clk_parent(ctrl->power);
+
+ dp_ctrl_set_clock_rate(ctrl, "ctrl_link_clk",
+ drm_dp_bw_code_to_link_rate(ctrl->link->link_params.bw_code));
+
+ dp_ctrl_set_clock_rate(ctrl, "ctrl_pixel_clk", ctrl->pixel_rate);
+
+ ret = ctrl->power->clk_enable(ctrl->power, DP_CTRL_PM, true);
+ if (ret) {
+ pr_err("Unabled to start link clocks\n");
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int dp_ctrl_disable_mainlink_clocks(struct dp_ctrl_private
*ctrl)
+{
+ return ctrl->power->clk_enable(ctrl->power, DP_CTRL_PM, false);
+}
This function isn't necesary (even less so when all of the catalog
abstractions
are gone).
+
+static int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip)
+{
+ struct dp_ctrl_private *ctrl;
+ struct dp_catalog_ctrl *catalog;
+
+ if (!dp_ctrl) {
+ pr_err("Invalid input data\n");
+ return -EINVAL;
+ }
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ ctrl->orientation = flip;
+ catalog = ctrl->catalog;
+
+ catalog->usb_reset(ctrl->catalog, flip);
+ catalog->phy_reset(ctrl->catalog);
+ catalog->enable_irq(ctrl->catalog, true);
+
+ return 0;
+}
+
+/**
+ * dp_ctrl_host_deinit() - Uninitialize DP controller
+ * @ctrl: Display Port Driver data
+ *
+ * Perform required steps to uninitialize DP controller
+ * and its resources.
+ */
+static void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+
+ if (!dp_ctrl) {
+ pr_err("Invalid input data\n");
+ return;
+ }
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ ctrl->catalog->enable_irq(ctrl->catalog, false);
+
+ pr_debug("Host deinitialized successfully\n");
+}
+
+static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
+{
+ u8 *dpcd = ctrl->panel->dpcd;
+
+ /*
+ * For better interop experience, used a fixed NVID=0x8000
+ * whenever connected to a VGA dongle downstream.
+ */
+ if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
+ u8 type = dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+ DP_DWN_STRM_PORT_TYPE_MASK;
+ if (type == DP_DWN_STRM_PORT_TYPE_ANALOG)
+ return true;
+ }
+
+ return false;
+}
For this, you should use the DP_DPCD_QUIRK_CONSTANT_N quirk and
drm_dp_has_quirk() instead.
+
+static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
+{
+ int ret = 0;
+
+ ctrl->dp_ctrl.push_idle(&ctrl->dp_ctrl);
+ ctrl->dp_ctrl.reset(&ctrl->dp_ctrl);
+
+ ctrl->pixel_rate = ctrl->panel->pinfo.pixel_clk_khz;
+
+ do {
+ if (ret == -EAGAIN) {
+ /* try with lower link rate */
+ dp_ctrl_link_rate_down_shift(ctrl);
+
+ ctrl->catalog->mainlink_ctrl(ctrl->catalog, false);
+ }
+
+ ctrl->catalog->phy_lane_cfg(ctrl->catalog,
+ ctrl->orientation, ctrl->link->link_params.lane_count);
+
+ /*
+ * Disable and re-enable the mainlink clock since the
+ * link clock might have been adjusted as part of the
+ * link maintenance.
+ */
+ dp_ctrl_disable_mainlink_clocks(ctrl);
Return value should be checked
Below you have:
+ /* hw recommended delay before re-enabling clocks */
+ msleep(20);
Between the disable and enable, so you should probably have that here
too?
+
+ ret = dp_ctrl_enable_mainlink_clocks(ctrl);
+ if (ret)
+ continue;
Do you really want to continue here? Seems like you could get into an
infinite
loop pretty easily in the -EINVAL case.
+
+ dp_ctrl_configure_source_params(ctrl);
+
+ ctrl->catalog->config_msa(ctrl->catalog,
+ drm_dp_bw_code_to_link_rate(
+ ctrl->link->link_params.bw_code),
+ ctrl->pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
+
+ reinit_completion(&ctrl->idle_comp);
Are you guaranteed that this doesn't race with other idle_comp uses?
+
+ ret = dp_ctrl_setup_main_link(ctrl, true);
+ } while (ret == -EAGAIN);
The do/while is a bit awkward, especially since it starts with a
conditional
block that can't possibly be true on the first iteration. How about:
for (tries = 0; tries < max_tries; tries++) {
ctrl->catalog->phy_lane_cfg(ctrl->catalog,
ctrl->orientation, ctrl->link->link_params.lane_count);
/*
* Disable and re-enable the mainlink clock since the
* link clock might have been adjusted as part of the
* link maintenance.
*/
ret = dp_ctrl_disable_mainlink_clocks(ctrl);
if (ret)
return ret;
ret = dp_ctrl_enable_mainlink_clocks(ctrl);
if (ret)
return ret;
dp_ctrl_configure_source_params(ctrl);
ctrl->catalog->config_msa(ctrl->catalog,
drm_dp_bw_code_to_link_rate(
ctrl->link->link_params.bw_code),
ctrl->pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
reinit_completion(&ctrl->idle_comp);
ret = dp_ctrl_setup_main_link(ctrl, true);
if (ret != -EAGAIN)
return ret;
/* try with lower link rate */
dp_ctrl_link_rate_down_shift(ctrl);
ctrl->catalog->mainlink_ctrl(ctrl->catalog, false);
}
return -ETIMEDOUT;
There's a lot of similarity between this code and dp_ctrl_on. If the
code were
shared, we wouldn't have the missing 20ms delay above. So can you
please pull
the common bits which [re-]initialize the link into a helper function
that you
call from both functions?
+
+ return ret;
It's nice that we return a value here, but it's never checked by our
callers :(
+}
+
+static void dp_ctrl_process_phy_test_request(struct dp_ctrl_private
*ctrl)
+{
+ int ret = 0;
+
+ if (!ctrl->link->phy_params.phy_test_pattern_sel) {
+ pr_debug("no test pattern selected by sink\n");
+ return;
+ }
+
+ pr_debug("start\n");
This and the "end" debug statements should be removed
+
+ ctrl->dp_ctrl.push_idle(&ctrl->dp_ctrl);
+ /*
+ * The global reset will need DP link ralated clocks to be
s/related/ralated/
+ * running. Add the global reset just before disabling the
+ * link clocks and core clocks.
+ */
+ ctrl->dp_ctrl.reset(&ctrl->dp_ctrl);
+ ctrl->dp_ctrl.off(&ctrl->dp_ctrl);
+
+ ret = ctrl->dp_ctrl.on(&ctrl->dp_ctrl);
+ if (ret)
+ pr_err("failed to enable DP controller\n");
Please propagate errors instead of swallowing them.
+
+ pr_debug("end\n");
+}
+
+static void dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private
*ctrl)
+{
+ bool success = false;
+ u32 pattern_sent = 0x0;
+ u32 pattern_requested = ctrl->link->phy_params.phy_test_pattern_sel;
+
+ pr_debug("request: %s\n",
+ dp_link_get_phy_test_pattern(pattern_requested));
+
+ ctrl->catalog->update_vx_px(ctrl->catalog,
+ ctrl->link->phy_params.v_level,
+ ctrl->link->phy_params.p_level);
+ ctrl->catalog->send_phy_pattern(ctrl->catalog, pattern_requested);
+ ctrl->link->send_test_response(ctrl->link);
+
+ pattern_sent = ctrl->catalog->read_phy_pattern(ctrl->catalog);
+
+ switch (pattern_sent) {
+ case MR_LINK_TRAINING1:
+ if (pattern_requested ==
+ DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING)
+ success = true;
success = pattern_requested == DP_LINK_QUAL_PATTERN_11_D10_2;
Same can be done below.
+ break;
+ case MR_LINK_SYMBOL_ERM:
+ if ((pattern_requested ==
+ DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT)
+ || (pattern_requested ==
+ DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN))
+ success = true;
+ break;
+ case MR_LINK_PRBS7:
+ if (pattern_requested == DP_TEST_PHY_PATTERN_PRBS7)
+ success = true;
+ break;
+ case MR_LINK_CUSTOM80:
+ if (pattern_requested ==
+ DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN)
+ success = true;
+ break;
+ default:
+ success = false;
+ return;
+ }
I feel like you should return success (or a ret). As it is, this
doesn't do much.
+
+ pr_debug("%s: %s\n", success ? "success" : "failed",
+ dp_link_get_phy_test_pattern(pattern_requested));
+}
+
+static void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+ u32 sink_request = 0x0;
+
+ if (!dp_ctrl) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ sink_request = ctrl->link->sink_request;
+
+ if (sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) {
+ pr_info("PHY_TEST_PATTERN request\n");
Downgrade to DEBUG level please
+ dp_ctrl_process_phy_test_request(ctrl);
+ }
+
+ if (sink_request & DP_LINK_STATUS_UPDATED)
+ dp_ctrl_link_maintenance(ctrl);
Check return value
+
+ if (sink_request & DP_TEST_LINK_TRAINING) {
+ ctrl->link->send_test_response(ctrl->link);
+ dp_ctrl_link_maintenance(ctrl);
Check return value
+ }
+}
+
+static void dp_ctrl_reset(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+
+ if (!dp_ctrl) {
+ pr_err("invalid params\n");
+ return;
+ }
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ ctrl->catalog->reset(ctrl->catalog);
+}
I think this function goes away with all of the catalog stuff removed.
+
+static int dp_ctrl_on(struct dp_ctrl *dp_ctrl)
+{
+ int rc = 0;
+ struct dp_ctrl_private *ctrl;
+ u32 rate = 0;
+ u32 link_train_max_retries = 100;
This amount is excessive, 5 times should suffice (unless you have data
that
suggests otherwise).
+ u32 const phy_cts_pixel_clk_khz = 148500;
Should this be a #define in drm_dp_helper?
+
+ if (!dp_ctrl) {
+ rc = -EINVAL;
+ goto end;
+ }
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ atomic_set(&ctrl->aborted, 0);
Why is this needed over is_connected? It seems like if you had proper
locking
around is_connected (which I think is necessary), you could just
acquire the lock
and try the training. If it fails b/c someone unplugged the cable,
that's quite
Ok.
+ rate = ctrl->panel->link_info.rate;
+
+ ctrl->power->clk_enable(ctrl->power, DP_CORE_PM, true);
+ ctrl->catalog->hpd_config(ctrl->catalog, true);
+
+ if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) {
+ pr_debug("using phy test link parameters\n");
+ if (!ctrl->panel->pinfo.pixel_clk_khz)
+ ctrl->pixel_rate = phy_cts_pixel_clk_khz;
+ } else {
+ ctrl->link->link_params.bw_code =
+ drm_dp_link_rate_to_bw_code(rate);
+ ctrl->link->link_params.lane_count =
+ ctrl->panel->link_info.num_lanes;
+ ctrl->pixel_rate = ctrl->panel->pinfo.pixel_clk_khz;
+ }
+
+ pr_debug("bw_code=%d, lane_count=%d, pixel_rate=%d\n",
+ ctrl->link->link_params.bw_code,
+ ctrl->link->link_params.lane_count, ctrl->pixel_rate);
+
+ ctrl->catalog->phy_lane_cfg(ctrl->catalog,
+ ctrl->orientation, ctrl->link->link_params.lane_count);
+
+ rc = dp_ctrl_enable_mainlink_clocks(ctrl);
+ if (rc)
+ goto end;
+
+ reinit_completion(&ctrl->idle_comp);
Are you guaranteed that this doesn't race with other idle_comp uses (or
other
dp_ctrl_on callers)?
+
+ dp_ctrl_configure_source_params(ctrl);
+
+ while (--link_train_max_retries && !atomic_read(&ctrl->aborted)) {
+ ctrl->catalog->config_msa(ctrl->catalog,
+ drm_dp_bw_code_to_link_rate(
+ ctrl->link->link_params.bw_code),
+ ctrl->pixel_rate, dp_ctrl_use_fixed_nvid(ctrl));
+
+ rc = dp_ctrl_setup_main_link(ctrl, true);
+ if (!rc)
+ break;
+
+ /* try with lower link rate */
+ dp_ctrl_link_rate_down_shift(ctrl);
+
+ ctrl->catalog->mainlink_ctrl(ctrl->catalog, false);
+
+ dp_ctrl_disable_mainlink_clocks(ctrl);
Check return code
+ /* hw recommended delay before re-enabling clocks */
+ msleep(20);
+
+ dp_ctrl_enable_mainlink_clocks(ctrl);
Check return code
+ }
Same comment here as in dp_ctrl_link_maintenance(), we should pull out
the
common code and put it in a helper.
+
+ if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
This is racy, sink_request could change between the check above and
here.
+ dp_ctrl_send_phy_test_pattern(ctrl);
+
+ pr_debug("End-\n");
Please remove
+
+end:
+ return rc;
+}
+
+static void dp_ctrl_off(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+
+ if (!dp_ctrl)
+ return;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ ctrl->catalog->mainlink_ctrl(ctrl->catalog, false);
+ ctrl->catalog->reset(ctrl->catalog);
+
+ /* Make sure DP is disabled before clk disable */
+ wmb();
readl/wmb nit
+
+ dp_ctrl_disable_mainlink_clocks(ctrl);
Check and propagate return value
+
+ pr_debug("DP off done\n");
+}
+
+static void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+
+ if (!dp_ctrl)
+ return;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ ctrl->catalog->get_interrupt(ctrl->catalog);
This should return the isr instead of store it in ctrl->catalog->isr,
since that
could potentially be overwritten after this function acks the current
interrupts.
(Also, it's only used in this function, so there's no need to store
it). One you
remove the catalog indirection, hopefully there will be a bunch of
these cases
that become clear.
+
+ if (ctrl->catalog->isr & DP_CTRL_INTR_READY_FOR_VIDEO)
+ dp_ctrl_video_ready(ctrl);
+
+ if (ctrl->catalog->isr & DP_CTRL_INTR_IDLE_PATTERN_SENT)
+ dp_ctrl_idle_patterns_sent(ctrl);
As mentioned above, just call the completions directly.
+}
+
+struct dp_ctrl *dp_ctrl_get(struct dp_ctrl_in *in)
+{
+ int rc = 0;
+ struct dp_ctrl_private *ctrl;
+ struct dp_ctrl *dp_ctrl;
+
+ if (!in->dev || !in->panel || !in->aux ||
+ !in->link || !in->catalog) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ ctrl = devm_kzalloc(in->dev, sizeof(*ctrl), GFP_KERNEL);
+ if (!ctrl) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ init_completion(&ctrl->idle_comp);
+ init_completion(&ctrl->video_comp);
+
+ /* in parameters */
+ ctrl->parser = in->parser;
+ ctrl->panel = in->panel;
+ ctrl->power = in->power;
+ ctrl->aux = in->aux;
+ ctrl->link = in->link;
+ ctrl->catalog = in->catalog;
+ ctrl->dev = in->dev;
+
+ dp_ctrl = &ctrl->dp_ctrl;
+
+ /* out parameters */
+ dp_ctrl->init = dp_ctrl_host_init;
+ dp_ctrl->deinit = dp_ctrl_host_deinit;
+ dp_ctrl->on = dp_ctrl_on;
+ dp_ctrl->off = dp_ctrl_off;
+ dp_ctrl->push_idle = dp_ctrl_push_idle;
+ dp_ctrl->abort = dp_ctrl_abort;
+ dp_ctrl->isr = dp_ctrl_isr;
+ dp_ctrl->reset = dp_ctrl_reset;
+ dp_ctrl->handle_sink_request = dp_ctrl_handle_sink_request;
+
+ return dp_ctrl;
+error:
+ return ERR_PTR(rc);
+}
+
+void dp_ctrl_put(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+
+ if (!dp_ctrl)
+ return;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ devm_kfree(ctrl->dev, ctrl);
+}
Once again, these functions can be deleted. Just store the _necessary_
data in a
central struct (with substructs as necessary) and call functions
directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
b/drivers/gpu/drm/msm/dp/dp_ctrl.h
new file mode 100644
index 0000000..6ab221a
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2012-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_CTRL_H_
+#define _DP_CTRL_H_
+
+#include "dp_aux.h"
+#include "dp_panel.h"
+#include "dp_link.h"
+#include "dp_parser.h"
+#include "dp_power.h"
+#include "dp_catalog.h"
+
+struct dp_ctrl {
+ int (*init)(struct dp_ctrl *dp_ctrl, bool flip);
+ void (*deinit)(struct dp_ctrl *dp_ctrl);
+ int (*on)(struct dp_ctrl *dp_ctrl);
+ void (*off)(struct dp_ctrl *dp_ctrl);
+ void (*reset)(struct dp_ctrl *dp_ctrl);
+ void (*push_idle)(struct dp_ctrl *dp_ctrl);
+ void (*abort)(struct dp_ctrl *dp_ctrl);
+ void (*isr)(struct dp_ctrl *dp_ctrl);
+ void (*handle_sink_request)(struct dp_ctrl *dp_ctrl);
+};
Same comment as catalog, these should just be functions to be called
directly.
+
+struct dp_ctrl_in {
+ struct device *dev;
+ struct dp_panel *panel;
+ struct dp_aux *aux;
+ struct dp_link *link;
+ struct dp_parser *parser;
+ struct dp_power *power;
+ struct dp_catalog_ctrl *catalog;
+};
This struct is not needed, function arguments work just as well.
+
+struct dp_ctrl *dp_ctrl_get(struct dp_ctrl_in *in);
+void dp_ctrl_put(struct dp_ctrl *dp_ctrl);
+
+#endif /* _DP_CTRL_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
b/drivers/gpu/drm/msm/dp/dp_debug.c
new file mode 100644
index 0000000..5c0a8ce
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -0,0 +1,507 @@
+/*
+ * 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/debugfs.h>
+#include <drm/drm_connector.h>
+
+#include "dp_parser.h"
+#include "dp_power.h"
+#include "dp_catalog.h"
+#include "dp_aux.h"
+#include "dp_ctrl.h"
+#include "dp_debug.h"
+#include "dp_display.h"
+
+#define DEBUG_NAME "drm_dp"
+
+struct dp_debug_private {
+ struct dentry *root;
+
+ struct dp_usbpd *usbpd;
+ struct dp_link *link;
+ struct dp_panel *panel;
+ struct drm_connector **connector;
+ struct device *dev;
+
+ struct dp_debug dp_debug;
+};
+
+static ssize_t dp_debug_write_hpd(struct file *file,
+ const char __user *user_buff, size_t count, loff_t *ppos)
+{
+ struct dp_debug_private *debug = file->private_data;
+ char buf[SZ_8];
+ size_t len = 0;
+ int hpd;
+
+ if (!debug)
+ return -ENODEV;
+
+ if (*ppos)
+ return 0;
+
+ /* Leave room for termination char */
+ len = min_t(size_t, count, SZ_8 - 1);
+ if (copy_from_user(buf, user_buff, len))
+ goto end;
+
+ buf[len] = '\0';
+
+ if (kstrtoint(buf, 10, &hpd) != 0)
+ goto end;
+
+ if (debug->usbpd && debug->usbpd->connect)
+ debug->usbpd->connect(debug->usbpd, hpd);
+
+end:
+ return -len;
+}
+
+static ssize_t dp_debug_write_edid_modes(struct file *file,
+ const char __user *user_buff, size_t count, loff_t *ppos)
+{
+ struct dp_debug_private *debug = file->private_data;
+ char buf[SZ_32];
+ size_t len = 0;
+ int hdisplay = 0, vdisplay = 0, vrefresh = 0;
+
+ if (!debug)
+ return -ENODEV;
+
+ if (*ppos)
+ goto end;
+
+ /* Leave room for termination char */
+ len = min_t(size_t, count, SZ_32 - 1);
+ if (copy_from_user(buf, user_buff, len))
+ goto clear;
+
+ buf[len] = '\0';
+
+ if (sscanf(buf, "%d %d %d", &hdisplay, &vdisplay, &vrefresh) != 3)
+ goto clear;
+
+ if (!hdisplay || !vdisplay || !vrefresh)
+ goto clear;
+
+ debug->dp_debug.debug_en = true;
+ debug->dp_debug.hdisplay = hdisplay;
+ debug->dp_debug.vdisplay = vdisplay;
+ debug->dp_debug.vrefresh = vrefresh;
+ goto end;
+clear:
+ pr_debug("clearing debug modes\n");
+ debug->dp_debug.debug_en = false;
+end:
+ return len;
+}
+
+static ssize_t dp_debug_read_connected(struct file *file,
+ char __user *user_buff, size_t count, loff_t *ppos)
+{
+ struct dp_debug_private *debug = file->private_data;
+ char buf[SZ_8];
+ u32 len = 0;
+
+ if (!debug)
+ return -ENODEV;
+
+ if (*ppos)
+ return 0;
+
+ if (!debug->usbpd)
+ return -ENODEV;
+
+ len += snprintf(buf, SZ_8, "%d\n", debug->usbpd->hpd_high);
+
+ if (copy_to_user(user_buff, buf, len))
+ return -EFAULT;
+
+ *ppos += len;
+ return len;
+}
+
+static ssize_t dp_debug_read_edid_modes(struct file *file,
+ char __user *user_buff, size_t count, loff_t *ppos)
+{
+ struct dp_debug_private *debug = file->private_data;
+ char *buf;
+ u32 len = 0;
+ int rc = 0;
+ struct drm_connector *connector;
+ struct drm_display_mode *mode;
+
+ if (!debug) {
+ pr_err("invalid data\n");
+ rc = -ENODEV;
+ goto error;
+ }
+
+ connector = *debug->connector;
+
+ if (!connector) {
+ pr_err("connector is NULL\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ if (*ppos)
+ goto error;
+
+ buf = kzalloc(SZ_4K, GFP_KERNEL);
+ if (!buf) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ list_for_each_entry(mode, &connector->modes, head) {
+ len += snprintf(buf + len, SZ_4K - len,
+ "%s %d %d %d %d %d %d %d %d %d 0x%x\n",
+ mode->name, mode->vrefresh, mode->hdisplay,
+ mode->hsync_start, mode->hsync_end, mode->htotal,
+ mode->vdisplay, mode->vsync_start, mode->vsync_end,
+ mode->vtotal, mode->flags);
+ }
+
+ if (copy_to_user(user_buff, buf, len)) {
+ kfree(buf);
+ rc = -EFAULT;
+ goto error;
+ }
+
+ *ppos += len;
+ kfree(buf);
+
+ return len;
+error:
+ return rc;
+}
+
+static int dp_debug_check_buffer_overflow(int rc, int *max_size, int
*len)
+{
+ if (rc >= *max_size) {
+ pr_err("buffer overflow\n");
+ return -EINVAL;
+ }
+ *len += rc;
+ *max_size = SZ_4K - *len;
+
+ return 0;
+}
+
+static ssize_t dp_debug_read_info(struct file *file, char __user
*user_buff,
+ size_t count, loff_t *ppos)
+{
+ struct dp_debug_private *debug = file->private_data;
+ char *buf;
+ u32 len = 0, rc = 0;
+ u64 lclk = 0;
+ u32 max_size = SZ_4K;
+
+ if (!debug)
+ return -ENODEV;
+
+ if (*ppos)
+ return 0;
+
+ buf = kzalloc(SZ_4K, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ rc = snprintf(buf + len, max_size, "\tname = %s\n", DEBUG_NAME);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\tdp_panel\n\t\tmax_pclk_khz = %d\n",
+ debug->panel->max_pclk_khz);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\tdrm_dp_link\n\t\trate = %u\n",
+ debug->panel->link_info.rate);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tnum_lanes = %u\n",
+ debug->panel->link_info.num_lanes);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tcapabilities = %lu\n",
+ debug->panel->link_info.capabilities);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\tdp_panel_info:\n\t\tactive = %dx%d\n",
+ debug->panel->pinfo.h_active,
+ debug->panel->pinfo.v_active);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tback_porch = %dx%d\n",
+ debug->panel->pinfo.h_back_porch,
+ debug->panel->pinfo.v_back_porch);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tfront_porch = %dx%d\n",
+ debug->panel->pinfo.h_front_porch,
+ debug->panel->pinfo.v_front_porch);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tsync_width = %dx%d\n",
+ debug->panel->pinfo.h_sync_width,
+ debug->panel->pinfo.v_sync_width);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tactive_low = %dx%d\n",
+ debug->panel->pinfo.h_active_low,
+ debug->panel->pinfo.v_active_low);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\th_skew = %d\n",
+ debug->panel->pinfo.h_skew);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\trefresh rate = %d\n",
+ debug->panel->pinfo.refresh_rate);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tpixel clock khz = %d\n",
+ debug->panel->pinfo.pixel_clk_khz);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tbpp = %d\n",
+ debug->panel->pinfo.bpp);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ /* Link Information */
+ rc = snprintf(buf + len, max_size,
+ "\tdp_link:\n\t\ttest_requested = %d\n",
+ debug->link->sink_request);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tlane_count = %d\n", debug->link->link_params.lane_count);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tbw_code = %d\n", debug->link->link_params.bw_code);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ lclk = drm_dp_bw_code_to_link_rate(
+ debug->link->link_params.bw_code) * 1000;
+ rc = snprintf(buf + len, max_size,
+ "\t\tlclk = %lld\n", lclk);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tv_level = %d\n", debug->link->phy_params.v_level);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ rc = snprintf(buf + len, max_size,
+ "\t\tp_level = %d\n", debug->link->phy_params.p_level);
+ if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
+ goto error;
+
+ if (copy_to_user(user_buff, buf, len))
+ goto error;
+
+ *ppos += len;
+
+ kfree(buf);
+ return len;
+error:
+ kfree(buf);
+ return -EINVAL;
+}
+
+static const struct file_operations dp_debug_fops = {
+ .open = simple_open,
+ .read = dp_debug_read_info,
+};
+
+static const struct file_operations edid_modes_fops = {
+ .open = simple_open,
+ .read = dp_debug_read_edid_modes,
+ .write = dp_debug_write_edid_modes,
+};
+
+static const struct file_operations hpd_fops = {
+ .open = simple_open,
+ .write = dp_debug_write_hpd,
+};
+
+static const struct file_operations connected_fops = {
+ .open = simple_open,
+ .read = dp_debug_read_connected,
+};
edid, hpd, and connected can all be deleted. They're provided by core.
AFAICT, you can also infer the information from dp_debug, so I think
you can
just drop this entire file.
+
+static int dp_debug_init(struct dp_debug *dp_debug)
+{
+ int rc = 0;
+ struct dp_debug_private *debug = container_of(dp_debug,
+ struct dp_debug_private, dp_debug);
+ struct dentry *dir, *file, *edid_modes;
+ struct dentry *hpd, *connected;
+
+ dir = debugfs_create_dir(DEBUG_NAME, NULL);
+ if (IS_ERR_OR_NULL(dir)) {
+ rc = PTR_ERR(dir);
+ pr_err("[%s] debugfs create dir failed, rc = %d\n",
+ DEBUG_NAME, rc);
+ goto error;
+ }
+
+ file = debugfs_create_file("dp_debug", 0444, dir,
+ debug, &dp_debug_fops);
+ if (IS_ERR_OR_NULL(file)) {
+ rc = PTR_ERR(file);
+ pr_err("[%s] debugfs create file failed, rc=%d\n",
+ DEBUG_NAME, rc);
+ goto error_remove_dir;
+ }
+
+ edid_modes = debugfs_create_file("edid_modes", 0644, dir,
+ debug, &edid_modes_fops);
+ if (IS_ERR_OR_NULL(edid_modes)) {
+ rc = PTR_ERR(edid_modes);
+ pr_err("[%s] debugfs create edid_modes failed, rc=%d\n",
+ DEBUG_NAME, rc);
+ goto error_remove_dir;
+ }
+
+ hpd = debugfs_create_file("hpd", 0644, dir,
+ debug, &hpd_fops);
+ if (IS_ERR_OR_NULL(hpd)) {
+ rc = PTR_ERR(hpd);
+ pr_err("[%s] debugfs hpd failed, rc=%d\n",
+ DEBUG_NAME, rc);
+ goto error_remove_dir;
+ }
+
+ connected = debugfs_create_file("connected", 0444, dir,
+ debug, &connected_fops);
+ if (IS_ERR_OR_NULL(connected)) {
+ rc = PTR_ERR(connected);
+ pr_err("[%s] debugfs connected failed, rc=%d\n",
+ DEBUG_NAME, rc);
+ goto error_remove_dir;
+ }
+
+ debug->root = dir;
+ return rc;
+error_remove_dir:
+ debugfs_remove(dir);
+error:
+ return rc;
+}
+
+struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel
*panel,
+ struct dp_usbpd *usbpd, struct dp_link *link,
+ struct drm_connector **connector)
+{
+ int rc = 0;
+ struct dp_debug_private *debug;
+ struct dp_debug *dp_debug;
+
+ if (!dev || !panel || !usbpd || !link) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ debug = devm_kzalloc(dev, sizeof(*debug), GFP_KERNEL);
+ if (!debug) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ debug->dp_debug.debug_en = false;
+ debug->usbpd = usbpd;
+ debug->link = link;
+ debug->panel = panel;
+ debug->dev = dev;
+ debug->connector = connector;
+
+ dp_debug = &debug->dp_debug;
+ dp_debug->vdisplay = 0;
+ dp_debug->hdisplay = 0;
+ dp_debug->vrefresh = 0;
+
+ rc = dp_debug_init(dp_debug);
+ if (rc) {
+ devm_kfree(dev, debug);
+ goto error;
+ }
+
+ return dp_debug;
+error:
+ return ERR_PTR(rc);
+}
+
+static int dp_debug_deinit(struct dp_debug *dp_debug)
+{
+ struct dp_debug_private *debug;
+
+ if (!dp_debug)
+ return -EINVAL;
+
+ debug = container_of(dp_debug, struct dp_debug_private, dp_debug);
+
+ debugfs_remove_recursive(debug->root);
+
+ return 0;
+}
+
+void dp_debug_put(struct dp_debug *dp_debug)
+{
+ struct dp_debug_private *debug;
+
+ if (!dp_debug)
+ return;
+
+ debug = container_of(dp_debug, struct dp_debug_private, dp_debug);
+
+ dp_debug_deinit(dp_debug);
+
+ devm_kfree(debug->dev, debug);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h
b/drivers/gpu/drm/msm/dp/dp_debug.h
new file mode 100644
index 0000000..a6480e1
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_debug.h
@@ -0,0 +1,81 @@
+/*
+ * 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_DEBUG_H_
+#define _DP_DEBUG_H_
+
+#include "dp_panel.h"
+#include "dp_link.h"
+#include "dp_extcon.h"
+
+/**
+ * struct dp_debug
+ * @debug_en: specifies whether debug mode enabled
+ * @vdisplay: used to filter out vdisplay value
+ * @hdisplay: used to filter out hdisplay value
+ * @vrefresh: used to filter out vrefresh value
+ * @tpg_state: specifies whether tpg feature is enabled
+ */
+struct dp_debug {
+ bool debug_en;
+ int aspect_ratio;
+ int vdisplay;
+ int hdisplay;
+ int vrefresh;
+};
+
+#if defined(CONFIG_DEBUG_FS)
+
+/**
+ * dp_debug_get() - configure and get the DisplayPlot debug module
data
+ *
+ * @dev: device instance of the caller
+ * @panel: instance of panel module
+ * @usbpd: instance of usbpd module
+ * @link: instance of link module
+ * @connector: double pointer to display connector
+ * return: pointer to allocated debug module data
+ *
+ * This function sets up the debug module and provides a way
+ * for debugfs input to be communicated with existing modules
+ */
+struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel
*panel,
+ struct dp_usbpd *usbpd, struct dp_link *link,
+ struct drm_connector **connector);
+/**
+ * dp_debug_put()
+ *
+ * Cleans up dp_debug instance
+ *
+ * @dp_debug: instance of dp_debug
+ */
+void dp_debug_put(struct dp_debug *dp_debug);
+
+#else
+
+static inline
+struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel
*panel,
+ struct dp_usbpd *usbpd, struct dp_link *link,
+ struct drm_connector **connector)
+{
+ return ERR_PTR(-EINVAL);
+}
+
+static inline void dp_debug_put(struct dp_debug *dp_debug)
+{
+}
+
+#endif /* defined(CONFIG_DEBUG_FS) */
Drop this too
+
+#endif /* _DP_DEBUG_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
new file mode 100644
index 0000000..8c98399
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -0,0 +1,977 @@
+/*
+ * 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/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <linux/component.h>
+#include <linux/of_irq.h>
+
+#include "msm_drv.h"
+#include "dp_extcon.h"
+#include "dp_parser.h"
+#include "dp_power.h"
+#include "dp_catalog.h"
+#include "dp_aux.h"
+#include "dp_link.h"
+#include "dp_panel.h"
+#include "dp_ctrl.h"
+#include "dp_display.h"
+#include "dp_drm.h"
+#include "dp_debug.h"
+
+static struct msm_dp *g_dp_display;
+#define HPD_STRING_SIZE 30
+
+struct dp_display_private {
+ char *name;
+ int irq;
+
+ /* state variables */
+ bool core_initialized;
+ bool power_on;
+ bool hpd_irq_on;
+ bool audio_supported;
+
+ struct platform_device *pdev;
+ struct dentry *root;
+ struct completion notification_comp;
+
+ struct dp_usbpd *usbpd;
+ struct dp_parser *parser;
+ struct dp_power *power;
+ struct dp_catalog *catalog;
+ struct dp_aux *aux;
+ struct dp_link *link;
+ struct dp_panel *panel;
+ struct dp_ctrl *ctrl;
+ struct dp_debug *debug;
+
+ struct dp_usbpd_cb usbpd_cb;
+ struct dp_display_mode mode;
+ struct msm_dp dp_display;
+
+};
+
+static const struct of_device_id dp_dt_match[] = {
+ {.compatible = "qcom,dp-display"},
+ {}
+};
+
+static irqreturn_t dp_display_irq(int irq, void *dev_id)
+{
+ struct dp_display_private *dp = dev_id;
+
+ if (!dp) {
+ pr_err("invalid data\n");
+ return IRQ_NONE;
+ }
+
+ /* DP controller isr */
+ dp->ctrl->isr(dp->ctrl);
+
+ /* DP aux isr */
+ dp->aux->isr(dp->aux);
+
+ return IRQ_HANDLED;
+}
+
+static int dp_display_bind(struct device *dev, struct device *master,
+ void *data)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+ struct drm_device *drm;
+ struct msm_drm_private *priv;
+ struct platform_device *pdev = to_platform_device(dev);
+
+ if (!dev || !pdev || !master) {
+ pr_err("invalid param(s), dev %pK, pdev %pK, master %pK\n",
+ dev, pdev, master);
+ rc = -EINVAL;
+ goto end;
+ }
+
+ drm = dev_get_drvdata(master);
+ dp = platform_get_drvdata(pdev);
+ if (!drm || !dp) {
+ pr_err("invalid param(s), drm %pK, dp %pK\n",
+ drm, dp);
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp->dp_display.drm_dev = drm;
+ priv = drm->dev_private;
+ priv->dp = &(dp->dp_display);
+
+ rc = dp->parser->parse(dp->parser);
+ if (rc) {
+ pr_err("device tree parsing failed\n");
+ goto end;
+ }
+
+ rc = dp->aux->drm_aux_register(dp->aux);
+ if (rc) {
+ pr_err("DRM DP AUX register failed\n");
+ goto end;
+ }
+
+ rc = dp->power->power_client_init(dp->power);
+ if (rc) {
+ pr_err("Power client create failed\n");
+ goto end;
+ }
+
+end:
+ return rc;
+}
+
+static void dp_display_unbind(struct device *dev, struct device
*master,
+ void *data)
+{
+ struct dp_display_private *dp;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct drm_device *drm = dev_get_drvdata(master);
+ struct msm_drm_private *priv = drm->dev_private;
+
+ if (!dev || !pdev) {
+ pr_err("invalid param(s)\n");
+ return;
+ }
+
+ dp = platform_get_drvdata(pdev);
+ if (!dp) {
+ pr_err("Invalid params\n");
+ return;
+ }
+
+ (void)dp->power->power_client_deinit(dp->power);
+ (void)dp->aux->drm_aux_deregister(dp->aux);
+ priv->dp = NULL;
+}
+
+static const struct component_ops dp_display_comp_ops = {
+ .bind = dp_display_bind,
+ .unbind = dp_display_unbind,
+};
+
+static bool dp_display_is_ds_bridge(struct dp_panel *panel)
+{
+ return (panel->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+ DP_DWN_STRM_PORT_PRESENT);
+}
+
+static bool dp_display_is_sink_count_zero(struct dp_display_private
*dp)
+{
+ return dp_display_is_ds_bridge(dp->panel) &&
+ (dp->link->sink_count.count == 0);
+}
+
+static void dp_display_send_hpd_event(struct msm_dp *dp_display)
+{
+ struct dp_display_private *dp;
+ struct drm_connector *connector;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+ if (!dp) {
+ pr_err("invalid params\n");
+ return;
+ }
+
+ connector = dp->dp_display.connector;
+ drm_helper_hpd_irq_event(connector->dev);
+}
+
+static int dp_display_send_hpd_notification(struct dp_display_private
*dp,
+ bool hpd)
This function is called from a few different threads with no locking.
It seems
like there's good opportunity for is_connected and notification_comp to
get out
of sync.
Additionally, is_connected is set to true before the function returns
(and
enable has been completed). So is that going to cause problems when we
call
detect/get_modes where is_connected == true and the hw is not up?
Perhaps I'm misreading this function, but it seems like the plan is to
block
until userspace has been notified and does a full modeset (ie: enable)?
If that
is the case, we usually don't want to have the hardware lit up until
it's
actually going to be used. In these cases, we'll wrap get_modes in
enable/disable (with proper locking), and wait for modeset to leave it
on.
+{
+ if ((hpd && dp->dp_display.is_connected) ||
+ (!hpd && !dp->dp_display.is_connected)) {
+ pr_info("HPD already %s\n", (hpd ? "on" : "off"));
+ return 0;
+ }
+
+ /* reset video pattern flag on disconnect */
+ if (!hpd)
+ dp->panel->video_test = false;
+
+ dp->dp_display.is_connected = hpd;
+ reinit_completion(&dp->notification_comp);
+ dp_display_send_hpd_event(&dp->dp_display);
+
+ if (!wait_for_completion_timeout(&dp->notification_comp, HZ * 2)) {
+ pr_warn("%s timeout\n", hpd ? "connect" : "disconnect");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int dp_display_process_hpd_high(struct dp_display_private *dp)
+{
+ int rc = 0;
+ struct edid *edid;
+
+ dp->aux->init(dp->aux, dp->parser->aux_cfg);
+
+ if (dp->link->psm_enabled)
+ goto notify;
+
+ rc = dp->panel->read_sink_caps(dp->panel, dp->dp_display.connector);
+ if (rc)
+ goto notify;
+
+ dp->link->process_request(dp->link);
+
+ if (dp_display_is_sink_count_zero(dp)) {
+ pr_debug("no downstream devices connected\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ edid = dp->panel->edid;
+
+ dp->audio_supported = drm_detect_monitor_audio(edid);
+
+ dp->panel->handle_sink_request(dp->panel);
+
+ dp->dp_display.max_pclk_khz = dp->parser->max_pclk_khz;
+notify:
+ dp_display_send_hpd_notification(dp, true);
+
+end:
+ return rc;
+}
+
+static void dp_display_host_init(struct dp_display_private *dp)
+{
+ bool flip = false;
+
+ if (dp->core_initialized) {
+ pr_debug("DP core already initialized\n");
+ return;
+ }
+
+ if (dp->usbpd->orientation == ORIENTATION_CC2)
+ flip = true;
+
+ dp->power->init(dp->power, flip);
+ dp->ctrl->init(dp->ctrl, flip);
+ enable_irq(dp->irq);
+ dp->core_initialized = true;
+}
+
+static void dp_display_host_deinit(struct dp_display_private *dp)
+{
+ if (!dp->core_initialized) {
+ pr_debug("DP core already off\n");
+ return;
+ }
+
+ dp->ctrl->deinit(dp->ctrl);
+ dp->power->deinit(dp->power);
+ disable_irq(dp->irq);
+ dp->core_initialized = false;
+}
+
+static void dp_display_process_hpd_low(struct dp_display_private *dp)
+{
+ /* cancel any pending request */
+ dp->ctrl->abort(dp->ctrl);
+
+ dp_display_send_hpd_notification(dp, false);
+
+ dp->aux->deinit(dp->aux);
+}
+
+static int dp_display_usbpd_configure_cb(struct device *dev)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dev) {
+ pr_err("invalid dev\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp = dev_get_drvdata(dev);
+ if (!dp) {
+ pr_err("no driver data found\n");
+ rc = -ENODEV;
+ goto end;
+ }
+
+ dp_display_host_init(dp);
+
+ if (dp->usbpd->hpd_high)
+ dp_display_process_hpd_high(dp);
+end:
+ return rc;
+}
+
+static void dp_display_clean(struct dp_display_private *dp)
+{
+ dp->ctrl->push_idle(dp->ctrl);
+ dp->ctrl->off(dp->ctrl);
+}
+
+static int dp_display_usbpd_disconnect_cb(struct device *dev)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dev) {
+ pr_err("invalid dev\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp = dev_get_drvdata(dev);
+ if (!dp) {
+ pr_err("no driver data found\n");
+ rc = -ENODEV;
+ goto end;
+ }
+
+ /* cancel any pending request */
+ dp->ctrl->abort(dp->ctrl);
+
+ rc = dp_display_send_hpd_notification(dp, false);
+
+ /* if cable is disconnected, reset psm_enabled flag */
+ if (!dp->usbpd->alt_mode_cfg_done)
+ dp->link->psm_enabled = false;
+
+ if ((rc < 0) && dp->power_on)
+ dp_display_clean(dp);
+
+ dp_display_host_deinit(dp);
+end:
+ return rc;
+}
+
+static void dp_display_handle_video_request(struct dp_display_private
*dp)
+{
+ if (dp->link->sink_request & DP_TEST_LINK_VIDEO_PATTERN) {
+ /* force disconnect followed by connect */
+ dp->usbpd->connect(dp->usbpd, false);
+ dp->panel->video_test = true;
+ dp->usbpd->connect(dp->usbpd, true);
+ dp->link->send_test_response(dp->link);
+ }
+}
+
+static int dp_display_handle_hpd_irq(struct dp_display_private *dp)
+{
+ if (dp->link->sink_request & DS_PORT_STATUS_CHANGED) {
+ dp_display_send_hpd_notification(dp, false);
+
+ if (dp_display_is_sink_count_zero(dp)) {
+ pr_debug("sink count is zero, nothing to do\n");
+ return 0;
+ }
+
+ return dp_display_process_hpd_high(dp);
+ }
+
+ dp->ctrl->handle_sink_request(dp->ctrl);
+
+ dp_display_handle_video_request(dp);
+
+ return 0;
+}
+
+static int dp_display_usbpd_attention_cb(struct device *dev)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dev) {
+ pr_err("invalid dev\n");
+ return -EINVAL;
+ }
+
+ dp = dev_get_drvdata(dev);
+ if (!dp) {
+ pr_err("no driver data found\n");
+ return -ENODEV;
+ }
+
+ if (dp->usbpd->hpd_irq) {
+ dp->hpd_irq_on = true;
+
+ rc = dp->link->process_request(dp->link);
+ /* check for any test request issued by sink */
+ if (!rc)
+ dp_display_handle_hpd_irq(dp);
+
+ dp->hpd_irq_on = false;
+ goto end;
+ }
+
+ if (!dp->usbpd->hpd_high) {
+ dp_display_process_hpd_low(dp);
+ goto end;
+ }
+
+ if (dp->usbpd->alt_mode_cfg_done)
+ dp_display_process_hpd_high(dp);
+end:
+ return rc;
+}
+
+static void dp_display_deinit_sub_modules(struct dp_display_private
*dp)
+{
+ dp_ctrl_put(dp->ctrl);
+ dp_link_put(dp->link);
+ dp_panel_put(dp->panel);
+ dp_aux_put(dp->aux);
+ dp_power_put(dp->power);
+ dp_catalog_put(dp->catalog);
+ dp_parser_put(dp->parser);
+ dp_debug_put(dp->debug);
+}
+
+static int dp_init_sub_modules(struct dp_display_private *dp)
+{
+ int rc = 0;
+ struct device *dev = &dp->pdev->dev;
+ struct dp_usbpd_cb *cb = &dp->usbpd_cb;
+ struct dp_ctrl_in ctrl_in = {
+ .dev = dev,
+ };
+ struct dp_panel_in panel_in = {
+ .dev = dev,
+ };
+
+ /* Callback APIs used for cable status change event */
+ cb->configure = dp_display_usbpd_configure_cb;
+ cb->disconnect = dp_display_usbpd_disconnect_cb;
+ cb->attention = dp_display_usbpd_attention_cb;
+
+ dp->parser = dp_parser_get(dp->pdev);
+ if (IS_ERR(dp->parser)) {
+ rc = PTR_ERR(dp->parser);
+ pr_err("failed to initialize parser, rc = %d\n", rc);
+ dp->parser = NULL;
+ goto error_parser;
+ }
+
+ dp->catalog = dp_catalog_get(dev, &dp->parser->io);
+ if (IS_ERR(dp->catalog)) {
+ rc = PTR_ERR(dp->catalog);
+ pr_err("failed to initialize catalog, rc = %d\n", rc);
+ dp->catalog = NULL;
+ goto error_catalog;
+ }
+
+ dp->power = dp_power_get(dp->parser);
+ if (IS_ERR(dp->power)) {
+ rc = PTR_ERR(dp->power);
+ pr_err("failed to initialize power, rc = %d\n", rc);
+ dp->power = NULL;
+ goto error_power;
+ }
+
+ dp->aux = dp_aux_get(dev, &dp->catalog->aux, dp->parser->aux_cfg);
+ if (IS_ERR(dp->aux)) {
+ rc = PTR_ERR(dp->aux);
+ pr_err("failed to initialize aux, rc = %d\n", rc);
+ dp->aux = NULL;
+ goto error_aux;
+ }
+
+ dp->link = dp_link_get(dev, dp->aux);
+ if (IS_ERR(dp->link)) {
+ rc = PTR_ERR(dp->link);
+ pr_err("failed to initialize link, rc = %d\n", rc);
+ dp->link = NULL;
+ goto error_link;
+ }
+
+ panel_in.aux = dp->aux;
+ panel_in.catalog = &dp->catalog->panel;
+ panel_in.link = dp->link;
+
+ dp->panel = dp_panel_get(&panel_in);
+ if (IS_ERR(dp->panel)) {
+ rc = PTR_ERR(dp->panel);
+ pr_err("failed to initialize panel, rc = %d\n", rc);
+ dp->panel = NULL;
+ goto error_panel;
+ }
+
+ ctrl_in.link = dp->link;
+ ctrl_in.panel = dp->panel;
+ ctrl_in.aux = dp->aux;
+ ctrl_in.power = dp->power;
+ ctrl_in.catalog = &dp->catalog->ctrl;
+ ctrl_in.parser = dp->parser;
+
+ dp->ctrl = dp_ctrl_get(&ctrl_in);
+ if (IS_ERR(dp->ctrl)) {
+ rc = PTR_ERR(dp->ctrl);
+ pr_err("failed to initialize ctrl, rc = %d\n", rc);
+ dp->ctrl = NULL;
+ goto error_ctrl;
+ }
+
+ dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
+ dp->link, &dp->dp_display.connector);
+ if (IS_ERR(dp->debug)) {
+ rc = PTR_ERR(dp->debug);
+ pr_err("failed to initialize debug, rc = %d\n", rc);
+ dp->debug = NULL;
+ goto error_debug;
+ }
+
+ return rc;
+error_debug:
+ dp_ctrl_put(dp->ctrl);
+error_ctrl:
+ dp_panel_put(dp->panel);
+error_panel:
+ dp_link_put(dp->link);
+error_link:
+ dp_aux_put(dp->aux);
+error_aux:
+ dp_power_put(dp->power);
+error_power:
+ dp_catalog_put(dp->catalog);
+error_catalog:
+ dp_parser_put(dp->parser);
+error_parser:
+ return rc;
+}
+
+static int dp_display_set_mode(struct msm_dp *dp_display,
+ struct dp_display_mode *mode)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ dp->panel->pinfo = mode->timing;
+ dp->panel->init_info(dp->panel);
+error:
+ return rc;
+}
+
+static int dp_display_prepare(struct msm_dp *dp)
+{
+ return 0;
+}
+
+static int dp_display_enable(struct msm_dp *dp_display)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ if (dp->power_on) {
+ pr_debug("Link already setup, return\n");
+ return 0;
+ }
+
+ dp->aux->init(dp->aux, dp->parser->aux_cfg);
+
+ rc = dp->ctrl->on(dp->ctrl);
+ if (!rc)
+ dp->power_on = true;
+error:
+ return rc;
+}
+
+static int dp_display_post_enable(struct msm_dp *dp_display)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ complete_all(&dp->notification_comp);
+
+end:
+ return rc;
+}
+
+static int dp_display_pre_disable(struct msm_dp *dp_display)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ if (dp->usbpd->alt_mode_cfg_done && (dp->usbpd->hpd_high ||
+ dp->usbpd->forced_disconnect))
+ dp->link->psm_config(dp->link, &dp->panel->link_info, true);
+
+ dp->ctrl->push_idle(dp->ctrl);
+error:
+ return rc;
+}
+
+static int dp_display_disable(struct msm_dp *dp_display)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ if (!dp->power_on || !dp->core_initialized)
+ goto error;
+
+ dp->ctrl->off(dp->ctrl);
+
+ dp->power_on = false;
+
+ complete_all(&dp->notification_comp);
+error:
+ return rc;
+}
+
+static int dp_request_irq(struct msm_dp *dp_display)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+ if (dp->irq < 0) {
+ rc = dp->irq;
+ pr_err("failed to get irq: %d\n", rc);
+ return rc;
+ }
+
+ rc = devm_request_irq(&dp->pdev->dev, dp->irq, dp_display_irq,
+ IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
+ if (rc < 0) {
+ pr_err("failed to request IRQ%u: %d\n",
+ dp->irq, rc);
+ return rc;
+ }
+ disable_irq(dp->irq);
+
+ return 0;
+}
+
+static struct dp_debug *dp_get_debug(struct msm_dp *dp_display)
+{
+ struct dp_display_private *dp;
+
+ if (!dp_display) {
+ pr_err("invalid input\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ dp = container_of(dp_display, struct dp_display_private,
dp_display);
+
+ return dp->debug;
+}
+
+static int dp_display_unprepare(struct msm_dp *dp)
+{
+ return 0;
+}
+
+static int dp_display_validate_mode(struct msm_dp *dp, u32
mode_pclk_khz)
+{
+ const u32 num_components = 3, default_bpp = 24;
+ struct dp_display_private *dp_display;
+ struct drm_dp_link *link_info;
+ u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
+
+ if (!dp || !mode_pclk_khz || !dp->connector) {
+ pr_err("invalid params\n");
+ return -EINVAL;
+ }
+
+ dp_display = container_of(dp, struct dp_display_private,
dp_display);
+ link_info = &dp_display->panel->link_info;
+
+ mode_bpp = dp->connector->display_info.bpc * num_components;
+ if (!mode_bpp)
+ mode_bpp = default_bpp;
+
+ mode_bpp = dp_display->panel->get_mode_bpp(dp_display->panel,
+ mode_bpp, mode_pclk_khz);
+
+ mode_rate_khz = mode_pclk_khz * mode_bpp;
+ supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
+
+ if (mode_rate_khz > supported_rate_khz)
+ return MODE_BAD;
+
+ return MODE_OK;
+}
+
+static int dp_display_get_modes(struct msm_dp *dp,
+ struct dp_display_mode *dp_mode)
+{
+ struct dp_display_private *dp_display;
+ int ret = 0;
+
+ if (!dp) {
+ pr_err("invalid params\n");
+ return 0;
+ }
+
+ dp_display = container_of(dp, struct dp_display_private,
dp_display);
+
+ ret = dp_display->panel->get_modes(dp_display->panel,
+ dp->connector, dp_mode);
+ if (dp_mode->timing.pixel_clk_khz)
+ dp->max_pclk_khz = dp_mode->timing.pixel_clk_khz;
+ return ret;
+}
+
+static bool dp_display_check_video_test(struct msm_dp *dp)
+{
+ struct dp_display_private *dp_display;
+
+ if (!dp) {
+ pr_err("invalid params\n");
+ return false;
+ }
+
+ dp_display = container_of(dp, struct dp_display_private,
dp_display);
+
+ if (dp_display->panel->video_test)
+ return true;
+
+ return false;
+}
+
+static int dp_display_get_test_bpp(struct msm_dp *dp)
+{
+ struct dp_display_private *dp_display;
+
+ if (!dp) {
+ pr_err("invalid params\n");
+ return 0;
+ }
+
+ dp_display = container_of(dp, struct dp_display_private,
dp_display);
+
+ return dp_link_bit_depth_to_bpp(
+ dp_display->link->test_video.test_bit_depth);
+}
+
+static int dp_display_probe(struct platform_device *pdev)
+{
+ int rc = 0;
+ struct dp_display_private *dp;
+
+ if (!pdev || !pdev->dev.of_node) {
+ pr_err("pdev not found\n");
+ return -ENODEV;
+ }
+
+ dp = devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
+ if (!dp)
+ return -ENOMEM;
+
+ init_completion(&dp->notification_comp);
+
+ dp->pdev = pdev;
+ dp->name = "drm_dp";
+
+ rc = dp_init_sub_modules(dp);
+ if (rc) {
+ pr_err("init sub module failed\n");
+ devm_kfree(&pdev->dev, dp);
+ return -EPROBE_DEFER;
+ }
+
+ platform_set_drvdata(pdev, dp);
+
+ g_dp_display = &dp->dp_display;
+
+ g_dp_display->enable = dp_display_enable;
+ g_dp_display->post_enable = dp_display_post_enable;
+ g_dp_display->pre_disable = dp_display_pre_disable;
+ g_dp_display->disable = dp_display_disable;
+ g_dp_display->set_mode = dp_display_set_mode;
+ g_dp_display->validate_mode = dp_display_validate_mode;
+ g_dp_display->get_modes = dp_display_get_modes;
+ g_dp_display->prepare = dp_display_prepare;
+ g_dp_display->unprepare = dp_display_unprepare;
+ g_dp_display->request_irq = dp_request_irq;
+ g_dp_display->get_debug = dp_get_debug;
+ g_dp_display->send_hpd_event = dp_display_send_hpd_event;
+ g_dp_display->is_video_test = dp_display_check_video_test;
+ g_dp_display->get_test_bpp = dp_display_get_test_bpp;
+
+ rc = component_add(&pdev->dev, &dp_display_comp_ops);
+ if (rc) {
+ pr_err("component add failed, rc=%d\n", rc);
+ dp_display_deinit_sub_modules(dp);
+ devm_kfree(&pdev->dev, dp);
+ }
+
+ return rc;
+}
+
+int dp_display_get_displays(void **displays, int count)
+{
+ if (!displays) {
+ pr_err("invalid data\n");
+ return -EINVAL;
+ }
+
+ if (count != 1) {
+ pr_err("invalid number of displays\n");
+ return -EINVAL;
+ }
+
+ displays[0] = g_dp_display;
+ return count;
+}
+
+int dp_display_get_num_of_displays(void)
+{
+ return 1;
+}
These 2 functions are never used, and as such g_dp_display is never
used.
AFAICT, the callback hooks linked from g_dp_display also aren't called
from
anywhere else. So can you just dump these functions, the struct, and
all of the
unused callback hooks?
+
+static int dp_display_remove(struct platform_device *pdev)
+{
+ struct dp_display_private *dp;
+
+ if (!pdev)
+ return -EINVAL;
+
+ dp = platform_get_drvdata(pdev);
+
+ dp_display_deinit_sub_modules(dp);
+
+ platform_set_drvdata(pdev, NULL);
+ devm_kfree(&pdev->dev, dp);
+
+ return 0;
+}
+
+static struct platform_driver dp_display_driver = {
+ .probe = dp_display_probe,
+ .remove = dp_display_remove,
+ .driver = {
+ .name = "msm-dp-display",
+ .of_match_table = dp_dt_match,
+ },
+};
+
+int __init msm_dp_register(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&dp_display_driver);
+ if (ret) {
+ pr_err("driver register failed");
+ return ret;
+ }
+
+ return ret;
+}
+
+void __exit msm_dp_unregister(void)
+{
+ platform_driver_unregister(&dp_display_driver);
+}
+
+int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
*dev,
+ struct drm_encoder *encoder)
Where is this called?
+
+int dp_connector_post_init(struct drm_connector *connector, void
*display)
+{
+ struct msm_dp *dp_display = display;
+
+ if (!dp_display)
+ return -EINVAL;
+
+ dp_display->connector = connector;
+ return 0;
+}
This isn't called from anything?
+
+/**
+ * dp_connector_detect - callback to determine if connector is
connected
+ * @connector: Pointer to drm connector structure
+ * @force: Force detect setting from drm framework
+ * Returns: Connector 'is connected' status
+ */
+static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
+ bool force)
+{
+ enum drm_connector_status status = connector_status_unknown;
+ static enum drm_connector_status current_status =
connector_status_unknown;
+ struct msm_dp *dp;
+ struct dp_connector *dp_conn;
+ struct msm_drm_private *priv = conn->dev->dev_private;
+ struct msm_kms *kms = priv->kms;
+
+ dp_conn = to_dp_connector(conn);
+ dp = dp_conn->dp_display;
no need for dp_conn, just do
dp = to_dp_connector(conn)->dp_display;
+
+ pr_err("is_connected = %s\n",
+ (dp->is_connected) ? "true" : "false");
+
+ status = (dp->is_connected) ? connector_status_connected :
+ connector_status_disconnected;
+
+ if (dp->is_connected && dp->bridge->encoder
+ && (current_status != status)
This is always true.
+ && kms->funcs->set_encoder_mode) {
+ kms->funcs->set_encoder_mode(kms,
+ dp->bridge->encoder, false);
+ pr_err("call set_encoder_mode\n");
detect() should never do work, please remove.
+ }
+ current_status = status;
Why?
+ return status;
+}
+
+static void dp_connector_destroy(struct drm_connector *connector)
+{
+ struct dp_connector *dp_conn = to_dp_connector(connector);
+
+ DBG("");
+
+ drm_connector_cleanup(connector);
+
+ kfree(dp_conn);
Once dp_conn is devm managed, you can just remove this function and set
.destroy
to drm_connector_cleanup
+}
+
+void dp_connector_send_hpd_event(void *display)
Where is this called?
+{
+ struct msm_dp *dp;
+
+ if (!display) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ dp = display;
+
+ if (dp->send_hpd_event)
+ dp->send_hpd_event(dp);
+}
+
+/**
+ * dp_connector_get_modes - callback to add drm modes via
drm_mode_probed_add()
+ * @connector: Pointer to drm connector structure
+ * Returns: Number of modes added
+ */
+static int dp_connector_get_modes(struct drm_connector *connector)
+{
+ int rc = 0;
+ struct msm_dp *dp;
+ struct dp_display_mode *dp_mode = NULL;
+ struct drm_display_mode *m, drm_mode;
+ struct dp_connector *dp_conn;
+
+ if (!connector)
+ return 0;
+
+ dp_conn = to_dp_connector(connector);
+ dp = dp_conn->dp_display;
+
+ dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
+ if (!dp_mode)
+ return 0;
When you get rid of dp_display_mode, please use the stack for the new
drm_display_mode
+
+ /* pluggable case assumes EDID is read when HPD */
+ if (dp->is_connected) {
+ rc = dp->get_modes(dp, dp_mode);
Why is only one mode returned?
+ if (!rc)
This can also return -errno, you should check for that too.
+ pr_err("failed to get DP sink modes, rc=%d\n", rc);
Why continue if you know you failed?
+
+ if (dp_mode->timing.pixel_clk_khz) { /* valid DP mode */
+ memset(&drm_mode, 0x0, sizeof(drm_mode));
+ convert_to_drm_mode(dp_mode, &drm_mode);
+ m = drm_mode_duplicate(connector->dev, &drm_mode);
+ if (!m) {
+ pr_err("failed to add mode %ux%u\n",
+ drm_mode.hdisplay,
+ drm_mode.vdisplay);
+ kfree(dp_mode);
+ return 0;
+ }
+ m->width_mm = connector->display_info.width_mm;
+ m->height_mm = connector->display_info.height_mm;
You shouldn't overwrite these.
+ drm_mode_probed_add(connector, m);
+ }
+ } else {
+ pr_err("No sink connected\n");
This shouldn't be an error
+ }
+ kfree(dp_mode);
+
+ return rc;
+}
+
+int dp_drm_bridge_init(void *data, struct drm_encoder *encoder)
+{
+ int rc = 0;
+ struct dp_bridge *dp_bridge;
+ struct drm_device *dev;
+ struct msm_dp *dp_display = data;
+ struct msm_drm_private *priv = NULL;
+
+ dp_bridge = kzalloc(sizeof(*dp_bridge), GFP_KERNEL);
+ if (!dp_bridge) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ dev = dp_display->drm_dev;
+ dp_bridge->display = dp_display;
+ dp_bridge->base.funcs = &dp_bridge_ops;
+ dp_bridge->base.encoder = encoder;
+
+ priv = dev->dev_private;
+
+ rc = drm_bridge_attach(encoder, &dp_bridge->base, NULL);
+ if (rc) {
+ pr_err("failed to attach bridge, rc=%d\n", rc);
+ goto error_free_bridge;
+ }
+
+ rc = dp_display->request_irq(dp_display);
+ if (rc) {
+ pr_err("request_irq failed, rc=%d\n", rc);
+ goto error_free_bridge;
+ }
+
+ encoder->bridge = &dp_bridge->base;
+ dp_display->dp_bridge = dp_bridge;
+ dp_display->bridge = &dp_bridge->base;
+
+ return 0;
+error_free_bridge:
+ kfree(dp_bridge);
+error:
+ return rc;
+}
+
+void dp_drm_bridge_deinit(void *data)
+{
+ struct msm_dp *display = data;
+ struct dp_bridge *bridge = display->dp_bridge;
+
+ if (bridge && bridge->base.encoder)
+ bridge->base.encoder->bridge = NULL;
+
+ kfree(bridge);
+}
+
+/**
+ * dp_connector_mode_valid - callback to determine if specified mode
is valid
+ * @connector: Pointer to drm connector structure
+ * @mode: Pointer to drm mode structure
+ * Returns: Validity status for specified mode
+ */
+static enum drm_mode_status dp_connector_mode_valid(struct
drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ struct msm_dp *dp_disp;
+ struct dp_debug *debug;
+ struct dp_connector *dp_conn;
+
+ if (!mode || !connector) {
+ pr_err("invalid params\n");
+ return MODE_ERROR;
+ }
+
+ dp_conn = to_dp_connector(connector);
+ dp_disp = dp_conn->dp_display;
+ debug = dp_disp->get_debug(dp_disp);
+
+ mode->vrefresh = drm_mode_vrefresh(mode);
+
+ if (mode->clock > dp_disp->max_pclk_khz)
I think the value of max_pclk_khz is racy, you'll probably want to
audit its
use.
+ return MODE_BAD;
+
+ if (debug->debug_en && (mode->hdisplay != debug->hdisplay ||
+ mode->vdisplay != debug->vdisplay ||
+ mode->vrefresh != debug->vrefresh ||
+ mode->picture_aspect_ratio != debug->aspect_ratio))
+ return MODE_BAD;
Please drop this.
+
+ return dp_disp->validate_mode(dp_disp, mode->clock);
So I guess I was wrong when I said none of the g_dp_display functions
are being
used. I'm still unsure where they're set, perhaps there's some missing
code. At
any rate, what's the benefit of sticking all of that work into an
abstract
callback hook when it only has one implementation and one callsite.
All of that code can move in here (and the same goes for the other
callbacks
(move the code where it's used).
+}
+
+static const struct drm_connector_funcs dp_connector_funcs = {
+ .detect = dp_connector_detect,
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = 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 const struct drm_connector_helper_funcs
dp_connector_helper_funcs = {
+ .get_modes = dp_connector_get_modes,
+ .mode_valid = dp_connector_mode_valid,
+};
+
+/* connector initialization */
+struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display)
+{
+ struct drm_connector *connector = NULL;
+ struct dp_connector *dp_connector;
+ int ret;
+
+ dp_connector = kzalloc(sizeof(*dp_connector), GFP_KERNEL);
Use devm_kzalloc (you've already got a memory leak if
drm_connector_init fails
below).
+ if (!dp_connector)
+ return ERR_PTR(-ENOMEM);
+
+ dp_connector->dp_display = dp_display;
+
+ connector = &dp_connector->base;
+
+ ret = drm_connector_init(dp_display->drm_dev, connector,
&dp_connector_funcs,
+ DRM_MODE_CONNECTOR_DisplayPort);
+ if (ret)
+ return ERR_PTR(ret);
+
+ drm_connector_helper_add(connector, &dp_connector_helper_funcs);
+
+ /*
+ * Enable HPD to let hpd event is handled
+ * when cable is attached to the host.
These lines don't extend to 80 chars
+ */
+ connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+ /* Display driver doesn't support interlace now. */
+ connector->interlace_allowed = false;
+ connector->doublescan_allowed = false;
These are already false by virtue of kzalloc
+
+ drm_connector_attach_encoder(connector, dp_display->encoder);
+
+ return connector;
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h
b/drivers/gpu/drm/msm/dp/dp_drm.h
new file mode 100644
index 0000000..efbc1be
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -0,0 +1,52 @@
+/*
+ * 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_DRM_H_
+#define _DP_DRM_H_
+
+#include <linux/types.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+
+#include "msm_drv.h"
+#include "dp_display.h"
+
+struct dp_bridge {
+ struct drm_bridge base;
+ u32 id;
+
+ struct msm_dp *display;
+ struct dp_display_mode dp_mode;
+};
+
+/**
+ * dp_connector_post_init - callback to perform additional
initialization steps
+ * @connector: Pointer to drm connector structure
+ * @display: Pointer to private display handle
+ * Returns: Zero on success
+ */
+int dp_connector_post_init(struct drm_connector *connector, void
*display);
+
+void dp_connector_send_hpd_event(void *display);
+
+int dp_drm_bridge_init(void *display,
+ struct drm_encoder *encoder);
+
+void dp_drm_bridge_deinit(void *display);
+
+struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display);
+
+#endif /* _DP_DRM_H_ */
+
diff --git a/drivers/gpu/drm/msm/dp/dp_extcon.c
b/drivers/gpu/drm/msm/dp/dp_extcon.c
new file mode 100644
index 0000000..635c13b
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_extcon.c
@@ -0,0 +1,400 @@
+/*
+ * Copyright (c) 2012-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/slab.h>
+#include <linux/device.h>
+#include <linux/extcon.h>
+
+#include "dp_extcon.h"
+
+/* DP specific VDM commands */
+#define DP_USBPD_VDM_STATUS 0x10
+#define DP_USBPD_VDM_CONFIGURE 0x11
+
+/* USBPD-TypeC specific Macros */
+#define VDM_VERSION 0x0
+#define USB_C_DP_SID 0xFF01
+
+enum dp_usbpd_pin_assignment {
+ DP_USBPD_PIN_A,
+ DP_USBPD_PIN_B,
+ DP_USBPD_PIN_C,
+ DP_USBPD_PIN_D,
+ DP_USBPD_PIN_E,
+ DP_USBPD_PIN_F,
+ DP_USBPD_PIN_MAX,
+};
+
+struct dp_usbpd_capabilities {
+ enum dp_usbpd_port port;
+ bool receptacle_state;
+ u8 ulink_pin_config;
+ u8 dlink_pin_config;
+};
Where is this used?
+
+struct dp_extcon_private {
+ u32 vdo;
+ struct device *dev;
+ struct notifier_block extcon_nb;
+ struct extcon_dev *extcon;
+ struct workqueue_struct *extcon_wq;
+ struct work_struct event_work;
+ struct usbpd *pd;
+ struct dp_usbpd_cb *dp_cb;
+ struct dp_usbpd_capabilities cap;
+ struct dp_usbpd dp_usbpd;
+};
+
+static const char *dp_usbpd_pin_name(u8 pin)
+{
+ switch (pin) {
+ case DP_USBPD_PIN_A: return "DP_USBPD_PIN_ASSIGNMENT_A";
+ case DP_USBPD_PIN_B: return "DP_USBPD_PIN_ASSIGNMENT_B";
+ case DP_USBPD_PIN_C: return "DP_USBPD_PIN_ASSIGNMENT_C";
+ case DP_USBPD_PIN_D: return "DP_USBPD_PIN_ASSIGNMENT_D";
+ case DP_USBPD_PIN_E: return "DP_USBPD_PIN_ASSIGNMENT_E";
+ case DP_USBPD_PIN_F: return "DP_USBPD_PIN_ASSIGNMENT_F";
+ default: return "UNKNOWN";
+ }
+}
No need to have this just for one debug statement, just print the enum
value
with %d below.
+
+static const char *dp_usbpd_port_name(enum dp_usbpd_port port)
+{
+ switch (port) {
+ case DP_USBPD_PORT_NONE: return "DP_USBPD_PORT_NONE";
+ case DP_USBPD_PORT_UFP_D: return "DP_USBPD_PORT_UFP_D";
+ case DP_USBPD_PORT_DFP_D: return "DP_USBPD_PORT_DFP_D";
+ case DP_USBPD_PORT_D_UFP_D: return "DP_USBPD_PORT_D_UFP_D";
+ default: return "DP_USBPD_PORT_NONE";
+ }
+}
Same here.
+
+static void dp_usbpd_init_port(enum dp_usbpd_port *port, u32 in_port)
+{
+ switch (in_port) {
+ case 0:
+ *port = DP_USBPD_PORT_NONE;
+ break;
+ case 1:
+ *port = DP_USBPD_PORT_UFP_D;
+ break;
+ case 2:
+ *port = DP_USBPD_PORT_DFP_D;
+ break;
+ case 3:
+ *port = DP_USBPD_PORT_D_UFP_D;
+ break;
+ default:
+ *port = DP_USBPD_PORT_NONE;
+ }
+ pr_debug("port:%s\n", dp_usbpd_port_name(*port));
+}
+
+void dp_usbpd_get_capabilities(struct dp_extcon_private *pd)
Where is this called from?
+{
+ struct dp_usbpd_capabilities *cap = &pd->cap;
+ u32 buf = pd->vdo;
+ int port = buf & 0x3;
+
+ cap->receptacle_state = (buf & BIT(6)) ? true : false;
+ cap->dlink_pin_config = (buf >> 8) & 0xff;
+ cap->ulink_pin_config = (buf >> 16) & 0xff;
Please #define hardcoded values
+
+ dp_usbpd_init_port(&cap->port, port);
All of the values in cap->port seem to be hardcoded, so why not just
use the
#define's directly where applicable?
+}
+
+void dp_usbpd_get_status(struct dp_extcon_private *pd)
Where is this called?
+{
+ struct dp_usbpd *status = &pd->dp_usbpd;
+ u32 buf = pd->vdo;
+ int port = buf & 0x3;
+
+ status->low_pow_st = (buf & BIT(2)) ? true : false;
+ status->adaptor_dp_en = (buf & BIT(3)) ? true : false;
+ status->multi_func = (buf & BIT(4)) ? true : false;
+ status->usb_config_req = (buf & BIT(5)) ? true : false;
+ status->exit_dp_mode = (buf & BIT(6)) ? true : false;
+ status->hpd_high = (buf & BIT(7)) ? true : false;
+ status->hpd_irq = (buf & BIT(8)) ? true : false;
+
+ pr_debug("low_pow_st = %d, adaptor_dp_en = %d, multi_func = %d\n",
+ status->low_pow_st, status->adaptor_dp_en,
+ status->multi_func);
+ pr_debug("usb_config_req = %d, exit_dp_mode = %d, hpd_high =%d\n",
+ status->usb_config_req,
+ status->exit_dp_mode, status->hpd_high);
+ pr_debug("hpd_irq = %d\n", status->hpd_irq);
+
+ dp_usbpd_init_port(&status->port, port);
Only multi_func, hpd_high, and hpd_irq are used out of this struct. Can
you cull
the values down? hpd_irq is also only set to its hardcoded value, so
just use
that value directly where applicable.
+}
+
+u32 dp_usbpd_gen_config_pkt(struct dp_extcon_private *pd)
This function is also unused
+{
+ u8 pin_cfg, pin;
+ u32 config = 0;
+ const u32 ufp_d_config = 0x2, dp_ver = 0x1;
+
+ if (pd->cap.receptacle_state)
+ pin_cfg = pd->cap.ulink_pin_config;
+ else
+ pin_cfg = pd->cap.dlink_pin_config;
+
+ for (pin = DP_USBPD_PIN_A; pin < DP_USBPD_PIN_MAX; pin++) {
+ if (pin_cfg & BIT(pin)) {
+ if (pd->dp_usbpd.multi_func) {
+ if (pin == DP_USBPD_PIN_D)
+ break;
+ } else {
+ break;
+ }
+ }
+ }
+
+ if (pin == DP_USBPD_PIN_MAX)
+ pin = DP_USBPD_PIN_C;
+
+ pr_debug("pin assignment: %s\n", dp_usbpd_pin_name(pin));
+
+ config |= BIT(pin) << 8;
+
+ config |= (dp_ver << 2);
+ config |= ufp_d_config;
+
+ pr_debug("config = 0x%x\n", config);
+ return config;
+}
+
+static int dp_extcon_connect(struct dp_usbpd *dp_usbpd, bool hpd)
+{
+ int rc = 0;
+ struct dp_extcon_private *extcon;
+
+ if (!dp_usbpd) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
Just return -EINVAL and get rid of the error label and rc.
+ }
+
+ extcon = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
+
+ dp_usbpd->hpd_high = hpd;
+ dp_usbpd->forced_disconnect = !hpd;
If forced_disconnect is always !dp_usbpd->hpd_high, why not just use
!dp_usbpd->hpd_high directly?
+
+ if (hpd)
+ extcon->dp_cb->configure(extcon->dev);
+ else
+ extcon->dp_cb->disconnect(extcon->dev);
Do you need locking in this function/file?
+
+error:
+ return rc;
+}
+
+static int dp_extcon_get_lanes(struct dp_extcon_private *extcon_priv)
+{
+ union extcon_property_value property;
+ u8 lanes;
+
+ if (!extcon_priv || !extcon_priv->extcon) {
+ pr_err("Invalid input arguments\n");
+ return 0;
+ }
+
+ extcon_get_property(extcon_priv->extcon,
+ EXTCON_DISP_DP,
+ EXTCON_PROP_USB_SS,
+ &property);
+ lanes = ();
+
+ return lanes;
return (property.intval) ? 2 : 4;
and delete lanes
+}
+
+
+static void dp_extcon_event_work(struct work_struct *work)
+{
+ struct dp_extcon_private *extcon_priv;
+ int dp_state, ret;
+ int lanes;
+ union extcon_property_value property;
+
+ extcon_priv = container_of(work,
+ struct dp_extcon_private, event_work);
+
+ if (!extcon_priv || !extcon_priv->extcon) {
+ pr_err("Invalid extcon device handler\n");
+ return;
+ }
+
+ dp_state = extcon_get_state(extcon_priv->extcon,
+ EXTCON_DISP_DP);
Can fit on 1 line
+
+ if (dp_state > 0) {
+ ret = extcon_get_property(extcon_priv->extcon,
+ EXTCON_DISP_DP,
+ EXTCON_PROP_USB_TYPEC_POLARITY,
+ &property);
+ if (ret) {
+ pr_err("Get Polarity property failed\n");
+ return;
+ }
+ extcon_priv->dp_usbpd.orientation =
+ ((property.intval) ?
Can fit on 1 line, plus remove all () from this, they're not needed.
+ ORIENTATION_CC2 : ORIENTATION_CC1);
+
+ lanes = dp_extcon_get_lanes(extcon_priv);
+ if (!lanes)
+ return;
+ extcon_priv->dp_usbpd.multi_func =
+ ((lanes == 2) ? false : true);
This part is a bit confusing, you add a helper function to get the
lanes, and
interpret the USB_SS property to return 2 vs. 4, but then immediately
convert
that back into what the USB_SS property returns. Instead, remove the
helper
function and read USB_SS directly here.
+
+ if (extcon_priv->dp_cb && extcon_priv->dp_cb->configure)
I feel like this can never be false anyways, but you're not using
either of
these members in the conditional block. So why not just call
dp_extcon_connect
unconditionally?
+ dp_extcon_connect(&extcon_priv->dp_usbpd, true);
Check return value
+ } else {
+ if (extcon_priv->dp_cb && extcon_priv->dp_cb->disconnect)
Same here.
+ dp_extcon_connect(&extcon_priv->dp_usbpd, false);
Check return value
+ }
+}
+
+static int dp_extcon_create_workqueue(struct dp_extcon_private
*extcon_priv)
+{
+ extcon_priv->extcon_wq =
create_singlethread_workqueue("drm_dp_extcon");
What's the benefit of using a dedicated workqueue instead of the system
wq?
+ if (IS_ERR_OR_NULL(extcon_priv->extcon_wq)) {
+ pr_err("Error creating extcon wq\n");
+ return -EPERM;
+ }
+
+ INIT_WORK(&extcon_priv->event_work, dp_extcon_event_work);
+
+ return 0;
This function can't fail, so it should return void. In fact, this
really doesn't
need to be a separate function, roll it into dp_extcon_register().
+}
+
+static int dp_extcon_event_notify(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{
+ struct dp_extcon_private *extcon_priv;
+
+ extcon_priv = container_of(nb, struct dp_extcon_private,
+ extcon_nb);
+
+ queue_work(extcon_priv->extcon_wq, &extcon_priv->event_work);
+ return NOTIFY_DONE;
+}
+
+int dp_extcon_register(struct dp_usbpd *dp_usbpd)
+{
+ struct dp_extcon_private *extcon_priv;
+ int ret = 0;
+
+ if (!dp_usbpd)
+ return -EINVAL;
+
+ pr_err("Start++++");
Yikes, this doesn't look like an error!
+ extcon_priv = container_of(dp_usbpd, struct dp_extcon_private,
dp_usbpd);
+
+ extcon_priv->extcon_nb.notifier_call = dp_extcon_event_notify;
+ ret = devm_extcon_register_notifier(extcon_priv->dev,
extcon_priv->extcon,
+ EXTCON_DISP_DP,
+ &extcon_priv->extcon_nb);
+ if (ret) {
+ dev_err(extcon_priv->dev,
+ "register EXTCON_DISP_DP notifier err\n");
+ ret = -EINVAL;
Why keep going and create the workqueue if we have no notifier? This
should
probably be a return.
+ }
+
+ ret = dp_extcon_create_workqueue(extcon_priv);
+ if (ret) {
+ pr_err("Failed to create workqueue\n");
+ dp_extcon_unregister(dp_usbpd);
return ret?
+ }
+
+ pr_err("End----");
Same here
+ return ret;
+}
+
+void dp_extcon_unregister(struct dp_usbpd *dp_usbpd)
+{
+ struct dp_extcon_private *extcon_priv;
+
+ if (!dp_usbpd) {
+ pr_err("Invalid input\n");
+ return;
+ }
+
+ pr_err("Start++++");
remove.
+ extcon_priv = container_of(dp_usbpd, struct dp_extcon_private,
dp_usbpd);
+
+ devm_extcon_unregister_notifier(extcon_priv->dev,
extcon_priv->extcon,
+ EXTCON_DISP_DP,
+ &extcon_priv->extcon_nb);
+
+ if (extcon_priv->extcon_wq)
+ destroy_workqueue(extcon_priv->extcon_wq);
+
+ pr_err("End----");
remove.
+ return;
+}
+
+struct dp_usbpd *dp_extcon_get(struct device *dev, struct dp_usbpd_cb
*cb)
+{
+ int rc = 0;
+ struct dp_extcon_private *dp_extcon;
+ struct dp_usbpd *dp_usbpd;
+
+ if (!cb) {
+ pr_err("invalid cb data\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ pr_err("%s: Start", __func__);
remove.
+ dp_extcon = devm_kzalloc(dev, sizeof(*dp_extcon), GFP_KERNEL);
+ if (!dp_extcon) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ dp_extcon->extcon = extcon_get_edev_by_phandle(dev, 0);
+ if (!dp_extcon->extcon) {
+ pr_err("invalid extcon data\n");
+ rc = -EINVAL;
+ devm_kfree(dev, dp_extcon);
+ goto error;
+ }
+
+ dp_extcon->dev = dev;
+ dp_extcon->dp_cb = cb;
+
+ dp_extcon->dp_usbpd.connect = dp_extcon_connect;
+ dp_usbpd = &dp_extcon->dp_usbpd;
+
+ pr_err("%s: end", __func__);
remove.
+ return dp_usbpd;
+error:
This label isn't useful, return directly where applicable.
+ return ERR_PTR(rc);
+}
+
+void dp_extcon_put(struct dp_usbpd *dp_usbpd)
+{
+ struct dp_extcon_private *extcon;
+
+ if (!dp_usbpd)
+ return;
+
+ extcon = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
+
+ devm_kfree(extcon->dev, extcon);
+
+ pr_err("%s: end", __func__);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_extcon.h
b/drivers/gpu/drm/msm/dp/dp_extcon.h
new file mode 100644
index 0000000..1f83a18
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_extcon.h
@@ -0,0 +1,111 @@
+/*
+ * Copyright (c) 2012-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_EXTCON_H_
+#define _DP_EXTCON_H_
+
+//#include <linux/usb/usbpd.h>
+
+#include <linux/types.h>
+#include <linux/device.h>
+
+/**
+ * enum dp_usbpd_port - usb/dp port type
+ * @DP_USBPD_PORT_NONE: port not configured
+ * @DP_USBPD_PORT_UFP_D: Upstream Facing Port - DisplayPort
+ * @DP_USBPD_PORT_DFP_D: Downstream Facing Port - DisplayPort
+ * @DP_USBPD_PORT_D_UFP_D: Both UFP & DFP - DisplayPort
+ */
+
+enum dp_usbpd_port {
+ DP_USBPD_PORT_NONE,
+ DP_USBPD_PORT_UFP_D,
+ DP_USBPD_PORT_DFP_D,
+ DP_USBPD_PORT_D_UFP_D,
+};
+
+enum plug_orientation {
+ ORIENTATION_NONE,
+ ORIENTATION_CC1,
+ ORIENTATION_CC2,
+};
+
+/**
+ * struct dp_usbpd - DisplayPort status
+ *
+ * @port: port configured
+ * orientation: plug orientation configuration
+ * @low_pow_st: low power state
+ * @adaptor_dp_en: adaptor functionality enabled
+ * @multi_func: multi-function preferred
+ * @usb_config_req: request to switch to usb
+ * @exit_dp_mode: request exit from displayport mode
+ * @hpd_high: Hot Plug Detect signal is high.
+ * @hpd_irq: Change in the status since last message
+ * @alt_mode_cfg_done: bool to specify alt mode status
+ * @debug_en: bool to specify debug mode
+ * @connect: simulate disconnect or connect for debug mode
+ */
+struct dp_usbpd {
+ enum dp_usbpd_port port;
+ enum plug_orientation orientation;
+ bool low_pow_st;
+ bool adaptor_dp_en;
+ bool multi_func;
+ bool usb_config_req;
+ bool exit_dp_mode;
+ bool hpd_high;
+ bool hpd_irq;
+ bool alt_mode_cfg_done;
+ bool debug_en;
+ bool forced_disconnect;
+
+ int (*connect)(struct dp_usbpd *dp_usbpd, bool hpd);
+};
+
+/**
+ * struct dp_usbpd_cb - callback functions provided by the client
+ *
+ * @configure: called by usbpd module when PD communication has
+ * been completed and the usb peripheral has been configured on
+ * dp mode.
+ * @disconnect: notify the cable disconnect issued by usb.
+ * @attention: notify any attention message issued by usb.
+ */
+struct dp_usbpd_cb {
+ int (*configure)(struct device *dev);
+ int (*disconnect)(struct device *dev);
+ int (*attention)(struct device *dev);
+};
+
+/**
+ * dp_extcon_get() - setup usbpd module
+ *
+ * @dev: device instance of the caller
+ * @cb: struct containing callback function pointers.
+ *
+ * This function allows the client to initialize the usbpd
+ * module. The module will communicate with usb driver and
+ * handles the power delivery (PD) communication with the
+ * sink/usb device. This module will notify the client using
+ * the callback functions about the connection and status.
+ */
+struct dp_usbpd *dp_extcon_get(struct device *dev, struct dp_usbpd_cb
*cb);
+
+void dp_extcon_put(struct dp_usbpd *pd);
+
+int dp_extcon_register(struct dp_usbpd *dp_usbpd);
+void dp_extcon_unregister(struct dp_usbpd *dp_usbpd);
+
+#endif /* _DP_EXTCON_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
b/drivers/gpu/drm/msm/dp/dp_link.c
new file mode 100644
index 0000000..5ab1efc
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -0,0 +1,1549 @@
+/*
+ * Copyright (c) 2012-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 "dp_link.h"
+#include "dp_panel.h"
+
+enum dynamic_range {
+ DP_DYNAMIC_RANGE_RGB_VESA = 0x00,
+ DP_DYNAMIC_RANGE_RGB_CEA = 0x01,
+ DP_DYNAMIC_RANGE_UNKNOWN = 0xFFFFFFFF,
+};
These should be in drm_dp_helper.h
+
+enum audio_sample_rate {
+ AUDIO_SAMPLE_RATE_32_KHZ = 0x00,
+ AUDIO_SAMPLE_RATE_44_1_KHZ = 0x01,
+ AUDIO_SAMPLE_RATE_48_KHZ = 0x02,
+ AUDIO_SAMPLE_RATE_88_2_KHZ = 0x03,
+ AUDIO_SAMPLE_RATE_96_KHZ = 0x04,
+ AUDIO_SAMPLE_RATE_176_4_KHZ = 0x05,
+ AUDIO_SAMPLE_RATE_192_KHZ = 0x06,
+};
+
+enum audio_pattern_type {
+ AUDIO_TEST_PATTERN_OPERATOR_DEFINED = 0x00,
+ AUDIO_TEST_PATTERN_SAWTOOTH = 0x01,
+};
+
+struct dp_link_request {
+ u32 test_requested;
+ u32 test_link_rate;
+ u32 test_lane_count;
+};
+
+struct dp_link_private {
+ u32 prev_sink_count;
+ struct device *dev;
+ struct dp_aux *aux;
+ struct dp_link dp_link;
+
+ struct dp_link_request request;
+ u8 link_status[DP_LINK_STATUS_SIZE];
+};
+
+static char *dp_link_get_audio_test_pattern(u32 pattern)
+{
+ switch (pattern) {
+ case AUDIO_TEST_PATTERN_OPERATOR_DEFINED:
+ return DP_LINK_ENUM_STR(AUDIO_TEST_PATTERN_OPERATOR_DEFINED);
+ case AUDIO_TEST_PATTERN_SAWTOOTH:
+ return DP_LINK_ENUM_STR(AUDIO_TEST_PATTERN_SAWTOOTH);
+ default:
+ return "unknown";
+ }
+}
+
+static char *dp_link_get_audio_sample_rate(u32 rate)
+{
+ switch (rate) {
+ case AUDIO_SAMPLE_RATE_32_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_32_KHZ);
+ case AUDIO_SAMPLE_RATE_44_1_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_44_1_KHZ);
+ case AUDIO_SAMPLE_RATE_48_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_48_KHZ);
+ case AUDIO_SAMPLE_RATE_88_2_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_88_2_KHZ);
+ case AUDIO_SAMPLE_RATE_96_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_96_KHZ);
+ case AUDIO_SAMPLE_RATE_176_4_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_176_4_KHZ);
+ case AUDIO_SAMPLE_RATE_192_KHZ:
+ return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_192_KHZ);
+ default:
+ return "unknown";
+ }
+}
+
+static int dp_link_get_period(struct dp_link_private *link, int const
addr)
+{
+ int ret = 0;
+ u8 bp;
+ u8 data;
+ u32 const param_len = 0x1;
+ u32 const max_audio_period = 0xA;
+
+ /* TEST_AUDIO_PERIOD_CH_XX */
+ if (drm_dp_dpcd_read(link->aux->drm_aux, addr, &bp,
+ param_len) < param_len) {
+ pr_err("failed to read test_audio_period (0x%x)\n", addr);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ data = bp;
+
+ /* Period - Bits 3:0 */
+ data = data & 0xF;
+ if ((int)data > max_audio_period) {
+ pr_err("invalid test_audio_period_ch_1 = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = data;
+exit:
+ return ret;
+}
+
+static int dp_link_parse_audio_channel_period(struct dp_link_private
*link)
+{
+ int ret = 0;
+ struct dp_link_test_audio *req = &link->dp_link.test_audio;
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH1);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_1 = ret;
+ pr_debug("test_audio_period_ch_1 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH2);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_2 = ret;
+ pr_debug("test_audio_period_ch_2 = 0x%x\n", ret);
+
+ /* TEST_AUDIO_PERIOD_CH_3 (Byte 0x275) */
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH3);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_3 = ret;
+ pr_debug("test_audio_period_ch_3 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH4);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_4 = ret;
+ pr_debug("test_audio_period_ch_4 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH5);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_5 = ret;
+ pr_debug("test_audio_period_ch_5 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH6);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_6 = ret;
+ pr_debug("test_audio_period_ch_6 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH7);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_7 = ret;
+ pr_debug("test_audio_period_ch_7 = 0x%x\n", ret);
+
+ ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH8);
+ if (ret == -EINVAL)
+ goto exit;
+
+ req->test_audio_period_ch_8 = ret;
+ pr_debug("test_audio_period_ch_8 = 0x%x\n", ret);
+exit:
+ return ret;
+}
+
+static int dp_link_parse_audio_pattern_type(struct dp_link_private
*link)
+{
+ int ret = 0;
+ u8 bp;
+ u8 data;
+ int rlen;
+ int const param_len = 0x1;
+ int const max_audio_pattern_type = 0x1;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux,
+ DP_TEST_AUDIO_PATTERN_TYPE, &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read link audio mode data\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+
+ /* Audio Pattern Type - Bits 7:0 */
+ if ((int)data > max_audio_pattern_type) {
+ pr_err("invalid audio pattern type = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->dp_link.test_audio.test_audio_pattern_type = data;
+ pr_debug("audio pattern type = %s\n",
+ dp_link_get_audio_test_pattern(data));
+exit:
+ return ret;
+}
+
+static int dp_link_parse_audio_mode(struct dp_link_private *link)
+{
+ int ret = 0;
+ u8 bp;
+ u8 data;
+ int rlen;
+ int const param_len = 0x1;
+ int const max_audio_sampling_rate = 0x6;
+ int const max_audio_channel_count = 0x8;
+ int sampling_rate = 0x0;
+ int channel_count = 0x0;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_AUDIO_MODE,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read link audio mode data\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+
+ /* Sampling Rate - Bits 3:0 */
+ sampling_rate = data & 0xF;
+ if (sampling_rate > max_audio_sampling_rate) {
+ pr_err("sampling rate (0x%x) greater than max (0x%x)\n",
+ sampling_rate, max_audio_sampling_rate);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* Channel Count - Bits 7:4 */
+ channel_count = ((data & 0xF0) >> 4) + 1;
+ if (channel_count > max_audio_channel_count) {
+ pr_err("channel_count (0x%x) greater than max (0x%x)\n",
+ channel_count, max_audio_channel_count);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->dp_link.test_audio.test_audio_sampling_rate = sampling_rate;
+ link->dp_link.test_audio.test_audio_channel_count = channel_count;
+ pr_debug("sampling_rate = %s, channel_count = 0x%x\n",
+ dp_link_get_audio_sample_rate(sampling_rate), channel_count);
+exit:
+ return ret;
+}
+
+/**
+ * dp_parse_audio_pattern_params() - parses audio pattern parameters
from DPCD
+ * @link: Display Port Driver data
+ *
+ * Returns 0 if it successfully parses the audio link pattern
parameters.
+ */
+static int dp_link_parse_audio_pattern_params(struct dp_link_private
*link)
+{
+ int ret = 0;
+
+ ret = dp_link_parse_audio_mode(link);
+ if (ret)
+ goto exit;
+
+ ret = dp_link_parse_audio_pattern_type(link);
+ if (ret)
+ goto exit;
+
+ ret = dp_link_parse_audio_channel_period(link);
+
+exit:
+ return ret;
+}
+
+/**
+ * dp_link_is_video_pattern_valid() - validates the video pattern
+ * @pattern: video pattern requested by the sink
+ *
+ * Returns true if the requested video pattern is supported.
+ */
+static bool dp_link_is_video_pattern_valid(u32 pattern)
+{
+ switch (pattern) {
+ case DP_NO_TEST_PATTERN:
+ case DP_COLOR_RAMP:
+ case DP_BLACK_AND_WHITE_VERTICAL_LINES:
+ case DP_COLOR_SQUARE:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static char *dp_link_video_pattern_to_string(u32 test_video_pattern)
+{
+ switch (test_video_pattern) {
+ case DP_NO_TEST_PATTERN:
+ return DP_LINK_ENUM_STR(DP_NO_TEST_PATTERN);
+ case DP_COLOR_RAMP:
+ return DP_LINK_ENUM_STR(DP_COLOR_RAMP);
+ case DP_BLACK_AND_WHITE_VERTICAL_LINES:
+ return DP_LINK_ENUM_STR(DP_BLACK_AND_WHITE_VERTICAL_LINES);
+ case DP_COLOR_SQUARE:
+ return DP_LINK_ENUM_STR(DP_COLOR_SQUARE);
+ default:
+ return "unknown";
+ }
+}
+
+/**
+ * dp_link_is_dynamic_range_valid() - validates the dynamic range
+ * @bit_depth: the dynamic range value to be checked
+ *
+ * Returns true if the dynamic range value is supported.
+ */
+static bool dp_link_is_dynamic_range_valid(u32 dr)
+{
+ switch (dr) {
+ case DP_DYNAMIC_RANGE_RGB_VESA:
+ case DP_DYNAMIC_RANGE_RGB_CEA:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static char *dp_link_dynamic_range_to_string(u32 dr)
+{
+ switch (dr) {
+ case DP_DYNAMIC_RANGE_RGB_VESA:
+ return DP_LINK_ENUM_STR(DP_DYNAMIC_RANGE_RGB_VESA);
+ case DP_DYNAMIC_RANGE_RGB_CEA:
+ return DP_LINK_ENUM_STR(DP_DYNAMIC_RANGE_RGB_CEA);
+ case DP_DYNAMIC_RANGE_UNKNOWN:
+ default:
+ return "unknown";
+ }
+}
No need for this, just print the value in the debug statement.
+
+/**
+ * dp_link_is_bit_depth_valid() - validates the bit depth requested
+ * @bit_depth: bit depth requested by the sink
+ *
+ * Returns true if the requested bit depth is supported.
+ */
+static bool dp_link_is_bit_depth_valid(u32 tbd)
+{
+ /* DP_TEST_VIDEO_PATTERN_NONE is treated as invalid */
+ switch (tbd) {
+ case DP_TEST_BIT_DEPTH_6:
+ case DP_TEST_BIT_DEPTH_8:
+ case DP_TEST_BIT_DEPTH_10:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static char *dp_link_bit_depth_to_string(u32 tbd)
+{
+ switch (tbd) {
+ case DP_TEST_BIT_DEPTH_6:
+ return DP_LINK_ENUM_STR(DP_TEST_BIT_DEPTH_6);
+ case DP_TEST_BIT_DEPTH_8:
+ return DP_LINK_ENUM_STR(DP_TEST_BIT_DEPTH_8);
+ case DP_TEST_BIT_DEPTH_10:
+ return DP_LINK_ENUM_STR(DP_TEST_BIT_DEPTH_10);
+ case DP_TEST_BIT_DEPTH_UNKNOWN:
+ default:
+ return "unknown";
+ }
+}
Same thing here, just remove this and print the enum value.
+
+static int dp_link_parse_timing_params1(struct dp_link_private *link,
+ int const addr, int const len, u32 *val)
pass-by-value doesn't need const same comment applies elsewhere, please
sweep
the code for other instances.
+{
+ u8 bp[2];
+ int rlen;
+
+ if (len < 2)
If you're going to check len < 2, you should probably also check len >
2 to
prevent overflow. At this point, len can really only be 2, so you can
just remove
it entirely as an argument.
+ return -EINVAL;
+
+ /* Read the requested video link pattern (Byte 0x221). */
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, addr, bp, len);
+ if (rlen < len) {
+ pr_err("failed to read 0x%x\n", addr);
+ return -EINVAL;
+ }
+
+ *val = bp[1] | (bp[0] << 8);
+
+ return 0;
+}
+
+static int dp_link_parse_timing_params2(struct dp_link_private *link,
+ int const addr, int const len,
+ u32 *val1, u32 *val2)
+{
+ u8 bp[2];
+ int rlen;
+
+ if (len < 2)
Same comment here rt bounds checking
+ return -EINVAL;
+
+ /* Read the requested video link pattern (Byte 0x221). */
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, addr, bp, len);
+ if (rlen < len) {
+ pr_err("failed to read 0x%x\n", addr);
+ return -EINVAL;
+ }
+
+ *val1 = (bp[0] & BIT(7)) >> 7;
+ *val2 = bp[1] | ((bp[0] & 0x7F) << 8);
+
+ return 0;
+}
+
+static int dp_link_parse_timing_params3(struct dp_link_private *link,
+ int const addr, u32 *val)
+{
+ u8 bp;
+ u32 len = 1;
+ int rlen;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, addr, &bp, len);
+ if (rlen < 1) {
+ pr_err("failed to read 0x%x\n", addr);
+ return -EINVAL;
+ }
+ *val = bp;
+
+ return 0;
+}
Putting aside the uninformative names, these
dp_link_parse_timing_params*
can be combined into one pretty easily, like:
static int dp_link_read_u32(struct dp_link_private *link, unsigned int
offset,
u32 *val, int num_bytes)
{
int i;
ssize_t ret;
__be32 tmp_val;
for (i = 0; num_bytes > 0; i += sizeof(*val)) {
int to_read = min(sizeof(tmp_val), num_bytes);
tmp_val = 0;
ret = drm_dp_dpcd_read(link->aux->drm_aux, offset + i,
&tmp_val,
to_read);
if (rlen < 0) {
/* print error message here */
return rlen;
}
val[i] = be32_to_cpu(tmp_val);
num_bytes -= to_read;
}
return 0;
}
If you need to strip off the first bit, do it from the caller.
I didn't compile or test this, so it might need tweaking, but you get
the idea.
+
+/**
+ * dp_parse_video_pattern_params() - parses video pattern parameters
from DPCD
+ * @link: Display Port Driver data
+ *
+ * Returns 0 if it successfully parses the video link pattern and the
link
+ * bit depth requested by the sink and, and if the values parsed are
valid.
+ */
+static int dp_link_parse_video_pattern_params(struct dp_link_private
*link)
+{
+ int ret = 0;
+ int rlen;
ssize_t
+ u8 bp;
+ u8 data;
Why do you need data and bp? I think you only need one.
+ u32 dyn_range;
+ int const param_len = 0x1;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_PATTERN,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read link video pattern\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+
+ if (!dp_link_is_video_pattern_valid(data)) {
+ pr_err("invalid link video pattern = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->dp_link.test_video.test_video_pattern = data;
+ pr_debug("link video pattern = 0x%x (%s)\n",
+ link->dp_link.test_video.test_video_pattern,
+ dp_link_video_pattern_to_string(
+ link->dp_link.test_video.test_video_pattern));
+
+ /* Read the requested color bit depth and dynamic range (Byte 0x232)
*/
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_MISC0,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read link bit depth\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+
+ /* Dynamic Range */
+ dyn_range = (data & DP_TEST_DYNAMIC_RANGE_CEA) >> 3;
This mask isolates one bit from data
+ if (!dp_link_is_dynamic_range_valid(dyn_range)) {
and this function returns true if the input is 0 || 1
So remove this code, and remove dp_link_is_dynamic_range_valid.
+ pr_err("invalid link dynamic range = 0x%x", dyn_range);
+ ret = -EINVAL;
+ goto exit;
+ }
+ link->dp_link.test_video.test_dyn_range = dyn_range;
And now you can just assign this directly. You should probably stay
consistent
and store the value shifted like the other values you're storing.
+ pr_debug("link dynamic range = 0x%x (%s)\n",
+ link->dp_link.test_video.test_dyn_range,
+ dp_link_dynamic_range_to_string(
+ link->dp_link.test_video.test_dyn_range));
+
+ /* Color bit depth */
+ data &= DP_TEST_BIT_DEPTH_MASK;
+ if (!dp_link_is_bit_depth_valid(data)) {
+ pr_err("invalid link bit depth = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->dp_link.test_video.test_bit_depth = data;
+ pr_debug("link bit depth = 0x%x (%s)\n",
+ link->dp_link.test_video.test_bit_depth,
+ dp_link_bit_depth_to_string(
+ link->dp_link.test_video.test_bit_depth));
+
+ /* resolution timing params */
+ ret = dp_link_parse_timing_params1(link, DP_TEST_H_TOTAL_HI, 2,
+ &link->dp_link.test_video.test_h_total);
+ if (ret) {
+ pr_err("failed to parse test_h_total (DP_TEST_H_TOTAL_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_H_TOTAL = %d\n",
link->dp_link.test_video.test_h_total);
+
+ ret = dp_link_parse_timing_params1(link, DP_TEST_V_TOTAL_HI, 2,
+ &link->dp_link.test_video.test_v_total);
+ if (ret) {
+ pr_err("failed to parse test_v_total (DP_TEST_V_TOTAL_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_V_TOTAL = %d\n",
link->dp_link.test_video.test_v_total);
+
+ ret = dp_link_parse_timing_params1(link, DP_TEST_H_START_HI, 2,
+ &link->dp_link.test_video.test_h_start);
+ if (ret) {
+ pr_err("failed to parse test_h_start (DP_TEST_H_START_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_H_START = %d\n",
link->dp_link.test_video.test_h_start);
+
+ ret = dp_link_parse_timing_params1(link, DP_TEST_V_START_HI, 2,
+ &link->dp_link.test_video.test_v_start);
+ if (ret) {
+ pr_err("failed to parse test_v_start (DP_TEST_V_START_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_V_START = %d\n",
link->dp_link.test_video.test_v_start);
+
+ ret = dp_link_parse_timing_params2(link, DP_TEST_HSYNC_HI, 2,
+ &link->dp_link.test_video.test_hsync_pol,
+ &link->dp_link.test_video.test_hsync_width);
+ if (ret) {
+ pr_err("failed to parse (DP_TEST_HSYNC_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_HSYNC_POL = %d\n",
+ link->dp_link.test_video.test_hsync_pol);
+ pr_debug("TEST_HSYNC_WIDTH = %d\n",
+ link->dp_link.test_video.test_hsync_width);
+
+ ret = dp_link_parse_timing_params2(link, DP_TEST_VSYNC_HI, 2,
+ &link->dp_link.test_video.test_vsync_pol,
+ &link->dp_link.test_video.test_vsync_width);
+ if (ret) {
+ pr_err("failed to parse (DP_TEST_VSYNC_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_VSYNC_POL = %d\n",
+ link->dp_link.test_video.test_vsync_pol);
+ pr_debug("TEST_VSYNC_WIDTH = %d\n",
+ link->dp_link.test_video.test_vsync_width);
+
+ ret = dp_link_parse_timing_params1(link, DP_TEST_H_WIDTH_HI, 2,
+ &link->dp_link.test_video.test_h_width);
+ if (ret) {
+ pr_err("failed to parse test_h_width (DP_TEST_H_WIDTH_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_H_WIDTH = %d\n",
link->dp_link.test_video.test_h_width);
+
+ ret = dp_link_parse_timing_params1(link, DP_TEST_V_HEIGHT_HI, 2,
+ &link->dp_link.test_video.test_v_height);
+ if (ret) {
+ pr_err("failed to parse test_v_height (DP_TEST_V_HEIGHT_HI)\n");
+ goto exit;
+ }
+ pr_debug("TEST_V_HEIGHT = %d\n",
+ link->dp_link.test_video.test_v_height);
+
+ ret = dp_link_parse_timing_params3(link, DP_TEST_MISC1,
+ &link->dp_link.test_video.test_rr_d);
+ link->dp_link.test_video.test_rr_d &= DP_TEST_REFRESH_DENOMINATOR;
+ if (ret) {
+ pr_err("failed to parse test_rr_d (DP_TEST_MISC1)\n");
+ goto exit;
+ }
+ pr_debug("TEST_REFRESH_DENOMINATOR = %d\n",
+ link->dp_link.test_video.test_rr_d);
+
+ ret = dp_link_parse_timing_params3(link,
DP_TEST_REFRESH_RATE_NUMERATOR,
+ &link->dp_link.test_video.test_rr_n);
+ if (ret) {
+ pr_err("failed to parse test_rr_n
(DP_TEST_REFRESH_RATE_NUMERATOR)\n");
+ goto exit;
+ }
+ pr_debug("TEST_REFRESH_NUMERATOR = %d\n",
+ link->dp_link.test_video.test_rr_n);
Instead of sprinkling all of these pr_debug messages around, just have
one big
debug at the end to print everything.
+exit:
This label isn't useful, just return directly above
+ return ret;
+}
+
+/**
+ * dp_link_parse_link_training_params() - parses link training
parameters from
+ * DPCD
+ * @link: Display Port Driver data
+ *
+ * Returns 0 if it successfully parses the link rate (Byte 0x219) and
lane
+ * count (Byte 0x220), and if these values parse are valid.
+ */
+static int dp_link_parse_link_training_params(struct dp_link_private
*link)
+{
+ u8 bp;
+ u8 data;
You don't need both bp and data
+ int ret = 0;
+ int rlen;
ssize_t
+ int const param_len = 0x1;
Just use 1 directly below.
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_LINK_RATE,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read link rate\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+
+ if (!is_link_rate_valid(data)) {
+ pr_err("invalid link rate = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->request.test_link_rate = data;
+ pr_debug("link rate = 0x%x\n", link->request.test_link_rate);
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_LANE_COUNT,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read lane count\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+ data = bp;
+ data &= 0x1F;
DP_MAX_LANE_COUNT_MASK
+
+ if (!is_lane_count_valid(data)) {
You might also want to data against max lanes
+ pr_err("invalid lane count = 0x%x\n", data);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ link->request.test_lane_count = data;
+ pr_debug("lane count = 0x%x\n", link->request.test_lane_count);
+exit:
+ return ret;
return directly above and remove exit + ret;
+}
+
+static bool dp_link_is_phy_test_pattern_supported(u32
phy_test_pattern_sel)
+{
+ switch (phy_test_pattern_sel) {
+ case DP_TEST_PHY_PATTERN_NONE:
+ case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING:
+ case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT:
+ case DP_TEST_PHY_PATTERN_PRBS7:
+ case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN:
+ case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN:
+ return true;
+ default:
+ return false;
+ }
+}
+
+/**
+ * dp_parse_phy_test_params() - parses the phy link parameters
+ * @link: Display Port Driver data
+ *
+ * Parses the DPCD (Byte 0x248) for the DP PHY link pattern that is
being
+ * requested.
+ */
+static int dp_link_parse_phy_test_params(struct dp_link_private
*link)
+{
+ u8 bp;
+ u8 data;
Only need one of these.
+ int rlen;
ssize_t
+ int const param_len = 0x1;
Just use 1 below
+ int ret = 0;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_PHY_PATTERN,
+ &bp, param_len);
+ if (rlen < param_len) {
FWIW, drm_dp_dpcd_read will return either len or an -errno. Partial
reads return
-EPROTO. So instead of checking that rlen < param_len, you can just do
rlen < 0.
Please also change all of the 1-byte reads over to using
drm_dp_dpcd_readb().
+ pr_err("failed to read phy link pattern\n");
print and return rlen.
+ ret = -EINVAL;
+ goto end;
+ }
+
+ data = bp;
+
+ link->dp_link.phy_params.phy_test_pattern_sel = data;
+
+ pr_debug("phy_test_pattern_sel = %s\n",
+ dp_link_get_phy_test_pattern(data));
+
+ if (!dp_link_is_phy_test_pattern_supported(data))
Just embed that code here.
+ ret = -EINVAL;
+end:
Remove label/ret and return directly
+ return ret;
+}
+
+static char *dp_link_get_test_name(u32 test_requested)
+{
+ switch (test_requested) {
+ case DP_TEST_LINK_TRAINING:
+ return DP_LINK_ENUM_STR(DP_TEST_LINK_TRAINING);
+ case DP_TEST_LINK_VIDEO_PATTERN:
+ return DP_LINK_ENUM_STR(DP_TEST_LINK_VIDEO_PATTERN);
+ case DP_TEST_LINK_EDID_READ:
+ return DP_LINK_ENUM_STR(DP_TEST_LINK_EDID_READ);
+ case DP_TEST_LINK_PHY_TEST_PATTERN:
+ return DP_LINK_ENUM_STR(DP_TEST_LINK_PHY_TEST_PATTERN);
+ case DP_TEST_LINK_AUDIO_PATTERN:
+ return DP_LINK_ENUM_STR(DP_TEST_LINK_AUDIO_PATTERN);
+ default:
+ return "unknown";
+ }
+}
Remove this and just print the enum.
+
+/**
+ * dp_link_is_video_audio_test_requested() - checks for audio/video
link request
+ * @link: link requested by the sink
+ *
+ * Returns true if the requested link is a permitted audio/video
link.
+ */
+static bool dp_link_is_video_audio_test_requested(u32 link)
+{
Should you be masking link to isolate these bits?
+ return (link == DP_TEST_LINK_VIDEO_PATTERN) ||
+ (link == (DP_TEST_LINK_AUDIO_PATTERN |
+ DP_TEST_LINK_VIDEO_PATTERN)) ||
+ (link == DP_TEST_LINK_AUDIO_PATTERN) ||
+ (link == (DP_TEST_LINK_AUDIO_PATTERN |
+ DP_TEST_LINK_AUDIO_DISABLED_VIDEO));
And once you mask the value, just check if any of the bits are set
instead of
enumerating all possible values.
+}
+
+/**
+ * dp_link_supported() - checks if link requested by sink is
supported
+ * @test_requested: link requested by the sink
+ *
+ * Returns true if the requested link is supported.
+ */
+static bool dp_link_is_test_supported(u32 test_requested)
+{
+ return (test_requested == DP_TEST_LINK_TRAINING) ||
+ (test_requested == DP_TEST_LINK_EDID_READ) ||
+ (test_requested == DP_TEST_LINK_PHY_TEST_PATTERN) ||
+ dp_link_is_video_audio_test_requested(test_requested);
I think this all equates to:
return test_requested != DP_TEST_LINK_FAUX_PATTERN;
and in that case, you can just remove this function and insert that
condition in
the if statement below.
+}
+
+static bool dp_link_is_test_edid_read(struct dp_link_private *link)
+{
+ return (link->request.test_requested == DP_TEST_LINK_EDID_READ);
+}
This doesn't warrant its own function. Also, should this be & instead
of ==?
+
+/**
+ * dp_sink_parse_test_request() - parses link request parameters from
sink
+ * @link: Display Port Driver data
+ *
+ * Parses the DPCD to check if an automated link is requested (Byte
0x201),
+ * and what type of link automation is being requested (Byte 0x218).
+ */
+static int dp_link_parse_request(struct dp_link_private *link)
+{
+ int ret = 0;
+ u8 bp;
+ u8 data;
Only need 1 of bp/data
+ int rlen;
ssize_t
+ u32 const param_len = 0x1;
Not needed, use drm_dp_dpcd_readb()
+
+ /**
+ * Read the device service IRQ vector (Byte 0x201) to determine
+ * whether an automated link has been requested by the sink.
+ */
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux,
+ DP_DEVICE_SERVICE_IRQ_VECTOR, &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("aux read failed\n");
Print error. This goes for other failure messages in this file.
+ ret = -EINVAL;
+ goto end;
+ }
+
+ data = bp;
+
+ pr_debug("device service irq vector = 0x%x\n", data);
+
+ if (!(data & DP_AUTOMATED_TEST_REQUEST)) {
+ pr_debug("no test requested\n");
+ return 0;
+ }
+
+ /**
+ * Read the link request byte (Byte 0x218) to determine what type
+ * of automated link has been requested by the sink.
+ */
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_REQUEST,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("aux read failed\n");
+ ret = -EINVAL;
+ goto end;
+ }
+
+ data = bp;
+
+ if (!dp_link_is_test_supported(data)) {
+ pr_debug("link 0x%x not supported\n", data);
+ goto end;
+ }
+
+ pr_debug("%s (0x%x) requested\n", dp_link_get_test_name(data),
data);
+ link->request.test_requested = data;
If test_requested is only used in the scope of this function, just use
a local
and pass it around. If test_requested is used outside the scope of this
function, you need locking.
+
+ if (link->request.test_requested == DP_TEST_LINK_PHY_TEST_PATTERN) {
+ ret = dp_link_parse_phy_test_params(link);
+ if (ret)
+ goto end;
+ ret = dp_link_parse_link_training_params(link);
Check the return here or it could get stomped
+ }
+
+ if (link->request.test_requested == DP_TEST_LINK_TRAINING)
+ ret = dp_link_parse_link_training_params(link);
Check the return here or it could get stomped
+
+ if (dp_link_is_video_audio_test_requested(
+ link->request.test_requested)) {
Should fit on 1 line, I think
+ ret = dp_link_parse_video_pattern_params(link);
+ if (ret)
+ goto end;
+
+ ret = dp_link_parse_audio_pattern_params(link);
Check the return here or it could get stomped
+ }
+end:
+ /**
/*
+ * Send a DP_TEST_ACK if all link parameters are valid, otherwise
send
+ * a DP_TEST_NAK.
+ */
+ if (ret) {
+ link->dp_link.test_response = DP_TEST_NAK;
+ } else {
+ if (!dp_link_is_test_edid_read(link))
+ link->dp_link.test_response = DP_TEST_ACK;
+ else
+ link->dp_link.test_response =
+ DP_TEST_EDID_CHECKSUM_WRITE;
+ }
+
+ return ret;
+}
+
+/**
+ * dp_link_parse_sink_count() - parses the sink count
+ *
+ * Parses the DPCD to check if there is an update to the sink count
+ * (Byte 0x200), and whether all the sink devices connected have
Content
+ * Protection enabled.
+ */
+static int dp_link_parse_sink_count(struct dp_link *dp_link)
+{
+ int rlen;
+ int const param_len = 0x1;
drm_dp_dpcd_readb
+ struct dp_link_private *link = container_of(dp_link,
+ struct dp_link_private, dp_link);
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_SINK_COUNT,
+ &link->dp_link.sink_count.count, param_len);
+ if (rlen < param_len) {
+ pr_err("failed to read sink count\n");
print ret
+ return -EINVAL;
return ret
+ }
+
+ link->dp_link.sink_count.cp_ready =
+ link->dp_link.sink_count.count & DP_SINK_CP_READY;
cp_ready is only used in this function, please don't store stuff like
this
beyond its scope.
+ /* BIT 7, BIT 5:0 */
Don't need this comment
+ link->dp_link.sink_count.count =
+ DP_GET_SINK_COUNT(link->dp_link.sink_count.count);
+
+ pr_debug("sink_count = 0x%x, cp_ready = 0x%x\n",
+ link->dp_link.sink_count.count,
+ link->dp_link.sink_count.cp_ready);
+ return 0;
+}
+
+static void dp_link_parse_sink_status_field(struct dp_link_private
*link)
+{
+ int len = 0;
+
+ link->prev_sink_count = link->dp_link.sink_count.count;
+ dp_link_parse_sink_count(&link->dp_link);
+
+ len = drm_dp_dpcd_read_link_status(link->aux->drm_aux,
+ link->link_status);
+ if (len < DP_LINK_STATUS_SIZE)
+ pr_err("DP link status read failed\n");
+ dp_link_parse_request(link);
+}
+
+static bool dp_link_is_link_training_requested(struct dp_link_private
*link)
+{
+ return (link->request.test_requested == DP_TEST_LINK_TRAINING);
+}
No need for dedicated function
+
+/**
+ * dp_link_process_link_training_request() - processes new training
requests
+ * @link: Display Port link data
+ *
+ * This function will handle new link training requests that are
initiated by
+ * the sink. In particular, it will update the requested lane count
and link
+ * link rate, and then trigger the link retraining procedure.
+ *
+ * The function will return 0 if a link training request has been
processed,
+ * otherwise it will return -EINVAL.
+ */
+static int dp_link_process_link_training_request(struct
dp_link_private *link)
+{
+ if (!dp_link_is_link_training_requested(link))
+ return -EINVAL;
+
+ pr_debug("%s link rate = 0x%x, lane count = 0x%x\n",
+ dp_link_get_test_name(DP_TEST_LINK_TRAINING),
+ link->request.test_link_rate,
+ link->request.test_lane_count);
+
+ link->dp_link.link_params.lane_count =
link->request.test_lane_count;
+ link->dp_link.link_params.bw_code = link->request.test_link_rate;
Should be using dp helper structs for this stuff
+
+ return 0;
+}
+
+static void dp_link_send_test_response(struct dp_link *dp_link)
+{
+ struct dp_link_private *link = NULL;
+ u32 const response_len = 0x1;
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ drm_dp_dpcd_write(link->aux->drm_aux, DP_TEST_RESPONSE,
+ &dp_link->test_response, response_len);
I strongly suspect you should have locking protecting test_response.
+}
+
+static int dp_link_psm_config(struct dp_link *dp_link,
+ struct drm_dp_link *link_info, bool enable)
This is another racy function where you could be simultaneously
powering up and
powering down the link. So you might want some locking.
+{
+ struct dp_link_private *link = NULL;
+ int ret = 0;
+
+ if (!dp_link) {
+ pr_err("invalid params\n");
+ return -EINVAL;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ if (enable)
+ ret = drm_dp_link_power_down(link->aux->drm_aux, link_info);
+ else
+ ret = drm_dp_link_power_up(link->aux->drm_aux, link_info);
+
+ if (ret)
+ pr_err("Failed to %s low power mode\n",
+ (enable ? "enter" : "exit"));
unneeded (). You could probably reword to fit on one line too
+ else
+ dp_link->psm_enabled = enable;
+
+ return ret;
+}
+
+static void dp_link_send_edid_checksum(struct dp_link *dp_link, u8
checksum)
+{
+ struct dp_link_private *link = NULL;
+ u32 const response_len = 0x1;
drm_dp_dpcd_readb. I'll stop commenting on these, just please go
through and
convert all 1-byte reads/writes
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ drm_dp_dpcd_write(link->aux->drm_aux, DP_TEST_EDID_CHECKSUM,
+ &checksum, response_len);
Check and return the result
+}
+
+static int dp_link_parse_vx_px(struct dp_link_private *link)
+{
+ u8 bp;
+ u8 data;
Only need one of these
+ int const param_len = 0x1;
drm_dp_dpcd_readb
+ int ret = 0;
+ u32 v0, p0, v1, p1, v2, p2, v3, p3;
+ int rlen;
ssize_t
+
+ pr_debug("\n");
remove
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux,
DP_ADJUST_REQUEST_LANE0_1,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed reading lanes 0/1\n");
print error
+ ret = -EINVAL;
+ goto end;
+ }
+
+ data = bp;
+
+ pr_debug("lanes 0/1 (Byte 0x206): 0x%x\n", data);
+
+ v0 = data & 0x3;
+ data = data >> 2;
+ p0 = data & 0x3;
+ data = data >> 2;
+
+ v1 = data & 0x3;
+ data = data >> 2;
+ p1 = data & 0x3;
+ data = data >> 2;
+
+ rlen = drm_dp_dpcd_read(link->aux->drm_aux,
DP_ADJUST_REQUEST_LANE2_3,
+ &bp, param_len);
+ if (rlen < param_len) {
+ pr_err("failed reading lanes 2/3\n");
+ ret = -EINVAL;
+ goto end;
+ }
+
+ data = bp;
+
+ pr_debug("lanes 2/3 (Byte 0x207): 0x%x\n", data);
+
+ v2 = data & 0x3;
+ data = data >> 2;
+ p2 = data & 0x3;
+ data = data >> 2;
+
+ v3 = data & 0x3;
+ data = data >> 2;
+ p3 = data & 0x3;
+ data = data >> 2;
I think you can use drm_dp_get_adjust_request_voltage() for all of
this.
+
+ pr_debug("vx: 0=%d, 1=%d, 2=%d, 3=%d\n", v0, v1, v2, v3);
+ pr_debug("px: 0=%d, 1=%d, 2=%d, 3=%d\n", p0, p1, p2, p3);
+
+ /**
+ * Update the voltage and pre-emphasis levels as per DPCD request
+ * vector.
+ */
+ pr_debug("Current: v_level = 0x%x, p_level = 0x%x\n",
+ link->dp_link.phy_params.v_level,
+ link->dp_link.phy_params.p_level);
+ pr_debug("Requested: v_level = 0x%x, p_level = 0x%x\n", v0, p0);
+ link->dp_link.phy_params.v_level = v0;
+ link->dp_link.phy_params.p_level = p0;
+
+ pr_debug("Success\n");
Remove
+end:
return directly everywhere and remove end & ret
+ return ret;
+}
+
+/**
+ * dp_link_process_phy_test_pattern_request() - process new phy link
requests
+ * @link: Display Port Driver data
+ *
+ * This function will handle new phy link pattern requests that are
initiated
+ * by the sink. The function will return 0 if a phy link pattern has
been
+ * processed, otherwise it will return -EINVAL.
+ */
+static int dp_link_process_phy_test_pattern_request(
+ struct dp_link_private *link)
+{
+ u32 test_link_rate = 0, test_lane_count = 0;
+
+ if (!(link->request.test_requested & DP_TEST_LINK_PHY_TEST_PATTERN))
{
Is test_requested always expected to have this value populated when
dp_link_process_request()?
+ pr_debug("no phy test\n");
+ return -EINVAL;
+ }
+
+ test_link_rate = link->request.test_link_rate;
+ test_lane_count = link->request.test_lane_count;
Just use these values directly instead of adding more locals.
+
+ if (!is_link_rate_valid(test_link_rate) ||
+ !is_lane_count_valid(test_lane_count)) {
+ pr_err("Invalid params: link rate = 0x%x, lane count = 0x%x\n",
+ test_link_rate, test_lane_count);
+ return -EINVAL;
+ }
+
+ pr_debug("start\n");
Remove
+
+ pr_info("Current: bw_code = 0x%x, lane count = 0x%x\n",
+ link->dp_link.link_params.bw_code,
+ link->dp_link.link_params.lane_count);
+
+ pr_info("Requested: bw_code = 0x%x, lane count = 0x%x\n",
+ test_link_rate, test_lane_count);
These should be debug level prints
+
+ link->dp_link.link_params.lane_count =
link->request.test_lane_count;
+ link->dp_link.link_params.bw_code = link->request.test_link_rate;
+
+ dp_link_parse_vx_px(link);
Check return value and propagate it.
+
+ pr_debug("end\n");
Remove
+
+ return 0;
+}
+
+static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE],
int r)
+{
+ return link_status[r - DP_LANE0_1_STATUS];
I don't think you're really supposed to reach into the link_status like
this.
How about adding a new helper to check for DP_LINK_STATUS_UPDATED and
one for
DP_DOWNSTREAM_PORT_STATUS_CHANGED?
+}
+
+/**
+ * dp_link_process_link_status_update() - processes link status
updates
+ * @link: Display Port link module data
+ *
+ * This function will check for changes in the link status, e.g.
clock
+ * recovery done on all lanes, and trigger link training if there is
a
+ * failure/error on the link.
+ *
+ * The function will return 0 if the a link status update has been
processed,
+ * otherwise it will return -EINVAL.
+ */
+static int dp_link_process_link_status_update(struct dp_link_private
*link)
+{
+ if (!(get_link_status(link->link_status,
DP_LANE_ALIGN_STATUS_UPDATED) &
+ DP_LINK_STATUS_UPDATED) || /* link status updated */
+ (drm_dp_clock_recovery_ok(link->link_status,
+ link->dp_link.link_params.lane_count) &&
+ drm_dp_channel_eq_ok(link->link_status,
+ link->dp_link.link_params.lane_count)))
Can you please fix the alignment here, it's tough to read.
+ return -EINVAL;
+
+ pr_debug("channel_eq_done = %d, clock_recovery_done = %d\n",
+ drm_dp_clock_recovery_ok(link->link_status,
+ link->dp_link.link_params.lane_count),
+ drm_dp_clock_recovery_ok(link->link_status,
+ link->dp_link.link_params.lane_count));
+
+ return 0;
+}
+
+static bool dp_link_is_ds_port_status_changed(struct dp_link_private
*link)
+{
+ if (get_link_status(link->link_status, DP_LANE_ALIGN_STATUS_UPDATED)
&
+ DP_DOWNSTREAM_PORT_STATUS_CHANGED) /* port status changed */
+ return true;
+
+ if (link->prev_sink_count != link->dp_link.sink_count.count)
It seems like this is running in a separate thread from where
prev_sink_count is
set, potential race here?
+ return true;
+
+ return false;
return link->prev_sink_count != link->dp_link.sink_count.count;
+}
+
+/**
+ * dp_link_process_downstream_port_status_change() - process port
status changes
+ * @link: Display Port Driver data
+ *
+ * This function will handle downstream port updates that are
initiated by
+ * the sink. If the downstream port status has changed, the EDID is
read via
+ * AUX.
+ *
+ * The function will return 0 if a downstream port update has been
+ * processed, otherwise it will return -EINVAL.
+ */
+static int dp_link_process_ds_port_status_change(struct
dp_link_private *link)
+{
+ if (!dp_link_is_ds_port_status_changed(link))
Just embed the contents of is_ds_port_status_changed here, no need for
2 funcs
+ return -EINVAL;
+
+ /* reset prev_sink_count */
+ link->prev_sink_count = link->dp_link.sink_count.count;
+
+ return 0;
+}
+
+static bool dp_link_is_video_pattern_requested(struct dp_link_private
*link)
+{
+ return (link->request.test_requested & DP_TEST_LINK_VIDEO_PATTERN)
+ && !(link->request.test_requested &
+ DP_TEST_LINK_AUDIO_DISABLED_VIDEO);
+}
+
+static bool dp_link_is_audio_pattern_requested(struct dp_link_private
*link)
+{
+ return (link->request.test_requested & DP_TEST_LINK_AUDIO_PATTERN);
+}
+
+/**
+ * dp_link_process_video_pattern_request() - process new video
pattern request
+ * @link: Display Port link module's data
+ *
+ * This function will handle a new video pattern request that are
initiated by
+ * the sink. This is acheieved by first sending a disconnect
notification to
+ * the sink followed by a subsequent connect notification to the user
modules,
+ * where it is expected that the user modules would draw the required
link
+ * pattern.
+ */
+static int dp_link_process_video_pattern_request(struct
dp_link_private *link)
+{
+ if (!dp_link_is_video_pattern_requested(link))
Just do this directly in dp_link_process_request and get rid of this
function
+ goto end;
+
+ pr_debug("%s: bit depth=%d(%d bpp) pattern=%s\n",
+ dp_link_get_test_name(DP_TEST_LINK_VIDEO_PATTERN),
+ link->dp_link.test_video.test_bit_depth,
+ dp_link_bit_depth_to_bpp(
+ link->dp_link.test_video.test_bit_depth),
+ dp_link_video_pattern_to_string(
+ link->dp_link.test_video.test_video_pattern));
+
+ return 0;
+end:
+ return -EINVAL;
+}
+
+/**
+ * dp_link_process_audio_pattern_request() - process new audio
pattern request
+ * @link: Display Port link module data
+ *
+ * This function will handle a new audio pattern request that is
initiated by
+ * the sink. This is acheieved by sending the necessary secondary
data packets
+ * to the sink. It is expected that any simulatenous requests for
video
+ * patterns will be handled before the audio pattern is sent to the
sink.
+ */
+static int dp_link_process_audio_pattern_request(struct
dp_link_private *link)
+{
+ if (!dp_link_is_audio_pattern_requested(link))
Same here
+ return -EINVAL;
+
+ pr_debug("sampling_rate=%s, channel_count=%d, pattern_type=%s\n",
+ dp_link_get_audio_sample_rate(
+ link->dp_link.test_audio.test_audio_sampling_rate),
+ link->dp_link.test_audio.test_audio_channel_count,
+ dp_link_get_audio_test_pattern(
+ link->dp_link.test_audio.test_audio_pattern_type));
+
+ pr_debug("audio_period: ch1=0x%x, ch2=0x%x, ch3=0x%x, ch4=0x%x\n",
+ link->dp_link.test_audio.test_audio_period_ch_1,
+ link->dp_link.test_audio.test_audio_period_ch_2,
+ link->dp_link.test_audio.test_audio_period_ch_3,
+ link->dp_link.test_audio.test_audio_period_ch_4);
+
+ pr_debug("audio_period: ch5=0x%x, ch6=0x%x, ch7=0x%x, ch8=0x%x\n",
+ link->dp_link.test_audio.test_audio_period_ch_5,
+ link->dp_link.test_audio.test_audio_period_ch_6,
+ link->dp_link.test_audio.test_audio_period_ch_7,
+ link->dp_link.test_audio.test_audio_period_ch_8);
+
+ return 0;
+}
+
+static void dp_link_reset_data(struct dp_link_private *link)
+{
+ link->request = (const struct dp_link_request){ 0 };
+ link->dp_link.test_video = (const struct dp_link_test_video){ 0 };
+ link->dp_link.test_video.test_bit_depth = DP_TEST_BIT_DEPTH_UNKNOWN;
+ link->dp_link.test_audio = (const struct dp_link_test_audio){ 0 };
+ link->dp_link.phy_params.phy_test_pattern_sel = 0;
+ link->dp_link.sink_request = 0;
+ link->dp_link.test_response = 0;
memset?
+}
+
+/**
+ * dp_link_process_request() - handle HPD IRQ transition to HIGH
+ * @link: pointer to link module data
+ *
+ * This function will handle the HPD IRQ state transitions from LOW
to HIGH
+ * (including cases when there are back to back HPD IRQ HIGH)
indicating
+ * the start of a new link training request or sink status update.
+ */
+static int dp_link_process_request(struct dp_link *dp_link)
+{
+ int ret = 0;
+ struct dp_link_private *link;
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ pr_debug("start\n");
+
+ dp_link_reset_data(link);
+
+ dp_link_parse_sink_status_field(link);
+
+ if (dp_link_is_test_edid_read(link)) {
+ dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
+ goto exit;
+ }
+
+ ret = dp_link_process_ds_port_status_change(link);
+ if (!ret) {
+ dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
+ goto exit;
+ }
+
+ ret = dp_link_process_link_training_request(link);
+ if (!ret) {
+ dp_link->sink_request |= DP_TEST_LINK_TRAINING;
+ goto exit;
+ }
+
+ ret = dp_link_process_phy_test_pattern_request(link);
+ if (!ret) {
+ dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
+ goto exit;
+ }
+
+ ret = dp_link_process_link_status_update(link);
+ if (!ret) {
+ dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
+ goto exit;
+ }
+
+ ret = dp_link_process_video_pattern_request(link);
+ if (!ret) {
+ dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
+ goto exit;
+ }
+
+ ret = dp_link_process_audio_pattern_request(link);
+ if (!ret) {
+ dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
+ goto exit;
+ }
+
+ pr_debug("done\n");
+exit:
remove exit & ret, just return values directly
+ return ret;
+}
+
+static int dp_link_get_colorimetry_config(struct dp_link *dp_link)
+{
+ u32 cc;
+ enum dynamic_range dr;
+ struct dp_link_private *link;
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ /* unless a video pattern CTS test is ongoing, use CEA_VESA */
s/CEA_VESA/RGB_VESA?
+ if (dp_link_is_video_pattern_requested(link))
+ dr = link->dp_link.test_video.test_dyn_range;
+ else
+ dr = DP_DYNAMIC_RANGE_RGB_VESA;
+
+ /* Only RGB_VESA nd RGB_CEA supported for now */
s/nd/and/
+ switch (dr) {
+ case DP_DYNAMIC_RANGE_RGB_CEA:
+ cc = BIT(3);
+ break;
+ case DP_DYNAMIC_RANGE_RGB_VESA:
+ default:
+ cc = 0;
+ }
If you store the value shifted like I suggested, you don't have to do
this
switch (which really should be an if anyways since the range is 0:1).
+
+ return cc;
+}
+
+static int dp_link_adjust_levels(struct dp_link *dp_link, u8
*link_status)
+{
+ int i;
+ int max = 0;
+ u8 data;
+ struct dp_link_private *link;
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ /* use the max level across lanes */
+ for (i = 0; i < dp_link->link_params.lane_count; i++) {
+ data = drm_dp_get_adjust_request_voltage(link_status, i);
+ pr_debug("lane=%d req_voltage_swing=%d\n", i, data);
+ if (max < data)
+ max = data;
+ }
+
+ dp_link->phy_params.v_level = max >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
+
+ /* use the max level across lanes */
+ max = 0;
+ for (i = 0; i < dp_link->link_params.lane_count; i++) {
+ data = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
+ pr_debug("lane=%d req_pre_emphasis=%d\n", i, data);
+ if (max < data)
+ max = data;
+ }
You can combine these loops and do the work at once (check out
intel_get_adjust_train)
+
+ dp_link->phy_params.p_level = max >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
+
+ /**
+ * Adjust the voltage swing and pre-emphasis level combination to
within
+ * the allowable range.
+ */
+ if (dp_link->phy_params.v_level > DP_LINK_VOLTAGE_MAX) {
+ pr_debug("Requested vSwingLevel=%d, change to %d\n",
+ dp_link->phy_params.v_level, DP_LINK_VOLTAGE_MAX);
+ dp_link->phy_params.v_level = DP_LINK_VOLTAGE_MAX;
+ }
+
+ if (dp_link->phy_params.p_level > DP_LINK_PRE_EMPHASIS_MAX) {
+ pr_debug("Requested preEmphasisLevel=%d, change to %d\n",
+ dp_link->phy_params.p_level, DP_LINK_PRE_EMPHASIS_MAX);
+ dp_link->phy_params.p_level = DP_LINK_PRE_EMPHASIS_MAX;
+ }
+
+ if ((dp_link->phy_params.p_level > DP_LINK_PRE_EMPHASIS_LEVEL_1)
+ && (dp_link->phy_params.v_level == DP_LINK_VOLTAGE_LEVEL_2)) {
+ pr_debug("Requested preEmphasisLevel=%d, change to %d\n",
+ dp_link->phy_params.p_level,
+ DP_LINK_PRE_EMPHASIS_LEVEL_1);
+ dp_link->phy_params.p_level = DP_LINK_PRE_EMPHASIS_LEVEL_1;
+ }
+
+ pr_debug("adjusted: v_level=%d, p_level=%d\n",
+ dp_link->phy_params.v_level, dp_link->phy_params.p_level);
+
+ return 0;
+}
+
+static int dp_link_send_psm_request(struct dp_link *dp_link, bool
req)
+{
+ struct dp_link_private *link;
+
+ if (!dp_link) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
This is a stub, why do this?
+
+ return 0;
Further, why have a stub at all? The hook is never called, so delete it
all
please.
+}
+
+static u32 dp_link_get_test_bits_depth(struct dp_link *dp_link, u32
bpp)
+{
+ u32 tbd;
+
+ /*
+ * Few simplistic rules and assumptions made here:
+ * 1. Test bit depth is bit depth per color component
+ * 2. Assume 3 color components
+ */
+ switch (bpp) {
+ case 18:
+ tbd = DP_TEST_BIT_DEPTH_6;
+ break;
+ case 24:
+ tbd = DP_TEST_BIT_DEPTH_8;
+ break;
+ case 30:
+ tbd = DP_TEST_BIT_DEPTH_10;
+ break;
+ default:
+ tbd = DP_TEST_BIT_DEPTH_UNKNOWN;
+ break;
+ }
+
+ if (tbd != DP_TEST_BIT_DEPTH_UNKNOWN)
+ tbd = (tbd >> DP_TEST_BIT_DEPTH_SHIFT);
AFAICT tbd corresponds to a SoC register value, as opposed to the DPCD
value.
It's a happy coincidence that they're the same, however I think you
should use a
separate msm-specific #define instead of the drm_dp_helper values.
+
+ return tbd;
+}
+
+struct dp_link *dp_link_get(struct device *dev, struct dp_aux *aux)
+{
+ int rc = 0;
+ struct dp_link_private *link;
+ struct dp_link *dp_link;
+
+ if (!dev || !aux) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ link = devm_kzalloc(dev, sizeof(*link), GFP_KERNEL);
+ if (!link) {
+ rc = -EINVAL;
+ goto error;
+ }
+
+ link->dev = dev;
+ link->aux = aux;
+
+ dp_link = &link->dp_link;
+
+ dp_link->process_request = dp_link_process_request;
+ dp_link->get_test_bits_depth = dp_link_get_test_bits_depth;
+ dp_link->get_colorimetry_config = dp_link_get_colorimetry_config;
+ dp_link->adjust_levels = dp_link_adjust_levels;
+ dp_link->send_psm_request = dp_link_send_psm_request;
+ dp_link->send_test_response = dp_link_send_test_response;
+ dp_link->psm_config = dp_link_psm_config;
+ dp_link->send_edid_checksum = dp_link_send_edid_checksum;
+
+ return dp_link;
+error:
+ return ERR_PTR(rc);
+}
+
+void dp_link_put(struct dp_link *dp_link)
+{
+ struct dp_link_private *link;
+
+ if (!dp_link)
+ return;
+
+ link = container_of(dp_link, struct dp_link_private, dp_link);
+
+ devm_kfree(link->dev, link);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_link.h
b/drivers/gpu/drm/msm/dp/dp_link.h
new file mode 100644
index 0000000..6d6ef43
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_link.h
@@ -0,0 +1,184 @@
+/*
+ * Copyright (c) 2012-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_LINK_H_
+#define _DP_LINK_H_
+
+#include "dp_aux.h"
+
+#define DS_PORT_STATUS_CHANGED 0x200
+#define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
+#define DP_LINK_ENUM_STR(x) #x
+
+enum dp_link_voltage_level {
+ DP_LINK_VOLTAGE_LEVEL_0 = 0,
+ DP_LINK_VOLTAGE_LEVEL_1 = 1,
+ DP_LINK_VOLTAGE_LEVEL_2 = 2,
+ DP_LINK_VOLTAGE_MAX = DP_LINK_VOLTAGE_LEVEL_2,
+};
DP_TRAIN_VOLTAGE_SWING_*
+
+enum dp_link_preemaphasis_level {
+ DP_LINK_PRE_EMPHASIS_LEVEL_0 = 0,
+ DP_LINK_PRE_EMPHASIS_LEVEL_1 = 1,
+ DP_LINK_PRE_EMPHASIS_LEVEL_2 = 2,
+ DP_LINK_PRE_EMPHASIS_MAX = DP_LINK_PRE_EMPHASIS_LEVEL_2,
+};
DP_TRAIN_PRE_EMPH*
+
+struct dp_link_sink_count {
+ u32 count;
+ bool cp_ready;
+};
+
+struct dp_link_test_video {
+ u32 test_video_pattern;
+ u32 test_bit_depth;
+ u32 test_dyn_range;
+ u32 test_h_total;
+ u32 test_v_total;
+ u32 test_h_start;
+ u32 test_v_start;
+ u32 test_hsync_pol;
+ u32 test_hsync_width;
+ u32 test_vsync_pol;
+ u32 test_vsync_width;
+ u32 test_h_width;
+ u32 test_v_height;
+ u32 test_rr_d;
+ u32 test_rr_n;
+};
+
+struct dp_link_test_audio {
+ u32 test_audio_sampling_rate;
+ u32 test_audio_channel_count;
+ u32 test_audio_pattern_type;
+ u32 test_audio_period_ch_1;
+ u32 test_audio_period_ch_2;
+ u32 test_audio_period_ch_3;
+ u32 test_audio_period_ch_4;
+ u32 test_audio_period_ch_5;
+ u32 test_audio_period_ch_6;
+ u32 test_audio_period_ch_7;
+ u32 test_audio_period_ch_8;
+};
+
+struct dp_link_phy_params {
+ u32 phy_test_pattern_sel;
+ u8 v_level;
+ u8 p_level;
+};
+
+struct dp_link_params {
+ u32 lane_count;
+ u32 bw_code;
+};
Use drm_dp_link
+
+struct dp_link {
+ u32 sink_request;
+ u32 test_response;
+ bool psm_enabled;
+
+ struct dp_link_sink_count sink_count;
+ struct dp_link_test_video test_video;
+ struct dp_link_test_audio test_audio;
+ struct dp_link_phy_params phy_params;
+ struct dp_link_params link_params;
+
+ u32 (*get_test_bits_depth)(struct dp_link *dp_link, u32 bpp);
+ int (*process_request)(struct dp_link *dp_link);
+ int (*get_colorimetry_config)(struct dp_link *dp_link);
+ int (*adjust_levels)(struct dp_link *dp_link, u8 *link_status);
+ int (*send_psm_request)(struct dp_link *dp_link, bool req);
+ void (*send_test_response)(struct dp_link *dp_link);
+ int (*psm_config)(struct dp_link *dp_link,
+ struct drm_dp_link *link_info, bool enable);
+ void (*send_edid_checksum)(struct dp_link *dp_link, u8 checksum);
+};
+
+static inline char *dp_link_get_phy_test_pattern(u32
phy_test_pattern_sel)
+{
+ switch (phy_test_pattern_sel) {
+ case DP_TEST_PHY_PATTERN_NONE:
+ return DP_LINK_ENUM_STR(DP_TEST_PHY_PATTERN_NONE);
+ case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING:
+ return DP_LINK_ENUM_STR(
+ DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING);
+ case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT:
+ return DP_LINK_ENUM_STR(
+ DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT);
+ case DP_TEST_PHY_PATTERN_PRBS7:
+ return DP_LINK_ENUM_STR(DP_TEST_PHY_PATTERN_PRBS7);
+ case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN:
+ return DP_LINK_ENUM_STR(
+ DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN);
+ case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN:
+ return DP_LINK_ENUM_STR(
+ DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN);
+ default:
+ return "unknown";
+ }
+}
This is only used for debug statements, just remove it and print the
enum as an
int. This will let you remove DP_LINK_ENUM_STR as well.
+
+/**
+ * mdss_dp_test_bit_depth_to_bpp() - convert test bit depth to bpp
+ * @tbd: test bit depth
+ *
+ * Returns the bits per pixel (bpp) to be used corresponding to the
+ * git bit depth value. This function assumes that bit depth has
+ * already been validated.
+ */
+static inline u32 dp_link_bit_depth_to_bpp(u32 tbd)
+{
+ u32 bpp;
+
+ /*
+ * Few simplistic rules and assumptions made here:
+ * 1. Bit depth is per color component
+ * 2. If bit depth is unknown return 0
+ * 3. Assume 3 color components
+ */
+ switch (tbd) {
+ case DP_TEST_BIT_DEPTH_6:
+ bpp = 18;
+ break;
+ case DP_TEST_BIT_DEPTH_8:
+ bpp = 24;
+ break;
+ case DP_TEST_BIT_DEPTH_10:
+ bpp = 30;
+ break;
+ case DP_TEST_BIT_DEPTH_UNKNOWN:
+ default:
+ bpp = 0;
+ }
+
+ return bpp;
Just return directly from each case.
+}
+
+/**
+ * dp_link_get() - get the functionalities of dp test module
+ *
+ *
+ * return: a pointer to dp_link struct
+ */
+struct dp_link *dp_link_get(struct device *dev, struct dp_aux *aux);
+
+/**
+ * dp_link_put() - releases the dp test module's resources
+ *
+ * @dp_link: an instance of dp_link module
+ *
+ */
+void dp_link_put(struct dp_link *dp_link);
+
+#endif /* _DP_LINK_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
new file mode 100644
index 0000000..e497b44
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -0,0 +1,624 @@
+/*
+ * Copyright (c) 2012-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 "dp_panel.h"
+
+#include <drm/drm_connector.h>
+#include <drm/drm_edid.h>
+
+#define DP_PANEL_DEFAULT_BPP 24
unused
+#define DP_MAX_DS_PORT_COUNT 1
+
+enum {
+ DP_LINK_RATE_MULTIPLIER = 27000000,
+};
unused
+
+struct dp_panel_private {
+ struct device *dev;
+ struct dp_panel dp_panel;
+ struct dp_aux *aux;
+ struct dp_link *link;
+ struct dp_catalog_panel *catalog;
+ bool panel_on;
+ bool aux_cfg_update_done;
+};
+
+static const struct dp_panel_info fail_safe = {
+ .h_active = 640,
+ .v_active = 480,
+ .h_back_porch = 48,
+ .h_front_porch = 16,
+ .h_sync_width = 96,
+ .h_active_low = 0,
+ .v_back_porch = 33,
+ .v_front_porch = 10,
+ .v_sync_width = 2,
+ .v_active_low = 0,
+ .h_skew = 0,
+ .refresh_rate = 60,
+ .pixel_clk_khz = 25200,
+ .bpp = 24,
+};
Drop this, userspace can add a mode if appropriate
+
+static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
+{
+ int rlen, rc = 0;
+ struct dp_panel_private *panel;
+ struct drm_dp_link *link_info;
+ u8 *dpcd, major = 0, minor = 0;
+ u32 dfp_count = 0;
+ unsigned long caps = DP_LINK_CAP_ENHANCED_FRAMING;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ dpcd = dp_panel->dpcd;
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+ link_info = &dp_panel->link_info;
+
+ rlen = drm_dp_dpcd_read(panel->aux->drm_aux, DP_DPCD_REV,
+ dpcd, (DP_RECEIVER_CAP_SIZE + 1));
+ if (rlen < (DP_RECEIVER_CAP_SIZE + 1)) {
+ pr_err("dpcd read failed, rlen=%d\n", rlen);
+ rc = -EINVAL;
+ goto end;
+ }
+
+ link_info->revision = dp_panel->dpcd[DP_DPCD_REV];
+
+ major = (link_info->revision >> 4) & 0x0f;
+ minor = link_info->revision & 0x0f;
+ pr_debug("version: %d.%d\n", major, minor);
+
+ link_info->rate =
+ drm_dp_bw_code_to_link_rate(dp_panel->dpcd[DP_MAX_LINK_RATE]);
+ pr_debug("link_rate=%d\n", link_info->rate);
+
+ link_info->num_lanes = dp_panel->dpcd[DP_MAX_LANE_COUNT] &
+ DP_MAX_LANE_COUNT_MASK;
+
+ pr_debug("lane_count=%d\n", link_info->num_lanes);
+
+ if (drm_dp_enhanced_frame_cap(dpcd))
+ link_info->capabilities |= caps;
All of the link_info initialization should be done by
drm_dp_link_probe().
+
+ dfp_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] &
+ DP_DOWN_STREAM_PORT_COUNT;
Move this down to where it's used
+
+ if ((dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT)
+ && (dpcd[DP_DPCD_REV] > 0x10)) {
+ rlen = drm_dp_dpcd_read(panel->aux->drm_aux,
+ DP_DOWNSTREAM_PORT_0, dp_panel->ds_ports,
+ DP_MAX_DOWNSTREAM_PORTS);
+ if (rlen < DP_MAX_DOWNSTREAM_PORTS) {
+ pr_err("ds port status failed, rlen=%d\n", rlen);
+ rc = -EINVAL;
+ goto end;
+ }
+ }
You don't use ds_ports anywhere, so remove this and remove ds_ports
+
+ if (dfp_count > DP_MAX_DS_PORT_COUNT)
+ pr_debug("DS port count %d greater that max (%d) supported\n",
+ dfp_count, DP_MAX_DS_PORT_COUNT);
Should this be an error?
+
+end:
+ return rc;
Same drill as before
+}
+
+static int dp_panel_set_default_link_params(struct dp_panel
*dp_panel)
+{
Let's just fail instead of doing this.
+ struct drm_dp_link *link_info;
+ const int default_bw_code = 162000;
+ const int default_num_lanes = 1;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+ link_info = &dp_panel->link_info;
+ link_info->rate = default_bw_code;
+ link_info->num_lanes = default_num_lanes;
+ pr_debug("link_rate=%d num_lanes=%d\n",
+ link_info->rate, link_info->num_lanes);
+ return 0;
+}
+
+static int dp_panel_read_edid(struct dp_panel *dp_panel,
+ struct drm_connector *connector)
+{
+ int retry_cnt = 0;
+ const int max_retry = 10;
+ struct dp_panel_private *panel;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+ do {
+ kfree(dp_panel->edid);
+ dp_panel->edid = drm_get_edid(connector,
+ &panel->aux->drm_aux->ddc);
+ if (!dp_panel->edid) {
+ pr_err("EDID read failed\n");
+ retry_cnt++;
+ panel->aux->reconfig(panel->aux);
+ panel->aux_cfg_update_done = true;
+ } else {
+ return 0;
+ }
+ } while (retry_cnt < max_retry);
The core will do retrys when aux reads fail, so you don't need to. With
that,
you can just remove this function and call drm_get_edid in
read_sink_caps
+
+ return -EINVAL;
+}
+
+static int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
+ struct drm_connector *connector)
+{
+ int rc = 0;
+ struct dp_panel_private *panel;
+
+ if (!dp_panel || !connector) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+ rc = dp_panel_read_dpcd(dp_panel);
+ if (rc || !is_link_rate_valid(drm_dp_link_rate_to_bw_code(
+ dp_panel->link_info.rate)) || !is_lane_count_valid(
+ dp_panel->link_info.num_lanes) ||
+ ((drm_dp_link_rate_to_bw_code(dp_panel->link_info.rate)) >
+ dp_panel->max_bw_code)) {
+ if ((rc == -ETIMEDOUT) || (rc == -ENODEV)) {
+ pr_err("DPCD read failed, return early\n");
+ return rc;
+ }
+ pr_err("panel dpcd read failed/incorrect, set default params\n");
+ dp_panel_set_default_link_params(dp_panel);
+ }
Since the defaults are going away, this gets simpler (and even simpler
if you
stash bw_code):
bw_code = drm_dp_link_rate_to_bw_code(dp_panel->link_info.rate);
if (rc || !is_link_rate_valid(bw_code) ||
!is_lane_count_valid(dp_panel->link_info.num_lanes) ||
bw_code > dp_panel->max_bw_code) {
pr_err("read_dpcd failed %d\n", rc);
return rc;
}
+
+ rc = dp_panel_read_edid(dp_panel, connector);
Why read the edid so early? Can't you wait until get_modes is called
(you'll
have to re-read anyways).
+ if (rc) {
+ pr_err("panel edid read failed, set failsafe mode\n");
+ return rc;
+ }
+
+ if (panel->aux_cfg_update_done) {
+ pr_debug("read DPCD with updated AUX config\n");
+ dp_panel_read_dpcd(dp_panel);
Check return value. Should you also check the rate/lane_count validity
again too?
+ panel->aux_cfg_update_done = false;
Should you have locking for aux_cfg_update_done?
+ }
+
+ return 0;
+}
+
+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.
+ }
+
+ 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;
As mentioned earlier, no need to duplicate drm_display_mode into
dp_display_mode, same goes for dp_panel_info, please just use
drm_display_mode.
+ struct dp_link_test_video *test_info = NULL;
+
+ if (!panel) {
+ pr_err("invalid params\n");
+ return;
+ }
+
+ pinfo = &mode->timing;
+ test_info = &panel->link->test_video;
+
+ pinfo->h_active = test_info->test_h_width;
+ pinfo->h_sync_width = test_info->test_hsync_width;
+ pinfo->h_back_porch = test_info->test_h_start -
+ test_info->test_hsync_width;
+ pinfo->h_front_porch = test_info->test_h_total -
+ (test_info->test_h_start + test_info->test_h_width);
+
+ pinfo->v_active = test_info->test_v_height;
+ pinfo->v_sync_width = test_info->test_vsync_width;
+ pinfo->v_back_porch = test_info->test_v_start -
+ test_info->test_vsync_width;
+ pinfo->v_front_porch = test_info->test_v_total -
+ (test_info->test_v_start + test_info->test_v_height);
+
+ pinfo->bpp = dp_link_bit_depth_to_bpp(test_info->test_bit_depth);
+ pinfo->h_active_low = test_info->test_hsync_pol;
+ pinfo->v_active_low = test_info->test_vsync_pol;
+
+ pinfo->refresh_rate = test_info->test_rr_n;
+ pinfo->pixel_clk_khz = test_info->test_h_total *
+ test_info->test_v_total * pinfo->refresh_rate;
+
+ if (test_info->test_rr_d == 0)
+ pinfo->pixel_clk_khz /= 1000;
+ else
+ pinfo->pixel_clk_khz /= 1001;
+
+ if (test_info->test_h_width == 640)
+ pinfo->pixel_clk_khz = 25170;
+}
+
+static int dp_panel_update_modes(struct drm_connector *connector,
+ struct edid *edid)
+{
+ int rc = 0;
+
+ pr_debug("%s +", __func__);
remove
Ok, so this is where you should be reading edid.
+ if (edid) {
+ drm_connector_update_edid_property(connector,
+ edid);
Join on one line and check return value
+ rc = drm_add_edid_modes(connector, edid);
+ pr_debug("%s -", __func__);
+ return rc;
+ }
+
+ drm_connector_update_edid_property(connector, NULL);
Check return value
+ pr_debug("%s null edid -", __func__);
+ return rc;
This is always 0
+}
+
+static int dp_panel_get_modes(struct dp_panel *dp_panel,
+ struct drm_connector *connector, struct dp_display_mode *mode)
+{
+ struct dp_panel_private *panel;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ return -EINVAL;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+ if (dp_panel->video_test) {
+ dp_panel_set_test_mode(panel, mode);
+ return 1;
+ } else if (dp_panel->edid) {
+ return dp_panel_update_modes(connector, dp_panel->edid);
Indentation is wrong
+ }
+
+ /* fail-safe mode */
+ memcpy(&mode->timing, &fail_safe,
+ sizeof(fail_safe));
+ return 1;
Delete fail safe and return 0;
+}
+
+static u8 dp_panel_get_edid_checksum(struct edid *edid)
+{
+ struct edid *last_block = NULL;
+ u8 *raw_edid = NULL;
No need to initialize these.
+
+ if (!edid) {
+ pr_err("invalid edid input\n");
+ return 0;
+ }
+
+ raw_edid = (u8 *)edid;
+ raw_edid += (edid->extensions * EDID_LENGTH);
+ last_block = (struct edid *)raw_edid;
+
+ if (last_block)
I doubt last_block will ever be NULL, so this check is kind of
pointless.
Perhaps call drm_edid_block_valid() instead.
+ return last_block->checksum;
+
+ pr_err("Invalid block, no checksum\n");
+ return 0;
+}
+
+static void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
+{
+ struct dp_panel_private *panel;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+ if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
+ u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
+
+ panel->link->send_edid_checksum(panel->link, checksum);
+ panel->link->send_test_response(panel->link);
+ }
+}
+
+static void dp_panel_tpg_config(struct dp_panel *dp_panel, bool
enable)
+{
+ u32 hsync_start_x, hsync_end_x;
+ struct dp_catalog_panel *catalog;
+ struct dp_panel_private *panel;
+ struct dp_panel_info *pinfo;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ return;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+ catalog = panel->catalog;
+ pinfo = &panel->dp_panel.pinfo;
+
+ if (!panel->panel_on) {
+ pr_debug("DP panel not enabled, handle TPG on next panel on\n");
+ return;
+ }
+
+ if (!enable) {
+ panel->catalog->tpg_config(catalog, false);
+ return;
+ }
+
+ /* TPG config */
+ catalog->hsync_period = pinfo->h_sync_width + pinfo->h_back_porch +
+ pinfo->h_active + pinfo->h_front_porch;
+ catalog->vsync_period = pinfo->v_sync_width + pinfo->v_back_porch +
+ pinfo->v_active + pinfo->v_front_porch;
+
+ catalog->display_v_start = ((pinfo->v_sync_width +
+ pinfo->v_back_porch) * catalog->hsync_period);
+ catalog->display_v_end = ((catalog->vsync_period -
+ pinfo->v_front_porch) * catalog->hsync_period) - 1;
+
+ catalog->display_v_start += pinfo->h_sync_width +
pinfo->h_back_porch;
+ catalog->display_v_end -= pinfo->h_front_porch;
+
+ hsync_start_x = pinfo->h_back_porch + pinfo->h_sync_width;
+ hsync_end_x = catalog->hsync_period - pinfo->h_front_porch - 1;
+
+ catalog->v_sync_width = pinfo->v_sync_width;
+
+ catalog->hsync_ctl = (catalog->hsync_period << 16) |
+ pinfo->h_sync_width;
+ catalog->display_hctl = (hsync_end_x << 16) | hsync_start_x;
More mode timing parameters to delete!
+
+ pr_err("%s: calling catalog tpg_config\n", __func__);
+ panel->catalog->tpg_config(catalog, true);
+}
+
+static int dp_panel_timing_cfg(struct dp_panel *dp_panel)
+{
+ int rc = 0;
+ u32 data, total_ver, total_hor;
+ struct dp_catalog_panel *catalog;
+ struct dp_panel_private *panel;
+ struct dp_panel_info *pinfo;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
This is called from 2 different places, do you need locking?
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+ catalog = panel->catalog;
+ pinfo = &panel->dp_panel.pinfo;
+
+ pr_debug("width=%d hporch= %d %d %d\n",
+ pinfo->h_active, pinfo->h_back_porch,
+ pinfo->h_front_porch, pinfo->h_sync_width);
+
+ pr_debug("height=%d vporch= %d %d %d\n",
+ pinfo->v_active, pinfo->v_back_porch,
+ pinfo->v_front_porch, pinfo->v_sync_width);
+
+ total_hor = pinfo->h_active + pinfo->h_back_porch +
+ pinfo->h_front_porch + pinfo->h_sync_width;
+
+ total_ver = pinfo->v_active + pinfo->v_back_porch +
+ pinfo->v_front_porch + pinfo->v_sync_width;
+
+ data = total_ver;
+ data <<= 16;
+ data |= total_hor;
+
+ catalog->total = data;
+
+ data = (pinfo->v_back_porch + pinfo->v_sync_width);
+ data <<= 16;
+ data |= (pinfo->h_back_porch + pinfo->h_sync_width);
+
+ catalog->sync_start = data;
+
+ data = pinfo->v_sync_width;
+ data <<= 16;
+ data |= (pinfo->v_active_low << 31);
+ data |= pinfo->h_sync_width;
+ data |= (pinfo->h_active_low << 15);
+
+ catalog->width_blanking = data;
+
+ data = pinfo->v_active;
+ data <<= 16;
+ data |= pinfo->h_active;
+
+ catalog->dp_active = data;
+
+ panel->catalog->timing_cfg(catalog);
+ panel->panel_on = true;
Again, all of this should be derived from a drm_display_mode at the
point where
it is committed to hardware. No need to store it in another form in the
interim
+end:
+ return rc;
Remove end/rc and return directly
+}
+
+static int dp_panel_init_panel_info(struct dp_panel *dp_panel)
+{
+ int rc = 0;
+ struct dp_panel_info *pinfo;
+
+ if (!dp_panel) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto end;
+ }
+
+ pinfo = &dp_panel->pinfo;
+
+ /*
+ * print resolution info as this is a result
+ * of user initiated action of cable connection
+ */
+ pr_info("SET NEW RESOLUTION:\n");
+ pr_info("%dx%d@%dfps\n", pinfo->h_active,
+ pinfo->v_active, pinfo->refresh_rate);
+ pr_info("h_porches(back|front|width) = (%d|%d|%d)\n",
+ pinfo->h_back_porch,
+ pinfo->h_front_porch,
+ pinfo->h_sync_width);
+ pr_info("v_porches(back|front|width) = (%d|%d|%d)\n",
+ pinfo->v_back_porch,
+ pinfo->v_front_porch,
+ pinfo->v_sync_width);
+ pr_info("pixel clock (KHz)=(%d)\n", pinfo->pixel_clk_khz);
+ pr_info("bpp = %d\n", pinfo->bpp);
+ pr_info("active low (h|v)=(%d|%d)\n", pinfo->h_active_low,
+ pinfo->v_active_low);
+
+ pinfo->bpp = max_t(u32, 18, min_t(u32, pinfo->bpp, 30));
+ pr_info("updated bpp = %d\n", pinfo->bpp);
No info level logs, please.
+end:
+ return rc;
Remove end/rc and return directly
+}
+
+static u32 dp_panel_get_min_req_link_rate(struct dp_panel *dp_panel)
+{
+ const u32 encoding_factx10 = 8;
+ u32 min_link_rate_khz = 0, lane_cnt;
+ struct dp_panel_info *pinfo;
+
+ if (!dp_panel) {
The only way to call this function is through a dp_panel callback, and
yet we
check if dp_panel is NULL :)
The same is true for most functions in this patch. Things will be a lot
simpler
when all of these are removed. This function will no longer have an
error path
and lane_cnt/pinfo can be initialized directly above.
+ pr_err("invalid input\n");
+ goto end;
+ }
+
+ lane_cnt = dp_panel->link_info.num_lanes;
+ pinfo = &dp_panel->pinfo;
+
+ /* num_lanes * lane_count * 8 >= pclk * bpp * 10 */
+ min_link_rate_khz = pinfo->pixel_clk_khz /
+ (lane_cnt * encoding_factx10);
+ min_link_rate_khz *= pinfo->bpp;
+
+ pr_debug("min lclk req=%d khz for pclk=%d khz, lanes=%d, bpp=%d\n",
+ min_link_rate_khz, pinfo->pixel_clk_khz, lane_cnt,
+ pinfo->bpp);
+end:
+ return min_link_rate_khz;
Remove end and return directly above
+}
+
+struct dp_panel *dp_panel_get(struct dp_panel_in *in)
+{
+ int rc = 0;
+ struct dp_panel_private *panel;
+ struct dp_panel *dp_panel;
+
+ if (!in->dev || !in->catalog || !in->aux || !in->link) {
+ pr_err("invalid input\n");
+ rc = -EINVAL;
+ goto error;
+ }
+
+ panel = devm_kzalloc(in->dev, sizeof(*panel), GFP_KERNEL);
+ if (!panel) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ panel->dev = in->dev;
+ panel->aux = in->aux;
+ panel->catalog = in->catalog;
+ panel->link = in->link;
+
+ dp_panel = &panel->dp_panel;
+ dp_panel->max_bw_code = DP_LINK_BW_8_1;
+ panel->aux_cfg_update_done = false;
+
+ dp_panel->init_info = dp_panel_init_panel_info;
+ dp_panel->timing_cfg = dp_panel_timing_cfg;
+ dp_panel->read_sink_caps = dp_panel_read_sink_caps;
+ dp_panel->get_min_req_link_rate = dp_panel_get_min_req_link_rate;
+ dp_panel->get_mode_bpp = dp_panel_get_mode_bpp;
+ dp_panel->get_modes = dp_panel_get_modes;
+ dp_panel->handle_sink_request = dp_panel_handle_sink_request;
+ dp_panel->tpg_config = dp_panel_tpg_config;
+
+ return dp_panel;
+error:
+ return ERR_PTR(rc);
+}
+
+void dp_panel_put(struct dp_panel *dp_panel)
+{
+ struct dp_panel_private *panel;
+
+ if (!dp_panel)
+ return;
+
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+ kfree(dp_panel->edid);
+ dp_panel->edid = NULL;
+ devm_kfree(panel->dev, panel);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
b/drivers/gpu/drm/msm/dp/dp_panel.h
new file mode 100644
index 0000000..bf802df
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) 2012-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_PANEL_H_
+#define _DP_PANEL_H_
+
+#include <drm/msm_drm.h>
+
+#include "dp_aux.h"
+#include "dp_link.h"
+#include "dp_extcon.h"
+
+struct edid;
+
+enum dp_lane_count {
+ DP_LANE_COUNT_1 = 1,
+ DP_LANE_COUNT_2 = 2,
+ DP_LANE_COUNT_4 = 4,
+};
You don't need an enum for this, the value coming from dpcd is just an
integer and
can be used as such.
+
+#define DP_MAX_DOWNSTREAM_PORTS 0x10
+
+struct dp_panel_info {
+ u32 h_active;
+ u32 v_active;
+ u32 h_back_porch;
+ u32 h_front_porch;
+ u32 h_sync_width;
+ u32 h_active_low;
+ u32 v_back_porch;
+ u32 v_front_porch;
+ u32 v_sync_width;
+ u32 v_active_low;
+ u32 h_skew;
+ u32 refresh_rate;
+ u32 pixel_clk_khz;
+ u32 bpp;
+};
Drop this struct and use drm_display_mode
+
+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?
+ 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
+ u32 (*get_mode_bpp)(struct dp_panel *dp_panel, u32 mode_max_bpp,
+ u32 mode_pclk_khz);
+ int (*get_modes)(struct dp_panel *dp_panel,
+ struct drm_connector *connector, struct dp_display_mode *mode);
+ void (*handle_sink_request)(struct dp_panel *dp_panel);
+ void (*tpg_config)(struct dp_panel *dp_panel, bool enable);
+};
+
+/**
+ * is_link_rate_valid() - validates the link rate
+ * @lane_rate: link rate requested by the sink
+ *
+ * Returns true if the requested link rate is supported.
+ */
+static inline bool is_link_rate_valid(u32 bw_code)
+{
+ return ((bw_code == DP_LINK_BW_1_62) ||
+ (bw_code == DP_LINK_BW_2_7) ||
+ (bw_code == DP_LINK_BW_5_4) ||
+ (bw_code == DP_LINK_BW_8_1));
Each of these conditions has unneeded ()
+}
+
+/**
+ * dp_link_is_lane_count_valid() - validates the lane count
+ * @lane_count: lane count requested by the sink
+ *
+ * Returns true if the requested lane count is supported.
+ */
+static inline bool is_lane_count_valid(u32 lane_count)
+{
+ return (lane_count == DP_LANE_COUNT_1) ||
+ (lane_count == DP_LANE_COUNT_2) ||
+ (lane_count == DP_LANE_COUNT_4);
+}
+
+struct dp_panel *dp_panel_get(struct dp_panel_in *in);
+void dp_panel_put(struct dp_panel *dp_panel);
+#endif /* _DP_PANEL_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
new file mode 100644
index 0000000..a366dc5
--- /dev/null
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -0,0 +1,679 @@
+/*
+ * Copyright (c) 2012-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/of_gpio.h>
+
+#include "dp_parser.h"
+
+static void dp_parser_unmap_io_resources(struct dp_parser *parser)
+{
+ struct dp_io *io = &parser->io;
+
+ msm_dss_iounmap(&io->dp_ahb);
+ msm_dss_iounmap(&io->dp_aux);
+ msm_dss_iounmap(&io->dp_link);
+ msm_dss_iounmap(&io->dp_p0);
+ msm_dss_iounmap(&io->phy_io);
+ msm_dss_iounmap(&io->ln_tx0_io);
+ msm_dss_iounmap(&io->ln_tx0_io);
+ msm_dss_iounmap(&io->dp_pll_io);
+ msm_dss_iounmap(&io->dp_cc_io);
+ msm_dss_iounmap(&io->usb3_dp_com);
+ msm_dss_iounmap(&io->qfprom_io);
+}
+
+static int dp_parser_ctrl_res(struct dp_parser *parser)
+{
+ int rc = 0;
+ u32 index;
+ struct platform_device *pdev = parser->pdev;
+ struct device_node *of_node = parser->pdev->dev.of_node;
+ struct dp_io *io = &parser->io;
+
+ rc = of_property_read_u32(of_node, "cell-index", &index);
+ if (rc) {
+ pr_err("cell-index not specified, rc=%d\n", rc);
+ goto err;
This can be a direct return of rc.
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->dp_ahb, "dp_ahb");
+ if (rc) {
+ pr_err("unable to remap dp io resources\n");
Print rc here and below.
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->dp_aux, "dp_aux");
+ if (rc) {
+ pr_err("unable to remap dp io resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->dp_link, "dp_link");
+ if (rc) {
+ pr_err("unable to remap dp io resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->dp_p0, "dp_p0");
+ if (rc) {
+ pr_err("unable to remap dp io resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->phy_io, "dp_phy");
+ if (rc) {
+ pr_err("unable to remap dp PHY resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->ln_tx0_io, "dp_ln_tx0");
+ if (rc) {
+ pr_err("unable to remap dp TX0 resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->ln_tx1_io, "dp_ln_tx1");
+ if (rc) {
+ pr_err("unable to remap dp TX1 resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->dp_pll_io, "dp_pll");
+ if (rc) {
+ pr_err("unable to remap DP PLL resources\n");
+ goto err;
+ }
+
+ rc = msm_dss_ioremap_byname(pdev, &io->usb3_dp_com, "usb3_dp_com");
+ if (rc) {
+ pr_err("unable to remap USB3 DP com resources\n");
+ goto err;
+ }
+
+ if (msm_dss_ioremap_byname(pdev, &io->dp_cc_io, "dp_mmss_cc")) {
+ pr_err("unable to remap dp MMSS_CC resources\n");
+ goto err;
+ }
+
+ if (msm_dss_ioremap_byname(pdev, &io->qfprom_io, "qfprom_physical"))
+ pr_warn("unable to remap dp qfprom resources\n");
+
+ return 0;
+err:
+ dp_parser_unmap_io_resources(parser);
+ return rc;
+}
+
+static const char *dp_get_phy_aux_config_property(u32 cfg_type)
+{
+ switch (cfg_type) {
+ case PHY_AUX_CFG0:
+ return "qcom,aux-cfg0-settings";
+ case PHY_AUX_CFG1:
+ return "qcom,aux-cfg1-settings";
+ case PHY_AUX_CFG2:
+ return "qcom,aux-cfg2-settings";
+ case PHY_AUX_CFG3:
+ return "qcom,aux-cfg3-settings";
+ case PHY_AUX_CFG4:
+ return "qcom,aux-cfg4-settings";
+ case PHY_AUX_CFG5:
+ return "qcom,aux-cfg5-settings";
+ case PHY_AUX_CFG6:
+ return "qcom,aux-cfg6-settings";
+ case PHY_AUX_CFG7:
+ return "qcom,aux-cfg7-settings";
+ case PHY_AUX_CFG8:
+ return "qcom,aux-cfg8-settings";
+ case PHY_AUX_CFG9:
+ return "qcom,aux-cfg9-settings";
+ default:
+ return "unknown";
+ }
+}
+
+static void dp_parser_phy_aux_cfg_reset(struct dp_parser *parser)
+{
+ int i = 0;
+
+ for (i = 0; i < PHY_AUX_CFG_MAX; i++)
+ parser->aux_cfg[i] = (const struct dp_aux_cfg){ 0 };
+}
No need for a separate function, just put the loop in the error label
below.
+
+static int dp_parser_aux(struct dp_parser *parser)
+{
+ struct device_node *of_node = parser->pdev->dev.of_node;
+ int len = 0, i = 0, j = 0, config_count = 0;
+ const char *data;
+ int const minimum_config_count = 1;
Just use 1 directly
+
+ for (i = 0; i < PHY_AUX_CFG_MAX; i++) {
+ const char *property = dp_get_phy_aux_config_property(i);
+
+ data = of_get_property(of_node, property, &len);
+ if (!data) {
You should use of_property_read_u32() instead of reading as a string
+ pr_err("Unable to read %s\n", property);
+ goto error;
+ }
+
+ config_count = len - 1;
+ if ((config_count < minimum_config_count) ||
+ (config_count > DP_AUX_CFG_MAX_VALUE_CNT)) {
Remove extra ()
+ pr_err("Invalid config count (%d) configs for %s\n",
+ config_count, property);
+ goto error;
+ }
+
+ parser->aux_cfg[i].offset = data[0];
+ parser->aux_cfg[i].cfg_cnt = config_count;
+ pr_debug("%s offset=0x%x, cfg_cnt=%d\n",
+ property,
+ parser->aux_cfg[i].offset,
+ parser->aux_cfg[i].cfg_cnt);
+ for (j = 1; j < len; j++) {
+ parser->aux_cfg[i].lut[j - 1] = data[j];
And this could use of_property_read_u32_array()
+ pr_debug("%s lut[%d]=0x%x\n",
+ property,
+ i,
+ parser->aux_cfg[i].lut[j - 1]);
+ }
+ }
+ return 0;
+
+error:
+ dp_parser_phy_aux_cfg_reset(parser);
+ return -EINVAL;
+}
+
+static int dp_parser_misc(struct dp_parser *parser)
+{
+ int rc = 0;
+ struct device_node *of_node = parser->pdev->dev.of_node;
+
+ rc = of_property_read_u32(of_node,
+ "qcom,max-pclk-frequency-khz", &parser->max_pclk_khz);
+ if (rc)
+ parser->max_pclk_khz = DP_MAX_PIXEL_CLK_KHZ;
+
+ return 0;
+}
+
+static int dp_parser_pinctrl(struct dp_parser *parser)
+{
+ int rc = 0;
+ struct dp_pinctrl *pinctrl = &parser->pinctrl;
+
+ pinctrl->pin = devm_pinctrl_get(&parser->pdev->dev);
+
+ if (IS_ERR_OR_NULL(pinctrl->pin)) {
+ rc = PTR_ERR(pinctrl->pin);
+ pr_err("failed to get pinctrl, rc=%d\n", rc);
+ goto error;
+ }
+
+ pinctrl->state_active = pinctrl_lookup_state(pinctrl->pin,
+ "mdss_dp_active");
+ if (IS_ERR_OR_NULL(pinctrl->state_active)) {
+ rc = PTR_ERR(pinctrl->state_active);
What if it's NULL?
+ pr_err("failed to get pinctrl active state, rc=%d\n", rc);
+ goto error;
+ }
+
+ pinctrl->state_suspend = pinctrl_lookup_state(pinctrl->pin,
+ "mdss_dp_sleep");
+ if (IS_ERR_OR_NULL(pinctrl->state_suspend)) {
+ rc = PTR_ERR(pinctrl->state_suspend);
What if it's NULL?
+ pr_err("failed to get pinctrl suspend state, rc=%d\n", rc);
+ goto error;
+ }
+error:
+ return rc;
+}
+
+static int dp_parser_gpio(struct dp_parser *parser)
+{
+ int i = 0;
+ struct device *dev = &parser->pdev->dev;
+ struct device_node *of_node = dev->of_node;
+ struct dss_module_power *mp = &parser->mp[DP_CORE_PM];
+ static const char * const dp_gpios[] = {
+ "qcom,aux-en-gpio",
+ "qcom,aux-sel-gpio",
+ "qcom,usbplug-cc-gpio",
+ };
+
+ mp->gpio_config = devm_kzalloc(dev,
+ sizeof(struct dss_gpio) * ARRAY_SIZE(dp_gpios), GFP_KERNEL);
+ if (!mp->gpio_config)
+ return -ENOMEM;
+
+ mp->num_gpio = ARRAY_SIZE(dp_gpios);
If you do this first, you can s/ARRAY_SIZE(dp_gpios)/mp->num_gpio/ in
the rest
of the function.
+
+ for (i = 0; i < ARRAY_SIZE(dp_gpios); i++) {
+ mp->gpio_config[i].gpio = of_get_named_gpio(of_node,
+ dp_gpios[i], 0);
+
+ if (!gpio_is_valid(mp->gpio_config[i].gpio)) {
+ pr_err("%s gpio not specified\n", dp_gpios[i]);
+ return -EINVAL;
+ }
+
+ strlcpy(mp->gpio_config[i].gpio_name, dp_gpios[i],
+ sizeof(mp->gpio_config[i].gpio_name));
As mentioned earlier, please don't implement your own string
store/lookup, just
provide access to the gpios in a more direct manner (pointer or
function).
+
+ mp->gpio_config[i].value = 0;
+ }
+
+ return 0;
+}
+
+static const char *dp_parser_supply_node_name(enum dp_pm_type module)
+{
+ switch (module) {
+ case DP_CORE_PM: return "qcom,core-supply-entries";
+ case DP_CTRL_PM: return "qcom,ctrl-supply-entries";
+ case DP_PHY_PM: return "qcom,phy-supply-entries";
+ default: return "???";
+ }
+}
+
+static int dp_parser_get_vreg(struct dp_parser *parser,
+ enum dp_pm_type module)
+{
+ int i = 0, rc = 0;
+ u32 tmp = 0;
+ const char *pm_supply_name = NULL;
+ struct device_node *supply_node = NULL;
+ struct device_node *of_node = parser->pdev->dev.of_node;
+ struct device_node *supply_root_node = NULL;
+ struct dss_module_power *mp = &parser->mp[module];
+
+ mp->num_vreg = 0;
+ pm_supply_name = dp_parser_supply_node_name(module);
+ supply_root_node = of_get_child_by_name(of_node, pm_supply_name);
+ if (!supply_root_node) {
+ pr_err("no supply entry present: %s\n", pm_supply_name);
+ goto novreg;
+ }
+
+ mp->num_vreg = of_get_available_child_count(supply_root_node);
+
+ if (mp->num_vreg == 0) {
+ pr_debug("no vreg\n");
+ goto novreg;
+ } else {
+ pr_debug("vreg found. count=%d\n", mp->num_vreg);
This doesn't need to be wrapped in an else
+ }
+
+ mp->vreg_config = devm_kzalloc(&parser->pdev->dev,
+ sizeof(struct dss_vreg) * mp->num_vreg, GFP_KERNEL);
+ if (!mp->vreg_config) {
+ rc = -ENOMEM;
+ goto error;
+ }
+
+ for_each_child_of_node(supply_root_node, supply_node) {
+ const char *st = NULL;
+ /* vreg-name */
+ rc = of_property_read_string(supply_node,
+ "qcom,supply-name", &st);
+ if (rc) {
+ pr_err("error reading name. rc=%d\n",
+ rc);
+ goto error;
+ }
+ snprintf(mp->vreg_config[i].vreg_name,
+ ARRAY_SIZE((mp->vreg_config[i].vreg_name)), "%s", st);
Instead of storing the name just to get a regulator reference later,
why don't
you just grab the reference now and avoid the string ops?
+ /* vreg-min-voltage */
+ rc = of_property_read_u32(supply_node,
+ "qcom,supply-min-voltage", &tmp);
+ if (rc) {
+ pr_err("error reading min volt. rc=%d\n",
+ rc);
+ goto error;
+ }
+ mp->vreg_config[i].min_voltage = tmp;
+
+ /* vreg-max-voltage */
+ rc = of_property_read_u32(supply_node,
+ "qcom,supply-max-voltage", &tmp);
+ if (rc) {
+ pr_err("error reading max volt. rc=%d\n",
+ rc);
+ goto error;
+ }
+ mp->vreg_config[i].max_voltage = tmp;
+
+ /* enable-load */
+ rc = of_property_read_u32(supply_node,
+ "qcom,supply-enable-load", &tmp);
+ if (rc) {
+ pr_err("error reading enable load. rc=%d\n",
+ rc);
+ goto error;
+ }
+ mp->vreg_config[i].enable_load = tmp;
+
+ /* disable-load */
+ rc = of_property_read_u32(supply_node,
+ "qcom,supply-disable-load", &tmp);
+ if (rc) {
+ pr_err("error reading disable load. rc=%d\n",
+ rc);
+ goto error;
+ }
+ mp->vreg_config[i].disable_load = tmp;
+
+ pr_debug("%s min=%d, max=%d, enable=%d, disable=%d\n",
+ mp->vreg_config[i].vreg_name,
+ mp->vreg_config[i].min_voltage,
+ mp->vreg_config[i].max_voltage,
+ mp->vreg_config[i].enable_load,
+ mp->vreg_config[i].disable_load
+ );
+ ++i;
+ }
+
+ return rc;
+
+error:
+ if (mp->vreg_config) {
+ devm_kfree(&parser->pdev->dev, mp->vreg_config);
+ mp->vreg_config = NULL;
+ }
This shouldn't be necessary, that's the nice thing about device managed
resources.
+novreg:
+ mp->num_vreg = 0;
+
+ return rc;
+}
+
+static void dp_parser_put_vreg_data(struct device *dev,
+ struct dss_module_power *mp)
+{
+ if (!mp) {
+ DEV_ERR("invalid input\n");
+ return;
+ }
+
+ if (mp->vreg_config) {
+ devm_kfree(dev, mp->vreg_config);
+ mp->vreg_config = NULL;
+ }
Same here, this function can go away once you take out the dp_parser
abstraction layer.
+ mp->num_vreg = 0;
+}
+
+static int dp_parser_regulator(struct dp_parser *parser)
+{
+ int i, rc = 0;
+ struct platform_device *pdev = parser->pdev;
+
+ /* Parse the regulator information */
+ for (i = DP_CORE_PM; i < DP_MAX_PM; i++) {
+ rc = dp_parser_get_vreg(parser, i);
+ if (rc) {
+ pr_err("get_dt_vreg_data failed for %s. rc=%d\n",
+ dp_parser_pm_name(i), rc);
+ i--;
+ for (; i >= DP_CORE_PM; i--)
+ dp_parser_put_vreg_data(&pdev->dev,
+ &parser->mp[i]);
+ break;
And here you can just return the error directly and the resources will
be freed
magically.
+ }
+ }
+
+ return rc;
And this becomes return 0;
+}
+
+static bool dp_parser_check_prefix(const char *clk_prefix, const char
*clk_name)
+{
+ return !!strnstr(clk_name, clk_prefix, strlen(clk_name));
This is likely not what you want since this will look for prefix
_anywhere_ in
clk name. An example of how this could go sideways:
dp_parser_check_prefix("core", "core-dp-power-ctrl");
dp_parser_check_prefix("ctrl", "core-dp-power-ctrl");
Both return true.
Anyways, strncmp is a better choice here (if we must do string
operations).
+}
+
+static void dp_parser_put_clk_data(struct device *dev,
+ struct dss_module_power *mp)
+{
+ if (!mp) {
+ DEV_ERR("%s: invalid input\n", __func__);
+ return;
+ }
+
+ if (mp->clk_config) {
+ devm_kfree(dev, mp->clk_config);
+ mp->clk_config = NULL;
+ }
+
+ mp->num_clk = 0;
+}
+
+static void dp_parser_put_gpio_data(struct device *dev,
+ struct dss_module_power *mp)
+{
+ if (!mp) {
+ DEV_ERR("%s: invalid input\n", __func__);
+ return;
+ }
+
+ if (mp->gpio_config) {
+ devm_kfree(dev, mp->gpio_config);
+ mp->gpio_config = NULL;
+ }
+
+ mp->num_gpio = 0;
+}
+
+static int dp_parser_init_clk_data(struct dp_parser *parser)
+{
+ int num_clk = 0, i = 0, rc = 0;
+ int core_clk_count = 0, ctrl_clk_count = 0;
+ const char *core_clk = "core";
+ const char *ctrl_clk = "ctrl";
+ const char *clk_name;
+ struct device *dev = &parser->pdev->dev;
+ struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
+ struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
+
+ num_clk = of_property_count_strings(dev->of_node, "clock-names");
+ if (num_clk <= 0) {
+ pr_err("no clocks are defined\n");
+ rc = -EINVAL;
+ goto exit;
+ }
+
+ for (i = 0; i < num_clk; i++) {
+ of_property_read_string_index(dev->of_node,
+ "clock-names", i, &clk_name);
Check return value
+
+ if (dp_parser_check_prefix(core_clk, clk_name))
+ core_clk_count++;
+
+ if (dp_parser_check_prefix(ctrl_clk, clk_name))
+ ctrl_clk_count++;
Just use "core"/"ctrl" directly here.
Have you considered just having 2 clock lists? That would avoid having
to do
this.
