| Message ID | 1464879601-30569-1-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 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 Boris, > Changes in v5: > - drop the best_encoder() implementation > Why best_encoder() droped? It's not needed anymore? Regards, Meng
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
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");