Re: [Freedreno] [DPU PATCH 2/3] drm/msm/dp: add displayPort driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(-)

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux