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 <chand...@codeaurora.org>
> ---
> drivers/gpu/drm/msm/Kconfig | 9 +
> drivers/gpu/drm/msm/Makefile | 15 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 206 ++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 44 +
> drivers/gpu/drm/msm/dp/dp_aux.c | 570 ++++++++++
> drivers/gpu/drm/msm/dp/dp_aux.h | 44 +
> drivers/gpu/drm/msm/dp/dp_catalog.c | 1188 ++++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_catalog.h | 144 +++
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 1475 +++++++++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_ctrl.h | 50 +
> drivers/gpu/drm/msm/dp/dp_debug.c | 507 +++++++++
> drivers/gpu/drm/msm/dp/dp_debug.h | 81 ++
> drivers/gpu/drm/msm/dp/dp_display.c | 977 +++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_display.h | 55 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 542 ++++++++++
> drivers/gpu/drm/msm/dp/dp_drm.h | 52 +
> drivers/gpu/drm/msm/dp/dp_extcon.c | 400 +++++++
> drivers/gpu/drm/msm/dp/dp_extcon.h | 111 ++
> drivers/gpu/drm/msm/dp/dp_link.c | 1549
> +++++++++++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_link.h | 184 ++++
> drivers/gpu/drm/msm/dp/dp_panel.c | 624 +++++++++++
> drivers/gpu/drm/msm/dp/dp_panel.h | 121 +++
> drivers/gpu/drm/msm/dp/dp_parser.c | 679 ++++++++++++
> drivers/gpu/drm/msm/dp/dp_parser.h | 205 ++++
> drivers/gpu/drm/msm/dp/dp_power.c | 599 +++++++++++
> drivers/gpu/drm/msm/dp/dp_power.h | 57 +
> drivers/gpu/drm/msm/dp/dp_reg.h | 357 ++++++
> drivers/gpu/drm/msm/msm_drv.c | 2 +
> drivers/gpu/drm/msm/msm_drv.h | 22 +
> include/drm/drm_dp_helper.h | 19 +
> 30 files changed, 10887 insertions(+), 1 deletion(-)
This is my last go - the end is in sight.
<snip all the way to dp_parser.c>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> new file mode 100644
> index 0000000..a366dc5
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -0,0 +1,679 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include <linux/of_gpio.h>
> +
> +#include "dp_parser.h"
> +
> +static void dp_parser_unmap_io_resources(struct dp_parser *parser)
> +{
> + struct dp_io *io = &parser->io;
> +
> + msm_dss_iounmap(&io->dp_ahb);
> + msm_dss_iounmap(&io->dp_aux);
> + msm_dss_iounmap(&io->dp_link);
> + msm_dss_iounmap(&io->dp_p0);
> + msm_dss_iounmap(&io->phy_io);
> + msm_dss_iounmap(&io->ln_tx0_io);
> + msm_dss_iounmap(&io->ln_tx0_io);
> + msm_dss_iounmap(&io->dp_pll_io);
> + msm_dss_iounmap(&io->dp_cc_io);
> + msm_dss_iounmap(&io->usb3_dp_com);
> + msm_dss_iounmap(&io->qfprom_io);
> +}
If you use devm_ to ioremap as suggested previously, then this all ends up going
away.
> +static int dp_parser_ctrl_res(struct dp_parser *parser)
> +{
> + int rc = 0;
> + u32 index;
> + struct platform_device *pdev = parser->pdev;
> + struct device_node *of_node = parser->pdev->dev.of_node;
> + struct dp_io *io = &parser->io;
> +
> + rc = of_property_read_u32(of_node, "cell-index", &index);
> + if (rc) {
> + pr_err("cell-index not specified, rc=%d\n", rc);
> + goto err;
> + }
This value doesn't appear to be used anywhere in this code, so why are you
grabbing it?
> + rc = msm_dss_ioremap_byname(pdev, &io->dp_ahb, "dp_ahb");
> + if (rc) {
> + pr_err("unable to remap dp io resources\n");
msm_ioremap() helpfully gives an informative error message including the name of
the region that failed so you don't need a redundant message here.
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->dp_aux, "dp_aux");
> + if (rc) {
> + pr_err("unable to remap dp io resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->dp_link, "dp_link");
> + if (rc) {
> + pr_err("unable to remap dp io resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->dp_p0, "dp_p0");
> + if (rc) {
> + pr_err("unable to remap dp io resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->phy_io, "dp_phy");
> + if (rc) {
> + pr_err("unable to remap dp PHY resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->ln_tx0_io, "dp_ln_tx0");
> + if (rc) {
> + pr_err("unable to remap dp TX0 resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->ln_tx1_io, "dp_ln_tx1");
> + if (rc) {
> + pr_err("unable to remap dp TX1 resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->dp_pll_io, "dp_pll");
> + if (rc) {
> + pr_err("unable to remap DP PLL resources\n");
> + goto err;
> + }
> +
> + rc = msm_dss_ioremap_byname(pdev, &io->usb3_dp_com, "usb3_dp_com");
> + if (rc) {
> + pr_err("unable to remap USB3 DP com resources\n");
> + goto err;
> + }
> +
> + if (msm_dss_ioremap_byname(pdev, &io->dp_cc_io, "dp_mmss_cc")) {
> + pr_err("unable to remap dp MMSS_CC resources\n");
> + goto err;
> + }
> +
> + if (msm_dss_ioremap_byname(pdev, &io->qfprom_io, "qfprom_physical"))
> + pr_warn("unable to remap dp qfprom resources\n");
> +
It doesn't look like you are using this in the current code so you can remove it
but if you did need to read fuses use nvram_cell instead.
> + return 0;
> +err:
> + dp_parser_unmap_io_resources(parser);
> + return rc;
> +}
This whole function is crying out for an array:
struct {
void __iomem **base;
const char *name;
} regions[] = {
{ &io->dp_aux.base, "dp_aux" },
...
{ &io->dp_cc_io.base, "dp_mms_cc" },
{ NULL, NULL }
};
for(i = 0; regions[i].name; i++) {
void __iomem *ptr = msm_ioremap(pdev, regions[i].name, NULL);
if (IS_ERR(ptr))
return -ENODEV;
*(regions[i].base) = ptr;
}
return 0;
> +
> +static const char *dp_get_phy_aux_config_property(u32 cfg_type)
> +{
> + switch (cfg_type) {
> + case PHY_AUX_CFG0:
> + return "qcom,aux-cfg0-settings";
> + case PHY_AUX_CFG1:
> + return "qcom,aux-cfg1-settings";
> + case PHY_AUX_CFG2:
> + return "qcom,aux-cfg2-settings";
> + case PHY_AUX_CFG3:
> + return "qcom,aux-cfg3-settings";
> + case PHY_AUX_CFG4:
> + return "qcom,aux-cfg4-settings";
> + case PHY_AUX_CFG5:
> + return "qcom,aux-cfg5-settings";
> + case PHY_AUX_CFG6:
> + return "qcom,aux-cfg6-settings";
> + case PHY_AUX_CFG7:
> + return "qcom,aux-cfg7-settings";
> + case PHY_AUX_CFG8:
> + return "qcom,aux-cfg8-settings";
> + case PHY_AUX_CFG9:
> + return "qcom,aux-cfg9-settings";
> + default:
> + return "unknown";
> + }
> +}
Since CFG0 is 0 and CFG1 is and so on instead of calling
a function you can just construct the string inline when you need it....
> +static void dp_parser_phy_aux_cfg_reset(struct dp_parser *parser)
> +{
> + int i = 0;
> +
> + for (i = 0; i < PHY_AUX_CFG_MAX; i++)
> + parser->aux_cfg[i] = (const struct dp_aux_cfg){ 0 };
> +}
> +
> +static int dp_parser_aux(struct dp_parser *parser)
> +{
> + struct device_node *of_node = parser->pdev->dev.of_node;
> + int len = 0, i = 0, j = 0, config_count = 0;
> + const char *data;
> + int const minimum_config_count = 1;
> +
> + for (i = 0; i < PHY_AUX_CFG_MAX; i++) {
> + const char *property = dp_get_phy_aux_config_property(i);
Here, instead of this, do
char property[24];
snprintf(property, sizeof(property), "qcom,aux-cfg%d-settings", i);
> + data = of_get_property(of_node, property, &len);
> + if (!data) {
> + pr_err("Unable to read %s\n", property);
> + goto error;
> + }
> +
> + config_count = len - 1;
> + if ((config_count < minimum_config_count) ||
> + (config_count > DP_AUX_CFG_MAX_VALUE_CNT)) {
> + pr_err("Invalid config count (%d) configs for %s\n",
> + config_count, property);
> + goto error;
> + }
> +
> + parser->aux_cfg[i].offset = data[0];
> + parser->aux_cfg[i].cfg_cnt = config_count;
> + pr_debug("%s offset=0x%x, cfg_cnt=%d\n",
> + property,
> + parser->aux_cfg[i].offset,
> + parser->aux_cfg[i].cfg_cnt);
> + for (j = 1; j < len; j++) {
> + parser->aux_cfg[i].lut[j - 1] = data[j];
> + pr_debug("%s lut[%d]=0x%x\n",
> + property,
> + i,
> + parser->aux_cfg[i].lut[j - 1]);
> + }
qcom,aux-cfgN-settings appears to be an array. Seems like it would be easier
just to use of_property_read_u32_array()? Seems like it would save yourself a
bit of work.
> + }
> + return 0;
> +
> +error:
> + dp_parser_phy_aux_cfg_reset(parser);
> + return -EINVAL;
> +}
> +
> +static int dp_parser_misc(struct dp_parser *parser)
> +{
> + int rc = 0;
> + struct device_node *of_node = parser->pdev->dev.of_node;
> +
> + rc = of_property_read_u32(of_node,
> + "qcom,max-pclk-frequency-khz", &parser->max_pclk_khz);
> + if (rc)
> + parser->max_pclk_khz = DP_MAX_PIXEL_CLK_KHZ;
On error, of_property_read_u32 and friends will not touch the passed in pointer,
so you can safely set the pointer to a default and then not worry about checking
the value of the function - so this can just turn into:
parser->max_pclk_hz = DP_MAX_PIXEL_CLK_KHZ;
of_property_read_u32(of_node, "qcom,max-pclk-frequency-khz",
&parser->max_pclk_khz);
> +
> + return 0;
This function doesn't need to return anything. And technically, it doesn't need
to be a function, but the place where it gets called is entirely comprised of
functions so thats probably not a huge deal.
> +}
> +
> +static int dp_parser_pinctrl(struct dp_parser *parser)
> +{
> + int rc = 0;
> + struct dp_pinctrl *pinctrl = &parser->pinctrl;
> +
> + pinctrl->pin = devm_pinctrl_get(&parser->pdev->dev);
> +
> + if (IS_ERR_OR_NULL(pinctrl->pin)) {
It looks to me like pinctrl_get only returns ERR_PTR so you can just use
IS_ERR() for your check.
> + rc = PTR_ERR(pinctrl->pin);
> + pr_err("failed to get pinctrl, rc=%d\n", rc);
And you can just return PTR_ERR(pinctrl->pin) here
> + goto error;
> + }
> +
> + pinctrl->state_active = pinctrl_lookup_state(pinctrl->pin,
> + "mdss_dp_active");
> + if (IS_ERR_OR_NULL(pinctrl->state_active)) {
Same.
> + rc = PTR_ERR(pinctrl->state_active);
> + pr_err("failed to get pinctrl active state, rc=%d\n", rc);
> + goto error;
> + }
> +
> + pinctrl->state_suspend = pinctrl_lookup_state(pinctrl->pin,
> + "mdss_dp_sleep");
> + if (IS_ERR_OR_NULL(pinctrl->state_suspend)) {
Same
> + rc = PTR_ERR(pinctrl->state_suspend);
> + pr_err("failed to get pinctrl suspend state, rc=%d\n", rc);
> + goto error;
> + }
> +error:
> + return rc;
You don't need this label if you return as soon as you get the error.
> +}
> +
> +static int dp_parser_gpio(struct dp_parser *parser)
> +{
> + int i = 0;
> + struct device *dev = &parser->pdev->dev;
> + struct device_node *of_node = dev->of_node;
> + struct dss_module_power *mp = &parser->mp[DP_CORE_PM];
> + static const char * const dp_gpios[] = {
> + "qcom,aux-en-gpio",
> + "qcom,aux-sel-gpio",
> + "qcom,usbplug-cc-gpio",
> + };
> +
> + mp->gpio_config = devm_kzalloc(dev,
> + sizeof(struct dss_gpio) * ARRAY_SIZE(dp_gpios), GFP_KERNEL);
> + if (!mp->gpio_config)
> + return -ENOMEM;
> +
> + mp->num_gpio = ARRAY_SIZE(dp_gpios);
> +
> + for (i = 0; i < ARRAY_SIZE(dp_gpios); i++) {
> + mp->gpio_config[i].gpio = of_get_named_gpio(of_node,
> + dp_gpios[i], 0);
> +
> + if (!gpio_is_valid(mp->gpio_config[i].gpio)) {
> + pr_err("%s gpio not specified\n", dp_gpios[i]);
> + return -EINVAL;
> + }
> +
> + strlcpy(mp->gpio_config[i].gpio_name, dp_gpios[i],
> + sizeof(mp->gpio_config[i].gpio_name));
> +
> + mp->gpio_config[i].value = 0;
> + }
> +
> + return 0;
> +}
> +
> +static const char *dp_parser_supply_node_name(enum dp_pm_type module)
> +{
> + switch (module) {
> + case DP_CORE_PM: return "qcom,core-supply-entries";
> + case DP_CTRL_PM: return "qcom,ctrl-supply-entries";
> + case DP_PHY_PM: return "qcom,phy-supply-entries";
> + default: return "???";
This isn't possible. At the very least return NULL and give you self a chance
to detect the error - otherwise you'll be returning odd error messages with ????
in it that would be harder to debug.
> + }
> +}
> +
> +static int dp_parser_get_vreg(struct dp_parser *parser,
> + enum dp_pm_type module)
> +{
> + int i = 0, rc = 0;
> + u32 tmp = 0;
> + const char *pm_supply_name = NULL;
> + struct device_node *supply_node = NULL;
> + struct device_node *of_node = parser->pdev->dev.of_node;
> + struct device_node *supply_root_node = NULL;
> + struct dss_module_power *mp = &parser->mp[module];
> +
> + mp->num_vreg = 0;
> + pm_supply_name = dp_parser_supply_node_name(module);
> + supply_root_node = of_get_child_by_name(of_node, pm_supply_name);
> + if (!supply_root_node) {
> + pr_err("no supply entry present: %s\n", pm_supply_name);
> + goto novreg;
> + }
> +
> + mp->num_vreg = of_get_available_child_count(supply_root_node);
> +
> + if (mp->num_vreg == 0) {
> + pr_debug("no vreg\n");
> + goto novreg;
> + } else {
> + pr_debug("vreg found. count=%d\n", mp->num_vreg);
> + }
> +
> + mp->vreg_config = devm_kzalloc(&parser->pdev->dev,
> + sizeof(struct dss_vreg) * mp->num_vreg, GFP_KERNEL);
> + if (!mp->vreg_config) {
> + rc = -ENOMEM;
> + goto error;
> + }
> +
> + for_each_child_of_node(supply_root_node, supply_node) {
> + const char *st = NULL;
> + /* vreg-name */
> + rc = of_property_read_string(supply_node,
> + "qcom,supply-name", &st);
> + if (rc) {
> + pr_err("error reading name. rc=%d\n",
> + rc);
> + goto error;
> + }
> + snprintf(mp->vreg_config[i].vreg_name,
> + ARRAY_SIZE((mp->vreg_config[i].vreg_name)), "%s", st);
ARRAY_SIZE works for a string, but we prefer sizeof() in most cases. If you
are just copying a string, consider using strncpy() and friends. It will be
faster than snprintf.
> + /* vreg-min-voltage */
> + rc = of_property_read_u32(supply_node,
> + "qcom,supply-min-voltage", &tmp);
As mentioned above, on error they don't touch the value of the pointer so you
don't need to use a temporary variable.
> + if (rc) {
> + pr_err("error reading min volt. rc=%d\n",
> + rc);
> + goto error;
> + }
> + mp->vreg_config[i].min_voltage = tmp;
> +
> + /* vreg-max-voltage */
> + rc = of_property_read_u32(supply_node,
> + "qcom,supply-max-voltage", &tmp);
> + if (rc) {
> + pr_err("error reading max volt. rc=%d\n",
> + rc);
> + goto error;
> + }
> + mp->vreg_config[i].max_voltage = tmp;
> +
> + /* enable-load */
> + rc = of_property_read_u32(supply_node,
> + "qcom,supply-enable-load", &tmp);
> + if (rc) {
> + pr_err("error reading enable load. rc=%d\n",
> + rc);
> + goto error;
> + }
> + mp->vreg_config[i].enable_load = tmp;
> +
> + /* disable-load */
> + rc = of_property_read_u32(supply_node,
> + "qcom,supply-disable-load", &tmp);
> + if (rc) {
> + pr_err("error reading disable load. rc=%d\n",
> + rc);
> + goto error;
> + }
> + mp->vreg_config[i].disable_load = tmp;
> +
> + pr_debug("%s min=%d, max=%d, enable=%d, disable=%d\n",
> + mp->vreg_config[i].vreg_name,
> + mp->vreg_config[i].min_voltage,
> + mp->vreg_config[i].max_voltage,
> + mp->vreg_config[i].enable_load,
> + mp->vreg_config[i].disable_load
> + );
> + ++i;
> + }
> +
> + return rc;
> +
> +error:
> + if (mp->vreg_config) {
You don't need to check for NULL here.
> + devm_kfree(&parser->pdev->dev, mp->vreg_config);
> + mp->vreg_config = NULL;
> + }
> +novreg:
> + mp->num_vreg = 0;
> +
> + return rc;
> +}
> +
> +static void dp_parser_put_vreg_data(struct device *dev,
> + struct dss_module_power *mp)
> +{
> + if (!mp) {
> + DEV_ERR("invalid input\n");
This error message isn't useful when you are taking down the device - just
quietly skip.
> + return;
> + }
> +
> + if (mp->vreg_config) {
> + devm_kfree(dev, mp->vreg_config);
You don't need to kfree managed memory.
> + mp->vreg_config = NULL;
> + }
> + mp->num_vreg = 0;
> +}
> +
> +static int dp_parser_regulator(struct dp_parser *parser)
> +{
> + int i, rc = 0;
> + struct platform_device *pdev = parser->pdev;
> +
> + /* Parse the regulator information */
> + for (i = DP_CORE_PM; i < DP_MAX_PM; i++) {
> + rc = dp_parser_get_vreg(parser, i);
> + if (rc) {
> + pr_err("get_dt_vreg_data failed for %s. rc=%d\n",
> + dp_parser_pm_name(i), rc);
dp_parser_get_vreg() is very loud about errors - you don't need to pile on.
> + i--;
> + for (; i >= DP_CORE_PM; i--)
> + dp_parser_put_vreg_data(&pdev->dev,
> + &parser->mp[i]);
> + break;
> + }
> + }
> +
> + return rc;
> +}
> +
> +static bool dp_parser_check_prefix(const char *clk_prefix, const char
> *clk_name)
> +{
> + return !!strnstr(clk_name, clk_prefix, strlen(clk_name));
This isn't saving you any lines of code - you can do the string compare
directly in the calling functions.
> +}
> +
> +static void dp_parser_put_clk_data(struct device *dev,
> + struct dss_module_power *mp)
> +{
> + if (!mp) {
> + DEV_ERR("%s: invalid input\n", __func__);
You are taking down the device so you don't care if anything is invalid.
> + return;
> + }
> +
> + if (mp->clk_config) {
> + devm_kfree(dev, mp->clk_config);
You don't need to free managed device memory.
> + mp->clk_config = NULL;
> + }
> +
> + mp->num_clk = 0;
> +}
> +
> +static void dp_parser_put_gpio_data(struct device *dev,
> + struct dss_module_power *mp)
> +{
> + if (!mp) {
> + DEV_ERR("%s: invalid input\n", __func__);
As above.
> + return;
> + }
> +
> + if (mp->gpio_config) {
> + devm_kfree(dev, mp->gpio_config);
Also as above.
> + mp->gpio_config = NULL;
> + }
> +
> + mp->num_gpio = 0;
> +}
> +
> +static int dp_parser_init_clk_data(struct dp_parser *parser)
> +{
> + int num_clk = 0, i = 0, rc = 0;
> + int core_clk_count = 0, ctrl_clk_count = 0;
> + const char *core_clk = "core";
> + const char *ctrl_clk = "ctrl";
> + const char *clk_name;
> + struct device *dev = &parser->pdev->dev;
> + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> +
> + num_clk = of_property_count_strings(dev->of_node, "clock-names");
> + if (num_clk <= 0) {
> + pr_err("no clocks are defined\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + for (i = 0; i < num_clk; i++) {
> + of_property_read_string_index(dev->of_node,
> + "clock-names", i, &clk_name);
> +
> + if (dp_parser_check_prefix(core_clk, clk_name))
> + core_clk_count++;
According to your bindings, 'core' or 'ctrl' will be the prefix for the clock,
so why are you using strnstr? strncmp works just as well.
!strncmp("core", clk_name, 4)
> +
> + if (dp_parser_check_prefix(ctrl_clk, clk_name))
if (!strncmp("ctrl", clk_name, 4)
> + ctrl_clk_count++;
> + }
> +
> + /* Initialize the CORE power module */
> + if (core_clk_count <= 0) {
core_clk_count clearly can't be less than zero.
> + pr_err("no core clocks are defined\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + core_power->num_clk = core_clk_count;
> + core_power->clk_config = devm_kzalloc(dev,
> + sizeof(struct dss_clk) * core_power->num_clk,
> + GFP_KERNEL);
> + if (!core_power->clk_config) {
> + rc = -EINVAL;
the return value should be -ENOMEM here.
> + goto exit;
> + }
> +
> + /* Initialize the CTRL power module */
> + if (ctrl_clk_count <= 0) {
Again, this clearly can't be less than zero.
> + pr_err("no ctrl clocks are defined\n");
> + rc = -EINVAL;
> + goto ctrl_clock_error;
> + }
> +
> + ctrl_power->num_clk = ctrl_clk_count;
> + ctrl_power->clk_config = devm_kzalloc(dev,
> + sizeof(struct dss_clk) * ctrl_power->num_clk,
> + GFP_KERNEL);
> + if (!ctrl_power->clk_config) {
> + ctrl_power->num_clk = 0;
> + rc = -EINVAL;
And -ENOMEM again.
> + goto ctrl_clock_error;
> + }
> +
> + return rc;
> +
> +ctrl_clock_error:
> + dp_parser_put_clk_data(dev, core_power);
> +exit:
> + return rc;
Don't use this label - just return the error code directly instead.
> +}
> +
> +static int dp_parser_clock(struct dp_parser *parser)
> +{
> + int rc = 0, i = 0;
> + int num_clk = 0;
> + int core_clk_index = 0, ctrl_clk_index = 0;
> + int core_clk_count = 0, ctrl_clk_count = 0;
> + const char *clk_name;
> + const char *core_clk = "core";
> + const char *ctrl_clk = "ctrl";
> + struct device *dev = &parser->pdev->dev;
> + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> +
> + core_power = &parser->mp[DP_CORE_PM];
> + ctrl_power = &parser->mp[DP_CTRL_PM];
> +
> + rc = dp_parser_init_clk_data(parser);
> + if (rc) {
> + pr_err("failed to initialize power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + core_clk_count = core_power->num_clk;
> + ctrl_clk_count = ctrl_power->num_clk;
> +
> + num_clk = core_clk_count + ctrl_clk_count;
> +
According to your bindings, not every clock in your list is a core or a ctrl -
are those clocks ignored?
> + for (i = 0; i < num_clk; i++) {
> + of_property_read_string_index(dev->of_node, "clock-names",
> + i, &clk_name);
This implies that a core_ or ctrl_ clock are first in the list. Your bindings
do not specify an order.
> +
> + if (dp_parser_check_prefix(core_clk, clk_name) &&
Again, you should use strncmp() here
> + core_clk_index < core_clk_count) {
If you already went to the trouble of counting you can be pretty sure that
you won't overrun your array.
> + struct dss_clk *clk =
> + &core_power->clk_config[core_clk_index];
> + strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
> + clk->type = DSS_CLK_AHB;
> + core_clk_index++;
> + } else if (dp_parser_check_prefix(ctrl_clk, clk_name) &&
> + ctrl_clk_index < ctrl_clk_count) {
You aren't going to overrun your array.
> + struct dss_clk *clk =
> + &ctrl_power->clk_config[ctrl_clk_index];
> + strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
> + ctrl_clk_index++;
> +
> + if (!strcmp(clk_name, "ctrl_link_clk") ||
> + !strcmp(clk_name, "ctrl_pixel_clk"))
> + clk->type = DSS_CLK_PCLK;
> + else
> + clk->type = DSS_CLK_AHB;
> + }
> + }
There is a lot going on in these two functions but I feel they can be combined.
The number of clocks in each array shouldn't be a mystery - you can
pre-allocate or statically allocate the arrays. That will save you from the
initial step of walking the list to allocate the memory. Then you can have a
single walk of the list, checking the clock type and handle it accordingly.
> +
> + pr_debug("clock parsing successful\n");
> +
> +exit:
> + return rc;
> +}
> +
> +static int dp_parser_parse(struct dp_parser *parser)
> +{
> + int rc = 0;
> +
> + if (!parser) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto err;
> + }
>
parser can never be null here.
> + rc = dp_parser_ctrl_res(parser);
> + if (rc)
> + goto err;
You can just return rc here and throughout without the label.
> +
> + rc = dp_parser_aux(parser);
> + if (rc)
> + goto err;
> +
> + rc = dp_parser_misc(parser);
> + if (rc)
> + goto err;
> +
> + rc = dp_parser_clock(parser);
> + if (rc)
> + goto err;
> +
> + rc = dp_parser_regulator(parser);
> + if (rc)
> + goto err;
> +
> + rc = dp_parser_gpio(parser);
> + if (rc)
> + goto err;
> +
> + rc = dp_parser_pinctrl(parser);
> +err:
> + return rc;
> +}
> +
> +struct dp_parser *dp_parser_get(struct platform_device *pdev)
> +{
> + struct dp_parser *parser;
> +
> + parser = devm_kzalloc(&pdev->dev, sizeof(*parser), GFP_KERNEL);
> + if (!parser)
> + return ERR_PTR(-ENOMEM);
> +
> + parser->parse = dp_parser_parse;
> + parser->pdev = pdev;
> +
> + return parser;
> +}
> +
> +void dp_parser_put(struct dp_parser *parser)
> +{
> + int i = 0;
> + struct dss_module_power *power = NULL;
> +
> + if (!parser) {
> + pr_err("invalid parser module\n");
If parser is null, you don't care. Just quietly exit.
> + return;
> + }
> +
> + power = parser->mp;
> +
> + for (i = 0; i < DP_MAX_PM; i++) {
> + dp_parser_put_clk_data(&parser->pdev->dev, &power[i]);
> + dp_parser_put_vreg_data(&parser->pdev->dev, &power[i]);
> + dp_parser_put_gpio_data(&parser->pdev->dev, &power[i]);
> + }
> +
> + devm_kfree(&parser->pdev->dev, parser);
You don't need to free managed device memory.
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
> b/drivers/gpu/drm/msm/dp/dp_parser.h
> new file mode 100644
> index 0000000..b39ea02
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -0,0 +1,205 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please
> +#ifndef _DP_PARSER_H_
> +#define _DP_PARSER_H_
> +
> +#include "dpu_io_util.h"
> +
> +#define DP_LABEL "MDSS DP DISPLAY"
> +#define AUX_CFG_LEN 10
> +#define DP_MAX_PIXEL_CLK_KHZ 675000
> +
> +enum dp_pm_type {
> + DP_CORE_PM,
> + DP_CTRL_PM,
> + DP_PHY_PM,
> + DP_MAX_PM
> +};
> +
> +static inline const char *dp_parser_pm_name(enum dp_pm_type module)
> +{
> + switch (module) {
> + case DP_CORE_PM: return "DP_CORE_PM";
> + case DP_CTRL_PM: return "DP_CTRL_PM";
> + case DP_PHY_PM: return "DP_PHY_PM";
> + default: return "???";
You go out of your way to make sure that the default case can't happen here.
also this function is only used once, I don't think it needs to be a static
inline in the header.
> + }
> +}
> +
> +/**
> + * struct dp_display_data - display related device tree data.
> + *
> + * @ctrl_node: referece to controller device
> + * @phy_node: reference to phy device
> + * @is_active: is the controller currently active
> + * @name: name of the display
> + * @display_type: type of the display
> + */
> +struct dp_display_data {
> + struct device_node *ctrl_node;
> + struct device_node *phy_node;
> + bool is_active;
> + const char *name;
> + const char *display_type;
> +};
> +
> +/**
> + * struct dp_ctrl_resource - controller's IO related data
> + *
> + * @dp_ahb: controller's ahb mapped memory address
> + * @dp_aux: controller's aux mapped memory address
> + * @dp_link: controller's link mapped memory address
> + * @dp_p0: controller's p0 mapped memory address
> + * @phy_io: phy's mapped memory address
> + * @ln_tx0_io: USB-DP lane TX0's mapped memory address
> + * @ln_tx1_io: USB-DP lane TX1's mapped memory address
> + * @dp_cc_io: DP cc's mapped memory address
> + * @qfprom_io: qfprom's mapped memory address
> + * @dp_pll_io: DP PLL mapped memory address
> + * @usb3_dp_com: USB3 DP PHY combo mapped memory address
> + */
> +struct dp_io {
> + struct dss_io_data ctrl_io;
> + struct dss_io_data dp_ahb;
> + struct dss_io_data dp_aux;
> + struct dss_io_data dp_link;
> + struct dss_io_data dp_p0;
> + struct dss_io_data phy_io;
> + struct dss_io_data ln_tx0_io;
> + struct dss_io_data ln_tx1_io;
> + struct dss_io_data dp_cc_io;
> + struct dss_io_data qfprom_io;
> + struct dss_io_data dp_pll_io;
> + struct dss_io_data usb3_dp_com;
> +};
> +
> +/**
> + * struct dp_pinctrl - DP's pin control
> + *
> + * @pin: pin-controller's instance
> + * @state_active: active state pin control
> + * @state_hpd_active: hpd active state pin control
> + * @state_suspend: suspend state pin control
> + */
> +struct dp_pinctrl {
> + struct pinctrl *pin;
> + struct pinctrl_state *state_active;
> + struct pinctrl_state *state_hpd_active;
> + struct pinctrl_state *state_suspend;
> +};
> +
> +#define DP_ENUM_STR(x) #x
> +#define DP_AUX_CFG_MAX_VALUE_CNT 3
> +/**
> + * struct dp_aux_cfg - DP's AUX configuration settings
> + *
> + * @cfg_cnt: count of the configurable settings for the AUX register
> + * @current_index: current index of the AUX config lut
> + * @offset: register offset of the AUX config register
> + * @lut: look up table for the AUX config values for this register
> + */
> +struct dp_aux_cfg {
> + u32 cfg_cnt;
> + u32 current_index;
> + u32 offset;
> + u32 lut[DP_AUX_CFG_MAX_VALUE_CNT];
> +};
> +
> +/* PHY AUX config registers */
> +enum dp_phy_aux_config_type {
> + PHY_AUX_CFG0,
> + PHY_AUX_CFG1,
> + PHY_AUX_CFG2,
> + PHY_AUX_CFG3,
> + PHY_AUX_CFG4,
> + PHY_AUX_CFG5,
> + PHY_AUX_CFG6,
> + PHY_AUX_CFG7,
> + PHY_AUX_CFG8,
> + PHY_AUX_CFG9,
> + PHY_AUX_CFG_MAX,
> +};
> +
> +static inline char *dp_phy_aux_config_type_to_string(u32 cfg_type)
> +{
> + switch (cfg_type) {
> + case PHY_AUX_CFG0:
> + return DP_ENUM_STR(PHY_AUX_CFG0);
> + case PHY_AUX_CFG1:
> + return DP_ENUM_STR(PHY_AUX_CFG1);
> + case PHY_AUX_CFG2:
> + return DP_ENUM_STR(PHY_AUX_CFG2);
> + case PHY_AUX_CFG3:
> + return DP_ENUM_STR(PHY_AUX_CFG3);
> + case PHY_AUX_CFG4:
> + return DP_ENUM_STR(PHY_AUX_CFG4);
> + case PHY_AUX_CFG5:
> + return DP_ENUM_STR(PHY_AUX_CFG5);
> + case PHY_AUX_CFG6:
> + return DP_ENUM_STR(PHY_AUX_CFG6);
> + case PHY_AUX_CFG7:
> + return DP_ENUM_STR(PHY_AUX_CFG7);
> + case PHY_AUX_CFG8:
> + return DP_ENUM_STR(PHY_AUX_CFG8);
> + case PHY_AUX_CFG9:
> + return DP_ENUM_STR(PHY_AUX_CFG9);
> + default:
> + return "unknown";
> + }
> +}
> +
> +/**
> + * struct dp_parser - DP parser's data exposed to clients
> + *
> + * @pdev: platform data of the client
> + * @mp: gpio, regulator and clock related data
> + * @pinctrl: pin-control related data
> + * @disp_data: controller's display related data
> + * @parse: function to be called by client to parse device tree.
> + */
> +struct dp_parser {
> + struct platform_device *pdev;
> + struct dss_module_power mp[DP_MAX_PM];
> + struct dp_pinctrl pinctrl;
> + struct dp_io io;
> + struct dp_display_data disp_data;
> +
> + u8 l_map[4];
> + struct dp_aux_cfg aux_cfg[AUX_CFG_LEN];
> + u32 max_pclk_khz;
> +
> + int (*parse)(struct dp_parser *parser);
> +};
> +
> +/**
> + * dp_parser_get() - get the DP's device tree parser module
> + *
> + * @pdev: platform data of the client
> + * return: pointer to dp_parser structure.
> + *
> + * This function provides client capability to parse the
> + * device tree and populate the data structures. The data
> + * related to clock, regulators, pin-control and other
> + * can be parsed using this module.
> + */
> +struct dp_parser *dp_parser_get(struct platform_device *pdev);
> +
> +/**
> + * dp_parser_put() - cleans the dp_parser module
> + *
> + * @parser: pointer to the parser's data.
> + */
> +void dp_parser_put(struct dp_parser *parser);
> +#endif
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
> b/drivers/gpu/drm/msm/dp/dp_power.c
> new file mode 100644
> index 0000000..1d58480
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -0,0 +1,599 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +#define pr_fmt(fmt) "[drm-dp] %s: " fmt, __func__
> +
> +#include <linux/clk.h>
> +#include "dp_power.h"
> +
> +#define DP_CLIENT_NAME_SIZE 20
> +
> +struct dp_power_private {
> + struct dp_parser *parser;
> + struct platform_device *pdev;
> + struct clk *pixel_clk_rcg;
> + struct clk *pixel_parent;
> +
> + struct dp_power dp_power;
> +
> + bool core_clks_on;
> + bool link_clks_on;
> +};
> +
> +static int dp_power_regulator_init(struct dp_power_private *power)
> +{
> + int rc = 0, i = 0, j = 0;
> + struct platform_device *pdev;
> + struct dp_parser *parser;
> +
> + parser = power->parser;
> + pdev = power->pdev;
> +
> + for (i = DP_CORE_PM; !rc && (i < DP_MAX_PM); i++) {
> + rc = msm_dss_config_vreg(&pdev->dev,
> + parser->mp[i].vreg_config,
> + parser->mp[i].num_vreg, 1);
> + if (rc) {
> + pr_err("failed to init vregs for %s\n",
> + dp_parser_pm_name(i));
msm_dss_config_vreg() already prints lots of error messages for you.
> + for (j = i - 1; j >= DP_CORE_PM; j--) {
> + msm_dss_config_vreg(&pdev->dev,
> + parser->mp[j].vreg_config,
> + parser->mp[j].num_vreg, 0);
> + }
> +
> + goto error;
just return rc;
> + }
> + }
> +error:
> + return rc;
> +}
> +
> +static void dp_power_regulator_deinit(struct dp_power_private *power)
> +{
> + int rc = 0, i = 0;
> + struct platform_device *pdev;
> + struct dp_parser *parser;
> +
> + parser = power->parser;
> + pdev = power->pdev;
> +
> + for (i = DP_CORE_PM; (i < DP_MAX_PM); i++) {
> + rc = msm_dss_config_vreg(&pdev->dev,
> + parser->mp[i].vreg_config,
> + parser->mp[i].num_vreg, 0);
> + if (rc)
> + pr_err("failed to deinit vregs for %s\n",
> + dp_parser_pm_name(i));
Same.
> + }
> +}
> +
> +static int dp_power_regulator_ctrl(struct dp_power_private *power, bool
> enable)
> +{
> + int rc = 0, i = 0, j = 0;
> + struct dp_parser *parser;
> +
> + parser = power->parser;
> +
> + for (i = DP_CORE_PM; i < DP_MAX_PM; i++) {
> + rc = msm_dss_enable_vreg(
> + parser->mp[i].vreg_config,
> + parser->mp[i].num_vreg, enable);
> + if (rc) {
> + pr_err("failed to '%s' vregs for %s\n",
> + enable ? "enable" : "disable",
> + dp_parser_pm_name(i));
msm_dss_enable_vreg() already gives you the errors you need.
> + if (enable) {
> + for (j = i-1; j >= DP_CORE_PM; j--) {
> + msm_dss_enable_vreg(
> + parser->mp[j].vreg_config,
> + parser->mp[j].num_vreg, 0);
> + }
> + }
> + goto error;
> + }
> + }
> +error:
> + return rc;
You don't need a label with a single return in it.
> +}
> +
> +static int dp_power_pinctrl_set(struct dp_power_private *power, bool active)
> +{
> + int rc = -EFAULT;
> + struct pinctrl_state *pin_state;
> + struct dp_parser *parser;
> +
> + parser = power->parser;
> +
> + if (IS_ERR_OR_NULL(parser->pinctrl.pin))
Recall that pinctrl.in cannot be NULL.
> + return PTR_ERR(parser->pinctrl.pin);
> +
> + pin_state = active ? parser->pinctrl.state_active
> + : parser->pinctrl.state_suspend;
> + if (!IS_ERR_OR_NULL(pin_state)) {
Same
> + rc = pinctrl_select_state(parser->pinctrl.pin,
> + pin_state);
> + if (rc)
> + pr_err("can not set %s pins\n",
> + active ? "dp_active"
> + : "dp_sleep");
> + } else {
> + pr_err("invalid '%s' pinstate\n",
> + active ? "dp_active"
> + : "dp_sleep");
You already let the user know during inint that the pin_state was not
correct. You don't need to update them again every time you try to change it.
> + }
> +
> + return rc;
> +}
> +
> +static int dp_power_clk_init(struct dp_power_private *power, bool enable)
> +{
> + int rc = 0;
> + struct dss_module_power *core, *ctrl;
> + struct device *dev;
> +
> + core = &power->parser->mp[DP_CORE_PM];
> + ctrl = &power->parser->mp[DP_CTRL_PM];
> +
> + dev = &power->pdev->dev;
> +
> + if (!core || !ctrl) {
> + pr_err("invalid power_data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
These are both impossible since you are deriving the pointer above.
> +
> + if (enable) {
> + rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
> + if (rc) {
> + pr_err("failed to get %s clk. err=%d\n",
> + dp_parser_pm_name(DP_CORE_PM), rc);
msm_dss_get_clk already prints an error message for you.
> + goto exit;
> + }
> +
> + rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
> + if (rc) {
> + pr_err("failed to get %s clk. err=%d\n",
> + dp_parser_pm_name(DP_CTRL_PM), rc);
Same
> + goto ctrl_get_error;
> + }
> +
> + power->pixel_clk_rcg = devm_clk_get(dev, "pixel_clk_rcg");
> + if (IS_ERR(power->pixel_clk_rcg)) {
> + pr_debug("Unable to get DP pixel clk RCG\n");
This seems like it should be an error?
> + power->pixel_clk_rcg = NULL;
> + }
> +
> + power->pixel_parent = devm_clk_get(dev, "pixel_parent");
> + if (IS_ERR(power->pixel_parent)) {
> + pr_debug("Unable to get DP pixel RCG parent\n");
Same?
> + power->pixel_parent = NULL;
> + }
> + } else {
This is another one of those functions that shares very little with the other
side and can be easily split into a separate function that is easier to
understand.
> + if (power->pixel_parent)
> + devm_clk_put(dev, power->pixel_parent);
> +
> + if (power->pixel_clk_rcg)
> + devm_clk_put(dev, power->pixel_clk_rcg);
You don't need to put managed clocks.
> +
> + msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
> + msm_dss_put_clk(core->clk_config, core->num_clk);
> + }
> +
> + return rc;
> +
> +ctrl_get_error:
> + msm_dss_put_clk(core->clk_config, core->num_clk);
> +exit:
> + return rc;
> +}
> +
> +static int dp_power_clk_set_rate(struct dp_power_private *power,
> + enum dp_pm_type module, bool enable)
> +{
> + int rc = 0;
> + struct dss_module_power *mp;
> +
> + if (!power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
power will always be valid.
> + mp = &power->parser->mp[module];
> +
> + if (enable) {
> + rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
> + if (rc) {
> + pr_err("failed to set clks rate.\n");
msm_dss_clk_set_rate() already gives you an error message;
> + goto exit;
> + }
> +
> + rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, 1);
> + if (rc) {
> + pr_err("failed to enable clks\n");
Same.
> + goto exit;
> + }
> + } else {
> + rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, 0);
> + if (rc) {
> + pr_err("failed to disable clks\n");
Same
> + goto exit;
Suspect indentation, but as usual, don't use a single return in a label - just
return directly.
> + }
> + }
> +exit:
> + return rc;
> +}
> +
> +static int dp_power_clk_enable(struct dp_power *dp_power,
> + enum dp_pm_type pm_type, bool enable)
> +{
> + int rc = 0;
> + struct dss_module_power *mp;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto error;
> + }
dp_power will always be valid.
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + mp = &power->parser->mp[pm_type];
> +
> + if ((pm_type != DP_CORE_PM) && (pm_type != DP_CTRL_PM)) {
> + pr_err("unsupported power module: %s\n",
> + dp_parser_pm_name(pm_type));
This seems like it would be never be true outside of egregious developer
error. - I suppose you could WARN_ON here but it just doesn't seem like a thing.
> + return -EINVAL;
> + }
> +
> + if (enable) {
> + if ((pm_type == DP_CORE_PM)
> + && (power->core_clks_on)) {
> + pr_debug("core clks already enabled\n");
> + return 0;
> + }
> +
> + if ((pm_type == DP_CTRL_PM)
> + && (power->link_clks_on)) {
> + pr_debug("links clks already enabled\n");
> + return 0;
> + }
> +
> + if ((pm_type == DP_CTRL_PM) && (!power->core_clks_on)) {
> + pr_debug("Need to enable core clks before link clks\n");
> +
> + rc = dp_power_clk_set_rate(power, DP_CORE_PM, enable);
> + if (rc) {
> + pr_err("failed to enable clks: %s. err=%d\n",
> + dp_parser_pm_name(DP_CORE_PM), rc);
> + goto error;
> + } else {
> + power->core_clks_on = true;
> + }
> + }
> + }
> +
> + rc = dp_power_clk_set_rate(power, pm_type, enable);
> + if (rc) {
> + pr_err("failed to '%s' clks for: %s. err=%d\n",
> + enable ? "enable" : "disable",
> + dp_parser_pm_name(pm_type), rc);
> + goto error;
> + }
> +
> + if (pm_type == DP_CORE_PM)
> + power->core_clks_on = enable;
> + else
> + power->link_clks_on = enable;
With the number of if statements in here it almost seems more logical to have a
separate function for both DP_CORE_PM and DP_CTRL_PM - yeah, it would copy a bit
of code but it would be a heck of a lot more readable.
> + pr_debug("%s clocks for %s\n",
> + enable ? "enable" : "disable",
> + dp_parser_pm_name(pm_type));
> + pr_debug("link_clks:%s core_clks:%s\n",
> + power->link_clks_on ? "on" : "off",
> + power->core_clks_on ? "on" : "off");
> +error:
> + return rc;
> +}
> +
> +static int dp_power_request_gpios(struct dp_power_private *power)
> +{
> + int rc = 0, i;
> + struct device *dev;
> + struct dss_module_power *mp;
> + static const char * const gpio_names[] = {
> + "aux_enable", "aux_sel", "usbplug_cc",
> + };
> +
> + if (!power) {
> + pr_err("invalid power data\n");
> + return -EINVAL;
> + }
> +
> + dev = &power->pdev->dev;
> + mp = &power->parser->mp[DP_CORE_PM];
> +
> + for (i = 0; i < ARRAY_SIZE(gpio_names); i++) {
> + unsigned int gpio = mp->gpio_config[i].gpio;
> +
> + if (gpio_is_valid(gpio)) {
> + rc = devm_gpio_request(dev, gpio, gpio_names[i]);
> + if (rc) {
> + pr_err("request %s gpio failed, rc=%d\n",
> + gpio_names[i], rc);
> + goto error;
> + }
> + }
> + }
> + return 0;
> +error:
> + for (i = 0; i < ARRAY_SIZE(gpio_names); i++) {
> + unsigned int gpio = mp->gpio_config[i].gpio;
> +
> + if (gpio_is_valid(gpio))
> + gpio_free(gpio);
Can you call gpio_free() on a device managed resource? I think you probably want
devm_gpio_free.
> + }
> + return rc;
> +}
> +
> +static bool dp_power_find_gpio(const char *gpio1, const char *gpio2)
> +{
> + return !!strnstr(gpio1, gpio2, strlen(gpio1));
> +}
> +
> +static void dp_power_set_gpio(struct dp_power_private *power, bool flip)
> +{
> + int i;
> + struct dss_module_power *mp = &power->parser->mp[DP_CORE_PM];
> + struct dss_gpio *config = mp->gpio_config;
> +
> + for (i = 0; i < mp->num_gpio; i++) {
> + if (dp_power_find_gpio(config->gpio_name, "aux-sel"))
I think config->gpio_name is the name from the DT, basically qcom,aux-sel-gpio?
Why not just strcmp on that instead of going the much harder route
> + config->value = flip;
> +
> + if (gpio_is_valid(config->gpio)) {
> + pr_debug("gpio %s, value %d\n", config->gpio_name,
> + config->value);
> +
> + if (dp_power_find_gpio(config->gpio_name, "aux-en") ||
> + dp_power_find_gpio(config->gpio_name, "aux-sel"))
Same and same
> + gpio_direction_output(config->gpio,
> + config->value);
> + else
> + gpio_set_value(config->gpio, config->value);
> +
> + }
> + config++;
> + }
> +}
> +
> +static int dp_power_config_gpios(struct dp_power_private *power, bool flip,
> + bool enable)
> +{
> + int rc = 0, i;
> + struct dss_module_power *mp;
> + struct dss_gpio *config;
> +
> + mp = &power->parser->mp[DP_CORE_PM];
> + config = mp->gpio_config;
> +
> + if (enable) {
> + rc = dp_power_request_gpios(power);
> + if (rc) {
> + pr_err("gpio request failed\n");
dp_power_request_gpios prints an error message on every failure point.
> + return rc;
> + }
> +
> + dp_power_set_gpio(power, flip);
It seems like maybe dp_power_request_gpios() and dp_power_set_gpio() can be
combined - both of them only get called here.
> + } else {
Again, this feels like it could safely be in its own function for clarity.
> + for (i = 0; i < mp->num_gpio; i++) {
> + gpio_set_value(config[i].gpio, 0);
> + gpio_free(config[i].gpio);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int dp_power_client_init(struct dp_power *dp_power)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + return -EINVAL;
> + }
dp_power can never be NULL here.
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + pm_runtime_enable(&power->pdev->dev);
> +
> + rc = dp_power_regulator_init(power);
> + if (rc) {
> + pr_err("failed to init regulators\n");
The sub function already prints error messages.
> + goto error_power;
> + }
> +
> + rc = dp_power_clk_init(power, true);
> + if (rc) {
> + pr_err("failed to init clocks\n");
Also this one.
> + goto error_clk;
> + }
> + return 0;
You can flip this if statement and save a goto:
if (!rc)
return 0;
dp_power_regulator_deinit(power);
...
> +
> +error_clk:
> + dp_power_regulator_deinit(power);
> +error_power:
> + pm_runtime_disable(&power->pdev->dev);
> + return rc;
> +}
> +
> +static void dp_power_client_deinit(struct dp_power *dp_power)
> +{
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + return;
> + }
dp_power is always set.
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + dp_power_clk_init(power, false);
> + dp_power_regulator_deinit(power);
> + pm_runtime_disable(&power->pdev->dev);
> +
> +}
> +
> +static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
>
dp_power is always set.
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + if (power->pixel_clk_rcg && power->pixel_parent)
> + clk_set_parent(power->pixel_clk_rcg, power->pixel_parent);
> +exit:
> + return rc;
> +}
> +
> +static int dp_power_init(struct dp_power *dp_power, bool flip)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
dp_power is always set.
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + pm_runtime_get_sync(&power->pdev->dev);
> + rc = dp_power_regulator_ctrl(power, true);
> + if (rc) {
> + pr_err("failed to enable regulators\n");
This already prints the error messages you need.
> + goto exit;
> + }
> +
> + rc = dp_power_pinctrl_set(power, true);
> + if (rc) {
> + pr_err("failed to set pinctrl state\n");
Same.
> + goto err_pinctrl;
> + }
> +
> + rc = dp_power_config_gpios(power, flip, true);
> + if (rc) {
> + pr_err("failed to enable gpios\n");
Same.
> + goto err_gpio;
> + }
> +
> + rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> + if (rc) {
> + pr_err("failed to enable DP core clocks\n");
Same.
> + goto err_clk;
> + }
> +
> + return 0;
As above, flip the if statement and save a goto:
if (!rc)
return 0;
dp_power_config_gpios(power, flip, false);
...
> +
> +err_clk:
> + dp_power_config_gpios(power, flip, false);
> +err_gpio:
> + dp_power_pinctrl_set(power, false);
> +err_pinctrl:
> + dp_power_regulator_ctrl(power, false);
> +exit:
> + pm_runtime_put_sync(&power->pdev->dev);
> + return rc;
> +}
> +
> +static int dp_power_deinit(struct dp_power *dp_power)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
dp_power can never be NULL;
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> + dp_power_config_gpios(power, false, false);
> + dp_power_pinctrl_set(power, false);
> + dp_power_regulator_ctrl(power, false);
> + pm_runtime_put_sync(&power->pdev->dev);
> +exit:
> + return rc;
> +}
> +
> +struct dp_power *dp_power_get(struct dp_parser *parser)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> + struct dp_power *dp_power;
> +
> + if (!parser) {
> + pr_err("invalid input\n");
> + rc = -EINVAL;
> + goto error;
> + }
parser can never be NULL;
> + power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
> + if (!power) {
> + rc = -ENOMEM;
Just return ERR_PTR(-ENOMEM);
> + goto error;
> + }
> +
> + power->parser = parser;
> + power->pdev = parser->pdev;
> +
> + dp_power = &power->dp_power;
> +
> + dp_power->init = dp_power_init;
> + dp_power->deinit = dp_power_deinit;
> + dp_power->clk_enable = dp_power_clk_enable;
> + dp_power->set_pixel_clk_parent = dp_power_set_pixel_clk_parent;
> + dp_power->power_client_init = dp_power_client_init;
> + dp_power->power_client_deinit = dp_power_client_deinit;
> +
> + return dp_power;
> +error:
> + return ERR_PTR(rc);
> +}
> +
> +void dp_power_put(struct dp_power *dp_power)
> +{
> + struct dp_power_private *power = NULL;
> +
> + if (!dp_power)
> + return;
> +
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + devm_kfree(&power->pdev->dev, power);
You don't need to kfree device managed mmemory.
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h
> b/drivers/gpu/drm/msm/dp/dp_power.h
> new file mode 100644
> index 0000000..d1adaaf
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
> +
> +#ifndef _DP_POWER_H_
> +#define _DP_POWER_H_
> +
> +#include "dp_parser.h"
> +#include "dpu_power_handle.h"
> +
> +/**
> + * sruct dp_power - DisplayPort's power related data
> + *
> + * @init: initializes the regulators/core clocks/GPIOs/pinctrl
> + * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl
> + * @clk_enable: enable/disable the DP clocks
> + * @set_pixel_clk_parent: set the parent of DP pixel clock
> + */
> +struct dp_power {
> + int (*init)(struct dp_power *power, bool flip);
> + int (*deinit)(struct dp_power *power);
> + int (*clk_enable)(struct dp_power *power, enum dp_pm_type pm_type,
> + bool enable);
> + int (*set_pixel_clk_parent)(struct dp_power *power);
> + int (*power_client_init)(struct dp_power *power);
> + void (*power_client_deinit)(struct dp_power *power);
> +};
> +
> +/**
> + * dp_power_get() - configure and get the DisplayPort power module data
> + *
> + * @parser: instance of parser module
> + * return: pointer to allocated power module data
> + *
> + * This API will configure the DisplayPort's power module and provides
> + * methods to be called by the client to configure the power related
> + * modueles.
> + */
> +struct dp_power *dp_power_get(struct dp_parser *parser);
> +
> +/**
> + * dp_power_put() - release the power related resources
> + *
> + * @power: pointer to the power module's data
> + */
> +void dp_power_put(struct dp_power *power);
> +#endif /* _DP_POWER_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
> new file mode 100644
> index 0000000..77b5c0e
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
SPDX please.
<snip the rest>
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
