[PATCH v5 1/2] drm/bridge: Add sii902x driver

[PATCH v5 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon]

Add basic support for the sii902x RGB -> HDMI bridge.
This driver does not support audio output yet.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hello,

This patch is only adding basic support for the sii9022 chip.
As stated in the commit log, there's no audio support, but the
driver also hardcodes a lot of things (like the RGB input format to
use).

Best Regards,

Boris

Changes in v5:
- drop the best_encoder() implementation

Changes in v4:
- make reset GPIO optional
- only support attaching to DRM devices supporting atomic updates

Changes in v3:
- fix get_modes() implementation to avoid turning the screen in power
  save mode
- rename the driver (sil902x -> sii902x)

Changes in v2:
- fix errors reported by the kbuild robot

fixup! drm: bridge: Add sii902x driver
---
 drivers/gpu/drm/bridge/Kconfig   |   8 +
 drivers/gpu/drm/bridge/Makefile  |   1 +
 drivers/gpu/drm/bridge/sii902x.c | 469 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 478 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/sii902x.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 8f7423f..a1419214 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -50,6 +50,14 @@ config DRM_PARADE_PS8622
 	---help---
 	  Parade eDP-LVDS bridge chip driver.
 
+config DRM_SII902X
+	tristate "Silicon Image sii902x RGB/HDMI bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	select REGMAP_I2C
+	---help---
+	  Silicon Image sii902x bridge chip driver.
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 endmenu
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 96b13b3..bfec9f8 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
 obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
+obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
new file mode 100644
index 0000000..0912d2f
--- /dev/null
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -0,0 +1,469 @@
+/*
+ * Copyright (C) 2014 Atmel
+ *		      Bo Shen <voice.shen@atmel.com>
+ *
+ * Authors:	      Bo Shen <voice.shen@atmel.com>
+ *		      Boris Brezillon <boris.brezillon@free-electrons.com>
+ *		      Wu, Songjun <Songjun.Wu@atmel.com>
+ *
+ *
+ * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/component.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_encoder_slave.h>
+
+#define SIL902X_TPI_VIDEO_DATA			0x0
+
+#define SIL902X_TPI_PIXEL_REPETITION		0x8
+#define SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT     BIT(5)
+#define SIL902X_TPI_AVI_PIXEL_REP_RISING_EDGE   BIT(4)
+#define SIL902X_TPI_AVI_PIXEL_REP_4X		3
+#define SIL902X_TPI_AVI_PIXEL_REP_2X		1
+#define SIL902X_TPI_AVI_PIXEL_REP_NONE		0
+#define SIL902X_TPI_CLK_RATIO_HALF		(0 << 6)
+#define SIL902X_TPI_CLK_RATIO_1X		(1 << 6)
+#define SIL902X_TPI_CLK_RATIO_2X		(2 << 6)
+#define SIL902X_TPI_CLK_RATIO_4X		(3 << 6)
+
+#define SIL902X_TPI_AVI_IN_FORMAT		0x9
+#define SIL902X_TPI_AVI_INPUT_BITMODE_12BIT	BIT(7)
+#define SIL902X_TPI_AVI_INPUT_DITHER		BIT(6)
+#define SIL902X_TPI_AVI_INPUT_RANGE_LIMITED	(2 << 2)
+#define SIL902X_TPI_AVI_INPUT_RANGE_FULL	(1 << 2)
+#define SIL902X_TPI_AVI_INPUT_RANGE_AUTO	(0 << 2)
+#define SIL902X_TPI_AVI_INPUT_COLORSPACE_BLACK	(3 << 0)
+#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV422	(2 << 0)
+#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV444	(1 << 0)
+#define SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB	(0 << 0)
+
+#define SIL902X_TPI_AVI_INFOFRAME		0x0c
+
+#define SIL902X_SYS_CTRL_DATA			0x1a
+#define SIL902X_SYS_CTRL_PWR_DWN		BIT(4)
+#define SIL902X_SYS_CTRL_AV_MUTE		BIT(3)
+#define SIL902X_SYS_CTRL_DDC_BUS_REQ		BIT(2)
+#define SIL902X_SYS_CTRL_DDC_BUS_GRTD		BIT(1)
+#define SIL902X_SYS_CTRL_OUTPUT_MODE		BIT(0)
+#define SIL902X_SYS_CTRL_OUTPUT_HDMI		1
+#define SIL902X_SYS_CTRL_OUTPUT_DVI		0
+
+#define SIL902X_REG_CHIPID(n)			(0x1b + (n))
+
+#define SIL902X_PWR_STATE_CTRL			0x1e
+#define SIL902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
+#define SIL902X_AVI_POWER_STATE_D(l)		((l) & SIL902X_AVI_POWER_STATE_MSK)
+
+#define SI902X_INT_ENABLE			0x3c
+#define SI902X_INT_STATUS			0x3d
+#define SI902X_HOTPLUG_EVENT			BIT(0)
+#define SI902X_PLUGGED_STATUS			BIT(2)
+
+#define SIL902X_REG_TPI_RQB			0xc7
+
+struct sii902x {
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct gpio_desc *reset_gpio;
+	struct work_struct hotplug_work;
+};
+
+static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct sii902x, bridge);
+}
+
+static inline struct sii902x *connector_to_sii902x(struct drm_connector *con)
+{
+	return container_of(con, struct sii902x, connector);
+}
+
+static void sii902x_reset(struct sii902x *sii902x)
+{
+	if (!sii902x->reset_gpio)
+		return;
+
+	gpiod_set_value(sii902x->reset_gpio, 1);
+
+	msleep(100);
+
+	gpiod_set_value(sii902x->reset_gpio, 0);
+}
+
+static void sii902x_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+}
+
+static enum drm_connector_status
+sii902x_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct sii902x *sii902x = connector_to_sii902x(connector);
+	unsigned int status;
+
+	regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
+
+	return (status & SI902X_PLUGGED_STATUS) ?
+	       connector_status_connected : connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs sii902x_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.detect = sii902x_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = sii902x_connector_destroy,
+	.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 int sii902x_get_modes(struct drm_connector *connector)
+{
+	struct sii902x *sii902x = connector_to_sii902x(connector);
+	struct regmap *regmap = sii902x->regmap;
+	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	unsigned int status;
+	struct edid *edid;
+	int num = 0;
+	int ret;
+	int i;
+
+	ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
+				 SIL902X_SYS_CTRL_DDC_BUS_REQ,
+				 SIL902X_SYS_CTRL_DDC_BUS_REQ);
+	if (ret)
+		return ret;
+
+	i = 0;
+	do {
+		ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
+		if (ret)
+			return ret;
+		i++;
+	} while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));
+
+	ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
+	if (ret)
+		return ret;
+
+	edid = drm_get_edid(connector, sii902x->i2c->adapter);
+	drm_mode_connector_update_edid_property(connector, edid);
+	if (edid) {
+		num += drm_add_edid_modes(connector, edid);
+		kfree(edid);
+	}
+
+	ret = drm_display_info_set_bus_formats(&connector->display_info,
+					       &bus_format, 1);
+	if (ret)
+		return ret;
+
+	regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
+				 SIL902X_SYS_CTRL_DDC_BUS_REQ |
+				 SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
+	if (ret)
+		return ret;
+
+	i = 0;
+	do {
+		ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
+		if (ret)
+			return ret;
+		i++;
+	} while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
+			   SIL902X_SYS_CTRL_DDC_BUS_GRTD));
+
+	return num;
+}
+
+static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
+					       struct drm_display_mode *mode)
+{
+	/* TODO: check mode */
+
+	return MODE_OK;
+}
+
+static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = {
+	.get_modes = sii902x_get_modes,
+	.mode_valid = sii902x_mode_valid,
+};
+
+static void sii902x_bridge_disable(struct drm_bridge *bridge)
+{
+	struct sii902x *sii902x = bridge_to_sii902x(bridge);
+
+	regmap_update_bits(sii902x->regmap, SIL902X_SYS_CTRL_DATA,
+			   SIL902X_SYS_CTRL_PWR_DWN,
+			   SIL902X_SYS_CTRL_PWR_DWN);
+}
+
+static void sii902x_bridge_enable(struct drm_bridge *bridge)
+{
+	struct sii902x *sii902x = bridge_to_sii902x(bridge);
+
+	regmap_update_bits(sii902x->regmap, SIL902X_PWR_STATE_CTRL,
+			   SIL902X_AVI_POWER_STATE_MSK,
+			   SIL902X_AVI_POWER_STATE_D(0));
+	regmap_update_bits(sii902x->regmap, SIL902X_SYS_CTRL_DATA,
+			   SIL902X_SYS_CTRL_PWR_DWN, 0);
+}
+
+static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj)
+{
+	u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
+	struct sii902x *sii902x = bridge_to_sii902x(bridge);
+	struct regmap *regmap = sii902x->regmap;
+	struct hdmi_avi_infoframe frame;
+	int ret;
+
+	buf[0] = adj->clock;
+	buf[1] = adj->clock >> 8;
+	buf[2] = adj->vrefresh;
+	buf[3] = 0x00;
+	buf[4] = adj->hdisplay;
+	buf[5] = adj->hdisplay >> 8;
+	buf[6] = adj->vdisplay;
+	buf[7] = adj->vdisplay >> 8;
+	buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
+		 SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
+	buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
+		 SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
+
+	ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
+	if (ret)
+		return;
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return;
+	}
+
+	ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
+	if (ret < 0) {
+		DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
+		return;
+	}
+
+	/* Do not send the infoframe header, but keep the CRC field. */
+	regmap_bulk_write(regmap, SIL902X_TPI_AVI_INFOFRAME,
+			  buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
+			  HDMI_AVI_INFOFRAME_SIZE + 1);
+}
+
+static int sii902x_bridge_attach(struct drm_bridge *bridge)
+{
+	struct sii902x *sii902x = bridge_to_sii902x(bridge);
+	struct drm_device *drm = bridge->dev;
+	int ret;
+
+	drm_connector_helper_add(&sii902x->connector,
+				 &sii902x_connector_helper_funcs);
+
+	if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
+		dev_err(&sii902x->i2c->dev,
+			"sii902x driver is only compatible with DRM devices supporting atomic updates");
+		return -ENOTSUPP;
+	}
+
+	ret = drm_connector_init(drm, &sii902x->connector,
+				 &sii902x_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret)
+		return ret;
+
+	if (sii902x->i2c->irq > 0)
+		sii902x->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
+
+	drm_mode_connector_attach_encoder(&sii902x->connector, bridge->encoder);
+
+	return 0;
+}
+
+static void sii902x_bridge_nop(struct drm_bridge *bridge)
+{
+}
+
+static const struct drm_bridge_funcs sii902x_bridge_funcs = {
+	.attach = sii902x_bridge_attach,
+	.mode_set = sii902x_bridge_mode_set,
+	.disable = sii902x_bridge_disable,
+	.post_disable = sii902x_bridge_nop,
+	.pre_enable = sii902x_bridge_nop,
+	.enable = sii902x_bridge_enable,
+};
+
+static const struct regmap_range sii902x_volatile_ranges[] = {
+	{ .range_min = 0, .range_max = 0xff },
+};
+
+static const struct regmap_access_table sii902x_volatile_table = {
+	.yes_ranges = sii902x_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges),
+};
+
+static const struct regmap_config sii902x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &sii902x_volatile_table,
+	.cache_type = REGCACHE_NONE,
+};
+
+static irqreturn_t sii902x_interrupt(int irq, void *data)
+{
+	struct sii902x *sii902x = data;
+	unsigned int status = 0;
+
+	regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
+	regmap_write(sii902x->regmap, SI902X_INT_STATUS, status);
+
+	if ((status & SI902X_HOTPLUG_EVENT) && sii902x->bridge.dev)
+		drm_helper_hpd_irq_event(sii902x->bridge.dev);
+
+	return IRQ_HANDLED;
+}
+
+static int sii902x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	unsigned int status = 0;
+	struct sii902x *sii902x;
+	u8 chipid[4];
+	int ret;
+
+	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
+	if (!sii902x)
+		return -ENOMEM;
+
+	sii902x->i2c = client;
+	sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config);
+	if (IS_ERR(sii902x->regmap))
+		return PTR_ERR(sii902x->regmap);
+
+	sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(sii902x->reset_gpio))
+		dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
+			 PTR_ERR(sii902x->reset_gpio));
+
+	sii902x_reset(sii902x);
+
+	ret = regmap_write(sii902x->regmap, SIL902X_REG_TPI_RQB, 0x0);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(sii902x->regmap, SIL902X_REG_CHIPID(0),
+			       &chipid, 4);
+	if (ret) {
+		dev_err(dev, "regmap_read failed %d\n", ret);
+		return ret;
+	}
+
+	if (chipid[0] != 0xb0) {
+		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
+			chipid[0]);
+		return -EINVAL;
+	}
+
+	/* Clear all pending interrupts */
+	regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
+	regmap_write(sii902x->regmap, SI902X_INT_STATUS, status);
+
+	if (client->irq > 0) {
+		regmap_write(sii902x->regmap, SI902X_INT_ENABLE,
+			     SI902X_HOTPLUG_EVENT);
+
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						sii902x_interrupt,
+						IRQF_ONESHOT, dev_name(dev),
+						sii902x);
+		if (ret)
+			return ret;
+	}
+
+	sii902x->bridge.funcs = &sii902x_bridge_funcs;
+	sii902x->bridge.of_node = dev->of_node;
+	ret = drm_bridge_add(&sii902x->bridge);
+	if (ret) {
+		dev_err(dev, "Failed to add drm_bridge\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, sii902x);
+
+	return 0;
+}
+
+static int sii902x_remove(struct i2c_client *client)
+
+{
+	struct sii902x *sii902x = i2c_get_clientdata(client);
+
+	drm_bridge_remove(&sii902x->bridge);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id sii902x_dt_ids[] = {
+	{ .compatible = "sil,sii9022", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
+#endif
+
+static const struct i2c_device_id sii902x_i2c_ids[] = {
+	{ "sii9022", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
+
+static struct i2c_driver sii902x_driver = {
+	.probe = sii902x_probe,
+	.remove = sii902x_remove,
+	.driver = {
+		.name = "sii902x",
+		.of_match_table = of_match_ptr(sii902x_dt_ids),
+	},
+	.id_table = sii902x_i2c_ids,
+};
+module_i2c_driver(sii902x_driver);
+
+MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
+MODULE_DESCRIPTION("SIL902x RGB -> HDMI bridges");
+MODULE_LICENSE("GPL");
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/2] drm/bridge: Add sii902x DT bindings doc

[Boris Brezillon]

Add Sii9022 DT bindings description.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes since v1:
- rename doc file
- s/sil902/sii902/
---
 .../devicetree/bindings/display/bridge/sii902x.txt | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/sii902x.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
new file mode 100644
index 0000000..e8d0ffa
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -0,0 +1,35 @@
+sii902x HDMI bridge bindings
+
+Required properties:
+	- compatible: "sil,sii9022"
+	- reg: i2c address of the bridge
+	- reset-gpios: OF device-tree gpio specification for RST_N pin.
+
+Optional properties:
+	- interrupts-extended or interrupt-parent + interrupts: describe
+	  the interrupt line used to inform the host about hotplug events.
+
+Optional subnodes:
+	- video input: this subnode can contain a video input port node
+	  to connect the bridge to a display controller output (See this
+	  documentation [1]).
+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+	hdmi-bridge@39 {
+		compatible = "sil,sii9022";
+		reg = <0x39>;
+		reset-gpios = <&pioA 1 0>;
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				bridge_in: endpoint {
+					remote-endpoint = <&dc_out>;
+				};
+			};
+		};
+	};
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Enric Balletbo Serra]

Hi Boris,

A few comments below ...

2016-06-02 17:00 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Add basic support for the sii902x RGB -> HDMI bridge.
> This driver does not support audio output yet.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Hello,
>
> This patch is only adding basic support for the sii9022 chip.
> As stated in the commit log, there's no audio support, but the
> driver also hardcodes a lot of things (like the RGB input format to
> use).
>
> Best Regards,
>
> Boris
>
> Changes in v5:
> - drop the best_encoder() implementation
>
> Changes in v4:
> - make reset GPIO optional
> - only support attaching to DRM devices supporting atomic updates
>
> Changes in v3:
> - fix get_modes() implementation to avoid turning the screen in power
>   save mode
> - rename the driver (sil902x -> sii902x)
>
> Changes in v2:
> - fix errors reported by the kbuild robot
>
> fixup! drm: bridge: Add sii902x driver
> ---
>  drivers/gpu/drm/bridge/Kconfig   |   8 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/sii902x.c | 469 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 478 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/sii902x.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 8f7423f..a1419214 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -50,6 +50,14 @@ config DRM_PARADE_PS8622
>         ---help---
>           Parade eDP-LVDS bridge chip driver.
>
> +config DRM_SII902X
> +       tristate "Silicon Image sii902x RGB/HDMI bridge"
> +       depends on OF
> +       select DRM_KMS_HELPER
> +       select REGMAP_I2C
> +       ---help---
> +         Silicon Image sii902x bridge chip driver.
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>
>  endmenu
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 96b13b3..bfec9f8 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> +obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> new file mode 100644
> index 0000000..0912d2f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -0,0 +1,469 @@
> +/*
> + * Copyright (C) 2014 Atmel
> + *                   Bo Shen <voice.shen@atmel.com>
> + *
> + * Authors:          Bo Shen <voice.shen@atmel.com>
> + *                   Boris Brezillon <boris.brezillon@free-electrons.com>
> + *                   Wu, Songjun <Songjun.Wu@atmel.com>
> + *
> + *
> + * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/component.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_encoder_slave.h>
> +
> +#define SIL902X_TPI_VIDEO_DATA                 0x0
> +
> +#define SIL902X_TPI_PIXEL_REPETITION           0x8
> +#define SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT     BIT(5)
> +#define SIL902X_TPI_AVI_PIXEL_REP_RISING_EDGE   BIT(4)
> +#define SIL902X_TPI_AVI_PIXEL_REP_4X           3
> +#define SIL902X_TPI_AVI_PIXEL_REP_2X           1
> +#define SIL902X_TPI_AVI_PIXEL_REP_NONE         0
> +#define SIL902X_TPI_CLK_RATIO_HALF             (0 << 6)
> +#define SIL902X_TPI_CLK_RATIO_1X               (1 << 6)
> +#define SIL902X_TPI_CLK_RATIO_2X               (2 << 6)
> +#define SIL902X_TPI_CLK_RATIO_4X               (3 << 6)
> +
> +#define SIL902X_TPI_AVI_IN_FORMAT              0x9
> +#define SIL902X_TPI_AVI_INPUT_BITMODE_12BIT    BIT(7)
> +#define SIL902X_TPI_AVI_INPUT_DITHER           BIT(6)
> +#define SIL902X_TPI_AVI_INPUT_RANGE_LIMITED    (2 << 2)
> +#define SIL902X_TPI_AVI_INPUT_RANGE_FULL       (1 << 2)
> +#define SIL902X_TPI_AVI_INPUT_RANGE_AUTO       (0 << 2)
> +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_BLACK (3 << 0)
> +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV422        (2 << 0)
> +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV444        (1 << 0)
> +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB   (0 << 0)
> +
> +#define SIL902X_TPI_AVI_INFOFRAME              0x0c
> +
> +#define SIL902X_SYS_CTRL_DATA                  0x1a
> +#define SIL902X_SYS_CTRL_PWR_DWN               BIT(4)
> +#define SIL902X_SYS_CTRL_AV_MUTE               BIT(3)
> +#define SIL902X_SYS_CTRL_DDC_BUS_REQ           BIT(2)
> +#define SIL902X_SYS_CTRL_DDC_BUS_GRTD          BIT(1)
> +#define SIL902X_SYS_CTRL_OUTPUT_MODE           BIT(0)
> +#define SIL902X_SYS_CTRL_OUTPUT_HDMI           1
> +#define SIL902X_SYS_CTRL_OUTPUT_DVI            0
> +
> +#define SIL902X_REG_CHIPID(n)                  (0x1b + (n))
> +
> +#define SIL902X_PWR_STATE_CTRL                 0x1e
> +#define SIL902X_AVI_POWER_STATE_MSK            GENMASK(1, 0)
> +#define SIL902X_AVI_POWER_STATE_D(l)           ((l) & SIL902X_AVI_POWER_STATE_MSK)
> +
> +#define SI902X_INT_ENABLE                      0x3c
> +#define SI902X_INT_STATUS                      0x3d
> +#define SI902X_HOTPLUG_EVENT                   BIT(0)
> +#define SI902X_PLUGGED_STATUS                  BIT(2)
> +
> +#define SIL902X_REG_TPI_RQB                    0xc7
> +
> +struct sii902x {
> +       struct i2c_client *i2c;
> +       struct regmap *regmap;
> +       struct drm_bridge bridge;
> +       struct drm_connector connector;
> +       struct gpio_desc *reset_gpio;
> +       struct work_struct hotplug_work;
> +};
> +
> +static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> +{
> +       return container_of(bridge, struct sii902x, bridge);
> +}
> +
> +static inline struct sii902x *connector_to_sii902x(struct drm_connector *con)
> +{
> +       return container_of(con, struct sii902x, connector);
> +}
> +
> +static void sii902x_reset(struct sii902x *sii902x)
> +{
> +       if (!sii902x->reset_gpio)
> +               return;
> +
> +       gpiod_set_value(sii902x->reset_gpio, 1);
> +
> +       msleep(100);
> +
> +       gpiod_set_value(sii902x->reset_gpio, 0);
> +}
> +
> +static void sii902x_connector_destroy(struct drm_connector *connector)
> +{
> +       drm_connector_unregister(connector);
> +       drm_connector_cleanup(connector);
> +}
> +
> +static enum drm_connector_status
> +sii902x_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct sii902x *sii902x = connector_to_sii902x(connector);
> +       unsigned int status;
> +
> +       regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
> +
> +       return (status & SI902X_PLUGGED_STATUS) ?
> +              connector_status_connected : connector_status_disconnected;
> +}
> +
> +static const struct drm_connector_funcs sii902x_connector_funcs = {
> +       .dpms = drm_atomic_helper_connector_dpms,
> +       .detect = sii902x_connector_detect,
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .destroy = sii902x_connector_destroy,
> +       .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 int sii902x_get_modes(struct drm_connector *connector)
> +{
> +       struct sii902x *sii902x = connector_to_sii902x(connector);
> +       struct regmap *regmap = sii902x->regmap;
> +       u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +       unsigned int status;
> +       struct edid *edid;
> +       int num = 0;
> +       int ret;
> +       int i;

Is the i variable really needed? (see my comments below)

> +
> +       ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> +                                SIL902X_SYS_CTRL_DDC_BUS_REQ,
> +                                SIL902X_SYS_CTRL_DDC_BUS_REQ);
> +       if (ret)
> +               return ret;
> +
> +       i = 0;

You assign i to 0

> +       do {
> +               ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> +               if (ret)
> +                       return ret;
> +               i++;

And you increment i, for what?

> +       } while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> +
> +       ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
> +       if (ret)
> +               return ret;
> +
> +       edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +       drm_mode_connector_update_edid_property(connector, edid);
> +       if (edid) {
> +               num += drm_add_edid_modes(connector, edid);

This is always 0 + the returned value, so you can do:
  num = drm_add_edid_modes(connector, edid);
It's more clear for me.

> +               kfree(edid);
> +       }
> +
> +       ret = drm_display_info_set_bus_formats(&connector->display_info,
> +                                              &bus_format, 1);
> +       if (ret)
> +               return ret;
> +
> +       regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> +                                SIL902X_SYS_CTRL_DDC_BUS_REQ |
> +                                SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> +       if (ret)
> +               return ret;
> +
> +       i = 0;

Again, you can remove the i variable, here and the i++ from the loop below

> +       do {
> +               ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> +               if (ret)
> +                       return ret;
> +               i++;
> +       } while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
> +                          SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> +
> +       return num;
> +}
> +
> +static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
> +                                              struct drm_display_mode *mode)
> +{
> +       /* TODO: check mode */
> +
> +       return MODE_OK;
> +}
> +
> +static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = {
> +       .get_modes = sii902x_get_modes,
> +       .mode_valid = sii902x_mode_valid,
> +};
> +
> +static void sii902x_bridge_disable(struct drm_bridge *bridge)
> +{
> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +
> +       regmap_update_bits(sii902x->regmap, SIL902X_SYS_CTRL_DATA,
> +                          SIL902X_SYS_CTRL_PWR_DWN,
> +                          SIL902X_SYS_CTRL_PWR_DWN);
> +}
> +
> +static void sii902x_bridge_enable(struct drm_bridge *bridge)
> +{
> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +
> +       regmap_update_bits(sii902x->regmap, SIL902X_PWR_STATE_CTRL,
> +                          SIL902X_AVI_POWER_STATE_MSK,
> +                          SIL902X_AVI_POWER_STATE_D(0));
> +       regmap_update_bits(sii902x->regmap, SIL902X_SYS_CTRL_DATA,
> +                          SIL902X_SYS_CTRL_PWR_DWN, 0);
> +}
> +
> +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> +                                   struct drm_display_mode *mode,
> +                                   struct drm_display_mode *adj)
> +{
> +       u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +       struct regmap *regmap = sii902x->regmap;
> +       struct hdmi_avi_infoframe frame;
> +       int ret;
> +
> +       buf[0] = adj->clock;
> +       buf[1] = adj->clock >> 8;
> +       buf[2] = adj->vrefresh;
> +       buf[3] = 0x00;
> +       buf[4] = adj->hdisplay;
> +       buf[5] = adj->hdisplay >> 8;
> +       buf[6] = adj->vdisplay;
> +       buf[7] = adj->vdisplay >> 8;
> +       buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
> +                SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> +       buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
> +                SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> +
> +       ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
> +       if (ret)
> +               return;
> +
> +       ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj);
> +       if (ret < 0) {
> +               DRM_ERROR("couldn't fill AVI infoframe\n");
> +               return;
> +       }
> +
> +       ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
> +       if (ret < 0) {
> +               DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
> +               return;
> +       }
> +
> +       /* Do not send the infoframe header, but keep the CRC field. */
> +       regmap_bulk_write(regmap, SIL902X_TPI_AVI_INFOFRAME,
> +                         buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
> +                         HDMI_AVI_INFOFRAME_SIZE + 1);
> +}
> +
> +static int sii902x_bridge_attach(struct drm_bridge *bridge)
> +{
> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +       struct drm_device *drm = bridge->dev;
> +       int ret;
> +
> +       drm_connector_helper_add(&sii902x->connector,
> +                                &sii902x_connector_helper_funcs);
> +
> +       if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
> +               dev_err(&sii902x->i2c->dev,
> +                       "sii902x driver is only compatible with DRM devices supporting atomic updates");
> +               return -ENOTSUPP;
> +       }
> +
> +       ret = drm_connector_init(drm, &sii902x->connector,
> +                                &sii902x_connector_funcs,
> +                                DRM_MODE_CONNECTOR_HDMIA);
> +       if (ret)
> +               return ret;
> +
> +       if (sii902x->i2c->irq > 0)
> +               sii902x->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +       else
> +               sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
> +
> +       drm_mode_connector_attach_encoder(&sii902x->connector, bridge->encoder);
> +
> +       return 0;
> +}
> +
> +static void sii902x_bridge_nop(struct drm_bridge *bridge)
> +{
> +}
> +

You can remove this dummy callback function now.

> +static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> +       .attach = sii902x_bridge_attach,
> +       .mode_set = sii902x_bridge_mode_set,
> +       .disable = sii902x_bridge_disable,
> +       .post_disable = sii902x_bridge_nop,
> +       .pre_enable = sii902x_bridge_nop,

Remove .pre_enable

> +       .enable = sii902x_bridge_enable,
> +};
> +
> +static const struct regmap_range sii902x_volatile_ranges[] = {
> +       { .range_min = 0, .range_max = 0xff },
> +};
> +
> +static const struct regmap_access_table sii902x_volatile_table = {
> +       .yes_ranges = sii902x_volatile_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges),
> +};
> +
> +static const struct regmap_config sii902x_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .volatile_table = &sii902x_volatile_table,
> +       .cache_type = REGCACHE_NONE,
> +};
> +
> +static irqreturn_t sii902x_interrupt(int irq, void *data)
> +{
> +       struct sii902x *sii902x = data;
> +       unsigned int status = 0;
> +
> +       regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
> +       regmap_write(sii902x->regmap, SI902X_INT_STATUS, status);
> +
> +       if ((status & SI902X_HOTPLUG_EVENT) && sii902x->bridge.dev)
> +               drm_helper_hpd_irq_event(sii902x->bridge.dev);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int sii902x_probe(struct i2c_client *client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct device *dev = &client->dev;
> +       unsigned int status = 0;
> +       struct sii902x *sii902x;
> +       u8 chipid[4];
> +       int ret;
> +
> +       sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
> +       if (!sii902x)
> +               return -ENOMEM;
> +
> +       sii902x->i2c = client;
> +       sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config);
> +       if (IS_ERR(sii902x->regmap))
> +               return PTR_ERR(sii902x->regmap);
> +
> +       sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                     GPIOD_OUT_LOW);
> +       if (IS_ERR(sii902x->reset_gpio))
> +               dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
> +                        PTR_ERR(sii902x->reset_gpio));
> +
> +       sii902x_reset(sii902x);
> +
> +       ret = regmap_write(sii902x->regmap, SIL902X_REG_TPI_RQB, 0x0);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_bulk_read(sii902x->regmap, SIL902X_REG_CHIPID(0),
> +                              &chipid, 4);
> +       if (ret) {
> +               dev_err(dev, "regmap_read failed %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (chipid[0] != 0xb0) {
> +               dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
> +                       chipid[0]);
> +               return -EINVAL;
> +       }
> +
> +       /* Clear all pending interrupts */
> +       regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
> +       regmap_write(sii902x->regmap, SI902X_INT_STATUS, status);
> +
> +       if (client->irq > 0) {
> +               regmap_write(sii902x->regmap, SI902X_INT_ENABLE,
> +                            SI902X_HOTPLUG_EVENT);
> +
> +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +                                               sii902x_interrupt,
> +                                               IRQF_ONESHOT, dev_name(dev),
> +                                               sii902x);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       sii902x->bridge.funcs = &sii902x_bridge_funcs;
> +       sii902x->bridge.of_node = dev->of_node;
> +       ret = drm_bridge_add(&sii902x->bridge);
> +       if (ret) {
> +               dev_err(dev, "Failed to add drm_bridge\n");
> +               return ret;
> +       }
> +
> +       i2c_set_clientdata(client, sii902x);
> +
> +       return 0;
> +}
> +
> +static int sii902x_remove(struct i2c_client *client)
> +
> +{
> +       struct sii902x *sii902x = i2c_get_clientdata(client);
> +
> +       drm_bridge_remove(&sii902x->bridge);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF

You already depend on OF in Kconfig so this will always be evaluated.

> +static const struct of_device_id sii902x_dt_ids[] = {
> +       { .compatible = "sil,sii9022", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
> +#endif
> +
> +static const struct i2c_device_id sii902x_i2c_ids[] = {
> +       { "sii9022", 0 },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
> +
> +static struct i2c_driver sii902x_driver = {
> +       .probe = sii902x_probe,
> +       .remove = sii902x_remove,
> +       .driver = {
> +               .name = "sii902x",
> +               .of_match_table = of_match_ptr(sii902x_dt_ids),

You already depend on OF in Kconfig so you don't need of_match_ptr()
here, of_match_ptr(x) will always evaluate to x.

> +       },
> +       .id_table = sii902x_i2c_ids,
> +};
> +module_i2c_driver(sii902x_driver);
> +
> +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
> +MODULE_DESCRIPTION("SIL902x RGB -> HDMI bridges");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>

Best regards,
  Enric

> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon]

Hi Enric,

On Thu, 2 Jun 2016 23:47:23 +0200
Enric Balletbo Serra <eballetbo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:



> > +static int sii902x_get_modes(struct drm_connector *connector)
> > +{
> > +       struct sii902x *sii902x = connector_to_sii902x(connector);
> > +       struct regmap *regmap = sii902x->regmap;
> > +       u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +       unsigned int status;
> > +       struct edid *edid;
> > +       int num = 0;
> > +       int ret;
> > +       int i;  
> 
> Is the i variable really needed? (see my comments below)
> 
> > +
> > +       ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +                                SIL902X_SYS_CTRL_DDC_BUS_REQ,
> > +                                SIL902X_SYS_CTRL_DDC_BUS_REQ);
> > +       if (ret)
> > +               return ret;
> > +
> > +       i = 0;  
> 
> You assign i to 0
> 
> > +       do {
> > +               ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +               if (ret)
> > +                       return ret;
> > +               i++;  
> 
> And you increment i, for what?

Oops, this is a leftover from when I was debugging the implementation.

> 
> > +       } while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > +       ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
> > +       if (ret)
> > +               return ret;
> > +
> > +       edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > +       drm_mode_connector_update_edid_property(connector, edid);
> > +       if (edid) {
> > +               num += drm_add_edid_modes(connector, edid);  
> 
> This is always 0 + the returned value, so you can do:
>   num = drm_add_edid_modes(connector, edid);
> It's more clear for me.

Sure.

> 
> > +               kfree(edid);
> > +       }
> > +
> > +       ret = drm_display_info_set_bus_formats(&connector->display_info,
> > +                                              &bus_format, 1);
> > +       if (ret)
> > +               return ret;
> > +
> > +       regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +                                SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > +                                SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > +       if (ret)
> > +               return ret;
> > +
> > +       i = 0;  
> 
> Again, you can remove the i variable, here and the i++ from the loop below
> 
> > +       do {
> > +               ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +               if (ret)
> > +                       return ret;
> > +               i++;
> > +       } while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > +                          SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > +       return num;
> > +}

[...]

> > +static void sii902x_bridge_nop(struct drm_bridge *bridge)
> > +{
> > +}
> > +  
> 
> You can remove this dummy callback function now.
> 
> > +static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> > +       .attach = sii902x_bridge_attach,
> > +       .mode_set = sii902x_bridge_mode_set,
> > +       .disable = sii902x_bridge_disable,
> > +       .post_disable = sii902x_bridge_nop,
> > +       .pre_enable = sii902x_bridge_nop,  
> 
> Remove .pre_enable

I guess ->{pre,post}_enable() were mandatory when I started the
development of this driver. I'll drop both.

> 
> > +       .enable = sii902x_bridge_enable,
> > +};
> > +

[...]

> > +
> > +#ifdef CONFIG_OF  
> 
> You already depend on OF in Kconfig so this will always be evaluated.

Indeed.

> 
> > +static const struct of_device_id sii902x_dt_ids[] = {
> > +       { .compatible = "sil,sii9022", },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
> > +#endif
> > +
> > +static const struct i2c_device_id sii902x_i2c_ids[] = {
> > +       { "sii9022", 0 },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
> > +
> > +static struct i2c_driver sii902x_driver = {
> > +       .probe = sii902x_probe,
> > +       .remove = sii902x_remove,
> > +       .driver = {
> > +               .name = "sii902x",
> > +               .of_match_table = of_match_ptr(sii902x_dt_ids),  
> 
> You already depend on OF in Kconfig so you don't need of_match_ptr()
> here, of_match_ptr(x) will always evaluate to x.

I'll directly pass sii902x_dt_ids.

Thanks for your review.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Emil Velikov]

Hi Boris.

On 2 June 2016 at 16:00, Boris Brezillon
<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> +static void sii902x_reset(struct sii902x *sii902x)
> +{
> +       if (!sii902x->reset_gpio)
> +               return;
> +
This is wrong (reset_gpio is err_ptr) although we can/should nuke it
all together. See below for reasoning.

> +       gpiod_set_value(sii902x->reset_gpio, 1);
> +
> +       msleep(100);
Ouch that is some juicy number. Can we get a comment with
reasoning/origin of it ?

...

> +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> +                                   struct drm_display_mode *mode,
> +                                   struct drm_display_mode *adj)
> +{
> +       u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.

> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +       struct regmap *regmap = sii902x->regmap;
> +       struct hdmi_avi_infoframe frame;
> +       int ret;
> +
> +       buf[0] = adj->clock;
> +       buf[1] = adj->clock >> 8;
> +       buf[2] = adj->vrefresh;
> +       buf[3] = 0x00;
> +       buf[4] = adj->hdisplay;
> +       buf[5] = adj->hdisplay >> 8;
> +       buf[6] = adj->vdisplay;
> +       buf[7] = adj->vdisplay >> 8;
> +       buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
> +                SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> +       buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
> +                SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> +
Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
the above into const video_data[] ?

> +       ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?

...

> +static int sii902x_bridge_attach(struct drm_bridge *bridge)
> +{
> +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> +       struct drm_device *drm = bridge->dev;
> +       int ret;
> +
> +       drm_connector_helper_add(&sii902x->connector,
> +                                &sii902x_connector_helper_funcs);
> +
> +       if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
> +               dev_err(&sii902x->i2c->dev,
> +                       "sii902x driver is only compatible with DRM devices supporting atomic updates");
> +               return -ENOTSUPP;
> +       }
> +
> +       ret = drm_connector_init(drm, &sii902x->connector,
> +                                &sii902x_connector_funcs,
> +                                DRM_MODE_CONNECTOR_HDMIA);
Side note: seems like most places in DRM do not check the return value
(~80 vs ~20). I wonder how badly/likely are things to explode.

...

> +static int sii902x_probe(struct i2c_client *client,
> +                        const struct i2c_device_id *id)
> +{
...

> +
> +       sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                     GPIOD_OUT_LOW);
> +       if (IS_ERR(sii902x->reset_gpio))
> +               dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
> +                        PTR_ERR(sii902x->reset_gpio));
> +
Documentation says "Required" not optional. The above should be
updated and one should error out if missing, right ?

...

> +
> +       if (client->irq > 0) {
I was always confused which is the correct way to check this >= 0 vs >
0. DRM has both :-\
Do you have any suggestions, should be 'mass convert' DRM to use only
one of the two ?


Regards,
Emil
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon]

Hi Emil,

On Fri, 3 Jun 2016 10:38:49 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> Hi Boris.
> 
> On 2 June 2016 at 16:00, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> 
> > +static void sii902x_reset(struct sii902x *sii902x)
> > +{
> > +       if (!sii902x->reset_gpio)
> > +               return;
> > +  
> This is wrong (reset_gpio is err_ptr) although we can/should nuke it
> all together. See below for reasoning.
> 
> > +       gpiod_set_value(sii902x->reset_gpio, 1);
> > +
> > +       msleep(100);  
> Ouch that is some juicy number. Can we get a comment with
> reasoning/origin of it ?

As already explained to Maxime, I just don't know why this is needed,
simply because I don't have access to the datasheet and I just based my
implementation on another driver.
I can add a comment stating that this was extracted from another
implementation, but with no explanation on why this is needed.

Meng, do you have any information about startup-time, or something like
that?

> 
> ...
> 
> > +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> > +                                   struct drm_display_mode *mode,
> > +                                   struct drm_display_mode *adj)
> > +{
> > +       u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];  
> HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.

Yep.

> 
> > +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> > +       struct regmap *regmap = sii902x->regmap;
> > +       struct hdmi_avi_infoframe frame;
> > +       int ret;
> > +
> > +       buf[0] = adj->clock;
> > +       buf[1] = adj->clock >> 8;
> > +       buf[2] = adj->vrefresh;
> > +       buf[3] = 0x00;
> > +       buf[4] = adj->hdisplay;
> > +       buf[5] = adj->hdisplay >> 8;
> > +       buf[6] = adj->vdisplay;
> > +       buf[7] = adj->vdisplay >> 8;
> > +       buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
> > +                SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> > +       buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
> > +                SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> > +  
> Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
> the above into const video_data[] ?

Something like

	const video_data[] = {
		adj->clock,
		adj->clock >> 8,
		...
	};

So we would have 2 buffers on the stack? Is this really useful?

> 
> > +       ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);  
> ... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
> 
> ...
> 
> > +static int sii902x_bridge_attach(struct drm_bridge *bridge)
> > +{
> > +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
> > +       struct drm_device *drm = bridge->dev;
> > +       int ret;
> > +
> > +       drm_connector_helper_add(&sii902x->connector,
> > +                                &sii902x_connector_helper_funcs);
> > +
> > +       if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
> > +               dev_err(&sii902x->i2c->dev,
> > +                       "sii902x driver is only compatible with DRM devices supporting atomic updates");
> > +               return -ENOTSUPP;
> > +       }
> > +
> > +       ret = drm_connector_init(drm, &sii902x->connector,
> > +                                &sii902x_connector_funcs,
> > +                                DRM_MODE_CONNECTOR_HDMIA);  
> Side note: seems like most places in DRM do not check the return value
> (~80 vs ~20). I wonder how badly/likely are things to explode.

Yep. I tend to always check return code, but if you say it's useless
(and error-prone) I can remove it.

> 
> ...
> 
> > +static int sii902x_probe(struct i2c_client *client,
> > +                        const struct i2c_device_id *id)
> > +{  
> ...
> 
> > +
> > +       sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +                                                     GPIOD_OUT_LOW);
> > +       if (IS_ERR(sii902x->reset_gpio))
> > +               dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
> > +                        PTR_ERR(sii902x->reset_gpio));
> > +  
> Documentation says "Required" not optional. The above should be
> updated and one should error out if missing, right ?

Actually I was asked to make it optional, just forgot to update the
documentation. This being said, devm_gpiod_get_optional() returns NULL
if the property is not defined in the DT and an error code if the error
comes from the GPIO layer, so I should just switch back to dev_err()
and return the error code here.

This would make the test in sii902x_reset() valid again.

> 
> ...
> 
> > +
> > +       if (client->irq > 0) {  
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?

Not sure 0 is a valid irq number anymore, so I don't think it's really
important, but I can change it if you want.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Alexandre Belloni]

Hi,

On 03/06/2016 at 10:38:49 +0100, Emil Velikov wrote :
> On 2 June 2016 at 16:00, Boris Brezillon
> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > +
> > +       if (client->irq > 0) {
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?
> 

0 is an invalid irq number for any i2c devices since
  dab472eb931bc2916fa779e56deccd0ec319cf5b.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Lucas Stach]

Am Freitag, den 03.06.2016, 10:38 +0100 schrieb Emil Velikov:
> Hi Boris.
[...]
> 
> > +
> > +       if (client->irq > 0) {
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?
> 
IRQ 0 is "no assigned irq", so invalid. Checking for > 0 is the right
thing to do.

Regards,
LUcas

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Emil Velikov]

On 3 June 2016 at 11:02, Boris Brezillon
<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Emil,
>
> On Fri, 3 Jun 2016 10:38:49 +0100
> Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Hi Boris.
>>
>> On 2 June 2016 at 16:00, Boris Brezillon
>> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>
>> > +static void sii902x_reset(struct sii902x *sii902x)
>> > +{
>> > +       if (!sii902x->reset_gpio)
>> > +               return;
>> > +
>> This is wrong (reset_gpio is err_ptr) although we can/should nuke it
>> all together. See below for reasoning.
>>
>> > +       gpiod_set_value(sii902x->reset_gpio, 1);
>> > +
>> > +       msleep(100);
>> Ouch that is some juicy number. Can we get a comment with
>> reasoning/origin of it ?
>
> As already explained to Maxime, I just don't know why this is needed,
> simply because I don't have access to the datasheet and I just based my
> implementation on another driver.
> I can add a comment stating that this was extracted from another
> implementation, but with no explanation on why this is needed.
>
Considering we don't hear from Meng, that sounds perfectly reasonable imho.

> Meng, do you have any information about startup-time, or something like
> that?
>
>>
>> ...
>>
>> > +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>> > +                                   struct drm_display_mode *mode,
>> > +                                   struct drm_display_mode *adj)
>> > +{
>> > +       u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
>> HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.
>
> Yep.
>
>>
>> > +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
>> > +       struct regmap *regmap = sii902x->regmap;
>> > +       struct hdmi_avi_infoframe frame;
>> > +       int ret;
>> > +
>> > +       buf[0] = adj->clock;
>> > +       buf[1] = adj->clock >> 8;
>> > +       buf[2] = adj->vrefresh;
>> > +       buf[3] = 0x00;
>> > +       buf[4] = adj->hdisplay;
>> > +       buf[5] = adj->hdisplay >> 8;
>> > +       buf[6] = adj->vdisplay;
>> > +       buf[7] = adj->vdisplay >> 8;
>> > +       buf[8] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
>> > +                SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
>> > +       buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
>> > +                SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
>> > +
>> Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
>> the above into const video_data[] ?
>
> Something like
>
>         const video_data[] = {
>                 adj->clock,
>                 adj->clock >> 8,
>                 ...
>         };
>
> So we would have 2 buffers on the stack? Is this really useful?
>
IMHO It makes things substantially clearer, but if you/others
disagree, feel free to ignore.

>>
>> > +       ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
>> ... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
>>
>> ...
>>
>> > +static int sii902x_bridge_attach(struct drm_bridge *bridge)
>> > +{
>> > +       struct sii902x *sii902x = bridge_to_sii902x(bridge);
>> > +       struct drm_device *drm = bridge->dev;
>> > +       int ret;
>> > +
>> > +       drm_connector_helper_add(&sii902x->connector,
>> > +                                &sii902x_connector_helper_funcs);
>> > +
>> > +       if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
>> > +               dev_err(&sii902x->i2c->dev,
>> > +                       "sii902x driver is only compatible with DRM devices supporting atomic updates");
>> > +               return -ENOTSUPP;
>> > +       }
>> > +
>> > +       ret = drm_connector_init(drm, &sii902x->connector,
>> > +                                &sii902x_connector_funcs,
>> > +                                DRM_MODE_CONNECTOR_HDMIA);
>> Side note: seems like most places in DRM do not check the return value
>> (~80 vs ~20). I wonder how badly/likely are things to explode.
>
> Yep. I tend to always check return code, but if you say it's useless
> (and error-prone) I can remove it.
>
Not sure which one is the correct thing to do. My gut goes "keep it as
is for the moment". I only pointed it out since DRM subsystem was
quite inconsistent about it.

>>
>> ...
>>
>> > +static int sii902x_probe(struct i2c_client *client,
>> > +                        const struct i2c_device_id *id)
>> > +{
>> ...
>>
>> > +
>> > +       sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>> > +                                                     GPIOD_OUT_LOW);
>> > +       if (IS_ERR(sii902x->reset_gpio))
>> > +               dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
>> > +                        PTR_ERR(sii902x->reset_gpio));
>> > +
>> Documentation says "Required" not optional. The above should be
>> updated and one should error out if missing, right ?
>
> Actually I was asked to make it optional, just forgot to update the
> documentation. This being said, devm_gpiod_get_optional() returns NULL
> if the property is not defined in the DT and an error code if the error
> comes from the GPIO layer, so I should just switch back to dev_err()
> and return the error code here.
>
> This would make the test in sii902x_reset() valid again.
>
Nice :-)

>>
>> ...
>>
>> > +
>> > +       if (client->irq > 0) {
>> I was always confused which is the correct way to check this >= 0 vs >
>> 0. DRM has both :-\
>> Do you have any suggestions, should be 'mass convert' DRM to use only
>> one of the two ?
>
> Not sure 0 is a valid irq number anymore, so I don't think it's really
> important, but I can change it if you want.
>
Thanks. In that case I'd leave it as-is.

At some point we'll get a brave soul that goes through DRM. But that
would be some other day.

Thanks
Emil
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Meng Yi]

Hi Boris,

Sorry for reply late, I was on PTO. And another PTO on  June 9~11, 2016.UTC+8

>
> > > +       gpiod_set_value(sii902x->reset_gpio, 1);
> > > +
> > > +       msleep(100);
> > Ouch that is some juicy number. Can we get a comment with
> > reasoning/origin of it ?
> 
> As already explained to Maxime, I just don't know why this is needed, simply
> because I don't have access to the datasheet and I just based my
> implementation on another driver.
> I can add a comment stating that this was extracted from another
> implementation, but with no explanation on why this is needed.
> 
> Meng, do you have any information about startup-time, or something like that?
> 

I had checked the datasheet of sii9022a, which said the Min of TRESET is 100 μs, and Max is not specified.

Regards,
Meng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon]

