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). There are two reasons for this: 1/ the DRM framework does not allow for advanced link description between an encoder and a bridge (that's for the RGB format limitation). Any idea how this should be described?
2/ I don't have the datasheet of this HDMI encoder, and all logic has been extracted from those two drivers [1][2], which means I may miss some important things in my encoder setup.
Another thing I find weird in the drm_bridge interface is the fact that we have a ->attach() method, but no ->detach(), which can be a problem if we allow drm bridges and KMS drivers to be compiled as modules. Any reason for that?
That's all for the questions part :-).
Best Regards,
Boris
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 --- drivers/gpu/drm/bridge/Kconfig | 8 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/sii902x.c | 479 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 488 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 27e2022..fd463ea 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -40,4 +40,12 @@ 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. + endmenu diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index f13c33d..7144de3 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -4,3 +4,4 @@ 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 diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c new file mode 100644 index 0000000..4bcb84c --- /dev/null +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -0,0 +1,479 @@ +/* + * 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) +{ + 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_atomic_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 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, +}; + +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 struct drm_encoder *sii902x_best_encoder(struct drm_connector *connector) +{ + struct sii902x *sii902x = connector_to_sii902x(connector); + + return sii902x->bridge.encoder; +} + +static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = { + .get_modes = sii902x_get_modes, + .mode_valid = sii902x_mode_valid, + .best_encoder = sii902x_best_encoder, +}; + +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) +{ + const struct drm_connector_funcs *funcs = &sii902x_connector_funcs; + 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)) + funcs = &sii902x_atomic_connector_funcs; + + ret = drm_connector_init(drm, &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(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(sii902x->reset_gpio)) { + dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n", + PTR_ERR(sii902x->reset_gpio)); + return 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");
Add Sii9022 DT bindings description.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com --- 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>; + }; + }; + }; + };
Hi Boris,
-----Original Message----- From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
+static int sii902x_bridge_attach(struct drm_bridge *bridge) {
- const struct drm_connector_funcs *funcs = &sii902x_connector_funcs;
- 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))
funcs = &sii902x_atomic_connector_funcs;- ret = drm_connector_init(drm, &sii902x->connector, funcs,
DRM_MODE_CONNECTOR_HDMIA);- if (ret)
return ret;
I think drm_connector_register should be used here, or drmlib can't find this connector.
- 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 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(dev, "reset",
GPIOD_OUT_LOW);
- if (IS_ERR(sii902x->reset_gpio)) {
dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n",PTR_ERR(sii902x->reset_gpio));return PTR_ERR(sii902x->reset_gpio);
Maybe we can use "warning" instead of "dev_err" if there is no "reset_gpio" , because some board using CPLD to manage the reset order , they can't reset the HDMI chip independently.
- }
- sii902x_reset(sii902x);
Regards, Meng
Hi Meng,
On Fri, 22 Apr 2016 09:58:34 +0000 Meng Yi meng.yi@nxp.com wrote:
Hi Boris,
-----Original Message----- From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
+static int sii902x_bridge_attach(struct drm_bridge *bridge) {
- const struct drm_connector_funcs *funcs = &sii902x_connector_funcs;
- 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))
funcs = &sii902x_atomic_connector_funcs;- ret = drm_connector_init(drm, &sii902x->connector, funcs,
DRM_MODE_CONNECTOR_HDMIA);- if (ret)
return ret;I think drm_connector_register should be used here, or drmlib can't find this connector.
Hm, I think this should be left to the DRM master device (some of them already call drm_connector_register_all() at the end of their ->probe() function).
- 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 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(dev, "reset",
GPIOD_OUT_LOW);
- if (IS_ERR(sii902x->reset_gpio)) {
dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n",PTR_ERR(sii902x->reset_gpio));return PTR_ERR(sii902x->reset_gpio);Maybe we can use "warning" instead of "dev_err" if there is no "reset_gpio" , because some board using CPLD to manage the reset order , they can't reset the HDMI chip independently.
Sure, since it's optional maybe we should use dev_info().
Thanks for your review.
Regards,
Boris
Hi,
On Wed, Mar 16, 2016 at 03:14:43PM +0100, Boris Brezillon wrote:
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). There are two reasons for this: 1/ the DRM framework does not allow for advanced link description between an encoder and a bridge (that's for the RGB format limitation). Any idea how this should be described?
2/ I don't have the datasheet of this HDMI encoder, and all logic has been extracted from those two drivers [1][2], which means I may miss some important things in my encoder setup.
Another thing I find weird in the drm_bridge interface is the fact that we have a ->attach() method, but no ->detach(), which can be a problem if we allow drm bridges and KMS drivers to be compiled as modules. Any reason for that?
That's all for the questions part :-).
Best Regards,
Boris
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
drivers/gpu/drm/bridge/Kconfig | 8 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/sii902x.c | 479 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 488 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 27e2022..fd463ea 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -40,4 +40,12 @@ 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.endmenu diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index f13c33d..7144de3 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -4,3 +4,4 @@ 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 diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c new file mode 100644 index 0000000..4bcb84c --- /dev/null +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -0,0 +1,479 @@ +/*
- 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) +{
- gpiod_set_value(sii902x->reset_gpio, 1);
- msleep(100);
Sleeping for 100ms seems like a lot. Is it required, or is it just a leftover from an early development?
- 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_atomic_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 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,
+};
I'm guessing this is to support both atomic and !atomic drivers. I thought it was working automatically?
+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 struct drm_encoder *sii902x_best_encoder(struct drm_connector *connector) +{
- struct sii902x *sii902x = connector_to_sii902x(connector);
- return sii902x->bridge.encoder;
+}
+static const struct drm_connector_helper_funcs sii902x_connector_helper_funcs = {
- .get_modes = sii902x_get_modes,
- .mode_valid = sii902x_mode_valid,
- .best_encoder = sii902x_best_encoder,
+};
+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) +{
- const struct drm_connector_funcs *funcs = &sii902x_connector_funcs;
- 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))
funcs = &sii902x_atomic_connector_funcs;- ret = drm_connector_init(drm, &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(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(sii902x->reset_gpio)) {
dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n",PTR_ERR(sii902x->reset_gpio));return 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.5.0
It looks fine otherwise.
Maxime
Hi Maxime,
On Mon, 2 May 2016 13:05:57 +0200 Maxime Ripard maxime.ripard@free-electrons.com wrote:
+static void sii902x_reset(struct sii902x *sii902x) +{
- gpiod_set_value(sii902x->reset_gpio, 1);
- msleep(100);
Sleeping for 100ms seems like a lot. Is it required, or is it just a leftover from an early development?
I wish I had the answer, unfortunately I don't have the datasheet and the implementation I based my work on [1] is sleeping 100ms.
- 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_atomic_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 const struct drm_connector_funcs sii902x_connector_funcs = {
- .dpms = drm_atomic_helper_connector_dpms,
Should be drm_atomic_helper_dpms here.
- .detect = sii902x_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = sii902x_connector_destroy,
+};
I'm guessing this is to support both atomic and !atomic drivers. I thought it was working automatically?
Hm, the dw-hdmi driver is providing both, but maybe it's not needed. Daniel, any comments?
Thanks for the review.
Boris
[1]https://github.com/linux4sam/linux-at91/blob/linux-3.10-at91/drivers/hdmi/en...
On Tue, May 3, 2016 at 11:39 AM, Boris Brezillon boris.brezillon@free-electrons.com wrote:
- .detect = sii902x_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = sii902x_connector_destroy,
+};
I'm guessing this is to support both atomic and !atomic drivers. I thought it was working automatically?
Hm, the dw-hdmi driver is providing both, but maybe it's not needed. Daniel, any comments?
You only need this in case you actually have atomic and !atomic users of your bridge. We discussed whether we should have a dpms helper that dtrt automatically, but with just 1 driver that doesn't seem worth it. You wouldn't need to have 2 entier vfunc tables, you could just switch on DRIVER_ATOMIC. -Daniel
On Tue, 3 May 2016 11:53:18 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Tue, May 3, 2016 at 11:39 AM, Boris Brezillon boris.brezillon@free-electrons.com wrote:
- .detect = sii902x_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = sii902x_connector_destroy,
+};
I'm guessing this is to support both atomic and !atomic drivers. I thought it was working automatically?
Hm, the dw-hdmi driver is providing both, but maybe it's not needed. Daniel, any comments?
You only need this in case you actually have atomic and !atomic users of your bridge. We discussed whether we should have a dpms helper that dtrt automatically, but with just 1 driver that doesn't seem worth it. You wouldn't need to have 2 entier vfunc tables, you could just switch on DRIVER_ATOMIC.
Ok, I'll drop the !atomic helpers. Should I still test drm_core_check_feature(drm, DRIVER_ATOMIC) in the ->attach() implementation and return an error if the DRM device does not support atomic operations?
On Tue, May 03, 2016 at 12:03:08PM +0200, Boris Brezillon wrote:
On Tue, 3 May 2016 11:53:18 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Tue, May 3, 2016 at 11:39 AM, Boris Brezillon boris.brezillon@free-electrons.com wrote:
- .detect = sii902x_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = sii902x_connector_destroy,
+};
I'm guessing this is to support both atomic and !atomic drivers. I thought it was working automatically?
Hm, the dw-hdmi driver is providing both, but maybe it's not needed. Daniel, any comments?
You only need this in case you actually have atomic and !atomic users of your bridge. We discussed whether we should have a dpms helper that dtrt automatically, but with just 1 driver that doesn't seem worth it. You wouldn't need to have 2 entier vfunc tables, you could just switch on DRIVER_ATOMIC.
Ok, I'll drop the !atomic helpers. Should I still test drm_core_check_feature(drm, DRIVER_ATOMIC) in the ->attach() implementation and return an error if the DRM device does not support atomic operations?
If you feel like. If someone tries to use this bridge driver with a non-atomic driver then - they'll have a big fireworks show either way - a really good reason to fix up their driver to be atomic, like it should be ;-)
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Daniel Vetter -
Maxime Ripard -
Meng Yi