On 2018-04-14 00:59, Sean Paul wrote:

On Fri, Apr 13, 2018 at 10:53:00AM +0530, Sandeep Panda wrote:

...

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by: Sandeep Panda spanda@codeaurora.org

Hi Sandeep, Thank you for your patch!

...

---

drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1019 +++++++++++++++++++++++++++++++++ 1 file changed, 1019 insertions(+) create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c new file mode 100644 index 0000000..93aa1ad --- /dev/null +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -0,0 +1,1019 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+#define pr_fmt(fmt) "%s: " fmt, __func__

Instead of using pr_* for logging, please use the DRM_* variants. Since there is unlikely to be more than one of these bridge drivers in a system, you won't need to use the DRM_DEV_* versions.

...

+#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/of_gpio.h> +#include <linux/of_graph.h> +#include <linux/of_irq.h> +#include <linux/regulator/consumer.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_mipi_dsi.h>

+#define SN_DEVICE_REV_REG 0x08

+/* Link Training specific registers */ +#define SN_REFCLK_FREQ_REG 0x0A +#define SN_DSI_LANES_REG 0x10 +#define SN_DSIA_CLK_FREQ_REG 0x12 +#define SN_ENH_FRAME_REG 0x5A +#define SN_SSC_CONFIG_REG 0x93 +#define SN_DATARATE_CONFIG_REG 0x94 +#define SN_PLL_ENABLE_REG 0x0D +#define SN_SCRAMBLE_CONFIG_REG 0x95 +#define SN_AUX_WDATA0_REG 0x64 +#define SN_AUX_ADDR_19_16_REG 0x74 +#define SN_AUX_ADDR_15_8_REG 0x75 +#define SN_AUX_ADDR_7_0_REG 0x76 +#define SN_AUX_LENGTH_REG 0x77 +#define SN_AUX_CMD_REG 0x78 +#define SN_ML_TX_MODE_REG 0x96 +#define SN_PAGE_SELECT_REG 0xFF +#define SN_AUX_RDATA0_REG 0x79

+/* video config specific registers */ +#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG 0x20 +#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG 0x21 +#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG 0x24 +#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG 0x25 +#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG 0x2C +#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG 0x2D +#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG 0x30 +#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG 0x31 +#define SN_CHA_HORIZONTAL_BACK_PORCH_REG 0x34 +#define SN_CHA_VERTICAL_BACK_PORCH_REG 0x36 +#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG 0x38 +#define SN_CHA_VERTICAL_FRONT_PORCH_REG 0x3A +#define SN_DATA_FORMAT_REG 0x5B +#define SN_COLOR_BAR_CONFIG_REG 0x3C

It'd be nice if you could tab-align the values.

...

+struct sn65dsi86_reg_cfg {

• u8 reg;
• u8 val;
• int sleep_in_ms;

+};

+struct sn65dsi86_video_cfg {

• u32 h_active;
• u32 h_front_porch;
• u32 h_pulse_width;
• u32 h_back_porch;
• bool h_polarity;
• u32 v_active;
• u32 v_front_porch;
• u32 v_pulse_width;
• u32 v_back_porch;
• bool v_polarity;
• u32 pclk_khz;
• u32 num_of_lanes;

+};

All of these can be derived from drm_display_mode except for num_of_lanes which is hardcoded. So let's just use drm_display_mode directly.

...

+struct sn65dsi86_gpios {

• struct gpio_desc *irq_gpio;
• struct gpio_desc *enable_gpio;
• struct gpio_desc *aux_i2c_sda;
• struct gpio_desc *aux_i2c_scl;
• struct gpio_desc *edp_bias_en;
• struct gpio_desc *edp_bklt_en;
• struct gpio_desc *edp_bklt_ctrl;

+};

I see IRQ and EN in the datasheet, but not the others. It seems like the aux_* and edp_* gpios are always equal to en. So if you actually need them, don't specify a new struct, just add irq_gpio to the main struct and add an array of en_gpios to handle the rest.

...

+struct sn65dsi86 {

I will have fits trying to type this name. Could you please use something simple, like sn_bridge? Same comment applies to all of the function names.

...

• struct device *dev;
• struct drm_bridge bridge;
• struct drm_connector connector;
• struct device_node *host_node;
• struct mipi_dsi_device *dsi;
• u8 i2c_addr;

Unused

...

• int irq;
• struct sn65dsi86_gpios gpios;
• unsigned int num_supplies;
• struct regulator_bulk_data *supplies;
• struct i2c_client *i2c_client;
• enum drm_connector_status connector_status;

You never use the value of this, you just assign it. So you can remove this.

...

• bool power_on;
• bool is_pluggable;

As I mentioned in the dt review, you can determine this via panel. You should also take this into account in detect().

...

• u32 num_of_modes;
• struct list_head mode_list;
• struct edid *edid;
• struct drm_display_mode curr_mode;

This is not used anywhere

...

• struct sn65dsi86_video_cfg video_cfg;

+};

