| Message ID | 1539191759-14836-1-git-send-email-chandanu@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for DisplayPort driver on SnapDragon 845 | expand |
On 2018-10-10 10:15, Chandan Uddaraju wrote: > These patches add support for Display-Port driver on SnapDragon 845 > hardware. It adds > DP driver and DP PLL driver files along with the needed device-tree > bindings. > > The block diagram of DP driver is shown below: > > > +-------------+ > |DRM FRAMEWORK| > +------+------+ > | > +----v----+ > | DP DRM | > +----+----+ > | > +----v----+ > +------------+| DP +----------++------+ > + +---+| DISPLAY |+---+ | | > | + +-+-----+-+ | | | > | | | | | | | > | | | | | | | > | | | | | | | > v v v v v v v > +------+ +------+ +---+ +----+ +----+ +---+ +-----+ > | DP | | DP | |DP | | DP | | DP | |DP | | DP | > |PARSER| |EXTCON| |AUX| |LINK| |CTRL| |PHY| |POWER| > +--+---+ +---+--+ +---+ +----+ +--+-+ +-+-+ +-----+ > | | | | > +--v---+ +---v-----+ +v-----v+ > |DEVICE| |EXTCON | | DP | > | TREE | |INTERFACE| |CATALOG| > +------+ +---------+ +---+---+ > | > +---v----+ > |CTRL/PHY| > | HW | > +--------+ > It will be more helpful to have this block diagram in [DPU PATCH 2/3] drm/msm/dp: add displayPort driver support Thanks, Jeykumar S. > > These patches have dependency on clock driver changes mentioned below: > https://patchwork.kernel.org/patch/10632753/ > https://patchwork.kernel.org/patch/10632757/ > > > > Chandan Uddaraju (3): > dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon > 845 > drm/msm/dp: add displayPort driver support > drm/msm/dp: add support for DP PLL driver > > .../devicetree/bindings/display/msm/dp.txt | 249 ++++ > .../devicetree/bindings/display/msm/dpu.txt | 16 +- > drivers/gpu/drm/msm/Kconfig | 25 + > drivers/gpu/drm/msm/Makefile | 21 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 +++ > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 + > drivers/gpu/drm/msm/dp/dp_aux.c | 570 +++++++ > drivers/gpu/drm/msm/dp/dp_aux.h | 44 + > drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 > +++++++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 144 ++ > drivers/gpu/drm/msm/dp/dp_ctrl.c | 1476 > +++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 + > drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++ > drivers/gpu/drm/msm/dp/dp_debug.h | 81 + > drivers/gpu/drm/msm/dp/dp_display.c | 1027 > +++++++++++++ > drivers/gpu/drm/msm/dp/dp_display.h | 58 + > drivers/gpu/drm/msm/dp/dp_drm.c | 542 +++++++ > drivers/gpu/drm/msm/dp/dp_drm.h | 52 + > drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++ > drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++ > drivers/gpu/drm/msm/dp/dp_link.c | 1549 > ++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_link.h | 184 +++ > drivers/gpu/drm/msm/dp/dp_panel.c | 624 ++++++++ > drivers/gpu/drm/msm/dp/dp_panel.h | 121 ++ > drivers/gpu/drm/msm/dp/dp_parser.c | 679 +++++++++ > drivers/gpu/drm/msm/dp/dp_parser.h | 208 +++ > drivers/gpu/drm/msm/dp/dp_power.c | 652 ++++++++ > drivers/gpu/drm/msm/dp/dp_power.h | 59 + > drivers/gpu/drm/msm/dp/dp_reg.h | 357 +++++ > drivers/gpu/drm/msm/dp/pll/dp_pll.c | 153 ++ > drivers/gpu/drm/msm/dp/pll/dp_pll.h | 64 + > drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c | 401 +++++ > drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h | 94 ++ > drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c | 531 +++++++ > drivers/gpu/drm/msm/msm_drv.c | 2 + > drivers/gpu/drm/msm/msm_drv.h | 22 + > include/drm/drm_dp_helper.h | 19 + > 37 files changed, 12525 insertions(+), 5 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/display/msm/dp.txt > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h > create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.c > create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.h > create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c > create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h > create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote: > Add the needed displayPort files to enable DP driver > on msm target. > > "dp_display" module is the main module that calls into > other sub-modules. "dp_drm" file represents the interface > between DRM framework and DP driver. > > Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org> > --- > drivers/gpu/drm/msm/Kconfig | 9 + > drivers/gpu/drm/msm/Makefile | 15 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 ++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 + > drivers/gpu/drm/msm/dp/dp_aux.c | 570 ++++++++++ > drivers/gpu/drm/msm/dp/dp_aux.h | 44 + > drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 ++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 144 +++ > drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475 +++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 + > drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++++ > drivers/gpu/drm/msm/dp/dp_debug.h | 81 ++ > drivers/gpu/drm/msm/dp/dp_display.c | 977 +++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_display.h | 55 + > drivers/gpu/drm/msm/dp/dp_drm.c | 542 ++++++++++ > drivers/gpu/drm/msm/dp/dp_drm.h | 52 + > drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++++ > drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++ > drivers/gpu/drm/msm/dp/dp_link.c | 1549 +++++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_link.h | 184 ++++ > drivers/gpu/drm/msm/dp/dp_panel.c | 624 +++++++++++ > drivers/gpu/drm/msm/dp/dp_panel.h | 121 +++ > drivers/gpu/drm/msm/dp/dp_parser.c | 679 ++++++++++++ > drivers/gpu/drm/msm/dp/dp_parser.h | 205 ++++ > drivers/gpu/drm/msm/dp/dp_power.c | 599 +++++++++++ > drivers/gpu/drm/msm/dp/dp_power.h | 57 + > drivers/gpu/drm/msm/dp/dp_reg.h | 357 ++++++ > drivers/gpu/drm/msm/msm_drv.c | 2 + > drivers/gpu/drm/msm/msm_drv.h | 22 + > include/drm/drm_dp_helper.h | 19 + > 30 files changed, 10887 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h > > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig > index 843a9d4..c363f24 100644 > --- a/drivers/gpu/drm/msm/Kconfig > +++ b/drivers/gpu/drm/msm/Kconfig > @@ -49,6 +49,15 @@ config DRM_MSM_HDMI_HDCP > help > Choose this option to enable HDCP state machine > > +config DRM_MSM_DP > + bool "Enable DP support in MSM DRM driver" > + depends on DRM_MSM > + default n default n should be implied so you don't need to add it. > + help > + Compile in support for DP driver in msm drm > + driver. DP external display support is enabled > + through this config option. It can be primary or > + secondary display on device. > config DRM_MSM_DSI > bool "Enable DSI support in MSM DRM driver" > depends on DRM_MSM > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > index 19ab521..765a8d8 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -2,6 +2,7 @@ > ccflags-y := -Idrivers/gpu/drm/msm > ccflags-y += -Idrivers/gpu/drm/msm/disp/dpu1 > ccflags-$(CONFIG_DRM_MSM_DSI) += -Idrivers/gpu/drm/msm/dsi > +ccflags-$(CONFIG_DRM_MSM_DP) += -Idrivers/gpu/drm/msm/dp > > msm-y := \ > adreno/adreno_device.o \ > @@ -93,7 +94,19 @@ msm-y := \ > msm_submitqueue.o > > msm-$(CONFIG_DEBUG_FS) += adreno/a5xx_debugfs.o \ > - disp/dpu1/dpu_dbg.o > + disp/dpu1/dpu_dbg.o \ > + dp/dp_debug.o > + > +msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_extcon.o \ > + dp/dp_parser.o \ > + dp/dp_power.o \ > + dp/dp_catalog.o \ > + dp/dp_aux.o \ > + dp/dp_panel.o \ > + dp/dp_link.o \ > + dp/dp_ctrl.o \ > + dp/dp_display.o \ > + dp/dp_drm.o > > msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o > msm-$(CONFIG_COMMON_CLK) += disp/mdp4/mdp4_lvds_pll.o > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c > index 790d39f..0f53a15 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c > @@ -14,10 +14,216 @@ > #include <linux/clk.h> > #include <linux/clk/clk-conf.h> > #include <linux/err.h> > +#include <linux/io.h> > +#include <linux/regulator/consumer.h> > #include <linux/delay.h> > > #include "dpu_io_util.h" > > +static struct resource *msm_dss_get_res_byname(struct platform_device *pdev, > + unsigned int type, > + const char *name) > +{ > + struct resource *res = NULL; > + > + res = platform_get_resource_byname(pdev, type, name); > + if (!res) > + DEV_ERR("%s: '%s' resource not found\n", __func__, name); > + > + return res; > +} /* msm_dss_get_res_byname */ > + > +int msm_dss_ioremap_byname(struct platform_device *pdev, > + struct dss_io_data *io_data, const char *name) > +{ > + struct resource *res = NULL; > + > + if (!pdev || !io_data) { > + DEV_ERR("%pS->%s: invalid input\n", > + __builtin_return_address(0), __func__); > + return -EINVAL; > + } > + > + res = msm_dss_get_res_byname(pdev, IORESOURCE_MEM, name); > + if (!res) { > + DEV_ERR("%pS->%s: '%s' msm_dss_get_res_byname failed\n", > + __builtin_return_address(0), __func__, name); > + return -ENODEV; > + } > + > + io_data->len = (u32)resource_size(res); > + io_data->base = ioremap(res->start, io_data->len); > + if (!io_data->base) { > + DEV_ERR("%pS->%s: '%s' ioremap failed\n", > + __builtin_return_address(0), __func__, name); > + return -EIO; > + } > + > + return 0; > +} /* msm_dss_ioremap_byname */ We already have a function called msm_ioremap() that you can use instead of this. Bonus, it uses devm_ so you don't need to worry about unmap. > + > +void msm_dss_iounmap(struct dss_io_data *io_data) > +{ > + if (!io_data) { > + DEV_ERR("%pS->%s: invalid input\n", > + __builtin_return_address(0), __func__); > + return; > + } > + > + if (io_data->base) { > + iounmap(io_data->base); > + io_data->base = NULL; > + } > + io_data->len = 0; > +} /* msm_dss_iounmap */ > if you use the function above you shouldn't need this one anymore. > +int msm_dss_config_vreg(struct device *dev, struct dss_vreg *in_vreg, > + int num_vreg, int config) > +{ > + int i = 0, rc = 0; > + struct dss_vreg *curr_vreg = NULL; > + enum dss_vreg_type type; > + > + if (!in_vreg || !num_vreg) > + return rc; > + > + if (config) { > + for (i = 0; i < num_vreg; i++) { > + curr_vreg = &in_vreg[i]; > + curr_vreg->vreg = regulator_get(dev, > + curr_vreg->vreg_name); > + rc = PTR_RET(curr_vreg->vreg); > + if (rc) { This should be IS_ERR(rc)) and then use PTR_ERR() to take it apart if you need it for your return value or error message. PTR_RET() has been deprecated. > + DEV_ERR("%pS->%s: %s get failed. rc=%d\n", > + __builtin_return_address(0), __func__, > + curr_vreg->vreg_name, rc); If you care about the stack here then consider using WARN instead of __builtin_return_address(0) and __func__. It will give you what you need in a standard way. > + curr_vreg->vreg = NULL; > + goto vreg_get_fail; > + } > + type = (regulator_count_voltages(curr_vreg->vreg) > 0) > + ? DSS_REG_LDO : DSS_REG_VS; > + if (type == DSS_REG_LDO) { > + rc = regulator_set_voltage( > + curr_vreg->vreg, > + curr_vreg->min_voltage, > + curr_vreg->max_voltage); > + if (rc < 0) { > + DEV_ERR("%pS->%s: %s set vltg fail\n", Same as above - use WARN if you can. Try not to use abbreviations in log messages - 'voltage' isn't much bigger and it is a lot more understandable especially for non native speakers. > + __builtin_return_address(0), > + __func__, > + curr_vreg->vreg_name); > + goto vreg_set_voltage_fail; > + } > + } > + } > + } else { This clause shares nothing with the rest of the function and proves there is no reason to not have separate functions for setting up and taking down instead of using the boolean flag. > + for (i = num_vreg-1; i >= 0; i--) { > + curr_vreg = &in_vreg[i]; > + if (curr_vreg->vreg) { > + type = (regulator_count_voltages( > + curr_vreg->vreg) > 0) > + ? DSS_REG_LDO : DSS_REG_VS; > + if (type == DSS_REG_LDO) { > + regulator_set_voltage(curr_vreg->vreg, > + 0, curr_vreg->max_voltage); > + } > + regulator_put(curr_vreg->vreg); > + curr_vreg->vreg = NULL; > + } > + } > + } > + return 0; > + > +vreg_unconfig: > +if (type == DSS_REG_LDO) > + regulator_set_load(curr_vreg->vreg, 0); Both paths above use regulator_set_voltage(), so I'm not sure if regulator_set_load() is what you intend here. > + > +vreg_set_voltage_fail: > + regulator_put(curr_vreg->vreg); > + curr_vreg->vreg = NULL; > + > +vreg_get_fail: > + for (i--; i >= 0; i--) { > + curr_vreg = &in_vreg[i]; > + type = (regulator_count_voltages(curr_vreg->vreg) > 0) > + ? DSS_REG_LDO : DSS_REG_VS; > + goto vreg_unconfig; Oh my - jumping back out of a loop with goto is not okay. This entire block is basically just trying to take the place of the takedown clause above so if you use a separate function to put away the regulators you can just call that instead. > + } > + return rc; > +} /* msm_dss_config_vreg */ Maybe a nit, but we all know how to read C code. We don't need a comment to let us know when a bracket closes out a function. > + > +int msm_dss_enable_vreg(struct dss_vreg *in_vreg, int num_vreg, int enable) > +{ > + int i = 0, rc = 0; > + bool need_sleep; > + > + if (enable) { > + for (i = 0; i < num_vreg; i++) { > + rc = PTR_RET(in_vreg[i].vreg); > + if (rc) { > + DEV_ERR("%pS->%s: %s regulator error. rc=%d\n", > + __builtin_return_address(0), __func__, > + in_vreg[i].vreg_name, rc); As above - use WARN if you can. > + goto vreg_set_opt_mode_fail; > + } > + need_sleep = !regulator_is_enabled(in_vreg[i].vreg); > + if (in_vreg[i].pre_on_sleep && need_sleep) > + usleep_range(in_vreg[i].pre_on_sleep * 1000, > + in_vreg[i].pre_on_sleep * 1000); > + rc = regulator_set_load(in_vreg[i].vreg, > + in_vreg[i].enable_load); > + if (rc < 0) { > + DEV_ERR("%pS->%s: %s set opt m fail\n", > + __builtin_return_address(0), __func__, > + in_vreg[i].vreg_name); > + goto vreg_set_opt_mode_fail; > + } > + rc = regulator_enable(in_vreg[i].vreg); > + if (in_vreg[i].post_on_sleep && need_sleep) > + usleep_range(in_vreg[i].post_on_sleep * 1000, > + in_vreg[i].post_on_sleep * 1000); > + if (rc < 0) { > + DEV_ERR("%pS->%s: %s enable failed\n", > + __builtin_return_address(0), __func__, > + in_vreg[i].vreg_name); > + goto disable_vreg; > + } > + } > + } else { Also as above - this entire clause can stand alone in its own function. > + for (i = num_vreg-1; i >= 0; i--) { > + if (in_vreg[i].pre_off_sleep) > + usleep_range(in_vreg[i].pre_off_sleep * 1000, > + in_vreg[i].pre_off_sleep * 1000); > + regulator_set_load(in_vreg[i].vreg, > + in_vreg[i].disable_load); > + regulator_disable(in_vreg[i].vreg); > + if (in_vreg[i].post_off_sleep) > + usleep_range(in_vreg[i].post_off_sleep * 1000, > + in_vreg[i].post_off_sleep * 1000); > + } > + } > + return rc; > + > +disable_vreg: > + regulator_set_load(in_vreg[i].vreg, in_vreg[i].disable_load); > + > +vreg_set_opt_mode_fail: > + for (i--; i >= 0; i--) { > + if (in_vreg[i].pre_off_sleep) > + usleep_range(in_vreg[i].pre_off_sleep * 1000, > + in_vreg[i].pre_off_sleep * 1000); > + regulator_set_load(in_vreg[i].vreg, > + in_vreg[i].disable_load); > + regulator_disable(in_vreg[i].vreg); > + if (in_vreg[i].post_off_sleep) > + usleep_range(in_vreg[i].post_off_sleep * 1000, > + in_vreg[i].post_off_sleep * 1000); > + } > This is essentially a recreation of the code above - you can just use the standalone function for teardown. > + return rc; > +} /* msm_dss_enable_vreg */ > + > + > void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk) > { > int i; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h > index bc07381..524b394 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h > @@ -15,6 +15,7 @@ > > #include <linux/gpio.h> > #include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > #include <linux/types.h> > > #define DEV_DBG(fmt, args...) pr_debug(fmt, ##args) > @@ -22,6 +23,39 @@ > #define DEV_WARN(fmt, args...) pr_warn(fmt, ##args) > #define DEV_ERR(fmt, args...) pr_err(fmt, ##args) This isn't part of this patch but I would love to see these guys turn into standard functions. > +struct dss_io_data { > + u32 len; > + void __iomem *base; > +}; > + > +/* > +void dss_reg_w(struct dss_io_data *io, u32 offset, u32 value, u32 debug); > +u32 dss_reg_r(struct dss_io_data *io, u32 offset, u32 debug); > +void dss_reg_dump(void __iomem *base, u32 len, const char *prefix, u32 debug); > + > +#define DSS_REG_W_ND(io, offset, val) dss_reg_w(io, offset, val, false) > +#define DSS_REG_W(io, offset, val) dss_reg_w(io, offset, val, true) > +#define DSS_REG_R_ND(io, offset) dss_reg_r(io, offset, false) > +#define DSS_REG_R(io, offset) dss_reg_r(io, offset, true) > +*/ > +enum dss_vreg_type { > + DSS_REG_LDO, > + DSS_REG_VS, > +}; > + > +struct dss_vreg { > + struct regulator *vreg; /* vreg handle */ > + char vreg_name[32]; > + int min_voltage; > + int max_voltage; > + int enable_load; > + int disable_load; > + int pre_on_sleep; > + int post_on_sleep; > + int pre_off_sleep; > + int post_off_sleep; > +}; > + > struct dss_gpio { > unsigned int gpio; > unsigned int value; > @@ -42,12 +76,22 @@ struct dss_clk { > }; > > struct dss_module_power { > + unsigned int num_vreg; > + struct dss_vreg *vreg_config; > unsigned int num_gpio; > struct dss_gpio *gpio_config; > unsigned int num_clk; > struct dss_clk *clk_config; > }; > > +int msm_dss_ioremap_byname(struct platform_device *pdev, > + struct dss_io_data *io_data, const char *name); > +void msm_dss_iounmap(struct dss_io_data *io_data); > + > +int msm_dss_config_vreg(struct device *dev, struct dss_vreg *in_vreg, > + int num_vreg, int config); > +int msm_dss_enable_vreg(struct dss_vreg *in_vreg, int num_vreg, int enable); > + > int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk); > void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk); > int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk); > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > new file mode 100644 > index 0000000..00b82c2 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -0,0 +1,570 @@ > +/* > + * 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. > + * > + */ Please use SPDX headers. > +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__ > + > +#include <linux/delay.h> > + > +#include "dp_aux.h" > + > +#define DP_AUX_ENUM_STR(x) #x You should use this #define closer to where it is used. > + > +enum { > + DP_AUX_DATA_INDEX_WRITE = BIT(31), > +}; This enum of one is not needed - this could as easily be a #define or BIT(31) used inline. > +struct dp_aux_private { > + struct device *dev; > + struct dp_aux dp_aux; > + struct dp_catalog_aux *catalog; > + struct dp_aux_cfg *cfg; > + > + struct mutex mutex; > + struct completion comp; > + > + u32 aux_error_num; > + u32 retry_cnt; > + bool cmd_busy; > + bool native; > + bool read; > + bool no_send_addr; > + bool no_send_stop; > + u32 offset; > + u32 segment; > + > + struct drm_dp_aux drm_aux; > +}; > + > +static char *dp_aux_get_error(u32 aux_error) > +{ > + switch (aux_error) { > + case DP_AUX_ERR_NONE: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE); > + case DP_AUX_ERR_ADDR: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR); > + case DP_AUX_ERR_TOUT: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT); > + case DP_AUX_ERR_NACK: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK); > + case DP_AUX_ERR_DEFER: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER); > + case DP_AUX_ERR_NACK_DEFER: > + return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER); > + default: > + return "unknown"; > + } > +} > + > +static u32 dp_aux_write(struct dp_aux_private *aux, > + struct drm_dp_aux_msg *msg) > +{ > + u32 data[4], reg, len; > + u8 *msgdata = msg->buffer; > + int const aux_cmd_fifo_len = 128; This isn't useful since it is only used oncex, and constant values like 128 can be safely used inline when it is needed. > + int i = 0; > + > + if (aux->read) > + len = 4; > + else > + len = msg->size + 4; > + > + /* > + * cmd fifo only has depth of 144 bytes > + * limit buf length to 128 bytes here > + */ > + if (len > aux_cmd_fifo_len) { > + pr_err("buf len error\n"); I'm not sure if its possible, but if we could get to dev_err for these that would be fantastic (but don't go overboard if it is hard to get the dev pointer all the way down here). That said, this error message should be more informative - it should at least say what the offending length is. > + return 0; > + } > + > + /* Pack cmd and write to HW */ > + data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */ > + if (aux->read) > + data[0] |= BIT(4); /* R/W */ > + > + data[1] = (msg->address >> 8) & 0xff; /* addr[15:8] */ > + data[2] = msg->address & 0xff; /* addr[7:0] */ > + data[3] = (msg->size - 1) & 0xff; /* len[7:0] */ > + > + for (i = 0; i < len; i++) { > + reg = (i < 4) ? data[i] : msgdata[i - 4]; > + reg = ((reg) << 8) & 0x0000ff00; /* index = 0, write */ > + if (i == 0) > + reg |= DP_AUX_DATA_INDEX_WRITE; > + aux->catalog->data = reg; > + aux->catalog->write_data(aux->catalog); > + } > + > + aux->catalog->clear_trans(aux->catalog, false); > + > + reg = 0; /* Transaction number == 1 */ > + if (!aux->native) { /* i2c */ > + reg |= BIT(8); > + > + if (aux->no_send_addr) > + reg |= BIT(10); > + > + if (aux->no_send_stop) > + reg |= BIT(11); > + } > + > + reg |= BIT(9); > + aux->catalog->data = reg; > + aux->catalog->write_trans(aux->catalog); > + > + return len; > +} > + > +static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux, > + struct drm_dp_aux_msg *msg) > +{ > + u32 ret = 0, len = 0, timeout; > + int const aux_timeout_ms = HZ/4; As above. Generally I don't like const variables on the stack and especially if they are as simple as this. This forces the reviewer to look at the line where it is used, and then back at the initialization to see what the value is. > + > + reinit_completion(&aux->comp); > + > + len = dp_aux_write(aux, msg); > + if (len == 0) { > + pr_err("DP AUX write failed\n"); There is only one way len can be zero and you already print an error message for it. You don't need another one. > + return -EINVAL; > + } > + > + timeout = wait_for_completion_timeout(&aux->comp, aux_timeout_ms); This can easily be HZ/4 for the timeout > + if (!timeout) { > + pr_err("aux %s timeout\n", (aux->read ? "read" : "write")); > + return -ETIMEDOUT; > + } > + > + if (aux->aux_error_num == DP_AUX_ERR_NONE) { > + ret = len; > + } else { > + pr_err_ratelimited("aux err: %s\n", > + dp_aux_get_error(aux->aux_error_num)); > + > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +static void dp_aux_cmd_fifo_rx(struct dp_aux_private *aux, > + struct drm_dp_aux_msg *msg) > +{ > + u32 data; > + u8 *dp; > + u32 i, actual_i; > + u32 len = msg->size; > + > + aux->catalog->clear_trans(aux->catalog, true); > + > + data = 0; > + data |= DP_AUX_DATA_INDEX_WRITE; /* INDEX_WRITE */ > + data |= BIT(0); /* read */ > + > + aux->catalog->data = data; > + aux->catalog->write_data(aux->catalog); > + > + dp = msg->buffer; > + > + /* discard first byte */ > + data = aux->catalog->read_data(aux->catalog); > + > + for (i = 0; i < len; i++) { > + data = aux->catalog->read_data(aux->catalog); > + *dp++ = (u8)((data >> 8) & 0xff); > + > + actual_i = (data >> 16) & 0xFF; > + if (i != actual_i) > + pr_warn("Index mismatch: expected %d, found %d\n", > + i, actual_i); > + } > +} > + > +static void dp_aux_native_handler(struct dp_aux_private *aux) > +{ > + u32 isr = aux->catalog->isr; > + > + if (isr & DP_INTR_AUX_I2C_DONE) > + aux->aux_error_num = DP_AUX_ERR_NONE; > + else if (isr & DP_INTR_WRONG_ADDR) > + aux->aux_error_num = DP_AUX_ERR_ADDR; > + else if (isr & DP_INTR_TIMEOUT) > + aux->aux_error_num = DP_AUX_ERR_TOUT; > + if (isr & DP_INTR_NACK_DEFER) > + aux->aux_error_num = DP_AUX_ERR_NACK; > + > + complete(&aux->comp); > +} > + > +static void dp_aux_i2c_handler(struct dp_aux_private *aux) > +{ > + u32 isr = aux->catalog->isr; > + > + if (isr & DP_INTR_AUX_I2C_DONE) { > + if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER)) > + aux->aux_error_num = DP_AUX_ERR_NACK; > + else > + aux->aux_error_num = DP_AUX_ERR_NONE; > + } else { > + if (isr & DP_INTR_WRONG_ADDR) > + aux->aux_error_num = DP_AUX_ERR_ADDR; > + else if (isr & DP_INTR_TIMEOUT) > + aux->aux_error_num = DP_AUX_ERR_TOUT; > + if (isr & DP_INTR_NACK_DEFER) > + aux->aux_error_num = DP_AUX_ERR_NACK_DEFER; > + if (isr & DP_INTR_I2C_NACK) > + aux->aux_error_num = DP_AUX_ERR_NACK; > + if (isr & DP_INTR_I2C_DEFER) > + aux->aux_error_num = DP_AUX_ERR_DEFER; > + } > + > + complete(&aux->comp); > +} > + > +static void dp_aux_isr(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux) { > + pr_err("invalid input\n"); > + return; > + } I'm not 100% if this is possible or not but consider replacing this with if (WARN_ON(!dp_aux)) return; To avoid the custom error message yet still handle the possibility of a NULL pointer. Of course, if it isn't possible then it should just be removed entirely. > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + aux->catalog->get_irq(aux->catalog, aux->cmd_busy); > + > + if (!aux->cmd_busy) > + return; > + > + if (aux->native) > + dp_aux_native_handler(aux); > + else > + dp_aux_i2c_handler(aux); > +} > + > +static void dp_aux_reconfig(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux) { > + pr_err("invalid input\n"); > + return; > + } As above - WARN_ON can be useful here. > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + aux->catalog->update_aux_cfg(aux->catalog, > + aux->cfg, PHY_AUX_CFG1); > + aux->catalog->reset(aux->catalog); > +} > + > +static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux, > + struct drm_dp_aux_msg *input_msg) > +{ > + u32 const edid_address = 0x50; > + u32 const segment_address = 0x30; Prefer if you didn't use const variables on the stack. They seem to be popular in this code so I won't point them out every time. > + bool i2c_read = input_msg->request & > + (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ); > + u8 *data = NULL; > + > + if (aux->native || i2c_read || ((input_msg->address != edid_address) && > + (input_msg->address != segment_address))) > + return; > + > + > + data = input_msg->buffer; > + if (input_msg->address == segment_address) > + aux->segment = *data; > + else > + aux->offset = *data; > +} <snip> > +static void dp_aux_init(struct dp_aux *dp_aux, struct dp_aux_cfg *aux_cfg) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux || !aux_cfg) { > + pr_err("invalid input\n"); > + return; > + } Prefer to use WARN_ON here instead of a custom error message. > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + dp_aux_reset_phy_config_indices(aux_cfg); > + aux->catalog->setup(aux->catalog, aux_cfg); > + aux->catalog->reset(aux->catalog); > + aux->catalog->enable(aux->catalog, true); > + aux->retry_cnt = 0; > +} > + > +static void dp_aux_deinit(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux) { > + pr_err("invalid input\n"); > + return; > + } Pefer WARN_ON. > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + aux->catalog->enable(aux->catalog, false); > +} > + > +static int dp_aux_register(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + int ret = 0; > + > + if (!dp_aux) { > + pr_err("invalid input\n"); Prefer WARN_ON. > + ret = -EINVAL; You can just return -EINVAL; > + goto exit; > + } > + > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + aux->drm_aux.name = "dpu_dp_aux"; > + aux->drm_aux.dev = aux->dev; > + aux->drm_aux.transfer = dp_aux_transfer; > + ret = drm_dp_aux_register(&aux->drm_aux); > + if (ret) { > + pr_err("%s: failed to register drm aux: %d\n", __func__, ret); Don't forget that your pr_fmt already prints __func__. You don't need to dobule it up. > + goto exit; This can be return ret; > + } > + dp_aux->drm_aux = &aux->drm_aux; > +exit: Label not needed. > + return ret; This can be return 0; > +} > + > +static void dp_aux_deregister(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux) { > + pr_err("invalid input\n"); > + return; > + } Prefer WARN_ON > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + drm_dp_aux_unregister(&aux->drm_aux); > +} > + > +struct dp_aux *dp_aux_get(struct device *dev, struct dp_catalog_aux *catalog, > + struct dp_aux_cfg *aux_cfg) > +{ > + int rc = 0; > + struct dp_aux_private *aux; > + struct dp_aux *dp_aux; > + > + if (!catalog || !aux_cfg) { > + pr_err("invalid input\n"); > + rc = -ENODEV; > + goto error; > + } > This appears to only be called once with a known good value for catalog and aux_cfg, so this check does not appear to be needed. It also implies that the NULL checks (or WARN_ON) suggested for the functions above are not needed since all of them are to be called with known good pointers. In fact, even if you keep this check (with a WARN_ON instead of a custom message of course) you can still remove the checks in dp_aux_isr, dp_aux_init, dp_aux_deinit, dp_aux_register, dp_aux_deregister and dp_aux_reconfig because you've already vetted the pointers. > + aux = devm_kzalloc(dev, sizeof(*aux), GFP_KERNEL); > + if (!aux) { This can just be return ERR_PTR(-ENOMEM); > + rc = -ENOMEM; > + goto error; > + } > + > + init_completion(&aux->comp); > + aux->cmd_busy = false; > + mutex_init(&aux->mutex); > + > + aux->dev = dev; > + aux->catalog = catalog; > + aux->cfg = aux_cfg; > + dp_aux = &aux->dp_aux; > + aux->retry_cnt = 0; > + > + dp_aux->isr = dp_aux_isr; > + dp_aux->init = dp_aux_init; > + dp_aux->deinit = dp_aux_deinit; > + dp_aux->drm_aux_register = dp_aux_register; > + dp_aux->drm_aux_deregister = dp_aux_deregister; > + dp_aux->reconfig = dp_aux_reconfig; > + > + return dp_aux; > +error: > + return ERR_PTR(rc); This label should not be needed. > +} > + > +void dp_aux_put(struct dp_aux *dp_aux) > +{ > + struct dp_aux_private *aux; > + > + if (!dp_aux) > + return; > + > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + mutex_destroy(&aux->mutex); Mutex_destroy isn't needed for memory that will no longer be used. > + > + devm_kfree(aux->dev, aux); If you are using devm_ you don't need to call devm_kfree. If you don't expect to use the devm_ infrastructure to your advantage then just use kzalloc() in the init function and make sure you always free it but it seems to me you can just get rid of dp_aux_put() and be happy. > +} > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h > new file mode 100644 > index 0000000..0c300ed > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_aux.h > @@ -0,0 +1,44 @@ > +/* > + * 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. > + * > + */ SPDX header please. > + > +#ifndef _DP_AUX_H_ > +#define _DP_AUX_H_ > + > +#include "dp_catalog.h" > +#include <drm/drm_dp_helper.h> > + > +enum dp_aux_error { > + DP_AUX_ERR_NONE = 0, > + DP_AUX_ERR_ADDR = -1, > + DP_AUX_ERR_TOUT = -2, > + DP_AUX_ERR_NACK = -3, > + DP_AUX_ERR_DEFER = -4, > + DP_AUX_ERR_NACK_DEFER = -5, > +}; > + > +struct dp_aux { > + struct drm_dp_aux *drm_aux; > + int (*drm_aux_register)(struct dp_aux *aux); > + void (*drm_aux_deregister)(struct dp_aux *aux); > + void (*isr)(struct dp_aux *aux); > + void (*init)(struct dp_aux *aux, struct dp_aux_cfg *aux_cfg); > + void (*deinit)(struct dp_aux *aux); > + void (*reconfig)(struct dp_aux *aux); > +}; > + > +struct dp_aux *dp_aux_get(struct device *dev, struct dp_catalog_aux *catalog, > + struct dp_aux_cfg *aux_cfg); > +void dp_aux_put(struct dp_aux *aux); > + > +#endif /*__DP_AUX_H_*/ > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > new file mode 100644 > index 0000000..5818612 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -0,0 +1,1188 @@ > +/* > + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ SPDX header please. > + > +#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) > + > +#define dp_read(offset) readl_relaxed((offset)) > +#define dp_write(offset, data) writel_relaxed((data), (offset)) These macros do not serve any purpose > +#define dp_catalog_get_priv(x) { \ > + 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); \ > +} This would work better as a static inline function - it wouldn't be as ugly and you get free typechecking. > +#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) > + > +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"); > + goto end; > + } Not sure if this check is needed (I'm assuming I'll figure it as we go) but as above, replacing this with a handy if (WARN_ON(!aux)) return 0; Is preferred over a custom log message > + > + dp_catalog_get_priv(aux); Function like macros that set an implicit variable are so very dangerous - please use a static inline as I suggsted above. > + base = catalog->io->dp_aux.base; > + > + return dp_read(base + DP_AUX_DATA); > +end: This shouldn't be needed if you return immediately above. > + return 0; > +} > + > +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; > + } Same as above > + dp_catalog_get_priv(aux); > + base = catalog->io->dp_aux.base; > + > + dp_write(base + DP_AUX_DATA, aux->data); > +end: > + return rc; Same as above > +} > + > +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; > +} dp_catalog_aux_write_data and dp_catalog_aux_write_trans are basically the same function except the register offset. There must be some conslidation that can be done. > +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; > + } Use WARN_ON() here and throughout for all these dp catalog function hooks. > + 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); > + dp_write(base + DP_AUX_CTRL, aux_ctrl); > + usleep_range(1000, 1010); /* h/w recommended delay */ > + > + aux_ctrl &= ~BIT(1); > + dp_write(base + DP_AUX_CTRL, aux_ctrl); > + wmb(); > +} > + > +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; Even at the very worse a common function to derive the base would save lines of code given how much it gets used. > + > + 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); > + } else { > + aux_ctrl &= ~BIT(0); > + } > + > + dp_write(base + DP_AUX_CTRL, aux_ctrl); > +} <snip> > +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); > +} > Consolidation of dp_link write could also be done among these three. > +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 */ > + } 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; > + misc_val |= (tb << 5); > + 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; > + u32 const link_rate_hbr2 = 540000; > + u32 const link_rate_hbr3 = 810000; Put these constants inline rather than using a const on the stack. > + 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; You set a const variable, then you ste a normal variable and you only use it once - you could cut out all the middlemen by just hardcoding 0x8000 here. > + 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; > + } > + > + 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); > +} <snip> > +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; I'm assuming that whoever sets up the catalog in the first place is verifying that sdp and header are both in bounds for the audio_map array? > + audio->data = dp_read(base + sdp_map[sdp][header]); > +} > + > +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; Same question. > + > + dp_write(base + sdp_map[sdp][header], data); > +} > + > +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); > +} > + > +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); > +} > + > +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(); > +} > + > +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, > + }; > + 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, > + }; > + struct dp_catalog_audio audio = { > + .init = dp_catalog_audio_init, > + .config_acr = dp_catalog_audio_config_acr, > + .enable = dp_catalog_audio_enable, > + .config_sdp = dp_catalog_audio_config_sdp, > + .set_header = dp_catalog_audio_set_header, > + .get_header = dp_catalog_audio_get_header, > + .safe_to_exit_level = dp_catalog_audio_safe_to_exit_level, > + }; > + struct dp_catalog_panel panel = { > + .timing_cfg = dp_catalog_panel_timing_cfg, > + .tpg_config = dp_catalog_panel_tpg_cfg, > + }; > + > + if (!io) { > + pr_err("invalid input\n"); > + rc = -EINVAL; > + goto error; > + } It doesn't appear that io can be NULL here. > + > + catalog = devm_kzalloc(dev, sizeof(*catalog), GFP_KERNEL); > + if (!catalog) { You can just return ERR_PTR(-ENOMEM) here > + rc = -ENOMEM; > + goto error; > + } > + > + catalog->dev = dev; > + catalog->io = io; > + > + dp_catalog = &catalog->dp_catalog; > + > + dp_catalog->aux = aux; > + dp_catalog->ctrl = ctrl; > + dp_catalog->audio = audio; > + dp_catalog->panel = panel; I'll use this as an example because it is the shortest, but it should apply to all these members. You can set the value of dp_catalog->panel directly without an intermediate variable: dp_catalog->panel = { .timing_cfg = dp_catalog_panel_timing_cfg, .tpg_config = dp_catalog_panel_tpg_cfg, }; Its a small change but it makes it easier to understand what is going on in this function. > + > + return dp_catalog; > +error: > + return ERR_PTR(rc); This shouldn't be needed anymore. > +} > + > +void dp_catalog_put(struct dp_catalog *dp_catalog) > +{ > + struct dp_catalog_private *catalog; > + > + if (!dp_catalog) > + return; > + > + catalog = container_of(dp_catalog, struct dp_catalog_private, > + dp_catalog); > + > + devm_kfree(catalog->dev, catalog); You don't need to devm_kfree managed memory - just let it go. > +} > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h > new file mode 100644 > index 0000000..58951df > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > @@ -0,0 +1,144 @@ > +/* > + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ SPDX please. > +#ifndef _DP_CATALOG_H_ > +#define _DP_CATALOG_H_ > + > +#include "dp_parser.h" > + > +/* interrupts */ > +#define DP_INTR_HPD BIT(0) > +#define DP_INTR_AUX_I2C_DONE BIT(3) > +#define DP_INTR_WRONG_ADDR BIT(6) > +#define DP_INTR_TIMEOUT BIT(9) > +#define DP_INTR_NACK_DEFER BIT(12) > +#define DP_INTR_WRONG_DATA_CNT BIT(15) > +#define DP_INTR_I2C_NACK BIT(18) > +#define DP_INTR_I2C_DEFER BIT(21) > +#define DP_INTR_PLL_UNLOCKED BIT(24) > +#define DP_INTR_AUX_ERROR BIT(27) > + > +#define DP_INTR_READY_FOR_VIDEO BIT(0) > +#define DP_INTR_IDLE_PATTERN_SENT BIT(3) > +#define DP_INTR_FRAME_END BIT(6) > +#define DP_INTR_CRC_UPDATED BIT(9) > + > +struct dp_catalog_aux { > + u32 data; > + u32 isr; > + > + u32 (*read_data)(struct dp_catalog_aux *aux); > + int (*write_data)(struct dp_catalog_aux *aux); > + int (*write_trans)(struct dp_catalog_aux *aux); > + int (*clear_trans)(struct dp_catalog_aux *aux, bool read); > + void (*reset)(struct dp_catalog_aux *aux); > + void (*enable)(struct dp_catalog_aux *aux, bool enable); > + void (*update_aux_cfg)(struct dp_catalog_aux *aux, > + struct dp_aux_cfg *cfg, enum dp_phy_aux_config_type type); > + void (*setup)(struct dp_catalog_aux *aux, > + struct dp_aux_cfg *aux_cfg); > + void (*get_irq)(struct dp_catalog_aux *aux, bool cmd_busy); > +}; > + > +struct dp_catalog_ctrl { > + u32 dp_tu; > + u32 valid_boundary; > + u32 valid_boundary2; > + u32 isr; > + > + void (*state_ctrl)(struct dp_catalog_ctrl *ctrl, u32 state); > + void (*config_ctrl)(struct dp_catalog_ctrl *ctrl, u32 config); > + void (*lane_mapping)(struct dp_catalog_ctrl *ctrl); > + void (*mainlink_ctrl)(struct dp_catalog_ctrl *ctrl, bool enable); > + void (*config_misc)(struct dp_catalog_ctrl *ctrl, u32 cc, u32 tb); > + void (*config_msa)(struct dp_catalog_ctrl *ctrl, u32 rate, > + u32 stream_rate_khz, bool fixed_nvid); > + void (*set_pattern)(struct dp_catalog_ctrl *ctrl, u32 pattern); > + void (*reset)(struct dp_catalog_ctrl *ctrl); > + void (*usb_reset)(struct dp_catalog_ctrl *ctrl, bool flip); > + bool (*mainlink_ready)(struct dp_catalog_ctrl *ctrl); > + void (*enable_irq)(struct dp_catalog_ctrl *ctrl, bool enable); > + void (*hpd_config)(struct dp_catalog_ctrl *ctrl, bool enable); > + void (*phy_reset)(struct dp_catalog_ctrl *ctrl); > + void (*phy_lane_cfg)(struct dp_catalog_ctrl *ctrl, bool flipped, > + u8 lane_cnt); > + void (*update_vx_px)(struct dp_catalog_ctrl *ctrl, u8 v_level, > + u8 p_level); > + void (*get_interrupt)(struct dp_catalog_ctrl *ctrl); > + void (*update_transfer_unit)(struct dp_catalog_ctrl *ctrl); > + void (*send_phy_pattern)(struct dp_catalog_ctrl *ctrl, > + u32 pattern); > + u32 (*read_phy_pattern)(struct dp_catalog_ctrl *ctrl); > +}; > + > +enum dp_catalog_audio_sdp_type { > + DP_AUDIO_SDP_STREAM, > + DP_AUDIO_SDP_TIMESTAMP, > + DP_AUDIO_SDP_INFOFRAME, > + DP_AUDIO_SDP_COPYMANAGEMENT, > + DP_AUDIO_SDP_ISRC, > + DP_AUDIO_SDP_MAX, > +}; > + > +enum dp_catalog_audio_header_type { > + DP_AUDIO_SDP_HEADER_1, > + DP_AUDIO_SDP_HEADER_2, > + DP_AUDIO_SDP_HEADER_3, > + DP_AUDIO_SDP_HEADER_MAX, > +}; > + > +struct dp_catalog_audio { > + enum dp_catalog_audio_sdp_type sdp_type; > + enum dp_catalog_audio_header_type sdp_header; > + u32 data; > + > + void (*init)(struct dp_catalog_audio *audio); > + void (*enable)(struct dp_catalog_audio *audio); > + void (*config_acr)(struct dp_catalog_audio *audio); > + void (*config_sdp)(struct dp_catalog_audio *audio); > + void (*set_header)(struct dp_catalog_audio *audio); > + void (*get_header)(struct dp_catalog_audio *audio); > + void (*safe_to_exit_level)(struct dp_catalog_audio *audio); > +}; > + > +struct dp_catalog_panel { > + u32 total; > + u32 sync_start; > + u32 width_blanking; > + u32 dp_active; > + > + /* TPG */ > + u32 hsync_period; > + u32 vsync_period; > + u32 display_v_start; > + u32 display_v_end; > + u32 v_sync_width; > + u32 hsync_ctl; > + u32 display_hctl; > + > + int (*timing_cfg)(struct dp_catalog_panel *panel); > + void (*tpg_config)(struct dp_catalog_panel *panel, bool enable); > +}; > + > +struct dp_catalog { > + struct dp_catalog_aux aux; > + struct dp_catalog_ctrl ctrl; > + struct dp_catalog_audio audio; > + struct dp_catalog_panel panel; > +}; > + > +struct dp_catalog *dp_catalog_get(struct device *dev, struct dp_io *io); > +void dp_catalog_put(struct dp_catalog *catalog); > + > +#endif /* _DP_CATALOG_H_ */ > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > new file mode 100644 > index 0000000..08a52f5 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -0,0 +1,1475 @@ > +/* > + * 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. > + * > + */ SPDX please. > +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__ > + > +#include <linux/types.h> > +#include <linux/completion.h> > +#include <linux/delay.h> > + > +#include "dp_ctrl.h" > + > +#define DP_KHZ_TO_HZ 1000 > + > +#define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0) > +#define DP_CTRL_INTR_IDLE_PATTERN_SENT BIT(3) > + > +/* dp state ctrl */ > +#define ST_TRAIN_PATTERN_1 BIT(0) > +#define ST_TRAIN_PATTERN_2 BIT(1) > +#define ST_TRAIN_PATTERN_3 BIT(2) > +#define ST_TRAIN_PATTERN_4 BIT(3) > +#define ST_SYMBOL_ERR_RATE_MEASUREMENT BIT(4) > +#define ST_PRBS7 BIT(5) > +#define ST_CUSTOM_80_BIT_PATTERN BIT(6) > +#define ST_SEND_VIDEO BIT(7) > +#define ST_PUSH_IDLE BIT(8) > + > +#define MR_LINK_TRAINING1 0x8 > +#define MR_LINK_SYMBOL_ERM 0x80 > +#define MR_LINK_PRBS7 0x100 > +#define MR_LINK_CUSTOM80 0x200 > + > +struct dp_vc_tu_mapping_table { > + u32 vic; > + u8 lanes; > + u8 lrate; /* DP_LINK_RATE -> 162(6), 270(10), 540(20), 810 (30) */ > + u8 bpp; > + u8 valid_boundary_link; > + u16 delay_start_link; > + bool boundary_moderation_en; > + u8 valid_lower_boundary_link; > + u8 upper_boundary_count; > + u8 lower_boundary_count; > + u8 tu_size_minus1; > +}; > + > +struct dp_ctrl_private { > + struct dp_ctrl dp_ctrl; > + > + struct device *dev; > + struct dp_aux *aux; > + struct dp_panel *panel; > + struct dp_link *link; > + struct dp_power *power; > + struct dp_parser *parser; > + struct dp_catalog_ctrl *catalog; > + > + struct completion idle_comp; > + struct completion video_comp; > + > + bool orientation; > + atomic_t aborted; > + > + u32 pixel_rate; > + u32 vic; > +}; > + > +enum notification_status { > + NOTIFY_UNKNOWN, > + NOTIFY_CONNECT, > + NOTIFY_DISCONNECT, > + NOTIFY_CONNECT_IRQ_HPD, > + NOTIFY_DISCONNECT_IRQ_HPD, > +}; > + > +static void dp_ctrl_idle_patterns_sent(struct dp_ctrl_private *ctrl) > +{ > + pr_debug("idle_patterns_sent\n"); > + complete(&ctrl->idle_comp); > +} > + > +static void dp_ctrl_video_ready(struct dp_ctrl_private *ctrl) > +{ > + pr_debug("dp_video_ready\n"); > + complete(&ctrl->video_comp); > +} > + > +static void dp_ctrl_abort(struct dp_ctrl *dp_ctrl) > +{ > + struct dp_ctrl_private *ctrl; > + > + if (!dp_ctrl) { > + pr_err("Invalid input data\n"); > + return; > + } Common theme - please try to use WARN_ON instead of a custom error message. > + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); > + > + atomic_set(&ctrl->aborted, 1); > +} > + > +static void dp_ctrl_state_ctrl(struct dp_ctrl_private *ctrl, u32 state) > +{ > + ctrl->catalog->state_ctrl(ctrl->catalog, state); > +} > + > +static void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl) > +{ > + int const idle_pattern_completion_timeout_ms = 3 * HZ / 100; > + 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); > + > + reinit_completion(&ctrl->idle_comp); > + dp_ctrl_state_ctrl(ctrl, ST_PUSH_IDLE); > + > + if (!wait_for_completion_timeout(&ctrl->idle_comp, > + idle_pattern_completion_timeout_ms)) > + pr_warn("PUSH_IDLE pattern timedout\n"); > + > + pr_debug("mainlink off done\n"); > +} > + > +static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl) > +{ > + u32 config = 0, tbd; > + u8 *dpcd = ctrl->panel->dpcd; > + > + config |= (2 << 13); /* Default-> LSCLK DIV: 1/4 LCLK */ > + config |= (0 << 11); /* RGB */ > + > + /* Scrambler reset enable */ > + if (dpcd[DP_EDP_CONFIGURATION_CAP] & DP_ALTERNATE_SCRAMBLER_RESET_CAP) > + config |= (1 << 10); > + > + tbd = ctrl->link->get_test_bits_depth(ctrl->link, > + ctrl->panel->pinfo.bpp); > + > + if (tbd == DP_TEST_BIT_DEPTH_UNKNOWN) > + tbd = DP_TEST_BIT_DEPTH_8; > + > + config |= tbd << 8; > + > + /* Num of Lanes */ > + config |= ((ctrl->link->link_params.lane_count - 1) << 4); > + > + if (drm_dp_enhanced_frame_cap(dpcd)) > + config |= 0x40; > + > + config |= 0x04; /* progressive video */ > + > + config |= 0x03; /* sycn clock & static Mvid */ > + > + ctrl->catalog->config_ctrl(ctrl->catalog, config); > +} > + > +/** > + * dp_ctrl_configure_source_params() - configures DP transmitter source params > + * @ctrl: Display Port Driver data > + * > + * Configures the DP transmitter source params including details such as lane > + * configuration, output format and sink/panel timing information. > + */ > +static void dp_ctrl_configure_source_params(struct dp_ctrl_private *ctrl) > +{ > + u32 cc, tb; > + > + ctrl->catalog->lane_mapping(ctrl->catalog); > + ctrl->catalog->mainlink_ctrl(ctrl->catalog, true); > + > + dp_ctrl_config_ctrl(ctrl); > + > + tb = ctrl->link->get_test_bits_depth(ctrl->link, > + ctrl->panel->pinfo.bpp); > + cc = ctrl->link->get_colorimetry_config(ctrl->link); > + ctrl->catalog->config_misc(ctrl->catalog, cc, tb); > + ctrl->panel->timing_cfg(ctrl->panel); > +} > + > +static void dp_ctrl_get_extra_req_bytes(u64 result_valid, > + int valid_bdary_link, > + u64 value1, u64 value2, > + bool *negative, u64 *result, > + u64 compare) > +{ > + *negative = false; > + if (result_valid >= compare) { > + if (valid_bdary_link > + >= compare) > + *result = value1 + value2; > + else { > + if (value1 < value2) > + *negative = true; > + *result = (value1 >= value2) ? > + (value1 - value2) : (value2 - value1); > + } > + } else { > + if (valid_bdary_link > + >= compare) { > + if (value1 >= value2) > + *negative = true; > + *result = (value1 >= value2) ? > + (value1 - value2) : (value2 - value1); > + } else { > + *result = value1 + value2; > + *negative = true; > + } > + } > +} > + > +static u64 roundup_u64(u64 x, u64 y) > +{ > + x += (y - 1); > + return (div64_ul(x, y) * y); > +} Does roundup() not work for u64? > + > +static u64 rounddown_u64(u64 x, u64 y) > +{ > + u64 rem; > + > + div64_u64_rem(x, y, &rem); > + return (x - rem); > +} Same question <snip> > +static int dp_ctrl_wait4video_ready(struct dp_ctrl_private *ctrl) > +{ > + int ret = 0; > + > + ret = wait_for_completion_timeout(&ctrl->video_comp, HZ / 2); > + if (ret <= 0) { > + pr_err("Link Train timedout\n"); > + ret = -EINVAL; On timeout it would be helpful to return -ETIMEDOUT but if ret < 0 you should return the value that you got from wait_for_completion_timeout in case the caller wants to know about interrupted calls. > + } > + > + return ret; > +} <snip> > +static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl) > +{ > + int ret = 0; > + u8 encoding = 0x1; > + 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; > + > + 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"); > + goto end; > + } > + > + /* print success info as this is a result of user initiated action */ > + pr_info("link training #1 successful\n"); These seem like maybe they belong at pr_debug() level - they are interested while getting going but probably not in a commercial setting. > + ret = dp_ctrl_link_training_2(ctrl); > + if (ret) { > + pr_err("link training #2 failed\n"); > + goto end; > + } > + > + /* print success info as this is a result of user initiated action */ > + pr_info("link training #2 successful\n"); Same. > + > +end: > + dp_ctrl_state_ctrl(ctrl, 0); > + /* Make sure to clear the current pattern before starting a new one */ > + 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); > + if (ret) > + goto end; > + > + if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) > + goto end; > + > + if (!train) > + goto send_video; > + > + /* > + * 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. > + */ > + 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); > + 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); More accurately, the clock name isn't found in the list. > +} > + > +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; You should use the return value from ctrl->power->clk_enable instead of reclassifying it. > + } > + > + 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); > +} > + > +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; > + } Use the WARN_ON to avoid the custom error message. Throughout. > + 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; > +} <snip> > +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; > + } None of these will be NULL from the single call to this function. > + ctrl = devm_kzalloc(in->dev, sizeof(*ctrl), GFP_KERNEL); > + if (!ctrl) { You can jsut return ERR_PTR(-ENOMEM) here > + 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); This shouldn't be needed. > +} > + > +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); Not needed if you are using devm_ > +} > 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. > + * > + */ SPDX please. > +#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); > +}; > + > +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; > +}; > + > +struct dp_ctrl *dp_ctrl_get(struct dp_ctrl_in *in); > +void dp_ctrl_put(struct dp_ctrl *dp_ctrl); > + > +#endif /* _DP_CTRL_H_ */ This seems like a good ending spot for now. I'll do the rest later. Jordan
On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote: > Add the needed displayPort files to enable DP driver > on msm target. > > "dp_display" module is the main module that calls into > other sub-modules. "dp_drm" file represents the interface > between DRM framework and DP driver. > > Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org> > --- > drivers/gpu/drm/msm/Kconfig | 9 + > drivers/gpu/drm/msm/Makefile | 15 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 ++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 + > drivers/gpu/drm/msm/dp/dp_aux.c | 570 ++++++++++ > drivers/gpu/drm/msm/dp/dp_aux.h | 44 + > drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 ++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 144 +++ > drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475 +++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 + > drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++++ > drivers/gpu/drm/msm/dp/dp_debug.h | 81 ++ > drivers/gpu/drm/msm/dp/dp_display.c | 977 +++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_display.h | 55 + > drivers/gpu/drm/msm/dp/dp_drm.c | 542 ++++++++++ > drivers/gpu/drm/msm/dp/dp_drm.h | 52 + > drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++++ > drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++ > drivers/gpu/drm/msm/dp/dp_link.c | 1549 +++++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_link.h | 184 ++++ > drivers/gpu/drm/msm/dp/dp_panel.c | 624 +++++++++++ > drivers/gpu/drm/msm/dp/dp_panel.h | 121 +++ > drivers/gpu/drm/msm/dp/dp_parser.c | 679 ++++++++++++ > drivers/gpu/drm/msm/dp/dp_parser.h | 205 ++++ > drivers/gpu/drm/msm/dp/dp_power.c | 599 +++++++++++ > drivers/gpu/drm/msm/dp/dp_power.h | 57 + > drivers/gpu/drm/msm/dp/dp_reg.h | 357 ++++++ > drivers/gpu/drm/msm/msm_drv.c | 2 + > drivers/gpu/drm/msm/msm_drv.h | 22 + > include/drm/drm_dp_helper.h | 19 + > 30 files changed, 10887 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h > create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h <snip to dp_debug.c> > 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. > + * > + */ SPDX please > +#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; debug will not be NULL here. > + > + 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; > + All of this can be replaced by kstrtoint_from_user(). > + if (debug->usbpd && debug->usbpd->connect) > + debug->usbpd->connect(debug->usbpd, hpd); > + > +end: > + return -len; On error you should return a real error code or the number of bytes consumed (count) on success. > +} > + > +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]; Just use 32. > + size_t len = 0; > + int hdisplay = 0, vdisplay = 0, vrefresh = 0; > + > + if (!debug) > + return -ENODEV; > Debug can't be NULL here. > + if (*ppos) > + goto end; > + > + /* Leave room for termination char */ > + len = min_t(size_t, count, SZ_32 - 1); instead of SZ_23 - 1, use sizeof(buf) - 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; This should be 'count' > +} > + > +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; > Debug can't be NULL > + if (*ppos) > + return 0; > + > + if (!debug->usbpd) > + return -ENODEV; > + > + len += snprintf(buf, SZ_8, "%d\n", debug->usbpd->hpd_high); You should always use scnprintf instead of snprintf in these situations. And use sizeof(buf) instead of a constant. And you can set len = snprintf(...). you don't need to do a += and depend on the initialization. > + > + 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; > + } Debug cannot be NULL here. > + > + connector = *debug->connector; Why are you dereferencing debug? > + > + if (!connector) { > + pr_err("connector is NULL\n"); > + rc = -EINVAL; > + goto error; Just return -EINVAL; > + } I'm not sure if this can be an error or not, but perhaps consider skipping the print message. > + > + if (*ppos) > + goto error; Just return 0; > + > + buf = kzalloc(SZ_4K, GFP_KERNEL); > + if (!buf) { > + rc = -ENOMEM; > + goto error; Just return -ENOMEM; > + } > + > + list_for_each_entry(mode, &connector->modes, head) { > + len += snprintf(buf + len, SZ_4K - len, Definitely use scnprintf() - this is a disaster waiting to happen/ > + "%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); Just return -EFAULT here > + rc = -EFAULT; > + goto error; > + } > + > + *ppos += len; > + kfree(buf); > + > + return len; > +error: > + return rc; This label isn't needed with the above changes. > +} > + > +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; Debug cannot be 0. > + > + 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); Use scnprintf() > + if (dp_debug_check_buffer_overflow(rc, &max_size, &len)) > + goto error; This function is very odd - if you use scnprintf() you can safely just do len = scnprintf(buf + len, SZ_4K - len, "\ttname = %s\n", DEBUG_NAME); len += scnprintf(buf + len, SZ_4K - len, ....) But this whole thing seems like a really good fit for seq_file and then you don't have to worry about anything else. > + 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, > +}; > + > +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); You don't want to be putting the debugbfs in the root dir - you'll want it nicely nestled in with the rest of the driver. See the GPU or DPU debugfs files for clues on how to do that. > + if (IS_ERR_OR_NULL(dir)) { debugfs_create_dir only returns NULL on error. Getting the PTR_ERR will just return 0 in that case. > + rc = PTR_ERR(dir); > + pr_err("[%s] debugfs create dir failed, rc = %d\n", > + DEBUG_NAME, rc); You don't need DEBUG_NAME in the string - you already have it in the pr_fmt. > + goto error; > + } > + > + file = debugfs_create_file("dp_debug", 0444, dir, > + debug, &dp_debug_fops); > + if (IS_ERR_OR_NULL(file)) { As above. > + 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)) { As above. > + 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)) { As above. > + 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)) { As above. > + 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; Note that rc will have to be assigned by some value by you on error because you won't be getting it from the api functions. > +} > + > +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; > + } > I don't think any of these checks are needed - they should all be non-NULL from the caller. > + debug = devm_kzalloc(dev, sizeof(*debug), GFP_KERNEL); > + if (!debug) { just return ERR_PTR(-ENOMEM); > + 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); just return ERR_PTR(rc); > + goto error; > + } > + > + return dp_debug; > +error: > + return ERR_PTR(rc); THis isn't needed > +} > + > +static int dp_debug_deinit(struct dp_debug *dp_debug) This function doesn't need to return anything - the "error" is uninteresting. > +{ > + 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); In fact, dp_debug_deinit doesn't need to exist - you can keep it all inline and bonus - you don't have to do an unneeded container_of for no reason. > + devm_kfree(debug->dev, debug); If you are using devm_ you don't need to free here. > +} > 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. > + * > + */ SPDX please. > +#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 DisplayPlot? DisplayPort perhaps? > + * > + * @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) */ > + > +#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. > + * > + */ SPDX of course. > +#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"}, > + {} We like to have a comma after the closing {} for consistency. > +}; > + > +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 will never be NULL here > + /* 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; > + } > These will never be NULL coming out of a component bind. + > + 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; > + } These are also very unlikely to be NULL. > + > + 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; You don't need this goto here, you can just drop out. > + } > + > +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; > + } Both these checks are not likely to be NULL. > + > + (void)dp->power->power_client_deinit(dp->power); > + (void)dp->aux->drm_aux_deregister(dp->aux); If you don't care about the value of either of these then consider changing them so they don't return a value. > + 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; > + } It looks unlkely this will be NULL, so you can safely remove this or use WARN_ON. > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + if (!dp) { > + pr_err("invalid params\n"); > + return; > + } The result of a container_of() will NEVER be NULL at least not without a lot of trouble. > + 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) > +{ > + 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; Returning -ETIMEDOUT would be more informative. > + } > + > + 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; You can just return -EINVAL here. > + 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; Not needed. > +} > + > +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; > + } > I'm not sure if this ever be true (I suspect it can't) but if so you can use the WARN_ON method. > + dp = dev_get_drvdata(dev); > + if (!dp) { > + pr_err("no driver data found\n"); > + rc = -ENODEV; > + goto end; > + } I'm reasonably sure that if dev is valid, this will be valid. > + 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; > + } As above > + /* 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; > + } As above. > + 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; Just return rc; > + goto end; > + } > + > + if (!dp->usbpd->hpd_high) { > + dp_display_process_hpd_low(dp); > + goto end; Just return rc; > + } > + > + if (dp->usbpd->alt_mode_cfg_done) > + dp_display_process_hpd_high(dp); > +end: > + return rc; return 0; > +} > + > +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_get() prints all the interesting error messsages - you don't need to add on. In fact, I think this is probably true for the rest of the dp_ calls above - in general, try to keep the number of log messages per error down to 1. > + 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; > + } I'm betting this can't be NULL; > + 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; > + } I am guessing this can't be NULL. > + 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; > + } > As above. > + 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; > + } > + as above. > + 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; > + } > As above. > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + > + if (!dp->power_on || !dp->core_initialized) > + goto error; goto error is deceptive here. You can just return 0. In fact, if you get rid of the uneeded NULL check you don't have to return anything from this function. > + > + 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; > + } Likely not needed. > + 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); > + } Likely not needed. > + 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; > + } Same as above. > + 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; > + } Likely not needed. > + 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; > + } Likely not needed > + 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; > + } Likely not needed. > + 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; > + } Both of these will always be true in a probe function that you reach from a compatible match. > + 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"); I'm guessing that this log message will always be superfluous. > + devm_kfree(&pdev->dev, dp); > + return -EPROBE_DEFER; Always return -EPROBE_DEFER regardless of the error? It seems that at a certain point many of these errors you point out would be fatal to the device regardless of the order of initialization. > + } > + > + 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; > +} This isn't used in this patch so I don't see how it is used but based on everything it seems like you could just as easily get away with struct msm_dp *dp_display_get_display(void) { return g_dp_display; } Even if you are planning for the future, you can make the changes to support multiple displays then and save yourself the effort now. > +int dp_display_get_num_of_displays(void) > +{ > + return 1; > +} > + > +static int dp_display_remove(struct platform_device *pdev) > +{ > + struct dp_display_private *dp; > + > + if (!pdev) > + return -EINVAL; pdev will never be NULL here. > + > + dp = platform_get_drvdata(pdev); > + > + dp_display_deinit_sub_modules(dp); > + > + platform_set_drvdata(pdev, NULL); > + devm_kfree(&pdev->dev, dp); You don't need to devm_kfree() in a remove function here. > + > + 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; You don't need this return ret - you can just drop out to the bottom. > + } > + > + 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) > +{ > + struct msm_drm_private *priv; > + int ret; > + > + if (WARN_ON(!encoder) || WARN_ON(!dp_display) || WARN_ON(!dev)) > + return -EINVAL; Thats what I like to see - but you could also do if (WARN_ON(!encoder || !dp_display || !dev)) > > + priv = dev->dev_private; > + dp_display->drm_dev = dev; > + > + ret = dp_drm_bridge_init(dp_display, encoder); > + if (ret) { > + dev_err(dev->dev, "failed to create dp bridge: %d\n", ret); Just double check to see how redundant this error message is. > + dp_display->bridge = NULL; > + return ret; > + } > + dp_display->encoder = encoder; > + > + dp_display->connector = dp_drm_connector_init(dp_display); > + if (IS_ERR(dp_display->connector)) { > + ret = PTR_ERR(dp_display->connector); > + dev_err(dev->dev, > + "failed to create dp connector: %d\n", ret); Same. > + dp_display->connector = NULL; > + goto conn_fail; > + } > + > + priv->bridges[priv->num_bridges++] = dp_display->bridge; > + priv->connectors[priv->num_connectors++] = dp_display->connector; > + return 0; > + > +conn_fail: > + if (dp_display->bridge) { > + dp_drm_bridge_deinit(dp_display); > + dp_display->bridge = NULL; > + } > + return ret; > +} > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h > new file mode 100644 > index 0000000..ca5eae5 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_display.h > @@ -0,0 +1,55 @@ > +/* > + * 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. > + * > + */ SPDX please. > +#ifndef _DP_DISPLAY_H_ > +#define _DP_DISPLAY_H_ > + > +#include <drm/drmP.h> > + > +#include "dp_panel.h" > + > +struct msm_dp { > + struct drm_device *drm_dev; > + struct dp_bridge *dp_bridge; > + struct drm_connector *connector; > + struct drm_bridge *bridge; > + > + /* the encoder we are hooked to (outside of dsi block) */ > + struct drm_encoder *encoder; > + bool is_connected; > + u32 max_pclk_khz; > + > + int (*enable)(struct msm_dp *dp_display); > + int (*post_enable)(struct msm_dp *dp_display); > + > + int (*pre_disable)(struct msm_dp *dp_display); > + int (*disable)(struct msm_dp *dp_display); > + > + int (*set_mode)(struct msm_dp *dp_display, > + struct dp_display_mode *mode); > + int (*validate_mode)(struct msm_dp *dp_display, u32 mode_pclk_khz); > + int (*get_modes)(struct msm_dp *dp_display, > + struct dp_display_mode *dp_mode); > + int (*prepare)(struct msm_dp *dp_display); > + int (*unprepare)(struct msm_dp *dp_display); > + int (*request_irq)(struct msm_dp *dp_display); > + struct dp_debug *(*get_debug)(struct msm_dp *dp_display); > + void (*send_hpd_event)(struct msm_dp *dp_display); > + bool (*is_video_test)(struct msm_dp *dp_display); > + int (*get_test_bpp)(struct msm_dp *dp_display); > +}; > + > +int dp_display_get_num_of_displays(void); > +int dp_display_get_displays(void **displays, int count); > +#endif /* _DP_DISPLAY_H_ */ > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > new file mode 100644 > index 0000000..64d6449 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -0,0 +1,542 @@ > +/* > + * 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. > + * > + */ SPDX > +#define pr_fmt(fmt) "[drm-dp]: %s: " fmt, __func__ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_atomic.h> > +#include <drm/drm_crtc.h> > + > +#include "msm_drv.h" > +#include "msm_kms.h" > +//#include "dpu_connector.h" If this isn't needed just get rid of it > +#include "dp_drm.h" > +#include "dp_debug.h" > + > +struct dp_connector { > + struct drm_connector base; > + struct msm_dp *dp_display; > +}; > +#define to_dp_connector(x) container_of(x, struct dp_connector, base) > +#define to_dp_bridge(x) container_of((x), struct dp_bridge, base) > + > +static void convert_to_dp_mode(const struct drm_display_mode *drm_mode, > + struct dp_display_mode *dp_mode, struct msm_dp *dp) > +{ > + const u32 num_components = 3; I prefer to use just the constants inline, especially since you are only using once. > + > + memset(dp_mode, 0, sizeof(*dp_mode)); > + > + dp_mode->timing.h_active = drm_mode->hdisplay; > + dp_mode->timing.h_back_porch = drm_mode->htotal - drm_mode->hsync_end; > + dp_mode->timing.h_sync_width = drm_mode->htotal - > + (drm_mode->hsync_start + dp_mode->timing.h_back_porch); > + dp_mode->timing.h_front_porch = drm_mode->hsync_start - > + drm_mode->hdisplay; > + dp_mode->timing.h_skew = drm_mode->hskew; > + > + dp_mode->timing.v_active = drm_mode->vdisplay; > + dp_mode->timing.v_back_porch = drm_mode->vtotal - drm_mode->vsync_end; > + dp_mode->timing.v_sync_width = drm_mode->vtotal - > + (drm_mode->vsync_start + dp_mode->timing.v_back_porch); > + > + dp_mode->timing.v_front_porch = drm_mode->vsync_start - > + drm_mode->vdisplay; > + > + if (dp->is_video_test(dp)) > + dp_mode->timing.bpp = dp->get_test_bpp(dp); > + else > + dp_mode->timing.bpp = dp->connector->display_info.bpc * > + num_components; > + > + if (!dp_mode->timing.bpp) > + dp_mode->timing.bpp = 24; > + > + dp_mode->timing.refresh_rate = drm_mode->vrefresh; > + > + dp_mode->timing.pixel_clk_khz = drm_mode->clock; > + > + dp_mode->timing.v_active_low = > + !!(drm_mode->flags & DRM_MODE_FLAG_NVSYNC); > + > + dp_mode->timing.h_active_low = > + !!(drm_mode->flags & DRM_MODE_FLAG_NHSYNC); > +} > + > +static void convert_to_drm_mode(const struct dp_display_mode *dp_mode, > + struct drm_display_mode *drm_mode) > +{ > + u32 flags = 0; > + > + memset(drm_mode, 0, sizeof(*drm_mode)); > + > + drm_mode->hdisplay = dp_mode->timing.h_active; > + drm_mode->hsync_start = drm_mode->hdisplay + > + dp_mode->timing.h_front_porch; > + drm_mode->hsync_end = drm_mode->hsync_start + > + dp_mode->timing.h_sync_width; > + drm_mode->htotal = drm_mode->hsync_end + dp_mode->timing.h_back_porch; > + drm_mode->hskew = dp_mode->timing.h_skew; > + > + drm_mode->vdisplay = dp_mode->timing.v_active; > + drm_mode->vsync_start = drm_mode->vdisplay + > + dp_mode->timing.v_front_porch; > + drm_mode->vsync_end = drm_mode->vsync_start + > + dp_mode->timing.v_sync_width; > + drm_mode->vtotal = drm_mode->vsync_end + dp_mode->timing.v_back_porch; > + > + drm_mode->vrefresh = dp_mode->timing.refresh_rate; > + drm_mode->clock = dp_mode->timing.pixel_clk_khz; > + > + if (dp_mode->timing.h_active_low) > + flags |= DRM_MODE_FLAG_NHSYNC; > + else > + flags |= DRM_MODE_FLAG_PHSYNC; > + > + if (dp_mode->timing.v_active_low) > + flags |= DRM_MODE_FLAG_NVSYNC; > + else > + flags |= DRM_MODE_FLAG_PVSYNC; > + > + drm_mode->flags = flags; > + > + drm_mode->type = 0x48; > + drm_mode_set_name(drm_mode); > +} > + > +static void dp_bridge_pre_enable(struct drm_bridge *drm_bridge) > +{ > + int rc = 0; > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge) { > + pr_err("Invalid params\n"); > + return; > + } bridge will always be valid. > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; You can move these two up to the declaration and save yourself two more lines per function here and throughout. > + > + /* By this point mode should have been validated through mode_fixup */ > + rc = dp->set_mode(dp, &bridge->dp_mode); > + if (rc) { > + pr_err("[%d] failed to perform a mode set, rc=%d\n", > + bridge->id, rc); > + return; > + } > + > + rc = dp->prepare(dp); > + if (rc) { > + pr_err("[%d] DP display prepare failed, rc=%d\n", > + bridge->id, rc); > + return; > + } > + > + rc = dp->enable(dp); > + if (rc) { > + pr_err("[%d] DP display enable failed, rc=%d\n", > + bridge->id, rc); > + dp->unprepare(dp); > + } > +} > + > +static void dp_bridge_enable(struct drm_bridge *drm_bridge) > +{ > + int rc = 0; > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge) { > + pr_err("Invalid params\n"); > + return; > + } bridge will always be valid. > + > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; > + > + rc = dp->post_enable(dp); > + if (rc) > + pr_err("[%d] DP display post enable failed, rc=%d\n", > + bridge->id, rc); > +} > + > +static void dp_bridge_disable(struct drm_bridge *drm_bridge) > +{ > + int rc = 0; > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge) { > + pr_err("Invalid params\n"); > + return; > + } bridge will always be valid. > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; > + > + rc = dp->pre_disable(dp); > + if (rc) { > + pr_err("[%d] DP display pre disable failed, rc=%d\n", > + bridge->id, rc); > + } > +} > + > +static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) > +{ > + int rc = 0; > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge) { > + pr_err("Invalid params\n"); > + return; > + } bridge will always be valid. > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; > + > + rc = dp->disable(dp); > + if (rc) { > + pr_err("[%d] DP display disable failed, rc=%d\n", > + bridge->id, rc); > + return; > + } > + > + rc = dp->unprepare(dp); > + if (rc) { > + pr_err("[%d] DP display unprepare failed, rc=%d\n", > + bridge->id, rc); > + return; > + } > +} > + > +static void dp_bridge_mode_set(struct drm_bridge *drm_bridge, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge || !mode || !adjusted_mode) { > + pr_err("Invalid params\n"); > + return; > + } bridge will always be valid. Not sure about mode or adjusted_mode, maybe you should use WARN_ON for those. > + > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; > + > + memset(&bridge->dp_mode, 0x0, sizeof(struct dp_display_mode)); > + convert_to_dp_mode(adjusted_mode, &bridge->dp_mode, dp); > +} > + > +static bool dp_bridge_mode_fixup(struct drm_bridge *drm_bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + bool ret = true; > + struct dp_display_mode dp_mode; > + struct dp_bridge *bridge; > + struct msm_dp *dp; > + > + if (!drm_bridge || !mode || !adjusted_mode) { > + pr_err("Invalid params\n"); > + ret = false; > + goto end; > + } > drm_bridge will always be valid, not positive about the other two. > + bridge = to_dp_bridge(drm_bridge); > + dp = bridge->display; > + > + convert_to_dp_mode(mode, &dp_mode, dp); > + convert_to_drm_mode(&dp_mode, adjusted_mode); > +end: > + return ret; > +} > + > +static const struct drm_bridge_funcs dp_bridge_ops = { > + .mode_fixup = dp_bridge_mode_fixup, > + .pre_enable = dp_bridge_pre_enable, > + .enable = dp_bridge_enable, > + .disable = dp_bridge_disable, > + .post_disable = dp_bridge_post_disable, > + .mode_set = dp_bridge_mode_set, > +}; > + > +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; > +} > + > +/** > + * 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; > + > + pr_err("is_connected = %s\n", > + (dp->is_connected) ? "true" : "false"); Oops, this looks like a debug message. > + > + status = (dp->is_connected) ? connector_status_connected : > + connector_status_disconnected; > + > + if (dp->is_connected && dp->bridge->encoder > + && (current_status != status) > + && kms->funcs->set_encoder_mode) { > + kms->funcs->set_encoder_mode(kms, > + dp->bridge->encoder, false); > + pr_err("call set_encoder_mode\n"); This too. > + } > + current_status = status; > + 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); > +} > + > +void dp_connector_send_hpd_event(void *display) > +{ > + struct msm_dp *dp; > + > + if (!display) { > + pr_err("invalid input\n"); Even if this is invalid, you don't need a error message. A WARN_ON if you must. > + 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; > Connector can never be NULL here. + > + dp_conn = to_dp_connector(connector); > + dp = dp_conn->dp_display; You can move these to the declaration. > + dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); > + if (!dp_mode) > + return 0; Return 0 on -ENOMEM? > + > + /* pluggable case assumes EDID is read when HPD */ > + if (dp->is_connected) { > + rc = dp->get_modes(dp, dp_mode); > + if (!rc) > + pr_err("failed to get DP sink modes, rc=%d\n", rc); > + Do you wnat to keep going here even if the get_modes 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; > + drm_mode_probed_add(connector, m); > + } > + } else { > + pr_err("No sink connected\n"); > + } > + kfree(dp_mode); How big is dp_mode? You can't put it on the stack? > + > + 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; Just return -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; > + } > I'm sure connector will always be valid, not sure about mode. > + 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) > + 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; > + > + return dp_disp->validate_mode(dp_disp, mode->clock); > +} > + > +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); > + 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) You are missing a kfree here. > + 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. > + */ > + connector->polled = DRM_CONNECTOR_POLL_HPD; > + > + /* Display driver doesn't support interlace now. */ > + connector->interlace_allowed = false; > + connector->doublescan_allowed = false; > + > + 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. > + * > + */ SPDX please. > +#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; > +}; > + > +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"; > + } > +} > + > +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"; > + } > +} > + > +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) > +{ > + 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; > + > + dp_usbpd_init_port(&cap->port, port); > +} > + > +void dp_usbpd_get_status(struct dp_extcon_private *pd) > +{ > + 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); > +} > + > +u32 dp_usbpd_gen_config_pkt(struct dp_extcon_private *pd) > +{ > + 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; > + } > + > + extcon = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd); > + > + dp_usbpd->hpd_high = hpd; > + dp_usbpd->forced_disconnect = !hpd; > + > + if (hpd) > + extcon->dp_cb->configure(extcon->dev); > + else > + extcon->dp_cb->disconnect(extcon->dev); > + > +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 = ((property.intval) ? 2 : 4); > + > + return 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) { The result of container_of can never be NULL. > + pr_err("Invalid extcon device handler\n"); > + return; > + } > + > + dp_state = extcon_get_state(extcon_priv->extcon, > + EXTCON_DISP_DP); > + > + 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) ? > + ORIENTATION_CC2 : ORIENTATION_CC1); > + > + lanes = dp_extcon_get_lanes(extcon_priv); > + if (!lanes) > + return; > + extcon_priv->dp_usbpd.multi_func = > + ((lanes == 2) ? false : true); > + > + if (extcon_priv->dp_cb && extcon_priv->dp_cb->configure) > + dp_extcon_connect(&extcon_priv->dp_usbpd, true); > + } else { > + if (extcon_priv->dp_cb && extcon_priv->dp_cb->disconnect) > + dp_extcon_connect(&extcon_priv->dp_usbpd, false); > + } > +} > + > +static int dp_extcon_create_workqueue(struct dp_extcon_private *extcon_priv) > +{ > + extcon_priv->extcon_wq = create_singlethread_workqueue("drm_dp_extcon"); > + 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; > +} > + > +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++++"); Debug message snuck through. > + 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"); Prefer that you don't abbreviate err here. > + ret = -EINVAL; You set ret, but you don't do anything with it - you continue on. > + } > + > + ret = dp_extcon_create_workqueue(extcon_priv); > + if (ret) { > + pr_err("Failed to create workqueue\n"); > + dp_extcon_unregister(dp_usbpd); > + } > + > + pr_err("End----"); Also debug message. > + 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"); Doesn't seem that the error message will do you much good here. > + return; > + } > + > + pr_err("Start++++"); Debug log message > + 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----"); Debug log message > + 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"); Log message isn't useful, just return ERR_PTR(-EINVAL); > + rc = -EINVAL; > + goto error; > + } > + > + pr_err("%s: Start", __func__); Debug message. > + dp_extcon = devm_kzalloc(dev, sizeof(*dp_extcon), GFP_KERNEL); > + if (!dp_extcon) { return ERR_PTR(-ENOMEM); > + 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); Just return ERR_PTR(-EINVAL); > + 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__); Debug message > + return dp_usbpd; > +error: > + 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); You don't need to do this for managed memory. This whole function doesn't need to exist. > + > + 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. > + * > + */ SPDX please > +#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. > + * > + */ SPDX please. > +#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, > +}; > + > +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) You don't need the label for any of these. > + 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; This isn't my area of expertise but I can't shake the feeling that this code could be consolidated - like why are there individual members for req->test_audio_period_ch_N and not an array. Like I said, not my area, just feel like there is code that could be saved. > +} > + > +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"); just return -EINVAL; > + 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; just return -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; return 0; > +} > + > +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; Same as above - you should almost never have a just a return after a label. Same is true for much of this file. > + 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; > +} <snip> > +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; > + } It doesn't look to me like dp_link can be NULL here. > + 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); > +} > + > +static int dp_link_psm_config(struct dp_link *dp_link, > + struct drm_dp_link *link_info, bool enable) > +{ > + struct dp_link_private *link = NULL; > + int ret = 0; > + > + if (!dp_link) { > + pr_err("invalid params\n"); > + return -EINVAL; > + } > Same. It seems that all of these function pointers have a verified valid dp_link before calling. > + 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")); > + else > + dp_link->psm_enabled = enable; > + > + return ret; > +} <snip> > +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); > + > + return 0; This function looks like it doesn't do anything of value. > +} > + > +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); > + > + 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; > + } Dev and aux both appear to be non NULL from the claler. > + link = devm_kzalloc(dev, sizeof(*link), GFP_KERNEL); > + if (!link) { > + rc = -EINVAL; This should be -ENOMEM and you should just return ERR_PTR(-ENOMEM); > + 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); Label should not be needed. > +} > + > +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); You don't need to delete devm_ managed memory. > +} > 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. > + * > + */ SPDX please. > +#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, The last item in a enum will be the value of the previous item plus 1 - which is really the only good reason to use a enum IMO. You don't need to assign it which is good because if you ever added anything to this list it would take two lines of change and not just one. > +}; > + > +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, > +}; Same. > + > +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; > +}; > + > +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 a bit beefy for a inline for my tastes. > + > +/** > + * 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; > +} > + > +/** > + * 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. > + * > + */ SPDX please. > +#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 > +#define DP_MAX_DS_PORT_COUNT 1 > + > +enum { > + DP_LINK_RATE_MULTIPLIER = 27000000, > +}; An enum of 1 is never useful. > + > +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, > +}; > + > +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; > + } > It appears that dp_panel will always be valid at every call point. > + 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); Just return -ENVAL here and everywhere else end: is used. > + 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; > + > + dfp_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] & > + DP_DOWN_STREAM_PORT_COUNT; > + > + 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; > + } > + } > + > + 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); > + > +end: > + return rc; > +} > + > +static int dp_panel_set_default_link_params(struct dp_panel *dp_panel) > +{ > + 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; > + } dp_panel will always be valid. > + link_info = &dp_panel->link_info; > + link_info->rate = default_bw_code; > + link_info->num_lanes = default_num_lanes; Instead of usint const variables just set the variables directly. > + pr_debug("link_rate=%d num_lanes=%d\n", > + link_info->rate, link_info->num_lanes); > + return 0; It doesn't look like a return value is needed here. And maybe this function isn't needed at all? This could all happen inline where this function is called. > +} > + > +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; > + } dp_panel will always be valid. > + 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"); You don't need an error here because the error prints an error on your behalf and you certainly don't want to print 10 errors without more details about what the failure actually was. > + retry_cnt++; > + panel->aux->reconfig(panel->aux); > + panel->aux_cfg_update_done = true; > + } else { > + return 0; > + } > + } while (retry_cnt < max_retry); My edid parsing days were over back when DVI was new and interesting but as just a kernel developer this seems odd. I'll leave it to the experts. But at least I would use a valid for() loop: for(retry_cnt = 0; retry_cnt < 10; retry_cnt++) { .. } > + 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; > + } Looks like both of these are valid from the caller. > + 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); > + } > + > + rc = dp_panel_read_edid(dp_panel, connector); > + 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); > + panel->aux_cfg_update_done = false; > + } > + > + 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; > + } > + > + 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; > + } > dp_panel and mode_edid_bpp are valid from the caller, not sure about mode_pcklk_hz. Might want to use a WARN_ON for that one. > + 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; > + struct dp_link_test_video *test_info = NULL; > + > + if (!panel) { > + pr_err("invalid params\n"); > + return; > + } > Panel will always be valid. > + 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__); > + if (edid) { > + drm_connector_update_edid_property(connector, > + edid); > + rc = drm_add_edid_modes(connector, edid); > + pr_debug("%s -", __func__); > + return rc; > + } > + > + drm_connector_update_edid_property(connector, NULL); > + pr_debug("%s null edid -", __func__); > + return rc; > +} > + > +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 should always be valid. > + 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; What is the valid return of this function? Is it supposed to be boolean? > + } else if (dp_panel->edid) { > + return dp_panel_update_modes(connector, dp_panel->edid); Indentation is suspect here. > + } > + > + /* fail-safe mode */ > + memcpy(&mode->timing, &fail_safe, > + sizeof(fail_safe)); > + return 1; > +} > + > +static u8 dp_panel_get_edid_checksum(struct edid *edid) > +{ > + struct edid *last_block = NULL; > + u8 *raw_edid = NULL; > + > + if (!edid) { > + pr_err("invalid edid input\n"); You don't need a log message here. Maybe a WARN_ON. > + return 0; > + } > + > + raw_edid = (u8 *)edid; > + raw_edid += (edid->extensions * EDID_LENGTH); > + last_block = (struct edid *)raw_edid; > + > + if (last_block) > + 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 can't be NULL here. > + 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; > + } This likely can't be NULL. > + 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; > + > + 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 likely can't be NULL. > + 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; > +end: > + return rc; > +} > + > +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; > + } > dp_panel can't be NULL. > + 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); > +end: > + return rc; It doesn't seem like this function should return anything. > +} > + > +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) { > + pr_err("invalid input\n"); > + goto end; > + } dp_panel_can't be NULL. > + 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; > +} > + > +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; > + } It looks like none of these are likely to be NULL; > > + panel = devm_kzalloc(in->dev, sizeof(*panel), GFP_KERNEL); > + if (!panel) { return ERR_PTR(-ENOMEM); > + 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); You don't need to do this for devm managed memory. > +} > 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. > + * > + */ SPDX please. > +#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, > +}; > + > +#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; > +}; > + > +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]; > + 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); > + 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)); > +} > + > +/** > + * 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_ */ And I'll end here for for now. Jordan
These patches add support for Display-Port driver on SnapDragon 845 hardware. It adds DP driver and DP PLL driver files along with the needed device-tree bindings. The block diagram of DP driver is shown below: +-------------+ |DRM FRAMEWORK| +------+------+ | +----v----+ | DP DRM | +----+----+ | +----v----+ +------------+| DP +----------++------+ + +---+| DISPLAY |+---+ | | | + +-+-----+-+ | | | | | | | | | | | | | | | | | | | | | | | | v v v v v v v +------+ +------+ +---+ +----+ +----+ +---+ +-----+ | DP | | DP | |DP | | DP | | DP | |DP | | DP | |PARSER| |EXTCON| |AUX| |LINK| |CTRL| |PHY| |POWER| +--+---+ +---+--+ +---+ +----+ +--+-+ +-+-+ +-----+ | | | | +--v---+ +---v-----+ +v-----v+ |DEVICE| |EXTCON | | DP | | TREE | |INTERFACE| |CATALOG| +------+ +---------+ +---+---+ | +---v----+ |CTRL/PHY| | HW | +--------+ These patches have dependency on clock driver changes mentioned below: https://patchwork.kernel.org/patch/10632753/ https://patchwork.kernel.org/patch/10632757/ Chandan Uddaraju (3): dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon 845 drm/msm/dp: add displayPort driver support drm/msm/dp: add support for DP PLL driver .../devicetree/bindings/display/msm/dp.txt | 249 ++++ .../devicetree/bindings/display/msm/dpu.txt | 16 +- drivers/gpu/drm/msm/Kconfig | 25 + drivers/gpu/drm/msm/Makefile | 21 +- drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 + drivers/gpu/drm/msm/dp/dp_aux.c | 570 +++++++ drivers/gpu/drm/msm/dp/dp_aux.h | 44 + drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 +++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 144 ++ drivers/gpu/drm/msm/dp/dp_ctrl.c | 1476 +++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 + drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++ drivers/gpu/drm/msm/dp/dp_debug.h | 81 + drivers/gpu/drm/msm/dp/dp_display.c | 1027 +++++++++++++ drivers/gpu/drm/msm/dp/dp_display.h | 58 + drivers/gpu/drm/msm/dp/dp_drm.c | 542 +++++++ drivers/gpu/drm/msm/dp/dp_drm.h | 52 + drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++ drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++ drivers/gpu/drm/msm/dp/dp_link.c | 1549 ++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_link.h | 184 +++ drivers/gpu/drm/msm/dp/dp_panel.c | 624 ++++++++ drivers/gpu/drm/msm/dp/dp_panel.h | 121 ++ drivers/gpu/drm/msm/dp/dp_parser.c | 679 +++++++++ drivers/gpu/drm/msm/dp/dp_parser.h | 208 +++ drivers/gpu/drm/msm/dp/dp_power.c | 652 ++++++++ drivers/gpu/drm/msm/dp/dp_power.h | 59 + drivers/gpu/drm/msm/dp/dp_reg.h | 357 +++++ drivers/gpu/drm/msm/dp/pll/dp_pll.c | 153 ++ drivers/gpu/drm/msm/dp/pll/dp_pll.h | 64 + drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c | 401 +++++ drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h | 94 ++ drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c | 531 +++++++ drivers/gpu/drm/msm/msm_drv.c | 2 + drivers/gpu/drm/msm/msm_drv.h | 22 + include/drm/drm_dp_helper.h | 19 + 37 files changed, 12525 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/msm/dp.txt create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.c create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.h create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c