Re: [linux-sunxi] [PATCH v6 5/6] drm: sun4i: Add support for enabling DDC I2C bus to sun8i_dw_hdmi glue
From: Jernej Škrabec <jernej.skrabec@gmail.com>
Date: 2019-06-16 11:05:21
Also in:
dri-devel, linux-arm-kernel, linux-devicetree, lkml
Hi Ondrej! Dne ponedeljek, 27. maj 2019 ob 18:22:36 CEST je megous via linux-sunxi napisal(a):
quoted hunk ↗ jump to hunk
From: Ondrej Jirman <redacted> Orange Pi 3 board requires enabling a voltage shifting circuit via GPIO for the DDC bus to be usable. Add support for hdmi-connector node's optional ddc-en-gpios property to support this use case. Signed-off-by: Ondrej Jirman <redacted> --- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 55 +++++++++++++++++++++++++-- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 3 ++ 2 files changed, 55 insertions(+), 3 deletions(-)diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.cb/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index 39d8509d96a0..59b81ba02d96 100644--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c@@ -98,6 +98,30 @@ static u32 sun8i_dw_hdmi_find_possible_crtcs(structdrm_device *drm, return crtcs; } +static int sun8i_dw_hdmi_find_connector_pdev(struct device *dev, + struct
platform_device **pdev_out)
quoted hunk ↗ jump to hunk
+{ + struct platform_device *pdev; + struct device_node *remote; + + remote = of_graph_get_remote_node(dev->of_node, 1, -1); + if (!remote) + return -ENODEV; + + if (!of_device_is_compatible(remote, "hdmi-connector")) { + of_node_put(remote); + return -ENODEV; + } + + pdev = of_find_device_by_node(remote); + of_node_put(remote); + if (!pdev) + return -ENODEV; + + *pdev_out = pdev; + return 0; +} + static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, void *data) {@@ -151,16 +175,29 @@ static int sun8i_dw_hdmi_bind(struct device *dev,struct device *master, return PTR_ERR(hdmi->regulator); } + ret = sun8i_dw_hdmi_find_connector_pdev(dev, &hdmi->connector_pdev); + if (!ret) { + hdmi->ddc_en = gpiod_get_optional(&hdmi->connector_pdev- dev, + "ddc-en",
GPIOD_OUT_HIGH);
+ if (IS_ERR(hdmi->ddc_en)) {
+ platform_device_put(hdmi->connector_pdev);
+ dev_err(dev, "Couldn't get ddc-en gpio\n");
+ return PTR_ERR(hdmi->ddc_en);
+ }
+ }
+
ret = regulator_enable(hdmi->regulator);
if (ret) {
dev_err(dev, "Failed to enable regulator\n");
- return ret;
+ goto err_unref_ddc_en;
}
+ gpiod_set_value(hdmi->ddc_en, 1);Why don't you do that inside if clause where hdmi->ddc_en is assigned? It's not useful otherwise anyway. Besides, you would then only need to adjust one goto label in error path.
+
ret = reset_control_deassert(hdmi->rst_ctrl);
if (ret) {
dev_err(dev, "Could not deassert ctrl reset control\n");
quoted hunk ↗ jump to hunk
- goto err_disable_regulator; + goto err_disable_ddc_en; } ret = clk_prepare_enable(hdmi->clk_tmds);@@ -213,8 +250,14 @@ static int sun8i_dw_hdmi_bind(struct device *dev,struct device *master, clk_disable_unprepare(hdmi->clk_tmds); err_assert_ctrl_reset: reset_control_assert(hdmi->rst_ctrl); -err_disable_regulator: +err_disable_ddc_en: + gpiod_set_value(hdmi->ddc_en, 0); regulator_disable(hdmi->regulator); +err_unref_ddc_en: + if (hdmi->ddc_en) + gpiod_put(hdmi->ddc_en); + + platform_device_put(hdmi->connector_pdev); return ret; }@@ -228,7 +271,13 @@ static void sun8i_dw_hdmi_unbind(struct device *dev,struct device *master, sun8i_hdmi_phy_remove(hdmi); clk_disable_unprepare(hdmi->clk_tmds); reset_control_assert(hdmi->rst_ctrl); + gpiod_set_value(hdmi->ddc_en, 0); regulator_disable(hdmi->regulator); + + if (hdmi->ddc_en) + gpiod_put(hdmi->ddc_en); + + platform_device_put(hdmi->connector_pdev); } static const struct component_ops sun8i_dw_hdmi_ops = {diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.hb/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..dad66b8301c2 100644--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h@@ -9,6 +9,7 @@ #include <drm/bridge/dw_hdmi.h> #include <drm/drm_encoder.h> #include <linux/clk.h> +#include <linux/gpio/consumer.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset.h>@@ -190,6 +191,8 @@ struct sun8i_dw_hdmi { struct regulator *regulator; const struct sun8i_dw_hdmi_quirks *quirks; struct reset_control *rst_ctrl; + struct platform_device *connector_pdev;
It seems that connector_pdev is needed only during intialization. Why do you store it? Best regards, Jernej
+ struct gpio_desc *ddc_en; }; static inline struct sun8i_dw_hdmi *