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");
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>; + }; + }; + }; + };
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 OFselect DRM_KMS_HELPERselect 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;elsesii902x->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
Hi Enric,
On Thu, 2 Jun 2016 23:47:23 +0200 Enric Balletbo Serra eballetbo@gmail.com 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
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 ?
...
+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
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
On 3 June 2016 at 11:02, Boris Brezillon boris.brezillon@free-electrons.com wrote:
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.
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
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
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
Hi,
On 03/06/2016 at 10:38:49 +0100, Emil Velikov wrote :
On 2 June 2016 at 16:00, Boris Brezillon boris.brezillon@free-electrons.com 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.
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
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
dri-devel@lists.freedesktop.org
-
Alexandre Belloni -
Boris Brezillon -
Emil Velikov -
Enric Balletbo Serra -
Lucas Stach -
Meng Yi