Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 18, 2018 at 05:49:59PM +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@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 ++++++++++++++++++++++++++++++++++
>  1 file changed, 742 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..c798f2f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,742 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

There should be a copyright here.

> +
> +#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/of_gpio.h>
> +#include <linux/of_graph.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>
> +#include <drm/drm_panel.h>
> +
> +#define SN_BRIDGE_REVISION_ID 0x2
> +
> +/* Link Training specific registers */
> +#define SN_DEVICE_REV_REG			0x08
> +#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
> +
> +#define DPPLL_CLK_SRC_REFCLK	0
> +#define DPPLL_CLK_SRC_DSICLK	1
> +#define MIN_DSI_CLK_FREQ_MHZ	40
> +
> +enum ti_sn_bridge_ref_clks {
> +	SN_REF_CLK_12_MHZ = 0,
> +	SN_REF_CLK_19_2_MHZ,
> +	SN_REF_CLK_26_MHZ,
> +	SN_REF_CLK_27_MHZ,
> +	SN_REF_CLK_38_4_MHZ,
> +	SN_REF_CLK_MAX,
> +};
> +
> +struct ti_sn_bridge {
> +	struct device *dev;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +	struct device_node *host_node;
> +	struct mipi_dsi_device *dsi;
> +	/* handle to the connected eDP panel */
> +	struct drm_panel *panel;
> +	int irq;
> +	struct gpio_desc *irq_gpio;
> +	/* list of gpios needed to enable the bridge functionality */
> +	struct gpio_descs *enable_gpios;
> +	unsigned int num_supplies;
> +	struct regulator_bulk_data *supplies;
> +	struct i2c_client *i2c_client;
> +	struct i2c_adapter *ddc;
> +	bool power_on;
> +	u32 num_modes;
> +	struct drm_display_mode curr_mode;
> +};
> +
> +static int ti_sn_bridge_write(struct ti_sn_bridge *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) {
> +		DRM_ERROR("i2c write failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ti_sn_bridge_read(struct ti_sn_bridge *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) {
> +		DRM_ERROR("i2c read failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata, bool on)
> +{
> +	int value, i = 0, num = 0;
> +	int *value_array = NULL;
> +
> +	if (!pdata)
> +		return;

pdata should always be valid here.

> +	value = on ? 1 : 0;
> +	num = pdata->enable_gpios->ndescs;
> +	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
> +	if (value_array) {
> +		for (i = 0; i < num; i++)
> +			value_array[i] = value;
> +		gpiod_set_array_value(num, pdata->enable_gpios->desc,
> +				      value_array);
> +		kfree(value_array);
> +	}
> +
> +	if (pdata->irq_gpio)
> +		gpiod_set_value(pdata->irq_gpio, value);
> +
> +	DRM_DEBUG("config to: %d\n", value);
> +}
> +
> +static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
> +{
> +	static int ref_count;
> +
> +	if (!pdata)
> +		return;

pdata should always be valid here.

> +	if (enable)
> +		ref_count++;
> +	else
> +		ref_count--;
> +
> +	if (enable && (ref_count == 1)) {
> +		ti_sn_bridge_gpio_configure(pdata, true);
> +
> +		if (regulator_bulk_enable(pdata->num_supplies,
> +						pdata->supplies)) {
> +			DRM_ERROR("bridge regulator enable failed\n");
> +			return;
> +		}
> +		pdata->power_on = true;
> +	} else if (!enable && !ref_count) {
> +		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
> +
> +		ti_sn_bridge_gpio_configure(pdata, false);
> +		pdata->power_on = false;
> +	} else {
> +		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
> +							enable, ref_count);
> +	}
> +}
> +
> +/* Connector funcs */
> +static struct ti_sn_bridge *
> +connector_to_ti_sn_bridge(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct ti_sn_bridge, connector);
> +}
> +
> +static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> +	struct drm_panel *panel = pdata->panel;
> +	struct edid *edid;
> +
> +	if (panel) {
> +		DRM_DEBUG("get mode from connected drm_panel\n");
> +		pdata->num_modes = drm_panel_get_modes(panel);
> +	} else {
> +		/* get from EDID */
> +		if (!pdata->ddc)
> +			return 0;
> +		ti_sn_bridge_power_ctrl(pdata, true);
> +		edid = drm_get_edid(connector, pdata->ddc);
> +		if (edid) {
> +			drm_mode_connector_update_edid_property(connector,
> +								edid);
> +			pdata->num_modes = drm_add_edid_modes(connector, edid);
> +			drm_edid_to_eld(connector, edid);
> +			kfree(edid);
> +		} else {
> +			DRM_DEBUG("failed to get edid\n");
> +		}
> +		ti_sn_bridge_power_ctrl(pdata, false);
> +	}
> +
> +	return pdata->num_modes;
> +}
> +
> +static enum drm_mode_status
> +ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
> +			     struct drm_display_mode *mode)
> +{
> +	/* maximum supported resolution is 4K at 60 fps */
> +	if (mode->clock > 594000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
> +	.get_modes = ti_sn_bridge_connector_get_modes,
> +	.mode_valid = ti_sn_bridge_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> +
> +	/**
> +	 * TODO: Currently if drm_panel is present, then always
> +	 * return the status as connected. Need to add support to detect
> +	 * device state for no panel(hot pluggable) scenarios.
> +	 */
> +	if (pdata->panel)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
> +}
> +
> +static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = ti_sn_bridge_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 ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct ti_sn_bridge, bridge);
> +}
> +
> +static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
> +{
> +	u8 rev = 0;
> +	int ret = 0;
> +
> +	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
> +	if (ret)
> +		goto exit;
> +
> +	if (rev == SN_BRIDGE_REVISION_ID) {
> +		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
> +	} else {
> +		DRM_ERROR("ti_sn_bridge revision id mismatch\n");

I find it useful to print what the mismatch is so you can debug more
effectively.

> +		ret = -EINVAL;
> +	}
> +
> +exit:
> +	return ret;
> +}
> +
> +static const char * const ti_sn_bridge_supply_names[] = {
> +	"vccio",
> +	"vcca",
> +};
> +
> +static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_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 = ti_sn_bridge_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(pdata->dev,
> +			pdata->num_supplies, pdata->supplies);
> +
> +	return ret;