Hi Meng,

On Tue, 7 Jun 2016 02:40:56 +0000
Meng Yi <meng.yi@nxp.com> wrote:

> Hi Boris,
> 
> Sorry for reply late, I was on PTO. And another PTO on  June 9~11, 2016.UTC+8
> 
> >  
> > > > +       gpiod_set_value(sii902x->reset_gpio, 1);
> > > > +
> > > > +       msleep(100);  
> > > Ouch that is some juicy number. Can we get a comment with
> > > reasoning/origin of it ?  
> > 
> > As already explained to Maxime, I just don't know why this is needed, simply
> > because I don't have access to the datasheet and I just based my
> > implementation on another driver.
> > I can add a comment stating that this was extracted from another
> > implementation, but with no explanation on why this is needed.
> > 
> > Meng, do you have any information about startup-time, or something like that?
> >   
> 
> I had checked the datasheet of sii9022a, which said the Min of TRESET is 100 μs, and Max is not specified.

Oh, thanks a lot. treset-min is exactly what I needed.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Meng Yi]

Hi Boris,

> Changes in v5:
> - drop the best_encoder() implementation
> 

Why best_encoder() droped? It's not needed anymore?

Regards,
Meng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon]

On Tue, 7 Jun 2016 08:28:18 +0000
Meng Yi <meng.yi@nxp.com> wrote:

> Hi Boris,
> 
> > Changes in v5:
> > - drop the best_encoder() implementation
> >   
> 
> Why best_encoder() droped? It's not needed anymore?

Nope, not after this series [1].

[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/432869.html

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel