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
<snip to dp_debug.c>
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
> new file mode 100644
> index 0000000..5c0a8ce
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -0,0 +1,507 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include <linux/debugfs.h>
> +#include <drm/drm_connector.h>
> +
> +#include "dp_parser.h"
> +#include "dp_power.h"
> +#include "dp_catalog.h"
> +#include "dp_aux.h"
> +#include "dp_ctrl.h"
> +#include "dp_debug.h"
> +#include "dp_display.h"
> +
> +#define DEBUG_NAME "drm_dp"
> +
> +struct dp_debug_private {
> + struct dentry *root;
> +
> + struct dp_usbpd *usbpd;
> + struct dp_link *link;
> + struct dp_panel *panel;
> + struct drm_connector **connector;
> + struct device *dev;
> +
> + struct dp_debug dp_debug;
> +};
> +
> +static ssize_t dp_debug_write_hpd(struct file *file,
> + const char __user *user_buff, size_t count, loff_t *ppos)
> +{
> + struct dp_debug_private *debug = file->private_data;
> + char buf[SZ_8];
> + size_t len = 0;
> + int hpd;
> +
> + if (!debug)
> + return -ENODEV;
debug will not be NULL here.
> +
> + if (*ppos)
> + return 0;
> +
> + /* Leave room for termination char */
> + len = min_t(size_t, count, SZ_8 - 1);
> + if (copy_from_user(buf, user_buff, len))
> + goto end;
> +
> + buf[len] = '\0';
> +
> + if (kstrtoint(buf, 10, &hpd) != 0)
> + goto end;
> +
All of this can be replaced by kstrtoint_from_user().
> + if (debug->usbpd && debug->usbpd->connect)
> + debug->usbpd->connect(debug->usbpd, hpd);
> +
> +end:
> + return -len;
On error you should return a real error code or the number of bytes consumed
(count) on success.
> +}
> +
> +static ssize_t dp_debug_write_edid_modes(struct file *file,
> + const char __user *user_buff, size_t count, loff_t *ppos)
> +{
> + struct dp_debug_private *debug = file->private_data;
> + char buf[SZ_32];
Just use 32.
> + size_t len = 0;
> + int hdisplay = 0, vdisplay = 0, vrefresh = 0;
> +
> + if (!debug)
> + return -ENODEV;
>
Debug can't be NULL here.
> + if (*ppos)
> + goto end;
> +
> + /* Leave room for termination char */
> + len = min_t(size_t, count, SZ_32 - 1);
instead of SZ_23 - 1, use sizeof(buf) - 1
> + if (copy_from_user(buf, user_buff, len))
> + goto clear;
> +
> + buf[len] = '\0';
> +
> + if (sscanf(buf, "%d %d %d", &hdisplay, &vdisplay, &vrefresh) != 3)
> + goto clear;
> +
> + if (!hdisplay || !vdisplay || !vrefresh)
> + goto clear;
> +
> + debug->dp_debug.debug_en = true;
> + debug->dp_debug.hdisplay = hdisplay;
> + debug->dp_debug.vdisplay = vdisplay;
> + debug->dp_debug.vrefresh = vrefresh;
> + goto end;
> +clear:
> + pr_debug("clearing debug modes\n");
> + debug->dp_debug.debug_en = false;
> +end:
> + return len;
This should be 'count'
> +}
> +
> +static ssize_t dp_debug_read_connected(struct file *file,
> + char __user *user_buff, size_t count, loff_t *ppos)
> +{
> + struct dp_debug_private *debug = file->private_data;
> + char buf[SZ_8];
> + u32 len = 0;
> +
> + if (!debug)
> + return -ENODEV;
>
Debug can't be NULL
> + if (*ppos)
> + return 0;
> +
> + if (!debug->usbpd)
> + return -ENODEV;
> +
> + len += snprintf(buf, SZ_8, "%d\n", debug->usbpd->hpd_high);
You should always use scnprintf instead of snprintf in these situations.
And use sizeof(buf) instead of a constant.
And you can set len = snprintf(...). you don't need to do a += and depend on the
initialization.
> +
> + if (copy_to_user(user_buff, buf, len))
> + return -EFAULT;
> +
> + *ppos += len;
> + return len;
> +}
> +
> +static ssize_t dp_debug_read_edid_modes(struct file *file,
> + char __user *user_buff, size_t count, loff_t *ppos)
> +{
> + struct dp_debug_private *debug = file->private_data;
> + char *buf;
> + u32 len = 0;
> + int rc = 0;
> + struct drm_connector *connector;
> + struct drm_display_mode *mode;
> +
> + if (!debug) {
> + pr_err("invalid data\n");
> + rc = -ENODEV;
> + goto error;
> + }
Debug cannot be NULL here.
> +
> + connector = *debug->connector;
Why are you dereferencing debug?
> +
> + if (!connector) {
> + pr_err("connector is NULL\n");
> + rc = -EINVAL;
> + goto error;
Just return -EINVAL;
> + }
I'm not sure if this can be an error or not, but perhaps consider skipping
the print message.
> +
> + if (*ppos)
> + goto error;
Just return 0;
> +
> + buf = kzalloc(SZ_4K, GFP_KERNEL);
> + if (!buf) {
> + rc = -ENOMEM;
> + goto error;
Just return -ENOMEM;
> + }
> +
> + list_for_each_entry(mode, &connector->modes, head) {
> + len += snprintf(buf + len, SZ_4K - len,
Definitely use scnprintf() - this is a disaster waiting to happen/
> + "%s %d %d %d %d %d %d %d %d %d 0x%x\n",
> + mode->name, mode->vrefresh, mode->hdisplay,
> + mode->hsync_start, mode->hsync_end, mode->htotal,
> + mode->vdisplay, mode->vsync_start, mode->vsync_end,
> + mode->vtotal, mode->flags);
> + }
> +
> + if (copy_to_user(user_buff, buf, len)) {
> + kfree(buf);
Just return -EFAULT here
> + rc = -EFAULT;
> + goto error;
> + }
> +
> + *ppos += len;
> + kfree(buf);
> +
> + return len;
> +error:
> + return rc;
This label isn't needed with the above changes.
> +}
> +
> +static int dp_debug_check_buffer_overflow(int rc, int *max_size, int *len)
> +{
> + if (rc >= *max_size) {
> + pr_err("buffer overflow\n");
> + return -EINVAL;
> + }
> + *len += rc;
> + *max_size = SZ_4K - *len;
> +
> + return 0;
> +}
> +
> +static ssize_t dp_debug_read_info(struct file *file, char __user *user_buff,
> + size_t count, loff_t *ppos)
> +{
> + struct dp_debug_private *debug = file->private_data;
> + char *buf;
> + u32 len = 0, rc = 0;
> + u64 lclk = 0;
> + u32 max_size = SZ_4K;
> +
> + if (!debug)
> + return -ENODEV;
Debug cannot be 0.
> +
> + if (*ppos)
> + return 0;
> +
> + buf = kzalloc(SZ_4K, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + rc = snprintf(buf + len, max_size, "\tname = %s\n", DEBUG_NAME);
Use scnprintf()
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
This function is very odd - if you use scnprintf() you can safely just do
len = scnprintf(buf + len, SZ_4K - len, "\ttname = %s\n", DEBUG_NAME);
len += scnprintf(buf + len, SZ_4K - len, ....)
But this whole thing seems like a really good fit for seq_file and then you
don't have to worry about anything else.
> + rc = snprintf(buf + len, max_size,
> + "\tdp_panel\n\t\tmax_pclk_khz = %d\n",
> + debug->panel->max_pclk_khz);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\tdrm_dp_link\n\t\trate = %u\n",
> + debug->panel->link_info.rate);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tnum_lanes = %u\n",
> + debug->panel->link_info.num_lanes);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tcapabilities = %lu\n",
> + debug->panel->link_info.capabilities);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\tdp_panel_info:\n\t\tactive = %dx%d\n",
> + debug->panel->pinfo.h_active,
> + debug->panel->pinfo.v_active);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tback_porch = %dx%d\n",
> + debug->panel->pinfo.h_back_porch,
> + debug->panel->pinfo.v_back_porch);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tfront_porch = %dx%d\n",
> + debug->panel->pinfo.h_front_porch,
> + debug->panel->pinfo.v_front_porch);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tsync_width = %dx%d\n",
> + debug->panel->pinfo.h_sync_width,
> + debug->panel->pinfo.v_sync_width);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tactive_low = %dx%d\n",
> + debug->panel->pinfo.h_active_low,
> + debug->panel->pinfo.v_active_low);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\th_skew = %d\n",
> + debug->panel->pinfo.h_skew);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\trefresh rate = %d\n",
> + debug->panel->pinfo.refresh_rate);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tpixel clock khz = %d\n",
> + debug->panel->pinfo.pixel_clk_khz);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tbpp = %d\n",
> + debug->panel->pinfo.bpp);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + /* Link Information */
> + rc = snprintf(buf + len, max_size,
> + "\tdp_link:\n\t\ttest_requested = %d\n",
> + debug->link->sink_request);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tlane_count = %d\n", debug->link->link_params.lane_count);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tbw_code = %d\n", debug->link->link_params.bw_code);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + lclk = drm_dp_bw_code_to_link_rate(
> + debug->link->link_params.bw_code) * 1000;
> + rc = snprintf(buf + len, max_size,
> + "\t\tlclk = %lld\n", lclk);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tv_level = %d\n", debug->link->phy_params.v_level);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + rc = snprintf(buf + len, max_size,
> + "\t\tp_level = %d\n", debug->link->phy_params.p_level);
> + if (dp_debug_check_buffer_overflow(rc, &max_size, &len))
> + goto error;
> +
> + if (copy_to_user(user_buff, buf, len))
> + goto error;
> +
> + *ppos += len;
> +
> + kfree(buf);
> + return len;
> +error:
> + kfree(buf);
> + return -EINVAL;
> +}
> +
> +static const struct file_operations dp_debug_fops = {
> + .open = simple_open,
> + .read = dp_debug_read_info,
> +};
> +
> +static const struct file_operations edid_modes_fops = {
> + .open = simple_open,
> + .read = dp_debug_read_edid_modes,
> + .write = dp_debug_write_edid_modes,
> +};
> +
> +static const struct file_operations hpd_fops = {
> + .open = simple_open,
> + .write = dp_debug_write_hpd,
> +};
> +
> +static const struct file_operations connected_fops = {
> + .open = simple_open,
> + .read = dp_debug_read_connected,
> +};
> +
> +static int dp_debug_init(struct dp_debug *dp_debug)
> +{
> + int rc = 0;
> + struct dp_debug_private *debug = container_of(dp_debug,
> + struct dp_debug_private, dp_debug);
> + struct dentry *dir, *file, *edid_modes;
> + struct dentry *hpd, *connected;
> +
> + dir = debugfs_create_dir(DEBUG_NAME, NULL);
You don't want to be putting the debugbfs in the root dir - you'll want it
nicely nestled in with the rest of the driver. See the GPU or DPU debugfs files
for clues on how to do that.
> + if (IS_ERR_OR_NULL(dir)) {
debugfs_create_dir only returns NULL on error. Getting the PTR_ERR will just
return 0 in that case.
> + rc = PTR_ERR(dir);
> + pr_err("[%s] debugfs create dir failed, rc = %d\n",
> + DEBUG_NAME, rc);
You don't need DEBUG_NAME in the string - you already have it in the pr_fmt.
> + goto error;
> + }
> +
> + file = debugfs_create_file("dp_debug", 0444, dir,
> + debug, &dp_debug_fops);
> + if (IS_ERR_OR_NULL(file)) {
As above.
> + rc = PTR_ERR(file);
> + pr_err("[%s] debugfs create file failed, rc=%d\n",
> + DEBUG_NAME, rc);
> + goto error_remove_dir;
> + }
> +
> + edid_modes = debugfs_create_file("edid_modes", 0644, dir,
> + debug, &edid_modes_fops);
> + if (IS_ERR_OR_NULL(edid_modes)) {
As above.
> + rc = PTR_ERR(edid_modes);
> + pr_err("[%s] debugfs create edid_modes failed, rc=%d\n",
> + DEBUG_NAME, rc);
> + goto error_remove_dir;
> + }
> +
> + hpd = debugfs_create_file("hpd", 0644, dir,
> + debug, &hpd_fops);
> + if (IS_ERR_OR_NULL(hpd)) {
As above.
> + rc = PTR_ERR(hpd);
> + pr_err("[%s] debugfs hpd failed, rc=%d\n",
> + DEBUG_NAME, rc);
> + goto error_remove_dir;
> + }
> +
> + connected = debugfs_create_file("connected", 0444, dir,
> + debug, &connected_fops);
> + if (IS_ERR_OR_NULL(connected)) {
As above.
> + rc = PTR_ERR(connected);
> + pr_err("[%s] debugfs connected failed, rc=%d\n",
> + DEBUG_NAME, rc);
> + goto error_remove_dir;
> + }
> +
> + debug->root = dir;
> + return rc;
> +error_remove_dir:
> + debugfs_remove(dir);
> +error:
> + return rc;
Note that rc will have to be assigned by some value by you on error because you
won't be getting it from the api functions.
> +}
> +
> +struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
> + struct dp_usbpd *usbpd, struct dp_link *link,
> + struct drm_connector **connector)
> +{
> + int rc = 0;
> + struct dp_debug_private *debug;
> + struct dp_debug *dp_debug;
> +
> + if (!dev || !panel || !usbpd || !link) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
>
I don't think any of these checks are needed - they should all be non-NULL from
the caller.
> + debug = devm_kzalloc(dev, sizeof(*debug), GFP_KERNEL);
> + if (!debug) {
just return ERR_PTR(-ENOMEM);
> + rc = -ENOMEM;
> + goto error;
> + }
> +
> + debug->dp_debug.debug_en = false;
> + debug->usbpd = usbpd;
> + debug->link = link;
> + debug->panel = panel;
> + debug->dev = dev;
> + debug->connector = connector;
> +
> + dp_debug = &debug->dp_debug;
> + dp_debug->vdisplay = 0;
> + dp_debug->hdisplay = 0;
> + dp_debug->vrefresh = 0;
> +
> + rc = dp_debug_init(dp_debug);
> + if (rc) {
> + devm_kfree(dev, debug);
just return ERR_PTR(rc);
> + goto error;
> + }
> +
> + return dp_debug;
> +error:
> + return ERR_PTR(rc);
THis isn't needed
> +}
> +
> +static int dp_debug_deinit(struct dp_debug *dp_debug)
This function doesn't need to return anything - the "error" is uninteresting.
> +{
> + struct dp_debug_private *debug;
> +
> + if (!dp_debug)
> + return -EINVAL;
> +
> + debug = container_of(dp_debug, struct dp_debug_private, dp_debug);
> +
> + debugfs_remove_recursive(debug->root);
> +
> + return 0;
> +}
> +
> +void dp_debug_put(struct dp_debug *dp_debug)
> +{
> + struct dp_debug_private *debug;
> +
> + if (!dp_debug)
> + return;
> +
> + debug = container_of(dp_debug, struct dp_debug_private, dp_debug);
> +
> + dp_debug_deinit(dp_debug);
In fact, dp_debug_deinit doesn't need to exist - you can keep it all inline
and bonus - you don't have to do an unneeded container_of for no reason.
> + devm_kfree(debug->dev, debug);
If you are using devm_ you don't need to free here.
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h b/drivers/gpu/drm/msm/dp/dp_debug.h
> new file mode 100644
> index 0000000..a6480e1
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.h
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#ifndef _DP_DEBUG_H_
> +#define _DP_DEBUG_H_
> +
> +#include "dp_panel.h"
> +#include "dp_link.h"
> +#include "dp_extcon.h"
> +
> +/**
> + * struct dp_debug
> + * @debug_en: specifies whether debug mode enabled
> + * @vdisplay: used to filter out vdisplay value
> + * @hdisplay: used to filter out hdisplay value
> + * @vrefresh: used to filter out vrefresh value
> + * @tpg_state: specifies whether tpg feature is enabled
> + */
> +struct dp_debug {
> + bool debug_en;
> + int aspect_ratio;
> + int vdisplay;
> + int hdisplay;
> + int vrefresh;
> +};
> +
> +#if defined(CONFIG_DEBUG_FS)
> +
> +/**
> + * dp_debug_get() - configure and get the DisplayPlot debug module data
DisplayPlot? DisplayPort perhaps?
> + *
> + * @dev: device instance of the caller
> + * @panel: instance of panel module
> + * @usbpd: instance of usbpd module
> + * @link: instance of link module
> + * @connector: double pointer to display connector
> + * return: pointer to allocated debug module data
> + *
> + * This function sets up the debug module and provides a way
> + * for debugfs input to be communicated with existing modules
> + */
> +struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
> + struct dp_usbpd *usbpd, struct dp_link *link,
> + struct drm_connector **connector);
> +/**
> + * dp_debug_put()
> + *
> + * Cleans up dp_debug instance
> + *
> + * @dp_debug: instance of dp_debug
> + */
> +void dp_debug_put(struct dp_debug *dp_debug);
> +
> +#else
> +
> +static inline
> +struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
> + struct dp_usbpd *usbpd, struct dp_link *link,
> + struct drm_connector **connector)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +
> +static inline void dp_debug_put(struct dp_debug *dp_debug)
> +{
> +}
> +
> +#endif /* defined(CONFIG_DEBUG_FS) */
> +
> +#endif /* _DP_DEBUG_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> new file mode 100644
> index 0000000..8c98399
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -0,0 +1,977 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX of course.
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/debugfs.h>
> +#include <linux/component.h>
> +#include <linux/of_irq.h>
> +
> +#include "msm_drv.h"
> +#include "dp_extcon.h"
> +#include "dp_parser.h"
> +#include "dp_power.h"
> +#include "dp_catalog.h"
> +#include "dp_aux.h"
> +#include "dp_link.h"
> +#include "dp_panel.h"
> +#include "dp_ctrl.h"
> +#include "dp_display.h"
> +#include "dp_drm.h"
> +#include "dp_debug.h"
> +
> +static struct msm_dp *g_dp_display;
> +#define HPD_STRING_SIZE 30
> +
> +struct dp_display_private {
> + char *name;
> + int irq;
> +
> + /* state variables */
> + bool core_initialized;
> + bool power_on;
> + bool hpd_irq_on;
> + bool audio_supported;
> +
> + struct platform_device *pdev;
> + struct dentry *root;
> + struct completion notification_comp;
> +
> + struct dp_usbpd *usbpd;
> + struct dp_parser *parser;
> + struct dp_power *power;
> + struct dp_catalog *catalog;
> + struct dp_aux *aux;
> + struct dp_link *link;
> + struct dp_panel *panel;
> + struct dp_ctrl *ctrl;
> + struct dp_debug *debug;
> +
> + struct dp_usbpd_cb usbpd_cb;
> + struct dp_display_mode mode;
> + struct msm_dp dp_display;
> +
> +};
> +
> +static const struct of_device_id dp_dt_match[] = {
> + {.compatible = "qcom,dp-display"},
> + {}
We like to have a comma after the closing {} for consistency.
> +};
> +
> +static irqreturn_t dp_display_irq(int irq, void *dev_id)
> +{
> + struct dp_display_private *dp = dev_id;
> +
> + if (!dp) {
> + pr_err("invalid data\n");
> + return IRQ_NONE;
> + }
>
dp will never be NULL here
> + /* DP controller isr */
> + dp->ctrl->isr(dp->ctrl);
> +
> + /* DP aux isr */
> + dp->aux->isr(dp->aux);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int dp_display_bind(struct device *dev, struct device *master,
> + void *data)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> + struct drm_device *drm;
> + struct msm_drm_private *priv;
> + struct platform_device *pdev = to_platform_device(dev);
> +
> + if (!dev || !pdev || !master) {
> + pr_err("invalid param(s), dev %pK, pdev %pK, master %pK\n",
> + dev, pdev, master);
> + rc = -EINVAL;
> + goto end;
> + }
>
These will never be NULL coming out of a component bind.
+
> + drm = dev_get_drvdata(master);
> + dp = platform_get_drvdata(pdev);
> + if (!drm || !dp) {
> + pr_err("invalid param(s), drm %pK, dp %pK\n",
> + drm, dp);
> + rc = -EINVAL;
> + goto end;
> + }
These are also very unlikely to be NULL.
> +
> + dp->dp_display.drm_dev = drm;
> + priv = drm->dev_private;
> + priv->dp = &(dp->dp_display);
> +
> + rc = dp->parser->parse(dp->parser);
> + if (rc) {
> + pr_err("device tree parsing failed\n");
> + goto end;
> + }
> +
> + rc = dp->aux->drm_aux_register(dp->aux);
> + if (rc) {
> + pr_err("DRM DP AUX register failed\n");
> + goto end;
> + }
> +
> + rc = dp->power->power_client_init(dp->power);
> + if (rc) {
> + pr_err("Power client create failed\n");
> + goto end;
You don't need this goto here, you can just drop out.
> + }
> +
> +end:
> + return rc;
> +}
> +
> +static void dp_display_unbind(struct device *dev, struct device *master,
> + void *data)
> +{
> + struct dp_display_private *dp;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct drm_device *drm = dev_get_drvdata(master);
> + struct msm_drm_private *priv = drm->dev_private;
> +
> + if (!dev || !pdev) {
> + pr_err("invalid param(s)\n");
> + return;
> + }
> +
> + dp = platform_get_drvdata(pdev);
> + if (!dp) {
> + pr_err("Invalid params\n");
> + return;
> + }
Both these checks are not likely to be NULL.
> +
> + (void)dp->power->power_client_deinit(dp->power);
> + (void)dp->aux->drm_aux_deregister(dp->aux);
If you don't care about the value of either of these then consider changing them
so they don't return a value.
> + priv->dp = NULL;
> +}
> +
> +static const struct component_ops dp_display_comp_ops = {
> + .bind = dp_display_bind,
> + .unbind = dp_display_unbind,
> +};
> +
> +static bool dp_display_is_ds_bridge(struct dp_panel *panel)
> +{
> + return (panel->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> + DP_DWN_STRM_PORT_PRESENT);
> +}
> +
> +static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
> +{
> + return dp_display_is_ds_bridge(dp->panel) &&
> + (dp->link->sink_count.count == 0);
> +}
> +
> +static void dp_display_send_hpd_event(struct msm_dp *dp_display)
> +{
> + struct dp_display_private *dp;
> + struct drm_connector *connector;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + return;
> + }
It looks unlkely this will be NULL, so you can safely remove this or use
WARN_ON.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> + if (!dp) {
> + pr_err("invalid params\n");
> + return;
> + }
The result of a container_of() will NEVER be NULL at least not without a lot of
trouble.
> + connector = dp->dp_display.connector;
> + drm_helper_hpd_irq_event(connector->dev);
> +}
> +
> +static int dp_display_send_hpd_notification(struct dp_display_private *dp,
> + bool hpd)
> +{
> + if ((hpd && dp->dp_display.is_connected) ||
> + (!hpd && !dp->dp_display.is_connected)) {
> + pr_info("HPD already %s\n", (hpd ? "on" : "off"));
> + return 0;
> + }
> +
> + /* reset video pattern flag on disconnect */
> + if (!hpd)
> + dp->panel->video_test = false;
> +
> + dp->dp_display.is_connected = hpd;
> + reinit_completion(&dp->notification_comp);
> + dp_display_send_hpd_event(&dp->dp_display);
> +
> + if (!wait_for_completion_timeout(&dp->notification_comp, HZ * 2)) {
> + pr_warn("%s timeout\n", hpd ? "connect" : "disconnect");
> + return -EINVAL;
Returning -ETIMEDOUT would be more informative.
> + }
> +
> + return 0;
> +}
> +
> +static int dp_display_process_hpd_high(struct dp_display_private *dp)
> +{
> + int rc = 0;
> + struct edid *edid;
> +
> + dp->aux->init(dp->aux, dp->parser->aux_cfg);
> +
> + if (dp->link->psm_enabled)
> + goto notify;
> +
> + rc = dp->panel->read_sink_caps(dp->panel, dp->dp_display.connector);
> + if (rc)
> + goto notify;
> +
> + dp->link->process_request(dp->link);
> +
> + if (dp_display_is_sink_count_zero(dp)) {
> + pr_debug("no downstream devices connected\n");
> + rc = -EINVAL;
You can just return -EINVAL here.
> + goto end;
> + }
> +
> + edid = dp->panel->edid;
> +
> + dp->audio_supported = drm_detect_monitor_audio(edid);
> +
> + dp->panel->handle_sink_request(dp->panel);
> +
> + dp->dp_display.max_pclk_khz = dp->parser->max_pclk_khz;
> +notify:
> + dp_display_send_hpd_notification(dp, true);
> +
> +end:
> + return rc;
Not needed.
> +}
> +
> +static void dp_display_host_init(struct dp_display_private *dp)
> +{
> + bool flip = false;
> +
> + if (dp->core_initialized) {
> + pr_debug("DP core already initialized\n");
> + return;
> + }
> +
> + if (dp->usbpd->orientation == ORIENTATION_CC2)
> + flip = true;
> +
> + dp->power->init(dp->power, flip);
> + dp->ctrl->init(dp->ctrl, flip);
> + enable_irq(dp->irq);
> + dp->core_initialized = true;
> +}
> +
> +static void dp_display_host_deinit(struct dp_display_private *dp)
> +{
> + if (!dp->core_initialized) {
> + pr_debug("DP core already off\n");
> + return;
> + }
> +
> + dp->ctrl->deinit(dp->ctrl);
> + dp->power->deinit(dp->power);
> + disable_irq(dp->irq);
> + dp->core_initialized = false;
> +}
> +
> +static void dp_display_process_hpd_low(struct dp_display_private *dp)
> +{
> + /* cancel any pending request */
> + dp->ctrl->abort(dp->ctrl);
> +
> + dp_display_send_hpd_notification(dp, false);
> +
> + dp->aux->deinit(dp->aux);
> +}
> +
> +static int dp_display_usbpd_configure_cb(struct device *dev)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dev) {
> + pr_err("invalid dev\n");
> + rc = -EINVAL;
> + goto end;
> + }
>
I'm not sure if this ever be true (I suspect it can't) but if so you can use the
WARN_ON method.
> + dp = dev_get_drvdata(dev);
> + if (!dp) {
> + pr_err("no driver data found\n");
> + rc = -ENODEV;
> + goto end;
> + }
I'm reasonably sure that if dev is valid, this will be valid.
> + dp_display_host_init(dp);
> +
> + if (dp->usbpd->hpd_high)
> + dp_display_process_hpd_high(dp);
> +end:
> + return rc;
> +}
> +
> +static void dp_display_clean(struct dp_display_private *dp)
> +{
> + dp->ctrl->push_idle(dp->ctrl);
> + dp->ctrl->off(dp->ctrl);
> +}
> +
> +static int dp_display_usbpd_disconnect_cb(struct device *dev)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dev) {
> + pr_err("invalid dev\n");
> + rc = -EINVAL;
> + goto end;
> + }
> +
> + dp = dev_get_drvdata(dev);
> + if (!dp) {
> + pr_err("no driver data found\n");
> + rc = -ENODEV;
> + goto end;
> + }
As above
> + /* cancel any pending request */
> + dp->ctrl->abort(dp->ctrl);
> +
> + rc = dp_display_send_hpd_notification(dp, false);
> +
> + /* if cable is disconnected, reset psm_enabled flag */
> + if (!dp->usbpd->alt_mode_cfg_done)
> + dp->link->psm_enabled = false;
> +
> + if ((rc < 0) && dp->power_on)
> + dp_display_clean(dp);
> +
> + dp_display_host_deinit(dp);
> +end:
> + return rc;
> +}
> +
> +static void dp_display_handle_video_request(struct dp_display_private *dp)
> +{
> + if (dp->link->sink_request & DP_TEST_LINK_VIDEO_PATTERN) {
> + /* force disconnect followed by connect */
> + dp->usbpd->connect(dp->usbpd, false);
> + dp->panel->video_test = true;
> + dp->usbpd->connect(dp->usbpd, true);
> + dp->link->send_test_response(dp->link);
> + }
> +}
> +
> +static int dp_display_handle_hpd_irq(struct dp_display_private *dp)
> +{
> + if (dp->link->sink_request & DS_PORT_STATUS_CHANGED) {
> + dp_display_send_hpd_notification(dp, false);
> +
> + if (dp_display_is_sink_count_zero(dp)) {
> + pr_debug("sink count is zero, nothing to do\n");
> + return 0;
> + }
> +
> + return dp_display_process_hpd_high(dp);
> + }
> +
> + dp->ctrl->handle_sink_request(dp->ctrl);
> +
> + dp_display_handle_video_request(dp);
> +
> + return 0;
> +}
> +
> +static int dp_display_usbpd_attention_cb(struct device *dev)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dev) {
> + pr_err("invalid dev\n");
> + return -EINVAL;
> + }
> +
> + dp = dev_get_drvdata(dev);
> + if (!dp) {
> + pr_err("no driver data found\n");
> + return -ENODEV;
> + }
As above.
> + if (dp->usbpd->hpd_irq) {
> + dp->hpd_irq_on = true;
> +
> + rc = dp->link->process_request(dp->link);
> + /* check for any test request issued by sink */
> + if (!rc)
> + dp_display_handle_hpd_irq(dp);
> +
> + dp->hpd_irq_on = false;
Just return rc;
> + goto end;
> + }
> +
> + if (!dp->usbpd->hpd_high) {
> + dp_display_process_hpd_low(dp);
> + goto end;
Just return rc;
> + }
> +
> + if (dp->usbpd->alt_mode_cfg_done)
> + dp_display_process_hpd_high(dp);
> +end:
> + return rc;
return 0;
> +}
> +
> +static void dp_display_deinit_sub_modules(struct dp_display_private *dp)
> +{
> + dp_ctrl_put(dp->ctrl);
> + dp_link_put(dp->link);
> + dp_panel_put(dp->panel);
> + dp_aux_put(dp->aux);
> + dp_power_put(dp->power);
> + dp_catalog_put(dp->catalog);
> + dp_parser_put(dp->parser);
> + dp_debug_put(dp->debug);
> +}
> +
> +static int dp_init_sub_modules(struct dp_display_private *dp)
> +{
> + int rc = 0;
> + struct device *dev = &dp->pdev->dev;
> + struct dp_usbpd_cb *cb = &dp->usbpd_cb;
> + struct dp_ctrl_in ctrl_in = {
> + .dev = dev,
> + };
> + struct dp_panel_in panel_in = {
> + .dev = dev,
> + };
> +
> + /* Callback APIs used for cable status change event */
> + cb->configure = dp_display_usbpd_configure_cb;
> + cb->disconnect = dp_display_usbpd_disconnect_cb;
> + cb->attention = dp_display_usbpd_attention_cb;
> +
> + dp->parser = dp_parser_get(dp->pdev);
> + if (IS_ERR(dp->parser)) {
> + rc = PTR_ERR(dp->parser);
> + pr_err("failed to initialize parser, rc = %d\n", rc);
> + dp->parser = NULL;
> + goto error_parser;
> + }
> +
> + dp->catalog = dp_catalog_get(dev, &dp->parser->io);
> + if (IS_ERR(dp->catalog)) {
> + rc = PTR_ERR(dp->catalog);
> + pr_err("failed to initialize catalog, rc = %d\n", rc);
> + dp->catalog = NULL;
> + goto error_catalog;
> + }
> +
> + dp->power = dp_power_get(dp->parser);
> + if (IS_ERR(dp->power)) {
> + rc = PTR_ERR(dp->power);
> + pr_err("failed to initialize power, rc = %d\n", rc);
> + dp->power = NULL;
> + goto error_power;
> + }
> +
> + dp->aux = dp_aux_get(dev, &dp->catalog->aux, dp->parser->aux_cfg);
> + if (IS_ERR(dp->aux)) {
> + rc = PTR_ERR(dp->aux);
> + pr_err("failed to initialize aux, rc = %d\n", rc);
> + dp->aux = NULL;
> + goto error_aux;
> + }
> +
> + dp->link = dp_link_get(dev, dp->aux);
> + if (IS_ERR(dp->link)) {
> + rc = PTR_ERR(dp->link);
> + pr_err("failed to initialize link, rc = %d\n", rc);
> + dp->link = NULL;
> + goto error_link;
> + }
> +
> + panel_in.aux = dp->aux;
> + panel_in.catalog = &dp->catalog->panel;
> + panel_in.link = dp->link;
> +
> + dp->panel = dp_panel_get(&panel_in);
> + if (IS_ERR(dp->panel)) {
> + rc = PTR_ERR(dp->panel);
> + pr_err("failed to initialize panel, rc = %d\n", rc);
> + dp->panel = NULL;
> + goto error_panel;
> + }
> +
> + ctrl_in.link = dp->link;
> + ctrl_in.panel = dp->panel;
> + ctrl_in.aux = dp->aux;
> + ctrl_in.power = dp->power;
> + ctrl_in.catalog = &dp->catalog->ctrl;
> + ctrl_in.parser = dp->parser;
> +
> + dp->ctrl = dp_ctrl_get(&ctrl_in);
> + if (IS_ERR(dp->ctrl)) {
> + rc = PTR_ERR(dp->ctrl);
> + pr_err("failed to initialize ctrl, rc = %d\n", rc);
> + dp->ctrl = NULL;
> + goto error_ctrl;
> + }
> +
> + dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
> + dp->link, &dp->dp_display.connector);
> + if (IS_ERR(dp->debug)) {
> + rc = PTR_ERR(dp->debug);
> + pr_err("failed to initialize debug, rc = %d\n", rc);
dp_debug_get() prints all the interesting error messsages - you don't need to
add on.
In fact, I think this is probably true for the rest of the dp_ calls above - in
general, try to keep the number of log messages per error down to 1.
> + dp->debug = NULL;
> + goto error_debug;
> + }
> +
> + return rc;
> +error_debug:
> + dp_ctrl_put(dp->ctrl);
> +error_ctrl:
> + dp_panel_put(dp->panel);
> +error_panel:
> + dp_link_put(dp->link);
> +error_link:
> + dp_aux_put(dp->aux);
> +error_aux:
> + dp_power_put(dp->power);
> +error_power:
> + dp_catalog_put(dp->catalog);
> +error_catalog:
> + dp_parser_put(dp->parser);
> +error_parser:
> + return rc;
> +}
> +
> +static int dp_display_set_mode(struct msm_dp *dp_display,
> + struct dp_display_mode *mode)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
I'm betting this can't be NULL;
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + dp->panel->pinfo = mode->timing;
> + dp->panel->init_info(dp->panel);
> +error:
> + return rc;
> +}
> +
> +static int dp_display_prepare(struct msm_dp *dp)
> +{
> + return 0;
> +}
> +
> +static int dp_display_enable(struct msm_dp *dp_display)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
I am guessing this can't be NULL.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + if (dp->power_on) {
> + pr_debug("Link already setup, return\n");
> + return 0;
> + }
> +
> + dp->aux->init(dp->aux, dp->parser->aux_cfg);
> +
> + rc = dp->ctrl->on(dp->ctrl);
> + if (!rc)
> + dp->power_on = true;
> +error:
> + return rc;
> +}
> +
> +static int dp_display_post_enable(struct msm_dp *dp_display)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto end;
> + }
>
As above.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + complete_all(&dp->notification_comp);
> +
> +end:
> + return rc;
> +}
> +
> +static int dp_display_pre_disable(struct msm_dp *dp_display)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
> +
as above.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + if (dp->usbpd->alt_mode_cfg_done && (dp->usbpd->hpd_high ||
> + dp->usbpd->forced_disconnect))
> + dp->link->psm_config(dp->link, &dp->panel->link_info, true);
> +
> + dp->ctrl->push_idle(dp->ctrl);
> +error:
> + return rc;
> +}
> +
> +static int dp_display_disable(struct msm_dp *dp_display)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
>
As above.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + if (!dp->power_on || !dp->core_initialized)
> + goto error;
goto error is deceptive here. You can just return 0. In fact, if you get
rid of the uneeded NULL check you don't have to return anything from this
function.
> +
> + dp->ctrl->off(dp->ctrl);
> +
> + dp->power_on = false;
> +
> + complete_all(&dp->notification_comp);
> +error:
> + return rc;
> +}
> +
> +static int dp_request_irq(struct msm_dp *dp_display)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
Likely not needed.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> + if (dp->irq < 0) {
> + rc = dp->irq;
> + pr_err("failed to get irq: %d\n", rc);
> + return rc;
> + }
> +
> + rc = devm_request_irq(&dp->pdev->dev, dp->irq, dp_display_irq,
> + IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> + if (rc < 0) {
> + pr_err("failed to request IRQ%u: %d\n",
> + dp->irq, rc);
> + return rc;
> + }
> + disable_irq(dp->irq);
> +
> + return 0;
> +}
> +
> +static struct dp_debug *dp_get_debug(struct msm_dp *dp_display)
> +{
> + struct dp_display_private *dp;
> +
> + if (!dp_display) {
> + pr_err("invalid input\n");
> + return ERR_PTR(-EINVAL);
> + }
Likely not needed.
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + return dp->debug;
> +}
> +
> +static int dp_display_unprepare(struct msm_dp *dp)
> +{
> + return 0;
> +}
> +
> +static int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz)
> +{
> + const u32 num_components = 3, default_bpp = 24;
> + struct dp_display_private *dp_display;
> + struct drm_dp_link *link_info;
> + u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
> +
> + if (!dp || !mode_pclk_khz || !dp->connector) {
> + pr_err("invalid params\n");
> + return -EINVAL;
> + }
Same as above.
> + dp_display = container_of(dp, struct dp_display_private, dp_display);
> + link_info = &dp_display->panel->link_info;
> +
> + mode_bpp = dp->connector->display_info.bpc * num_components;
> + if (!mode_bpp)
> + mode_bpp = default_bpp;
> +
> + mode_bpp = dp_display->panel->get_mode_bpp(dp_display->panel,
> + mode_bpp, mode_pclk_khz);
> +
> + mode_rate_khz = mode_pclk_khz * mode_bpp;
> + supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
> +
> + if (mode_rate_khz > supported_rate_khz)
> + return MODE_BAD;
> +
> + return MODE_OK;
> +}
> +
> +static int dp_display_get_modes(struct msm_dp *dp,
> + struct dp_display_mode *dp_mode)
> +{
> + struct dp_display_private *dp_display;
> + int ret = 0;
> +
> + if (!dp) {
> + pr_err("invalid params\n");
> + return 0;
> + }
Likely not needed.
> + dp_display = container_of(dp, struct dp_display_private, dp_display);
> +
> + ret = dp_display->panel->get_modes(dp_display->panel,
> + dp->connector, dp_mode);
> + if (dp_mode->timing.pixel_clk_khz)
> + dp->max_pclk_khz = dp_mode->timing.pixel_clk_khz;
> + return ret;
> +}
> +
> +static bool dp_display_check_video_test(struct msm_dp *dp)
> +{
> + struct dp_display_private *dp_display;
> +
> + if (!dp) {
> + pr_err("invalid params\n");
> + return false;
> + }
Likely not needed
> + dp_display = container_of(dp, struct dp_display_private, dp_display);
> +
> + if (dp_display->panel->video_test)
> + return true;
> +
> + return false;
> +}
> +
> +static int dp_display_get_test_bpp(struct msm_dp *dp)
> +{
> + struct dp_display_private *dp_display;
> +
> + if (!dp) {
> + pr_err("invalid params\n");
> + return 0;
> + }
Likely not needed.
> + dp_display = container_of(dp, struct dp_display_private, dp_display);
> +
> + return dp_link_bit_depth_to_bpp(
> + dp_display->link->test_video.test_bit_depth);
> +}
> +
> +static int dp_display_probe(struct platform_device *pdev)
> +{
> + int rc = 0;
> + struct dp_display_private *dp;
> +
> + if (!pdev || !pdev->dev.of_node) {
> + pr_err("pdev not found\n");
> + return -ENODEV;
> + }
Both of these will always be true in a probe function that you reach from a
compatible match.
> + dp = devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
> + if (!dp)
> + return -ENOMEM;
> +
> + init_completion(&dp->notification_comp);
> +
> + dp->pdev = pdev;
> + dp->name = "drm_dp";
> +
> + rc = dp_init_sub_modules(dp);
> + if (rc) {
> + pr_err("init sub module failed\n");
I'm guessing that this log message will always be superfluous.
> + devm_kfree(&pdev->dev, dp);
> + return -EPROBE_DEFER;
Always return -EPROBE_DEFER regardless of the error? It seems that at a certain
point many of these errors you point out would be fatal to the device regardless
of the order of initialization.
> + }
> +
> + platform_set_drvdata(pdev, dp);
> +
> + g_dp_display = &dp->dp_display;
> +
> + g_dp_display->enable = dp_display_enable;
> + g_dp_display->post_enable = dp_display_post_enable;
> + g_dp_display->pre_disable = dp_display_pre_disable;
> + g_dp_display->disable = dp_display_disable;
> + g_dp_display->set_mode = dp_display_set_mode;
> + g_dp_display->validate_mode = dp_display_validate_mode;
> + g_dp_display->get_modes = dp_display_get_modes;
> + g_dp_display->prepare = dp_display_prepare;
> + g_dp_display->unprepare = dp_display_unprepare;
> + g_dp_display->request_irq = dp_request_irq;
> + g_dp_display->get_debug = dp_get_debug;
> + g_dp_display->send_hpd_event = dp_display_send_hpd_event;
> + g_dp_display->is_video_test = dp_display_check_video_test;
> + g_dp_display->get_test_bpp = dp_display_get_test_bpp;
> +
> + rc = component_add(&pdev->dev, &dp_display_comp_ops);
> + if (rc) {
> + pr_err("component add failed, rc=%d\n", rc);
> + dp_display_deinit_sub_modules(dp);
> + devm_kfree(&pdev->dev, dp);
> + }
> +
> + return rc;
> +}
> +
> +int dp_display_get_displays(void **displays, int count)
> +{
> + if (!displays) {
> + pr_err("invalid data\n");
> + return -EINVAL;
> + }
> +
> + if (count != 1) {
> + pr_err("invalid number of displays\n");
> + return -EINVAL;
> + }
> +
> + displays[0] = g_dp_display;
> + return count;
> +}
This isn't used in this patch so I don't see how it is used but based on
everything it seems like you could just as easily get away with
struct msm_dp *dp_display_get_display(void)
{
return g_dp_display;
}
Even if you are planning for the future, you can make the changes to support
multiple displays then and save yourself the effort now.
> +int dp_display_get_num_of_displays(void)
> +{
> + return 1;
> +}
> +
> +static int dp_display_remove(struct platform_device *pdev)
> +{
> + struct dp_display_private *dp;
> +
> + if (!pdev)
> + return -EINVAL;
pdev will never be NULL here.
> +
> + dp = platform_get_drvdata(pdev);
> +
> + dp_display_deinit_sub_modules(dp);
> +
> + platform_set_drvdata(pdev, NULL);
> + devm_kfree(&pdev->dev, dp);
You don't need to devm_kfree() in a remove function here.
> +
> + return 0;
> +}
> +
> +static struct platform_driver dp_display_driver = {
> + .probe = dp_display_probe,
> + .remove = dp_display_remove,
> + .driver = {
> + .name = "msm-dp-display",
> + .of_match_table = dp_dt_match,
> + },
> +};
> +
> +int __init msm_dp_register(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&dp_display_driver);
> + if (ret) {
> + pr_err("driver register failed");
> + return ret;
You don't need this return ret - you can just drop out to the bottom.
> + }
> +
> + return ret;
> +}
> +
> +void __exit msm_dp_unregister(void)
> +{
> + platform_driver_unregister(&dp_display_driver);
> +}
> +
> +int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> + struct drm_encoder *encoder)
> +{
> + struct msm_drm_private *priv;
> + int ret;
> +
> + if (WARN_ON(!encoder) || WARN_ON(!dp_display) || WARN_ON(!dev))
> + return -EINVAL;
Thats what I like to see - but you could also do
if (WARN_ON(!encoder || !dp_display || !dev))
>
> + priv = dev->dev_private;
> + dp_display->drm_dev = dev;
> +
> + ret = dp_drm_bridge_init(dp_display, encoder);
> + if (ret) {
> + dev_err(dev->dev, "failed to create dp bridge: %d\n", ret);
Just double check to see how redundant this error message is.
> + dp_display->bridge = NULL;
> + return ret;
> + }
> + dp_display->encoder = encoder;
> +
> + dp_display->connector = dp_drm_connector_init(dp_display);
> + if (IS_ERR(dp_display->connector)) {
> + ret = PTR_ERR(dp_display->connector);
> + dev_err(dev->dev,
> + "failed to create dp connector: %d\n", ret);
Same.
> + dp_display->connector = NULL;
> + goto conn_fail;
> + }
> +
> + priv->bridges[priv->num_bridges++] = dp_display->bridge;
> + priv->connectors[priv->num_connectors++] = dp_display->connector;
> + return 0;
> +
> +conn_fail:
> + if (dp_display->bridge) {
> + dp_drm_bridge_deinit(dp_display);
> + dp_display->bridge = NULL;
> + }
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> new file mode 100644
> index 0000000..ca5eae5
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#ifndef _DP_DISPLAY_H_
> +#define _DP_DISPLAY_H_
> +
> +#include <drm/drmP.h>
> +
> +#include "dp_panel.h"
> +
> +struct msm_dp {
> + struct drm_device *drm_dev;
> + struct dp_bridge *dp_bridge;
> + struct drm_connector *connector;
> + struct drm_bridge *bridge;
> +
> + /* the encoder we are hooked to (outside of dsi block) */
> + struct drm_encoder *encoder;
> + bool is_connected;
> + u32 max_pclk_khz;
> +
> + int (*enable)(struct msm_dp *dp_display);
> + int (*post_enable)(struct msm_dp *dp_display);
> +
> + int (*pre_disable)(struct msm_dp *dp_display);
> + int (*disable)(struct msm_dp *dp_display);
> +
> + int (*set_mode)(struct msm_dp *dp_display,
> + struct dp_display_mode *mode);
> + int (*validate_mode)(struct msm_dp *dp_display, u32 mode_pclk_khz);
> + int (*get_modes)(struct msm_dp *dp_display,
> + struct dp_display_mode *dp_mode);
> + int (*prepare)(struct msm_dp *dp_display);
> + int (*unprepare)(struct msm_dp *dp_display);
> + int (*request_irq)(struct msm_dp *dp_display);
> + struct dp_debug *(*get_debug)(struct msm_dp *dp_display);
> + void (*send_hpd_event)(struct msm_dp *dp_display);
> + bool (*is_video_test)(struct msm_dp *dp_display);
> + int (*get_test_bpp)(struct msm_dp *dp_display);
> +};
> +
> +int dp_display_get_num_of_displays(void);
> +int dp_display_get_displays(void **displays, int count);
> +#endif /* _DP_DISPLAY_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> new file mode 100644
> index 0000000..64d6449
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -0,0 +1,542 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX
> +#define pr_fmt(fmt) "[drm-dp]: %s: " fmt, __func__
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_crtc.h>
> +
> +#include "msm_drv.h"
> +#include "msm_kms.h"
> +//#include "dpu_connector.h"
If this isn't needed just get rid of it
> +#include "dp_drm.h"
> +#include "dp_debug.h"
> +
> +struct dp_connector {
> + struct drm_connector base;
> + struct msm_dp *dp_display;
> +};
> +#define to_dp_connector(x) container_of(x, struct dp_connector, base)
> +#define to_dp_bridge(x) container_of((x), struct dp_bridge, base)
> +
> +static void convert_to_dp_mode(const struct drm_display_mode *drm_mode,
> + struct dp_display_mode *dp_mode, struct msm_dp *dp)
> +{
> + const u32 num_components = 3;
I prefer to use just the constants inline, especially since you are only using
once.
> +
> + memset(dp_mode, 0, sizeof(*dp_mode));
> +
> + dp_mode->timing.h_active = drm_mode->hdisplay;
> + dp_mode->timing.h_back_porch = drm_mode->htotal - drm_mode->hsync_end;
> + dp_mode->timing.h_sync_width = drm_mode->htotal -
> + (drm_mode->hsync_start + dp_mode->timing.h_back_porch);
> + dp_mode->timing.h_front_porch = drm_mode->hsync_start -
> + drm_mode->hdisplay;
> + dp_mode->timing.h_skew = drm_mode->hskew;
> +
> + dp_mode->timing.v_active = drm_mode->vdisplay;
> + dp_mode->timing.v_back_porch = drm_mode->vtotal - drm_mode->vsync_end;
> + dp_mode->timing.v_sync_width = drm_mode->vtotal -
> + (drm_mode->vsync_start + dp_mode->timing.v_back_porch);
> +
> + dp_mode->timing.v_front_porch = drm_mode->vsync_start -
> + drm_mode->vdisplay;
> +
> + if (dp->is_video_test(dp))
> + dp_mode->timing.bpp = dp->get_test_bpp(dp);
> + else
> + dp_mode->timing.bpp = dp->connector->display_info.bpc *
> + num_components;
> +
> + if (!dp_mode->timing.bpp)
> + dp_mode->timing.bpp = 24;
> +
> + dp_mode->timing.refresh_rate = drm_mode->vrefresh;
> +
> + dp_mode->timing.pixel_clk_khz = drm_mode->clock;
> +
> + dp_mode->timing.v_active_low =
> + !!(drm_mode->flags & DRM_MODE_FLAG_NVSYNC);
> +
> + dp_mode->timing.h_active_low =
> + !!(drm_mode->flags & DRM_MODE_FLAG_NHSYNC);
> +}
> +
> +static void convert_to_drm_mode(const struct dp_display_mode *dp_mode,
> + struct drm_display_mode *drm_mode)
> +{
> + u32 flags = 0;
> +
> + memset(drm_mode, 0, sizeof(*drm_mode));
> +
> + drm_mode->hdisplay = dp_mode->timing.h_active;
> + drm_mode->hsync_start = drm_mode->hdisplay +
> + dp_mode->timing.h_front_porch;
> + drm_mode->hsync_end = drm_mode->hsync_start +
> + dp_mode->timing.h_sync_width;
> + drm_mode->htotal = drm_mode->hsync_end + dp_mode->timing.h_back_porch;
> + drm_mode->hskew = dp_mode->timing.h_skew;
> +
> + drm_mode->vdisplay = dp_mode->timing.v_active;
> + drm_mode->vsync_start = drm_mode->vdisplay +
> + dp_mode->timing.v_front_porch;
> + drm_mode->vsync_end = drm_mode->vsync_start +
> + dp_mode->timing.v_sync_width;
> + drm_mode->vtotal = drm_mode->vsync_end + dp_mode->timing.v_back_porch;
> +
> + drm_mode->vrefresh = dp_mode->timing.refresh_rate;
> + drm_mode->clock = dp_mode->timing.pixel_clk_khz;
> +
> + if (dp_mode->timing.h_active_low)
> + flags |= DRM_MODE_FLAG_NHSYNC;
> + else
> + flags |= DRM_MODE_FLAG_PHSYNC;
> +
> + if (dp_mode->timing.v_active_low)
> + flags |= DRM_MODE_FLAG_NVSYNC;
> + else
> + flags |= DRM_MODE_FLAG_PVSYNC;
> +
> + drm_mode->flags = flags;
> +
> + drm_mode->type = 0x48;
> + drm_mode_set_name(drm_mode);
> +}
> +
> +static void dp_bridge_pre_enable(struct drm_bridge *drm_bridge)
> +{
> + int rc = 0;
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge) {
> + pr_err("Invalid params\n");
> + return;
> + }
bridge will always be valid.
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
You can move these two up to the declaration and save yourself two more lines
per function here and throughout.
> +
> + /* By this point mode should have been validated through mode_fixup */
> + rc = dp->set_mode(dp, &bridge->dp_mode);
> + if (rc) {
> + pr_err("[%d] failed to perform a mode set, rc=%d\n",
> + bridge->id, rc);
> + return;
> + }
> +
> + rc = dp->prepare(dp);
> + if (rc) {
> + pr_err("[%d] DP display prepare failed, rc=%d\n",
> + bridge->id, rc);
> + return;
> + }
> +
> + rc = dp->enable(dp);
> + if (rc) {
> + pr_err("[%d] DP display enable failed, rc=%d\n",
> + bridge->id, rc);
> + dp->unprepare(dp);
> + }
> +}
> +
> +static void dp_bridge_enable(struct drm_bridge *drm_bridge)
> +{
> + int rc = 0;
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge) {
> + pr_err("Invalid params\n");
> + return;
> + }
bridge will always be valid.
> +
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
> +
> + rc = dp->post_enable(dp);
> + if (rc)
> + pr_err("[%d] DP display post enable failed, rc=%d\n",
> + bridge->id, rc);
> +}
> +
> +static void dp_bridge_disable(struct drm_bridge *drm_bridge)
> +{
> + int rc = 0;
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge) {
> + pr_err("Invalid params\n");
> + return;
> + }
bridge will always be valid.
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
> +
> + rc = dp->pre_disable(dp);
> + if (rc) {
> + pr_err("[%d] DP display pre disable failed, rc=%d\n",
> + bridge->id, rc);
> + }
> +}
> +
> +static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
> +{
> + int rc = 0;
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge) {
> + pr_err("Invalid params\n");
> + return;
> + }
bridge will always be valid.
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
> +
> + rc = dp->disable(dp);
> + if (rc) {
> + pr_err("[%d] DP display disable failed, rc=%d\n",
> + bridge->id, rc);
> + return;
> + }
> +
> + rc = dp->unprepare(dp);
> + if (rc) {
> + pr_err("[%d] DP display unprepare failed, rc=%d\n",
> + bridge->id, rc);
> + return;
> + }
> +}
> +
> +static void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> +{
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge || !mode || !adjusted_mode) {
> + pr_err("Invalid params\n");
> + return;
> + }
bridge will always be valid. Not sure about mode or adjusted_mode, maybe you
should use WARN_ON for those.
> +
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
> +
> + memset(&bridge->dp_mode, 0x0, sizeof(struct dp_display_mode));
> + convert_to_dp_mode(adjusted_mode, &bridge->dp_mode, dp);
> +}
> +
> +static bool dp_bridge_mode_fixup(struct drm_bridge *drm_bridge,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> +{
> + bool ret = true;
> + struct dp_display_mode dp_mode;
> + struct dp_bridge *bridge;
> + struct msm_dp *dp;
> +
> + if (!drm_bridge || !mode || !adjusted_mode) {
> + pr_err("Invalid params\n");
> + ret = false;
> + goto end;
> + }
>
drm_bridge will always be valid, not positive about the other two.
> + bridge = to_dp_bridge(drm_bridge);
> + dp = bridge->display;
> +
> + convert_to_dp_mode(mode, &dp_mode, dp);
> + convert_to_drm_mode(&dp_mode, adjusted_mode);
> +end:
> + return ret;
> +}
> +
> +static const struct drm_bridge_funcs dp_bridge_ops = {
> + .mode_fixup = dp_bridge_mode_fixup,
> + .pre_enable = dp_bridge_pre_enable,
> + .enable = dp_bridge_enable,
> + .disable = dp_bridge_disable,
> + .post_disable = dp_bridge_post_disable,
> + .mode_set = dp_bridge_mode_set,
> +};
> +
> +int dp_connector_post_init(struct drm_connector *connector, void *display)
> +{
> + struct msm_dp *dp_display = display;
> +
> + if (!dp_display)
> + return -EINVAL;
> +
> + dp_display->connector = connector;
> + return 0;
> +}
> +
> +/**
> + * dp_connector_detect - callback to determine if connector is connected
> + * @connector: Pointer to drm connector structure
> + * @force: Force detect setting from drm framework
> + * Returns: Connector 'is connected' status
> + */
> +static enum drm_connector_status dp_connector_detect(struct drm_connector *conn,
> + bool force)
> +{
> + enum drm_connector_status status = connector_status_unknown;
> + static enum drm_connector_status current_status = connector_status_unknown;
> + struct msm_dp *dp;
> + struct dp_connector *dp_conn;
> + struct msm_drm_private *priv = conn->dev->dev_private;
> + struct msm_kms *kms = priv->kms;
> +
> + dp_conn = to_dp_connector(conn);
> + dp = dp_conn->dp_display;
> +
> + pr_err("is_connected = %s\n",
> + (dp->is_connected) ? "true" : "false");
Oops, this looks like a debug message.
> +
> + status = (dp->is_connected) ? connector_status_connected :
> + connector_status_disconnected;
> +
> + if (dp->is_connected && dp->bridge->encoder
> + && (current_status != status)
> + && kms->funcs->set_encoder_mode) {
> + kms->funcs->set_encoder_mode(kms,
> + dp->bridge->encoder, false);
> + pr_err("call set_encoder_mode\n");
This too.
> + }
> + current_status = status;
> + return status;
> +}
> +
> +static void dp_connector_destroy(struct drm_connector *connector)
> +{
> + struct dp_connector *dp_conn = to_dp_connector(connector);
> +
> + DBG("");
> +
> + drm_connector_cleanup(connector);
> +
> + kfree(dp_conn);
> +}
> +
> +void dp_connector_send_hpd_event(void *display)
> +{
> + struct msm_dp *dp;
> +
> + if (!display) {
> + pr_err("invalid input\n");
Even if this is invalid, you don't need a error message. A WARN_ON if you must.
> + return;
> + }
> +
> + dp = display;
> +
> + if (dp->send_hpd_event)
> + dp->send_hpd_event(dp);
> +}
> +
> +/**
> + * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add()
> + * @connector: Pointer to drm connector structure
> + * Returns: Number of modes added
> + */
> +static int dp_connector_get_modes(struct drm_connector *connector)
> +{
> + int rc = 0;
> + struct msm_dp *dp;
> + struct dp_display_mode *dp_mode = NULL;
> + struct drm_display_mode *m, drm_mode;
> + struct dp_connector *dp_conn;
> +
> + if (!connector)
> + return 0;
>
Connector can never be NULL here.
+
> + dp_conn = to_dp_connector(connector);
> + dp = dp_conn->dp_display;
You can move these to the declaration.
> + dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
> + if (!dp_mode)
> + return 0;
Return 0 on -ENOMEM?
> +
> + /* pluggable case assumes EDID is read when HPD */
> + if (dp->is_connected) {
> + rc = dp->get_modes(dp, dp_mode);
> + if (!rc)
> + pr_err("failed to get DP sink modes, rc=%d\n", rc);
> +
Do you wnat to keep going here even if the get_modes failed?
> + if (dp_mode->timing.pixel_clk_khz) { /* valid DP mode */
> + memset(&drm_mode, 0x0, sizeof(drm_mode));
> + convert_to_drm_mode(dp_mode, &drm_mode);
> + m = drm_mode_duplicate(connector->dev, &drm_mode);
> + if (!m) {
> + pr_err("failed to add mode %ux%u\n",
> + drm_mode.hdisplay,
> + drm_mode.vdisplay);
> + kfree(dp_mode);
> + return 0;
> + }
> + m->width_mm = connector->display_info.width_mm;
> + m->height_mm = connector->display_info.height_mm;
> + drm_mode_probed_add(connector, m);
> + }
> + } else {
> + pr_err("No sink connected\n");
> + }
> + kfree(dp_mode);
How big is dp_mode? You can't put it on the stack?
> +
> + return rc;
> +}
> +
> +int dp_drm_bridge_init(void *data, struct drm_encoder *encoder)
> +{
> + int rc = 0;
> + struct dp_bridge *dp_bridge;
> + struct drm_device *dev;
> + struct msm_dp *dp_display = data;
> + struct msm_drm_private *priv = NULL;
> +
> + dp_bridge = kzalloc(sizeof(*dp_bridge), GFP_KERNEL);
> + if (!dp_bridge) {
> + rc = -ENOMEM;
Just return -ENOMEM;
> + goto error;
> + }
> +
> + dev = dp_display->drm_dev;
> + dp_bridge->display = dp_display;
> + dp_bridge->base.funcs = &dp_bridge_ops;
> + dp_bridge->base.encoder = encoder;
> +
> + priv = dev->dev_private;
> +
> + rc = drm_bridge_attach(encoder, &dp_bridge->base, NULL);
> + if (rc) {
> + pr_err("failed to attach bridge, rc=%d\n", rc);
> + goto error_free_bridge;
> + }
> +
> + rc = dp_display->request_irq(dp_display);
> + if (rc) {
> + pr_err("request_irq failed, rc=%d\n", rc);
> + goto error_free_bridge;
> + }
> +
> + encoder->bridge = &dp_bridge->base;
> + dp_display->dp_bridge = dp_bridge;
> + dp_display->bridge = &dp_bridge->base;
> +
> + return 0;
> +error_free_bridge:
> + kfree(dp_bridge);
> +error:
> + return rc;
> +}
> +
> +void dp_drm_bridge_deinit(void *data)
> +{
> + struct msm_dp *display = data;
> + struct dp_bridge *bridge = display->dp_bridge;
> +
> + if (bridge && bridge->base.encoder)
> + bridge->base.encoder->bridge = NULL;
> +
> + kfree(bridge);
> +}
> +
> +/**
> + * dp_connector_mode_valid - callback to determine if specified mode is valid
> + * @connector: Pointer to drm connector structure
> + * @mode: Pointer to drm mode structure
> + * Returns: Validity status for specified mode
> + */
> +static enum drm_mode_status dp_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct msm_dp *dp_disp;
> + struct dp_debug *debug;
> + struct dp_connector *dp_conn;
> +
> + if (!mode || !connector) {
> + pr_err("invalid params\n");
> + return MODE_ERROR;
> + }
>
I'm sure connector will always be valid, not sure about mode.
> + dp_conn = to_dp_connector(connector);
> + dp_disp = dp_conn->dp_display;
> + debug = dp_disp->get_debug(dp_disp);
> +
> + mode->vrefresh = drm_mode_vrefresh(mode);
> +
> + if (mode->clock > dp_disp->max_pclk_khz)
> + return MODE_BAD;
> +
> + if (debug->debug_en && (mode->hdisplay != debug->hdisplay ||
> + mode->vdisplay != debug->vdisplay ||
> + mode->vrefresh != debug->vrefresh ||
> + mode->picture_aspect_ratio != debug->aspect_ratio))
> + return MODE_BAD;
> +
> + return dp_disp->validate_mode(dp_disp, mode->clock);
> +}
> +
> +static const struct drm_connector_funcs dp_connector_funcs = {
> + .detect = dp_connector_detect,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = dp_connector_destroy,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
> + .get_modes = dp_connector_get_modes,
> + .mode_valid = dp_connector_mode_valid,
> +};
> +
> +/* connector initialization */
> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
> +{
> + struct drm_connector *connector = NULL;
> + struct dp_connector *dp_connector;
> + int ret;
> +
> + dp_connector = kzalloc(sizeof(*dp_connector), GFP_KERNEL);
> + if (!dp_connector)
> + return ERR_PTR(-ENOMEM);
> +
> + dp_connector->dp_display = dp_display;
> +
> + connector = &dp_connector->base;
> +
> + ret = drm_connector_init(dp_display->drm_dev, connector, &dp_connector_funcs,
> + DRM_MODE_CONNECTOR_DisplayPort);
> + if (ret)
You are missing a kfree here.
> + return ERR_PTR(ret);
> +
> + drm_connector_helper_add(connector, &dp_connector_helper_funcs);
> +
> + /*
> + * Enable HPD to let hpd event is handled
> + * when cable is attached to the host.
> + */
> + connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> + /* Display driver doesn't support interlace now. */
> + connector->interlace_allowed = false;
> + connector->doublescan_allowed = false;
> +
> + drm_connector_attach_encoder(connector, dp_display->encoder);
> +
> + return connector;
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
> new file mode 100644
> index 0000000..efbc1be
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#ifndef _DP_DRM_H_
> +#define _DP_DRM_H_
> +
> +#include <linux/types.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +#include "msm_drv.h"
> +#include "dp_display.h"
> +
> +struct dp_bridge {
> + struct drm_bridge base;
> + u32 id;
> +
> + struct msm_dp *display;
> + struct dp_display_mode dp_mode;
> +};
> +
> +/**
> + * dp_connector_post_init - callback to perform additional initialization steps
> + * @connector: Pointer to drm connector structure
> + * @display: Pointer to private display handle
> + * Returns: Zero on success
> + */
> +int dp_connector_post_init(struct drm_connector *connector, void *display);
> +
> +void dp_connector_send_hpd_event(void *display);
> +
> +int dp_drm_bridge_init(void *display,
> + struct drm_encoder *encoder);
> +
> +void dp_drm_bridge_deinit(void *display);
> +
> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display);
> +
> +#endif /* _DP_DRM_H_ */
> +
> diff --git a/drivers/gpu/drm/msm/dp/dp_extcon.c b/drivers/gpu/drm/msm/dp/dp_extcon.c
> new file mode 100644
> index 0000000..635c13b
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_extcon.c
> @@ -0,0 +1,400 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/extcon.h>
> +
> +#include "dp_extcon.h"
> +
> +/* DP specific VDM commands */
> +#define DP_USBPD_VDM_STATUS 0x10
> +#define DP_USBPD_VDM_CONFIGURE 0x11
> +
> +/* USBPD-TypeC specific Macros */
> +#define VDM_VERSION 0x0
> +#define USB_C_DP_SID 0xFF01
> +
> +enum dp_usbpd_pin_assignment {
> + DP_USBPD_PIN_A,
> + DP_USBPD_PIN_B,
> + DP_USBPD_PIN_C,
> + DP_USBPD_PIN_D,
> + DP_USBPD_PIN_E,
> + DP_USBPD_PIN_F,
> + DP_USBPD_PIN_MAX,
> +};
> +
> +struct dp_usbpd_capabilities {
> + enum dp_usbpd_port port;
> + bool receptacle_state;
> + u8 ulink_pin_config;
> + u8 dlink_pin_config;
> +};
> +
> +struct dp_extcon_private {
> + u32 vdo;
> + struct device *dev;
> + struct notifier_block extcon_nb;
> + struct extcon_dev *extcon;
> + struct workqueue_struct *extcon_wq;
> + struct work_struct event_work;
> + struct usbpd *pd;
> + struct dp_usbpd_cb *dp_cb;
> + struct dp_usbpd_capabilities cap;
> + struct dp_usbpd dp_usbpd;
> +};
> +
> +static const char *dp_usbpd_pin_name(u8 pin)
> +{
> + switch (pin) {
> + case DP_USBPD_PIN_A: return "DP_USBPD_PIN_ASSIGNMENT_A";
> + case DP_USBPD_PIN_B: return "DP_USBPD_PIN_ASSIGNMENT_B";
> + case DP_USBPD_PIN_C: return "DP_USBPD_PIN_ASSIGNMENT_C";
> + case DP_USBPD_PIN_D: return "DP_USBPD_PIN_ASSIGNMENT_D";
> + case DP_USBPD_PIN_E: return "DP_USBPD_PIN_ASSIGNMENT_E";
> + case DP_USBPD_PIN_F: return "DP_USBPD_PIN_ASSIGNMENT_F";
> + default: return "UNKNOWN";
> + }
> +}
> +
> +static const char *dp_usbpd_port_name(enum dp_usbpd_port port)
> +{
> + switch (port) {
> + case DP_USBPD_PORT_NONE: return "DP_USBPD_PORT_NONE";
> + case DP_USBPD_PORT_UFP_D: return "DP_USBPD_PORT_UFP_D";
> + case DP_USBPD_PORT_DFP_D: return "DP_USBPD_PORT_DFP_D";
> + case DP_USBPD_PORT_D_UFP_D: return "DP_USBPD_PORT_D_UFP_D";
> + default: return "DP_USBPD_PORT_NONE";
> + }
> +}
> +
> +static void dp_usbpd_init_port(enum dp_usbpd_port *port, u32 in_port)
> +{
> + switch (in_port) {
> + case 0:
> + *port = DP_USBPD_PORT_NONE;
> + break;
> + case 1:
> + *port = DP_USBPD_PORT_UFP_D;
> + break;
> + case 2:
> + *port = DP_USBPD_PORT_DFP_D;
> + break;
> + case 3:
> + *port = DP_USBPD_PORT_D_UFP_D;
> + break;
> + default:
> + *port = DP_USBPD_PORT_NONE;
> + }
> + pr_debug("port:%s\n", dp_usbpd_port_name(*port));
> +}
> +
> +void dp_usbpd_get_capabilities(struct dp_extcon_private *pd)
> +{
> + struct dp_usbpd_capabilities *cap = &pd->cap;
> + u32 buf = pd->vdo;
> + int port = buf & 0x3;
> +
> + cap->receptacle_state = (buf & BIT(6)) ? true : false;
> + cap->dlink_pin_config = (buf >> 8) & 0xff;
> + cap->ulink_pin_config = (buf >> 16) & 0xff;
> +
> + dp_usbpd_init_port(&cap->port, port);
> +}
> +
> +void dp_usbpd_get_status(struct dp_extcon_private *pd)
> +{
> + struct dp_usbpd *status = &pd->dp_usbpd;
> + u32 buf = pd->vdo;
> + int port = buf & 0x3;
> +
> + status->low_pow_st = (buf & BIT(2)) ? true : false;
> + status->adaptor_dp_en = (buf & BIT(3)) ? true : false;
> + status->multi_func = (buf & BIT(4)) ? true : false;
> + status->usb_config_req = (buf & BIT(5)) ? true : false;
> + status->exit_dp_mode = (buf & BIT(6)) ? true : false;
> + status->hpd_high = (buf & BIT(7)) ? true : false;
> + status->hpd_irq = (buf & BIT(8)) ? true : false;
> +
> + pr_debug("low_pow_st = %d, adaptor_dp_en = %d, multi_func = %d\n",
> + status->low_pow_st, status->adaptor_dp_en,
> + status->multi_func);
> + pr_debug("usb_config_req = %d, exit_dp_mode = %d, hpd_high =%d\n",
> + status->usb_config_req,
> + status->exit_dp_mode, status->hpd_high);
> + pr_debug("hpd_irq = %d\n", status->hpd_irq);
> +
> + dp_usbpd_init_port(&status->port, port);
> +}
> +
> +u32 dp_usbpd_gen_config_pkt(struct dp_extcon_private *pd)
> +{
> + u8 pin_cfg, pin;
> + u32 config = 0;
> + const u32 ufp_d_config = 0x2, dp_ver = 0x1;
> +
> + if (pd->cap.receptacle_state)
> + pin_cfg = pd->cap.ulink_pin_config;
> + else
> + pin_cfg = pd->cap.dlink_pin_config;
> +
> + for (pin = DP_USBPD_PIN_A; pin < DP_USBPD_PIN_MAX; pin++) {
> + if (pin_cfg & BIT(pin)) {
> + if (pd->dp_usbpd.multi_func) {
> + if (pin == DP_USBPD_PIN_D)
> + break;
> + } else {
> + break;
> + }
> + }
> + }
> +
> + if (pin == DP_USBPD_PIN_MAX)
> + pin = DP_USBPD_PIN_C;
> +
> + pr_debug("pin assignment: %s\n", dp_usbpd_pin_name(pin));
> +
> + config |= BIT(pin) << 8;
> +
> + config |= (dp_ver << 2);
> + config |= ufp_d_config;
> +
> + pr_debug("config = 0x%x\n", config);
> + return config;
> +}
> +
> +static int dp_extcon_connect(struct dp_usbpd *dp_usbpd, bool hpd)
> +{
> + int rc = 0;
> + struct dp_extcon_private *extcon;
> +
> + if (!dp_usbpd) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
> +
> + extcon = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
> +
> + dp_usbpd->hpd_high = hpd;
> + dp_usbpd->forced_disconnect = !hpd;
> +
> + if (hpd)
> + extcon->dp_cb->configure(extcon->dev);
> + else
> + extcon->dp_cb->disconnect(extcon->dev);
> +
> +error:
> + return rc;
> +}
> +
> +static int dp_extcon_get_lanes(struct dp_extcon_private *extcon_priv)
> +{
> + union extcon_property_value property;
> + u8 lanes;
> +
> + if (!extcon_priv || !extcon_priv->extcon) {
> + pr_err("Invalid input arguments\n");
> + return 0;
> + }
> +
> + extcon_get_property(extcon_priv->extcon,
> + EXTCON_DISP_DP,
> + EXTCON_PROP_USB_SS,
> + &property);
> + lanes = ((property.intval) ? 2 : 4);
> +
> + return lanes;
> +}
> +
> +
> +static void dp_extcon_event_work(struct work_struct *work)
> +{
> + struct dp_extcon_private *extcon_priv;
> + int dp_state, ret;
> + int lanes;
> + union extcon_property_value property;
> +
> + extcon_priv = container_of(work,
> + struct dp_extcon_private, event_work);
> +
> + if (!extcon_priv || !extcon_priv->extcon) {
The result of container_of can never be NULL.
> + pr_err("Invalid extcon device handler\n");
> + return;
> + }
> +
> + dp_state = extcon_get_state(extcon_priv->extcon,
> + EXTCON_DISP_DP);
> +
> + if (dp_state > 0) {
> + ret = extcon_get_property(extcon_priv->extcon,
> + EXTCON_DISP_DP,
> + EXTCON_PROP_USB_TYPEC_POLARITY,
> + &property);
> + if (ret) {
> + pr_err("Get Polarity property failed\n");
> + return;
> + }
> + extcon_priv->dp_usbpd.orientation =
> + ((property.intval) ?
> + ORIENTATION_CC2 : ORIENTATION_CC1);
> +
> + lanes = dp_extcon_get_lanes(extcon_priv);
> + if (!lanes)
> + return;
> + extcon_priv->dp_usbpd.multi_func =
> + ((lanes == 2) ? false : true);
> +
> + if (extcon_priv->dp_cb && extcon_priv->dp_cb->configure)
> + dp_extcon_connect(&extcon_priv->dp_usbpd, true);
> + } else {
> + if (extcon_priv->dp_cb && extcon_priv->dp_cb->disconnect)
> + dp_extcon_connect(&extcon_priv->dp_usbpd, false);
> + }
> +}
> +
> +static int dp_extcon_create_workqueue(struct dp_extcon_private *extcon_priv)
> +{
> + extcon_priv->extcon_wq = create_singlethread_workqueue("drm_dp_extcon");
> + if (IS_ERR_OR_NULL(extcon_priv->extcon_wq)) {
> + pr_err("Error creating extcon wq\n");
> + return -EPERM;
> + }
> +
> + INIT_WORK(&extcon_priv->event_work, dp_extcon_event_work);
> +
> + return 0;
> +}
> +
> +static int dp_extcon_event_notify(struct notifier_block *nb,
> + unsigned long event, void *priv)
> +{
> + struct dp_extcon_private *extcon_priv;
> +
> + extcon_priv = container_of(nb, struct dp_extcon_private,
> + extcon_nb);
> +
> + queue_work(extcon_priv->extcon_wq, &extcon_priv->event_work);
> + return NOTIFY_DONE;
> +}
> +
> +int dp_extcon_register(struct dp_usbpd *dp_usbpd)
> +{
> + struct dp_extcon_private *extcon_priv;
> + int ret = 0;
> +
> + if (!dp_usbpd)
> + return -EINVAL;
> +
> + pr_err("Start++++");
Debug message snuck through.
> + extcon_priv = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
> +
> + extcon_priv->extcon_nb.notifier_call = dp_extcon_event_notify;
> + ret = devm_extcon_register_notifier(extcon_priv->dev, extcon_priv->extcon,
> + EXTCON_DISP_DP,
> + &extcon_priv->extcon_nb);
> + if (ret) {
> + dev_err(extcon_priv->dev,
> + "register EXTCON_DISP_DP notifier err\n");
Prefer that you don't abbreviate err here.
> + ret = -EINVAL;
You set ret, but you don't do anything with it - you continue on.
> + }
> +
> + ret = dp_extcon_create_workqueue(extcon_priv);
> + if (ret) {
> + pr_err("Failed to create workqueue\n");
> + dp_extcon_unregister(dp_usbpd);
> + }
> +
> + pr_err("End----");
Also debug message.
> + return ret;
> +}
> +
> +void dp_extcon_unregister(struct dp_usbpd *dp_usbpd)
> +{
> + struct dp_extcon_private *extcon_priv;
> +
> + if (!dp_usbpd) {
> + pr_err("Invalid input\n");
Doesn't seem that the error message will do you much good here.
> + return;
> + }
> +
> + pr_err("Start++++");
Debug log message
> + extcon_priv = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
> +
> + devm_extcon_unregister_notifier(extcon_priv->dev, extcon_priv->extcon,
> + EXTCON_DISP_DP,
> + &extcon_priv->extcon_nb);
> +
> + if (extcon_priv->extcon_wq)
> + destroy_workqueue(extcon_priv->extcon_wq);
> +
> + pr_err("End----");
Debug log message
> + return;
> +}
> +
> +struct dp_usbpd *dp_extcon_get(struct device *dev, struct dp_usbpd_cb *cb)
> +{
> + int rc = 0;
> + struct dp_extcon_private *dp_extcon;
> + struct dp_usbpd *dp_usbpd;
> +
> + if (!cb) {
> + pr_err("invalid cb data\n");
Log message isn't useful, just return ERR_PTR(-EINVAL);
> + rc = -EINVAL;
> + goto error;
> + }
> +
> + pr_err("%s: Start", __func__);
Debug message.
> + dp_extcon = devm_kzalloc(dev, sizeof(*dp_extcon), GFP_KERNEL);
> + if (!dp_extcon) {
return ERR_PTR(-ENOMEM);
> + rc = -ENOMEM;
> + goto error;
> + }
> +
> + dp_extcon->extcon = extcon_get_edev_by_phandle(dev, 0);
> + if (!dp_extcon->extcon) {
> + pr_err("invalid extcon data\n");
> + rc = -EINVAL;
> + devm_kfree(dev, dp_extcon);
Just return ERR_PTR(-EINVAL);
> + goto error;
> + }
> +
> + dp_extcon->dev = dev;
> + dp_extcon->dp_cb = cb;
> +
> + dp_extcon->dp_usbpd.connect = dp_extcon_connect;
> + dp_usbpd = &dp_extcon->dp_usbpd;
> +
> + pr_err("%s: end", __func__);
Debug message
> + return dp_usbpd;
> +error:
> + return ERR_PTR(rc);
> +}
> +
> +void dp_extcon_put(struct dp_usbpd *dp_usbpd)
> +{
> + struct dp_extcon_private *extcon;
> +
> + if (!dp_usbpd)
> + return;
> +
> + extcon = container_of(dp_usbpd, struct dp_extcon_private, dp_usbpd);
> +
> + devm_kfree(extcon->dev, extcon);
You don't need to do this for managed memory. This whole function doesn't need
to exist.
> +
> + pr_err("%s: end", __func__);
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_extcon.h b/drivers/gpu/drm/msm/dp/dp_extcon.h
> new file mode 100644
> index 0000000..1f83a18
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_extcon.h
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please
> +#ifndef _DP_EXTCON_H_
> +#define _DP_EXTCON_H_
> +
> +//#include <linux/usb/usbpd.h>
> +
> +#include <linux/types.h>
> +#include <linux/device.h>
> +
> +/**
> + * enum dp_usbpd_port - usb/dp port type
> + * @DP_USBPD_PORT_NONE: port not configured
> + * @DP_USBPD_PORT_UFP_D: Upstream Facing Port - DisplayPort
> + * @DP_USBPD_PORT_DFP_D: Downstream Facing Port - DisplayPort
> + * @DP_USBPD_PORT_D_UFP_D: Both UFP & DFP - DisplayPort
> + */
> +
> +enum dp_usbpd_port {
> + DP_USBPD_PORT_NONE,
> + DP_USBPD_PORT_UFP_D,
> + DP_USBPD_PORT_DFP_D,
> + DP_USBPD_PORT_D_UFP_D,
> +};
> +
> +enum plug_orientation {
> + ORIENTATION_NONE,
> + ORIENTATION_CC1,
> + ORIENTATION_CC2,
> +};
> +
> +/**
> + * struct dp_usbpd - DisplayPort status
> + *
> + * @port: port configured
> + * orientation: plug orientation configuration
> + * @low_pow_st: low power state
> + * @adaptor_dp_en: adaptor functionality enabled
> + * @multi_func: multi-function preferred
> + * @usb_config_req: request to switch to usb
> + * @exit_dp_mode: request exit from displayport mode
> + * @hpd_high: Hot Plug Detect signal is high.
> + * @hpd_irq: Change in the status since last message
> + * @alt_mode_cfg_done: bool to specify alt mode status
> + * @debug_en: bool to specify debug mode
> + * @connect: simulate disconnect or connect for debug mode
> + */
> +struct dp_usbpd {
> + enum dp_usbpd_port port;
> + enum plug_orientation orientation;
> + bool low_pow_st;
> + bool adaptor_dp_en;
> + bool multi_func;
> + bool usb_config_req;
> + bool exit_dp_mode;
> + bool hpd_high;
> + bool hpd_irq;
> + bool alt_mode_cfg_done;
> + bool debug_en;
> + bool forced_disconnect;
> +
> + int (*connect)(struct dp_usbpd *dp_usbpd, bool hpd);
> +};
> +
> +/**
> + * struct dp_usbpd_cb - callback functions provided by the client
> + *
> + * @configure: called by usbpd module when PD communication has
> + * been completed and the usb peripheral has been configured on
> + * dp mode.
> + * @disconnect: notify the cable disconnect issued by usb.
> + * @attention: notify any attention message issued by usb.
> + */
> +struct dp_usbpd_cb {
> + int (*configure)(struct device *dev);
> + int (*disconnect)(struct device *dev);
> + int (*attention)(struct device *dev);
> +};
> +
> +/**
> + * dp_extcon_get() - setup usbpd module
> + *
> + * @dev: device instance of the caller
> + * @cb: struct containing callback function pointers.
> + *
> + * This function allows the client to initialize the usbpd
> + * module. The module will communicate with usb driver and
> + * handles the power delivery (PD) communication with the
> + * sink/usb device. This module will notify the client using
> + * the callback functions about the connection and status.
> + */
> +struct dp_usbpd *dp_extcon_get(struct device *dev, struct dp_usbpd_cb *cb);
> +
> +void dp_extcon_put(struct dp_usbpd *pd);
> +
> +int dp_extcon_register(struct dp_usbpd *dp_usbpd);
> +void dp_extcon_unregister(struct dp_usbpd *dp_usbpd);
> +
> +#endif /* _DP_EXTCON_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> new file mode 100644
> index 0000000..5ab1efc
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -0,0 +1,1549 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include "dp_link.h"
> +#include "dp_panel.h"
> +
> +enum dynamic_range {
> + DP_DYNAMIC_RANGE_RGB_VESA = 0x00,
> + DP_DYNAMIC_RANGE_RGB_CEA = 0x01,
> + DP_DYNAMIC_RANGE_UNKNOWN = 0xFFFFFFFF,
> +};
> +
> +enum audio_sample_rate {
> + AUDIO_SAMPLE_RATE_32_KHZ = 0x00,
> + AUDIO_SAMPLE_RATE_44_1_KHZ = 0x01,
> + AUDIO_SAMPLE_RATE_48_KHZ = 0x02,
> + AUDIO_SAMPLE_RATE_88_2_KHZ = 0x03,
> + AUDIO_SAMPLE_RATE_96_KHZ = 0x04,
> + AUDIO_SAMPLE_RATE_176_4_KHZ = 0x05,
> + AUDIO_SAMPLE_RATE_192_KHZ = 0x06,
> +};
> +
> +enum audio_pattern_type {
> + AUDIO_TEST_PATTERN_OPERATOR_DEFINED = 0x00,
> + AUDIO_TEST_PATTERN_SAWTOOTH = 0x01,
> +};
> +
> +struct dp_link_request {
> + u32 test_requested;
> + u32 test_link_rate;
> + u32 test_lane_count;
> +};
> +
> +struct dp_link_private {
> + u32 prev_sink_count;
> + struct device *dev;
> + struct dp_aux *aux;
> + struct dp_link dp_link;
> +
> + struct dp_link_request request;
> + u8 link_status[DP_LINK_STATUS_SIZE];
> +};
> +
> +static char *dp_link_get_audio_test_pattern(u32 pattern)
> +{
> + switch (pattern) {
> + case AUDIO_TEST_PATTERN_OPERATOR_DEFINED:
> + return DP_LINK_ENUM_STR(AUDIO_TEST_PATTERN_OPERATOR_DEFINED);
> + case AUDIO_TEST_PATTERN_SAWTOOTH:
> + return DP_LINK_ENUM_STR(AUDIO_TEST_PATTERN_SAWTOOTH);
> + default:
> + return "unknown";
> + }
> +}
> +
> +static char *dp_link_get_audio_sample_rate(u32 rate)
> +{
> + switch (rate) {
> + case AUDIO_SAMPLE_RATE_32_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_32_KHZ);
> + case AUDIO_SAMPLE_RATE_44_1_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_44_1_KHZ);
> + case AUDIO_SAMPLE_RATE_48_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_48_KHZ);
> + case AUDIO_SAMPLE_RATE_88_2_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_88_2_KHZ);
> + case AUDIO_SAMPLE_RATE_96_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_96_KHZ);
> + case AUDIO_SAMPLE_RATE_176_4_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_176_4_KHZ);
> + case AUDIO_SAMPLE_RATE_192_KHZ:
> + return DP_LINK_ENUM_STR(AUDIO_SAMPLE_RATE_192_KHZ);
> + default:
> + return "unknown";
> + }
> +}
> +
> +static int dp_link_get_period(struct dp_link_private *link, int const addr)
> +{
> + int ret = 0;
> + u8 bp;
> + u8 data;
> + u32 const param_len = 0x1;
> + u32 const max_audio_period = 0xA;
> +
> + /* TEST_AUDIO_PERIOD_CH_XX */
> + if (drm_dp_dpcd_read(link->aux->drm_aux, addr, &bp,
> + param_len) < param_len) {
> + pr_err("failed to read test_audio_period (0x%x)\n", addr);
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + data = bp;
> +
> + /* Period - Bits 3:0 */
> + data = data & 0xF;
> + if ((int)data > max_audio_period) {
> + pr_err("invalid test_audio_period_ch_1 = 0x%x\n", data);
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + ret = data;
> +exit:
> + return ret;
> +}
> +
> +static int dp_link_parse_audio_channel_period(struct dp_link_private *link)
> +{
> + int ret = 0;
> + struct dp_link_test_audio *req = &link->dp_link.test_audio;
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH1);
> + if (ret == -EINVAL)
You don't need the label for any of these.
> + goto exit;
> +
> + req->test_audio_period_ch_1 = ret;
> + pr_debug("test_audio_period_ch_1 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH2);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_2 = ret;
> + pr_debug("test_audio_period_ch_2 = 0x%x\n", ret);
> +
> + /* TEST_AUDIO_PERIOD_CH_3 (Byte 0x275) */
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH3);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_3 = ret;
> + pr_debug("test_audio_period_ch_3 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH4);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_4 = ret;
> + pr_debug("test_audio_period_ch_4 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH5);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_5 = ret;
> + pr_debug("test_audio_period_ch_5 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH6);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_6 = ret;
> + pr_debug("test_audio_period_ch_6 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH7);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_7 = ret;
> + pr_debug("test_audio_period_ch_7 = 0x%x\n", ret);
> +
> + ret = dp_link_get_period(link, DP_TEST_AUDIO_PERIOD_CH8);
> + if (ret == -EINVAL)
> + goto exit;
> +
> + req->test_audio_period_ch_8 = ret;
> + pr_debug("test_audio_period_ch_8 = 0x%x\n", ret);
> +exit:
> + return ret;
This isn't my area of expertise but I can't shake the feeling that this code
could be consolidated - like why are there individual members for
req->test_audio_period_ch_N and not an array. Like I said, not my area, just
feel like there is code that could be saved.
> +}
> +
> +static int dp_link_parse_audio_pattern_type(struct dp_link_private *link)
> +{
> + int ret = 0;
> + u8 bp;
> + u8 data;
> + int rlen;
> + int const param_len = 0x1;
> + int const max_audio_pattern_type = 0x1;
> +
> + rlen = drm_dp_dpcd_read(link->aux->drm_aux,
> + DP_TEST_AUDIO_PATTERN_TYPE, &bp, param_len);
> + if (rlen < param_len) {
> + pr_err("failed to read link audio mode data\n");
just return -EINVAL;
> + ret = -EINVAL;
> + goto exit;
> + }
> + data = bp;
> +
> + /* Audio Pattern Type - Bits 7:0 */
> + if ((int)data > max_audio_pattern_type) {
> + pr_err("invalid audio pattern type = 0x%x\n", data);
> + ret = -EINVAL;
just return -EINVAL;
> + goto exit;
> + }
> +
> + link->dp_link.test_audio.test_audio_pattern_type = data;
> + pr_debug("audio pattern type = %s\n",
> + dp_link_get_audio_test_pattern(data));
> +exit:
> + return ret;
return 0;
> +}
> +
> +static int dp_link_parse_audio_mode(struct dp_link_private *link)
> +{
> + int ret = 0;
> + u8 bp;
> + u8 data;
> + int rlen;
> + int const param_len = 0x1;
> + int const max_audio_sampling_rate = 0x6;
> + int const max_audio_channel_count = 0x8;
> + int sampling_rate = 0x0;
> + int channel_count = 0x0;
> +
> + rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_AUDIO_MODE,
> + &bp, param_len);
> + if (rlen < param_len) {
> + pr_err("failed to read link audio mode data\n");
> + ret = -EINVAL;
Same as above - you should almost never have a just a return after a label. Same
is true for much of this file.
> + goto exit;
> + }
> + data = bp;
> +
> + /* Sampling Rate - Bits 3:0 */
> + sampling_rate = data & 0xF;
> + if (sampling_rate > max_audio_sampling_rate) {
> + pr_err("sampling rate (0x%x) greater than max (0x%x)\n",
> + sampling_rate, max_audio_sampling_rate);
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* Channel Count - Bits 7:4 */
> + channel_count = ((data & 0xF0) >> 4) + 1;
> + if (channel_count > max_audio_channel_count) {
> + pr_err("channel_count (0x%x) greater than max (0x%x)\n",
> + channel_count, max_audio_channel_count);
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + link->dp_link.test_audio.test_audio_sampling_rate = sampling_rate;
> + link->dp_link.test_audio.test_audio_channel_count = channel_count;
> + pr_debug("sampling_rate = %s, channel_count = 0x%x\n",
> + dp_link_get_audio_sample_rate(sampling_rate), channel_count);
> +exit:
> + return ret;
> +}
<snip>
> +static void dp_link_send_test_response(struct dp_link *dp_link)
> +{
> + struct dp_link_private *link = NULL;
> + u32 const response_len = 0x1;
> +
> + if (!dp_link) {
> + pr_err("invalid input\n");
> + return;
> + }
It doesn't look to me like dp_link can be NULL here.
> + link = container_of(dp_link, struct dp_link_private, dp_link);
> +
> + drm_dp_dpcd_write(link->aux->drm_aux, DP_TEST_RESPONSE,
> + &dp_link->test_response, response_len);
> +}
> +
> +static int dp_link_psm_config(struct dp_link *dp_link,
> + struct drm_dp_link *link_info, bool enable)
> +{
> + struct dp_link_private *link = NULL;
> + int ret = 0;
> +
> + if (!dp_link) {
> + pr_err("invalid params\n");
> + return -EINVAL;
> + }
>
Same. It seems that all of these function pointers have a verified valid
dp_link before calling.
> + link = container_of(dp_link, struct dp_link_private, dp_link);
> +
> + if (enable)
> + ret = drm_dp_link_power_down(link->aux->drm_aux, link_info);
> + else
> + ret = drm_dp_link_power_up(link->aux->drm_aux, link_info);
> +
> + if (ret)
> + pr_err("Failed to %s low power mode\n",
> + (enable ? "enter" : "exit"));
> + else
> + dp_link->psm_enabled = enable;
> +
> + return ret;
> +}
<snip>
> +static int dp_link_send_psm_request(struct dp_link *dp_link, bool req)
> +{
> + struct dp_link_private *link;
> +
> + if (!dp_link) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
> +
> + link = container_of(dp_link, struct dp_link_private, dp_link);
> +
> + return 0;
This function looks like it doesn't do anything of value.
> +}
> +
> +static u32 dp_link_get_test_bits_depth(struct dp_link *dp_link, u32 bpp)
> +{
> + u32 tbd;
> +
> + /*
> + * Few simplistic rules and assumptions made here:
> + * 1. Test bit depth is bit depth per color component
> + * 2. Assume 3 color components
> + */
> + switch (bpp) {
> + case 18:
> + tbd = DP_TEST_BIT_DEPTH_6;
> + break;
> + case 24:
> + tbd = DP_TEST_BIT_DEPTH_8;
> + break;
> + case 30:
> + tbd = DP_TEST_BIT_DEPTH_10;
> + break;
> + default:
> + tbd = DP_TEST_BIT_DEPTH_UNKNOWN;
> + break;
> + }
> +
> + if (tbd != DP_TEST_BIT_DEPTH_UNKNOWN)
> + tbd = (tbd >> DP_TEST_BIT_DEPTH_SHIFT);
> +
> + return tbd;
> +}
> +
> +struct dp_link *dp_link_get(struct device *dev, struct dp_aux *aux)
> +{
> + int rc = 0;
> + struct dp_link_private *link;
> + struct dp_link *dp_link;
> +
> + if (!dev || !aux) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
Dev and aux both appear to be non NULL from the claler.
> + link = devm_kzalloc(dev, sizeof(*link), GFP_KERNEL);
> + if (!link) {
> + rc = -EINVAL;
This should be -ENOMEM and you should just return ERR_PTR(-ENOMEM);
> + goto error;
> + }
> +
> + link->dev = dev;
> + link->aux = aux;
> +
> + dp_link = &link->dp_link;
> +
> + dp_link->process_request = dp_link_process_request;
> + dp_link->get_test_bits_depth = dp_link_get_test_bits_depth;
> + dp_link->get_colorimetry_config = dp_link_get_colorimetry_config;
> + dp_link->adjust_levels = dp_link_adjust_levels;
> + dp_link->send_psm_request = dp_link_send_psm_request;
> + dp_link->send_test_response = dp_link_send_test_response;
> + dp_link->psm_config = dp_link_psm_config;
> + dp_link->send_edid_checksum = dp_link_send_edid_checksum;
> +
> + return dp_link;
> +error:
> + return ERR_PTR(rc);
Label should not be needed.
> +}
> +
> +void dp_link_put(struct dp_link *dp_link)
> +{
> + struct dp_link_private *link;
> +
> + if (!dp_link)
> + return;
> +
> + link = container_of(dp_link, struct dp_link_private, dp_link);
> +
> + devm_kfree(link->dev, link);
You don't need to delete devm_ managed memory.
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
> new file mode 100644
> index 0000000..6d6ef43
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_link.h
> @@ -0,0 +1,184 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#ifndef _DP_LINK_H_
> +#define _DP_LINK_H_
> +
> +#include "dp_aux.h"
> +
> +#define DS_PORT_STATUS_CHANGED 0x200
> +#define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
> +#define DP_LINK_ENUM_STR(x) #x
> +
> +enum dp_link_voltage_level {
> + DP_LINK_VOLTAGE_LEVEL_0 = 0,
> + DP_LINK_VOLTAGE_LEVEL_1 = 1,
> + DP_LINK_VOLTAGE_LEVEL_2 = 2,
> + DP_LINK_VOLTAGE_MAX = DP_LINK_VOLTAGE_LEVEL_2,
The last item in a enum will be the value of the previous item plus 1 - which is
really the only good reason to use a enum IMO. You don't need to assign it
which is good because if you ever added anything to this list it would take two
lines of change and not just one.
> +};
> +
> +enum dp_link_preemaphasis_level {
> + DP_LINK_PRE_EMPHASIS_LEVEL_0 = 0,
> + DP_LINK_PRE_EMPHASIS_LEVEL_1 = 1,
> + DP_LINK_PRE_EMPHASIS_LEVEL_2 = 2,
> + DP_LINK_PRE_EMPHASIS_MAX = DP_LINK_PRE_EMPHASIS_LEVEL_2,
> +};
Same.
> +
> +struct dp_link_sink_count {
> + u32 count;
> + bool cp_ready;
> +};
> +
> +struct dp_link_test_video {
> + u32 test_video_pattern;
> + u32 test_bit_depth;
> + u32 test_dyn_range;
> + u32 test_h_total;
> + u32 test_v_total;
> + u32 test_h_start;
> + u32 test_v_start;
> + u32 test_hsync_pol;
> + u32 test_hsync_width;
> + u32 test_vsync_pol;
> + u32 test_vsync_width;
> + u32 test_h_width;
> + u32 test_v_height;
> + u32 test_rr_d;
> + u32 test_rr_n;
> +};
> +
> +struct dp_link_test_audio {
> + u32 test_audio_sampling_rate;
> + u32 test_audio_channel_count;
> + u32 test_audio_pattern_type;
> + u32 test_audio_period_ch_1;
> + u32 test_audio_period_ch_2;
> + u32 test_audio_period_ch_3;
> + u32 test_audio_period_ch_4;
> + u32 test_audio_period_ch_5;
> + u32 test_audio_period_ch_6;
> + u32 test_audio_period_ch_7;
> + u32 test_audio_period_ch_8;
> +};
> +
> +struct dp_link_phy_params {
> + u32 phy_test_pattern_sel;
> + u8 v_level;
> + u8 p_level;
> +};
> +
> +struct dp_link_params {
> + u32 lane_count;
> + u32 bw_code;
> +};
> +
> +struct dp_link {
> + u32 sink_request;
> + u32 test_response;
> + bool psm_enabled;
> +
> + struct dp_link_sink_count sink_count;
> + struct dp_link_test_video test_video;
> + struct dp_link_test_audio test_audio;
> + struct dp_link_phy_params phy_params;
> + struct dp_link_params link_params;
> +
> + u32 (*get_test_bits_depth)(struct dp_link *dp_link, u32 bpp);
> + int (*process_request)(struct dp_link *dp_link);
> + int (*get_colorimetry_config)(struct dp_link *dp_link);
> + int (*adjust_levels)(struct dp_link *dp_link, u8 *link_status);
> + int (*send_psm_request)(struct dp_link *dp_link, bool req);
> + void (*send_test_response)(struct dp_link *dp_link);
> + int (*psm_config)(struct dp_link *dp_link,
> + struct drm_dp_link *link_info, bool enable);
> + void (*send_edid_checksum)(struct dp_link *dp_link, u8 checksum);
> +};
> +
> +static inline char *dp_link_get_phy_test_pattern(u32 phy_test_pattern_sel)
> +{
> + switch (phy_test_pattern_sel) {
> + case DP_TEST_PHY_PATTERN_NONE:
> + return DP_LINK_ENUM_STR(DP_TEST_PHY_PATTERN_NONE);
> + case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING:
> + return DP_LINK_ENUM_STR(
> + DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING);
> + case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT:
> + return DP_LINK_ENUM_STR(
> + DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT);
> + case DP_TEST_PHY_PATTERN_PRBS7:
> + return DP_LINK_ENUM_STR(DP_TEST_PHY_PATTERN_PRBS7);
> + case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN:
> + return DP_LINK_ENUM_STR(
> + DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN);
> + case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN:
> + return DP_LINK_ENUM_STR(
> + DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN);
> + default:
> + return "unknown";
> + }
> +}
This is a bit beefy for a inline for my tastes.
> +
> +/**
> + * mdss_dp_test_bit_depth_to_bpp() - convert test bit depth to bpp
> + * @tbd: test bit depth
> + *
> + * Returns the bits per pixel (bpp) to be used corresponding to the
> + * git bit depth value. This function assumes that bit depth has
> + * already been validated.
> + */
> +static inline u32 dp_link_bit_depth_to_bpp(u32 tbd)
> +{
> + u32 bpp;
> +
> + /*
> + * Few simplistic rules and assumptions made here:
> + * 1. Bit depth is per color component
> + * 2. If bit depth is unknown return 0
> + * 3. Assume 3 color components
> + */
> + switch (tbd) {
> + case DP_TEST_BIT_DEPTH_6:
> + bpp = 18;
> + break;
> + case DP_TEST_BIT_DEPTH_8:
> + bpp = 24;
> + break;
> + case DP_TEST_BIT_DEPTH_10:
> + bpp = 30;
> + break;
> + case DP_TEST_BIT_DEPTH_UNKNOWN:
> + default:
> + bpp = 0;
> + }
> +
> + return bpp;
> +}
> +
> +/**
> + * dp_link_get() - get the functionalities of dp test module
> + *
> + *
> + * return: a pointer to dp_link struct
> + */
> +struct dp_link *dp_link_get(struct device *dev, struct dp_aux *aux);
> +
> +/**
> + * dp_link_put() - releases the dp test module's resources
> + *
> + * @dp_link: an instance of dp_link module
> + *
> + */
> +void dp_link_put(struct dp_link *dp_link);
> +
> +#endif /* _DP_LINK_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> new file mode 100644
> index 0000000..e497b44
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -0,0 +1,624 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include "dp_panel.h"
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_edid.h>
> +
> +#define DP_PANEL_DEFAULT_BPP 24
> +#define DP_MAX_DS_PORT_COUNT 1
> +
> +enum {
> + DP_LINK_RATE_MULTIPLIER = 27000000,
> +};
An enum of 1 is never useful.
> +
> +struct dp_panel_private {
> + struct device *dev;
> + struct dp_panel dp_panel;
> + struct dp_aux *aux;
> + struct dp_link *link;
> + struct dp_catalog_panel *catalog;
> + bool panel_on;
> + bool aux_cfg_update_done;
> +};
> +
> +static const struct dp_panel_info fail_safe = {
> + .h_active = 640,
> + .v_active = 480,
> + .h_back_porch = 48,
> + .h_front_porch = 16,
> + .h_sync_width = 96,
> + .h_active_low = 0,
> + .v_back_porch = 33,
> + .v_front_porch = 10,
> + .v_sync_width = 2,
> + .v_active_low = 0,
> + .h_skew = 0,
> + .refresh_rate = 60,
> + .pixel_clk_khz = 25200,
> + .bpp = 24,
> +};
> +
> +static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> +{
> + int rlen, rc = 0;
> + struct dp_panel_private *panel;
> + struct drm_dp_link *link_info;
> + u8 *dpcd, major = 0, minor = 0;
> + u32 dfp_count = 0;
> + unsigned long caps = DP_LINK_CAP_ENHANCED_FRAMING;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto end;
> + }
>
It appears that dp_panel will always be valid at every call point.
> + dpcd = dp_panel->dpcd;
> +
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> + link_info = &dp_panel->link_info;
> +
> + rlen = drm_dp_dpcd_read(panel->aux->drm_aux, DP_DPCD_REV,
> + dpcd, (DP_RECEIVER_CAP_SIZE + 1));
> + if (rlen < (DP_RECEIVER_CAP_SIZE + 1)) {
> + pr_err("dpcd read failed, rlen=%d\n", rlen);
Just return -ENVAL here and everywhere else end: is used.
> + rc = -EINVAL;
> + goto end;
> + }
> +
> + link_info->revision = dp_panel->dpcd[DP_DPCD_REV];
> +
> + major = (link_info->revision >> 4) & 0x0f;
> + minor = link_info->revision & 0x0f;
> + pr_debug("version: %d.%d\n", major, minor);
> +
> + link_info->rate =
> + drm_dp_bw_code_to_link_rate(dp_panel->dpcd[DP_MAX_LINK_RATE]);
> + pr_debug("link_rate=%d\n", link_info->rate);
> +
> + link_info->num_lanes = dp_panel->dpcd[DP_MAX_LANE_COUNT] &
> + DP_MAX_LANE_COUNT_MASK;
> +
> + pr_debug("lane_count=%d\n", link_info->num_lanes);
> +
> + if (drm_dp_enhanced_frame_cap(dpcd))
> + link_info->capabilities |= caps;
> +
> + dfp_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> + DP_DOWN_STREAM_PORT_COUNT;
> +
> + if ((dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT)
> + && (dpcd[DP_DPCD_REV] > 0x10)) {
> + rlen = drm_dp_dpcd_read(panel->aux->drm_aux,
> + DP_DOWNSTREAM_PORT_0, dp_panel->ds_ports,
> + DP_MAX_DOWNSTREAM_PORTS);
> + if (rlen < DP_MAX_DOWNSTREAM_PORTS) {
> + pr_err("ds port status failed, rlen=%d\n", rlen);
> + rc = -EINVAL;
> + goto end;
> + }
> + }
> +
> + if (dfp_count > DP_MAX_DS_PORT_COUNT)
> + pr_debug("DS port count %d greater that max (%d) supported\n",
> + dfp_count, DP_MAX_DS_PORT_COUNT);
> +
> +end:
> + return rc;
> +}
> +
> +static int dp_panel_set_default_link_params(struct dp_panel *dp_panel)
> +{
> + struct drm_dp_link *link_info;
> + const int default_bw_code = 162000;
> + const int default_num_lanes = 1;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
dp_panel will always be valid.
> + link_info = &dp_panel->link_info;
> + link_info->rate = default_bw_code;
> + link_info->num_lanes = default_num_lanes;
Instead of usint const variables just set the variables directly.
> + pr_debug("link_rate=%d num_lanes=%d\n",
> + link_info->rate, link_info->num_lanes);
> + return 0;
It doesn't look like a return value is needed here.
And maybe this function isn't needed at all? This could all happen inline where
this function is called.
> +}
> +
> +static int dp_panel_read_edid(struct dp_panel *dp_panel,
> + struct drm_connector *connector)
> +{
> + int retry_cnt = 0;
> + const int max_retry = 10;
> + struct dp_panel_private *panel;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
dp_panel will always be valid.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + do {
> + kfree(dp_panel->edid);
> + dp_panel->edid = drm_get_edid(connector,
> + &panel->aux->drm_aux->ddc);
> + if (!dp_panel->edid) {
> + pr_err("EDID read failed\n");
You don't need an error here because the error prints an error on your behalf
and you certainly don't want to print 10 errors without more details about what
the failure actually was.
> + retry_cnt++;
> + panel->aux->reconfig(panel->aux);
> + panel->aux_cfg_update_done = true;
> + } else {
> + return 0;
> + }
> + } while (retry_cnt < max_retry);
My edid parsing days were over back when DVI was new and interesting but as
just a kernel developer this seems odd. I'll leave it to the experts.
But at least I would use a valid for() loop:
for(retry_cnt = 0; retry_cnt < 10; retry_cnt++) {
..
}
> + return -EINVAL;
> +}
> +
> +static int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
> + struct drm_connector *connector)
> +{
> + int rc = 0;
> + struct dp_panel_private *panel;
> +
> + if (!dp_panel || !connector) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
Looks like both of these are valid from the caller.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + rc = dp_panel_read_dpcd(dp_panel);
> + if (rc || !is_link_rate_valid(drm_dp_link_rate_to_bw_code(
> + dp_panel->link_info.rate)) || !is_lane_count_valid(
> + dp_panel->link_info.num_lanes) ||
> + ((drm_dp_link_rate_to_bw_code(dp_panel->link_info.rate)) >
> + dp_panel->max_bw_code)) {
> + if ((rc == -ETIMEDOUT) || (rc == -ENODEV)) {
> + pr_err("DPCD read failed, return early\n");
> + return rc;
> + }
> + pr_err("panel dpcd read failed/incorrect, set default params\n");
> + dp_panel_set_default_link_params(dp_panel);
> + }
> +
> + rc = dp_panel_read_edid(dp_panel, connector);
> + if (rc) {
> + pr_err("panel edid read failed, set failsafe mode\n");
> + return rc;
> + }
> +
> + if (panel->aux_cfg_update_done) {
> + pr_debug("read DPCD with updated AUX config\n");
> + dp_panel_read_dpcd(dp_panel);
> + panel->aux_cfg_update_done = false;
> + }
> +
> + return 0;
> +}
> +
> +static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel,
> + u32 mode_edid_bpp, u32 mode_pclk_khz)
> +{
> + struct drm_dp_link *link_info;
> + const u32 max_supported_bpp = 30, min_supported_bpp = 18;
> + u32 bpp = 0, data_rate_khz = 0;
> +
> + bpp = min_t(u32, mode_edid_bpp, max_supported_bpp);
> +
> + link_info = &dp_panel->link_info;
> + data_rate_khz = link_info->num_lanes * link_info->rate * 8;
> +
> + while (bpp > min_supported_bpp) {
> + if (mode_pclk_khz * bpp <= data_rate_khz)
> + break;
> + bpp -= 6;
> + }
> +
> + return bpp;
> +}
> +
> +static u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel,
> + u32 mode_edid_bpp, u32 mode_pclk_khz)
> +{
> + struct dp_panel_private *panel;
> + u32 bpp = mode_edid_bpp;
> +
> + if (!dp_panel || !mode_edid_bpp || !mode_pclk_khz) {
> + pr_err("invalid input\n");
> + return 0;
> + }
>
dp_panel and mode_edid_bpp are valid from the caller, not sure about
mode_pcklk_hz. Might want to use a WARN_ON for that one.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + if (dp_panel->video_test)
> + bpp = dp_link_bit_depth_to_bpp(
> + panel->link->test_video.test_bit_depth);
> + else
> + bpp = dp_panel_get_supported_bpp(dp_panel, mode_edid_bpp,
> + mode_pclk_khz);
> +
> + return bpp;
> +}
> +
> +static void dp_panel_set_test_mode(struct dp_panel_private *panel,
> + struct dp_display_mode *mode)
> +{
> + struct dp_panel_info *pinfo = NULL;
> + struct dp_link_test_video *test_info = NULL;
> +
> + if (!panel) {
> + pr_err("invalid params\n");
> + return;
> + }
>
Panel will always be valid.
> + pinfo = &mode->timing;
> + test_info = &panel->link->test_video;
> +
> + pinfo->h_active = test_info->test_h_width;
> + pinfo->h_sync_width = test_info->test_hsync_width;
> + pinfo->h_back_porch = test_info->test_h_start -
> + test_info->test_hsync_width;
> + pinfo->h_front_porch = test_info->test_h_total -
> + (test_info->test_h_start + test_info->test_h_width);
> +
> + pinfo->v_active = test_info->test_v_height;
> + pinfo->v_sync_width = test_info->test_vsync_width;
> + pinfo->v_back_porch = test_info->test_v_start -
> + test_info->test_vsync_width;
> + pinfo->v_front_porch = test_info->test_v_total -
> + (test_info->test_v_start + test_info->test_v_height);
> +
> + pinfo->bpp = dp_link_bit_depth_to_bpp(test_info->test_bit_depth);
> + pinfo->h_active_low = test_info->test_hsync_pol;
> + pinfo->v_active_low = test_info->test_vsync_pol;
> +
> + pinfo->refresh_rate = test_info->test_rr_n;
> + pinfo->pixel_clk_khz = test_info->test_h_total *
> + test_info->test_v_total * pinfo->refresh_rate;
> +
> + if (test_info->test_rr_d == 0)
> + pinfo->pixel_clk_khz /= 1000;
> + else
> + pinfo->pixel_clk_khz /= 1001;
> +
> + if (test_info->test_h_width == 640)
> + pinfo->pixel_clk_khz = 25170;
> +}
> +
> +static int dp_panel_update_modes(struct drm_connector *connector,
> + struct edid *edid)
> +{
> + int rc = 0;
> +
> + pr_debug("%s +", __func__);
> + if (edid) {
> + drm_connector_update_edid_property(connector,
> + edid);
> + rc = drm_add_edid_modes(connector, edid);
> + pr_debug("%s -", __func__);
> + return rc;
> + }
> +
> + drm_connector_update_edid_property(connector, NULL);
> + pr_debug("%s null edid -", __func__);
> + return rc;
> +}
> +
> +static int dp_panel_get_modes(struct dp_panel *dp_panel,
> + struct drm_connector *connector, struct dp_display_mode *mode)
> +{
> + struct dp_panel_private *panel;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + return -EINVAL;
> + }
>
panel should always be valid.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + if (dp_panel->video_test) {
> + dp_panel_set_test_mode(panel, mode);
> + return 1;
What is the valid return of this function? Is it supposed to be boolean?
> + } else if (dp_panel->edid) {
> + return dp_panel_update_modes(connector, dp_panel->edid);
Indentation is suspect here.
> + }
> +
> + /* fail-safe mode */
> + memcpy(&mode->timing, &fail_safe,
> + sizeof(fail_safe));
> + return 1;
> +}
> +
> +static u8 dp_panel_get_edid_checksum(struct edid *edid)
> +{
> + struct edid *last_block = NULL;
> + u8 *raw_edid = NULL;
> +
> + if (!edid) {
> + pr_err("invalid edid input\n");
You don't need a log message here. Maybe a WARN_ON.
> + return 0;
> + }
> +
> + raw_edid = (u8 *)edid;
> + raw_edid += (edid->extensions * EDID_LENGTH);
> + last_block = (struct edid *)raw_edid;
> +
> + if (last_block)
> + return last_block->checksum;
> +
> + pr_err("Invalid block, no checksum\n");
> + return 0;
> +}
> +
> +static void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
> +{
> + struct dp_panel_private *panel;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + return;
> + }
>
Panel can't be NULL here.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> + u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> +
> + panel->link->send_edid_checksum(panel->link, checksum);
> + panel->link->send_test_response(panel->link);
> + }
> +}
> +
> +static void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable)
> +{
> + u32 hsync_start_x, hsync_end_x;
> + struct dp_catalog_panel *catalog;
> + struct dp_panel_private *panel;
> + struct dp_panel_info *pinfo;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + return;
> + }
This likely can't be NULL.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> + catalog = panel->catalog;
> + pinfo = &panel->dp_panel.pinfo;
> +
> + if (!panel->panel_on) {
> + pr_debug("DP panel not enabled, handle TPG on next panel on\n");
> + return;
> + }
> +
> + if (!enable) {
> + panel->catalog->tpg_config(catalog, false);
> + return;
> + }
> +
> + /* TPG config */
> + catalog->hsync_period = pinfo->h_sync_width + pinfo->h_back_porch +
> + pinfo->h_active + pinfo->h_front_porch;
> + catalog->vsync_period = pinfo->v_sync_width + pinfo->v_back_porch +
> + pinfo->v_active + pinfo->v_front_porch;
> +
> + catalog->display_v_start = ((pinfo->v_sync_width +
> + pinfo->v_back_porch) * catalog->hsync_period);
> + catalog->display_v_end = ((catalog->vsync_period -
> + pinfo->v_front_porch) * catalog->hsync_period) - 1;
> +
> + catalog->display_v_start += pinfo->h_sync_width + pinfo->h_back_porch;
> + catalog->display_v_end -= pinfo->h_front_porch;
> +
> + hsync_start_x = pinfo->h_back_porch + pinfo->h_sync_width;
> + hsync_end_x = catalog->hsync_period - pinfo->h_front_porch - 1;
> +
> + catalog->v_sync_width = pinfo->v_sync_width;
> +
> + catalog->hsync_ctl = (catalog->hsync_period << 16) |
> + pinfo->h_sync_width;
> + catalog->display_hctl = (hsync_end_x << 16) | hsync_start_x;
> +
> + pr_err("%s: calling catalog tpg_config\n", __func__);
> + panel->catalog->tpg_config(catalog, true);
> +}
> +
> +static int dp_panel_timing_cfg(struct dp_panel *dp_panel)
> +{
> + int rc = 0;
> + u32 data, total_ver, total_hor;
> + struct dp_catalog_panel *catalog;
> + struct dp_panel_private *panel;
> + struct dp_panel_info *pinfo;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto end;
> + }
>
This likely can't be NULL.
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> + catalog = panel->catalog;
> + pinfo = &panel->dp_panel.pinfo;
> +
> + pr_debug("width=%d hporch= %d %d %d\n",
> + pinfo->h_active, pinfo->h_back_porch,
> + pinfo->h_front_porch, pinfo->h_sync_width);
> +
> + pr_debug("height=%d vporch= %d %d %d\n",
> + pinfo->v_active, pinfo->v_back_porch,
> + pinfo->v_front_porch, pinfo->v_sync_width);
> +
> + total_hor = pinfo->h_active + pinfo->h_back_porch +
> + pinfo->h_front_porch + pinfo->h_sync_width;
> +
> + total_ver = pinfo->v_active + pinfo->v_back_porch +
> + pinfo->v_front_porch + pinfo->v_sync_width;
> +
> + data = total_ver;
> + data <<= 16;
> + data |= total_hor;
> +
> + catalog->total = data;
> +
> + data = (pinfo->v_back_porch + pinfo->v_sync_width);
> + data <<= 16;
> + data |= (pinfo->h_back_porch + pinfo->h_sync_width);
> +
> + catalog->sync_start = data;
> +
> + data = pinfo->v_sync_width;
> + data <<= 16;
> + data |= (pinfo->v_active_low << 31);
> + data |= pinfo->h_sync_width;
> + data |= (pinfo->h_active_low << 15);
> +
> + catalog->width_blanking = data;
> +
> + data = pinfo->v_active;
> + data <<= 16;
> + data |= pinfo->h_active;
> +
> + catalog->dp_active = data;
> +
> + panel->catalog->timing_cfg(catalog);
> + panel->panel_on = true;
> +end:
> + return rc;
> +}
> +
> +static int dp_panel_init_panel_info(struct dp_panel *dp_panel)
> +{
> + int rc = 0;
> + struct dp_panel_info *pinfo;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto end;
> + }
>
dp_panel can't be NULL.
> + pinfo = &dp_panel->pinfo;
> +
> + /*
> + * print resolution info as this is a result
> + * of user initiated action of cable connection
> + */
> + pr_info("SET NEW RESOLUTION:\n");
> + pr_info("%dx%d@%dfps\n", pinfo->h_active,
> + pinfo->v_active, pinfo->refresh_rate);
> + pr_info("h_porches(back|front|width) = (%d|%d|%d)\n",
> + pinfo->h_back_porch,
> + pinfo->h_front_porch,
> + pinfo->h_sync_width);
> + pr_info("v_porches(back|front|width) = (%d|%d|%d)\n",
> + pinfo->v_back_porch,
> + pinfo->v_front_porch,
> + pinfo->v_sync_width);
> + pr_info("pixel clock (KHz)=(%d)\n", pinfo->pixel_clk_khz);
> + pr_info("bpp = %d\n", pinfo->bpp);
> + pr_info("active low (h|v)=(%d|%d)\n", pinfo->h_active_low,
> + pinfo->v_active_low);
> +
> + pinfo->bpp = max_t(u32, 18, min_t(u32, pinfo->bpp, 30));
> + pr_info("updated bpp = %d\n", pinfo->bpp);
> +end:
> + return rc;
It doesn't seem like this function should return anything.
> +}
> +
> +static u32 dp_panel_get_min_req_link_rate(struct dp_panel *dp_panel)
> +{
> + const u32 encoding_factx10 = 8;
> + u32 min_link_rate_khz = 0, lane_cnt;
> + struct dp_panel_info *pinfo;
> +
> + if (!dp_panel) {
> + pr_err("invalid input\n");
> + goto end;
> + }
dp_panel_can't be NULL.
> + lane_cnt = dp_panel->link_info.num_lanes;
> + pinfo = &dp_panel->pinfo;
> +
> + /* num_lanes * lane_count * 8 >= pclk * bpp * 10 */
> + min_link_rate_khz = pinfo->pixel_clk_khz /
> + (lane_cnt * encoding_factx10);
> + min_link_rate_khz *= pinfo->bpp;
> +
> + pr_debug("min lclk req=%d khz for pclk=%d khz, lanes=%d, bpp=%d\n",
> + min_link_rate_khz, pinfo->pixel_clk_khz, lane_cnt,
> + pinfo->bpp);
> +end:
> + return min_link_rate_khz;
> +}
> +
> +struct dp_panel *dp_panel_get(struct dp_panel_in *in)
> +{
> + int rc = 0;
> + struct dp_panel_private *panel;
> + struct dp_panel *dp_panel;
> +
> + if (!in->dev || !in->catalog || !in->aux || !in->link) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
It looks like none of these are likely to be NULL;
>
> + panel = devm_kzalloc(in->dev, sizeof(*panel), GFP_KERNEL);
> + if (!panel) {
return ERR_PTR(-ENOMEM);
> + rc = -ENOMEM;
> + goto error;
> + }
> +
> + panel->dev = in->dev;
> + panel->aux = in->aux;
> + panel->catalog = in->catalog;
> + panel->link = in->link;
> +
> + dp_panel = &panel->dp_panel;
> + dp_panel->max_bw_code = DP_LINK_BW_8_1;
> + panel->aux_cfg_update_done = false;
> +
> + dp_panel->init_info = dp_panel_init_panel_info;
> + dp_panel->timing_cfg = dp_panel_timing_cfg;
> + dp_panel->read_sink_caps = dp_panel_read_sink_caps;
> + dp_panel->get_min_req_link_rate = dp_panel_get_min_req_link_rate;
> + dp_panel->get_mode_bpp = dp_panel_get_mode_bpp;
> + dp_panel->get_modes = dp_panel_get_modes;
> + dp_panel->handle_sink_request = dp_panel_handle_sink_request;
> + dp_panel->tpg_config = dp_panel_tpg_config;
> +
> + return dp_panel;
> +error:
> + return ERR_PTR(rc);
> +}
> +
> +void dp_panel_put(struct dp_panel *dp_panel)
> +{
> + struct dp_panel_private *panel;
> +
> + if (!dp_panel)
> + return;
> +
> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +
> + kfree(dp_panel->edid);
> + dp_panel->edid = NULL;
> + devm_kfree(panel->dev, panel);
You don't need to do this for devm managed memory.
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> new file mode 100644
> index 0000000..bf802df
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#ifndef _DP_PANEL_H_
> +#define _DP_PANEL_H_
> +
> +#include <drm/msm_drm.h>
> +
> +#include "dp_aux.h"
> +#include "dp_link.h"
> +#include "dp_extcon.h"
> +
> +struct edid;
> +
> +enum dp_lane_count {
> + DP_LANE_COUNT_1 = 1,
> + DP_LANE_COUNT_2 = 2,
> + DP_LANE_COUNT_4 = 4,
> +};
> +
> +#define DP_MAX_DOWNSTREAM_PORTS 0x10
> +
> +struct dp_panel_info {
> + u32 h_active;
> + u32 v_active;
> + u32 h_back_porch;
> + u32 h_front_porch;
> + u32 h_sync_width;
> + u32 h_active_low;
> + u32 v_back_porch;
> + u32 v_front_porch;
> + u32 v_sync_width;
> + u32 v_active_low;
> + u32 h_skew;
> + u32 refresh_rate;
> + u32 pixel_clk_khz;
> + u32 bpp;
> +};
> +
> +struct dp_display_mode {
> + struct dp_panel_info timing;
> + u32 capabilities;
> +};
> +
> +struct dp_panel_in {
> + struct device *dev;
> + struct dp_aux *aux;
> + struct dp_link *link;
> + struct dp_catalog_panel *catalog;
> +};
> +
> +struct dp_panel {
> + /* dpcd raw data */
> + u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
> + u8 ds_ports[DP_MAX_DOWNSTREAM_PORTS];
> +
> + struct drm_dp_link link_info;
> + struct edid *edid;
> + struct drm_connector *connector;
> + struct dp_panel_info pinfo;
> + bool video_test;
> +
> + u32 vic;
> + u32 max_pclk_khz;
> +
> + u32 max_bw_code;
> + int (*init_info)(struct dp_panel *dp_panel);
> + int (*deinit)(struct dp_panel *dp_panel);
> + int (*timing_cfg)(struct dp_panel *dp_panel);
> + int (*read_sink_caps)(struct dp_panel *dp_panel,
> + struct drm_connector *connector);
> + u32 (*get_min_req_link_rate)(struct dp_panel *dp_panel);
> + u32 (*get_mode_bpp)(struct dp_panel *dp_panel, u32 mode_max_bpp,
> + u32 mode_pclk_khz);
> + int (*get_modes)(struct dp_panel *dp_panel,
> + struct drm_connector *connector, struct dp_display_mode *mode);
> + void (*handle_sink_request)(struct dp_panel *dp_panel);
> + void (*tpg_config)(struct dp_panel *dp_panel, bool enable);
> +};
> +
> +/**
> + * is_link_rate_valid() - validates the link rate
> + * @lane_rate: link rate requested by the sink
> + *
> + * Returns true if the requested link rate is supported.
> + */
> +static inline bool is_link_rate_valid(u32 bw_code)
> +{
> + return ((bw_code == DP_LINK_BW_1_62) ||
> + (bw_code == DP_LINK_BW_2_7) ||
> + (bw_code == DP_LINK_BW_5_4) ||
> + (bw_code == DP_LINK_BW_8_1));
> +}
> +
> +/**
> + * dp_link_is_lane_count_valid() - validates the lane count
> + * @lane_count: lane count requested by the sink
> + *
> + * Returns true if the requested lane count is supported.
> + */
> +static inline bool is_lane_count_valid(u32 lane_count)
> +{
> + return (lane_count == DP_LANE_COUNT_1) ||
> + (lane_count == DP_LANE_COUNT_2) ||
> + (lane_count == DP_LANE_COUNT_4);
> +}
> +
> +struct dp_panel *dp_panel_get(struct dp_panel_in *in);
> +void dp_panel_put(struct dp_panel *dp_panel);
> +#endif /* _DP_PANEL_H_ */
And I'll end here for for now.
Jordan
--
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