Nit: you can combine the above two lines and get rid of 'ret'.

> +}
> +
> +static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
> +{
> +	struct device_node *panel_node, *port, *endpoint;
> +	struct drm_panel *panel = NULL;
> +
> +	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
> +	if (port) {
> +		endpoint = of_get_child_by_name(port, "endpoint");
> +		of_node_put(port);
> +		if (!endpoint) {
> +			dev_err(pdata->dev, "no output endpoint found\n");
> +			goto error;
> +		}
> +
> +		panel_node = of_graph_get_remote_port_parent(endpoint);
> +		of_node_put(endpoint);
> +		if (!panel_node) {
> +			dev_err(pdata->dev, "no output node found\n");
> +			goto error;
> +		}
> +
> +		panel = of_drm_find_panel(panel_node);
> +		of_node_put(panel_node);
> +		if (!panel) {
> +			dev_err(pdata->dev, "no panel node found\n");
> +			goto error;
> +		}
> +	}
> +
> +	pdata->panel = panel;
> +	drm_panel_attach(panel, &pdata->connector);
> +
> +	return;
> +
> +error:
> +	pdata->panel = NULL;

Is this needed?  isn't pdata->panel already null when we come in?  If so
you can just return the error cases and remove the label.

> +}
> +
> +static int ti_sn_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct mipi_dsi_host *host;
> +	struct mipi_dsi_device *dsi;
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	int ret;
> +	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");

Missing \n on the end of the string.

> +		return -ENODEV;
> +	}
> +
> +	/* HPD not supported */
> +	pdata->connector.polled = 0;
> +
> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
> +				 &ti_sn_bridge_connector_funcs,
> +				 DRM_MODE_CONNECTOR_eDP);

The actual #define is CONNNECTOR_eDP?

> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(&pdata->connector,
> +				 &ti_sn_bridge_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) {
> +		DRM_ERROR("failed to find dsi host\n");

There isn't any tear down that needs to happen here?

> +		return -ENODEV;
> +	}
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi)) {
> +		DRM_ERROR("failed to create dsi device\n");
> +		ret = PTR_ERR(dsi);
> +		goto err_dsi_device;

Above you return directly but here you goto a return.  You should use the same
consistent approach in both cases.

> +	}
> +
> +	/* TODO: setting to 4 lanes always for now */
> +	dsi->lanes = 4;
> +	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) {
> +		DRM_ERROR("failed to attach dsi to host\n");
> +		goto err_dsi_attach;
> +	}
> +
> +	pdata->dsi = dsi;
> +
> +	DRM_DEBUG("bridge attached\n");
> +	/* attach panel to bridge */
> +	ti_sn_bridge_attach_panel(pdata);
> +
> +	return 0;
> +
> +err_dsi_attach:
> +	mipi_dsi_device_unregister(dsi);
> +err_dsi_device:
> +	return ret;
> +}
> +
> +static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
> +				    struct drm_display_mode *mode,
> +				    struct drm_display_mode *adj_mode)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +	DRM_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);
> +}
> +
> +static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	struct drm_panel *panel = pdata->panel;
> +
> +	if (panel) {
> +		drm_panel_disable(panel);
> +		drm_panel_unprepare(panel);
> +	}
> +
> +	ti_sn_bridge_power_ctrl(pdata, false);
> +}
> +
> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	struct drm_panel *panel = pdata->panel;
> +	struct drm_display_mode *mode = &pdata->curr_mode;
> +	u32 bit_rate_mhz, clk_freq_mhz;
> +	u8 val = 0;
> +
> +	if (!pdata->num_modes)
> +		return;
> +
> +	ti_sn_bridge_power_ctrl(pdata, true);
> +
> +	if (panel)
> +		drm_panel_prepare(panel);
> +
> +	/* CLK_SRC config */
> +	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
> +			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
> +
> +	/* DSI_A lane config */
> +	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
> +	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
> +	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
> +
> +	/* DP lane config */
> +	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
> +	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
> +	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
> +
> +	/* set dsi clk frequency value */
> +	bit_rate_mhz = (mode->clock / 1000) *
> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> +	/* for each increment in val, frequency increases by 5MHz */
> +	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
> +
> +	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR */
> +
> +	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
> +	usleep_range(10000, 10001);
> +
> +	/* DPCD Register 0x0010A in Sink */
> +	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
> +	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
> +	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
> +	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
> +	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
> +	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
> +	usleep_range(10000, 10001);
> +
> +	/* Semi auto link training mode */
> +	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
> +	usleep_range(20000, 20001);
> +
> +	/* config video parameters */
> +	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
> +			mode->hdisplay & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
> +			(mode->hdisplay >> 0x8) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
> +			mode->vdisplay & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
> +			(mode->hdisplay >> 0x8) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
> +			(mode->htotal - mode->hsync_end) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
> +			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
> +			(mode->vtotal - mode->vsync_end) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
> +			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
> +			(mode->hsync_start - mode->hdisplay) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
> +			(mode->vsync_start - mode->vdisplay) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
> +			(mode->hsync_end - mode->hsync_start) & 0xFF);
> +	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
> +			(mode->vsync_end - mode->vsync_start) & 0xFF);
> +	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
> +		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
> +	else
> +		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
> +	usleep_range(10000, 10001);
> +
> +	/* enable video stream */
> +	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
> +	val |= BIT(3);
> +	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
> +
> +	if (panel)
> +		drm_panel_enable(panel);
> +}
> +
> +static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> +	.attach = ti_sn_bridge_attach,
> +	.enable = ti_sn_bridge_enable,
> +	.disable = ti_sn_bridge_disable,
> +	.mode_set = ti_sn_bridge_mode_set,
> +};
> +
> +static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
> +{
> +	int ret = 0;
> +
> +	if (!pdata || !pdata->dev)
> +		return -EINVAL;

Both of these are known to be valid at this point.

> +	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
> +						"enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(pdata->enable_gpios)) {
> +		DRM_ERROR("failed to get enable gpio from DT\n");
> +		ret = PTR_ERR(pdata->enable_gpios);
> +		goto exit;
> +	}
> +
> +	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
> +						  GPIOD_OUT_LOW);
> +	if (IS_ERR(pdata->irq_gpio))
> +		DRM_ERROR("failed to get irq gpio from DT\n");

You need to set ret to PTR_ERR(pdata->irq_gpio) here.

