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

Hi Archit,

On Thu, 9 Jun 2016 12:15:00 +0530 Archit Taneja architt@codeaurora.org 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@free-electrons.com Tested-by: Nicolas Ferre nicolas.ferre@atmel.com

---

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@atmel.com>  
    

Maybe a newer copyright here?

Sure.

...

• • 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>

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@free-electrons.com"); +MODULE_DESCRIPTION("SIL902x RGB -> HDMI bridges");

SII902x here.

Looks good apart from the minor comments.

Thanks for the review.

Regards,

Boris