+struct sn65dsi86_reg_cfg cfg_proto_0_init[] = {

• {SN_REFCLK_FREQ_REG, 0x02, 0x0},
• {SN_DSI_LANES_REG, 0x26, 0x14},
• {SN_DSIA_CLK_FREQ_REG, 0x7B, 0x0},
• {SN_ENH_FRAME_REG, 0x05, 0x0},
• {SN_SSC_CONFIG_REG, 0x30, 0x0},
• {SN_DATARATE_CONFIG_REG, 0x80, 0x0},
• {SN_PLL_ENABLE_REG, 0x01, 0x0},
• {SN_SCRAMBLE_CONFIG_REG, 0x00, 0x0},
• {SN_AUX_WDATA0_REG, 0x01, 0x0},
• {SN_AUX_ADDR_19_16_REG, 0x00, 0x0},
• {SN_AUX_ADDR_15_8_REG, 0x01, 0x0},
• {SN_AUX_ADDR_7_0_REG, 0x0A, 0x0},
• {SN_AUX_LENGTH_REG, 0x01, 0x0},
• {SN_AUX_CMD_REG, 0x81, 0x14},
• {SN_ML_TX_MODE_REG, 0x0A, 0x14},
• {SN_PAGE_SELECT_REG, 0x14, 0x0},
• {SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, 0x70, 0x0},
• {SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, 0x08, 0x0},
• {SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, 0xA0, 0x0},
• {SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, 0x05, 0x0},
• {SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, 0x20, 0x0},
• {SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
• {SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, 0x0A, 0x0},
• {SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
• {SN_CHA_HORIZONTAL_BACK_PORCH_REG, 0x50, 0x0},
• {SN_CHA_VERTICAL_BACK_PORCH_REG, 0x1B, 0x0},
• {SN_CHA_HORIZONTAL_FRONT_PORCH_REG, 0x30, 0x0},
• {SN_CHA_VERTICAL_FRONT_PORCH_REG, 0x03, 0x0},
• {SN_DATA_FORMAT_REG, 0x00, 0x0},
• {SN_COLOR_BAR_CONFIG_REG, 0x00, 0x14},
• {SN_ENH_FRAME_REG, 0x0D, 0x0},

+};

This should be programmed using the display parameters set out by mode/panel, as opposed to just dumped in a table.

...

+static int sn65dsi86_write(struct sn65dsi86 *pdata, u8 reg, u8 val) +{

• struct i2c_client *client = pdata->i2c_client;
• u8 buf[2] = {reg, val};
• struct i2c_msg msg = {

• .addr = client->addr,
  

• .flags = 0,
  

• .len = 2,
  

• .buf = buf,
  

• };
• if (i2c_transfer(client->adapter, &msg, 1) < 1) {

• pr_err("i2c write failed\n");
  

• return -EIO;
  

• }
• return 0;

+}

+static int sn65dsi86_read(struct sn65dsi86 *pdata, u8 reg, char *buf, u32 size) +{

• struct i2c_client *client = pdata->i2c_client;
• struct i2c_msg msg[2] = {

• {
  

• 	.addr = client->addr,
  

• 	.flags = 0,
  

• 	.len = 1,
  

• 	.buf = &reg,
  

• },
  

• {
  

• 	.addr = client->addr,
  

• 	.flags = I2C_M_RD,
  

• 	.len = size,
  

• 	.buf = buf,
  

• }
  

• };
• if (i2c_transfer(client->adapter, msg, 2) != 2) {

• pr_err("i2c read failed\n");
  

• return -EIO;
  

• }
• return 0;

+}

+static int sn65dsi86_write_array(struct sn65dsi86 *pdata,

• struct sn65dsi86_reg_cfg *cfg, int size)

+{

• int ret = 0;
• int i;
• size = size / sizeof(struct sn65dsi86_reg_cfg);
• for (i = 0; i < size; i++) {

• ret = sn65dsi86_write(pdata, cfg[i].reg, cfg[i].val);
  

• if (ret != 0) {
  

• 	pr_err("reg writes failed. Last write %02X to %02X\n",
  

• 		cfg[i].val, cfg[i].reg);
  

• 	goto w_regs_fail;
  

• }
  

• if (cfg[i].sleep_in_ms)
  

• 	msleep(cfg[i].sleep_in_ms);
  

• }

+w_regs_fail:

• if (ret != 0)

• pr_err("exiting with ret = %d after %d writes\n", ret, i);
  

• return ret;

+}

+static void sn65dsi86_gpio_configure(struct sn65dsi86 *pdata, bool on) +{

• int value;
• value = on ? 1: 0;
• gpiod_set_value(pdata->gpios.enable_gpio, value);
• gpiod_set_value(pdata->gpios.aux_i2c_sda, value);
• gpiod_set_value(pdata->gpios.aux_i2c_scl, value);
• gpiod_set_value(pdata->gpios.edp_bias_en, value);
• gpiod_set_value(pdata->gpios.edp_bklt_en, value);
• gpiod_set_value(pdata->gpios.irq_gpio, value);
• gpiod_set_value(pdata->gpios.edp_bklt_ctrl, value);
• pr_debug("config to: %d\n", value);

+}

+static void sn65dsi86_power_ctrl(struct sn65dsi86 *pdata, bool enable) +{

• if (!pdata)

• return;
  

• if (!pdata->power_on && enable) {

• sn65dsi86_gpio_configure(pdata, true);
  

• if (regulator_bulk_enable(pdata->num_supplies,
  

• 				pdata->supplies)) {
  

• 	pr_err("bridge regulator enable failed\n");
  

• 	return;
  

• }
  

• pdata->power_on = true;
  

• } else if (pdata->power_on && !enable) {

• regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
  

• sn65dsi86_gpio_configure(pdata, false);
  

• pdata->power_on = false;
  

• } else {

• pr_debug("unnecessary call to power control\n");
  

• }

+}

+/* Connector funcs */ +static struct sn65dsi86 *connector_to_sn65dsi86(struct drm_connector *connector) +{

• return container_of(connector, struct sn65dsi86, connector);

+}

+static int sn65dsi86_send_aux_cmd(struct sn65dsi86 *pdata,

• 		  u8 cmd, u8 addr, u8 length, int w_data)
  

+{

• u8 read = 0;
• int retry_cnt = 10;
• sn65dsi86_write(pdata, SN_AUX_CMD_REG, (cmd << 4));
• sn65dsi86_write(pdata, SN_AUX_ADDR_7_0_REG, addr);
• sn65dsi86_write(pdata, SN_AUX_LENGTH_REG, length);
• if (w_data >= 0)

• sn65dsi86_write(pdata, SN_AUX_WDATA0_REG, (u8)w_data);
  

• /* set SEND bit */
• sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 1);
• read |= BIT(0);
• sn65dsi86_write(pdata, SN_AUX_CMD_REG, read);
• /* poll for bridge to ack SEND bit */
• while (retry_cnt) {

• sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 0x1);
  

• if (!(read & BIT(0)))
  

• 	break;
  

• retry_cnt--;
  

• udelay(1000);
  

• }
• if (!retry_cnt) {

• pr_err("aux_cmd transfer failed\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static int sn65dsi86_read_edid(struct sn65dsi86 *pdata, u8 *buf) +{

• int i = 0;
• u8 addr = SN_AUX_RDATA0_REG;
• u8 *data = buf;
• if (!data)

• return -ENOMEM;
  

• if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x0, -1) ||

• sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x01, 0x0) ||
  

• sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x0, -1) ||
  

• sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x10, -1))
  

• goto error;
  

Instead of hand-crafting this, you should implement an i2c_adapter. You can then use that in drm_get_edid() to read the edid via the core.

See the comment about drm_do_get_edid():

• drivers must make all reasonable efforts to expose it as an I2C
• adapter and use drm_get_edid() instead of abusing this function.

...

• for (i = 0; i < 16; i++) {

• if (sn65dsi86_read(pdata, addr, data, 0x1))
  

• 	goto error;
  

• addr++;
  

• data++;
  

• }
• return 0;

+error:

• pr_err("edid read over i2c failed\n");
• return -EINVAL;

+}

+static int sn65dsi86_read_edid_block(struct sn65dsi86 *pdata,

• 	       u8 *buf, unsigned int block)
  

+{

• if (block == 0) {

• if (sn65dsi86_read_edid(pdata, buf))
  

• 	goto error;
  

• } else if (block == 1) {

• /* move segment pointer */
  

• if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x0, -1) ||
  

• 	sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x01, 0x1) ||
  

• 	sn65dsi86_send_aux_cmd(pdata, 0x0, 0x30, 0x00, -1))
  

• 	goto error;
  

• else
  

• 	if (sn65dsi86_read_edid(pdata, buf))
  

• 		goto error;
  

• } else {

• pr_debug("unsupported edid block\n");
  

• goto error;
  

• }
• return 0;

+error:

• pr_err("edid block read failed\n");
• return -EINVAL;

+}

+static int sn65dsi86_get_edid_block(void *data, u8 *buf, unsigned int block,

• 		  size_t len)
  

+{

• struct sn65dsi86 *pdata = data;
• int ret = 0;
• pr_debug("get edid block: block=%d, len=%d\n", block, (int)len);
• if (len > 128 || block > 1)

• return -EINVAL;
  

• ret = sn65dsi86_read_edid_block(pdata, buf, block);
• if (ret) {

• pr_err("edid read failed for block: %d ret: %d\n", block, ret);
  

• return ret;
  

• }
• return 0;

+}

+static int sn65dsi86_connector_get_modes(struct drm_connector *connector) +{

• struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
• struct drm_display_mode *mode, *m;
• if (pdata->edid)

• return drm_add_edid_modes(connector, pdata->edid);
  

What if the edid changes? You'll never update it.

...

• sn65dsi86_power_ctrl(pdata, true);
• if (pdata->is_pluggable) {

Here, call the panel func if a panel is connected, or probe the edid.

...

• pdata->edid = drm_do_get_edid(connector,
  

• 		sn65dsi86_get_edid_block, pdata);
  

Are you leaking memory here?

...

• drm_mode_connector_update_edid_property(connector, pdata->edid);
  

• pdata->num_of_modes = drm_add_edid_modes(connector,
  

• 						pdata->edid);
  

You should be able to remove num_of_modes and mode_list once you remove the dt custom modes. Make sure you incorporate a call to drm_panel_get_modes() here.

...

• }
• if (!pdata->is_pluggable || !pdata->num_of_modes) {

• /*
  

•  * if device does not support HPD or due to some reason
  

•  * EDID read failed then fall back to mode_list which is
  

•  * already parsed from dt if any.
  

•  */
  

• list_for_each_entry(mode, &pdata->mode_list, head) {
  

• 	m = drm_mode_duplicate(connector->dev, mode);
  

• 	if (!m) {
  

• 		pr_err("failed to get mode %dx%d\n",
  

• 			mode->hdisplay, mode->vdisplay);
  

• 		break;
  

• 	}
  

• 	drm_mode_probed_add(connector, m);
  

• }
  

• }
• sn65dsi86_power_ctrl(pdata, false);

What if power was already on?

...

• return pdata->num_of_modes;

+}

+static enum drm_mode_status +sn65dsi86_connector_mode_valid(struct drm_connector *connector,

• 	     struct drm_display_mode *mode)
  

+{

• struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
• struct drm_display_mode *m;
• if (pdata->edid)

• return MODE_OK;
  

Careful here, userspace could add an invalid mode and this would say it's correct. Instead of doing this, and the check below, just check the given mode against the max hardware limitations listed in the datasheet (4k60). I assume this will also depend on how many dsi channels we have coming in.

...

• if (!pdata->is_pluggable) {

• list_for_each_entry(m, &pdata->mode_list, head) {
  

• 	if (m->hdisplay == mode->hdisplay &&
  

• 		m->vdisplay == mode->vdisplay)
  

• 		return MODE_OK;
  

• }
  

• }
• return MODE_BAD;

+}

+static struct drm_connector_helper_funcs sn65dsi86_connector_helper_funcs = {

• .get_modes = sn65dsi86_connector_get_modes,
• .mode_valid = sn65dsi86_connector_mode_valid,

+};

+static enum drm_connector_status +sn65dsi86_connector_detect(struct drm_connector *connector, bool force) +{

• struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
• pdata->connector_status = pdata->power_on ?

• connector_status_connected : connector_status_disconnected;
  

It's possible detect() will be called while the device is off, in that case this will return the incorrect value. Is there some reason you can't just detect whether there's something connected?

...

• return pdata->connector_status;

+}

+static const struct drm_connector_funcs sn65dsi86_connector_funcs = {

• .fill_modes = drm_helper_probe_single_connector_modes,
• .detect = sn65dsi86_connector_detect,
• .destroy = 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 struct sn65dsi86 *bridge_to_sn65dsi86(struct drm_bridge *bridge) +{

• return container_of(bridge, struct sn65dsi86, bridge);

+}

+static int sn65dsi86_read_device_rev(struct sn65dsi86 *pdata) +{

• u8 rev = 0;
• int ret = 0;
• ret = sn65dsi86_read(pdata, SN_DEVICE_REV_REG, &rev, 1);

extra line

...

• if (!ret) {

In this case, flip the condition so you can avoid indenting everything:

    if (ret)
            return ret;

...

• if (rev == 0x2) {
  

What's 0x2? Can you please #define this?

...

• 	pr_info("SN65DSI86 revision id: 0x%x\n", rev);
  

While I personally appreciate info messages, others not so much. So downgrade this to DRM_DEBUG_KMS()

...

• } else {
  

• 	pr_warn("SN65DSI86 revision id mismatch\n");
  

• 	ret = -EINVAL;
  

• }
  

• }
• return ret;

+}

+static irqreturn_t sn65dsi86_irq_thread_handler(int irq, void *dev_id) +{

• return IRQ_HANDLED;

+}

What's the point of even registering this? Just leave it all out if it's unused. If there's a need in the future, it can easily be added back in.

...

+static const char * const sn65dsi86_supply_names[] = {

• "vccio",
• "vcca",
• "vccs"

+};

+static int sn65dsi86_init_regulators(struct sn65dsi86 *pdata) +{

• const char * const *supply_names;
• unsigned int i;
• int ret;
• supply_names = sn65dsi86_supply_names;

This local isn't necessary.

...

• pdata->num_supplies = ARRAY_SIZE(sn65dsi86_supply_names);
• pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,

• 		     sizeof(*pdata->supplies), GFP_KERNEL);
  

• if (!pdata->supplies)

• return -ENOMEM;
  

• for (i = 0; i < pdata->num_supplies; i++)

• pdata->supplies[i].supply = supply_names[i];
  

• ret = devm_regulator_bulk_get(pdata->dev,

• 	pdata->num_supplies, pdata->supplies);
  

• if (ret)

• return ret;
  

• return regulator_bulk_enable(pdata->num_supplies, pdata->supplies);

+}