> +
> +exit:
> +	return ret;
> +}
> +
> +static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> +{
> +	struct device_node *np = pdata->dev->of_node;
> +	struct device_node *end_node;
> +
> +	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
> +	if (!end_node) {
> +		DRM_ERROR("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) {
> +		DRM_ERROR("remote node not found\n");
> +		return -ENODEV;
> +	}
> +	of_node_put(pdata->host_node);
> +
> +	return 0;
> +}
> +
> +static int ti_sn_bridge_probe(struct i2c_client *client,
> +	 const struct i2c_device_id *id)
> +{
> +	struct ti_sn_bridge *pdata;
> +	struct device_node *ddc_node;
> +	int ret = 0;
> +
> +	if (!client || !client->dev.of_node) {

I doubt the i2c probe would send you a NULL value for client and you are
matching on a DT device, so there is a pretty solid argument that
client->dev.of_node will be valid as well.

> +		DRM_ERROR("invalid input\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		DRM_ERROR("device doesn't support I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	pdata = devm_kzalloc(&client->dev,
> +		sizeof(struct ti_sn_bridge), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->power_on = false;
> +	pdata->dev = &client->dev;
> +	pdata->i2c_client = client;
> +	DRM_DEBUG("I2C address is %x\n", client->addr);
> +
> +	ret = ti_sn_bridge_parse_dsi_host(pdata);
> +	if (ret) {
> +		DRM_ERROR("failed to parse device tree\n");

ti_sn_bridge_parse_dsi_host prints a error message on every failure point - you
don't need another one here.

> +		goto err_dt_parse;
> +	}
> +
> +	ret = ti_sn_bridge_parse_gpios(pdata);
> +	if (ret) {
> +		DRM_ERROR("failed to parse gpios from DT\n");

Same - all the paths are covered in the child function.

> +		goto err_dt_parse;
> +	}
> +
> +	ret = ti_sn_bridge_parse_regulators(pdata);
> +	if (ret) {
> +		DRM_ERROR("failed to parse regulators\n");
> +		goto err_dt_parse;
> +	}
> +
> +	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
> +	if (ddc_node) {
> +		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
> +		of_node_put(ddc_node);
> +		if (!pdata->ddc) {
> +			dev_dbg(pdata->dev, "failed to read ddc node\n");
> +			return -EPROBE_DEFER;
> +		}
> +	} else {
> +		dev_dbg(pdata->dev, "no ddc property found\n");
> +	}
> +
> +	ti_sn_bridge_power_ctrl(pdata, true);
> +	ret = ti_sn_bridge_read_device_rev(pdata);
> +	if (ret) {
> +		DRM_ERROR("failed to read chip rev\n");

ti_sn_bridge_read_device_rev has you covered with the error messages.

> +		goto err_config;
> +	} else {
> +		DRM_DEBUG("bridge chip enabled successfully\n");
> +	}

And also the debug messages for whatever its worth.

> +	ti_sn_bridge_power_ctrl(pdata, false);
> +
> +	i2c_set_clientdata(client, pdata);
> +	dev_set_drvdata(&client->dev, pdata);
> +
> +	pdata->bridge.funcs = &ti_sn_bridge_funcs;
> +	pdata->bridge.of_node = client->dev.of_node;
> +
> +	drm_bridge_add(&pdata->bridge);
> +
> +	return ret;
> +
> +err_config:
> +	ti_sn_bridge_power_ctrl(pdata, false);
> +err_dt_parse:
> +	devm_kfree(&client->dev, pdata);
> +	return ret;
> +}
> +
> +static int ti_sn_bridge_remove(struct i2c_client *client)
> +{
> +	int ret = -EINVAL;
> +	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
> +
> +	if (!pdata)
> +		goto end;

pdata is very unlikely to be null here, but as long as this is a light weight
check, I don't think it hurts too much.

> +
> +	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);
> +
> +	ti_sn_bridge_gpio_configure(pdata, false);
> +	i2c_put_adapter(pdata->ddc);
> +
> +	devm_kfree(&client->dev, pdata);
> +
> +end:
> +	return ret;
> +}
> +
> +static struct i2c_device_id ti_sn_bridge_id[] = {
> +	{ "ti,sn65dsi86", 0},
> +	{}

Missing a comma on the end of the empty entry.

> +};
> +MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
> +
> +static const struct of_device_id ti_sn_bridge_match_table[] = {
> +	{.compatible = "ti,sn65dsi86"},
> +	{}

Same.

> +};
> +MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
> +
> +static struct i2c_driver ti_sn_bridge_driver = {
> +	.driver = {
> +		.name = "ti_sn65dsi86",
> +		.owner = THIS_MODULE,
> +		.of_match_table = ti_sn_bridge_match_table,
> +	},
> +	.probe = ti_sn_bridge_probe,
> +	.remove = ti_sn_bridge_remove,
> +	.id_table = ti_sn_bridge_id,
> +};
> +
> +module_i2c_driver(ti_sn_bridge_driver);
> +MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");

Looks like you are missing the module license here.

> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux