| 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(-) This is my last go - the end is in sight. <snip all the way to dp_parser.c> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c > new file mode 100644 > index 0000000..a366dc5 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > @@ -0,0 +1,679 @@ > +/* > + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ SPDX please > +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__ > + > +#include <linux/of_gpio.h> > + > +#include "dp_parser.h" > + > +static void dp_parser_unmap_io_resources(struct dp_parser *parser) > +{ > + struct dp_io *io = &parser->io; > + > + msm_dss_iounmap(&io->dp_ahb); > + msm_dss_iounmap(&io->dp_aux); > + msm_dss_iounmap(&io->dp_link); > + msm_dss_iounmap(&io->dp_p0); > + msm_dss_iounmap(&io->phy_io); > + msm_dss_iounmap(&io->ln_tx0_io); > + msm_dss_iounmap(&io->ln_tx0_io); > + msm_dss_iounmap(&io->dp_pll_io); > + msm_dss_iounmap(&io->dp_cc_io); > + msm_dss_iounmap(&io->usb3_dp_com); > + msm_dss_iounmap(&io->qfprom_io); > +} If you use devm_ to ioremap as suggested previously, then this all ends up going away. > +static int dp_parser_ctrl_res(struct dp_parser *parser) > +{ > + int rc = 0; > + u32 index; > + struct platform_device *pdev = parser->pdev; > + struct device_node *of_node = parser->pdev->dev.of_node; > + struct dp_io *io = &parser->io; > + > + rc = of_property_read_u32(of_node, "cell-index", &index); > + if (rc) { > + pr_err("cell-index not specified, rc=%d\n", rc); > + goto err; > + } This value doesn't appear to be used anywhere in this code, so why are you grabbing it? > + rc = msm_dss_ioremap_byname(pdev, &io->dp_ahb, "dp_ahb"); > + if (rc) { > + pr_err("unable to remap dp io resources\n"); msm_ioremap() helpfully gives an informative error message including the name of the region that failed so you don't need a redundant message here. > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->dp_aux, "dp_aux"); > + if (rc) { > + pr_err("unable to remap dp io resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->dp_link, "dp_link"); > + if (rc) { > + pr_err("unable to remap dp io resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->dp_p0, "dp_p0"); > + if (rc) { > + pr_err("unable to remap dp io resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->phy_io, "dp_phy"); > + if (rc) { > + pr_err("unable to remap dp PHY resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->ln_tx0_io, "dp_ln_tx0"); > + if (rc) { > + pr_err("unable to remap dp TX0 resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->ln_tx1_io, "dp_ln_tx1"); > + if (rc) { > + pr_err("unable to remap dp TX1 resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->dp_pll_io, "dp_pll"); > + if (rc) { > + pr_err("unable to remap DP PLL resources\n"); > + goto err; > + } > + > + rc = msm_dss_ioremap_byname(pdev, &io->usb3_dp_com, "usb3_dp_com"); > + if (rc) { > + pr_err("unable to remap USB3 DP com resources\n"); > + goto err; > + } > + > + if (msm_dss_ioremap_byname(pdev, &io->dp_cc_io, "dp_mmss_cc")) { > + pr_err("unable to remap dp MMSS_CC resources\n"); > + goto err; > + } > + > + if (msm_dss_ioremap_byname(pdev, &io->qfprom_io, "qfprom_physical")) > + pr_warn("unable to remap dp qfprom resources\n"); > + It doesn't look like you are using this in the current code so you can remove it but if you did need to read fuses use nvram_cell instead. > + return 0; > +err: > + dp_parser_unmap_io_resources(parser); > + return rc; > +} This whole function is crying out for an array: struct { void __iomem **base; const char *name; } regions[] = { { &io->dp_aux.base, "dp_aux" }, ... { &io->dp_cc_io.base, "dp_mms_cc" }, { NULL, NULL } }; for(i = 0; regions[i].name; i++) { void __iomem *ptr = msm_ioremap(pdev, regions[i].name, NULL); if (IS_ERR(ptr)) return -ENODEV; *(regions[i].base) = ptr; } return 0; > + > +static const char *dp_get_phy_aux_config_property(u32 cfg_type) > +{ > + switch (cfg_type) { > + case PHY_AUX_CFG0: > + return "qcom,aux-cfg0-settings"; > + case PHY_AUX_CFG1: > + return "qcom,aux-cfg1-settings"; > + case PHY_AUX_CFG2: > + return "qcom,aux-cfg2-settings"; > + case PHY_AUX_CFG3: > + return "qcom,aux-cfg3-settings"; > + case PHY_AUX_CFG4: > + return "qcom,aux-cfg4-settings"; > + case PHY_AUX_CFG5: > + return "qcom,aux-cfg5-settings"; > + case PHY_AUX_CFG6: > + return "qcom,aux-cfg6-settings"; > + case PHY_AUX_CFG7: > + return "qcom,aux-cfg7-settings"; > + case PHY_AUX_CFG8: > + return "qcom,aux-cfg8-settings"; > + case PHY_AUX_CFG9: > + return "qcom,aux-cfg9-settings"; > + default: > + return "unknown"; > + } > +} Since CFG0 is 0 and CFG1 is and so on instead of calling a function you can just construct the string inline when you need it.... > +static void dp_parser_phy_aux_cfg_reset(struct dp_parser *parser) > +{ > + int i = 0; > + > + for (i = 0; i < PHY_AUX_CFG_MAX; i++) > + parser->aux_cfg[i] = (const struct dp_aux_cfg){ 0 }; > +} > + > +static int dp_parser_aux(struct dp_parser *parser) > +{ > + struct device_node *of_node = parser->pdev->dev.of_node; > + int len = 0, i = 0, j = 0, config_count = 0; > + const char *data; > + int const minimum_config_count = 1; > + > + for (i = 0; i < PHY_AUX_CFG_MAX; i++) { > + const char *property = dp_get_phy_aux_config_property(i); Here, instead of this, do char property[24]; snprintf(property, sizeof(property), "qcom,aux-cfg%d-settings", i); > + data = of_get_property(of_node, property, &len); > + if (!data) { > + pr_err("Unable to read %s\n", property); > + goto error; > + } > + > + config_count = len - 1; > + if ((config_count < minimum_config_count) || > + (config_count > DP_AUX_CFG_MAX_VALUE_CNT)) { > + pr_err("Invalid config count (%d) configs for %s\n", > + config_count, property); > + goto error; > + } > + > + parser->aux_cfg[i].offset = data[0]; > + parser->aux_cfg[i].cfg_cnt = config_count; > + pr_debug("%s offset=0x%x, cfg_cnt=%d\n", > + property, > + parser->aux_cfg[i].offset, > + parser->aux_cfg[i].cfg_cnt); > + for (j = 1; j < len; j++) { > + parser->aux_cfg[i].lut[j - 1] = data[j]; > + pr_debug("%s lut[%d]=0x%x\n", > + property, > + i, > + parser->aux_cfg[i].lut[j - 1]); > + } qcom,aux-cfgN-settings appears to be an array. Seems like it would be easier just to use of_property_read_u32_array()? Seems like it would save yourself a bit of work. > + } > + return 0; > + > +error: > + dp_parser_phy_aux_cfg_reset(parser); > + return -EINVAL; > +} > + > +static int dp_parser_misc(struct dp_parser *parser) > +{ > + int rc = 0; > + struct device_node *of_node = parser->pdev->dev.of_node; > + > + rc = of_property_read_u32(of_node, > + "qcom,max-pclk-frequency-khz", &parser->max_pclk_khz); > + if (rc) > + parser->max_pclk_khz = DP_MAX_PIXEL_CLK_KHZ; On error, of_property_read_u32 and friends will not touch the passed in pointer, so you can safely set the pointer to a default and then not worry about checking the value of the function - so this can just turn into: parser->max_pclk_hz = DP_MAX_PIXEL_CLK_KHZ; of_property_read_u32(of_node, "qcom,max-pclk-frequency-khz", &parser->max_pclk_khz); > + > + return 0; This function doesn't need to return anything. And technically, it doesn't need to be a function, but the place where it gets called is entirely comprised of functions so thats probably not a huge deal. > +} > + > +static int dp_parser_pinctrl(struct dp_parser *parser) > +{ > + int rc = 0; > + struct dp_pinctrl *pinctrl = &parser->pinctrl; > + > + pinctrl->pin = devm_pinctrl_get(&parser->pdev->dev); > + > + if (IS_ERR_OR_NULL(pinctrl->pin)) { It looks to me like pinctrl_get only returns ERR_PTR so you can just use IS_ERR() for your check. > + rc = PTR_ERR(pinctrl->pin); > + pr_err("failed to get pinctrl, rc=%d\n", rc); And you can just return PTR_ERR(pinctrl->pin) here > + goto error; > + } > + > + pinctrl->state_active = pinctrl_lookup_state(pinctrl->pin, > + "mdss_dp_active"); > + if (IS_ERR_OR_NULL(pinctrl->state_active)) { Same. > + rc = PTR_ERR(pinctrl->state_active); > + pr_err("failed to get pinctrl active state, rc=%d\n", rc); > + goto error; > + } > + > + pinctrl->state_suspend = pinctrl_lookup_state(pinctrl->pin, > + "mdss_dp_sleep"); > + if (IS_ERR_OR_NULL(pinctrl->state_suspend)) { Same > + rc = PTR_ERR(pinctrl->state_suspend); > + pr_err("failed to get pinctrl suspend state, rc=%d\n", rc); > + goto error; > + } > +error: > + return rc; You don't need this label if you return as soon as you get the error. > +} > + > +static int dp_parser_gpio(struct dp_parser *parser) > +{ > + int i = 0; > + struct device *dev = &parser->pdev->dev; > + struct device_node *of_node = dev->of_node; > + struct dss_module_power *mp = &parser->mp[DP_CORE_PM]; > + static const char * const dp_gpios[] = { > + "qcom,aux-en-gpio", > + "qcom,aux-sel-gpio", > + "qcom,usbplug-cc-gpio", > + }; > + > + mp->gpio_config = devm_kzalloc(dev, > + sizeof(struct dss_gpio) * ARRAY_SIZE(dp_gpios), GFP_KERNEL); > + if (!mp->gpio_config) > + return -ENOMEM; > + > + mp->num_gpio = ARRAY_SIZE(dp_gpios); > + > + for (i = 0; i < ARRAY_SIZE(dp_gpios); i++) { > + mp->gpio_config[i].gpio = of_get_named_gpio(of_node, > + dp_gpios[i], 0); > + > + if (!gpio_is_valid(mp->gpio_config[i].gpio)) { > + pr_err("%s gpio not specified\n", dp_gpios[i]); > + return -EINVAL; > + } > + > + strlcpy(mp->gpio_config[i].gpio_name, dp_gpios[i], > + sizeof(mp->gpio_config[i].gpio_name)); > + > + mp->gpio_config[i].value = 0; > + } > + > + return 0; > +} > + > +static const char *dp_parser_supply_node_name(enum dp_pm_type module) > +{ > + switch (module) { > + case DP_CORE_PM: return "qcom,core-supply-entries"; > + case DP_CTRL_PM: return "qcom,ctrl-supply-entries"; > + case DP_PHY_PM: return "qcom,phy-supply-entries"; > + default: return "???"; This isn't possible. At the very least return NULL and give you self a chance to detect the error - otherwise you'll be returning odd error messages with ???? in it that would be harder to debug. > + } > +} > + > +static int dp_parser_get_vreg(struct dp_parser *parser, > + enum dp_pm_type module) > +{ > + int i = 0, rc = 0; > + u32 tmp = 0; > + const char *pm_supply_name = NULL; > + struct device_node *supply_node = NULL; > + struct device_node *of_node = parser->pdev->dev.of_node; > + struct device_node *supply_root_node = NULL; > + struct dss_module_power *mp = &parser->mp[module]; > + > + mp->num_vreg = 0; > + pm_supply_name = dp_parser_supply_node_name(module); > + supply_root_node = of_get_child_by_name(of_node, pm_supply_name); > + if (!supply_root_node) { > + pr_err("no supply entry present: %s\n", pm_supply_name); > + goto novreg; > + } > + > + mp->num_vreg = of_get_available_child_count(supply_root_node); > + > + if (mp->num_vreg == 0) { > + pr_debug("no vreg\n"); > + goto novreg; > + } else { > + pr_debug("vreg found. count=%d\n", mp->num_vreg); > + } > + > + mp->vreg_config = devm_kzalloc(&parser->pdev->dev, > + sizeof(struct dss_vreg) * mp->num_vreg, GFP_KERNEL); > + if (!mp->vreg_config) { > + rc = -ENOMEM; > + goto error; > + } > + > + for_each_child_of_node(supply_root_node, supply_node) { > + const char *st = NULL; > + /* vreg-name */ > + rc = of_property_read_string(supply_node, > + "qcom,supply-name", &st); > + if (rc) { > + pr_err("error reading name. rc=%d\n", > + rc); > + goto error; > + } > + snprintf(mp->vreg_config[i].vreg_name, > + ARRAY_SIZE((mp->vreg_config[i].vreg_name)), "%s", st); ARRAY_SIZE works for a string, but we prefer sizeof() in most cases. If you are just copying a string, consider using strncpy() and friends. It will be faster than snprintf. > + /* vreg-min-voltage */ > + rc = of_property_read_u32(supply_node, > + "qcom,supply-min-voltage", &tmp); As mentioned above, on error they don't touch the value of the pointer so you don't need to use a temporary variable. > + if (rc) { > + pr_err("error reading min volt. rc=%d\n", > + rc); > + goto error; > + } > + mp->vreg_config[i].min_voltage = tmp; > + > + /* vreg-max-voltage */ > + rc = of_property_read_u32(supply_node, > + "qcom,supply-max-voltage", &tmp); > + if (rc) { > + pr_err("error reading max volt. rc=%d\n", > + rc); > + goto error; > + } > + mp->vreg_config[i].max_voltage = tmp; > + > + /* enable-load */ > + rc = of_property_read_u32(supply_node, > + "qcom,supply-enable-load", &tmp); > + if (rc) { > + pr_err("error reading enable load. rc=%d\n", > + rc); > + goto error; > + } > + mp->vreg_config[i].enable_load = tmp; > + > + /* disable-load */ > + rc = of_property_read_u32(supply_node, > + "qcom,supply-disable-load", &tmp); > + if (rc) { > + pr_err("error reading disable load. rc=%d\n", > + rc); > + goto error; > + } > + mp->vreg_config[i].disable_load = tmp; > + > + pr_debug("%s min=%d, max=%d, enable=%d, disable=%d\n", > + mp->vreg_config[i].vreg_name, > + mp->vreg_config[i].min_voltage, > + mp->vreg_config[i].max_voltage, > + mp->vreg_config[i].enable_load, > + mp->vreg_config[i].disable_load > + ); > + ++i; > + } > + > + return rc; > + > +error: > + if (mp->vreg_config) { You don't need to check for NULL here. > + devm_kfree(&parser->pdev->dev, mp->vreg_config); > + mp->vreg_config = NULL; > + } > +novreg: > + mp->num_vreg = 0; > + > + return rc; > +} > + > +static void dp_parser_put_vreg_data(struct device *dev, > + struct dss_module_power *mp) > +{ > + if (!mp) { > + DEV_ERR("invalid input\n"); This error message isn't useful when you are taking down the device - just quietly skip. > + return; > + } > + > + if (mp->vreg_config) { > + devm_kfree(dev, mp->vreg_config); You don't need to kfree managed memory. > + mp->vreg_config = NULL; > + } > + mp->num_vreg = 0; > +} > + > +static int dp_parser_regulator(struct dp_parser *parser) > +{ > + int i, rc = 0; > + struct platform_device *pdev = parser->pdev; > + > + /* Parse the regulator information */ > + for (i = DP_CORE_PM; i < DP_MAX_PM; i++) { > + rc = dp_parser_get_vreg(parser, i); > + if (rc) { > + pr_err("get_dt_vreg_data failed for %s. rc=%d\n", > + dp_parser_pm_name(i), rc); dp_parser_get_vreg() is very loud about errors - you don't need to pile on. > + i--; > + for (; i >= DP_CORE_PM; i--) > + dp_parser_put_vreg_data(&pdev->dev, > + &parser->mp[i]); > + break; > + } > + } > + > + return rc; > +} > + > +static bool dp_parser_check_prefix(const char *clk_prefix, const char *clk_name) > +{ > + return !!strnstr(clk_name, clk_prefix, strlen(clk_name)); This isn't saving you any lines of code - you can do the string compare directly in the calling functions. > +} > + > +static void dp_parser_put_clk_data(struct device *dev, > + struct dss_module_power *mp) > +{ > + if (!mp) { > + DEV_ERR("%s: invalid input\n", __func__); You are taking down the device so you don't care if anything is invalid. > + return; > + } > + > + if (mp->clk_config) { > + devm_kfree(dev, mp->clk_config); You don't need to free managed device memory. > + mp->clk_config = NULL; > + } > + > + mp->num_clk = 0; > +} > + > +static void dp_parser_put_gpio_data(struct device *dev, > + struct dss_module_power *mp) > +{ > + if (!mp) { > + DEV_ERR("%s: invalid input\n", __func__); As above. > + return; > + } > + > + if (mp->gpio_config) { > + devm_kfree(dev, mp->gpio_config); Also as above. > + mp->gpio_config = NULL; > + } > + > + mp->num_gpio = 0; > +} > + > +static int dp_parser_init_clk_data(struct dp_parser *parser) > +{ > + int num_clk = 0, i = 0, rc = 0; > + int core_clk_count = 0, ctrl_clk_count = 0; > + const char *core_clk = "core"; > + const char *ctrl_clk = "ctrl"; > + const char *clk_name; > + struct device *dev = &parser->pdev->dev; > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM]; > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM]; > + > + num_clk = of_property_count_strings(dev->of_node, "clock-names"); > + if (num_clk <= 0) { > + pr_err("no clocks are defined\n"); > + rc = -EINVAL; > + goto exit; > + } > + > + for (i = 0; i < num_clk; i++) { > + of_property_read_string_index(dev->of_node, > + "clock-names", i, &clk_name); > + > + if (dp_parser_check_prefix(core_clk, clk_name)) > + core_clk_count++; According to your bindings, 'core' or 'ctrl' will be the prefix for the clock, so why are you using strnstr? strncmp works just as well. !strncmp("core", clk_name, 4) > + > + if (dp_parser_check_prefix(ctrl_clk, clk_name)) if (!strncmp("ctrl", clk_name, 4) > + ctrl_clk_count++; > + } > + > + /* Initialize the CORE power module */ > + if (core_clk_count <= 0) { core_clk_count clearly can't be less than zero. > + pr_err("no core clocks are defined\n"); > + rc = -EINVAL; > + goto exit; > + } > + > + core_power->num_clk = core_clk_count; > + core_power->clk_config = devm_kzalloc(dev, > + sizeof(struct dss_clk) * core_power->num_clk, > + GFP_KERNEL); > + if (!core_power->clk_config) { > + rc = -EINVAL; the return value should be -ENOMEM here. > + goto exit; > + } > + > + /* Initialize the CTRL power module */ > + if (ctrl_clk_count <= 0) { Again, this clearly can't be less than zero. > + pr_err("no ctrl clocks are defined\n"); > + rc = -EINVAL; > + goto ctrl_clock_error; > + } > + > + ctrl_power->num_clk = ctrl_clk_count; > + ctrl_power->clk_config = devm_kzalloc(dev, > + sizeof(struct dss_clk) * ctrl_power->num_clk, > + GFP_KERNEL); > + if (!ctrl_power->clk_config) { > + ctrl_power->num_clk = 0; > + rc = -EINVAL; And -ENOMEM again. > + goto ctrl_clock_error; > + } > + > + return rc; > + > +ctrl_clock_error: > + dp_parser_put_clk_data(dev, core_power); > +exit: > + return rc; Don't use this label - just return the error code directly instead. > +} > + > +static int dp_parser_clock(struct dp_parser *parser) > +{ > + int rc = 0, i = 0; > + int num_clk = 0; > + int core_clk_index = 0, ctrl_clk_index = 0; > + int core_clk_count = 0, ctrl_clk_count = 0; > + const char *clk_name; > + const char *core_clk = "core"; > + const char *ctrl_clk = "ctrl"; > + struct device *dev = &parser->pdev->dev; > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM]; > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM]; > + > + core_power = &parser->mp[DP_CORE_PM]; > + ctrl_power = &parser->mp[DP_CTRL_PM]; > + > + rc = dp_parser_init_clk_data(parser); > + if (rc) { > + pr_err("failed to initialize power data\n"); > + rc = -EINVAL; > + goto exit; > + } > + > + core_clk_count = core_power->num_clk; > + ctrl_clk_count = ctrl_power->num_clk; > + > + num_clk = core_clk_count + ctrl_clk_count; > + According to your bindings, not every clock in your list is a core or a ctrl - are those clocks ignored? > + for (i = 0; i < num_clk; i++) { > + of_property_read_string_index(dev->of_node, "clock-names", > + i, &clk_name); This implies that a core_ or ctrl_ clock are first in the list. Your bindings do not specify an order. > + > + if (dp_parser_check_prefix(core_clk, clk_name) && Again, you should use strncmp() here > + core_clk_index < core_clk_count) { If you already went to the trouble of counting you can be pretty sure that you won't overrun your array. > + struct dss_clk *clk = > + &core_power->clk_config[core_clk_index]; > + strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name)); > + clk->type = DSS_CLK_AHB; > + core_clk_index++; > + } else if (dp_parser_check_prefix(ctrl_clk, clk_name) && > + ctrl_clk_index < ctrl_clk_count) { You aren't going to overrun your array. > + struct dss_clk *clk = > + &ctrl_power->clk_config[ctrl_clk_index]; > + strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name)); > + ctrl_clk_index++; > + > + if (!strcmp(clk_name, "ctrl_link_clk") || > + !strcmp(clk_name, "ctrl_pixel_clk")) > + clk->type = DSS_CLK_PCLK; > + else > + clk->type = DSS_CLK_AHB; > + } > + } There is a lot going on in these two functions but I feel they can be combined. The number of clocks in each array shouldn't be a mystery - you can pre-allocate or statically allocate the arrays. That will save you from the initial step of walking the list to allocate the memory. Then you can have a single walk of the list, checking the clock type and handle it accordingly. > + > + pr_debug("clock parsing successful\n"); > + > +exit: > + return rc; > +} > + > +static int dp_parser_parse(struct dp_parser *parser) > +{ > + int rc = 0; > + > + if (!parser) { > + pr_err("invalid input\n"); > + rc = -EINVAL; > + goto err; > + } > parser can never be null here. > + rc = dp_parser_ctrl_res(parser); > + if (rc) > + goto err; You can just return rc here and throughout without the label. > + > + rc = dp_parser_aux(parser); > + if (rc) > + goto err; > + > + rc = dp_parser_misc(parser); > + if (rc) > + goto err; > + > + rc = dp_parser_clock(parser); > + if (rc) > + goto err; > + > + rc = dp_parser_regulator(parser); > + if (rc) > + goto err; > + > + rc = dp_parser_gpio(parser); > + if (rc) > + goto err; > + > + rc = dp_parser_pinctrl(parser); > +err: > + return rc; > +} > + > +struct dp_parser *dp_parser_get(struct platform_device *pdev) > +{ > + struct dp_parser *parser; > + > + parser = devm_kzalloc(&pdev->dev, sizeof(*parser), GFP_KERNEL); > + if (!parser) > + return ERR_PTR(-ENOMEM); > + > + parser->parse = dp_parser_parse; > + parser->pdev = pdev; > + > + return parser; > +} > + > +void dp_parser_put(struct dp_parser *parser) > +{ > + int i = 0; > + struct dss_module_power *power = NULL; > + > + if (!parser) { > + pr_err("invalid parser module\n"); If parser is null, you don't care. Just quietly exit. > + return; > + } > + > + power = parser->mp; > + > + for (i = 0; i < DP_MAX_PM; i++) { > + dp_parser_put_clk_data(&parser->pdev->dev, &power[i]); > + dp_parser_put_vreg_data(&parser->pdev->dev, &power[i]); > + dp_parser_put_gpio_data(&parser->pdev->dev, &power[i]); > + } > + > + devm_kfree(&parser->pdev->dev, parser); You don't need to free managed device memory. > +} > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h > new file mode 100644 > index 0000000..b39ea02 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h > @@ -0,0 +1,205 @@ > +/* > + * 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_PARSER_H_ > +#define _DP_PARSER_H_ > + > +#include "dpu_io_util.h" > + > +#define DP_LABEL "MDSS DP DISPLAY" > +#define AUX_CFG_LEN 10 > +#define DP_MAX_PIXEL_CLK_KHZ 675000 > + > +enum dp_pm_type { > + DP_CORE_PM, > + DP_CTRL_PM, > + DP_PHY_PM, > + DP_MAX_PM > +}; > + > +static inline const char *dp_parser_pm_name(enum dp_pm_type module) > +{ > + switch (module) { > + case DP_CORE_PM: return "DP_CORE_PM"; > + case DP_CTRL_PM: return "DP_CTRL_PM"; > + case DP_PHY_PM: return "DP_PHY_PM"; > + default: return "???"; You go out of your way to make sure that the default case can't happen here. also this function is only used once, I don't think it needs to be a static inline in the header. > + } > +} > + > +/** > + * struct dp_display_data - display related device tree data. > + * > + * @ctrl_node: referece to controller device > + * @phy_node: reference to phy device > + * @is_active: is the controller currently active > + * @name: name of the display > + * @display_type: type of the display > + */ > +struct dp_display_data { > + struct device_node *ctrl_node; > + struct device_node *phy_node; > + bool is_active; > + const char *name; > + const char *display_type; > +}; > + > +/** > + * struct dp_ctrl_resource - controller's IO related data > + * > + * @dp_ahb: controller's ahb mapped memory address > + * @dp_aux: controller's aux mapped memory address > + * @dp_link: controller's link mapped memory address > + * @dp_p0: controller's p0 mapped memory address > + * @phy_io: phy's mapped memory address > + * @ln_tx0_io: USB-DP lane TX0's mapped memory address > + * @ln_tx1_io: USB-DP lane TX1's mapped memory address > + * @dp_cc_io: DP cc's mapped memory address > + * @qfprom_io: qfprom's mapped memory address > + * @dp_pll_io: DP PLL mapped memory address > + * @usb3_dp_com: USB3 DP PHY combo mapped memory address > + */ > +struct dp_io { > + struct dss_io_data ctrl_io; > + struct dss_io_data dp_ahb; > + struct dss_io_data dp_aux; > + struct dss_io_data dp_link; > + struct dss_io_data dp_p0; > + struct dss_io_data phy_io; > + struct dss_io_data ln_tx0_io; > + struct dss_io_data ln_tx1_io; > + struct dss_io_data dp_cc_io; > + struct dss_io_data qfprom_io; > + struct dss_io_data dp_pll_io; > + struct dss_io_data usb3_dp_com; > +}; > + > +/** > + * struct dp_pinctrl - DP's pin control > + * > + * @pin: pin-controller's instance > + * @state_active: active state pin control > + * @state_hpd_active: hpd active state pin control > + * @state_suspend: suspend state pin control > + */ > +struct dp_pinctrl { > + struct pinctrl *pin; > + struct pinctrl_state *state_active; > + struct pinctrl_state *state_hpd_active; > + struct pinctrl_state *state_suspend; > +}; > + > +#define DP_ENUM_STR(x) #x > +#define DP_AUX_CFG_MAX_VALUE_CNT 3 > +/** > + * struct dp_aux_cfg - DP's AUX configuration settings > + * > + * @cfg_cnt: count of the configurable settings for the AUX register > + * @current_index: current index of the AUX config lut > + * @offset: register offset of the AUX config register > + * @lut: look up table for the AUX config values for this register > + */ > +struct dp_aux_cfg { > + u32 cfg_cnt; > + u32 current_index; > + u32 offset; > + u32 lut[DP_AUX_CFG_MAX_VALUE_CNT]; > +}; > + > +/* PHY AUX config registers */ > +enum dp_phy_aux_config_type { > + PHY_AUX_CFG0, > + PHY_AUX_CFG1, > + PHY_AUX_CFG2, > + PHY_AUX_CFG3, > + PHY_AUX_CFG4, > + PHY_AUX_CFG5, > + PHY_AUX_CFG6, > + PHY_AUX_CFG7, > + PHY_AUX_CFG8, > + PHY_AUX_CFG9, > + PHY_AUX_CFG_MAX, > +}; > + > +static inline char *dp_phy_aux_config_type_to_string(u32 cfg_type) > +{ > + switch (cfg_type) { > + case PHY_AUX_CFG0: > + return DP_ENUM_STR(PHY_AUX_CFG0); > + case PHY_AUX_CFG1: > + return DP_ENUM_STR(PHY_AUX_CFG1); > + case PHY_AUX_CFG2: > + return DP_ENUM_STR(PHY_AUX_CFG2); > + case PHY_AUX_CFG3: > + return DP_ENUM_STR(PHY_AUX_CFG3); > + case PHY_AUX_CFG4: > + return DP_ENUM_STR(PHY_AUX_CFG4); > + case PHY_AUX_CFG5: > + return DP_ENUM_STR(PHY_AUX_CFG5); > + case PHY_AUX_CFG6: > + return DP_ENUM_STR(PHY_AUX_CFG6); > + case PHY_AUX_CFG7: > + return DP_ENUM_STR(PHY_AUX_CFG7); > + case PHY_AUX_CFG8: > + return DP_ENUM_STR(PHY_AUX_CFG8); > + case PHY_AUX_CFG9: > + return DP_ENUM_STR(PHY_AUX_CFG9); > + default: > + return "unknown"; > + } > +} > + > +/** > + * struct dp_parser - DP parser's data exposed to clients > + * > + * @pdev: platform data of the client > + * @mp: gpio, regulator and clock related data > + * @pinctrl: pin-control related data > + * @disp_data: controller's display related data > + * @parse: function to be called by client to parse device tree. > + */ > +struct dp_parser { > + struct platform_device *pdev; > + struct dss_module_power mp[DP_MAX_PM]; > + struct dp_pinctrl pinctrl; > + struct dp_io io; > + struct dp_display_data disp_data; > + > + u8 l_map[4]; > + struct dp_aux_cfg aux_cfg[AUX_CFG_LEN]; > + u32 max_pclk_khz; > + > + int (*parse)(struct dp_parser *parser); > +}; > + > +/** > + * dp_parser_get() - get the DP's device tree parser module > + * > + * @pdev: platform data of the client > + * return: pointer to dp_parser structure. > + * > + * This function provides client capability to parse the > + * device tree and populate the data structures. The data > + * related to clock, regulators, pin-control and other > + * can be parsed using this module. > + */ > +struct dp_parser *dp_parser_get(struct platform_device *pdev); > + > +/** > + * dp_parser_put() - cleans the dp_parser module > + * > + * @parser: pointer to the parser's data. > + */ > +void dp_parser_put(struct dp_parser *parser); > +#endif > diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c > new file mode 100644 > index 0000000..1d58480 > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_power.c > @@ -0,0 +1,599 @@ > +/* > + * 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/clk.h> > +#include "dp_power.h" > + > +#define DP_CLIENT_NAME_SIZE 20 > + > +struct dp_power_private { > + struct dp_parser *parser; > + struct platform_device *pdev; > + struct clk *pixel_clk_rcg; > + struct clk *pixel_parent; > + > + struct dp_power dp_power; > + > + bool core_clks_on; > + bool link_clks_on; > +}; > + > +static int dp_power_regulator_init(struct dp_power_private *power) > +{ > + int rc = 0, i = 0, j = 0; > + struct platform_device *pdev; > + struct dp_parser *parser; > + > + parser = power->parser; > + pdev = power->pdev; > + > + for (i = DP_CORE_PM; !rc && (i < DP_MAX_PM); i++) { > + rc = msm_dss_config_vreg(&pdev->dev, > + parser->mp[i].vreg_config, > + parser->mp[i].num_vreg, 1); > + if (rc) { > + pr_err("failed to init vregs for %s\n", > + dp_parser_pm_name(i)); msm_dss_config_vreg() already prints lots of error messages for you. > + for (j = i - 1; j >= DP_CORE_PM; j--) { > + msm_dss_config_vreg(&pdev->dev, > + parser->mp[j].vreg_config, > + parser->mp[j].num_vreg, 0); > + } > + > + goto error; just return rc; > + } > + } > +error: > + return rc; > +} > + > +static void dp_power_regulator_deinit(struct dp_power_private *power) > +{ > + int rc = 0, i = 0; > + struct platform_device *pdev; > + struct dp_parser *parser; > + > + parser = power->parser; > + pdev = power->pdev; > + > + for (i = DP_CORE_PM; (i < DP_MAX_PM); i++) { > + rc = msm_dss_config_vreg(&pdev->dev, > + parser->mp[i].vreg_config, > + parser->mp[i].num_vreg, 0); > + if (rc) > + pr_err("failed to deinit vregs for %s\n", > + dp_parser_pm_name(i)); Same. > + } > +} > + > +static int dp_power_regulator_ctrl(struct dp_power_private *power, bool enable) > +{ > + int rc = 0, i = 0, j = 0; > + struct dp_parser *parser; > + > + parser = power->parser; > + > + for (i = DP_CORE_PM; i < DP_MAX_PM; i++) { > + rc = msm_dss_enable_vreg( > + parser->mp[i].vreg_config, > + parser->mp[i].num_vreg, enable); > + if (rc) { > + pr_err("failed to '%s' vregs for %s\n", > + enable ? "enable" : "disable", > + dp_parser_pm_name(i)); msm_dss_enable_vreg() already gives you the errors you need. > + if (enable) { > + for (j = i-1; j >= DP_CORE_PM; j--) { > + msm_dss_enable_vreg( > + parser->mp[j].vreg_config, > + parser->mp[j].num_vreg, 0); > + } > + } > + goto error; > + } > + } > +error: > + return rc; You don't need a label with a single return in it. > +} > + > +static int dp_power_pinctrl_set(struct dp_power_private *power, bool active) > +{ > + int rc = -EFAULT; > + struct pinctrl_state *pin_state; > + struct dp_parser *parser; > + > + parser = power->parser; > + > + if (IS_ERR_OR_NULL(parser->pinctrl.pin)) Recall that pinctrl.in cannot be NULL. > + return PTR_ERR(parser->pinctrl.pin); > + > + pin_state = active ? parser->pinctrl.state_active > + : parser->pinctrl.state_suspend; > + if (!IS_ERR_OR_NULL(pin_state)) { Same > + rc = pinctrl_select_state(parser->pinctrl.pin, > + pin_state); > + if (rc) > + pr_err("can not set %s pins\n", > + active ? "dp_active" > + : "dp_sleep"); > + } else { > + pr_err("invalid '%s' pinstate\n", > + active ? "dp_active" > + : "dp_sleep"); You already let the user know during inint that the pin_state was not correct. You don't need to update them again every time you try to change it. > + } > + > + return rc; > +} > + > +static int dp_power_clk_init(struct dp_power_private *power, bool enable) > +{ > + int rc = 0; > + struct dss_module_power *core, *ctrl; > + struct device *dev; > + > + core = &power->parser->mp[DP_CORE_PM]; > + ctrl = &power->parser->mp[DP_CTRL_PM]; > + > + dev = &power->pdev->dev; > + > + if (!core || !ctrl) { > + pr_err("invalid power_data\n"); > + rc = -EINVAL; > + goto exit; > + } These are both impossible since you are deriving the pointer above. > + > + if (enable) { > + rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk); > + if (rc) { > + pr_err("failed to get %s clk. err=%d\n", > + dp_parser_pm_name(DP_CORE_PM), rc); msm_dss_get_clk already prints an error message for you. > + goto exit; > + } > + > + rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk); > + if (rc) { > + pr_err("failed to get %s clk. err=%d\n", > + dp_parser_pm_name(DP_CTRL_PM), rc); Same > + goto ctrl_get_error; > + } > + > + power->pixel_clk_rcg = devm_clk_get(dev, "pixel_clk_rcg"); > + if (IS_ERR(power->pixel_clk_rcg)) { > + pr_debug("Unable to get DP pixel clk RCG\n"); This seems like it should be an error? > + power->pixel_clk_rcg = NULL; > + } > + > + power->pixel_parent = devm_clk_get(dev, "pixel_parent"); > + if (IS_ERR(power->pixel_parent)) { > + pr_debug("Unable to get DP pixel RCG parent\n"); Same? > + power->pixel_parent = NULL; > + } > + } else { This is another one of those functions that shares very little with the other side and can be easily split into a separate function that is easier to understand. > + if (power->pixel_parent) > + devm_clk_put(dev, power->pixel_parent); > + > + if (power->pixel_clk_rcg) > + devm_clk_put(dev, power->pixel_clk_rcg); You don't need to put managed clocks. > + > + msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk); > + msm_dss_put_clk(core->clk_config, core->num_clk); > + } > + > + return rc; > + > +ctrl_get_error: > + msm_dss_put_clk(core->clk_config, core->num_clk); > +exit: > + return rc; > +} > + > +static int dp_power_clk_set_rate(struct dp_power_private *power, > + enum dp_pm_type module, bool enable) > +{ > + int rc = 0; > + struct dss_module_power *mp; > + > + if (!power) { > + pr_err("invalid power data\n"); > + rc = -EINVAL; > + goto exit; > + } power will always be valid. > + mp = &power->parser->mp[module]; > + > + if (enable) { > + rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); > + if (rc) { > + pr_err("failed to set clks rate.\n"); msm_dss_clk_set_rate() already gives you an error message; > + goto exit; > + } > + > + rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, 1); > + if (rc) { > + pr_err("failed to enable clks\n"); Same. > + goto exit; > + } > + } else { > + rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, 0); > + if (rc) { > + pr_err("failed to disable clks\n"); Same > + goto exit; Suspect indentation, but as usual, don't use a single return in a label - just return directly. > + } > + } > +exit: > + return rc; > +} > + > +static int dp_power_clk_enable(struct dp_power *dp_power, > + enum dp_pm_type pm_type, bool enable) > +{ > + int rc = 0; > + struct dss_module_power *mp; > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + rc = -EINVAL; > + goto error; > + } dp_power will always be valid. > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + mp = &power->parser->mp[pm_type]; > + > + if ((pm_type != DP_CORE_PM) && (pm_type != DP_CTRL_PM)) { > + pr_err("unsupported power module: %s\n", > + dp_parser_pm_name(pm_type)); This seems like it would be never be true outside of egregious developer error. - I suppose you could WARN_ON here but it just doesn't seem like a thing. > + return -EINVAL; > + } > + > + if (enable) { > + if ((pm_type == DP_CORE_PM) > + && (power->core_clks_on)) { > + pr_debug("core clks already enabled\n"); > + return 0; > + } > + > + if ((pm_type == DP_CTRL_PM) > + && (power->link_clks_on)) { > + pr_debug("links clks already enabled\n"); > + return 0; > + } > + > + if ((pm_type == DP_CTRL_PM) && (!power->core_clks_on)) { > + pr_debug("Need to enable core clks before link clks\n"); > + > + rc = dp_power_clk_set_rate(power, DP_CORE_PM, enable); > + if (rc) { > + pr_err("failed to enable clks: %s. err=%d\n", > + dp_parser_pm_name(DP_CORE_PM), rc); > + goto error; > + } else { > + power->core_clks_on = true; > + } > + } > + } > + > + rc = dp_power_clk_set_rate(power, pm_type, enable); > + if (rc) { > + pr_err("failed to '%s' clks for: %s. err=%d\n", > + enable ? "enable" : "disable", > + dp_parser_pm_name(pm_type), rc); > + goto error; > + } > + > + if (pm_type == DP_CORE_PM) > + power->core_clks_on = enable; > + else > + power->link_clks_on = enable; With the number of if statements in here it almost seems more logical to have a separate function for both DP_CORE_PM and DP_CTRL_PM - yeah, it would copy a bit of code but it would be a heck of a lot more readable. > + pr_debug("%s clocks for %s\n", > + enable ? "enable" : "disable", > + dp_parser_pm_name(pm_type)); > + pr_debug("link_clks:%s core_clks:%s\n", > + power->link_clks_on ? "on" : "off", > + power->core_clks_on ? "on" : "off"); > +error: > + return rc; > +} > + > +static int dp_power_request_gpios(struct dp_power_private *power) > +{ > + int rc = 0, i; > + struct device *dev; > + struct dss_module_power *mp; > + static const char * const gpio_names[] = { > + "aux_enable", "aux_sel", "usbplug_cc", > + }; > + > + if (!power) { > + pr_err("invalid power data\n"); > + return -EINVAL; > + } > + > + dev = &power->pdev->dev; > + mp = &power->parser->mp[DP_CORE_PM]; > + > + for (i = 0; i < ARRAY_SIZE(gpio_names); i++) { > + unsigned int gpio = mp->gpio_config[i].gpio; > + > + if (gpio_is_valid(gpio)) { > + rc = devm_gpio_request(dev, gpio, gpio_names[i]); > + if (rc) { > + pr_err("request %s gpio failed, rc=%d\n", > + gpio_names[i], rc); > + goto error; > + } > + } > + } > + return 0; > +error: > + for (i = 0; i < ARRAY_SIZE(gpio_names); i++) { > + unsigned int gpio = mp->gpio_config[i].gpio; > + > + if (gpio_is_valid(gpio)) > + gpio_free(gpio); Can you call gpio_free() on a device managed resource? I think you probably want devm_gpio_free. > + } > + return rc; > +} > + > +static bool dp_power_find_gpio(const char *gpio1, const char *gpio2) > +{ > + return !!strnstr(gpio1, gpio2, strlen(gpio1)); > +} > + > +static void dp_power_set_gpio(struct dp_power_private *power, bool flip) > +{ > + int i; > + struct dss_module_power *mp = &power->parser->mp[DP_CORE_PM]; > + struct dss_gpio *config = mp->gpio_config; > + > + for (i = 0; i < mp->num_gpio; i++) { > + if (dp_power_find_gpio(config->gpio_name, "aux-sel")) I think config->gpio_name is the name from the DT, basically qcom,aux-sel-gpio? Why not just strcmp on that instead of going the much harder route > + config->value = flip; > + > + if (gpio_is_valid(config->gpio)) { > + pr_debug("gpio %s, value %d\n", config->gpio_name, > + config->value); > + > + if (dp_power_find_gpio(config->gpio_name, "aux-en") || > + dp_power_find_gpio(config->gpio_name, "aux-sel")) Same and same > + gpio_direction_output(config->gpio, > + config->value); > + else > + gpio_set_value(config->gpio, config->value); > + > + } > + config++; > + } > +} > + > +static int dp_power_config_gpios(struct dp_power_private *power, bool flip, > + bool enable) > +{ > + int rc = 0, i; > + struct dss_module_power *mp; > + struct dss_gpio *config; > + > + mp = &power->parser->mp[DP_CORE_PM]; > + config = mp->gpio_config; > + > + if (enable) { > + rc = dp_power_request_gpios(power); > + if (rc) { > + pr_err("gpio request failed\n"); dp_power_request_gpios prints an error message on every failure point. > + return rc; > + } > + > + dp_power_set_gpio(power, flip); It seems like maybe dp_power_request_gpios() and dp_power_set_gpio() can be combined - both of them only get called here. > + } else { Again, this feels like it could safely be in its own function for clarity. > + for (i = 0; i < mp->num_gpio; i++) { > + gpio_set_value(config[i].gpio, 0); > + gpio_free(config[i].gpio); > + } > + } > + > + return 0; > +} > + > +static int dp_power_client_init(struct dp_power *dp_power) > +{ > + int rc = 0; > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + return -EINVAL; > + } dp_power can never be NULL here. > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + pm_runtime_enable(&power->pdev->dev); > + > + rc = dp_power_regulator_init(power); > + if (rc) { > + pr_err("failed to init regulators\n"); The sub function already prints error messages. > + goto error_power; > + } > + > + rc = dp_power_clk_init(power, true); > + if (rc) { > + pr_err("failed to init clocks\n"); Also this one. > + goto error_clk; > + } > + return 0; You can flip this if statement and save a goto: if (!rc) return 0; dp_power_regulator_deinit(power); ... > + > +error_clk: > + dp_power_regulator_deinit(power); > +error_power: > + pm_runtime_disable(&power->pdev->dev); > + return rc; > +} > + > +static void dp_power_client_deinit(struct dp_power *dp_power) > +{ > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + return; > + } dp_power is always set. > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + dp_power_clk_init(power, false); > + dp_power_regulator_deinit(power); > + pm_runtime_disable(&power->pdev->dev); > + > +} > + > +static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power) > +{ > + int rc = 0; > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + rc = -EINVAL; > + goto exit; > + } > dp_power is always set. > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + if (power->pixel_clk_rcg && power->pixel_parent) > + clk_set_parent(power->pixel_clk_rcg, power->pixel_parent); > +exit: > + return rc; > +} > + > +static int dp_power_init(struct dp_power *dp_power, bool flip) > +{ > + int rc = 0; > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + rc = -EINVAL; > + goto exit; > + } dp_power is always set. > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + pm_runtime_get_sync(&power->pdev->dev); > + rc = dp_power_regulator_ctrl(power, true); > + if (rc) { > + pr_err("failed to enable regulators\n"); This already prints the error messages you need. > + goto exit; > + } > + > + rc = dp_power_pinctrl_set(power, true); > + if (rc) { > + pr_err("failed to set pinctrl state\n"); Same. > + goto err_pinctrl; > + } > + > + rc = dp_power_config_gpios(power, flip, true); > + if (rc) { > + pr_err("failed to enable gpios\n"); Same. > + goto err_gpio; > + } > + > + rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true); > + if (rc) { > + pr_err("failed to enable DP core clocks\n"); Same. > + goto err_clk; > + } > + > + return 0; As above, flip the if statement and save a goto: if (!rc) return 0; dp_power_config_gpios(power, flip, false); ... > + > +err_clk: > + dp_power_config_gpios(power, flip, false); > +err_gpio: > + dp_power_pinctrl_set(power, false); > +err_pinctrl: > + dp_power_regulator_ctrl(power, false); > +exit: > + pm_runtime_put_sync(&power->pdev->dev); > + return rc; > +} > + > +static int dp_power_deinit(struct dp_power *dp_power) > +{ > + int rc = 0; > + struct dp_power_private *power; > + > + if (!dp_power) { > + pr_err("invalid power data\n"); > + rc = -EINVAL; > + goto exit; > + } dp_power can never be NULL; > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + dp_power_clk_enable(dp_power, DP_CORE_PM, false); > + dp_power_config_gpios(power, false, false); > + dp_power_pinctrl_set(power, false); > + dp_power_regulator_ctrl(power, false); > + pm_runtime_put_sync(&power->pdev->dev); > +exit: > + return rc; > +} > + > +struct dp_power *dp_power_get(struct dp_parser *parser) > +{ > + int rc = 0; > + struct dp_power_private *power; > + struct dp_power *dp_power; > + > + if (!parser) { > + pr_err("invalid input\n"); > + rc = -EINVAL; > + goto error; > + } parser can never be NULL; > + power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL); > + if (!power) { > + rc = -ENOMEM; Just return ERR_PTR(-ENOMEM); > + goto error; > + } > + > + power->parser = parser; > + power->pdev = parser->pdev; > + > + dp_power = &power->dp_power; > + > + dp_power->init = dp_power_init; > + dp_power->deinit = dp_power_deinit; > + dp_power->clk_enable = dp_power_clk_enable; > + dp_power->set_pixel_clk_parent = dp_power_set_pixel_clk_parent; > + dp_power->power_client_init = dp_power_client_init; > + dp_power->power_client_deinit = dp_power_client_deinit; > + > + return dp_power; > +error: > + return ERR_PTR(rc); > +} > + > +void dp_power_put(struct dp_power *dp_power) > +{ > + struct dp_power_private *power = NULL; > + > + if (!dp_power) > + return; > + > + power = container_of(dp_power, struct dp_power_private, dp_power); > + > + devm_kfree(&power->pdev->dev, power); You don't need to kfree device managed mmemory. > +} > diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h > new file mode 100644 > index 0000000..d1adaaf > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_power.h > @@ -0,0 +1,57 @@ > +/* > + * 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_POWER_H_ > +#define _DP_POWER_H_ > + > +#include "dp_parser.h" > +#include "dpu_power_handle.h" > + > +/** > + * sruct dp_power - DisplayPort's power related data > + * > + * @init: initializes the regulators/core clocks/GPIOs/pinctrl > + * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl > + * @clk_enable: enable/disable the DP clocks > + * @set_pixel_clk_parent: set the parent of DP pixel clock > + */ > +struct dp_power { > + int (*init)(struct dp_power *power, bool flip); > + int (*deinit)(struct dp_power *power); > + int (*clk_enable)(struct dp_power *power, enum dp_pm_type pm_type, > + bool enable); > + int (*set_pixel_clk_parent)(struct dp_power *power); > + int (*power_client_init)(struct dp_power *power); > + void (*power_client_deinit)(struct dp_power *power); > +}; > + > +/** > + * dp_power_get() - configure and get the DisplayPort power module data > + * > + * @parser: instance of parser module > + * return: pointer to allocated power module data > + * > + * This API will configure the DisplayPort's power module and provides > + * methods to be called by the client to configure the power related > + * modueles. > + */ > +struct dp_power *dp_power_get(struct dp_parser *parser); > + > +/** > + * dp_power_put() - release the power related resources > + * > + * @power: pointer to the power module's data > + */ > +void dp_power_put(struct dp_power *power); > +#endif /* _DP_POWER_H_ */ > diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h > new file mode 100644 > index 0000000..77b5c0e > --- /dev/null > +++ b/drivers/gpu/drm/msm/dp/dp_reg.h > @@ -0,0 +1,357 @@ > +/* > + * 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. <snip the rest> Jordan
On Mon, Nov 19, 2018 at 02:34:53PM -0800, chandanu@codeaurora.org wrote: > On 2018-10-23 09:28, Sean Paul wrote: > > On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote: > > > Add the needed displayPort files to enable DP driver > > > on msm target. > > > > > > "dp_display" module is the main module that calls into > > > other sub-modules. "dp_drm" file represents the interface > > > between DRM framework and DP driver. > > > > > > > Hello Sean, > I had few queries regarding your comments. Shared my queries below. Please > share your feedback. /snip > > > 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 @@ > > > > /snip > > > > > 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 > > > > /snip > > > > I didn't really review the dp_aux.* files as-is. Please convert to using > > drm_dp_aux and I'll take another look. > > > > Please correct me if I am wrong. Looks like you are suggesting to use most > of the drm_dp_aux struct variable > instead of what we have defined locally. > Correct /snip > > > + struct dp_catalog_ctrl ctrl = { > > > + .state_ctrl = dp_catalog_ctrl_state_ctrl, > > > + .config_ctrl = dp_catalog_ctrl_config_ctrl, > > > + .lane_mapping = dp_catalog_ctrl_lane_mapping, > > > + .mainlink_ctrl = dp_catalog_ctrl_mainlink_ctrl, > > > + .config_misc = dp_catalog_ctrl_config_misc, > > > + .config_msa = dp_catalog_ctrl_config_msa, > > > + .set_pattern = dp_catalog_ctrl_set_pattern, > > > + .reset = dp_catalog_ctrl_reset, > > > + .usb_reset = dp_catalog_ctrl_usb_reset, > > > + .mainlink_ready = dp_catalog_ctrl_mainlink_ready, > > > + .enable_irq = dp_catalog_ctrl_enable_irq, > > > + .hpd_config = dp_catalog_ctrl_hpd_config, > > > + .phy_reset = dp_catalog_ctrl_phy_reset, > > > + .phy_lane_cfg = dp_catalog_ctrl_phy_lane_cfg, > > > + .update_vx_px = dp_catalog_ctrl_update_vx_px, > > > + .get_interrupt = dp_catalog_ctrl_get_interrupt, > > > + .update_transfer_unit = dp_catalog_ctrl_update_transfer_unit, > > > + .send_phy_pattern = dp_catalog_ctrl_send_phy_pattern, > > > + .read_phy_pattern = dp_catalog_ctrl_read_phy_pattern, > > > > So, what's the benefit of adding the catalog abstractions? Do you ever > > have > > multiple/alternate implementations of a catalog? It doesn't seem like > > it. You > > should just remove all of the catalog overhead and call the functions > > directly. > > We use catalog abstractions for two different reasons: > 1. To support multiple hardware since these implementations are hardware > specific. > 2. To virtualize DP hardware. > > Our internal hardware already supports multiple chips. We want to keep these > abstractions to > make the code consistent between upstream and internal implementation. > I'm not sure how this differs from having a common header/function declarations and multiple definitions in a .c gated by CONFIG_*? Ideally even this wouldn't be necessary since hw changes are either incremental and can be done more precisely than wholesale changing the implementation. If you want to virtualize, you can do the CONFIG_* thing. However since that's not going upstream, you can carry the Kconfig/Makefile change in your downstream. All of these abstractions add thousands of lines of code, extra files, and multiple levels of indirection. IMO, I don't think the benefits outweigh the costs. /snip > > > +static void dp_ctrl_calc_tu_parameters(struct dp_ctrl_private *ctrl, > > > + struct dp_vc_tu_mapping_table *tu_table) > > > +{ > > > + u32 multiplier = 1000000; > > > + u64 pclk, lclk; > > > + u8 bpp, ln_cnt; > > > + int run_idx = 0; > > > + u32 lwidth, h_blank; > > > + u32 fifo_empty = 0; > > > + u32 ratio_scale = 1001; > > > + u64 temp, ratio, original_ratio; > > > + u64 temp2, reminder; > > > + u64 temp3, temp4, result = 0; > > > + > > > + u64 err = multiplier; > > > + u64 n_err = 0, n_n_err = 0; > > > + bool n_err_neg, nn_err_neg; > > > + u8 hblank_margin = 16; > > > + > > > + u8 tu_size, tu_size_desired = 0, tu_size_minus1; > > > + int valid_boundary_link; > > > + u64 resulting_valid; > > > + u64 total_valid; > > > + u64 effective_valid; > > > + u64 effective_valid_recorded; > > > + int n_tus; > > > + int n_tus_per_lane; > > > + int paired_tus; > > > + int remainder_tus; > > > + int remainder_tus_upper, remainder_tus_lower; > > > + int extra_bytes; > > > + int filler_size; > > > + int delay_start_link; > > > + int boundary_moderation_en = 0; > > > + int upper_bdry_cnt = 0; > > > + int lower_bdry_cnt = 0; > > > + int i_upper_bdry_cnt = 0; > > > + int i_lower_bdry_cnt = 0; > > > + int valid_lower_boundary_link = 0; > > > + int even_distribution_bf = 0; > > > + int even_distribution_legacy = 0; > > > + int even_distribution = 0; > > > + int min_hblank = 0; > > > + int extra_pclk_cycles; > > > + u8 extra_pclk_cycle_delay = 4; > > > + int extra_pclk_cycles_in_link_clk; > > > + u64 ratio_by_tu; > > > + u64 average_valid2; > > > + u64 extra_buffer_margin; > > > + int new_valid_boundary_link; > > > + > > > + u64 resulting_valid_tmp; > > > + u64 ratio_by_tu_tmp; > > > + int n_tus_tmp; > > > + int extra_pclk_cycles_tmp; > > > + int extra_pclk_cycles_in_lclk_tmp; > > > + int extra_req_bytes_new_tmp; > > > + int filler_size_tmp; > > > + int lower_filler_size_tmp; > > > + int delay_start_link_tmp; > > > + int min_hblank_tmp = 0; > > > + bool extra_req_bytes_is_neg = false; > > > + struct dp_panel_info *pinfo = &ctrl->panel->pinfo; > > > + > > > + u8 dp_brute_force = 1; > > > + u64 brute_force_threshold = 10; > > > + u64 diff_abs; > > > > > > 76 stack vars and ~450 lines must be a new record somewhere :) > > > > I'm going to skip reviewing this function and let you > > split/simplify/reduce it. > > Please just make it more readable (comments, docs, helper functions, > > etc). > > Yes, will try to split this function as modular as possible. > JFYI, this function is IP/hardware specific implementation. It might not be > possible > to provide lot of comments to make is understandable. Ok, please do what you can to simplify/split and add a comment that this is magic from the HW team. /snip > > > +int msm_dp_modeset_init(struct msm_dp *dp_display, struct > > > drm_device *dev, > > > + struct drm_encoder *encoder) > > > > Where is this called? > > This is called from dpu_kms.c. The changes from DPU will be dependent on > these changes. > Hmm, is that code missing? I don't see this function being called anywhere in this patch. /snip > > > +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, > > > +}; > > > > I'm not convinced you need any of this. The only advantage a bridge gets > > you is > > to be involved in modeset. However the only thing you do in modeset is > > save the > > mode (which is only used in enable). So why not just use the mode from > > the > > crtc's state when encoder->enable is called? > > > > That allows you to delete all of the bridge stuff here. > > > Will keep enable, disable and mode_set func ops and remove the remaining > ops. > Please let me know if you still have concerns with these func ops. > Why not remove all of it and call this code from the encoder's enable function? You shouldn't need a bridge at all. /snip > > > +static int dp_parser_init_clk_data(struct dp_parser *parser) > > > +{ > > > + int num_clk = 0, i = 0, rc = 0; > > > + int core_clk_count = 0, ctrl_clk_count = 0; > > > + const char *core_clk = "core"; > > > + const char *ctrl_clk = "ctrl"; > > > + const char *clk_name; > > > + struct device *dev = &parser->pdev->dev; > > > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM]; > > > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM]; > > > + > > > + num_clk = of_property_count_strings(dev->of_node, "clock-names"); > > > + if (num_clk <= 0) { > > > + pr_err("no clocks are defined\n"); > > > + rc = -EINVAL; > > > + goto exit; > > > + } > > > + > > > + for (i = 0; i < num_clk; i++) { > > > + of_property_read_string_index(dev->of_node, > > > + "clock-names", i, &clk_name); > > > > Check return value > > > > > + > > > + if (dp_parser_check_prefix(core_clk, clk_name)) > > > + core_clk_count++; > > > + > > > + if (dp_parser_check_prefix(ctrl_clk, clk_name)) > > > + ctrl_clk_count++; > > > > Just use "core"/"ctrl" directly here. > > > > Have you considered just having 2 clock lists? That would avoid having > > to do > > this. > I am not clear on what you are suggesting? We currently have two clock-lists > (core & ctrl). > > I meant in the dt bindings. Instead of one clocks list, could you split into core and ctrl? That way you wouldn't have to parse based on name. /snip > > > > ---- START hmmmm > I didn't understand, can you please provide more details regarding "START" > and "END". > > > > > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM]; > > > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM]; > > > + > > > + core_power = &parser->mp[DP_CORE_PM]; > > > + ctrl_power = &parser->mp[DP_CTRL_PM]; > > > > ---- END hmmmm > > You assign core_power and ctrl_power in the declaration and then immediately assign the value again. /snip
On Thu, Feb 14, 2019 at 04:28:33AM -0800, chandanu@codeaurora.org wrote: > On 2018-10-23 09:28, Sean Paul wrote: > > On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote: > > > Add the needed displayPort files to enable DP driver > > > on msm target. > > > > > > "dp_display" module is the main module that calls into > > > other sub-modules. "dp_drm" file represents the interface > > > between DRM framework and DP driver. > > > > > > > Hello Sean, > I have responded to all of your comments and queries. I have specified the > comments addressed > in V2 patch series and what I have missed that will be fixed in V3. > > /snip > > > > > > +struct dss_io_data { > > > + u32 len; > > > + void __iomem *base; > > > +}; > > > > len isn't used anywhere, so this struct can go away. If the struct goes > > away, > > you can really simplify (or eliminate msm_dss_ioremap_byname()) > > > > We want to keep this since we use len for our internal unit level testing > and in simulation mode. > That's not really a valid reason to keep unused code around. We can't reasonably expect upstream developers to know which structs and unused fields are used for Qualcomm's downstream testing, and afaict, those results aren't published so there's no benefit to the upsteam community. So you can either opensource the unit tests along with the driver, or keep the downstream code in the downstream kernel. /snip > > > + > > > +#define dp_catalog_get_priv(x) { \ > > > > static inline struct dp_catalog_private *dp_catalog_get_priv(...) > > { > > ... > > } > > > For inline, we have to use 3 different functions (one each for aux, ctrl and > panel). > Looking at "type-checking" w.r.t macro, container_of will through an error > if its not valid pointer. > Please let me know if you still want to change this macro to inline func. > Yeah, I think it's worth it to have the 3. > > > + 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); \ > > > +} /snip > > > > Then again, I'm guessing hitting the NULL case is likely impossible, so > > you can > > probably remove all of the machinery around checking !aux. > > > > Removed most of the arg null checks except few APIs where > unit-testing/simulator-mode is used. > There should only be null checks if null is a possibility. If you have pathelogical unit tests injecting bogus values in your function, you should probably just fix/delete those unit tests. If upstream doesn't have visibility into your internal testing, we can't distinguish between test-pleasing vs unused code. > > > +static void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog_ctrl > > > *ctrl, > > > + u32 pattern) > > > +{ > > > + struct dp_catalog_private *catalog; > > > + u32 value = 0x0; > > > + void __iomem *base = NULL; > > > + > > > + if (!ctrl) { > > > + pr_err("invalid input\n"); > > > + return; > > > + } > > > + > > > + dp_catalog_get_priv(ctrl); > > > + > > > + base = catalog->io->dp_link.base; > > > + > > > + dp_write(base + DP_STATE_CTRL, 0x0); > > > > Do you really need to do this? > > > Yes, to make sure no other pattern is set/enabled in hardware before any new > pattern is configured. > > > > + > > > + switch (pattern) { > > > + case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING: > > > + dp_write(base + DP_STATE_CTRL, 0x1); > > > + break; > > > + case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT: > > > + value &= ~(1 << 16); > > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); > > > + value |= 0xFC; > > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); > > > + dp_write(base + DP_MAINLINK_LEVELS, 0x2); > > > + dp_write(base + DP_STATE_CTRL, 0x10); > > > + break; > > > + case DP_TEST_PHY_PATTERN_PRBS7: > > > + dp_write(base + DP_STATE_CTRL, 0x20); > > > + break; > > > + case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN: > > > + dp_write(base + DP_STATE_CTRL, 0x40); > > > + /* 00111110000011111000001111100000 */ > > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG0, 0x3E0F83E0); > > > + /* 00001111100000111110000011111000 */ > > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG1, 0x0F83E0F8); > > > + /* 1111100000111110 */ > > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG2, 0x0000F83E); > > > + break; > > > + case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN: > > > + value = BIT(16); > > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); > > > + value |= 0xFC; > > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value); > > > + dp_write(base + DP_MAINLINK_LEVELS, 0x2); > > > + dp_write(base + DP_STATE_CTRL, 0x10); > > > > This code is the exact same as the case for > > DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT, aside from BIT(16) vs > > ~BIT(16). Can you please combine these cases > > and share code? > > Will fix this in V3. > > > > > > + break; > > > + default: > > > + pr_debug("No valid test pattern requested: 0x%x\n", pattern); > > > + return; > > > + } > > > > All of the DP_STATE_CTRL values should be #defined, and you can move the > > write > > down here instead of duplicating it everywhere (with > > DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN writing a 0). > > > All the DP_STATE_CTRL values are defines in V2. > > I am not clear on moving the write operations after the switch cases. Can > you please provide > some more details? I think what I meant was that since all cases finish with a write to DP_STATE_CTRL, you can move that write out of each case and put it at the bottom. But I think I missed that DP_STATE_CTRL is set at the beginning of DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN, so I don't think that'll work. /snip > > > +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; > > > > Once you remove struct dp_ctrl and call the functions directly, I > > suspect you > > can get rid of all of these. > > > Even though we call the functions directly, we still need variables present > in each module. > For example, we need to access link parameters, panel parameters etc > > Unnecessary stuff is removed but we still need other variables in the > structure. All have been > updated in V2. Ok, I'll re-evaluate in v3 /snip > > > +static void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl) > > > +{ > > > + int const idle_pattern_completion_timeout_ms = 3 * HZ / 100; > > > > This should be #define'd outside of the functioninstead. Also, why the > > '_ms_' > > suffix? The units are in jiffies. > > > > I think something like > > > > #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000) /* 30 > > ms */ > > > > would be clearer. > > > Fixed in V2. > > > > + 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); > > > > This function is called from a bunch of places, and idle_comp is also > > used in a > > bunch of places. Are you certain this is thread-safe if you have > > concurrent > > callers or users of idle_comp? > > > I don't think we need any synchronization logic for idle_comp since it is > reinitialized only > when the complete core is reset. Only place where it is needed is in > push_idle which already has > a mutex. So here are the callsites for push_idle(): - dp_display_clean- called on extcon event thread, unlocked - dp_display_pre_disable- called on drm thread, locked with modeset locks - dp_ctrl_link_maintenance- called in hpd handler, unlocked It _seems_ like there could be races here, and I don't know which mutex you're referring to above. Mind walking me through it in more detail? > > > > + 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"); > > > > Return an error? > > > Its not fatal error since we are going to reset the controller and we are > going to recover from > this timeout. In that case, can you please downgrade the log level to debug or info? /snip > > > + > > > + if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) > > > > You should really protect sink_request with a lock, it's racy. I'm > > trying > > to catch the race conditions while I review, but I'd urge you to audit > > the > > driver for other places that need locking. > > > sink_request will be running in a single-threaded work queue. > ("drm_dp_extcon" work queue). > We might not need locking for sink_request. > It seems like sink_request could change in between reads in dp_ctrl_on? dp_ctrl_on reads the value twice, and calls dp_ctrl_setup_main_link which also reads the value. /snip > > > +static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl, > > > + char *name, u32 rate) > > > +{ > > > + u32 num = ctrl->parser->mp[DP_CTRL_PM].num_clk; > > > + struct dss_clk *cfg = ctrl->parser->mp[DP_CTRL_PM].clk_config; > > > + > > > + while (num && strcmp(cfg->clk_name, name)) { > > > + num--; > > > + cfg++; > > > + } > > > + > > > + pr_debug("setting rate=%d on clk=%s\n", rate, name); > > > + > > > + if (num) > > > + cfg->rate = rate; > > > + else > > > + pr_err("%s clock could not be set with rate %d\n", name, rate); > > > +} > > > > Instead of doing all of this work, just store pointers to the link and > > pixel > > clock when you parse the dt. That way you can just access them directly > > in > > dp_ctrl_enable_mainlink_clocks. > > > These are generic implementation for DP_CORE and DP_CTRL clocks. This will > help > for any future upgrades where the only change will be done dtsi and maybe > minor > changes in Driver when we add/remove any clocks. Just add new pointers and NULL checks in the future if/when this is needed. /snip > > > + > > > + if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) > > > > This is racy, sink_request could change between the check above and > > here. > > > Sink_request is accessed in single thread context. > Ah, so I mentioned that in the earlier review. I guess I'm not seeing what prevents that single thread from modifying the value twice? This code will be run in a different thread, so it really seems like there could be a race here. /snip > > > +static void dp_display_send_hpd_event(struct msm_dp *dp_display) > > > +{ > > > + struct dp_display_private *dp; > > > + struct drm_connector *connector; > > > + > > > + if (!dp_display) { > > > + pr_err("invalid input\n"); > > > + return; > > > + } > > > + > > > + dp = container_of(dp_display, struct dp_display_private, > > > dp_display); > > > + if (!dp) { > > > + pr_err("invalid params\n"); > > > + return; > > > + } > > > + > > > + connector = dp->dp_display.connector; > > > + drm_helper_hpd_irq_event(connector->dev); > > > +} > > > + > > > +static int dp_display_send_hpd_notification(struct > > > dp_display_private *dp, > > > + bool hpd) > > > > This function is called from a few different threads with no locking. It > > seems > > like there's good opportunity for is_connected and notification_comp to > > get out > > of sync. > > > > Additionally, is_connected is set to true before the function returns > > (and > > enable has been completed). So is that going to cause problems when we > > call > > detect/get_modes where is_connected == true and the hw is not up? > > > Will fix this in V3. > > > > Perhaps I'm misreading this function, but it seems like the plan is to > > block > > until userspace has been notified and does a full modeset (ie: enable)? > > If that > > is the case, we usually don't want to have the hardware lit up until > > it's > > actually going to be used. In these cases, we'll wrap get_modes in > > enable/disable (with proper locking), and wait for modeset to leave it > > on. > > > When HPD goes high, we do dpcd reads to get the sink capabilities and then > inform the framework so that "enable" > gets called by drm framework. > I'm not sure I follow what you're saying. Is notification_comp meant to ratelimit/debounce hpd notifications? I don't think it's needed, and if it is, it would be interesting to know why. > > > +{ > > > + if ((hpd && dp->dp_display.is_connected) || > > > + (!hpd && !dp->dp_display.is_connected)) { > > > + pr_info("HPD already %s\n", (hpd ? "on" : "off")); > > > + return 0; > > > + } > > > + > > > + /* reset video pattern flag on disconnect */ > > > + if (!hpd) > > > + dp->panel->video_test = false; > > > + > > > + dp->dp_display.is_connected = hpd; > > > + reinit_completion(&dp->notification_comp); > > > + dp_display_send_hpd_event(&dp->dp_display); > > > + > > > + if (!wait_for_completion_timeout(&dp->notification_comp, HZ * 2)) { > > > + pr_warn("%s timeout\n", hpd ? "connect" : "disconnect"); > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} /snip > > > +/** > > > + * dp_connector_get_modes - callback to add drm modes via > > > drm_mode_probed_add() > > > + * @connector: Pointer to drm connector structure > > > + * Returns: Number of modes added > > > + */ > > > +static int dp_connector_get_modes(struct drm_connector *connector) > > > +{ > > > + int rc = 0; > > > + struct msm_dp *dp; > > > + struct dp_display_mode *dp_mode = NULL; > > > + struct drm_display_mode *m, drm_mode; > > > + struct dp_connector *dp_conn; > > > + > > > + if (!connector) > > > + return 0; > > > + > > > + dp_conn = to_dp_connector(connector); > > > + dp = dp_conn->dp_display; > > > + > > > + dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); > > > + if (!dp_mode) > > > + return 0; > > > > When you get rid of dp_display_mode, please use the stack for the new > > drm_display_mode > > > Fixed in V2. > > > > + > > > + /* pluggable case assumes EDID is read when HPD */ > > > + if (dp->is_connected) { > > > + rc = dp->get_modes(dp, dp_mode); > > > > Why is only one mode returned? > > > The get_modes() function might return one mode that is stored > in dp_mode when compliance test is in progress. If not, the > return value is equal to the total number of modes supported > by the sink Ah. I think this would be better to do this work in dp_panel_get_modes and return 1 from there. Then you can remove all of the special handling from this function. > > > > + if (!rc) > > > > This can also return -errno, you should check for that too. > > > Fixed in V2. > > > > + pr_err("failed to get DP sink modes, rc=%d\n", rc); > > > > Why continue if you know you failed? > > > > > + > > > + if (dp_mode->timing.pixel_clk_khz) { /* valid DP mode */ > > > + memset(&drm_mode, 0x0, sizeof(drm_mode)); > > > + convert_to_drm_mode(dp_mode, &drm_mode); > > > + m = drm_mode_duplicate(connector->dev, &drm_mode); > > > + if (!m) { > > > + pr_err("failed to add mode %ux%u\n", > > > + drm_mode.hdisplay, > > > + drm_mode.vdisplay); > > > + kfree(dp_mode); > > > + return 0; > > > + } > > > + m->width_mm = connector->display_info.width_mm; > > > + m->height_mm = connector->display_info.height_mm; > > > > You shouldn't overwrite these. > > > Fixed in V2. > > > > + drm_mode_probed_add(connector, m); > > > + } > > > + } else { > > > + pr_err("No sink connected\n"); > > > > This shouldn't be an error > > > Fixed in V2. > > > > + } > > > + kfree(dp_mode); > > > + > > > + return rc; > > > +} /snip > > > + * @link: Display Port Driver data > > > + * > > > + * Parses the DPCD to check if an automated link is requested (Byte > > > 0x201), > > > + * and what type of link automation is being requested (Byte 0x218). > > > + */ > > > +static int dp_link_parse_request(struct dp_link_private *link) > > > +{ > > > + int ret = 0; > > > + u8 bp; > > > + u8 data; > > > > Only need 1 of bp/data > > > > > + int rlen; > > > > ssize_t > > > > > + u32 const param_len = 0x1; > > > > Not needed, use drm_dp_dpcd_readb() > > > > > + > > > + /** > > > + * Read the device service IRQ vector (Byte 0x201) to determine > > > + * whether an automated link has been requested by the sink. > > > + */ > > > + rlen = drm_dp_dpcd_read(link->aux->drm_aux, > > > + DP_DEVICE_SERVICE_IRQ_VECTOR, &bp, param_len); > > > + if (rlen < param_len) { > > > + pr_err("aux read failed\n"); > > > > Print error. This goes for other failure messages in this file. > > > Addressed all the comments above in this function in V2. > > > > + ret = -EINVAL; > > > + goto end; > > > + } > > > + > > > + data = bp; > > > + > > > + pr_debug("device service irq vector = 0x%x\n", data); > > > + > > > + if (!(data & DP_AUTOMATED_TEST_REQUEST)) { > > > + pr_debug("no test requested\n"); > > > + return 0; > > > + } > > > + > > > + /** > > > + * Read the link request byte (Byte 0x218) to determine what type > > > + * of automated link has been requested by the sink. > > > + */ > > > + rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_REQUEST, > > > + &bp, param_len); > > > + if (rlen < param_len) { > > > + pr_err("aux read failed\n"); > > > + ret = -EINVAL; > > > + goto end; > > > + } > > > + > > > + data = bp; > > > + > > > + if (!dp_link_is_test_supported(data)) { > > > + pr_debug("link 0x%x not supported\n", data); > > > + goto end; > > > + } > > > + > > > + pr_debug("%s (0x%x) requested\n", dp_link_get_test_name(data), > > > data); > > > + link->request.test_requested = data; > > > > If test_requested is only used in the scope of this function, just use a > > local > > and pass it around. If test_requested is used outside the scope of this > > function, you need locking. > > test_requested in only used in dp_link.c. But we use this in dp_debug for > debugging. We also > use this in unit testing. This is one of main reason we have it in dp_link > structure. > Ok, same comment regarding unit testing as above. Please change this to a local in the upstream version. /snip > > > + /** > > > > /* > > > Multi-line comments, we use "/**" > This will cause warnings in the htmldocs build. See https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting /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; > > > + } > > > + > > > + link = container_of(dp_link, struct dp_link_private, dp_link); > > > + > > > + drm_dp_dpcd_write(link->aux->drm_aux, DP_TEST_RESPONSE, > > > + &dp_link->test_response, response_len); > > > > I strongly suspect you should have locking protecting test_response. > > > Will cross-check whether we need locking and fix it in V3. Just curious, do > you still think > we need locking even though we handle this in a single thread? AFAICT, this is called from the drm thread (via dp_ctrl_on->dp_ctrl_send_phy_test_pattern), and extcon hpd thread? /snip > > > +static u8 get_link_status(const u8 > > > link_status[DP_LINK_STATUS_SIZE], int r) > > > +{ > > > + return link_status[r - DP_LANE0_1_STATUS]; > > > > I don't think you're really supposed to reach into the link_status like > > this. > > How about adding a new helper to check for DP_LINK_STATUS_UPDATED and > > one for > > DP_DOWNSTREAM_PORT_STATUS_CHANGED? > > > This function is similar to function in upstream drm_dp_helper.c file. > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.18/drivers/gpu/drm/drm_dp_helper.c#259 > Yeah, that's my point, it's something that's done internally to the helper. Adding a couple helpers to drm_dp_helper will clean up this code and also benefit others who need to do the same thing (I don't see anyone else checking those bits yet). > > > +} /snip > > > +static bool dp_link_is_ds_port_status_changed(struct > > > dp_link_private *link) > > > +{ > > > + if (get_link_status(link->link_status, > > > DP_LANE_ALIGN_STATUS_UPDATED) & > > > + DP_DOWNSTREAM_PORT_STATUS_CHANGED) /* port status changed */ > > > + return true; > > > + > > > + if (link->prev_sink_count != link->dp_link.sink_count.count) > > > > It seems like this is running in a separate thread from where > > prev_sink_count is > > set, potential race here? > > > I don't think we might need locking here. All the updates/checks related to > prev_sink_count are done > in dp_link_process_request() which will run in single threaded work queue. > Hmm, didn't realize it's only set/accessed in dp_link_process_request. Can you change this to a local var and just pass it via arguments so 1- the code is more clear, and 2- there's no chance it gets stomped by another thread. /snip > > > +static u32 dp_link_get_test_bits_depth(struct dp_link *dp_link, u32 > > > bpp) > > > +{ > > > + u32 tbd; > > > + > > > + /* > > > + * Few simplistic rules and assumptions made here: > > > + * 1. Test bit depth is bit depth per color component > > > + * 2. Assume 3 color components > > > + */ > > > + switch (bpp) { > > > + case 18: > > > + tbd = DP_TEST_BIT_DEPTH_6; > > > + break; > > > + case 24: > > > + tbd = DP_TEST_BIT_DEPTH_8; > > > + break; > > > + case 30: > > > + tbd = DP_TEST_BIT_DEPTH_10; > > > + break; > > > + default: > > > + tbd = DP_TEST_BIT_DEPTH_UNKNOWN; > > > + break; > > > + } > > > + > > > + if (tbd != DP_TEST_BIT_DEPTH_UNKNOWN) > > > + tbd = (tbd >> DP_TEST_BIT_DEPTH_SHIFT); > > > > AFAICT tbd corresponds to a SoC register value, as opposed to the DPCD > > value. > > It's a happy coincidence that they're the same, however I think you > > should use a > > separate msm-specific #define instead of the drm_dp_helper values. > > > These are standard bpp values and will match to the DPCD values. For the > same reason > we are using existing standard macros. Hmm, Ok. Mind adding a comment then? It's a bit confusing to mix the values. /snip > > > + rc = dp_panel_read_edid(dp_panel, connector); > > > > Why read the edid so early? Can't you wait until get_modes is called > > (you'll > > have to re-read anyways). > > > We won't do re-read again when get_modes is called. > You really should be, userspace should determine when/whether to limit it. With that change, you can remove the read from here. > > > + if (rc) { > > > + pr_err("panel edid read failed, set failsafe mode\n"); > > > + return rc; > > > + } > > > + > > > + if (panel->aux_cfg_update_done) { > > > + pr_debug("read DPCD with updated AUX config\n"); > > > + dp_panel_read_dpcd(dp_panel); > > > > Check return value. Should you also check the rate/lane_count validity > > again too? > > > Fixed in V2. > > > > + panel->aux_cfg_update_done = false; > > > > Should you have locking for aux_cfg_update_done? > > > panel_read_edid() and aux_cfg_update_done = false is done one after the > other in this function. I don't think > we locking for this. Another good case of not needing to store this in a central place. Just make this a local in dp_panel_read_sink_caps(). /snip > > > + mp->vreg_config = devm_kzalloc(&parser->pdev->dev, > > > + sizeof(struct dss_vreg) * mp->num_vreg, GFP_KERNEL); > > > + if (!mp->vreg_config) { > > > + rc = -ENOMEM; > > > + goto error; > > > + } > > > + > > > + for_each_child_of_node(supply_root_node, supply_node) { > > > + const char *st = NULL; > > > + /* vreg-name */ > > > + rc = of_property_read_string(supply_node, > > > + "qcom,supply-name", &st); > > > + if (rc) { > > > + pr_err("error reading name. rc=%d\n", > > > + rc); > > > + goto error; > > > + } > > > + snprintf(mp->vreg_config[i].vreg_name, > > > + ARRAY_SIZE((mp->vreg_config[i].vreg_name)), "%s", st); > > > > Instead of storing the name just to get a regulator reference later, why > > don't > > you just grab the reference now and avoid the string ops? > > > Vreg keep changing for each chip. This is more generic implementation that > avoids doing changes to the code. > Updating the dtsi-platform specific changes will have access to the vregs. Changing the code is fun, string operations are not fun. In order to maximize fun, can we please change these? More seriously, trying to come up with generic solutions is super tricky and more often than not, incorrect. The only thing we know for certain about designing code is that it will eventually be obsolete :) /snip > > > + > > > + if (dp_parser_check_prefix(core_clk, clk_name)) > > > + core_clk_count++; > > > + > > > + if (dp_parser_check_prefix(ctrl_clk, clk_name)) > > > + ctrl_clk_count++; > > > > Just use "core"/"ctrl" directly here. > > > > Have you considered just having 2 clock lists? That would avoid having > > to do > > this. > > > I didn't get what you meant by two clock lists. We currently have two clock > list to store them locally. > You mean having two clock lists in dtsi? > Yes, I believe that's what I meant. /snip > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > > > Forum, > > > a Linux Foundation Collaborative Project > > > > > > _______________________________________________ > > > Freedreno mailing list > > > Freedreno@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/freedreno
On 2019-02-14 04:28, chandanu@codeaurora.org wrote: Hello Sean I had few more queries regarding your comments. Can you please provide your feedback? (Queries/responses are present below) thanks Chandan > On 2018-10-23 09:28, Sean Paul wrote: >> On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote: >>> Add the needed displayPort files to enable DP driver >>> on msm target. >>> >>> "dp_display" module is the main module that calls into >>> other sub-modules. "dp_drm" file represents the interface >>> between DRM framework and DP driver. >>> >> > > Hello Sean, > I have responded to all of your comments and queries. I have specified > the comments addressed > in V2 patch series and what I have missed that will be fixed in V3. > > >> Hi Chandan, >> A few themes which I've pointed out below, but want to call out at the >> top so >> it's obvious. >> >> - Please remove all superfluous NULL checks at the beginning of >> functions. >> Included in this is reflowing initialization of variables and >> checking that >> return types still make sense (ie: if a function's only error path >> is the NULL >> check, make it void). >> - Please remove all of the new callbacks and just call functions >> directly. This >> should also get rid of the _get/_put functions that allocate/set >> them up >> - Please consolidate state structs into one struct with a few >> substructs, >> there's too much state right now. >> - Please #define hardcoded values >> - Instead of sprinkling wmb() everywhere, use readl/writel instead of >> *_relaxed >> - There are a bunch of races that I found, please audit the code and >> remove >> races with locks as necessary >> - I tried to catch as much dead code as possible, please audit the >> rest for >> unused structs/variables/functions/etc >> - Use drm dp helper structs for link parameter storage >> >> >>> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org> >>> --- >>> drivers/gpu/drm/msm/Kconfig | 9 + >>> drivers/gpu/drm/msm/Makefile | 15 +- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 ++++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 + >>> drivers/gpu/drm/msm/dp/dp_aux.c | 570 ++++++++++ >>> drivers/gpu/drm/msm/dp/dp_aux.h | 44 + >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 >>> ++++++++++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_catalog.h | 144 +++ >>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475 >>> +++++++++++++++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 + >>> drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++++ >>> drivers/gpu/drm/msm/dp/dp_debug.h | 81 ++ >>> drivers/gpu/drm/msm/dp/dp_display.c | 977 +++++++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_display.h | 55 + >>> drivers/gpu/drm/msm/dp/dp_drm.c | 542 ++++++++++ >>> drivers/gpu/drm/msm/dp/dp_drm.h | 52 + >>> drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++++ >>> drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++ >>> drivers/gpu/drm/msm/dp/dp_link.c | 1549 >>> +++++++++++++++++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_link.h | 184 ++++ >>> drivers/gpu/drm/msm/dp/dp_panel.c | 624 +++++++++++ >>> drivers/gpu/drm/msm/dp/dp_panel.h | 121 +++ >>> drivers/gpu/drm/msm/dp/dp_parser.c | 679 ++++++++++++ >>> drivers/gpu/drm/msm/dp/dp_parser.h | 205 ++++ >>> drivers/gpu/drm/msm/dp/dp_power.c | 599 +++++++++++ >>> drivers/gpu/drm/msm/dp/dp_power.h | 57 + >>> drivers/gpu/drm/msm/dp/dp_reg.h | 357 ++++++ >>> drivers/gpu/drm/msm/msm_drv.c | 2 + >>> drivers/gpu/drm/msm/msm_drv.h | 22 + >>> include/drm/drm_dp_helper.h | 19 + >>> 30 files changed, 10887 insertions(+), 1 deletion(-) >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_aux.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_catalog.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_ctrl.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_debug.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_display.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_display.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_drm.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_extcon.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_link.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_link.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_panel.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_parser.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_power.c >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_power.h >>> create mode 100644 drivers/gpu/drm/msm/dp/dp_reg.h >>> >> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c >>> new file mode 100644 >>> index 0000000..5818612 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >>> @@ -0,0 +1,1188 @@ >>> +/* >>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights >>> reserved. >>> + * >>> + * This program is free software; you can redistribute it and/or >>> modify >>> + * it under the terms of the GNU General Public License version 2 >>> and >>> + * only version 2 as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + */ >>> + >>> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__ >>> + >>> +#include <linux/delay.h> >>> +#include <drm/drm_dp_helper.h> >>> + >>> +#include "dp_catalog.h" >>> +#include "dp_reg.h" >>> + >>> +#define DP_GET_MSB(x) (x >> 8) >>> +#define DP_GET_LSB(x) (x & 0xff) >> >> These aren't used anywhere >> > Fixed in V2. > >>> + >>> +#define dp_read(offset) readl_relaxed((offset)) >>> +#define dp_write(offset, data) writel_relaxed((data), (offset)) >> >> These macros aren't useful. I think you'd be much better having a >> read_/write_ >> for the different io types, ie: >> >> static inline u32 dp_read_audio(struct dp_catalog *catalog, u32 >> offset) >> { >> return readl_relaxed(catalog->io->dp_link.base + offset); >> } >> >> Then you can remove a ton of boilerplate code below getting base; > > Fixed in V2. > >> >>> + >>> +#define dp_catalog_get_priv(x) { \ >> >> static inline struct dp_catalog_private *dp_catalog_get_priv(...) >> { >> ... >> } >> > For inline, we have to use 3 different functions (one each for aux, > ctrl and panel). > Looking at "type-checking" w.r.t macro, container_of will through an > error if its not valid pointer. > Please let me know if you still want to change this macro to inline > func. > /snip >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h >>> b/drivers/gpu/drm/msm/dp/dp_catalog.h >>> new file mode 100644 >>> index 0000000..58951df >>> --- /dev/null >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h >>> @@ -0,0 +1,144 @@ >>> +/* >>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights >>> reserved. >>> + * >>> + * This program is free software; you can redistribute it and/or >>> modify >>> + * it under the terms of the GNU General Public License version 2 >>> and >>> + * only version 2 as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + */ >>> + >>> +#ifndef _DP_CATALOG_H_ >>> +#define _DP_CATALOG_H_ >>> + >>> +#include "dp_parser.h" >>> + >>> +/* interrupts */ >>> +#define DP_INTR_HPD BIT(0) >>> +#define DP_INTR_AUX_I2C_DONE BIT(3) >>> +#define DP_INTR_WRONG_ADDR BIT(6) >>> +#define DP_INTR_TIMEOUT BIT(9) >>> +#define DP_INTR_NACK_DEFER BIT(12) >>> +#define DP_INTR_WRONG_DATA_CNT BIT(15) >>> +#define DP_INTR_I2C_NACK BIT(18) >>> +#define DP_INTR_I2C_DEFER BIT(21) >>> +#define DP_INTR_PLL_UNLOCKED BIT(24) >>> +#define DP_INTR_AUX_ERROR BIT(27) >>> + >>> +#define DP_INTR_READY_FOR_VIDEO BIT(0) >>> +#define DP_INTR_IDLE_PATTERN_SENT BIT(3) >>> +#define DP_INTR_FRAME_END BIT(6) >>> +#define DP_INTR_CRC_UPDATED BIT(9) >>> + >>> +struct dp_catalog_aux { >>> + u32 data; >>> + u32 isr; >>> + >>> + u32 (*read_data)(struct dp_catalog_aux *aux); >>> + int (*write_data)(struct dp_catalog_aux *aux); >>> + int (*write_trans)(struct dp_catalog_aux *aux); >>> + int (*clear_trans)(struct dp_catalog_aux *aux, bool read); >>> + void (*reset)(struct dp_catalog_aux *aux); >>> + void (*enable)(struct dp_catalog_aux *aux, bool enable); >>> + void (*update_aux_cfg)(struct dp_catalog_aux *aux, >>> + struct dp_aux_cfg *cfg, enum dp_phy_aux_config_type type); >>> + void (*setup)(struct dp_catalog_aux *aux, >>> + struct dp_aux_cfg *aux_cfg); >>> + void (*get_irq)(struct dp_catalog_aux *aux, bool cmd_busy); >>> +}; >>> + >>> +struct dp_catalog_ctrl { >>> + u32 dp_tu; >>> + u32 valid_boundary; >>> + u32 valid_boundary2; >>> + u32 isr; >>> + >>> + void (*state_ctrl)(struct dp_catalog_ctrl *ctrl, u32 state); >>> + void (*config_ctrl)(struct dp_catalog_ctrl *ctrl, u32 config); >>> + void (*lane_mapping)(struct dp_catalog_ctrl *ctrl); >>> + void (*mainlink_ctrl)(struct dp_catalog_ctrl *ctrl, bool enable); >>> + void (*config_misc)(struct dp_catalog_ctrl *ctrl, u32 cc, u32 tb); >>> + void (*config_msa)(struct dp_catalog_ctrl *ctrl, u32 rate, >>> + u32 stream_rate_khz, bool fixed_nvid); >>> + void (*set_pattern)(struct dp_catalog_ctrl *ctrl, u32 pattern); >>> + void (*reset)(struct dp_catalog_ctrl *ctrl); >>> + void (*usb_reset)(struct dp_catalog_ctrl *ctrl, bool flip); >>> + bool (*mainlink_ready)(struct dp_catalog_ctrl *ctrl); >>> + void (*enable_irq)(struct dp_catalog_ctrl *ctrl, bool enable); >>> + void (*hpd_config)(struct dp_catalog_ctrl *ctrl, bool enable); >>> + void (*phy_reset)(struct dp_catalog_ctrl *ctrl); >>> + void (*phy_lane_cfg)(struct dp_catalog_ctrl *ctrl, bool flipped, >>> + u8 lane_cnt); >>> + void (*update_vx_px)(struct dp_catalog_ctrl *ctrl, u8 v_level, >>> + u8 p_level); >>> + void (*get_interrupt)(struct dp_catalog_ctrl *ctrl); >>> + void (*update_transfer_unit)(struct dp_catalog_ctrl *ctrl); >>> + void (*send_phy_pattern)(struct dp_catalog_ctrl *ctrl, >>> + u32 pattern); >>> + u32 (*read_phy_pattern)(struct dp_catalog_ctrl *ctrl); >>> +}; >>> + >>> +enum dp_catalog_audio_sdp_type { >>> + DP_AUDIO_SDP_STREAM, >>> + DP_AUDIO_SDP_TIMESTAMP, >>> + DP_AUDIO_SDP_INFOFRAME, >>> + DP_AUDIO_SDP_COPYMANAGEMENT, >>> + DP_AUDIO_SDP_ISRC, >>> + DP_AUDIO_SDP_MAX, >>> +}; >>> + >>> +enum dp_catalog_audio_header_type { >>> + DP_AUDIO_SDP_HEADER_1, >>> + DP_AUDIO_SDP_HEADER_2, >>> + DP_AUDIO_SDP_HEADER_3, >>> + DP_AUDIO_SDP_HEADER_MAX, >>> +}; >>> + >>> +struct dp_catalog_audio { >>> + enum dp_catalog_audio_sdp_type sdp_type; >>> + enum dp_catalog_audio_header_type sdp_header; >>> + u32 data; >>> + >>> + void (*init)(struct dp_catalog_audio *audio); >>> + void (*enable)(struct dp_catalog_audio *audio); >>> + void (*config_acr)(struct dp_catalog_audio *audio); >>> + void (*config_sdp)(struct dp_catalog_audio *audio); >>> + void (*set_header)(struct dp_catalog_audio *audio); >>> + void (*get_header)(struct dp_catalog_audio *audio); >>> + void (*safe_to_exit_level)(struct dp_catalog_audio *audio); >> >> None of these are called? So get rid of this entire structure, the >> functions >> that aren't called, and the audio enums above. >> > Removed all the Audio related APIs in V2. > >>> +}; >>> + >>> +struct dp_catalog_panel { >>> + u32 total; >>> + u32 sync_start; >>> + u32 width_blanking; >>> + u32 dp_active; >>> + >>> + /* TPG */ >>> + u32 hsync_period; >>> + u32 vsync_period; >>> + u32 display_v_start; >>> + u32 display_v_end; >>> + u32 v_sync_width; >>> + u32 hsync_ctl; >>> + u32 display_hctl; >> >> All of this information is already available from drm core, so it can >> go. > > Will address this in V3. > /snip >>> + >>> +static void dp_ctrl_setup_tr_unit(struct dp_ctrl_private *ctrl) >>> +{ >>> + u32 dp_tu = 0x0; >>> + u32 valid_boundary = 0x0; >>> + u32 valid_boundary2 = 0x0; >>> + struct dp_vc_tu_mapping_table tu_calc_table; >>> + >>> + dp_ctrl_calc_tu_parameters(ctrl, &tu_calc_table); >>> + >>> + dp_tu |= tu_calc_table.tu_size_minus1; >>> + valid_boundary |= tu_calc_table.valid_boundary_link; >>> + valid_boundary |= (tu_calc_table.delay_start_link << 16); >>> + >>> + valid_boundary2 |= (tu_calc_table.valid_lower_boundary_link << 1); >>> + valid_boundary2 |= (tu_calc_table.upper_boundary_count << 16); >>> + valid_boundary2 |= (tu_calc_table.lower_boundary_count << 20); >>> + >>> + if (tu_calc_table.boundary_moderation_en) >>> + valid_boundary2 |= BIT(0); >>> + >>> + pr_debug("dp_tu=0x%x, valid_boundary=0x%x, valid_boundary2=0x%x\n", >>> + dp_tu, valid_boundary, valid_boundary2); >>> + >>> + ctrl->catalog->dp_tu = dp_tu; >>> + ctrl->catalog->valid_boundary = valid_boundary; >>> + ctrl->catalog->valid_boundary2 = valid_boundary2; >>> + >>> + ctrl->catalog->update_transfer_unit(ctrl->catalog); >> >> Again here, if you just call the function directly, you can avoid the >> callback. >> If you add function arguments, you can avoid having to store >> dp_tu/valid_boundary/ >> valid_boundary2 entirely. >> > Will remove the dp_tu/valid_boundary/valid_boundary2 in V3. >>> +} After removing all the function ptrs in each structure and removing dp_tu/valid_boundary/valid_boundary2 for dp_catalog_ctrl structure, we can get rid of dp_catalog_ctrl structure. Similarly, even the other aux and panel structures have minimal data that can be removed. If we remove these structures and get read of dp_catalog structure, we have to find a different way to access dp_catalog_private structure present in dp_catalog.c file. Having "static dp_catalog_private *dp_catalog_private_ptr" is acceptable in upstream? Please share if you have any better suggestion? >>> + >>> +static int dp_ctrl_wait4video_ready(struct dp_ctrl_private *ctrl) >>> +{ >>> + int ret = 0; >>> + >>> + ret = wait_for_completion_timeout(&ctrl->video_comp, HZ / 2); >> >> Break out the timeout into a #define > > Fixed in V2. > >> >>> + if (ret <= 0) { >>> + pr_err("Link Train timedout\n"); >>> + ret = -EINVAL; >>> + } >>> + >>> + return ret; >> >> You're not checking the return value at the callsite. >> > Fixed in V2. > >>> +} /snip >>> + >>> +static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel, >>> + u32 mode_edid_bpp, u32 mode_pclk_khz) >>> +{ >>> + struct drm_dp_link *link_info; >>> + const u32 max_supported_bpp = 30, min_supported_bpp = 18; >>> + u32 bpp = 0, data_rate_khz = 0; >>> + >>> + bpp = min_t(u32, mode_edid_bpp, max_supported_bpp); >>> + >>> + link_info = &dp_panel->link_info; >>> + data_rate_khz = link_info->num_lanes * link_info->rate * 8; >>> + >>> + while (bpp > min_supported_bpp) { >>> + if (mode_pclk_khz * bpp <= data_rate_khz) >>> + break; >>> + bpp -= 6; >> >> Watch out for underflow here. >> > Will add check in V3 patch series. > Since min_supported_bpp is 18, bpp can never go below 12. I don't think we need to check underflow here. >>> + } >>> + >>> + return bpp; >>> +} >>> + >>> +static u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel, >>> + u32 mode_edid_bpp, u32 mode_pclk_khz) >>> +{ >>> + struct dp_panel_private *panel; >>> + u32 bpp = mode_edid_bpp; >>> + >>> + if (!dp_panel || !mode_edid_bpp || !mode_pclk_khz) { >>> + pr_err("invalid input\n"); >>> + return 0; >>> + } >>> + >>> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel); >>> + >>> + if (dp_panel->video_test) >>> + bpp = dp_link_bit_depth_to_bpp( >>> + panel->link->test_video.test_bit_depth); >>> + else >>> + bpp = dp_panel_get_supported_bpp(dp_panel, mode_edid_bpp, >>> + mode_pclk_khz); >>> + >>> + return bpp; >>> +} >>> + >>> +static void dp_panel_set_test_mode(struct dp_panel_private *panel, >>> + struct dp_display_mode *mode) >>> +{ >>> + struct dp_panel_info *pinfo = NULL; >> /snip >>> + >>> +struct dp_display_mode { >>> + struct dp_panel_info timing; >>> + u32 capabilities; >>> +}; >>> + >>> +struct dp_panel_in { >>> + struct device *dev; >>> + struct dp_aux *aux; >>> + struct dp_link *link; >>> + struct dp_catalog_panel *catalog; >>> +}; >>> + >>> +struct dp_panel { >>> + /* dpcd raw data */ >>> + u8 dpcd[DP_RECEIVER_CAP_SIZE + 1]; >> >> Why + 1? >> > Will follow up on this in V3. DP_RECIEVER_CAP_SIZE is 0xf. We need 16 bytes for dpcd read. > >>> + u8 ds_ports[DP_MAX_DOWNSTREAM_PORTS]; >>> + >>> + struct drm_dp_link link_info; >>> + struct edid *edid; >>> + struct drm_connector *connector; >>> + struct dp_panel_info pinfo; >>> + bool video_test; >>> + >>> + u32 vic; >>> + u32 max_pclk_khz; >>> + >>> + u32 max_bw_code; >>> + int (*init_info)(struct dp_panel *dp_panel); >>> + int (*deinit)(struct dp_panel *dp_panel); >>> + int (*timing_cfg)(struct dp_panel *dp_panel); >>> + int (*read_sink_caps)(struct dp_panel *dp_panel, >>> + struct drm_connector *connector); >>> + u32 (*get_min_req_link_rate)(struct dp_panel *dp_panel); >> >> This function isn't called anywhere >> > Removed in V2. > /Snip > >> >>> + >>> +static int dp_power_pinctrl_set(struct dp_power_private *power, bool >>> active) >>> +{ >>> + int rc = -EFAULT; >>> + struct pinctrl_state *pin_state; >>> + struct dp_parser *parser; >>> + >>> + parser = power->parser; >>> + >>> + if (IS_ERR_OR_NULL(parser->pinctrl.pin)) >>> + return PTR_ERR(parser->pinctrl.pin); >>> + >>> + pin_state = active ? parser->pinctrl.state_active >>> + : parser->pinctrl.state_suspend; >>> + if (!IS_ERR_OR_NULL(pin_state)) { >> >> How is this not possible? You check these on initialization. >> > Will remove this in V3. GPIO/pin_ctrl entries are optional entries in dtsi for DP since we don't need them for specific target. During init, we allow the probe to go through even though we initialize pin_state or not. So, we have to check this again here. >>> + rc = pinctrl_select_state(parser->pinctrl.pin, >>> + pin_state); >> >> Combine on 1 line >> > Fixed in V2. > >>> + if (rc) >>> + pr_err("can not set %s pins\n", >>> + active ? "dp_active" >>> + : "dp_sleep"); >> >> >> Combine on one line. Also return immediately if this is the case. >> > Fixed in V2. > >>> + } else { >>> + pr_err("invalid '%s' pinstate\n", >>> + active ? "dp_active" >>> + : "dp_sleep"); >>> + } >>> + >>> + return rc; >>> +} >>> + >>> +static int dp_power_clk_init(struct dp_power_private *power, bool >>> enable) >> >> Split this into init/deinit, the paths don't share code. >> > Fixed in V2. > /snip >>> #define DP_TEST_SINK 0x270 >>> # define DP_TEST_SINK_START (1 << 0) >>> +#define DP_TEST_AUDIO_MODE 0x271 >>> +#define DP_TEST_AUDIO_PATTERN_TYPE 0x272 >>> +#define DP_TEST_AUDIO_PERIOD_CH1 0x273 >>> +#define DP_TEST_AUDIO_PERIOD_CH2 0x274 >>> +#define DP_TEST_AUDIO_PERIOD_CH3 0x275 >>> +#define DP_TEST_AUDIO_PERIOD_CH4 0x276 >>> +#define DP_TEST_AUDIO_PERIOD_CH5 0x277 >>> +#define DP_TEST_AUDIO_PERIOD_CH6 0x278 >>> +#define DP_TEST_AUDIO_PERIOD_CH7 0x279 >>> +#define DP_TEST_AUDIO_PERIOD_CH8 0x27A >>> >>> #define DP_FEC_STATUS 0x280 /* 1.4 */ >>> # define DP_FEC_DECODE_EN_DETECTED (1 << 0) >>> -- >>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>> Forum, >>> a Linux Foundation Collaborative Project >>> >>> _______________________________________________ >>> Freedreno mailing list >>> Freedreno@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/freedreno
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