+static int sn65dsi86_bridge_attach(struct drm_bridge *bridge) +{

• struct mipi_dsi_host *host;
• struct mipi_dsi_device *dsi;
• struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
• int ret;
• const struct mipi_dsi_device_info info = { .type = "sn65dsi86",

• 				   .channel = 0,
  

• 				   .node = NULL,
  

• 				 };
  

• if (!bridge->encoder) {

• DRM_ERROR("Parent encoder object not found");
  

• return -ENODEV;
  

• }
• /* HPD not supported */
• pdata->connector.polled = 0;

You'll need to refactor the below to accommodate panels. If you're not planning on supporting hotplug, you should probably remove all of the connector-related stuff from this driver, since you will always be using a panel driver.

Thanks for reviewing the patch in detail.

I have one doubt here. If we remove connector from bridge driver, then how will detect() and get_modes() called. If you are suggesting to use panel func's detect() and get_mode() then it might not work, because once upstream DSI driver sees an external bridge is connected to DSI, then it does not create a connector of it own, it expects the external bridge to create the connector node. I think here the external bridge has to create the connector and when detect() and get_modes() call come to external bridge then it should query connected panel's detect() and get_modes() API.

...

• ret = drm_connector_init(bridge->dev, &pdata->connector,

• 		 &sn65dsi86_connector_funcs,
  

• 		 DRM_MODE_CONNECTOR_eDP);
  

• if (ret) {

• DRM_ERROR("Failed to initialize connector with drm\n");
  

• return ret;
  

• }
• drm_connector_helper_add(&pdata->connector,

• 		 &sn65dsi86_connector_helper_funcs);
  

• drm_mode_connector_attach_encoder(&pdata->connector,

bridge->encoder);

• host = of_find_mipi_dsi_host_by_node(pdata->host_node);
• if (!host) {

• pr_err("failed to find dsi host\n");
  

• return -ENODEV;
  

• }
• dsi = mipi_dsi_device_register_full(host, &info);
• if (IS_ERR(dsi)) {

• pr_err("failed to create dsi device\n");
  

• ret = PTR_ERR(dsi);
  

• goto err_dsi_device;
  

• }
• /* setting to 4 lanes always for now */
• dsi->lanes = 4;

Perhaps add a TODO for this?

...

• dsi->format = MIPI_DSI_FMT_RGB888;
• dsi->mode_flags = MIPI_DSI_MODE_VIDEO |

MIPI_DSI_MODE_VIDEO_SYNC_PULSE |

• 	  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
  

• ret = mipi_dsi_attach(dsi);
• if (ret < 0) {

• pr_err("failed to attach dsi to host\n");
  

• goto err_dsi_attach;
  

• }
• pdata->dsi = dsi;
• pr_debug("bridge attached\n");
• return 0;

+err_dsi_attach:

• mipi_dsi_device_unregister(dsi);

+err_dsi_device:

• return ret;

+}

+static void sn65dsi86_set_video_cfg(struct sn65dsi86 *pdata,

• struct drm_display_mode *mode,
• struct sn65dsi86_video_cfg *video_cfg)

+{

• video_cfg->h_active = mode->hdisplay;
• video_cfg->v_active = mode->vdisplay;
• video_cfg->h_front_porch = mode->hsync_start - mode->hdisplay;
• video_cfg->v_front_porch = mode->vsync_start - mode->vdisplay;
• video_cfg->h_back_porch = mode->htotal - mode->hsync_end;
• video_cfg->v_back_porch = mode->vtotal - mode->vsync_end;
• video_cfg->h_pulse_width = mode->hsync_end - mode->hsync_start;
• video_cfg->v_pulse_width = mode->vsync_end - mode->vsync_start;
• video_cfg->pclk_khz = mode->clock;
• video_cfg->h_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
• video_cfg->v_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
• /* setting to 4 lanes always for now */
• video_cfg->num_of_lanes = 4;
• pr_debug("video=h[%d,%d,%d,%d] v[%d,%d,%d,%d] pclk=%d lane=%d\n",

• video_cfg->h_active, video_cfg->h_front_porch,
  

• video_cfg->h_pulse_width, video_cfg->h_back_porch,
  

• video_cfg->v_active, video_cfg->v_front_porch,
  

• video_cfg->v_pulse_width, video_cfg->v_back_porch,
  

• video_cfg->pclk_khz, video_cfg->num_of_lanes);
  

+}

As mentioned above, this needs to be removed.

...

+static void sn65dsi86_bridge_mode_set(struct drm_bridge *bridge,

• 		    struct drm_display_mode *mode,
  

• 		    struct drm_display_mode *adj_mode)
  

+{

• struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
• struct sn65dsi86_video_cfg *video_cfg = &pdata->video_cfg;
• int ret = 0;
• pr_debug("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d,

clock=%d\n",

• adj_mode->hdisplay, adj_mode->vdisplay,
  

• adj_mode->vrefresh, adj_mode->clock);
  

• drm_mode_copy(&pdata->curr_mode, adj_mode);
• memset(video_cfg, 0, sizeof(struct sn65dsi86_video_cfg));
• sn65dsi86_set_video_cfg(pdata, adj_mode, video_cfg);
• if (video_cfg->num_of_lanes != pdata->dsi->lanes) {

• mipi_dsi_detach(pdata->dsi);
  

• pdata->dsi->lanes = video_cfg->num_of_lanes;
  

• ret = mipi_dsi_attach(pdata->dsi);
  

Hmm, if this is configurable is there no better way of doing this than detaching and attaching?

...

• if (ret)
  

• 	pr_err("failed to change host lanes\n");
  

• }

+}

+static void sn65dsi86_bridge_disable(struct drm_bridge *bridge) +{

• struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
• sn65dsi86_power_ctrl(pdata, false);
• pdata->connector_status = connector_status_disconnected;

+}

+static void sn65dsi86_bridge_enable(struct drm_bridge *bridge) +{

• struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
• sn65dsi86_power_ctrl(pdata, true);
• pdata->connector_status = connector_status_connected;
• sn65dsi86_write_array(pdata, cfg_proto_0_init,

• 	      sizeof(cfg_proto_0_init));
  

As mentioned above, most these should likely be set in mode_set() using the values set in the current mode.

Sean

...

+}

+static const struct drm_bridge_funcs sn65dsi86_bridge_funcs = {

• .attach = sn65dsi86_bridge_attach,
• .enable = sn65dsi86_bridge_enable,
• .disable = sn65dsi86_bridge_disable,
• .mode_set = sn65dsi86_bridge_mode_set,

+};

+static int sn65dsi86_parse_dt_modes(struct device_node *np,

• 			struct list_head *head,
  

• 			u32 *num_of_modes)
  

+{

• int rc = 0;
• struct drm_display_mode *mode;
• u32 mode_count = 0;
• struct device_node *node = NULL;
• struct device_node *root_node = NULL;
• u32 h_front_porch, h_pulse_width, h_back_porch;
• u32 v_front_porch, v_pulse_width, v_back_porch;
• bool h_active_high, v_active_high;
• u32 flags = 0;
• root_node = of_get_child_by_name(np, "sn,custom-modes");
• if (!root_node) {

• root_node = of_parse_phandle(np, "sn,custom-modes", 0);
  

• if (!root_node) {
  

• 	pr_info("No modes present for sn,custom-modes");
  

• 	goto end;
  

• }
  

• }
• for_each_child_of_node(root_node, node) {

• rc = 0;
  

• mode = kzalloc(sizeof(*mode), GFP_KERNEL);
  

• if (!mode) {
  

• 	rc =  -ENOMEM;
  

• 	goto end;
  

• }
  

• of_property_read_u32(node, "sn,mode-h-active",
  

• 				&mode->hdisplay);
  

• of_property_read_u32(node, "sn,mode-h-front-porch",
  

• 				&h_front_porch);
  

• of_property_read_u32(node, "sn,mode-h-pulse-width",
  

• 				&h_pulse_width);
  

• of_property_read_u32(node, "sn,mode-h-back-porch",
  

• 				&h_back_porch);
  

• h_active_high = of_property_read_bool(node,
  

• 				"sn,mode-h-active-high");
  

• of_property_read_u32(node, "sn,mode-v-active",
  

• 				&mode->vdisplay);
  

• of_property_read_u32(node, "sn,mode-v-front-porch",
  

• 				&v_front_porch);
  

• of_property_read_u32(node, "sn,mode-v-pulse-width",
  

• 				&v_pulse_width);
  

• of_property_read_u32(node, "sn,mode-v-back-porch",
  

• 				&v_back_porch);
  

• v_active_high = of_property_read_bool(node,
  

• 				"sn,mode-v-active-high");
  

• of_property_read_u32(node, "sn,mode-refresh-rate",
  

• 				&mode->vrefresh);
  

• of_property_read_u32(node, "sn,mode-clock-in-khz",
  

• 				&mode->clock);
  

• mode->hsync_start = mode->hdisplay + h_front_porch;
  

• mode->hsync_end = mode->hsync_start + h_pulse_width;
  

• mode->htotal = mode->hsync_end + h_back_porch;
  

• mode->vsync_start = mode->vdisplay + v_front_porch;
  

• mode->vsync_end = mode->vsync_start + v_pulse_width;
  

• mode->vtotal = mode->vsync_end + v_back_porch;
  

• if (!mode->htotal || !mode->vtotal) {
  

• 	rc = -EINVAL;
  

• 	goto fail;
  

• }
  

• if (h_active_high)
  

• 	flags |= DRM_MODE_FLAG_PHSYNC;
  

• else
  

• 	flags |= DRM_MODE_FLAG_NHSYNC;
  

• if (v_active_high)
  

• 	flags |= DRM_MODE_FLAG_PVSYNC;
  

• else
  

• 	flags |= DRM_MODE_FLAG_NVSYNC;
  

• mode->flags = flags;
  

• if (!rc) {
  

• 	mode_count++;
  

• 	list_add_tail(&mode->head, head);
  

• }
  

• drm_mode_set_name(mode);
  

• pr_debug("mode[%s] h[%d,%d,%d,%d] v[%d,%d,%d,%d] %d %x %dkHZ\n",
  

• 	mode->name, mode->hdisplay, mode->hsync_start,
  

• 	mode->hsync_end, mode->htotal, mode->vdisplay,
  

• 	mode->vsync_start, mode->vsync_end, mode->vtotal,
  

• 	mode->vrefresh, mode->flags, mode->clock);
  

+fail:

• if (rc) {
  

• 	kfree(mode);
  

• 	continue;
  

• }
  

• }
• if (num_of_modes)

• *num_of_modes = mode_count;
  

+end:

• return rc;

+}

+static int sn65dsi86_parse_gpios(struct device_node *np,

• 			struct sn65dsi86 *pdata)
  

+{

• int ret = 0;
• if (!pdata || !pdata->dev)

• return -EINVAL;
  

• pdata->gpios.enable_gpio = devm_gpiod_get(pdata->dev, "enable",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.enable_gpio)) {

• pr_err("failed to get enable gpio from DT\n");
  

• ret = PTR_ERR(pdata->gpios.enable_gpio);
  

• goto exit;
  

• }
• pdata->gpios.aux_i2c_scl = devm_gpiod_get(pdata->dev, "aux-scl",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.aux_i2c_scl)) {

• pr_err("failed to get aux scl gpio from DT\n");
  

• ret = PTR_ERR(pdata->gpios.aux_i2c_scl);
  

• goto exit;
  

• }
• pdata->gpios.aux_i2c_sda = devm_gpiod_get(pdata->dev, "aux-sda",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.aux_i2c_sda)) {

• pr_err("failed to get aux sda gpio from DT\n");
  

• ret = PTR_ERR(pdata->gpios.aux_i2c_sda);
  

• goto exit;
  

• }
• pdata->gpios.edp_bias_en = devm_gpiod_get(pdata->dev, "bias-en",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.edp_bias_en)) {

• pr_err("failed to get bias en gpio from DT\n");
  

• ret = PTR_ERR(pdata->gpios.edp_bias_en);
  

• goto exit;
  

• }
• pdata->gpios.edp_bklt_en = devm_gpiod_get(pdata->dev, "bklt-en",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.edp_bklt_en)) {

• pr_err("failed to get bklt en gpio from DT\n");
  

• ret = PTR_ERR(pdata->gpios.edp_bklt_en);
  

• goto exit;
  

• }
• pdata->gpios.irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",

• 				  GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.irq_gpio))

• pr_err("failed to get irq gpio from DT\n");
  

• pdata->gpios.edp_bklt_ctrl = devm_gpiod_get_optional(pdata->dev,

• 			"bklt-ctrl", GPIOD_OUT_HIGH);
  

• if (IS_ERR(pdata->gpios.edp_bklt_ctrl))

• pr_err("failed to get bklt ctrl gpio from DT\n");
  

+exit:

• return ret;

+}

+static int sn65dsi86_parse_dt(struct device *dev, struct sn65dsi86 *pdata) +{

• struct device_node *np = dev->of_node;
• struct device_node *end_node;
• int ret = 0;
• end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
• if (!end_node) {

• pr_err("remote endpoint not found\n");
  

• return -ENODEV;
  

• }
• pdata->host_node = of_graph_get_remote_port_parent(end_node);
• of_node_put(end_node);
• if (!pdata->host_node) {

• pr_err("remote node not found\n");
  

• return -ENODEV;
  

• }
• of_node_put(pdata->host_node);
• ret = sn65dsi86_parse_gpios(np, pdata);
• pdata->is_pluggable = of_property_read_bool(np, "sn,is-pluggable");
• pr_debug("is_pluggable = %d\n", pdata->is_pluggable);
• if (!pdata->is_pluggable) {

• INIT_LIST_HEAD(&pdata->mode_list);
  

• sn65dsi86_parse_dt_modes(np,
  

• 	&pdata->mode_list, &pdata->num_of_modes);
  

• }
• return ret;

+}

+static int sn65dsi86_probe(struct i2c_client *client,

• const struct i2c_device_id *id)
  

+{

• struct sn65dsi86 *pdata;
• int ret = 0;
• struct drm_display_mode *mode, *n;
• if (!client || !client->dev.of_node) {

• pr_err("invalid input\n");
  

• return -EINVAL;
  

• }
• if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {

• pr_err("device doesn't support I2C\n");
  

• return -ENODEV;
  

• }
• pdata = devm_kzalloc(&client->dev,

• sizeof(struct sn65dsi86), GFP_KERNEL);
  

• if (!pdata)

• return -ENOMEM;
  

• pdata->power_on = false;
• pdata->is_pluggable = false;
• pdata->connector_status = connector_status_disconnected;
• pdata->dev = &client->dev;
• pdata->i2c_client = client;
• pr_debug("I2C address is %x\n", client->addr);
• ret = sn65dsi86_parse_dt(&client->dev, pdata);
• if (ret) {

• pr_err("failed to parse device tree\n");
  

• goto err_dt_parse;
  

• }
• sn65dsi86_gpio_configure(pdata, true);
• ret = sn65dsi86_init_regulators(pdata);
• if (ret) {

• pr_err("failed to enable regulators\n");
  

• goto err_gpio_config;
  

• }
• ret = sn65dsi86_read_device_rev(pdata);
• if (ret) {

• pr_err("failed to read chip rev\n");
  

• goto err_gpio_config;
  

• } else {

• pr_debug("bridge chip enabled successfully\n");
  

• pdata->power_on = true;
  

• }
• pdata->irq = gpiod_to_irq(pdata->gpios.irq_gpio);
• ret = request_threaded_irq(pdata->irq, NULL,

• 	sn65dsi86_irq_thread_handler,
  

• 	IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
  

• 	"sn65dsi86", pdata);
  

• i2c_set_clientdata(client, pdata);
• dev_set_drvdata(&client->dev, pdata);
• pdata->bridge.funcs = &sn65dsi86_bridge_funcs;
• pdata->bridge.of_node = client->dev.of_node;
• drm_bridge_add(&pdata->bridge);
• return ret;

+err_gpio_config:

• sn65dsi86_gpio_configure(pdata, false);

+err_dt_parse:

• if (!pdata->is_pluggable) {

• list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
  

• 	list_del(&mode->head);
  

• 	kfree(mode);
  

• }
  

• pdata->num_of_modes = 0;
  

• }
• devm_kfree(&client->dev, pdata);
• return ret;

