Message ID | 20220608120723.2987843-7-dmitry.baryshkov@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/msm/hdmi: YAML-ify schema and cleanup some platform properties | expand |
Hi Dmitry, I love your patch! Perhaps something to improve: [auto build test WARNING on drm/drm-next] [also build test WARNING on robh/for-next linus/master v5.19-rc1 next-20220608] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-msm-hdmi-YAML-ify-schema-and-cleanup-some-platform-properties/20220608-200925 base: git://anongit.freedesktop.org/drm/drm drm-next config: hexagon-randconfig-r045-20220608 (https://download.01.org/0day-ci/archive/20220608/202206082312.XB745jWy-lkp@intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/c3e263fe0a077b382c2a76911c8ace385bd59a4c git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Dmitry-Baryshkov/drm-msm-hdmi-YAML-ify-schema-and-cleanup-some-platform-properties/20220608-200925 git checkout c3e263fe0a077b382c2a76911c8ace385bd59a4c # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpu/drm/msm/ If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/gpu/drm/msm/hdmi/hdmi.c:532:2: warning: variable 'hdmi' is uninitialized when used here [-Wuninitialized] hdmi->hpd_gpiod = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); ^~~~ drivers/gpu/drm/msm/hdmi/hdmi.c:518:19: note: initialize the variable 'hdmi' to silence this warning struct hdmi *hdmi; ^ = NULL 1 warning generated. Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for DRM_DP_AUX_BUS Depends on HAS_IOMEM && DRM && OF Selected by - DRM_MSM && HAS_IOMEM && DRM && (ARCH_QCOM || SOC_IMX5 || COMPILE_TEST && COMMON_CLK && IOMMU_SUPPORT && (QCOM_OCMEM || QCOM_OCMEM && (QCOM_LLCC || QCOM_LLCC && (QCOM_COMMAND_DB || QCOM_COMMAND_DB vim +/hdmi +532 drivers/gpu/drm/msm/hdmi/hdmi.c 513 514 static int msm_hdmi_bind(struct device *dev, struct device *master, void *data) 515 { 516 struct msm_drm_private *priv = dev_get_drvdata(master); 517 struct hdmi_platform_config *hdmi_cfg; 518 struct hdmi *hdmi; 519 struct device_node *of_node = dev->of_node; 520 int err; 521 522 hdmi_cfg = (struct hdmi_platform_config *) 523 of_device_get_match_data(dev); 524 if (!hdmi_cfg) { 525 DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node); 526 return -ENXIO; 527 } 528 529 hdmi_cfg->mmio_name = "core_physical"; 530 hdmi_cfg->qfprom_mmio_name = "qfprom_physical"; 531 > 532 hdmi->hpd_gpiod = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); 533 /* This will catch e.g. -PROBE_DEFER */ 534 if (IS_ERR(hdmi->hpd_gpiod)) 535 return PTR_ERR(hdmi->hpd_gpiod); 536 537 if (!hdmi->hpd_gpiod) 538 DBG("failed to get HPD gpio"); 539 540 if (hdmi->hpd_gpiod) 541 gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD"); 542 543 dev->platform_data = hdmi_cfg; 544 545 hdmi = msm_hdmi_init(to_platform_device(dev)); 546 if (IS_ERR(hdmi)) 547 return PTR_ERR(hdmi); 548 priv->hdmi = hdmi; 549 550 err = msm_hdmi_register_audio_driver(hdmi, dev); 551 if (err) { 552 DRM_ERROR("Failed to attach an audio codec %d\n", err); 553 hdmi->audio_pdev = NULL; 554 } 555 556 return 0; 557 } 558
Quoting Dmitry Baryshkov (2022-06-08 05:07:17) > @@ -543,41 +529,16 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data) > hdmi_cfg->mmio_name = "core_physical"; > hdmi_cfg->qfprom_mmio_name = "qfprom_physical"; > > - for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { [...] > - if (gpiod) > - gpiod_set_consumer_name(gpiod, msm_hdmi_gpio_pdata[i].label); > - hdmi_cfg->gpios[i].output = msm_hdmi_gpio_pdata[i].output; > - hdmi_cfg->gpios[i].value = msm_hdmi_gpio_pdata[i].value; > - } > + hdmi->hpd_gpiod = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); > + /* This will catch e.g. -PROBE_DEFER */ EPROBE_DEFER? > + if (IS_ERR(hdmi->hpd_gpiod)) > + return PTR_ERR(hdmi->hpd_gpiod); > + > + if (!hdmi->hpd_gpiod) > + DBG("failed to get HPD gpio"); Does DBG() add newlines? > + > + if (hdmi->hpd_gpiod) > + gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD"); > > dev->platform_data = hdmi_cfg; >
On 08/06/2022 23:59, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-06-08 05:07:17) >> @@ -543,41 +529,16 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data) >> hdmi_cfg->mmio_name = "core_physical"; >> hdmi_cfg->qfprom_mmio_name = "qfprom_physical"; >> >> - for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { > [...] >> - if (gpiod) >> - gpiod_set_consumer_name(gpiod, msm_hdmi_gpio_pdata[i].label); >> - hdmi_cfg->gpios[i].output = msm_hdmi_gpio_pdata[i].output; >> - hdmi_cfg->gpios[i].value = msm_hdmi_gpio_pdata[i].value; >> - } >> + hdmi->hpd_gpiod = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); >> + /* This will catch e.g. -PROBE_DEFER */ > > EPROBE_DEFER? Ack. > >> + if (IS_ERR(hdmi->hpd_gpiod)) >> + return PTR_ERR(hdmi->hpd_gpiod); >> + >> + if (!hdmi->hpd_gpiod) >> + DBG("failed to get HPD gpio"); > > Does DBG() add newlines? Yes, it does. > >> + >> + if (hdmi->hpd_gpiod) >> + gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD"); >> >> dev->platform_data = hdmi_cfg; >>
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 06b44b40ec09..7cb687458a56 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -406,20 +406,6 @@ static struct hdmi_platform_config hdmi_tx_8996_config = { .hpd_freq = hpd_clk_freq_8x74, }; -static const struct { - const char *name; - const bool output; - const int value; - const char *label; -} msm_hdmi_gpio_pdata[] = { - { "qcom,hdmi-tx-ddc-clk", true, 1, "HDMI_DDC_CLK" }, - { "qcom,hdmi-tx-ddc-data", true, 1, "HDMI_DDC_DATA" }, - { "qcom,hdmi-tx-hpd", false, 1, "HDMI_HPD" }, - { "qcom,hdmi-tx-mux-en", true, 1, "HDMI_MUX_EN" }, - { "qcom,hdmi-tx-mux-sel", true, 0, "HDMI_MUX_SEL" }, - { "qcom,hdmi-tx-mux-lpm", true, 1, "HDMI_MUX_LPM" }, -}; - /* * HDMI audio codec callbacks */ @@ -531,7 +517,7 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data) struct hdmi_platform_config *hdmi_cfg; struct hdmi *hdmi; struct device_node *of_node = dev->of_node; - int i, err; + int err; hdmi_cfg = (struct hdmi_platform_config *) of_device_get_match_data(dev); @@ -543,41 +529,16 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data) hdmi_cfg->mmio_name = "core_physical"; hdmi_cfg->qfprom_mmio_name = "qfprom_physical"; - for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { - const char *name = msm_hdmi_gpio_pdata[i].name; - struct gpio_desc *gpiod; - - /* - * We are fetching the GPIO lines "as is" since the connector - * code is enabling and disabling the lines. Until that point - * the power-on default value will be kept. - */ - gpiod = devm_gpiod_get_optional(dev, name, GPIOD_ASIS); - /* This will catch e.g. -PROBE_DEFER */ - if (IS_ERR(gpiod)) - return PTR_ERR(gpiod); - if (!gpiod) { - /* Try a second time, stripping down the name */ - char name3[32]; - - /* - * Try again after stripping out the "qcom,hdmi-tx" - * prefix. This is mainly to match "hpd-gpios" used - * in the upstream bindings. - */ - if (sscanf(name, "qcom,hdmi-tx-%s", name3)) - gpiod = devm_gpiod_get_optional(dev, name3, GPIOD_ASIS); - if (IS_ERR(gpiod)) - return PTR_ERR(gpiod); - if (!gpiod) - DBG("failed to get gpio: %s", name); - } - hdmi_cfg->gpios[i].gpiod = gpiod; - if (gpiod) - gpiod_set_consumer_name(gpiod, msm_hdmi_gpio_pdata[i].label); - hdmi_cfg->gpios[i].output = msm_hdmi_gpio_pdata[i].output; - hdmi_cfg->gpios[i].value = msm_hdmi_gpio_pdata[i].value; - } + hdmi->hpd_gpiod = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); + /* This will catch e.g. -PROBE_DEFER */ + if (IS_ERR(hdmi->hpd_gpiod)) + return PTR_ERR(hdmi->hpd_gpiod); + + if (!hdmi->hpd_gpiod) + DBG("failed to get HPD gpio"); + + if (hdmi->hpd_gpiod) + gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD"); dev->platform_data = hdmi_cfg; diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h index 736f348befb3..a6c88d157bc3 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.h +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h @@ -19,17 +19,9 @@ #include "msm_drv.h" #include "hdmi.xml.h" -#define HDMI_MAX_NUM_GPIO 6 - struct hdmi_phy; struct hdmi_platform_config; -struct hdmi_gpio_data { - struct gpio_desc *gpiod; - bool output; - int value; -}; - struct hdmi_audio { bool enabled; struct hdmi_audio_infoframe infoframe; @@ -61,6 +53,8 @@ struct hdmi { struct clk **hpd_clks; struct clk **pwr_clks; + struct gpio_desc *hpd_gpiod; + struct hdmi_phy *phy; struct device *phy_dev; @@ -109,9 +103,6 @@ struct hdmi_platform_config { /* clks that need to be on for screen pwr (ie pixel clk): */ const char **pwr_clk_names; int pwr_clk_cnt; - - /* gpio's: */ - struct hdmi_gpio_data gpios[HDMI_MAX_NUM_GPIO]; }; struct hdmi_bridge { diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c index 75605ddac7c4..bfa827b47989 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c @@ -60,48 +60,6 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi) } } -static int gpio_config(struct hdmi *hdmi, bool on) -{ - const struct hdmi_platform_config *config = hdmi->config; - int i; - - if (on) { - for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { - struct hdmi_gpio_data gpio = config->gpios[i]; - - if (gpio.gpiod) { - if (gpio.output) { - gpiod_direction_output(gpio.gpiod, - gpio.value); - } else { - gpiod_direction_input(gpio.gpiod); - gpiod_set_value_cansleep(gpio.gpiod, - gpio.value); - } - } - } - - DBG("gpio on"); - } else { - for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { - struct hdmi_gpio_data gpio = config->gpios[i]; - - if (!gpio.gpiod) - continue; - - if (gpio.output) { - int value = gpio.value ? 0 : 1; - - gpiod_set_value_cansleep(gpio.gpiod, value); - } - } - - DBG("gpio off"); - } - - return 0; -} - static void enable_hpd_clocks(struct hdmi *hdmi, bool enable) { const struct hdmi_platform_config *config = hdmi->config; @@ -154,11 +112,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge) goto fail; } - ret = gpio_config(hdmi, true); - if (ret) { - DRM_DEV_ERROR(dev, "failed to configure GPIOs: %d\n", ret); - goto fail; - } + if (hdmi->hpd_gpiod) + gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1); pm_runtime_get_sync(dev); enable_hpd_clocks(hdmi, true); @@ -207,10 +162,6 @@ void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge) enable_hpd_clocks(hdmi, false); pm_runtime_put(dev); - ret = gpio_config(hdmi, false); - if (ret) - dev_warn(dev, "failed to unconfigure GPIOs: %d\n", ret); - ret = pinctrl_pm_select_sleep_state(dev); if (ret) dev_warn(dev, "pinctrl state chg failed: %d\n", ret); @@ -269,10 +220,7 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi) #define HPD_GPIO_INDEX 2 static enum drm_connector_status detect_gpio(struct hdmi *hdmi) { - const struct hdmi_platform_config *config = hdmi->config; - struct hdmi_gpio_data hpd_gpio = config->gpios[HPD_GPIO_INDEX]; - - return gpiod_get_value(hpd_gpio.gpiod) ? + return gpiod_get_value(hdmi->hpd_gpiod) ? connector_status_connected : connector_status_disconnected; } @@ -282,8 +230,6 @@ enum drm_connector_status msm_hdmi_bridge_detect( { struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); struct hdmi *hdmi = hdmi_bridge->hdmi; - const struct hdmi_platform_config *config = hdmi->config; - struct hdmi_gpio_data hpd_gpio = config->gpios[HPD_GPIO_INDEX]; enum drm_connector_status stat_gpio, stat_reg; int retry = 20; @@ -291,7 +237,7 @@ enum drm_connector_status msm_hdmi_bridge_detect( * some platforms may not have hpd gpio. Rely only on the status * provided by REG_HDMI_HPD_INT_STATUS in this case. */ - if (!hpd_gpio.gpiod) + if (!hdmi->hpd_gpiod) return detect_reg(hdmi); do {
The HDMI driver has code to configure extra GPIOs, which predates pinctrl support. Nowadays all platforms should use pinctrl instead. Neither of upstreamed Qualcomm pltforms uses these properties, so it's safe to drop them. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/hdmi/hdmi.c | 61 +++++----------------------- drivers/gpu/drm/msm/hdmi/hdmi.h | 13 +----- drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 62 ++--------------------------- 3 files changed, 17 insertions(+), 119 deletions(-)