Hi Archit, On Thu, 9 Jun 2016 12:15:00 +0530 Archit Taneja <architt@xxxxxxxxxxxxxx> wrote: > Hi Boris, > > On 6/8/2016 6:00 PM, Boris Brezillon wrote: > > Add basic support for the sii902x RGB -> HDMI bridge. > > This driver does not support audio output yet. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> > > Tested-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxx> > > --- > > Hi Archit, > > > > I recently learned you were the drm/bridge maintainer, and I realize > > you were not in Cc of the previous versions. > > > > Once you have some time, could you review this series, and let me know > > what's the procedure to get it merged (send a PR to Dave, or let you > > take it into your tree). > > I'm still in the process of preparing my account, and figuring out > how to not mess up sending pull requests, etc. If it's done fast > enough, I'll prepare a tree and start collecting stuff. It it's getting > too late, Daniel/Dave/Thierry can pull it for this merge window. Ok, cool! > > Some comments below. > > > > > Thanks, > > > > Boris > > > > Changes in v6: > > - use HDMI_INFOFRAME_SIZE(AVI) > > - fix reset_gpio initialization > > - reduce the reset time based on Ming feedback > > > > 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 | 459 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 468 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..d46bf98 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/sii902x.c > > @@ -0,0 +1,459 @@ > > +/* > > + * Copyright (C) 2014 Atmel > > + * Bo Shen <voice.shen@xxxxxxxxx> > > Maybe a newer copyright here? Sure. > > > + * > > + * Authors: Bo Shen <voice.shen@xxxxxxxxx> > > + * Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> > > + * Wu, Songjun <Songjun.Wu@xxxxxxxxx> > > + * > > + * > > + * 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> > > This doesn't seem to be needed. > > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_gpio.h> > > Same for the two above. > > > +#include <linux/regmap.h> > > + > > +#include <drm/drmP.h> > > +#include <drm/drm_atomic.h> > > drm_atomic.h isn't needed either. > > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_crtc_helper.h> > > +#include <drm/drm_edid.h> > > +#include <drm/drm_encoder_slave.h> > > This one above too. Will remove all the unneeded inclusions. > > > + > > +#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 > > I suppose the registers could be renamed to SII902x_... too? Yep, and I'll also do the following change: s/SIL/SII/ > > > + > > +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; > > hotplug_work doesn't seem to be used in the driver. Indeed. > > > +}; > > + [...] > > + > > +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, > > You can pass the drm_connector_cleanup helper directly instead of > creating a destroy op. Yes, I'll pass drm_connector_cleanup. > > > + .reset = drm_atomic_helper_connector_reset, > > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > +}; > > + > > +static 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; > > + > > + 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; > > + > > + do { > > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status); > > + if (ret) > > + return ret; > > + } while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD)); > > Can this result into an infinite while loop? Maybe we should timeout > after a few tries? You can ignore this comment if it's something > that's highly unprobable. No, I guess it's better to have a timeout. > > > + > > + 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); > > + } > > Shouldn't we ideally call drm_mode_connector_update_edid_property once > we know edid is non-NULL? Hm, drm_mode_connector_update_edid_property() seems to test edid value, and I guess you still want to drop the previous edid information even if the new one is NULL. > > > + > > + 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; > > + > > + do { > > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status); > > + if (ret) > > + return ret; > > + } while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ | > > + SIL902X_SYS_CTRL_DDC_BUS_GRTD)); > > Same thing about the infinite loop here. > > > + > > + return num; > > +} > > + [...] > > + 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_err(dev, "Failed to retrieve/request reset gpio: %ld\n", > > + PTR_ERR(sii902x->reset_gpio)); > > + return PTR_ERR(sii902x->reset_gpio); > > + } > > + > > + sii902x_reset(sii902x); > > + > > + ret = regmap_write(sii902x->regmap, SIL902X_REG_TPI_RQB, 0x0); > > + if (ret) > > + return ret; > > Is there a reason why we're checking for the return values of regmap > funcs in a couple of the functions but not all? I've seen it in other > drivers too, just wondering.. Well, I guess we should check all regmap accesses everywhere, especially when the regmap is accessed over an external bus, but people usually assume that regmap accesses will never fail :-/. The places where I'm not checking it are those called from 'void func()' functions, where checking the return code would be useless. > > > + > > + 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; > > +} > > + > > +static const struct of_device_id sii902x_dt_ids[] = { > > + { .compatible = "sil,sii9022", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, sii902x_dt_ids); > > + > > +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 = sii902x_dt_ids, > > + }, > > + .id_table = sii902x_i2c_ids, > > +}; > > +module_i2c_driver(sii902x_driver); > > + > > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("SIL902x RGB -> HDMI bridges"); > > SII902x here. > > Looks good apart from the minor comments. Thanks for the review. Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel