* [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 10:05 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2020-05-13 10:05 UTC (permalink / raw)
To: Daniel Vetter, David Airlie, Rob Clark
Cc: devicetree, Jernej Skrabec, Neil Armstrong, linux-arm-msm,
Jonas Karlman, linux-kernel, dri-devel, Bjorn Andersson,
Andrzej Hajda, Vinod Koul, Rob Herring, Laurent Pinchart
Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
I2S port as an input and HDMI port as output
Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/gpu/drm/bridge/Kconfig | 13 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/lt9611.c | 1113 +++++++++++++++++++++++++++++++
3 files changed, 1127 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/lt9611.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index aaed2347ace9..5cac15ce2027 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -38,6 +38,19 @@ config DRM_DISPLAY_CONNECTOR
on ARM-based platforms. Saying Y here when this driver is not needed
will not cause any issue.
+config DRM_LONTIUM_LT9611
+ tristate "Lontium LT9611 DSI/HDMI bridge"
+ select SND_SOC_HDMI_CODEC if SND_SOC
+ depends on OF
+ select DRM_PANEL_BRIDGE
+ select DRM_KMS_HELPER
+ select REGMAP_I2C
+ help
+ Driver for Lontium LT9611 DSI to HDMI bridge
+ chip driver that converts dual DSI and I2S to
+ HDMI signals
+ Please say Y if you have such hardware.
+
config DRM_LVDS_CODEC
tristate "Transparent LVDS encoders and decoders support"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 6fb062b5b0f0..d2a696e8ca5d 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
+obj-$(CONFIG_DRM_LONTIUM_LT9611) += lt9611.o
obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
diff --git a/drivers/gpu/drm/bridge/lt9611.c b/drivers/gpu/drm/bridge/lt9611.c
new file mode 100644
index 000000000000..e6e7ce43980d
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lt9611.c
@@ -0,0 +1,1113 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2019-2020. Linaro Limited.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_print.h>
+
+#define EDID_SEG_SIZE 256
+
+#define LT9611_4LANES 0
+
+struct lt9611 {
+ struct device *dev;
+ struct drm_bridge bridge;
+ struct drm_connector connector;
+
+ struct regmap *regmap;
+
+ struct device_node *dsi0_node;
+ struct device_node *dsi1_node;
+ struct mipi_dsi_device *dsi0;
+ struct mipi_dsi_device *dsi1;
+
+ bool ac_mode;
+
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *enable_gpio;
+
+ bool power_on;
+ bool sleep;
+
+ struct regulator_bulk_data supplies[2];
+
+ struct i2c_client *client;
+
+ enum drm_connector_status status;
+
+ u8 edid_buf[EDID_SEG_SIZE];
+ u32 vic;
+};
+
+#define LT9611_PAGE_CONTROL 0xff
+
+static const struct regmap_range_cfg lt9611_ranges[] = {
+ {
+ .name = "register_range",
+ .range_min = 0,
+ .range_max = 0x85ff,
+ .selector_reg = LT9611_PAGE_CONTROL,
+ .selector_mask = 0xff,
+ .selector_shift = 0,
+ .window_start = 0,
+ .window_len = 0x100,
+ },
+};
+
+static const struct regmap_config lt9611_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0xffff,
+ .ranges = lt9611_ranges,
+ .num_ranges = ARRAY_SIZE(lt9611_ranges),
+};
+
+struct lt9611_mode {
+ u16 hdisplay;
+ u16 vdisplay;
+ u8 fps;
+ u8 lanes;
+ u8 intfs;
+};
+
+static struct lt9611_mode lt9611_modes[] = {
+ { 3840, 2160, 30, 4, 2 }, /* 3840x2160 24bit 30Hz 4Lane 2ports */
+ { 1920, 1080, 60, 4, 1 }, /* 1080P 24bit 60Hz 4lane 1port */
+ { 1920, 1080, 30, 3, 1 }, /* 1080P 24bit 30Hz 3lane 1port */
+ { 1920, 1080, 24, 3, 1 },
+ { 720, 480, 60, 4, 1 },
+ { 720, 576, 50, 2, 1 },
+ { 640, 480, 60, 2, 1 },
+};
+
+static struct lt9611 *bridge_to_lt9611(struct drm_bridge *bridge)
+{
+ return container_of(bridge, struct lt9611, bridge);
+}
+
+static struct lt9611 *connector_to_lt9611(struct drm_connector *connector)
+{
+ return container_of(connector, struct lt9611, connector);
+}
+
+static int lt9611_mipi_input_analog(struct lt9611 *lt9611)
+{
+ struct reg_sequence reg_cfg[] = {
+ { 0x8106, 0x40 }, /*port A rx current*/
+ { 0x810a, 0xfe }, /*port A ldo voltage set*/
+ { 0x810b, 0xbf }, /*enable port A lprx*/
+ { 0x8111, 0x40 }, /*port B rx current*/
+ { 0x8115, 0xfe }, /*port B ldo voltage set*/
+ { 0x8116, 0xbf }, /*enable port B lprx*/
+
+ { 0x811c, 0x03 }, /*PortA clk lane no-LP mode*/
+ { 0x8120, 0x03 }, /*PortB clk lane with-LP mode*/
+ };
+
+ regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
+
+ return 0;
+}
+
+static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
+ const struct drm_display_mode *mode)
+{
+ regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
+
+ if (mode->hdisplay == 3840)
+ regmap_write(lt9611->regmap, 0x830a, 0x03);
+ else
+ regmap_write(lt9611->regmap, 0x830a, 0x00);
+
+ regmap_write(lt9611->regmap, 0x824f, 0x80);
+ regmap_write(lt9611->regmap, 0x8250, 0x10);
+ regmap_write(lt9611->regmap, 0x8302, 0x0a);
+ regmap_write(lt9611->regmap, 0x8306, 0x0a);
+
+ return 0;
+}
+
+static void lt9611_mipi_video_setup(struct lt9611 *lt9611,
+ const struct drm_display_mode *mode)
+{
+ u32 h_total, h_act, hpw, hfp, hss;
+ u32 v_total, v_act, vpw, vfp, vss;
+
+ h_total = mode->htotal;
+ v_total = mode->vtotal;
+
+ h_act = mode->hdisplay;
+ hpw = mode->hsync_end - mode->hsync_start;
+ hfp = mode->hsync_start - mode->hdisplay;
+ hss = (mode->hsync_end - mode->hsync_start) +
+ (mode->htotal - mode->hsync_end);
+
+ v_act = mode->vdisplay;
+ vpw = mode->vsync_end - mode->vsync_start;
+ vfp = mode->vsync_start - mode->vdisplay;
+ vss = (mode->vsync_end - mode->vsync_start) +
+ (mode->vtotal - mode->vsync_end);
+
+ regmap_write(lt9611->regmap, 0x830d, (u8)(v_total / 256));
+ regmap_write(lt9611->regmap, 0x830e, (u8)(v_total % 256));
+
+ regmap_write(lt9611->regmap, 0x830f, (u8)(v_act / 256));
+ regmap_write(lt9611->regmap, 0x8310, (u8)(v_act % 256));
+
+ regmap_write(lt9611->regmap, 0x8311, (u8)(h_total / 256));
+ regmap_write(lt9611->regmap, 0x8312, (u8)(h_total % 256));
+
+ regmap_write(lt9611->regmap, 0x8313, (u8)(h_act / 256));
+ regmap_write(lt9611->regmap, 0x8314, (u8)(h_act % 256));
+
+ regmap_write(lt9611->regmap, 0x8315, (u8)(vpw % 256));
+ regmap_write(lt9611->regmap, 0x8316, (u8)(hpw % 256));
+
+ regmap_write(lt9611->regmap, 0x8317, (u8)(vfp % 256));
+
+ regmap_write(lt9611->regmap, 0x8318, (u8)(vss % 256));
+
+ regmap_write(lt9611->regmap, 0x8319, (u8)(hfp % 256));
+
+ regmap_write(lt9611->regmap, 0x831a, (u8)(hss / 256));
+ regmap_write(lt9611->regmap, 0x831b, (u8)(hss % 256));
+}
+
+static int lt9611_pcr_setup(struct lt9611 *lt9611,
+ const struct drm_display_mode *mode)
+{
+ struct reg_sequence reg_cfg[] = {
+ { 0x830b, 0x01 },
+ { 0x830c, 0x10 },
+ { 0x8348, 0x00 },
+ { 0x8349, 0x81 },
+
+ /* stage 1 */
+ { 0x8321, 0x4a },
+ { 0x8324, 0x71 },
+ { 0x8325, 0x30 },
+ { 0x832a, 0x01 },
+
+ /* stage 2 */
+ { 0x834a, 0x40 },
+ { 0x831d, 0x10 },
+
+ /* MK limit */
+ { 0x832d, 0x38 },
+ { 0x8331, 0x08 },
+ };
+
+ regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
+
+ switch (mode->hdisplay) {
+ case 640:
+ regmap_write(lt9611->regmap, 0x8326, 0x14);
+ break;
+ case 1920:
+ regmap_write(lt9611->regmap, 0x8326, 0x37);
+ break;
+ case 3840:
+ regmap_write(lt9611->regmap, 0x830b, 0x03);
+ regmap_write(lt9611->regmap, 0x830c, 0xd0);
+ regmap_write(lt9611->regmap, 0x8348, 0x03);
+ regmap_write(lt9611->regmap, 0x8349, 0xe0);
+ regmap_write(lt9611->regmap, 0x8324, 0x72);
+ regmap_write(lt9611->regmap, 0x8325, 0x00);
+ regmap_write(lt9611->regmap, 0x832a, 0x01);
+ regmap_write(lt9611->regmap, 0x834a, 0x10);
+ regmap_write(lt9611->regmap, 0x831d, 0x10);
+ regmap_write(lt9611->regmap, 0x8326, 0x37);
+ break;
+ }
+
+ /* pcr rst */
+ regmap_write(lt9611->regmap, 0x8011, 0x5a);
+ regmap_write(lt9611->regmap, 0x8011, 0xfa);
+
+ return 0;
+}
+
+static int lt9611_pll_setup(struct lt9611 *lt9611,
+ const struct drm_display_mode *mode)
+{
+ unsigned int pclk = mode->clock;
+ struct reg_sequence reg_cfg[] = {
+ /* txpll init */
+ { 0x8123, 0x40 },
+ { 0x8124, 0x64 },
+ { 0x8125, 0x80 },
+ { 0x8126, 0x55 },
+ { 0x812c, 0x37 },
+ { 0x812f, 0x01 },
+ { 0x8126, 0x55 },
+ { 0x8127, 0x66 },
+ { 0x8128, 0x88 },
+ };
+
+ regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
+
+ if (pclk > 150000)
+ regmap_write(lt9611->regmap, 0x812d, 0x88);
+ else if (pclk > 70000)
+ regmap_write(lt9611->regmap, 0x812d, 0x99);
+ else
+ regmap_write(lt9611->regmap, 0x812d, 0xaa);
+
+ regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
+ regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
+ regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
+
+ regmap_write(lt9611->regmap, 0x82de, 0x20);
+ regmap_write(lt9611->regmap, 0x82de, 0xe0);
+
+ regmap_write(lt9611->regmap, 0x8016, 0xf1);
+ regmap_write(lt9611->regmap, 0x8016, 0xf3);
+
+ return 0;
+}
+
+static int lt9611_video_check(struct lt9611 *lt9611)
+{
+ u32 v_total, v_act, h_act_a, h_act_b, h_total_sysclk;
+ unsigned int temp;
+ int ret;
+
+ /* top module video check */
+
+ /* v_act */
+ ret = regmap_read(lt9611->regmap, 0x8282, &temp);
+ if (ret)
+ goto end;
+
+ v_act = temp << 8;
+ ret = regmap_read(lt9611->regmap, 0x8283, &temp);
+ if (ret)
+ goto end;
+ v_act = v_act + temp;
+
+ /* v_total */
+ ret = regmap_read(lt9611->regmap, 0x826c, &temp);
+ if (ret)
+ goto end;
+ v_total = temp << 8;
+ ret = regmap_read(lt9611->regmap, 0x826d, &temp);
+ if (ret)
+ goto end;
+ v_total = v_total + temp;
+
+ /* h_total_sysclk */
+ ret = regmap_read(lt9611->regmap, 0x8286, &temp);
+ if (ret)
+ goto end;
+ h_total_sysclk = temp << 8;
+ ret = regmap_read(lt9611->regmap, 0x8287, &temp);
+ if (ret)
+ goto end;
+ h_total_sysclk = h_total_sysclk + temp;
+
+ /* h_act_a */
+ ret = regmap_read(lt9611->regmap, 0x8382, &temp);
+ if (ret)
+ goto end;
+ h_act_a = temp << 8;
+ ret = regmap_read(lt9611->regmap, 0x8383, &temp);
+ if (ret)
+ goto end;
+ h_act_a = (h_act_a + temp) / 3;
+
+ /* h_act_b */
+ ret = regmap_read(lt9611->regmap, 0x8386, &temp);
+ if (ret)
+ goto end;
+ h_act_b = temp << 8;
+ ret = regmap_read(lt9611->regmap, 0x8387, &temp);
+ if (ret)
+ goto end;
+ h_act_b = (h_act_b + temp) / 3;
+
+ dev_info(lt9611->dev,
+ "video check: h_act_a=%d, h_act_b=%d, v_act=%d, v_total=%d, h_total_sysclk=%d\n",
+ h_act_a, h_act_b, v_act, v_total, h_total_sysclk);
+
+ return 0;
+
+end:
+ dev_err(lt9611->dev, "read video check error\n");
+ return ret;
+}
+
+static void lt9611_hdmi_tx_digital(struct lt9611 *lt9611)
+{
+ regmap_write(lt9611->regmap, 0x8443, 0x46 - lt9611->vic);
+ regmap_write(lt9611->regmap, 0x8447, lt9611->vic);
+ regmap_write(lt9611->regmap, 0x843d, 0x0a); /* UD1 infoframe */
+
+ regmap_write(lt9611->regmap, 0x82d6, 0x8c);
+ regmap_write(lt9611->regmap, 0x82d7, 0x04);
+}
+
+static void lt9611_hdmi_tx_phy(struct lt9611 *lt9611)
+{
+ struct reg_sequence reg_cfg[] = {
+ { 0x8130, 0x6a },
+ { 0x8131, 0x44 }, /* HDMI DC mode */
+ { 0x8132, 0x4a },
+ { 0x8133, 0x0b },
+ { 0x8134, 0x00 },
+ { 0x8135, 0x00 },
+ { 0x8136, 0x00 },
+ { 0x8137, 0x44 },
+ { 0x813f, 0x0f },
+ { 0x8140, 0xa0 },
+ { 0x8141, 0xa0 },
+ { 0x8142, 0xa0 },
+ { 0x8143, 0xa0 },
+ { 0x8144, 0x0a },
+ };
+
+ /* HDMI AC mode */
+ if (lt9611->ac_mode)
+ reg_cfg[2].def = 0x73;
+
+ regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
+}
+
+static irqreturn_t lt9611_irq_thread_handler(int irq, void *dev_id)
+{
+ struct lt9611 *lt9611 = dev_id;
+ unsigned int irq_flag0 = 0;
+ unsigned int irq_flag3 = 0;
+
+ regmap_read(lt9611->regmap, 0x820f, &irq_flag3);
+ regmap_read(lt9611->regmap, 0x820c, &irq_flag0);
+
+ pr_debug("%s() irq_flag0: %#x irq_flag3: %#x\n",
+ __func__, irq_flag0, irq_flag3);
+
+ /* hpd changed low */
+ if (irq_flag3 & 0x80) {
+ dev_info(lt9611->dev, "hdmi cable disconnected\n");
+
+ regmap_write(lt9611->regmap, 0x8207, 0xbf);
+ regmap_write(lt9611->regmap, 0x8207, 0x3f);
+ }
+ /* hpd changed high */
+ if (irq_flag3 & 0x40) {
+ dev_info(lt9611->dev, "hdmi cable connected\n");
+
+ regmap_write(lt9611->regmap, 0x8207, 0x7f);
+ regmap_write(lt9611->regmap, 0x8207, 0x3f);
+ }
+
+ if (irq_flag3 & 0xc0)
+ drm_kms_helper_hotplug_event(lt9611->bridge.dev);
+
+ /* video input changed */
+ if (irq_flag0 & 0x01) {
+ dev_info(lt9611->dev, "video input changed\n");
+ regmap_write(lt9611->regmap, 0x829e, 0xff);
+ regmap_write(lt9611->regmap, 0x829e, 0xf7);
+ regmap_write(lt9611->regmap, 0x8204, 0xff);
+ regmap_write(lt9611->regmap, 0x8204, 0xfe);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void lt9611_enable_hpd_interrupts(struct lt9611 *lt9611)
+{
+ unsigned int val;
+
+ dev_dbg(lt9611->dev, "enabling hpd interrupts\n");
+
+ regmap_read(lt9611->regmap, 0x8203, &val);
+
+ val &= ~0xc0;
+ regmap_write(lt9611->regmap, 0x8203, val);
+ regmap_write(lt9611->regmap, 0x8207, 0xff); //clear
+ regmap_write(lt9611->regmap, 0x8207, 0x3f);
+}
+
+static void lt9611_sleep_setup(struct lt9611 *lt9611)
+{
+ struct reg_sequence sleep_setup[] = {
+ { 0x8024, 0x76 },
+ { 0x8023, 0x01 },
+ { 0x8157, 0x03 }, //set addr pin as output
+ { 0x8149, 0x0b },
+ { 0x8151, 0x30 }, //disable IRQ
+ { 0x8102, 0x48 }, //MIPI Rx power down
+ { 0x8123, 0x80 },
+ { 0x8130, 0x00 },
+ { 0x8100, 0x01 }, //bandgap power down
+ { 0x8101, 0x00 }, //system clk power down
+ };
+
+ dev_dbg(lt9611->dev, "sleep\n");
+
+ regmap_multi_reg_write(lt9611->regmap,
+ sleep_setup, ARRAY_SIZE(sleep_setup));
+ lt9611->sleep = true;
+}
+
+static int lt9611_power_on(struct lt9611 *lt9611)
+{
+ int ret;
+ const struct reg_sequence seq[] = {
+ /* LT9611_System_Init */
+ { 0x8101, 0x18 }, /* sel xtal clock */
+
+ /* timer for frequency meter */
+ { 0x821b, 0x69 }, /*timer 2*/
+ { 0x821c, 0x78 },
+ { 0x82cb, 0x69 }, /*timer 1 */
+ { 0x82cc, 0x78 },
+
+ /* irq init */
+ { 0x8251, 0x01 },
+ { 0x8258, 0x0a }, /* hpd irq */
+ { 0x8259, 0x80 }, /* hpd debounce width */
+ { 0x829e, 0xf7 }, /* video check irq */
+
+ /* power consumption for work */
+ { 0x8004, 0xf0 },
+ { 0x8006, 0xf0 },
+ { 0x800a, 0x80 },
+ { 0x800b, 0x40 },
+ { 0x800d, 0xef },
+ { 0x8011, 0xfa },
+ };
+
+ if (lt9611->power_on)
+ return 0;
+
+ dev_dbg(lt9611->dev, "power on\n");
+
+ ret = regmap_multi_reg_write(lt9611->regmap, seq, ARRAY_SIZE(seq));
+ if (!ret)
+ lt9611->power_on = true;
+
+ return ret;
+}
+
+static int lt9611_power_off(struct lt9611 *lt9611)
+{
+ int ret;
+
+ dev_dbg(lt9611->dev, "power off\n");
+
+ ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
+ if (!ret)
+ lt9611->power_on = false;
+
+ return ret;
+}
+
+static void lt9611_reset(struct lt9611 *lt9611)
+{
+ gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
+ msleep(20);
+ gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
+ msleep(20);
+ gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
+ msleep(100);
+}
+
+static void lt9611_assert_5v(struct lt9611 *lt9611)
+{
+ if (!lt9611->enable_gpio)
+ return;
+
+ gpiod_set_value_cansleep(lt9611->enable_gpio, 1);
+ msleep(20);
+}
+
+static int lt9611_regulator_init(struct lt9611 *lt9611)
+{
+ int ret;
+
+ lt9611->supplies[0].supply = "vdd";
+ lt9611->supplies[1].supply = "vcc";
+ ret = devm_regulator_bulk_get(lt9611->dev, 2, lt9611->supplies);
+ if (ret < 0)
+ return ret;
+
+ return regulator_set_load(lt9611->supplies[0].consumer, 300000);
+}
+
+static int lt9611_regulator_enable(struct lt9611 *lt9611)
+{
+ int ret;
+
+ ret = regulator_enable(lt9611->supplies[0].consumer);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(1000, 10000);
+
+ ret = regulator_enable(lt9611->supplies[1].consumer);
+ if (ret < 0) {
+ regulator_disable(lt9611->supplies[0].consumer);
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct lt9611_mode *lt9611_find_mode(const struct drm_display_mode *mode)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lt9611_modes); i++) {
+ if (lt9611_modes[i].hdisplay == mode->hdisplay &&
+ lt9611_modes[i].vdisplay == mode->vdisplay &&
+ lt9611_modes[i].fps == drm_mode_vrefresh(mode)) {
+ return <9611_modes[i];
+ }
+ }
+
+ return NULL;
+}
+
+/* connector funcs */
+static enum drm_connector_status
+lt9611_connector_detect(struct drm_connector *connector, bool force)
+{
+ struct lt9611 *lt9611 = connector_to_lt9611(connector);
+ unsigned int reg_val = 0;
+ int connected = 0;
+
+ regmap_read(lt9611->regmap, 0x825e, ®_val);
+ connected = (reg_val & BIT(2));
+ dev_dbg(lt9611->dev, "connected = %x\n", connected);
+
+ lt9611->status = connected ? connector_status_connected :
+ connector_status_disconnected;
+
+ return lt9611->status;
+}
+
+static int lt9611_read_edid(struct lt9611 *lt9611)
+{
+ unsigned int temp;
+ int ret = 0;
+ int i, j;
+
+ memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
+
+ regmap_write(lt9611->regmap, 0x8503, 0xc9);
+
+ /* 0xA0 is EDID device address */
+ regmap_write(lt9611->regmap, 0x8504, 0xa0);
+ /* 0x00 is EDID offset address */
+ regmap_write(lt9611->regmap, 0x8505, 0x00);
+ /* length for read */
+ regmap_write(lt9611->regmap, 0x8506, 0x20);
+ regmap_write(lt9611->regmap, 0x8514, 0x7f);
+
+ for (i = 0 ; i < 8 ; i++) {
+ /* offset address */
+ regmap_write(lt9611->regmap, 0x8505, i * 32);
+ regmap_write(lt9611->regmap, 0x8507, 0x36);
+ regmap_write(lt9611->regmap, 0x8507, 0x31);
+ regmap_write(lt9611->regmap, 0x8507, 0x37);
+ usleep_range(5000, 10000);
+
+ regmap_read(lt9611->regmap, 0x8540, &temp);
+
+ if (temp & 0x02) { /*KEY_DDC_ACCS_DONE=1*/
+ for (j = 0; j < 32; j++) {
+ regmap_read(lt9611->regmap, 0x8583, &temp);
+ lt9611->edid_buf[i * 32 + j] = temp;
+ }
+ } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
+ dev_err(lt9611->dev, "read edid failed: no ack\n");
+ ret = -EIO;
+ goto end;
+ } else {
+ dev_err(lt9611->dev,
+ "read edid failed: access not done\n");
+ ret = -EIO;
+ goto end;
+ }
+ }
+
+ dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
+ lt9611->edid_buf[255]);
+
+end:
+ regmap_write(lt9611->regmap, 0x8507, 0x1f);
+ return ret;
+}
+
+/* TODO: add support for more extension blocks */
+static int
+lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
+{
+ struct lt9611 *lt9611 = data;
+ int ret;
+
+ dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
+ block, (int)len);
+
+ if (len > 128)
+ return -EINVAL;
+
+ /* support up to 1 extension block */
+ if (block > 1)
+ return -EINVAL;
+
+ if (block == 0) {
+ /* always read 2 edid blocks once */
+ ret = lt9611_read_edid(lt9611);
+ if (ret) {
+ dev_err(lt9611->dev, "edid read failed\n");
+ return ret;
+ }
+ }
+
+ if (block % 2 == 0)
+ memcpy(buf, lt9611->edid_buf, len);
+ else
+ memcpy(buf, lt9611->edid_buf + 128, len);
+
+ return 0;
+}
+
+static int lt9611_connector_get_modes(struct drm_connector *connector)
+{
+ struct lt9611 *lt9611 = connector_to_lt9611(connector);
+ unsigned int count;
+ struct edid *edid;
+
+ dev_dbg(lt9611->dev, "get modes\n");
+
+ lt9611_power_on(lt9611);
+ edid = drm_do_get_edid(connector, lt9611_get_edid_block, lt9611);
+ drm_connector_update_edid_property(connector, edid);
+ count = drm_add_edid_modes(connector, edid);
+ kfree(edid);
+
+ return count;
+}
+
+static enum drm_mode_status
+lt9611_connector_mode_valid(struct drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
+
+ return lt9611_mode ? MODE_OK : MODE_BAD;
+}
+
+/* bridge funcs */
+static void lt9611_bridge_enable(struct drm_bridge *bridge)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+ dev_dbg(lt9611->dev, "bridge enable\n");
+
+ if (lt9611_power_on(lt9611)) {
+ dev_err(lt9611->dev, "power on failed\n");
+ return;
+ }
+
+ dev_dbg(lt9611->dev, "video on\n");
+
+ lt9611_mipi_input_analog(lt9611);
+ lt9611_hdmi_tx_digital(lt9611);
+ lt9611_hdmi_tx_phy(lt9611);
+
+ msleep(500);
+
+ lt9611_video_check(lt9611);
+
+ /* Enable HDMI output */
+ regmap_write(lt9611->regmap, 0x8130, 0xea);
+}
+
+static void lt9611_bridge_disable(struct drm_bridge *bridge)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+ int ret;
+
+ dev_dbg(lt9611->dev, "bridge disable\n");
+
+ /* Disable HDMI output */
+ ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
+ if (ret) {
+ dev_err(lt9611->dev, "video on failed\n");
+ return;
+ }
+
+ if (lt9611_power_off(lt9611)) {
+ dev_err(lt9611->dev, "power on failed\n");
+ return;
+ }
+}
+
+static struct
+drm_connector_helper_funcs lt9611_bridge_connector_helper_funcs = {
+ .get_modes = lt9611_connector_get_modes,
+ .mode_valid = lt9611_connector_mode_valid,
+};
+
+static const struct drm_connector_funcs lt9611_bridge_connector_funcs = {
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .detect = lt9611_connector_detect,
+ .destroy = drm_connector_cleanup,
+ .reset = drm_atomic_helper_connector_reset,
+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
+ struct device_node *dsi_node)
+{
+ const struct mipi_dsi_device_info info = { "lt9611", 0, NULL };
+ struct mipi_dsi_device *dsi;
+ struct mipi_dsi_host *host;
+ int ret;
+
+ host = of_find_mipi_dsi_host_by_node(dsi_node);
+ if (!host) {
+ dev_err(lt9611->dev, "failed to find dsi host\n");
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ dsi = mipi_dsi_device_register_full(host, &info);
+ if (IS_ERR(dsi)) {
+ dev_err(lt9611->dev, "failed to create dsi device\n");
+ return dsi;
+ }
+
+ dsi->lanes = 4;
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+ MIPI_DSI_MODE_VIDEO_HSE;
+
+ ret = mipi_dsi_attach(dsi);
+ if (ret < 0) {
+ dev_err(lt9611->dev, "failed to attach dsi to host\n");
+ mipi_dsi_device_unregister(dsi);
+ return ERR_PTR(ret);
+ }
+
+ return dsi;
+}
+
+static int lt9611_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+ int ret;
+
+ dev_dbg(lt9611->dev, "bridge attach\n");
+
+ ret = drm_connector_init(bridge->dev, <9611->connector,
+ <9611_bridge_connector_funcs,
+ DRM_MODE_CONNECTOR_HDMIA);
+ if (ret) {
+ DRM_ERROR("Failed to initialize connector with drm\n");
+ return ret;
+ }
+
+ drm_connector_helper_add(<9611->connector,
+ <9611_bridge_connector_helper_funcs);
+ drm_connector_attach_encoder(<9611->connector, bridge->encoder);
+
+ if (!bridge->encoder) {
+ DRM_ERROR("Parent encoder object not found");
+ return -ENODEV;
+ }
+
+ /* Attach primary DSI */
+ lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node);
+ if (IS_ERR(lt9611->dsi0))
+ return PTR_ERR(lt9611->dsi0);
+
+ /* Attach secondary DSI, if specified */
+ if (lt9611->dsi1_node) {
+ lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
+ if (IS_ERR(lt9611->dsi1)) {
+ ret = PTR_ERR(lt9611->dsi1);
+ goto err_unregister_dsi0;
+ }
+ }
+
+ return 0;
+
+err_unregister_dsi0:
+ mipi_dsi_device_unregister(lt9611->dsi0);
+
+ return ret;
+}
+
+static void lt9611_bridge_detach(struct drm_bridge *bridge)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+ if (lt9611->dsi1) {
+ mipi_dsi_detach(lt9611->dsi1);
+ mipi_dsi_device_unregister(lt9611->dsi1);
+ }
+
+ mipi_dsi_detach(lt9611->dsi0);
+ mipi_dsi_device_unregister(lt9611->dsi0);
+}
+
+static enum drm_mode_status
+lt9611_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_mode *mode)
+{
+ struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+ if (lt9611_mode->intfs > 1 && !lt9611->dsi1)
+ return MODE_PANEL;
+ else
+ return MODE_OK;
+}
+
+static void lt9611_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+ dev_dbg(lt9611->dev, "bridge pre_enable\n");
+
+ if (!lt9611->sleep)
+ return;
+
+ lt9611_reset(lt9611);
+ regmap_write(lt9611->regmap, 0x80ee, 0x01);
+
+ lt9611->sleep = false;
+}
+
+static void lt9611_bridge_post_disable(struct drm_bridge *bridge)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+
+ dev_dbg(lt9611->dev, "bridge post_disable\n");
+
+ lt9611_sleep_setup(lt9611);
+}
+
+static void lt9611_bridge_mode_set(struct drm_bridge *bridge,
+ const struct drm_display_mode *mode,
+ const struct drm_display_mode *adj_mode)
+{
+ struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+ struct hdmi_avi_infoframe avi_frame;
+ int ret;
+
+ dev_dbg(lt9611->dev, "bridge mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
+ adj_mode->hdisplay, adj_mode->vdisplay,
+ adj_mode->vrefresh, adj_mode->clock);
+
+ lt9611_bridge_pre_enable(bridge);
+
+ lt9611_mipi_input_digital(lt9611, mode);
+ lt9611_pll_setup(lt9611, mode);
+ lt9611_mipi_video_setup(lt9611, mode);
+ lt9611_pcr_setup(lt9611, mode);
+
+ ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
+ <9611->connector,
+ mode);
+ if (!ret)
+ lt9611->vic = avi_frame.video_code;
+}
+
+static const struct drm_bridge_funcs lt9611_bridge_funcs = {
+ .attach = lt9611_bridge_attach,
+ .detach = lt9611_bridge_detach,
+ .mode_valid = lt9611_bridge_mode_valid,
+ .enable = lt9611_bridge_enable,
+ .disable = lt9611_bridge_disable,
+ .post_disable = lt9611_bridge_post_disable,
+ .mode_set = lt9611_bridge_mode_set,
+};
+
+static int lt9611_parse_dt(struct device *dev,
+ struct lt9611 *lt9611)
+{
+ lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 1, -1);
+ if (!lt9611->dsi0_node) {
+ DRM_DEV_ERROR(dev, "failed to get remote node for primary dsi\n");
+ return -ENODEV;
+ }
+
+ lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 2, -1);
+
+ lt9611->ac_mode = of_property_read_bool(dev->of_node, "lt,ac-mode");
+ dev_dbg(lt9611->dev, "ac_mode=%d\n", lt9611->ac_mode);
+
+ return 0;
+}
+
+static int lt9611_gpio_init(struct lt9611 *lt9611)
+{
+ struct device *dev = lt9611->dev;
+
+ lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(lt9611->reset_gpio)) {
+ dev_err(dev, "failed to acquire reset gpio\n");
+ return PTR_ERR(lt9611->reset_gpio);
+ }
+
+ lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(lt9611->enable_gpio)) {
+ dev_err(dev, "failed to acquire enable gpio\n");
+ return PTR_ERR(lt9611->enable_gpio);
+ }
+
+ return 0;
+}
+
+static int lt9611_read_device_rev(struct lt9611 *lt9611)
+{
+ unsigned int rev;
+ int ret;
+
+ regmap_write(lt9611->regmap, 0x80ee, 0x01);
+ ret = regmap_read(lt9611->regmap, 0x8002, &rev);
+ if (ret)
+ dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
+
+ dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
+
+ return ret;
+}
+
+static int lt9611_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct lt9611 *lt9611;
+ struct device *dev = &client->dev;
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(dev, "device doesn't support I2C\n");
+ return -ENODEV;
+ }
+
+ lt9611 = devm_kzalloc(dev, sizeof(*lt9611), GFP_KERNEL);
+ if (!lt9611)
+ return -ENOMEM;
+
+ lt9611->dev = &client->dev;
+ lt9611->client = client;
+ lt9611->sleep = false;
+
+ lt9611->regmap = devm_regmap_init_i2c(client, <9611_regmap_config);
+ if (IS_ERR(lt9611->regmap)) {
+ DRM_ERROR("regmap i2c init failed\n");
+ return PTR_ERR(lt9611->regmap);
+ }
+
+ ret = lt9611_parse_dt(&client->dev, lt9611);
+ if (ret) {
+ dev_err(dev, "failed to parse device tree\n");
+ return ret;
+ }
+
+ ret = lt9611_gpio_init(lt9611);
+ if (ret < 0)
+ return ret;
+
+ ret = lt9611_regulator_init(lt9611);
+ if (ret < 0)
+ return ret;
+
+ lt9611_assert_5v(lt9611);
+
+ ret = lt9611_regulator_enable(lt9611);
+ if (ret)
+ return ret;
+
+ lt9611_reset(lt9611);
+
+ ret = lt9611_read_device_rev(lt9611);
+ if (ret) {
+ dev_err(dev, "failed to read chip rev\n");
+ goto err_disable_regulators;
+ }
+
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ lt9611_irq_thread_handler,
+ IRQF_ONESHOT, "lt9611", lt9611);
+ if (ret) {
+ dev_err(dev, "failed to request irq\n");
+ goto err_disable_regulators;
+ }
+
+ i2c_set_clientdata(client, lt9611);
+
+ lt9611->bridge.funcs = <9611_bridge_funcs;
+ lt9611->bridge.of_node = client->dev.of_node;
+
+ drm_bridge_add(<9611->bridge);
+
+ lt9611_enable_hpd_interrupts(lt9611);
+
+ return 0;
+
+err_disable_regulators:
+ regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
+
+ of_node_put(lt9611->dsi0_node);
+ of_node_put(lt9611->dsi1_node);
+
+ return ret;
+}
+
+static int lt9611_remove(struct i2c_client *client)
+{
+ struct lt9611 *lt9611 = i2c_get_clientdata(client);
+
+ disable_irq(client->irq);
+ drm_bridge_remove(<9611->bridge);
+
+ regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
+
+ of_node_put(lt9611->dsi0_node);
+ of_node_put(lt9611->dsi1_node);
+
+ return 0;
+}
+
+static struct i2c_device_id lt9611_id[] = {
+ { "lontium,lt9611", 0},
+ {}
+};
+
+static const struct of_device_id lt9611_match_table[] = {
+ {.compatible = "lontium,lt9611"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, lt9611_match_table);
+
+static struct i2c_driver lt9611_driver = {
+ .driver = {
+ .name = "lt9611",
+ .of_match_table = lt9611_match_table,
+ },
+ .probe = lt9611_probe,
+ .remove = lt9611_remove,
+ .id_table = lt9611_id,
+};
+module_i2c_driver(lt9611_driver);
+
+MODULE_LICENSE("GPL v2");
--
2.25.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-13 10:05 ` Vinod Koul
(?)
@ 2020-05-13 16:14 ` kbuild test robot
-1 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:14 UTC (permalink / raw)
To: Vinod Koul, Daniel Vetter, David Airlie, Rob Clark
Cc: kbuild-all, devicetree, Jernej Skrabec, Neil Armstrong,
linux-arm-msm, Jonas Karlman, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 10464 bytes --]
Hi Vinod,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
518 | gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get' [-Werror=implicit-function-declaration]
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
969 | lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
| ^~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_optional
drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
970 | GPIOD_OUT_LOW);
| ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: At top level:
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: data definition has no type or storage class
1100 | MODULE_DEVICE_TABLE(of, lt9611_match_table);
| ^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1100:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: parameter names (without types) in function declaration
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:263:1: warning: data definition has no type or storage class
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:263:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int]
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:268:1: warning: data definition has no type or storage class
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:268:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int]
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1113:16: error: expected declaration specifiers or '...' before string constant
1113 | MODULE_LICENSE("GPL v2");
| ^~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
drivers/gpu/drm/bridge/lt9611.c:1111:19: warning: 'lt9611_driver_init' defined but not used [-Wunused-function]
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~
include/linux/device/driver.h:259:19: note: in definition of macro 'module_driver'
259 | static int __init __driver##_init(void) | ^~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +19 include/linux/export.h
b67067f1176df6 Nicholas Piggin 2016-08-24 4
f50169324df4ad Paul Gortmaker 2011-05-23 5 /*
f50169324df4ad Paul Gortmaker 2011-05-23 6 * Export symbols from the kernel to modules. Forked from module.h
f50169324df4ad Paul Gortmaker 2011-05-23 7 * to reduce the amount of pointless cruft we feed to gcc when only
f50169324df4ad Paul Gortmaker 2011-05-23 8 * exporting a simple symbol or two.
f50169324df4ad Paul Gortmaker 2011-05-23 9 *
b92021b09df70c Rusty Russell 2013-03-15 10 * Try not to add #includes here. It slows compilation and makes kernel
b92021b09df70c Rusty Russell 2013-03-15 11 * hackers place grumpy comments in header files.
f50169324df4ad Paul Gortmaker 2011-05-23 12 */
f50169324df4ad Paul Gortmaker 2011-05-23 13
b92021b09df70c Rusty Russell 2013-03-15 14 #ifndef __ASSEMBLY__
f50169324df4ad Paul Gortmaker 2011-05-23 15 #ifdef MODULE
f50169324df4ad Paul Gortmaker 2011-05-23 16 extern struct module __this_module;
f50169324df4ad Paul Gortmaker 2011-05-23 17 #define THIS_MODULE (&__this_module)
f50169324df4ad Paul Gortmaker 2011-05-23 18 #else
f50169324df4ad Paul Gortmaker 2011-05-23 @19 #define THIS_MODULE ((struct module *)0)
f50169324df4ad Paul Gortmaker 2011-05-23 20 #endif
f50169324df4ad Paul Gortmaker 2011-05-23 21
:::::: The code at line 19 was first introduced by commit
:::::: f50169324df4ad942e544386d136216c8617636a module.h: split out the EXPORT_SYMBOL into export.h
:::::: TO: Paul Gortmaker <paul.gortmaker@windriver.com>
:::::: CC: Paul Gortmaker <paul.gortmaker@windriver.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55353 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 16:14 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:14 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 10652 bytes --]
Hi Vinod,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
518 | gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get' [-Werror=implicit-function-declaration]
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
969 | lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
| ^~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_optional
drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
970 | GPIOD_OUT_LOW);
| ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: At top level:
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: data definition has no type or storage class
1100 | MODULE_DEVICE_TABLE(of, lt9611_match_table);
| ^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1100:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: parameter names (without types) in function declaration
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:263:1: warning: data definition has no type or storage class
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:263:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int]
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:268:1: warning: data definition has no type or storage class
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:268:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int]
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1113:16: error: expected declaration specifiers or '...' before string constant
1113 | MODULE_LICENSE("GPL v2");
| ^~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
drivers/gpu/drm/bridge/lt9611.c:1111:19: warning: 'lt9611_driver_init' defined but not used [-Wunused-function]
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~
include/linux/device/driver.h:259:19: note: in definition of macro 'module_driver'
259 | static int __init __driver##_init(void) | ^~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +19 include/linux/export.h
b67067f1176df6 Nicholas Piggin 2016-08-24 4
f50169324df4ad Paul Gortmaker 2011-05-23 5 /*
f50169324df4ad Paul Gortmaker 2011-05-23 6 * Export symbols from the kernel to modules. Forked from module.h
f50169324df4ad Paul Gortmaker 2011-05-23 7 * to reduce the amount of pointless cruft we feed to gcc when only
f50169324df4ad Paul Gortmaker 2011-05-23 8 * exporting a simple symbol or two.
f50169324df4ad Paul Gortmaker 2011-05-23 9 *
b92021b09df70c Rusty Russell 2013-03-15 10 * Try not to add #includes here. It slows compilation and makes kernel
b92021b09df70c Rusty Russell 2013-03-15 11 * hackers place grumpy comments in header files.
f50169324df4ad Paul Gortmaker 2011-05-23 12 */
f50169324df4ad Paul Gortmaker 2011-05-23 13
b92021b09df70c Rusty Russell 2013-03-15 14 #ifndef __ASSEMBLY__
f50169324df4ad Paul Gortmaker 2011-05-23 15 #ifdef MODULE
f50169324df4ad Paul Gortmaker 2011-05-23 16 extern struct module __this_module;
f50169324df4ad Paul Gortmaker 2011-05-23 17 #define THIS_MODULE (&__this_module)
f50169324df4ad Paul Gortmaker 2011-05-23 18 #else
f50169324df4ad Paul Gortmaker 2011-05-23 @19 #define THIS_MODULE ((struct module *)0)
f50169324df4ad Paul Gortmaker 2011-05-23 20 #endif
f50169324df4ad Paul Gortmaker 2011-05-23 21
:::::: The code at line 19 was first introduced by commit
:::::: f50169324df4ad942e544386d136216c8617636a module.h: split out the EXPORT_SYMBOL into export.h
:::::: TO: Paul Gortmaker <paul.gortmaker@windriver.com>
:::::: CC: Paul Gortmaker <paul.gortmaker@windriver.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 55353 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 16:14 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:14 UTC (permalink / raw)
To: Vinod Koul, Daniel Vetter, David Airlie, Rob Clark
Cc: devicetree, Jernej Skrabec, kbuild-all, Jonas Karlman,
linux-arm-msm, Neil Armstrong, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 10464 bytes --]
Hi Vinod,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
518 | gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get' [-Werror=implicit-function-declaration]
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
963 | lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
| ^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
969 | lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
| ^~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_optional
drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
970 | GPIOD_OUT_LOW);
| ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: At top level:
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: data definition has no type or storage class
1100 | MODULE_DEVICE_TABLE(of, lt9611_match_table);
| ^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1100:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
drivers/gpu/drm/bridge/lt9611.c:1100:1: warning: parameter names (without types) in function declaration
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:263:1: warning: data definition has no type or storage class
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:263:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int]
263 | module_init(__driver##_init); | ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
include/linux/device/driver.h:268:1: warning: data definition has no type or storage class
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:268:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int]
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/asm-generic/bug.h:19,
from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/i2c.h:855:22: note: in expansion of macro 'THIS_MODULE'
855 | i2c_register_driver(THIS_MODULE, driver)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'i2c_add_driver'
261 | return __register(&(__driver) , ##__VA_ARGS__); | ^~~~~~~~~~
include/linux/i2c.h:923:2: note: in expansion of macro 'module_driver'
923 | module_driver(__i2c_driver, i2c_add_driver, | ^~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1113:16: error: expected declaration specifiers or '...' before string constant
1113 | MODULE_LICENSE("GPL v2");
| ^~~~~~~~
In file included from include/linux/device.h:31,
from include/linux/platform_device.h:13,
from drivers/gpu/drm/bridge/lt9611.c:7:
drivers/gpu/drm/bridge/lt9611.c:1111:19: warning: 'lt9611_driver_init' defined but not used [-Wunused-function]
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~
include/linux/device/driver.h:259:19: note: in definition of macro 'module_driver'
259 | static int __init __driver##_init(void) | ^~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:1111:1: note: in expansion of macro 'module_i2c_driver'
1111 | module_i2c_driver(lt9611_driver);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +19 include/linux/export.h
b67067f1176df6 Nicholas Piggin 2016-08-24 4
f50169324df4ad Paul Gortmaker 2011-05-23 5 /*
f50169324df4ad Paul Gortmaker 2011-05-23 6 * Export symbols from the kernel to modules. Forked from module.h
f50169324df4ad Paul Gortmaker 2011-05-23 7 * to reduce the amount of pointless cruft we feed to gcc when only
f50169324df4ad Paul Gortmaker 2011-05-23 8 * exporting a simple symbol or two.
f50169324df4ad Paul Gortmaker 2011-05-23 9 *
b92021b09df70c Rusty Russell 2013-03-15 10 * Try not to add #includes here. It slows compilation and makes kernel
b92021b09df70c Rusty Russell 2013-03-15 11 * hackers place grumpy comments in header files.
f50169324df4ad Paul Gortmaker 2011-05-23 12 */
f50169324df4ad Paul Gortmaker 2011-05-23 13
b92021b09df70c Rusty Russell 2013-03-15 14 #ifndef __ASSEMBLY__
f50169324df4ad Paul Gortmaker 2011-05-23 15 #ifdef MODULE
f50169324df4ad Paul Gortmaker 2011-05-23 16 extern struct module __this_module;
f50169324df4ad Paul Gortmaker 2011-05-23 17 #define THIS_MODULE (&__this_module)
f50169324df4ad Paul Gortmaker 2011-05-23 18 #else
f50169324df4ad Paul Gortmaker 2011-05-23 @19 #define THIS_MODULE ((struct module *)0)
f50169324df4ad Paul Gortmaker 2011-05-23 20 #endif
f50169324df4ad Paul Gortmaker 2011-05-23 21
:::::: The code at line 19 was first introduced by commit
:::::: f50169324df4ad942e544386d136216c8617636a module.h: split out the EXPORT_SYMBOL into export.h
:::::: TO: Paul Gortmaker <paul.gortmaker@windriver.com>
:::::: CC: Paul Gortmaker <paul.gortmaker@windriver.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55353 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-13 10:05 ` Vinod Koul
(?)
@ 2020-05-13 16:46 ` kbuild test robot
-1 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:46 UTC (permalink / raw)
To: Vinod Koul, Daniel Vetter, David Airlie, Rob Clark
Cc: kbuild-all, devicetree, Jernej Skrabec, Neil Armstrong,
linux-arm-msm, Jonas Karlman, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]
Hi Vinod,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
>> drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
>> drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get'; did you mean 'devm_iounmap'? [-Werror=implicit-function-declaration]
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
devm_iounmap
>> drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
^~~~~~~~~~~~~~~~~~~~~~~
devm_regulator_get_optional
>> drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOD_OUT_HIGH'?
GPIOD_OUT_LOW);
^~~~~~~~~~~~~
GPIOD_OUT_HIGH
cc1: some warnings being treated as errors
vim +/gpiod_set_value_cansleep +518 drivers/gpu/drm/bridge/lt9611.c
515
516 static void lt9611_reset(struct lt9611 *lt9611)
517 {
> 518 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
519 msleep(20);
520 gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
521 msleep(20);
522 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
523 msleep(100);
524 }
525
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72238 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 16:46 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:46 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2995 bytes --]
Hi Vinod,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
>> drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
>> drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get'; did you mean 'devm_iounmap'? [-Werror=implicit-function-declaration]
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
devm_iounmap
>> drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
^~~~~~~~~~~~~~~~~~~~~~~
devm_regulator_get_optional
>> drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOD_OUT_HIGH'?
GPIOD_OUT_LOW);
^~~~~~~~~~~~~
GPIOD_OUT_HIGH
cc1: some warnings being treated as errors
vim +/gpiod_set_value_cansleep +518 drivers/gpu/drm/bridge/lt9611.c
515
516 static void lt9611_reset(struct lt9611 *lt9611)
517 {
> 518 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
519 msleep(20);
520 gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
521 msleep(20);
522 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
523 msleep(100);
524 }
525
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 72238 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 16:46 ` kbuild test robot
0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2020-05-13 16:46 UTC (permalink / raw)
To: Vinod Koul, Daniel Vetter, David Airlie, Rob Clark
Cc: devicetree, Jernej Skrabec, kbuild-all, Jonas Karlman,
linux-arm-msm, Neil Armstrong, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda
[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]
Hi Vinod,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip linus/master v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-LT9611-DSI-to-HDMI-bridge/20200513-181150
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_reset':
>> drivers/gpu/drm/bridge/lt9611.c:518:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c: In function 'lt9611_gpio_init':
>> drivers/gpu/drm/bridge/lt9611.c:963:23: error: implicit declaration of function 'devm_gpiod_get'; did you mean 'devm_iounmap'? [-Werror=implicit-function-declaration]
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
devm_iounmap
>> drivers/gpu/drm/bridge/lt9611.c:963:52: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
^~~~~~~~~~~~~~
drivers/gpu/drm/bridge/lt9611.c:963:52: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/bridge/lt9611.c:969:24: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_regulator_get_optional'? [-Werror=implicit-function-declaration]
lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
^~~~~~~~~~~~~~~~~~~~~~~
devm_regulator_get_optional
>> drivers/gpu/drm/bridge/lt9611.c:970:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOD_OUT_HIGH'?
GPIOD_OUT_LOW);
^~~~~~~~~~~~~
GPIOD_OUT_HIGH
cc1: some warnings being treated as errors
vim +/gpiod_set_value_cansleep +518 drivers/gpu/drm/bridge/lt9611.c
515
516 static void lt9611_reset(struct lt9611 *lt9611)
517 {
> 518 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
519 msleep(20);
520 gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
521 msleep(20);
522 gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
523 msleep(100);
524 }
525
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72238 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-13 10:05 ` Vinod Koul
@ 2020-05-13 19:20 ` Emil Velikov
-1 siblings, 0 replies; 34+ messages in thread
From: Emil Velikov @ 2020-05-13 19:20 UTC (permalink / raw)
To: Vinod Koul
Cc: Daniel Vetter, David Airlie, Rob Clark, devicetree,
Jernej Skrabec, Neil Armstrong, linux-arm-msm, Jonas Karlman,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel, Bjorn Andersson,
Andrzej Hajda, Rob Herring, Laurent Pinchart
Hi Vinod,
Few high-level comments:
- handful of functions always return 0 and the return value is never
checked - switch to return void
- annotate all (nearly) arrays as static const
- consistently use multi_reg_write - in some cases non-const array
will be fine, overwriting a few entries as needed
- there is very partial comments about the registers/values - missing docs or?
Personally I'm in favour of using symbolic names, instead of
hex+comment. Considering how partial the comments are, current
approach is perfectly fine.
On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
>
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c
Please add a vendor prefix to the filename.
> @@ -0,0 +1,1113 @@
> +struct lt9611_mode {
> + u16 hdisplay;
> + u16 vdisplay;
> + u8 fps;
We all enjoy the odd fps game, but let's use vrefresh here.
> + u8 lanes;
> + u8 intfs;
> +};
> +
> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> + if (mode->hdisplay == 3840)
> + regmap_write(lt9611->regmap, 0x830a, 0x03);
> + else
> + regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> + regmap_write(lt9611->regmap, 0x824f, 0x80);
> + regmap_write(lt9611->regmap, 0x8250, 0x10);
> + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> + regmap_write(lt9611->regmap, 0x8306, 0x0a);
Create an (non-const) array, overwriting the [1] entry for 3840 mode?
> +
> + return 0;
Kill return type.
> +}
> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + struct reg_sequence reg_cfg[] = {
static const?
> + { 0x830b, 0x01 },
> + { 0x830c, 0x10 },
> + { 0x8348, 0x00 },
> + { 0x8349, 0x81 },
> +
> + /* stage 1 */
> + { 0x8321, 0x4a },
> + { 0x8324, 0x71 },
> + { 0x8325, 0x30 },
> + { 0x832a, 0x01 },
> +
> + /* stage 2 */
> + { 0x834a, 0x40 },
> + { 0x831d, 0x10 },
> +
> + /* MK limit */
> + { 0x832d, 0x38 },
> + { 0x8331, 0x08 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + switch (mode->hdisplay) {
> + case 640:
> + regmap_write(lt9611->regmap, 0x8326, 0x14);
> + break;
> + case 1920:
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + case 3840:
> + regmap_write(lt9611->regmap, 0x830b, 0x03);
> + regmap_write(lt9611->regmap, 0x830c, 0xd0);
> + regmap_write(lt9611->regmap, 0x8348, 0x03);
> + regmap_write(lt9611->regmap, 0x8349, 0xe0);
> + regmap_write(lt9611->regmap, 0x8324, 0x72);
> + regmap_write(lt9611->regmap, 0x8325, 0x00);
> + regmap_write(lt9611->regmap, 0x832a, 0x01);
> + regmap_write(lt9611->regmap, 0x834a, 0x10);
> + regmap_write(lt9611->regmap, 0x831d, 0x10);
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
Throw this in another const array?
> + break;
> + }
> +
> + /* pcr rst */
> + regmap_write(lt9611->regmap, 0x8011, 0x5a);
> + regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> + return 0;
> +}
> + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
Comment does not match the code.
We're discarding the LSB, so we cannot realistically be writing
pclk[7:0]. Similar applies for the other two.
> + /* v_act */
> + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> + if (ret)
> + goto end;
> +
> + v_act = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> + if (ret)
> + goto end;
> + v_act = v_act + temp;
> +
Having a helper for the above "result = read(x) << 8 | read(x+1)"
would be great.
This way one doesn't have to repeat the pattern 4-5 times.
> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> + unsigned int temp;
> + int ret = 0;
> + int i, j;
> +
> + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
How about:
memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));
Then again, do we need the memset()? We are allocating the memory with
devm_kzalloc()
> +
> + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> + /* 0xA0 is EDID device address */
> + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> + /* 0x00 is EDID offset address */
> + regmap_write(lt9611->regmap, 0x8505, 0x00);
> + /* length for read */
> + regmap_write(lt9611->regmap, 0x8506, 0x20);
Is this the same 32 as seen in the loops below? #define and use consistently?
> + regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> + for (i = 0 ; i < 8 ; i++) {
Add a #define for the magic 8
> + /* offset address */
> + regmap_write(lt9611->regmap, 0x8505, i * 32);
> + regmap_write(lt9611->regmap, 0x8507, 0x36);
> + regmap_write(lt9611->regmap, 0x8507, 0x31);
> + regmap_write(lt9611->regmap, 0x8507, 0x37);
> + usleep_range(5000, 10000);
> +
> + regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> + if (temp & 0x02) { /*KEY_DDC_ACCS_DONE=1*/
Use #define KEY_DDC_ACCS_DONE 0x02
> + for (j = 0; j < 32; j++) {
Another #define for 32
> + regmap_read(lt9611->regmap, 0x8583, &temp);
> + lt9611->edid_buf[i * 32 + j] = temp;
> + }
> + } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> + dev_err(lt9611->dev, "read edid failed: no ack\n");
> + ret = -EIO;
> + goto end;
> + } else {
> + dev_err(lt9611->dev,
> + "read edid failed: access not done\n");
> + ret = -EIO;
> + goto end;
> + }
> + }
> +
> + dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> + lt9611->edid_buf[255]);
> +
> +end:
> + regmap_write(lt9611->regmap, 0x8507, 0x1f);
> + return ret;
> +}
> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> + struct lt9611 *lt9611 = data;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> + block, (int)len);
> +
> + if (len > 128)
> + return -EINVAL;
> +
> + /* support up to 1 extension block */
Move the TODO here?
> + if (block > 1)
> + return -EINVAL;
> +
> + if (block == 0) {
> + /* always read 2 edid blocks once */
Please mention why that's a good idea. From memory - there aren't many
other drivers that do this.
> + ret = lt9611_read_edid(lt9611);
> + if (ret) {
> + dev_err(lt9611->dev, "edid read failed\n");
> + return ret;
> + }
> + }
> +
> + if (block % 2 == 0)
> + memcpy(buf, lt9611->edid_buf, len);
> + else
> + memcpy(buf, lt9611->edid_buf + 128, len);
The above can be written as:
memcpy(buf, lt9611->edid_buf + (block * 128), len);
> +
> + return 0;
> +}
> +
> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + /* Attach secondary DSI, if specified */
> + if (lt9611->dsi1_node) {
> + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> + if (IS_ERR(lt9611->dsi1)) {
> + ret = PTR_ERR(lt9611->dsi1);
> + goto err_unregister_dsi0;
> + }
> + }
> +
> + return 0;
> +
> +err_unregister_dsi0:
Missing detach? If possible directly use lt9611_bridge_detach().
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +
> + return ret;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> + unsigned int rev;
> + int ret;
> +
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> + if (ret)
> + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
The "failed" message will be followed by printing random kernel memory.
Initialize rev to some dummy number or omit the dev_info.
> + dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> + return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + ret = lt9611_parse_dt(&client->dev, lt9611);
> + if (ret) {
> + dev_err(dev, "failed to parse device tree\n");
> + return ret;
> + }
> +
> + ret = lt9611_gpio_init(lt9611);
> + if (ret < 0)
Missing of_node_put() here and for the next few error paths.
> + return ret;
> +
> + ret = lt9611_regulator_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + lt9611_assert_5v(lt9611);
> +
> + ret = lt9611_regulator_enable(lt9611);
> + if (ret)
> + return ret;
> +
> + return 0;
> +
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
Use the inverse order wrt the get() operation.
> +
> + return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> + struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> + disable_irq(client->irq);
> + drm_bridge_remove(<9611->bridge);
> +
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
Flip the order - dsi1, then dsi0
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> + { "lontium,lt9611", 0},
> + {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> + {.compatible = "lontium,lt9611"},
In the above two - add space after { and before }. Pretty sure
./scripts/checkpatch.pl will complain about those.
Might want to double-check for other issues reported by said tool.
-Emil
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-13 19:20 ` Emil Velikov
0 siblings, 0 replies; 34+ messages in thread
From: Emil Velikov @ 2020-05-13 19:20 UTC (permalink / raw)
To: Vinod Koul
Cc: devicetree, Jernej Skrabec, Neil Armstrong, David Airlie,
linux-arm-msm, Jonas Karlman, Linux-Kernel@Vger. Kernel. Org,
ML dri-devel, Bjorn Andersson, Andrzej Hajda, Rob Herring,
Laurent Pinchart
Hi Vinod,
Few high-level comments:
- handful of functions always return 0 and the return value is never
checked - switch to return void
- annotate all (nearly) arrays as static const
- consistently use multi_reg_write - in some cases non-const array
will be fine, overwriting a few entries as needed
- there is very partial comments about the registers/values - missing docs or?
Personally I'm in favour of using symbolic names, instead of
hex+comment. Considering how partial the comments are, current
approach is perfectly fine.
On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
>
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c
Please add a vendor prefix to the filename.
> @@ -0,0 +1,1113 @@
> +struct lt9611_mode {
> + u16 hdisplay;
> + u16 vdisplay;
> + u8 fps;
We all enjoy the odd fps game, but let's use vrefresh here.
> + u8 lanes;
> + u8 intfs;
> +};
> +
> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> + if (mode->hdisplay == 3840)
> + regmap_write(lt9611->regmap, 0x830a, 0x03);
> + else
> + regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> + regmap_write(lt9611->regmap, 0x824f, 0x80);
> + regmap_write(lt9611->regmap, 0x8250, 0x10);
> + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> + regmap_write(lt9611->regmap, 0x8306, 0x0a);
Create an (non-const) array, overwriting the [1] entry for 3840 mode?
> +
> + return 0;
Kill return type.
> +}
> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + struct reg_sequence reg_cfg[] = {
static const?
> + { 0x830b, 0x01 },
> + { 0x830c, 0x10 },
> + { 0x8348, 0x00 },
> + { 0x8349, 0x81 },
> +
> + /* stage 1 */
> + { 0x8321, 0x4a },
> + { 0x8324, 0x71 },
> + { 0x8325, 0x30 },
> + { 0x832a, 0x01 },
> +
> + /* stage 2 */
> + { 0x834a, 0x40 },
> + { 0x831d, 0x10 },
> +
> + /* MK limit */
> + { 0x832d, 0x38 },
> + { 0x8331, 0x08 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + switch (mode->hdisplay) {
> + case 640:
> + regmap_write(lt9611->regmap, 0x8326, 0x14);
> + break;
> + case 1920:
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + case 3840:
> + regmap_write(lt9611->regmap, 0x830b, 0x03);
> + regmap_write(lt9611->regmap, 0x830c, 0xd0);
> + regmap_write(lt9611->regmap, 0x8348, 0x03);
> + regmap_write(lt9611->regmap, 0x8349, 0xe0);
> + regmap_write(lt9611->regmap, 0x8324, 0x72);
> + regmap_write(lt9611->regmap, 0x8325, 0x00);
> + regmap_write(lt9611->regmap, 0x832a, 0x01);
> + regmap_write(lt9611->regmap, 0x834a, 0x10);
> + regmap_write(lt9611->regmap, 0x831d, 0x10);
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
Throw this in another const array?
> + break;
> + }
> +
> + /* pcr rst */
> + regmap_write(lt9611->regmap, 0x8011, 0x5a);
> + regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> + return 0;
> +}
> + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
Comment does not match the code.
We're discarding the LSB, so we cannot realistically be writing
pclk[7:0]. Similar applies for the other two.
> + /* v_act */
> + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> + if (ret)
> + goto end;
> +
> + v_act = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> + if (ret)
> + goto end;
> + v_act = v_act + temp;
> +
Having a helper for the above "result = read(x) << 8 | read(x+1)"
would be great.
This way one doesn't have to repeat the pattern 4-5 times.
> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> + unsigned int temp;
> + int ret = 0;
> + int i, j;
> +
> + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
How about:
memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));
Then again, do we need the memset()? We are allocating the memory with
devm_kzalloc()
> +
> + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> + /* 0xA0 is EDID device address */
> + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> + /* 0x00 is EDID offset address */
> + regmap_write(lt9611->regmap, 0x8505, 0x00);
> + /* length for read */
> + regmap_write(lt9611->regmap, 0x8506, 0x20);
Is this the same 32 as seen in the loops below? #define and use consistently?
> + regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> + for (i = 0 ; i < 8 ; i++) {
Add a #define for the magic 8
> + /* offset address */
> + regmap_write(lt9611->regmap, 0x8505, i * 32);
> + regmap_write(lt9611->regmap, 0x8507, 0x36);
> + regmap_write(lt9611->regmap, 0x8507, 0x31);
> + regmap_write(lt9611->regmap, 0x8507, 0x37);
> + usleep_range(5000, 10000);
> +
> + regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> + if (temp & 0x02) { /*KEY_DDC_ACCS_DONE=1*/
Use #define KEY_DDC_ACCS_DONE 0x02
> + for (j = 0; j < 32; j++) {
Another #define for 32
> + regmap_read(lt9611->regmap, 0x8583, &temp);
> + lt9611->edid_buf[i * 32 + j] = temp;
> + }
> + } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> + dev_err(lt9611->dev, "read edid failed: no ack\n");
> + ret = -EIO;
> + goto end;
> + } else {
> + dev_err(lt9611->dev,
> + "read edid failed: access not done\n");
> + ret = -EIO;
> + goto end;
> + }
> + }
> +
> + dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> + lt9611->edid_buf[255]);
> +
> +end:
> + regmap_write(lt9611->regmap, 0x8507, 0x1f);
> + return ret;
> +}
> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> + struct lt9611 *lt9611 = data;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> + block, (int)len);
> +
> + if (len > 128)
> + return -EINVAL;
> +
> + /* support up to 1 extension block */
Move the TODO here?
> + if (block > 1)
> + return -EINVAL;
> +
> + if (block == 0) {
> + /* always read 2 edid blocks once */
Please mention why that's a good idea. From memory - there aren't many
other drivers that do this.
> + ret = lt9611_read_edid(lt9611);
> + if (ret) {
> + dev_err(lt9611->dev, "edid read failed\n");
> + return ret;
> + }
> + }
> +
> + if (block % 2 == 0)
> + memcpy(buf, lt9611->edid_buf, len);
> + else
> + memcpy(buf, lt9611->edid_buf + 128, len);
The above can be written as:
memcpy(buf, lt9611->edid_buf + (block * 128), len);
> +
> + return 0;
> +}
> +
> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + /* Attach secondary DSI, if specified */
> + if (lt9611->dsi1_node) {
> + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> + if (IS_ERR(lt9611->dsi1)) {
> + ret = PTR_ERR(lt9611->dsi1);
> + goto err_unregister_dsi0;
> + }
> + }
> +
> + return 0;
> +
> +err_unregister_dsi0:
Missing detach? If possible directly use lt9611_bridge_detach().
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +
> + return ret;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> + unsigned int rev;
> + int ret;
> +
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> + if (ret)
> + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
The "failed" message will be followed by printing random kernel memory.
Initialize rev to some dummy number or omit the dev_info.
> + dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> + return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + ret = lt9611_parse_dt(&client->dev, lt9611);
> + if (ret) {
> + dev_err(dev, "failed to parse device tree\n");
> + return ret;
> + }
> +
> + ret = lt9611_gpio_init(lt9611);
> + if (ret < 0)
Missing of_node_put() here and for the next few error paths.
> + return ret;
> +
> + ret = lt9611_regulator_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + lt9611_assert_5v(lt9611);
> +
> + ret = lt9611_regulator_enable(lt9611);
> + if (ret)
> + return ret;
> +
> + return 0;
> +
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
Use the inverse order wrt the get() operation.
> +
> + return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> + struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> + disable_irq(client->irq);
> + drm_bridge_remove(<9611->bridge);
> +
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
Flip the order - dsi1, then dsi0
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> + { "lontium,lt9611", 0},
> + {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> + {.compatible = "lontium,lt9611"},
In the above two - add space after { and before }. Pretty sure
./scripts/checkpatch.pl will complain about those.
Might want to double-check for other issues reported by said tool.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-13 19:20 ` Emil Velikov
@ 2020-05-14 7:06 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2020-05-14 7:06 UTC (permalink / raw)
To: Emil Velikov
Cc: Daniel Vetter, David Airlie, Rob Clark, devicetree,
Jernej Skrabec, Neil Armstrong, linux-arm-msm, Jonas Karlman,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel, Bjorn Andersson,
Andrzej Hajda, Rob Herring, Laurent Pinchart
Hello Emil,
Thanks for the comments.
On 13-05-20, 20:20, Emil Velikov wrote:
> Hi Vinod,
>
> Few high-level comments:
> - handful of functions always return 0 and the return value is never
> checked - switch to return void
Sure makes sense, will do
> - annotate all (nearly) arrays as static const
Will do
> - consistently use multi_reg_write - in some cases non-const array
> will be fine, overwriting a few entries as needed
Okay that makes sense
> - there is very partial comments about the registers/values - missing docs or?
yeah am not a big fan either, problem is documentation.
Well the spec I have doesn't have register names and few registers are
missing :( I have few name created but naming registers turned nasty
super quick.. Do let me know if you have suggestions, I will give it one
more shot though
> Personally I'm in favour of using symbolic names, instead of
> hex+comment. Considering how partial the comments are, current
> approach is perfectly fine.
>
> On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> > I2S port as an input and HDMI port as output
> >
> > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lt9611.c
>
> Please add a vendor prefix to the filename.
Okay
> > +struct lt9611_mode {
> > + u16 hdisplay;
> > + u16 vdisplay;
> > + u8 fps;
> We all enjoy the odd fps game, but let's use vrefresh here.
Sure will change
> > +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> > + const struct drm_display_mode *mode)
> > +{
> > + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> > +
> > + if (mode->hdisplay == 3840)
> > + regmap_write(lt9611->regmap, 0x830a, 0x03);
> > + else
> > + regmap_write(lt9611->regmap, 0x830a, 0x00);
> > +
> > + regmap_write(lt9611->regmap, 0x824f, 0x80);
> > + regmap_write(lt9611->regmap, 0x8250, 0x10);
> > + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> > + regmap_write(lt9611->regmap, 0x8306, 0x0a);
> Create an (non-const) array, overwriting the [1] entry for 3840 mode?
So array is the recommendation, I dont have much liking for them but I
can see they would be useful here, so will change this and other
instances and we can use regmap_multi_reg_write() while taking care of
static const for non modified arrays
>
> > +
> > + return 0;
> Kill return type.
Yup, here and other places
> > + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> > + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> > + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
> Comment does not match the code.
> We're discarding the LSB, so we cannot realistically be writing
> pclk[7:0]. Similar applies for the other two.
Thanks for pointing, will fix it up
> > + /* v_act */
> > + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> > + if (ret)
> > + goto end;
> > +
> > + v_act = temp << 8;
> > + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> > + if (ret)
> > + goto end;
> > + v_act = v_act + temp;
> > +
> Having a helper for the above "result = read(x) << 8 | read(x+1)"
> would be great.
> This way one doesn't have to repeat the pattern 4-5 times.
will add
> > +static int lt9611_read_edid(struct lt9611 *lt9611)
> > +{
> > + unsigned int temp;
> > + int ret = 0;
> > + int i, j;
> > +
> > + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> How about:
> memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));
>
> Then again, do we need the memset()? We are allocating the memory with
> devm_kzalloc()
Yes but lt9611_read_edid() is called multiple times so would make sense
to memset it, will modify this to sizeof.
> > +
> > + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> > +
> > + /* 0xA0 is EDID device address */
> > + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> > + /* 0x00 is EDID offset address */
> > + regmap_write(lt9611->regmap, 0x8505, 0x00);
> > + /* length for read */
> > + regmap_write(lt9611->regmap, 0x8506, 0x20);
> Is this the same 32 as seen in the loops below? #define and use consistently?
Sure will use defines here and other places
> > + if (block > 1)
> > + return -EINVAL;
> > +
> > + if (block == 0) {
> > + /* always read 2 edid blocks once */
> Please mention why that's a good idea. From memory - there aren't many
> other drivers that do this.
Okay will find the reason for this and
>
> > + ret = lt9611_read_edid(lt9611);
> > + if (ret) {
> > + dev_err(lt9611->dev, "edid read failed\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (block % 2 == 0)
> > + memcpy(buf, lt9611->edid_buf, len);
> > + else
> > + memcpy(buf, lt9611->edid_buf + 128, len);
> The above can be written as:
> memcpy(buf, lt9611->edid_buf + (block * 128), len);
correct
> > + /* Attach secondary DSI, if specified */
> > + if (lt9611->dsi1_node) {
> > + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> > + if (IS_ERR(lt9611->dsi1)) {
> > + ret = PTR_ERR(lt9611->dsi1);
> > + goto err_unregister_dsi0;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_dsi0:
> Missing detach? If possible directly use lt9611_bridge_detach().
will update
> > +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> > +{
> > + unsigned int rev;
> > + int ret;
> > +
> > + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> > + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> > + if (ret)
> > + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> > +
> The "failed" message will be followed by printing random kernel memory.
> Initialize rev to some dummy number or omit the dev_info.
Am printing the 'ret' error code here and not the uninitialized rev, so
I guess this one should be fine
> > + ret = lt9611_parse_dt(&client->dev, lt9611);
> > + if (ret) {
> > + dev_err(dev, "failed to parse device tree\n");
> > + return ret;
> > + }
> > +
> > + ret = lt9611_gpio_init(lt9611);
> > + if (ret < 0)
> Missing of_node_put() here and for the next few error paths.
Yes this should be replaced by jump to of_node_put below
> > +static const struct of_device_id lt9611_match_table[] = {
> > + {.compatible = "lontium,lt9611"},
> In the above two - add space after { and before }. Pretty sure
Correct, will fix this.
> ./scripts/checkpatch.pl will complain about those.
> Might want to double-check for other issues reported by said tool.
Somehow that was not the case :( I always run checkpatch.pl with
--strict option. I have 1 warn about 80 char limit for a error message
which I have ignored :)
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-14 7:06 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2020-05-14 7:06 UTC (permalink / raw)
To: Emil Velikov
Cc: devicetree, Jernej Skrabec, Neil Armstrong, David Airlie,
linux-arm-msm, Jonas Karlman, Linux-Kernel@Vger. Kernel. Org,
ML dri-devel, Bjorn Andersson, Andrzej Hajda, Rob Herring,
Laurent Pinchart
Hello Emil,
Thanks for the comments.
On 13-05-20, 20:20, Emil Velikov wrote:
> Hi Vinod,
>
> Few high-level comments:
> - handful of functions always return 0 and the return value is never
> checked - switch to return void
Sure makes sense, will do
> - annotate all (nearly) arrays as static const
Will do
> - consistently use multi_reg_write - in some cases non-const array
> will be fine, overwriting a few entries as needed
Okay that makes sense
> - there is very partial comments about the registers/values - missing docs or?
yeah am not a big fan either, problem is documentation.
Well the spec I have doesn't have register names and few registers are
missing :( I have few name created but naming registers turned nasty
super quick.. Do let me know if you have suggestions, I will give it one
more shot though
> Personally I'm in favour of using symbolic names, instead of
> hex+comment. Considering how partial the comments are, current
> approach is perfectly fine.
>
> On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> > I2S port as an input and HDMI port as output
> >
> > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lt9611.c
>
> Please add a vendor prefix to the filename.
Okay
> > +struct lt9611_mode {
> > + u16 hdisplay;
> > + u16 vdisplay;
> > + u8 fps;
> We all enjoy the odd fps game, but let's use vrefresh here.
Sure will change
> > +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> > + const struct drm_display_mode *mode)
> > +{
> > + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> > +
> > + if (mode->hdisplay == 3840)
> > + regmap_write(lt9611->regmap, 0x830a, 0x03);
> > + else
> > + regmap_write(lt9611->regmap, 0x830a, 0x00);
> > +
> > + regmap_write(lt9611->regmap, 0x824f, 0x80);
> > + regmap_write(lt9611->regmap, 0x8250, 0x10);
> > + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> > + regmap_write(lt9611->regmap, 0x8306, 0x0a);
> Create an (non-const) array, overwriting the [1] entry for 3840 mode?
So array is the recommendation, I dont have much liking for them but I
can see they would be useful here, so will change this and other
instances and we can use regmap_multi_reg_write() while taking care of
static const for non modified arrays
>
> > +
> > + return 0;
> Kill return type.
Yup, here and other places
> > + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> > + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> > + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
> Comment does not match the code.
> We're discarding the LSB, so we cannot realistically be writing
> pclk[7:0]. Similar applies for the other two.
Thanks for pointing, will fix it up
> > + /* v_act */
> > + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> > + if (ret)
> > + goto end;
> > +
> > + v_act = temp << 8;
> > + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> > + if (ret)
> > + goto end;
> > + v_act = v_act + temp;
> > +
> Having a helper for the above "result = read(x) << 8 | read(x+1)"
> would be great.
> This way one doesn't have to repeat the pattern 4-5 times.
will add
> > +static int lt9611_read_edid(struct lt9611 *lt9611)
> > +{
> > + unsigned int temp;
> > + int ret = 0;
> > + int i, j;
> > +
> > + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> How about:
> memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));
>
> Then again, do we need the memset()? We are allocating the memory with
> devm_kzalloc()
Yes but lt9611_read_edid() is called multiple times so would make sense
to memset it, will modify this to sizeof.
> > +
> > + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> > +
> > + /* 0xA0 is EDID device address */
> > + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> > + /* 0x00 is EDID offset address */
> > + regmap_write(lt9611->regmap, 0x8505, 0x00);
> > + /* length for read */
> > + regmap_write(lt9611->regmap, 0x8506, 0x20);
> Is this the same 32 as seen in the loops below? #define and use consistently?
Sure will use defines here and other places
> > + if (block > 1)
> > + return -EINVAL;
> > +
> > + if (block == 0) {
> > + /* always read 2 edid blocks once */
> Please mention why that's a good idea. From memory - there aren't many
> other drivers that do this.
Okay will find the reason for this and
>
> > + ret = lt9611_read_edid(lt9611);
> > + if (ret) {
> > + dev_err(lt9611->dev, "edid read failed\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (block % 2 == 0)
> > + memcpy(buf, lt9611->edid_buf, len);
> > + else
> > + memcpy(buf, lt9611->edid_buf + 128, len);
> The above can be written as:
> memcpy(buf, lt9611->edid_buf + (block * 128), len);
correct
> > + /* Attach secondary DSI, if specified */
> > + if (lt9611->dsi1_node) {
> > + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> > + if (IS_ERR(lt9611->dsi1)) {
> > + ret = PTR_ERR(lt9611->dsi1);
> > + goto err_unregister_dsi0;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_dsi0:
> Missing detach? If possible directly use lt9611_bridge_detach().
will update
> > +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> > +{
> > + unsigned int rev;
> > + int ret;
> > +
> > + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> > + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> > + if (ret)
> > + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> > +
> The "failed" message will be followed by printing random kernel memory.
> Initialize rev to some dummy number or omit the dev_info.
Am printing the 'ret' error code here and not the uninitialized rev, so
I guess this one should be fine
> > + ret = lt9611_parse_dt(&client->dev, lt9611);
> > + if (ret) {
> > + dev_err(dev, "failed to parse device tree\n");
> > + return ret;
> > + }
> > +
> > + ret = lt9611_gpio_init(lt9611);
> > + if (ret < 0)
> Missing of_node_put() here and for the next few error paths.
Yes this should be replaced by jump to of_node_put below
> > +static const struct of_device_id lt9611_match_table[] = {
> > + {.compatible = "lontium,lt9611"},
> In the above two - add space after { and before }. Pretty sure
Correct, will fix this.
> ./scripts/checkpatch.pl will complain about those.
> Might want to double-check for other issues reported by said tool.
Somehow that was not the case :( I always run checkpatch.pl with
--strict option. I have 1 warn about 80 char limit for a error message
which I have ignored :)
--
~Vinod
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-13 10:05 ` Vinod Koul
@ 2020-05-28 1:52 ` Laurent Pinchart
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2020-05-28 1:52 UTC (permalink / raw)
To: Vinod Koul
Cc: Daniel Vetter, David Airlie, Rob Clark, linux-arm-msm,
Bjorn Andersson, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
Jernej Skrabec, Rob Herring, devicetree, linux-kernel, dri-devel
Hi Vinod,
Thank you for the patch.
On Wed, May 13, 2020 at 03:35:33PM +0530, Vinod Koul wrote:
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/gpu/drm/bridge/Kconfig | 13 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/lt9611.c | 1113 +++++++++++++++++++++++++++++++
> 3 files changed, 1127 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/lt9611.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..5cac15ce2027 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -38,6 +38,19 @@ config DRM_DISPLAY_CONNECTOR
> on ARM-based platforms. Saying Y here when this driver is not needed
> will not cause any issue.
>
> +config DRM_LONTIUM_LT9611
> + tristate "Lontium LT9611 DSI/HDMI bridge"
> + select SND_SOC_HDMI_CODEC if SND_SOC
> + depends on OF
> + select DRM_PANEL_BRIDGE
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> + Driver for Lontium LT9611 DSI to HDMI bridge
> + chip driver that converts dual DSI and I2S to
> + HDMI signals
> + Please say Y if you have such hardware.
> +
> config DRM_LVDS_CODEC
> tristate "Transparent LVDS encoders and decoders support"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 6fb062b5b0f0..d2a696e8ca5d 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> +obj-$(CONFIG_DRM_LONTIUM_LT9611) += lt9611.o
> obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lt9611.c b/drivers/gpu/drm/bridge/lt9611.c
> new file mode 100644
> index 000000000000..e6e7ce43980d
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c
> @@ -0,0 +1,1113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2019-2020. Linaro Limited.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_print.h>
> +
> +#define EDID_SEG_SIZE 256
> +
> +#define LT9611_4LANES 0
> +
> +struct lt9611 {
> + struct device *dev;
> + struct drm_bridge bridge;
> + struct drm_connector connector;
> +
> + struct regmap *regmap;
> +
> + struct device_node *dsi0_node;
> + struct device_node *dsi1_node;
> + struct mipi_dsi_device *dsi0;
> + struct mipi_dsi_device *dsi1;
> +
> + bool ac_mode;
> +
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *enable_gpio;
> +
> + bool power_on;
> + bool sleep;
> +
> + struct regulator_bulk_data supplies[2];
> +
> + struct i2c_client *client;
> +
> + enum drm_connector_status status;
> +
> + u8 edid_buf[EDID_SEG_SIZE];
> + u32 vic;
> +};
> +
> +#define LT9611_PAGE_CONTROL 0xff
> +
> +static const struct regmap_range_cfg lt9611_ranges[] = {
> + {
> + .name = "register_range",
> + .range_min = 0,
> + .range_max = 0x85ff,
> + .selector_reg = LT9611_PAGE_CONTROL,
> + .selector_mask = 0xff,
> + .selector_shift = 0,
> + .window_start = 0,
> + .window_len = 0x100,
> + },
> +};
> +
> +static const struct regmap_config lt9611_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0xffff,
> + .ranges = lt9611_ranges,
> + .num_ranges = ARRAY_SIZE(lt9611_ranges),
> +};
> +
> +struct lt9611_mode {
> + u16 hdisplay;
> + u16 vdisplay;
> + u8 fps;
> + u8 lanes;
> + u8 intfs;
> +};
> +
> +static struct lt9611_mode lt9611_modes[] = {
> + { 3840, 2160, 30, 4, 2 }, /* 3840x2160 24bit 30Hz 4Lane 2ports */
> + { 1920, 1080, 60, 4, 1 }, /* 1080P 24bit 60Hz 4lane 1port */
> + { 1920, 1080, 30, 3, 1 }, /* 1080P 24bit 30Hz 3lane 1port */
> + { 1920, 1080, 24, 3, 1 },
> + { 720, 480, 60, 4, 1 },
> + { 720, 576, 50, 2, 1 },
> + { 640, 480, 60, 2, 1 },
> +};
> +
> +static struct lt9611 *bridge_to_lt9611(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct lt9611, bridge);
> +}
> +
> +static struct lt9611 *connector_to_lt9611(struct drm_connector *connector)
> +{
> + return container_of(connector, struct lt9611, connector);
> +}
> +
> +static int lt9611_mipi_input_analog(struct lt9611 *lt9611)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x8106, 0x40 }, /*port A rx current*/
> + { 0x810a, 0xfe }, /*port A ldo voltage set*/
> + { 0x810b, 0xbf }, /*enable port A lprx*/
> + { 0x8111, 0x40 }, /*port B rx current*/
> + { 0x8115, 0xfe }, /*port B ldo voltage set*/
> + { 0x8116, 0xbf }, /*enable port B lprx*/
> +
> + { 0x811c, 0x03 }, /*PortA clk lane no-LP mode*/
> + { 0x8120, 0x03 }, /*PortB clk lane with-LP mode*/
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + return 0;
> +}
> +
> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> + if (mode->hdisplay == 3840)
> + regmap_write(lt9611->regmap, 0x830a, 0x03);
> + else
> + regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> + regmap_write(lt9611->regmap, 0x824f, 0x80);
> + regmap_write(lt9611->regmap, 0x8250, 0x10);
> + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> + regmap_write(lt9611->regmap, 0x8306, 0x0a);
> +
> + return 0;
> +}
> +
> +static void lt9611_mipi_video_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + u32 h_total, h_act, hpw, hfp, hss;
> + u32 v_total, v_act, vpw, vfp, vss;
> +
> + h_total = mode->htotal;
> + v_total = mode->vtotal;
> +
> + h_act = mode->hdisplay;
> + hpw = mode->hsync_end - mode->hsync_start;
> + hfp = mode->hsync_start - mode->hdisplay;
> + hss = (mode->hsync_end - mode->hsync_start) +
> + (mode->htotal - mode->hsync_end);
> +
> + v_act = mode->vdisplay;
> + vpw = mode->vsync_end - mode->vsync_start;
> + vfp = mode->vsync_start - mode->vdisplay;
> + vss = (mode->vsync_end - mode->vsync_start) +
> + (mode->vtotal - mode->vsync_end);
> +
> + regmap_write(lt9611->regmap, 0x830d, (u8)(v_total / 256));
> + regmap_write(lt9611->regmap, 0x830e, (u8)(v_total % 256));
> +
> + regmap_write(lt9611->regmap, 0x830f, (u8)(v_act / 256));
> + regmap_write(lt9611->regmap, 0x8310, (u8)(v_act % 256));
> +
> + regmap_write(lt9611->regmap, 0x8311, (u8)(h_total / 256));
> + regmap_write(lt9611->regmap, 0x8312, (u8)(h_total % 256));
> +
> + regmap_write(lt9611->regmap, 0x8313, (u8)(h_act / 256));
> + regmap_write(lt9611->regmap, 0x8314, (u8)(h_act % 256));
> +
> + regmap_write(lt9611->regmap, 0x8315, (u8)(vpw % 256));
> + regmap_write(lt9611->regmap, 0x8316, (u8)(hpw % 256));
> +
> + regmap_write(lt9611->regmap, 0x8317, (u8)(vfp % 256));
> +
> + regmap_write(lt9611->regmap, 0x8318, (u8)(vss % 256));
> +
> + regmap_write(lt9611->regmap, 0x8319, (u8)(hfp % 256));
> +
> + regmap_write(lt9611->regmap, 0x831a, (u8)(hss / 256));
> + regmap_write(lt9611->regmap, 0x831b, (u8)(hss % 256));
> +}
> +
> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x830b, 0x01 },
> + { 0x830c, 0x10 },
> + { 0x8348, 0x00 },
> + { 0x8349, 0x81 },
> +
> + /* stage 1 */
> + { 0x8321, 0x4a },
> + { 0x8324, 0x71 },
> + { 0x8325, 0x30 },
> + { 0x832a, 0x01 },
> +
> + /* stage 2 */
> + { 0x834a, 0x40 },
> + { 0x831d, 0x10 },
> +
> + /* MK limit */
> + { 0x832d, 0x38 },
> + { 0x8331, 0x08 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + switch (mode->hdisplay) {
> + case 640:
> + regmap_write(lt9611->regmap, 0x8326, 0x14);
> + break;
> + case 1920:
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + case 3840:
> + regmap_write(lt9611->regmap, 0x830b, 0x03);
> + regmap_write(lt9611->regmap, 0x830c, 0xd0);
> + regmap_write(lt9611->regmap, 0x8348, 0x03);
> + regmap_write(lt9611->regmap, 0x8349, 0xe0);
> + regmap_write(lt9611->regmap, 0x8324, 0x72);
> + regmap_write(lt9611->regmap, 0x8325, 0x00);
> + regmap_write(lt9611->regmap, 0x832a, 0x01);
> + regmap_write(lt9611->regmap, 0x834a, 0x10);
> + regmap_write(lt9611->regmap, 0x831d, 0x10);
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + }
> +
> + /* pcr rst */
> + regmap_write(lt9611->regmap, 0x8011, 0x5a);
> + regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> + return 0;
> +}
> +
> +static int lt9611_pll_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + unsigned int pclk = mode->clock;
> + struct reg_sequence reg_cfg[] = {
> + /* txpll init */
> + { 0x8123, 0x40 },
> + { 0x8124, 0x64 },
> + { 0x8125, 0x80 },
> + { 0x8126, 0x55 },
> + { 0x812c, 0x37 },
> + { 0x812f, 0x01 },
> + { 0x8126, 0x55 },
> + { 0x8127, 0x66 },
> + { 0x8128, 0x88 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + if (pclk > 150000)
> + regmap_write(lt9611->regmap, 0x812d, 0x88);
> + else if (pclk > 70000)
> + regmap_write(lt9611->regmap, 0x812d, 0x99);
> + else
> + regmap_write(lt9611->regmap, 0x812d, 0xaa);
> +
> + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
> +
> + regmap_write(lt9611->regmap, 0x82de, 0x20);
> + regmap_write(lt9611->regmap, 0x82de, 0xe0);
> +
> + regmap_write(lt9611->regmap, 0x8016, 0xf1);
> + regmap_write(lt9611->regmap, 0x8016, 0xf3);
> +
> + return 0;
> +}
> +
> +static int lt9611_video_check(struct lt9611 *lt9611)
> +{
> + u32 v_total, v_act, h_act_a, h_act_b, h_total_sysclk;
> + unsigned int temp;
> + int ret;
> +
> + /* top module video check */
> +
> + /* v_act */
> + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> + if (ret)
> + goto end;
> +
> + v_act = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> + if (ret)
> + goto end;
> + v_act = v_act + temp;
> +
> + /* v_total */
> + ret = regmap_read(lt9611->regmap, 0x826c, &temp);
> + if (ret)
> + goto end;
> + v_total = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x826d, &temp);
> + if (ret)
> + goto end;
> + v_total = v_total + temp;
> +
> + /* h_total_sysclk */
> + ret = regmap_read(lt9611->regmap, 0x8286, &temp);
> + if (ret)
> + goto end;
> + h_total_sysclk = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8287, &temp);
> + if (ret)
> + goto end;
> + h_total_sysclk = h_total_sysclk + temp;
> +
> + /* h_act_a */
> + ret = regmap_read(lt9611->regmap, 0x8382, &temp);
> + if (ret)
> + goto end;
> + h_act_a = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8383, &temp);
> + if (ret)
> + goto end;
> + h_act_a = (h_act_a + temp) / 3;
> +
> + /* h_act_b */
> + ret = regmap_read(lt9611->regmap, 0x8386, &temp);
> + if (ret)
> + goto end;
> + h_act_b = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8387, &temp);
> + if (ret)
> + goto end;
> + h_act_b = (h_act_b + temp) / 3;
> +
> + dev_info(lt9611->dev,
> + "video check: h_act_a=%d, h_act_b=%d, v_act=%d, v_total=%d, h_total_sysclk=%d\n",
> + h_act_a, h_act_b, v_act, v_total, h_total_sysclk);
> +
> + return 0;
> +
> +end:
> + dev_err(lt9611->dev, "read video check error\n");
> + return ret;
> +}
> +
> +static void lt9611_hdmi_tx_digital(struct lt9611 *lt9611)
> +{
> + regmap_write(lt9611->regmap, 0x8443, 0x46 - lt9611->vic);
> + regmap_write(lt9611->regmap, 0x8447, lt9611->vic);
> + regmap_write(lt9611->regmap, 0x843d, 0x0a); /* UD1 infoframe */
> +
> + regmap_write(lt9611->regmap, 0x82d6, 0x8c);
> + regmap_write(lt9611->regmap, 0x82d7, 0x04);
> +}
> +
> +static void lt9611_hdmi_tx_phy(struct lt9611 *lt9611)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x8130, 0x6a },
> + { 0x8131, 0x44 }, /* HDMI DC mode */
> + { 0x8132, 0x4a },
> + { 0x8133, 0x0b },
> + { 0x8134, 0x00 },
> + { 0x8135, 0x00 },
> + { 0x8136, 0x00 },
> + { 0x8137, 0x44 },
> + { 0x813f, 0x0f },
> + { 0x8140, 0xa0 },
> + { 0x8141, 0xa0 },
> + { 0x8142, 0xa0 },
> + { 0x8143, 0xa0 },
> + { 0x8144, 0x0a },
> + };
> +
> + /* HDMI AC mode */
> + if (lt9611->ac_mode)
> + reg_cfg[2].def = 0x73;
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +}
> +
> +static irqreturn_t lt9611_irq_thread_handler(int irq, void *dev_id)
> +{
> + struct lt9611 *lt9611 = dev_id;
> + unsigned int irq_flag0 = 0;
> + unsigned int irq_flag3 = 0;
> +
> + regmap_read(lt9611->regmap, 0x820f, &irq_flag3);
> + regmap_read(lt9611->regmap, 0x820c, &irq_flag0);
> +
> + pr_debug("%s() irq_flag0: %#x irq_flag3: %#x\n",
> + __func__, irq_flag0, irq_flag3);
> +
> + /* hpd changed low */
> + if (irq_flag3 & 0x80) {
> + dev_info(lt9611->dev, "hdmi cable disconnected\n");
> +
> + regmap_write(lt9611->regmap, 0x8207, 0xbf);
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> + }
> + /* hpd changed high */
> + if (irq_flag3 & 0x40) {
> + dev_info(lt9611->dev, "hdmi cable connected\n");
> +
> + regmap_write(lt9611->regmap, 0x8207, 0x7f);
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> + }
> +
> + if (irq_flag3 & 0xc0)
> + drm_kms_helper_hotplug_event(lt9611->bridge.dev);
> +
> + /* video input changed */
> + if (irq_flag0 & 0x01) {
> + dev_info(lt9611->dev, "video input changed\n");
> + regmap_write(lt9611->regmap, 0x829e, 0xff);
> + regmap_write(lt9611->regmap, 0x829e, 0xf7);
> + regmap_write(lt9611->regmap, 0x8204, 0xff);
> + regmap_write(lt9611->regmap, 0x8204, 0xfe);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void lt9611_enable_hpd_interrupts(struct lt9611 *lt9611)
> +{
> + unsigned int val;
> +
> + dev_dbg(lt9611->dev, "enabling hpd interrupts\n");
> +
> + regmap_read(lt9611->regmap, 0x8203, &val);
> +
> + val &= ~0xc0;
> + regmap_write(lt9611->regmap, 0x8203, val);
> + regmap_write(lt9611->regmap, 0x8207, 0xff); //clear
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> +}
> +
> +static void lt9611_sleep_setup(struct lt9611 *lt9611)
> +{
> + struct reg_sequence sleep_setup[] = {
> + { 0x8024, 0x76 },
> + { 0x8023, 0x01 },
> + { 0x8157, 0x03 }, //set addr pin as output
> + { 0x8149, 0x0b },
> + { 0x8151, 0x30 }, //disable IRQ
> + { 0x8102, 0x48 }, //MIPI Rx power down
> + { 0x8123, 0x80 },
> + { 0x8130, 0x00 },
> + { 0x8100, 0x01 }, //bandgap power down
> + { 0x8101, 0x00 }, //system clk power down
> + };
> +
> + dev_dbg(lt9611->dev, "sleep\n");
> +
> + regmap_multi_reg_write(lt9611->regmap,
> + sleep_setup, ARRAY_SIZE(sleep_setup));
> + lt9611->sleep = true;
> +}
> +
> +static int lt9611_power_on(struct lt9611 *lt9611)
> +{
> + int ret;
> + const struct reg_sequence seq[] = {
> + /* LT9611_System_Init */
> + { 0x8101, 0x18 }, /* sel xtal clock */
> +
> + /* timer for frequency meter */
> + { 0x821b, 0x69 }, /*timer 2*/
> + { 0x821c, 0x78 },
> + { 0x82cb, 0x69 }, /*timer 1 */
> + { 0x82cc, 0x78 },
> +
> + /* irq init */
> + { 0x8251, 0x01 },
> + { 0x8258, 0x0a }, /* hpd irq */
> + { 0x8259, 0x80 }, /* hpd debounce width */
> + { 0x829e, 0xf7 }, /* video check irq */
> +
> + /* power consumption for work */
> + { 0x8004, 0xf0 },
> + { 0x8006, 0xf0 },
> + { 0x800a, 0x80 },
> + { 0x800b, 0x40 },
> + { 0x800d, 0xef },
> + { 0x8011, 0xfa },
> + };
> +
> + if (lt9611->power_on)
> + return 0;
> +
> + dev_dbg(lt9611->dev, "power on\n");
> +
> + ret = regmap_multi_reg_write(lt9611->regmap, seq, ARRAY_SIZE(seq));
> + if (!ret)
> + lt9611->power_on = true;
> +
> + return ret;
> +}
> +
> +static int lt9611_power_off(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + dev_dbg(lt9611->dev, "power off\n");
> +
> + ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> + if (!ret)
> + lt9611->power_on = false;
> +
> + return ret;
> +}
> +
> +static void lt9611_reset(struct lt9611 *lt9611)
> +{
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> + msleep(20);
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
> + msleep(20);
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> + msleep(100);
> +}
> +
> +static void lt9611_assert_5v(struct lt9611 *lt9611)
> +{
> + if (!lt9611->enable_gpio)
> + return;
> +
> + gpiod_set_value_cansleep(lt9611->enable_gpio, 1);
> + msleep(20);
> +}
> +
> +static int lt9611_regulator_init(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + lt9611->supplies[0].supply = "vdd";
> + lt9611->supplies[1].supply = "vcc";
> + ret = devm_regulator_bulk_get(lt9611->dev, 2, lt9611->supplies);
> + if (ret < 0)
> + return ret;
> +
> + return regulator_set_load(lt9611->supplies[0].consumer, 300000);
> +}
> +
> +static int lt9611_regulator_enable(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + ret = regulator_enable(lt9611->supplies[0].consumer);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(1000, 10000);
> +
> + ret = regulator_enable(lt9611->supplies[1].consumer);
> + if (ret < 0) {
> + regulator_disable(lt9611->supplies[0].consumer);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct lt9611_mode *lt9611_find_mode(const struct drm_display_mode *mode)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lt9611_modes); i++) {
> + if (lt9611_modes[i].hdisplay == mode->hdisplay &&
> + lt9611_modes[i].vdisplay == mode->vdisplay &&
> + lt9611_modes[i].fps == drm_mode_vrefresh(mode)) {
> + return <9611_modes[i];
> + }
> + }
> +
> + return NULL;
> +}
> +
> +/* connector funcs */
> +static enum drm_connector_status
> +lt9611_connector_detect(struct drm_connector *connector, bool force)
> +{
> + struct lt9611 *lt9611 = connector_to_lt9611(connector);
> + unsigned int reg_val = 0;
> + int connected = 0;
> +
> + regmap_read(lt9611->regmap, 0x825e, ®_val);
> + connected = (reg_val & BIT(2));
> + dev_dbg(lt9611->dev, "connected = %x\n", connected);
> +
> + lt9611->status = connected ? connector_status_connected :
> + connector_status_disconnected;
> +
> + return lt9611->status;
> +}
> +
> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> + unsigned int temp;
> + int ret = 0;
> + int i, j;
> +
> + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> +
> + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> + /* 0xA0 is EDID device address */
> + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> + /* 0x00 is EDID offset address */
> + regmap_write(lt9611->regmap, 0x8505, 0x00);
> + /* length for read */
> + regmap_write(lt9611->regmap, 0x8506, 0x20);
> + regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> + for (i = 0 ; i < 8 ; i++) {
> + /* offset address */
> + regmap_write(lt9611->regmap, 0x8505, i * 32);
> + regmap_write(lt9611->regmap, 0x8507, 0x36);
> + regmap_write(lt9611->regmap, 0x8507, 0x31);
> + regmap_write(lt9611->regmap, 0x8507, 0x37);
> + usleep_range(5000, 10000);
> +
> + regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> + if (temp & 0x02) { /*KEY_DDC_ACCS_DONE=1*/
> + for (j = 0; j < 32; j++) {
> + regmap_read(lt9611->regmap, 0x8583, &temp);
> + lt9611->edid_buf[i * 32 + j] = temp;
> + }
> + } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> + dev_err(lt9611->dev, "read edid failed: no ack\n");
> + ret = -EIO;
> + goto end;
> + } else {
> + dev_err(lt9611->dev,
> + "read edid failed: access not done\n");
> + ret = -EIO;
> + goto end;
> + }
> + }
> +
> + dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> + lt9611->edid_buf[255]);
> +
> +end:
> + regmap_write(lt9611->regmap, 0x8507, 0x1f);
> + return ret;
> +}
> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> + struct lt9611 *lt9611 = data;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> + block, (int)len);
> +
> + if (len > 128)
> + return -EINVAL;
> +
> + /* support up to 1 extension block */
> + if (block > 1)
> + return -EINVAL;
> +
> + if (block == 0) {
> + /* always read 2 edid blocks once */
> + ret = lt9611_read_edid(lt9611);
> + if (ret) {
> + dev_err(lt9611->dev, "edid read failed\n");
> + return ret;
> + }
> + }
> +
> + if (block % 2 == 0)
> + memcpy(buf, lt9611->edid_buf, len);
> + else
> + memcpy(buf, lt9611->edid_buf + 128, len);
> +
> + return 0;
> +}
> +
> +static int lt9611_connector_get_modes(struct drm_connector *connector)
> +{
> + struct lt9611 *lt9611 = connector_to_lt9611(connector);
> + unsigned int count;
> + struct edid *edid;
> +
> + dev_dbg(lt9611->dev, "get modes\n");
> +
> + lt9611_power_on(lt9611);
> + edid = drm_do_get_edid(connector, lt9611_get_edid_block, lt9611);
> + drm_connector_update_edid_property(connector, edid);
> + count = drm_add_edid_modes(connector, edid);
> + kfree(edid);
> +
> + return count;
> +}
> +
> +static enum drm_mode_status
> +lt9611_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> +
> + return lt9611_mode ? MODE_OK : MODE_BAD;
> +}
> +
> +/* bridge funcs */
> +static void lt9611_bridge_enable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge enable\n");
> +
> + if (lt9611_power_on(lt9611)) {
> + dev_err(lt9611->dev, "power on failed\n");
> + return;
> + }
> +
> + dev_dbg(lt9611->dev, "video on\n");
> +
> + lt9611_mipi_input_analog(lt9611);
> + lt9611_hdmi_tx_digital(lt9611);
> + lt9611_hdmi_tx_phy(lt9611);
> +
> + msleep(500);
> +
> + lt9611_video_check(lt9611);
> +
> + /* Enable HDMI output */
> + regmap_write(lt9611->regmap, 0x8130, 0xea);
> +}
> +
> +static void lt9611_bridge_disable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge disable\n");
> +
> + /* Disable HDMI output */
> + ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> + if (ret) {
> + dev_err(lt9611->dev, "video on failed\n");
> + return;
> + }
> +
> + if (lt9611_power_off(lt9611)) {
> + dev_err(lt9611->dev, "power on failed\n");
> + return;
> + }
> +}
> +
> +static struct
> +drm_connector_helper_funcs lt9611_bridge_connector_helper_funcs = {
> + .get_modes = lt9611_connector_get_modes,
> + .mode_valid = lt9611_connector_mode_valid,
> +};
> +
> +static const struct drm_connector_funcs lt9611_bridge_connector_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .detect = lt9611_connector_detect,
> + .destroy = drm_connector_cleanup,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
> + struct device_node *dsi_node)
> +{
> + const struct mipi_dsi_device_info info = { "lt9611", 0, NULL };
> + struct mipi_dsi_device *dsi;
> + struct mipi_dsi_host *host;
> + int ret;
> +
> + host = of_find_mipi_dsi_host_by_node(dsi_node);
> + if (!host) {
> + dev_err(lt9611->dev, "failed to find dsi host\n");
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + dsi = mipi_dsi_device_register_full(host, &info);
> + if (IS_ERR(dsi)) {
> + dev_err(lt9611->dev, "failed to create dsi device\n");
> + return dsi;
> + }
> +
> + dsi->lanes = 4;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_MODE_VIDEO_HSE;
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0) {
> + dev_err(lt9611->dev, "failed to attach dsi to host\n");
> + mipi_dsi_device_unregister(dsi);
> + return ERR_PTR(ret);
> + }
> +
> + return dsi;
> +}
> +
> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge attach\n");
Connector creation in bridge drivers is deprecated. Please at least add
support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
should not be implemented at all. This will require the display
controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
controller(s) do you use this driver with ?
> +
> + ret = drm_connector_init(bridge->dev, <9611->connector,
> + <9611_bridge_connector_funcs,
> + DRM_MODE_CONNECTOR_HDMIA);
> + if (ret) {
> + DRM_ERROR("Failed to initialize connector with drm\n");
> + return ret;
> + }
> +
> + drm_connector_helper_add(<9611->connector,
> + <9611_bridge_connector_helper_funcs);
> + drm_connector_attach_encoder(<9611->connector, bridge->encoder);
> +
> + if (!bridge->encoder) {
> + DRM_ERROR("Parent encoder object not found");
> + return -ENODEV;
> + }
> +
> + /* Attach primary DSI */
> + lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node);
> + if (IS_ERR(lt9611->dsi0))
> + return PTR_ERR(lt9611->dsi0);
> +
> + /* Attach secondary DSI, if specified */
> + if (lt9611->dsi1_node) {
> + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> + if (IS_ERR(lt9611->dsi1)) {
> + ret = PTR_ERR(lt9611->dsi1);
> + goto err_unregister_dsi0;
> + }
> + }
> +
> + return 0;
> +
> +err_unregister_dsi0:
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +
> + return ret;
> +}
> +
> +static void lt9611_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + if (lt9611->dsi1) {
> + mipi_dsi_detach(lt9611->dsi1);
> + mipi_dsi_device_unregister(lt9611->dsi1);
> + }
> +
> + mipi_dsi_detach(lt9611->dsi0);
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +}
> +
> +static enum drm_mode_status
> +lt9611_bridge_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode)
> +{
> + struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + if (lt9611_mode->intfs > 1 && !lt9611->dsi1)
> + return MODE_PANEL;
> + else
> + return MODE_OK;
> +}
> +
> +static void lt9611_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge pre_enable\n");
> +
> + if (!lt9611->sleep)
> + return;
> +
> + lt9611_reset(lt9611);
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +
> + lt9611->sleep = false;
> +}
> +
> +static void lt9611_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge post_disable\n");
> +
> + lt9611_sleep_setup(lt9611);
> +}
> +
> +static void lt9611_bridge_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adj_mode)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + struct hdmi_avi_infoframe avi_frame;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
> + adj_mode->hdisplay, adj_mode->vdisplay,
> + adj_mode->vrefresh, adj_mode->clock);
> +
> + lt9611_bridge_pre_enable(bridge);
> +
> + lt9611_mipi_input_digital(lt9611, mode);
> + lt9611_pll_setup(lt9611, mode);
> + lt9611_mipi_video_setup(lt9611, mode);
> + lt9611_pcr_setup(lt9611, mode);
> +
> + ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
> + <9611->connector,
> + mode);
> + if (!ret)
> + lt9611->vic = avi_frame.video_code;
> +}
> +
> +static const struct drm_bridge_funcs lt9611_bridge_funcs = {
> + .attach = lt9611_bridge_attach,
> + .detach = lt9611_bridge_detach,
> + .mode_valid = lt9611_bridge_mode_valid,
> + .enable = lt9611_bridge_enable,
> + .disable = lt9611_bridge_disable,
> + .post_disable = lt9611_bridge_post_disable,
> + .mode_set = lt9611_bridge_mode_set,
> +};
> +
> +static int lt9611_parse_dt(struct device *dev,
> + struct lt9611 *lt9611)
> +{
> + lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 1, -1);
> + if (!lt9611->dsi0_node) {
> + DRM_DEV_ERROR(dev, "failed to get remote node for primary dsi\n");
> + return -ENODEV;
> + }
> +
> + lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 2, -1);
> +
> + lt9611->ac_mode = of_property_read_bool(dev->of_node, "lt,ac-mode");
> + dev_dbg(lt9611->dev, "ac_mode=%d\n", lt9611->ac_mode);
> +
> + return 0;
> +}
> +
> +static int lt9611_gpio_init(struct lt9611 *lt9611)
> +{
> + struct device *dev = lt9611->dev;
> +
> + lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(lt9611->reset_gpio)) {
> + dev_err(dev, "failed to acquire reset gpio\n");
> + return PTR_ERR(lt9611->reset_gpio);
> + }
> +
> + lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(lt9611->enable_gpio)) {
> + dev_err(dev, "failed to acquire enable gpio\n");
> + return PTR_ERR(lt9611->enable_gpio);
> + }
> +
> + return 0;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> + unsigned int rev;
> + int ret;
> +
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> + if (ret)
> + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
> + dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> + return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct lt9611 *lt9611;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "device doesn't support I2C\n");
> + return -ENODEV;
> + }
> +
> + lt9611 = devm_kzalloc(dev, sizeof(*lt9611), GFP_KERNEL);
> + if (!lt9611)
> + return -ENOMEM;
> +
> + lt9611->dev = &client->dev;
> + lt9611->client = client;
> + lt9611->sleep = false;
> +
> + lt9611->regmap = devm_regmap_init_i2c(client, <9611_regmap_config);
> + if (IS_ERR(lt9611->regmap)) {
> + DRM_ERROR("regmap i2c init failed\n");
> + return PTR_ERR(lt9611->regmap);
> + }
> +
> + ret = lt9611_parse_dt(&client->dev, lt9611);
> + if (ret) {
> + dev_err(dev, "failed to parse device tree\n");
> + return ret;
> + }
> +
> + ret = lt9611_gpio_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + ret = lt9611_regulator_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + lt9611_assert_5v(lt9611);
> +
> + ret = lt9611_regulator_enable(lt9611);
> + if (ret)
> + return ret;
> +
> + lt9611_reset(lt9611);
> +
> + ret = lt9611_read_device_rev(lt9611);
> + if (ret) {
> + dev_err(dev, "failed to read chip rev\n");
> + goto err_disable_regulators;
> + }
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + lt9611_irq_thread_handler,
> + IRQF_ONESHOT, "lt9611", lt9611);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + goto err_disable_regulators;
> + }
> +
> + i2c_set_clientdata(client, lt9611);
> +
> + lt9611->bridge.funcs = <9611_bridge_funcs;
> + lt9611->bridge.of_node = client->dev.of_node;
> +
> + drm_bridge_add(<9611->bridge);
> +
> + lt9611_enable_hpd_interrupts(lt9611);
> +
> + return 0;
> +
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> + struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> + disable_irq(client->irq);
> + drm_bridge_remove(<9611->bridge);
> +
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> + { "lontium,lt9611", 0},
> + {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> + {.compatible = "lontium,lt9611"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lt9611_match_table);
> +
> +static struct i2c_driver lt9611_driver = {
> + .driver = {
> + .name = "lt9611",
> + .of_match_table = lt9611_match_table,
> + },
> + .probe = lt9611_probe,
> + .remove = lt9611_remove,
> + .id_table = lt9611_id,
> +};
> +module_i2c_driver(lt9611_driver);
> +
> +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-05-28 1:52 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2020-05-28 1:52 UTC (permalink / raw)
To: Vinod Koul
Cc: devicetree, Jernej Skrabec, Neil Armstrong, David Airlie,
linux-arm-msm, Jonas Karlman, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda, Rob Herring
Hi Vinod,
Thank you for the patch.
On Wed, May 13, 2020 at 03:35:33PM +0530, Vinod Koul wrote:
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/gpu/drm/bridge/Kconfig | 13 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/lt9611.c | 1113 +++++++++++++++++++++++++++++++
> 3 files changed, 1127 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/lt9611.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..5cac15ce2027 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -38,6 +38,19 @@ config DRM_DISPLAY_CONNECTOR
> on ARM-based platforms. Saying Y here when this driver is not needed
> will not cause any issue.
>
> +config DRM_LONTIUM_LT9611
> + tristate "Lontium LT9611 DSI/HDMI bridge"
> + select SND_SOC_HDMI_CODEC if SND_SOC
> + depends on OF
> + select DRM_PANEL_BRIDGE
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> + Driver for Lontium LT9611 DSI to HDMI bridge
> + chip driver that converts dual DSI and I2S to
> + HDMI signals
> + Please say Y if you have such hardware.
> +
> config DRM_LVDS_CODEC
> tristate "Transparent LVDS encoders and decoders support"
> depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 6fb062b5b0f0..d2a696e8ca5d 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> +obj-$(CONFIG_DRM_LONTIUM_LT9611) += lt9611.o
> obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
> obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lt9611.c b/drivers/gpu/drm/bridge/lt9611.c
> new file mode 100644
> index 000000000000..e6e7ce43980d
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c
> @@ -0,0 +1,1113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2019-2020. Linaro Limited.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_print.h>
> +
> +#define EDID_SEG_SIZE 256
> +
> +#define LT9611_4LANES 0
> +
> +struct lt9611 {
> + struct device *dev;
> + struct drm_bridge bridge;
> + struct drm_connector connector;
> +
> + struct regmap *regmap;
> +
> + struct device_node *dsi0_node;
> + struct device_node *dsi1_node;
> + struct mipi_dsi_device *dsi0;
> + struct mipi_dsi_device *dsi1;
> +
> + bool ac_mode;
> +
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *enable_gpio;
> +
> + bool power_on;
> + bool sleep;
> +
> + struct regulator_bulk_data supplies[2];
> +
> + struct i2c_client *client;
> +
> + enum drm_connector_status status;
> +
> + u8 edid_buf[EDID_SEG_SIZE];
> + u32 vic;
> +};
> +
> +#define LT9611_PAGE_CONTROL 0xff
> +
> +static const struct regmap_range_cfg lt9611_ranges[] = {
> + {
> + .name = "register_range",
> + .range_min = 0,
> + .range_max = 0x85ff,
> + .selector_reg = LT9611_PAGE_CONTROL,
> + .selector_mask = 0xff,
> + .selector_shift = 0,
> + .window_start = 0,
> + .window_len = 0x100,
> + },
> +};
> +
> +static const struct regmap_config lt9611_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0xffff,
> + .ranges = lt9611_ranges,
> + .num_ranges = ARRAY_SIZE(lt9611_ranges),
> +};
> +
> +struct lt9611_mode {
> + u16 hdisplay;
> + u16 vdisplay;
> + u8 fps;
> + u8 lanes;
> + u8 intfs;
> +};
> +
> +static struct lt9611_mode lt9611_modes[] = {
> + { 3840, 2160, 30, 4, 2 }, /* 3840x2160 24bit 30Hz 4Lane 2ports */
> + { 1920, 1080, 60, 4, 1 }, /* 1080P 24bit 60Hz 4lane 1port */
> + { 1920, 1080, 30, 3, 1 }, /* 1080P 24bit 30Hz 3lane 1port */
> + { 1920, 1080, 24, 3, 1 },
> + { 720, 480, 60, 4, 1 },
> + { 720, 576, 50, 2, 1 },
> + { 640, 480, 60, 2, 1 },
> +};
> +
> +static struct lt9611 *bridge_to_lt9611(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct lt9611, bridge);
> +}
> +
> +static struct lt9611 *connector_to_lt9611(struct drm_connector *connector)
> +{
> + return container_of(connector, struct lt9611, connector);
> +}
> +
> +static int lt9611_mipi_input_analog(struct lt9611 *lt9611)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x8106, 0x40 }, /*port A rx current*/
> + { 0x810a, 0xfe }, /*port A ldo voltage set*/
> + { 0x810b, 0xbf }, /*enable port A lprx*/
> + { 0x8111, 0x40 }, /*port B rx current*/
> + { 0x8115, 0xfe }, /*port B ldo voltage set*/
> + { 0x8116, 0xbf }, /*enable port B lprx*/
> +
> + { 0x811c, 0x03 }, /*PortA clk lane no-LP mode*/
> + { 0x8120, 0x03 }, /*PortB clk lane with-LP mode*/
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + return 0;
> +}
> +
> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> + if (mode->hdisplay == 3840)
> + regmap_write(lt9611->regmap, 0x830a, 0x03);
> + else
> + regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> + regmap_write(lt9611->regmap, 0x824f, 0x80);
> + regmap_write(lt9611->regmap, 0x8250, 0x10);
> + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> + regmap_write(lt9611->regmap, 0x8306, 0x0a);
> +
> + return 0;
> +}
> +
> +static void lt9611_mipi_video_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + u32 h_total, h_act, hpw, hfp, hss;
> + u32 v_total, v_act, vpw, vfp, vss;
> +
> + h_total = mode->htotal;
> + v_total = mode->vtotal;
> +
> + h_act = mode->hdisplay;
> + hpw = mode->hsync_end - mode->hsync_start;
> + hfp = mode->hsync_start - mode->hdisplay;
> + hss = (mode->hsync_end - mode->hsync_start) +
> + (mode->htotal - mode->hsync_end);
> +
> + v_act = mode->vdisplay;
> + vpw = mode->vsync_end - mode->vsync_start;
> + vfp = mode->vsync_start - mode->vdisplay;
> + vss = (mode->vsync_end - mode->vsync_start) +
> + (mode->vtotal - mode->vsync_end);
> +
> + regmap_write(lt9611->regmap, 0x830d, (u8)(v_total / 256));
> + regmap_write(lt9611->regmap, 0x830e, (u8)(v_total % 256));
> +
> + regmap_write(lt9611->regmap, 0x830f, (u8)(v_act / 256));
> + regmap_write(lt9611->regmap, 0x8310, (u8)(v_act % 256));
> +
> + regmap_write(lt9611->regmap, 0x8311, (u8)(h_total / 256));
> + regmap_write(lt9611->regmap, 0x8312, (u8)(h_total % 256));
> +
> + regmap_write(lt9611->regmap, 0x8313, (u8)(h_act / 256));
> + regmap_write(lt9611->regmap, 0x8314, (u8)(h_act % 256));
> +
> + regmap_write(lt9611->regmap, 0x8315, (u8)(vpw % 256));
> + regmap_write(lt9611->regmap, 0x8316, (u8)(hpw % 256));
> +
> + regmap_write(lt9611->regmap, 0x8317, (u8)(vfp % 256));
> +
> + regmap_write(lt9611->regmap, 0x8318, (u8)(vss % 256));
> +
> + regmap_write(lt9611->regmap, 0x8319, (u8)(hfp % 256));
> +
> + regmap_write(lt9611->regmap, 0x831a, (u8)(hss / 256));
> + regmap_write(lt9611->regmap, 0x831b, (u8)(hss % 256));
> +}
> +
> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x830b, 0x01 },
> + { 0x830c, 0x10 },
> + { 0x8348, 0x00 },
> + { 0x8349, 0x81 },
> +
> + /* stage 1 */
> + { 0x8321, 0x4a },
> + { 0x8324, 0x71 },
> + { 0x8325, 0x30 },
> + { 0x832a, 0x01 },
> +
> + /* stage 2 */
> + { 0x834a, 0x40 },
> + { 0x831d, 0x10 },
> +
> + /* MK limit */
> + { 0x832d, 0x38 },
> + { 0x8331, 0x08 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + switch (mode->hdisplay) {
> + case 640:
> + regmap_write(lt9611->regmap, 0x8326, 0x14);
> + break;
> + case 1920:
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + case 3840:
> + regmap_write(lt9611->regmap, 0x830b, 0x03);
> + regmap_write(lt9611->regmap, 0x830c, 0xd0);
> + regmap_write(lt9611->regmap, 0x8348, 0x03);
> + regmap_write(lt9611->regmap, 0x8349, 0xe0);
> + regmap_write(lt9611->regmap, 0x8324, 0x72);
> + regmap_write(lt9611->regmap, 0x8325, 0x00);
> + regmap_write(lt9611->regmap, 0x832a, 0x01);
> + regmap_write(lt9611->regmap, 0x834a, 0x10);
> + regmap_write(lt9611->regmap, 0x831d, 0x10);
> + regmap_write(lt9611->regmap, 0x8326, 0x37);
> + break;
> + }
> +
> + /* pcr rst */
> + regmap_write(lt9611->regmap, 0x8011, 0x5a);
> + regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> + return 0;
> +}
> +
> +static int lt9611_pll_setup(struct lt9611 *lt9611,
> + const struct drm_display_mode *mode)
> +{
> + unsigned int pclk = mode->clock;
> + struct reg_sequence reg_cfg[] = {
> + /* txpll init */
> + { 0x8123, 0x40 },
> + { 0x8124, 0x64 },
> + { 0x8125, 0x80 },
> + { 0x8126, 0x55 },
> + { 0x812c, 0x37 },
> + { 0x812f, 0x01 },
> + { 0x8126, 0x55 },
> + { 0x8127, 0x66 },
> + { 0x8128, 0x88 },
> + };
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> + if (pclk > 150000)
> + regmap_write(lt9611->regmap, 0x812d, 0x88);
> + else if (pclk > 70000)
> + regmap_write(lt9611->regmap, 0x812d, 0x99);
> + else
> + regmap_write(lt9611->regmap, 0x812d, 0xaa);
> +
> + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
> +
> + regmap_write(lt9611->regmap, 0x82de, 0x20);
> + regmap_write(lt9611->regmap, 0x82de, 0xe0);
> +
> + regmap_write(lt9611->regmap, 0x8016, 0xf1);
> + regmap_write(lt9611->regmap, 0x8016, 0xf3);
> +
> + return 0;
> +}
> +
> +static int lt9611_video_check(struct lt9611 *lt9611)
> +{
> + u32 v_total, v_act, h_act_a, h_act_b, h_total_sysclk;
> + unsigned int temp;
> + int ret;
> +
> + /* top module video check */
> +
> + /* v_act */
> + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> + if (ret)
> + goto end;
> +
> + v_act = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> + if (ret)
> + goto end;
> + v_act = v_act + temp;
> +
> + /* v_total */
> + ret = regmap_read(lt9611->regmap, 0x826c, &temp);
> + if (ret)
> + goto end;
> + v_total = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x826d, &temp);
> + if (ret)
> + goto end;
> + v_total = v_total + temp;
> +
> + /* h_total_sysclk */
> + ret = regmap_read(lt9611->regmap, 0x8286, &temp);
> + if (ret)
> + goto end;
> + h_total_sysclk = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8287, &temp);
> + if (ret)
> + goto end;
> + h_total_sysclk = h_total_sysclk + temp;
> +
> + /* h_act_a */
> + ret = regmap_read(lt9611->regmap, 0x8382, &temp);
> + if (ret)
> + goto end;
> + h_act_a = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8383, &temp);
> + if (ret)
> + goto end;
> + h_act_a = (h_act_a + temp) / 3;
> +
> + /* h_act_b */
> + ret = regmap_read(lt9611->regmap, 0x8386, &temp);
> + if (ret)
> + goto end;
> + h_act_b = temp << 8;
> + ret = regmap_read(lt9611->regmap, 0x8387, &temp);
> + if (ret)
> + goto end;
> + h_act_b = (h_act_b + temp) / 3;
> +
> + dev_info(lt9611->dev,
> + "video check: h_act_a=%d, h_act_b=%d, v_act=%d, v_total=%d, h_total_sysclk=%d\n",
> + h_act_a, h_act_b, v_act, v_total, h_total_sysclk);
> +
> + return 0;
> +
> +end:
> + dev_err(lt9611->dev, "read video check error\n");
> + return ret;
> +}
> +
> +static void lt9611_hdmi_tx_digital(struct lt9611 *lt9611)
> +{
> + regmap_write(lt9611->regmap, 0x8443, 0x46 - lt9611->vic);
> + regmap_write(lt9611->regmap, 0x8447, lt9611->vic);
> + regmap_write(lt9611->regmap, 0x843d, 0x0a); /* UD1 infoframe */
> +
> + regmap_write(lt9611->regmap, 0x82d6, 0x8c);
> + regmap_write(lt9611->regmap, 0x82d7, 0x04);
> +}
> +
> +static void lt9611_hdmi_tx_phy(struct lt9611 *lt9611)
> +{
> + struct reg_sequence reg_cfg[] = {
> + { 0x8130, 0x6a },
> + { 0x8131, 0x44 }, /* HDMI DC mode */
> + { 0x8132, 0x4a },
> + { 0x8133, 0x0b },
> + { 0x8134, 0x00 },
> + { 0x8135, 0x00 },
> + { 0x8136, 0x00 },
> + { 0x8137, 0x44 },
> + { 0x813f, 0x0f },
> + { 0x8140, 0xa0 },
> + { 0x8141, 0xa0 },
> + { 0x8142, 0xa0 },
> + { 0x8143, 0xa0 },
> + { 0x8144, 0x0a },
> + };
> +
> + /* HDMI AC mode */
> + if (lt9611->ac_mode)
> + reg_cfg[2].def = 0x73;
> +
> + regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +}
> +
> +static irqreturn_t lt9611_irq_thread_handler(int irq, void *dev_id)
> +{
> + struct lt9611 *lt9611 = dev_id;
> + unsigned int irq_flag0 = 0;
> + unsigned int irq_flag3 = 0;
> +
> + regmap_read(lt9611->regmap, 0x820f, &irq_flag3);
> + regmap_read(lt9611->regmap, 0x820c, &irq_flag0);
> +
> + pr_debug("%s() irq_flag0: %#x irq_flag3: %#x\n",
> + __func__, irq_flag0, irq_flag3);
> +
> + /* hpd changed low */
> + if (irq_flag3 & 0x80) {
> + dev_info(lt9611->dev, "hdmi cable disconnected\n");
> +
> + regmap_write(lt9611->regmap, 0x8207, 0xbf);
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> + }
> + /* hpd changed high */
> + if (irq_flag3 & 0x40) {
> + dev_info(lt9611->dev, "hdmi cable connected\n");
> +
> + regmap_write(lt9611->regmap, 0x8207, 0x7f);
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> + }
> +
> + if (irq_flag3 & 0xc0)
> + drm_kms_helper_hotplug_event(lt9611->bridge.dev);
> +
> + /* video input changed */
> + if (irq_flag0 & 0x01) {
> + dev_info(lt9611->dev, "video input changed\n");
> + regmap_write(lt9611->regmap, 0x829e, 0xff);
> + regmap_write(lt9611->regmap, 0x829e, 0xf7);
> + regmap_write(lt9611->regmap, 0x8204, 0xff);
> + regmap_write(lt9611->regmap, 0x8204, 0xfe);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void lt9611_enable_hpd_interrupts(struct lt9611 *lt9611)
> +{
> + unsigned int val;
> +
> + dev_dbg(lt9611->dev, "enabling hpd interrupts\n");
> +
> + regmap_read(lt9611->regmap, 0x8203, &val);
> +
> + val &= ~0xc0;
> + regmap_write(lt9611->regmap, 0x8203, val);
> + regmap_write(lt9611->regmap, 0x8207, 0xff); //clear
> + regmap_write(lt9611->regmap, 0x8207, 0x3f);
> +}
> +
> +static void lt9611_sleep_setup(struct lt9611 *lt9611)
> +{
> + struct reg_sequence sleep_setup[] = {
> + { 0x8024, 0x76 },
> + { 0x8023, 0x01 },
> + { 0x8157, 0x03 }, //set addr pin as output
> + { 0x8149, 0x0b },
> + { 0x8151, 0x30 }, //disable IRQ
> + { 0x8102, 0x48 }, //MIPI Rx power down
> + { 0x8123, 0x80 },
> + { 0x8130, 0x00 },
> + { 0x8100, 0x01 }, //bandgap power down
> + { 0x8101, 0x00 }, //system clk power down
> + };
> +
> + dev_dbg(lt9611->dev, "sleep\n");
> +
> + regmap_multi_reg_write(lt9611->regmap,
> + sleep_setup, ARRAY_SIZE(sleep_setup));
> + lt9611->sleep = true;
> +}
> +
> +static int lt9611_power_on(struct lt9611 *lt9611)
> +{
> + int ret;
> + const struct reg_sequence seq[] = {
> + /* LT9611_System_Init */
> + { 0x8101, 0x18 }, /* sel xtal clock */
> +
> + /* timer for frequency meter */
> + { 0x821b, 0x69 }, /*timer 2*/
> + { 0x821c, 0x78 },
> + { 0x82cb, 0x69 }, /*timer 1 */
> + { 0x82cc, 0x78 },
> +
> + /* irq init */
> + { 0x8251, 0x01 },
> + { 0x8258, 0x0a }, /* hpd irq */
> + { 0x8259, 0x80 }, /* hpd debounce width */
> + { 0x829e, 0xf7 }, /* video check irq */
> +
> + /* power consumption for work */
> + { 0x8004, 0xf0 },
> + { 0x8006, 0xf0 },
> + { 0x800a, 0x80 },
> + { 0x800b, 0x40 },
> + { 0x800d, 0xef },
> + { 0x8011, 0xfa },
> + };
> +
> + if (lt9611->power_on)
> + return 0;
> +
> + dev_dbg(lt9611->dev, "power on\n");
> +
> + ret = regmap_multi_reg_write(lt9611->regmap, seq, ARRAY_SIZE(seq));
> + if (!ret)
> + lt9611->power_on = true;
> +
> + return ret;
> +}
> +
> +static int lt9611_power_off(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + dev_dbg(lt9611->dev, "power off\n");
> +
> + ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> + if (!ret)
> + lt9611->power_on = false;
> +
> + return ret;
> +}
> +
> +static void lt9611_reset(struct lt9611 *lt9611)
> +{
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> + msleep(20);
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 0);
> + msleep(20);
> + gpiod_set_value_cansleep(lt9611->reset_gpio, 1);
> + msleep(100);
> +}
> +
> +static void lt9611_assert_5v(struct lt9611 *lt9611)
> +{
> + if (!lt9611->enable_gpio)
> + return;
> +
> + gpiod_set_value_cansleep(lt9611->enable_gpio, 1);
> + msleep(20);
> +}
> +
> +static int lt9611_regulator_init(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + lt9611->supplies[0].supply = "vdd";
> + lt9611->supplies[1].supply = "vcc";
> + ret = devm_regulator_bulk_get(lt9611->dev, 2, lt9611->supplies);
> + if (ret < 0)
> + return ret;
> +
> + return regulator_set_load(lt9611->supplies[0].consumer, 300000);
> +}
> +
> +static int lt9611_regulator_enable(struct lt9611 *lt9611)
> +{
> + int ret;
> +
> + ret = regulator_enable(lt9611->supplies[0].consumer);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(1000, 10000);
> +
> + ret = regulator_enable(lt9611->supplies[1].consumer);
> + if (ret < 0) {
> + regulator_disable(lt9611->supplies[0].consumer);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct lt9611_mode *lt9611_find_mode(const struct drm_display_mode *mode)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lt9611_modes); i++) {
> + if (lt9611_modes[i].hdisplay == mode->hdisplay &&
> + lt9611_modes[i].vdisplay == mode->vdisplay &&
> + lt9611_modes[i].fps == drm_mode_vrefresh(mode)) {
> + return <9611_modes[i];
> + }
> + }
> +
> + return NULL;
> +}
> +
> +/* connector funcs */
> +static enum drm_connector_status
> +lt9611_connector_detect(struct drm_connector *connector, bool force)
> +{
> + struct lt9611 *lt9611 = connector_to_lt9611(connector);
> + unsigned int reg_val = 0;
> + int connected = 0;
> +
> + regmap_read(lt9611->regmap, 0x825e, ®_val);
> + connected = (reg_val & BIT(2));
> + dev_dbg(lt9611->dev, "connected = %x\n", connected);
> +
> + lt9611->status = connected ? connector_status_connected :
> + connector_status_disconnected;
> +
> + return lt9611->status;
> +}
> +
> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> + unsigned int temp;
> + int ret = 0;
> + int i, j;
> +
> + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> +
> + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> + /* 0xA0 is EDID device address */
> + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> + /* 0x00 is EDID offset address */
> + regmap_write(lt9611->regmap, 0x8505, 0x00);
> + /* length for read */
> + regmap_write(lt9611->regmap, 0x8506, 0x20);
> + regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> + for (i = 0 ; i < 8 ; i++) {
> + /* offset address */
> + regmap_write(lt9611->regmap, 0x8505, i * 32);
> + regmap_write(lt9611->regmap, 0x8507, 0x36);
> + regmap_write(lt9611->regmap, 0x8507, 0x31);
> + regmap_write(lt9611->regmap, 0x8507, 0x37);
> + usleep_range(5000, 10000);
> +
> + regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> + if (temp & 0x02) { /*KEY_DDC_ACCS_DONE=1*/
> + for (j = 0; j < 32; j++) {
> + regmap_read(lt9611->regmap, 0x8583, &temp);
> + lt9611->edid_buf[i * 32 + j] = temp;
> + }
> + } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> + dev_err(lt9611->dev, "read edid failed: no ack\n");
> + ret = -EIO;
> + goto end;
> + } else {
> + dev_err(lt9611->dev,
> + "read edid failed: access not done\n");
> + ret = -EIO;
> + goto end;
> + }
> + }
> +
> + dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> + lt9611->edid_buf[255]);
> +
> +end:
> + regmap_write(lt9611->regmap, 0x8507, 0x1f);
> + return ret;
> +}
> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> + struct lt9611 *lt9611 = data;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> + block, (int)len);
> +
> + if (len > 128)
> + return -EINVAL;
> +
> + /* support up to 1 extension block */
> + if (block > 1)
> + return -EINVAL;
> +
> + if (block == 0) {
> + /* always read 2 edid blocks once */
> + ret = lt9611_read_edid(lt9611);
> + if (ret) {
> + dev_err(lt9611->dev, "edid read failed\n");
> + return ret;
> + }
> + }
> +
> + if (block % 2 == 0)
> + memcpy(buf, lt9611->edid_buf, len);
> + else
> + memcpy(buf, lt9611->edid_buf + 128, len);
> +
> + return 0;
> +}
> +
> +static int lt9611_connector_get_modes(struct drm_connector *connector)
> +{
> + struct lt9611 *lt9611 = connector_to_lt9611(connector);
> + unsigned int count;
> + struct edid *edid;
> +
> + dev_dbg(lt9611->dev, "get modes\n");
> +
> + lt9611_power_on(lt9611);
> + edid = drm_do_get_edid(connector, lt9611_get_edid_block, lt9611);
> + drm_connector_update_edid_property(connector, edid);
> + count = drm_add_edid_modes(connector, edid);
> + kfree(edid);
> +
> + return count;
> +}
> +
> +static enum drm_mode_status
> +lt9611_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> +
> + return lt9611_mode ? MODE_OK : MODE_BAD;
> +}
> +
> +/* bridge funcs */
> +static void lt9611_bridge_enable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge enable\n");
> +
> + if (lt9611_power_on(lt9611)) {
> + dev_err(lt9611->dev, "power on failed\n");
> + return;
> + }
> +
> + dev_dbg(lt9611->dev, "video on\n");
> +
> + lt9611_mipi_input_analog(lt9611);
> + lt9611_hdmi_tx_digital(lt9611);
> + lt9611_hdmi_tx_phy(lt9611);
> +
> + msleep(500);
> +
> + lt9611_video_check(lt9611);
> +
> + /* Enable HDMI output */
> + regmap_write(lt9611->regmap, 0x8130, 0xea);
> +}
> +
> +static void lt9611_bridge_disable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge disable\n");
> +
> + /* Disable HDMI output */
> + ret = regmap_write(lt9611->regmap, 0x8130, 0x6a);
> + if (ret) {
> + dev_err(lt9611->dev, "video on failed\n");
> + return;
> + }
> +
> + if (lt9611_power_off(lt9611)) {
> + dev_err(lt9611->dev, "power on failed\n");
> + return;
> + }
> +}
> +
> +static struct
> +drm_connector_helper_funcs lt9611_bridge_connector_helper_funcs = {
> + .get_modes = lt9611_connector_get_modes,
> + .mode_valid = lt9611_connector_mode_valid,
> +};
> +
> +static const struct drm_connector_funcs lt9611_bridge_connector_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .detect = lt9611_connector_detect,
> + .destroy = drm_connector_cleanup,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static struct mipi_dsi_device *lt9611_attach_dsi(struct lt9611 *lt9611,
> + struct device_node *dsi_node)
> +{
> + const struct mipi_dsi_device_info info = { "lt9611", 0, NULL };
> + struct mipi_dsi_device *dsi;
> + struct mipi_dsi_host *host;
> + int ret;
> +
> + host = of_find_mipi_dsi_host_by_node(dsi_node);
> + if (!host) {
> + dev_err(lt9611->dev, "failed to find dsi host\n");
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + dsi = mipi_dsi_device_register_full(host, &info);
> + if (IS_ERR(dsi)) {
> + dev_err(lt9611->dev, "failed to create dsi device\n");
> + return dsi;
> + }
> +
> + dsi->lanes = 4;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_MODE_VIDEO_HSE;
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0) {
> + dev_err(lt9611->dev, "failed to attach dsi to host\n");
> + mipi_dsi_device_unregister(dsi);
> + return ERR_PTR(ret);
> + }
> +
> + return dsi;
> +}
> +
> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge attach\n");
Connector creation in bridge drivers is deprecated. Please at least add
support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
should not be implemented at all. This will require the display
controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
controller(s) do you use this driver with ?
> +
> + ret = drm_connector_init(bridge->dev, <9611->connector,
> + <9611_bridge_connector_funcs,
> + DRM_MODE_CONNECTOR_HDMIA);
> + if (ret) {
> + DRM_ERROR("Failed to initialize connector with drm\n");
> + return ret;
> + }
> +
> + drm_connector_helper_add(<9611->connector,
> + <9611_bridge_connector_helper_funcs);
> + drm_connector_attach_encoder(<9611->connector, bridge->encoder);
> +
> + if (!bridge->encoder) {
> + DRM_ERROR("Parent encoder object not found");
> + return -ENODEV;
> + }
> +
> + /* Attach primary DSI */
> + lt9611->dsi0 = lt9611_attach_dsi(lt9611, lt9611->dsi0_node);
> + if (IS_ERR(lt9611->dsi0))
> + return PTR_ERR(lt9611->dsi0);
> +
> + /* Attach secondary DSI, if specified */
> + if (lt9611->dsi1_node) {
> + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> + if (IS_ERR(lt9611->dsi1)) {
> + ret = PTR_ERR(lt9611->dsi1);
> + goto err_unregister_dsi0;
> + }
> + }
> +
> + return 0;
> +
> +err_unregister_dsi0:
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +
> + return ret;
> +}
> +
> +static void lt9611_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + if (lt9611->dsi1) {
> + mipi_dsi_detach(lt9611->dsi1);
> + mipi_dsi_device_unregister(lt9611->dsi1);
> + }
> +
> + mipi_dsi_detach(lt9611->dsi0);
> + mipi_dsi_device_unregister(lt9611->dsi0);
> +}
> +
> +static enum drm_mode_status
> +lt9611_bridge_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode)
> +{
> + struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + if (lt9611_mode->intfs > 1 && !lt9611->dsi1)
> + return MODE_PANEL;
> + else
> + return MODE_OK;
> +}
> +
> +static void lt9611_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge pre_enable\n");
> +
> + if (!lt9611->sleep)
> + return;
> +
> + lt9611_reset(lt9611);
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +
> + lt9611->sleep = false;
> +}
> +
> +static void lt9611_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +
> + dev_dbg(lt9611->dev, "bridge post_disable\n");
> +
> + lt9611_sleep_setup(lt9611);
> +}
> +
> +static void lt9611_bridge_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adj_mode)
> +{
> + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + struct hdmi_avi_infoframe avi_frame;
> + int ret;
> +
> + dev_dbg(lt9611->dev, "bridge mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
> + adj_mode->hdisplay, adj_mode->vdisplay,
> + adj_mode->vrefresh, adj_mode->clock);
> +
> + lt9611_bridge_pre_enable(bridge);
> +
> + lt9611_mipi_input_digital(lt9611, mode);
> + lt9611_pll_setup(lt9611, mode);
> + lt9611_mipi_video_setup(lt9611, mode);
> + lt9611_pcr_setup(lt9611, mode);
> +
> + ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
> + <9611->connector,
> + mode);
> + if (!ret)
> + lt9611->vic = avi_frame.video_code;
> +}
> +
> +static const struct drm_bridge_funcs lt9611_bridge_funcs = {
> + .attach = lt9611_bridge_attach,
> + .detach = lt9611_bridge_detach,
> + .mode_valid = lt9611_bridge_mode_valid,
> + .enable = lt9611_bridge_enable,
> + .disable = lt9611_bridge_disable,
> + .post_disable = lt9611_bridge_post_disable,
> + .mode_set = lt9611_bridge_mode_set,
> +};
> +
> +static int lt9611_parse_dt(struct device *dev,
> + struct lt9611 *lt9611)
> +{
> + lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 1, -1);
> + if (!lt9611->dsi0_node) {
> + DRM_DEV_ERROR(dev, "failed to get remote node for primary dsi\n");
> + return -ENODEV;
> + }
> +
> + lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 2, -1);
> +
> + lt9611->ac_mode = of_property_read_bool(dev->of_node, "lt,ac-mode");
> + dev_dbg(lt9611->dev, "ac_mode=%d\n", lt9611->ac_mode);
> +
> + return 0;
> +}
> +
> +static int lt9611_gpio_init(struct lt9611 *lt9611)
> +{
> + struct device *dev = lt9611->dev;
> +
> + lt9611->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(lt9611->reset_gpio)) {
> + dev_err(dev, "failed to acquire reset gpio\n");
> + return PTR_ERR(lt9611->reset_gpio);
> + }
> +
> + lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(lt9611->enable_gpio)) {
> + dev_err(dev, "failed to acquire enable gpio\n");
> + return PTR_ERR(lt9611->enable_gpio);
> + }
> +
> + return 0;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> + unsigned int rev;
> + int ret;
> +
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> + if (ret)
> + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
> + dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> + return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct lt9611 *lt9611;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "device doesn't support I2C\n");
> + return -ENODEV;
> + }
> +
> + lt9611 = devm_kzalloc(dev, sizeof(*lt9611), GFP_KERNEL);
> + if (!lt9611)
> + return -ENOMEM;
> +
> + lt9611->dev = &client->dev;
> + lt9611->client = client;
> + lt9611->sleep = false;
> +
> + lt9611->regmap = devm_regmap_init_i2c(client, <9611_regmap_config);
> + if (IS_ERR(lt9611->regmap)) {
> + DRM_ERROR("regmap i2c init failed\n");
> + return PTR_ERR(lt9611->regmap);
> + }
> +
> + ret = lt9611_parse_dt(&client->dev, lt9611);
> + if (ret) {
> + dev_err(dev, "failed to parse device tree\n");
> + return ret;
> + }
> +
> + ret = lt9611_gpio_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + ret = lt9611_regulator_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + lt9611_assert_5v(lt9611);
> +
> + ret = lt9611_regulator_enable(lt9611);
> + if (ret)
> + return ret;
> +
> + lt9611_reset(lt9611);
> +
> + ret = lt9611_read_device_rev(lt9611);
> + if (ret) {
> + dev_err(dev, "failed to read chip rev\n");
> + goto err_disable_regulators;
> + }
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + lt9611_irq_thread_handler,
> + IRQF_ONESHOT, "lt9611", lt9611);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + goto err_disable_regulators;
> + }
> +
> + i2c_set_clientdata(client, lt9611);
> +
> + lt9611->bridge.funcs = <9611_bridge_funcs;
> + lt9611->bridge.of_node = client->dev.of_node;
> +
> + drm_bridge_add(<9611->bridge);
> +
> + lt9611_enable_hpd_interrupts(lt9611);
> +
> + return 0;
> +
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> + struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> + disable_irq(client->irq);
> + drm_bridge_remove(<9611->bridge);
> +
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> + { "lontium,lt9611", 0},
> + {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> + {.compatible = "lontium,lt9611"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lt9611_match_table);
> +
> +static struct i2c_driver lt9611_driver = {
> + .driver = {
> + .name = "lt9611",
> + .of_match_table = lt9611_match_table,
> + },
> + .probe = lt9611_probe,
> + .remove = lt9611_remove,
> + .id_table = lt9611_id,
> +};
> +module_i2c_driver(lt9611_driver);
> +
> +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-05-28 1:52 ` Laurent Pinchart
@ 2020-06-04 7:25 ` Vinod Koul
-1 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2020-06-04 7:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Daniel Vetter, David Airlie, Rob Clark, linux-arm-msm,
Bjorn Andersson, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
Jernej Skrabec, Rob Herring, devicetree, linux-kernel, dri-devel
Hi Laurent,
On 28-05-20, 04:52, Laurent Pinchart wrote:
> > +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > + int ret;
> > +
> > + dev_dbg(lt9611->dev, "bridge attach\n");
>
>
> Connector creation in bridge drivers is deprecated. Please at least add
Okay what is the right way for connector creation? I can add support for
that.
> support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
> creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
will add that
> should not be implemented at all. This will require the display
> controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
> controller(s) do you use this driver with ?
I am using with msm display driver, this was tested on dragon-board
db845c board.
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-06-04 7:25 ` Vinod Koul
0 siblings, 0 replies; 34+ messages in thread
From: Vinod Koul @ 2020-06-04 7:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devicetree, Jernej Skrabec, Neil Armstrong, David Airlie,
linux-arm-msm, Jonas Karlman, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda, Rob Herring
Hi Laurent,
On 28-05-20, 04:52, Laurent Pinchart wrote:
> > +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > + int ret;
> > +
> > + dev_dbg(lt9611->dev, "bridge attach\n");
>
>
> Connector creation in bridge drivers is deprecated. Please at least add
Okay what is the right way for connector creation? I can add support for
that.
> support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
> creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
will add that
> should not be implemented at all. This will require the display
> controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
> controller(s) do you use this driver with ?
I am using with msm display driver, this was tested on dragon-board
db845c board.
Thanks
--
~Vinod
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
2020-06-04 7:25 ` Vinod Koul
@ 2020-06-04 7:38 ` Laurent Pinchart
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2020-06-04 7:38 UTC (permalink / raw)
To: Vinod Koul
Cc: Daniel Vetter, David Airlie, Rob Clark, linux-arm-msm,
Bjorn Andersson, Andrzej Hajda, Neil Armstrong, Jonas Karlman,
Jernej Skrabec, Rob Herring, devicetree, linux-kernel, dri-devel
Hi Vinod,
On Thu, Jun 04, 2020 at 12:55:48PM +0530, Vinod Koul wrote:
> On 28-05-20, 04:52, Laurent Pinchart wrote:
>
> > > +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> > > + enum drm_bridge_attach_flags flags)
> > > +{
> > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > > + int ret;
> > > +
> > > + dev_dbg(lt9611->dev, "bridge attach\n");
> >
> >
> > Connector creation in bridge drivers is deprecated. Please at least add
>
> Okay what is the right way for connector creation? I can add support for
> that.
Historically bridge drivers have created connectors. With support for
bridge chaining, this approach was considered not to scale. For
instance, I have a board where the SoC has an internal LVDS encoder, and
the board itself has an LVDS-to-DPI decoder followed by a DPI-to-HDMI
encoder. All three components are supported by bridge drivers, and only
the last one should create a connector. Furthermore, different
operations of the connector may be implemented by different bridges, for
instance with one bridge connected to the DDC lines to read EDID, and
another bridge connected to the HPD line to detect hotplug.
To support these systems, we have deprecated connector creation in
bridges, in favour of implementing new bridge callback functions for
connector-related operations (see .get_modes(), .get_edid() and
.detect() in struct drm_bridge_funcs). With this new model, each bridge
implements the operations it supports, and the display controller driver
binds the bridges together to create a connector that delegates the
connector operations to the appropriate bridge. A helper function,
drm_bridge_connector_init(), can be used to automate that.
To transition to this model, we require all new bridge to at least
optionally support disabling connector creation (as requested by the
DRM_BRIDGE_ATTACH_NO_CONNECTOR), and implement the drm_bridge_funcs
functions related to connector operations. Existing bridges are also
converted to the new model. Once all bridges used by a display
controller support the new model, the display controller is then
converted to use DRM_BRIDGE_ATTACH_NO_CONNECTOR and
drm_bridge_connector_init() (or implement the latter manually if the
helper doesn't support all the display controller's needs). Once all
display controllers using a bridge have been converted to the new model,
support for creating a connector (the !DRM_BRIDGE_ATTACH_NO_CONNECTOR
case) is removed from the bridge driver. Finally, once everybody will
use the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, we will simply drop it.
> > support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
> > creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
>
> will add that
>
> > should not be implemented at all. This will require the display
> > controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
> > controller(s) do you use this driver with ?
>
> I am using with msm display driver, this was tested on dragon-board
> db845c board.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
@ 2020-06-04 7:38 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2020-06-04 7:38 UTC (permalink / raw)
To: Vinod Koul
Cc: devicetree, Jernej Skrabec, Neil Armstrong, David Airlie,
linux-arm-msm, Jonas Karlman, linux-kernel, dri-devel,
Bjorn Andersson, Andrzej Hajda, Rob Herring
Hi Vinod,
On Thu, Jun 04, 2020 at 12:55:48PM +0530, Vinod Koul wrote:
> On 28-05-20, 04:52, Laurent Pinchart wrote:
>
> > > +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> > > + enum drm_bridge_attach_flags flags)
> > > +{
> > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > > + int ret;
> > > +
> > > + dev_dbg(lt9611->dev, "bridge attach\n");
> >
> >
> > Connector creation in bridge drivers is deprecated. Please at least add
>
> Okay what is the right way for connector creation? I can add support for
> that.
Historically bridge drivers have created connectors. With support for
bridge chaining, this approach was considered not to scale. For
instance, I have a board where the SoC has an internal LVDS encoder, and
the board itself has an LVDS-to-DPI decoder followed by a DPI-to-HDMI
encoder. All three components are supported by bridge drivers, and only
the last one should create a connector. Furthermore, different
operations of the connector may be implemented by different bridges, for
instance with one bridge connected to the DDC lines to read EDID, and
another bridge connected to the HPD line to detect hotplug.
To support these systems, we have deprecated connector creation in
bridges, in favour of implementing new bridge callback functions for
connector-related operations (see .get_modes(), .get_edid() and
.detect() in struct drm_bridge_funcs). With this new model, each bridge
implements the operations it supports, and the display controller driver
binds the bridges together to create a connector that delegates the
connector operations to the appropriate bridge. A helper function,
drm_bridge_connector_init(), can be used to automate that.
To transition to this model, we require all new bridge to at least
optionally support disabling connector creation (as requested by the
DRM_BRIDGE_ATTACH_NO_CONNECTOR), and implement the drm_bridge_funcs
functions related to connector operations. Existing bridges are also
converted to the new model. Once all bridges used by a display
controller support the new model, the display controller is then
converted to use DRM_BRIDGE_ATTACH_NO_CONNECTOR and
drm_bridge_connector_init() (or implement the latter manually if the
helper doesn't support all the display controller's needs). Once all
display controllers using a bridge have been converted to the new model,
support for creating a connector (the !DRM_BRIDGE_ATTACH_NO_CONNECTOR
case) is removed from the bridge driver. Finally, once everybody will
use the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, we will simply drop it.
> > support for the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, to make connector
> > creation optional. Ideally the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case
>
> will add that
>
> > should not be implemented at all. This will require the display
> > controller driver to use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Which display
> > controller(s) do you use this driver with ?
>
> I am using with msm display driver, this was tested on dragon-board
> db845c board.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread