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@xxxxxxxxxxxxxx>
> ---
> 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
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