+}

+static int sn65dsi86_remove(struct i2c_client *client) +{

• int ret = -EINVAL;
• struct sn65dsi86 *pdata = i2c_get_clientdata(client);
• struct drm_display_mode *mode, *n;
• if (!pdata)

• goto end;
  

• mipi_dsi_detach(pdata->dsi);
• mipi_dsi_device_unregister(pdata->dsi);
• drm_bridge_remove(&pdata->bridge);
• disable_irq(pdata->irq);
• free_irq(pdata->irq, pdata);
• sn65dsi86_gpio_configure(pdata, false);
• if (!pdata->is_pluggable) {

• list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
  

• 	list_del(&mode->head);
  

• 	kfree(mode);
  

• }
  

• }
• devm_kfree(&client->dev, pdata);

+end:

• return ret;

+}

+static struct i2c_device_id sn65dsi86_id[] = {

• { "ti,sn65dsi86", 0},
• {}

+}; +MODULE_DEVICE_TABLE(i2c, sn65dsi86_id);

+static const struct of_device_id sn65dsi86_match_table[] = {

• {.compatible = "ti,sn65dsi86"},
• {}

+}; +MODULE_DEVICE_TABLE(of, sn65dsi86_match_table);

+static struct i2c_driver sn65dsi86_driver = {

• .driver = {

• .name = "sn65dsi86",
  

• .owner = THIS_MODULE,
  

• .of_match_table = sn65dsi86_match_table,
  

• },
• .probe = sn65dsi86_probe,
• .remove = sn65dsi86_remove,
• .id_table = sn65dsi86_id,

+};

+module_i2c_driver(sn65dsi86_driver);

+MODULE_DESCRIPTION("SN65DSI86 DSI to eDP bridge driver");

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Fri, Apr 13, 2018 at 10:53:01AM +0530, Sandeep Panda wrote:

Document the bindings used for the sn65dsi86 DSI to eDP bridge.

Signed-off-by: Sandeep Panda spanda@codeaurora.org

.../bindings/display/bridge/ti,sn65dsi86.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file mode 100644 index 0000000..cbd2f0e --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt @@ -0,0 +1,75 @@ +SN65DSI86 DSI to eDP bridge chip +--------------------------------

+This is the binding for Texas Instruments SN65DSI86 bridge.

+Required properties: +- compatible: Must be "ti,sn65dsi86" +- reg: i2c address of the chip, 0x2d as per datasheet +- enable-gpios: OF device-tree gpio specification for EDP_BRIJ_EN pin +- aux-sda-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SDA pin of AUX channel +- aux-scl-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SCL pin of AUX channel

What if I use I2C host on my device? If your board design uses bitbanged I2C, that is outside the scope of this binding and it should use i2c-gpio binding.

+- bias-en-gpios: OF device-tree gpio specification for EN_PP3300_DX_EDP pin to

• 	enable 3.3V supply to eDP connector
  

+- bklt-en-gpios: OF device-tree gpio specification for AP_EDP_BKLTEN

pin

+- vccio-supply: A 1.8V supply that powers up the PHY. +- vcca-supply: A 1.2V supply that powers up the Controller. +- vccs-supply: A 3.3V supply that power the eDP connector

+Optional properties:

+- irq-gpios: OF device-tree gpio specification for interrupt pin

Use "interrupts" property instead.

+- bklt-ctrl-gpios: OF device-tree gpio specification for EDP_BKLTCTL pin

+- sn,is-pluggable: boolean property to specify if HPD supported or not +- sn,custom-modes: OF device-tree specifiction to add support for custom modes

"sn" is not a vendor.

+Required nodes:

+This device has two video ports. Their connections are modelled using the OF +graph bindings specified in Documentation/devicetree/bindings/graph.txt.

+- Video port 0 for DSI input +- Video port 1 for eDP output

+Example +-------

+edp-bridge@2d {

• compatible = "ti,sn65dsi86";
• #address-cells = <1>;
• #size-cells = <0>;
• reg = <0x2d>;
• enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
• aux-sda-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
• aux-scl-gpios = <&msmgpio 35 GPIO_ACTIVE_HIGH>;
• bias-en-gpios = <&msmgpio 36 GPIO_ACTIVE_HIGH>;
• bklt-en-gpios = <&msmgpio 37 GPIO_ACTIVE_HIGH>;
• vccio-supply = <&pm8916_l17>;
• vcca-supply = <&pm8916_l6>;
• vccs-supply = <&pm8916_l7>;
• ports {

• #address-cells = <1>;
  

• #size-cells = <0>;
  

• port@0 {
  

• 	reg = <0>;
  

• 	edp_bridge_in: endpoint {
  

• 		remote-endpoint = <&dsi_out>;
  

• 	};
  

• };
  

• port@1 {
  

• 	reg = <1>;
  

• 	edp_bridge_out: endpoint {
  

• 		remote-endpoint = <&edp_panel_in>;
  

• 	};
  

• };
  

• };

+}

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html