[[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver
spanda at codeaurora.org
spanda at codeaurora.org
Tue Apr 24 09:48:51 UTC 2018
On 2018-04-21 01:06, Sean Paul wrote:
> On Thu, Apr 19, 2018 at 11:26:05PM +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.
>>
>> Changes in v1:
>> - Split the dt-bindings and the driver support into separate patches
>> (Andrzej Hajda).
>> - Use of gpiod APIs to parse and configure gpios instead of obsolete
>> ones
>> (Andrzej Hajda).
>> - Use macros to define the register offsets (Andrzej Hajda).
>>
>> Changes in v2:
>> - Separate out edp panel specific HW resource handling from bridge
>> driver and create a separate edp panel drivers to handle panel
>> specific mode information and HW resources (Sean Paul).
>> - Replace pr_* APIs to DRM_* APIs to log error or debug information
>> (Sean Paul).
>> - Remove some of the unnecessary structure/variable from driver (Sean
>> Paul).
>> - Rename the function and structure prefix "sn65dsi86" to
>> "ti_sn_bridge"
>> (Sean Paul / Rob Herring).
>> - Remove most of the hard-coding and modified the bridge init
>> sequence
>> based on current mode (Sean Paul).
>> - Remove the existing function to retrieve the EDID data and
>> implemented this as an i2c_adapter and use drm_get_edid() (Sean
>> Paul).
>> - Remove the dummy irq handler implementation, will add back the
>> proper irq handling later (Sean Paul).
>> - Capture the required enable gpios in a single array based on dt
>> entry
>> instead of having individual descriptor for each gpio (Sean Paul).
>>
>> Changes in v3:
>> - Remove usage of irq_gpio and replace it as "interrupts" property
>> (Rob
>> Herring).
>> - Remove the unnecessary header file inclusions (Sean Paul).
>> - Rearrange the header files in alphabetical order (Sean Paul).
>> - Use regmap interface to perform i2c transactions.
>> - Update Copyright/License field and address other review comments
>> (Jordan Crouse).
>>
>> Signed-off-by: Sandeep Panda <spanda at codeaurora.org>
>> ---
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 690
>> ++++++++++++++++++++++++++++++++++
>
> What about Kconfig/Makefile?
Thought of adding this in another change once review is over. But will
add to current change in next patchset.
>
>> 1 file changed, 690 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..a2a95f5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -0,0 +1,690 @@
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. 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 version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * 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.
>> + */
>
> Use SPDX license
Will add back SPDX along with copyright in next patchset.
>
>> +
>> +#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>
>> +#include <linux/gpio.h>
>> +#include <linux/i2c.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.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
>> +/* 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 MIN_DSI_CLK_FREQ_MHZ 40
>> +
>> +#define DPPLL_CLK_SRC_REFCLK 0
>> +#define DPPLL_CLK_SRC_DSICLK 1
>> +
>> +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 regmap *regmap;
>> + 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;
>> + /* list of gpios needed to enable the bridge functionality */
>> + struct gpio_descs *enable_gpios;
>
> From my reading of the datasheet, these gpios can be used for a variety
> of
> things, not just tied to enable (input/output). So, separate enable
> input
> from the gpios, and make the gpios configurable via dt.
>
So are you suggesting something like,
struct gpio_desc *enable; --> for enable gpio
struct gpio_descs *misc; --> for other gpios like aux_channel, mentioned
in the schematic
>> + int *value_array;
>> + int intp_irq;
>
> What's this used for?
>
Will remove this for now and will add back while adding support for
HPD/interrupts.
>> + unsigned int num_supplies;
>> + struct regulator_bulk_data *supplies;
>> + struct i2c_client *i2c_client;
>> + struct i2c_adapter *ddc;
>> + u32 num_modes;
>> + struct drm_display_mode curr_mode;
>> +};
>> +
>> +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
>> + { .range_min = 0, .range_max = 0xff },
>> +};
>> +
>> +static const struct regmap_access_table ti_sn_bridge_volatile_table =
>> {
>> + .yes_ranges = ti_sn_bridge_volatile_ranges,
>> + .n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
>> +};
>> +
>> +static const struct regmap_config ti_sn_bridge_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .volatile_table = &ti_sn_bridge_volatile_table,
>> + .cache_type = REGCACHE_NONE,
>> +};
>> +
>> +static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata,
>> bool on)
>> +{
>> + int value, i = 0;
>> +
>> + value = on ? 1 : 0;
>> +
>> + if (pdata->enable_gpios->ndescs && pdata->value_array) {
>> + for (i = 0; i < pdata->enable_gpios->ndescs; i++)
>> + pdata->value_array[i] = value;
>> + gpiod_set_array_value(pdata->enable_gpios->ndescs,
>> + pdata->enable_gpios->desc, pdata->value_array);
>> + }
>> +
>> + 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;
>
> I'm a little confused why you chose a static variable instead of
> stashing this
> in the pdata. Also, this isn't locked, so you will have races between
> setting
> ref_count and doing the enable/disable.
Want to limit the ref count to this function only, instead of exposing
the ref count to
outisde. I will add mutex protection to this function, since it is
called from multiple
places. Missed it.
>
>> +
>> + 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;
>
> Why return void if you can fail?
We are not using the return value of this function anywhere. So just
wanted to
put a print here in case, even though regulator_bulk_enable is highly
unlikely to fail.
>
>> + }
>> + } else if (!enable && !ref_count) {
>> + regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
>> +
>> + ti_sn_bridge_gpio_configure(pdata, 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);
>
> Just return early instead of indenting the rest of the function.
OK.
>
>> + } 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);
>
> This is a static function inside drm_edid.c. How did this even compile?
>
This is not static in drm_edid.c, Atleast i can see this as a
non-static function in 4.14 kernel, it is used by other drivers also.
>> + kfree(edid);
>> + } else {
>> + DRM_DEBUG("failed to get edid\n");
>
> Flip the if (edid) condition around and exit early.
OK
>
>> + }
>> + 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.
>
> Perhaps then you should return unknown instead of disconnected.
OK
>
>> + */
>> + 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)
>> +{
>> + unsigned int rev = 0;
>> + int ret = 0;
>> +
>> + ret = regmap_read(pdata->regmap, SN_DEVICE_REV_REG, &rev);
>> + if (ret)
>> + goto exit;
>
> Why do you need a label just to return?
Ok, some kernel approvers dont allow multiple return statements so using
label wherever possible through out the driver.
If its ok to have numerous return statements then will follow that
through out.
>
>> +
>> + if (rev == SN_BRIDGE_REVISION_ID) {
>> + DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
>
> The revision is hardcoded, so this print isn't really useful.
>
Ok, will remove this print from here and add a debug print in end of
probe, the intention is to indicate that probe has been successful.
>> + } else {
>> + DRM_ERROR("ti_sn_bridge revision id: 0x%x mismatch\n", rev);
>> + 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;
>> +
>> + 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];
>> +
>> + return devm_regulator_bulk_get(pdata->dev,
>> + pdata->num_supplies, pdata->supplies);
>> +}
>> +
>> +static int ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
>> +{
>> + struct device_node *panel_node, *port, *endpoint;
>> +
>> + pdata->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");
>
> Why are you switching to dev_err?
will make this DRM_ERROR
>
>> + return -EINVAL;
>> + }
>> +
>> + 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");
>> + return -EINVAL;
>> + }
>> +
>> + pdata->panel = of_drm_find_panel(panel_node);
>> + of_node_put(panel_node);
>> + if (!pdata->panel) {
>> + dev_err(pdata->dev, "no panel node found\n");
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + drm_panel_attach(pdata->panel, &pdata->connector);
>
> Why is this outside the if statement? If port is NULL, then
> pdata->panel is
> NULL, and the first thing drm_panel_attach does is dereference panel.
Thanks for pointing out this, drm_panel_attach() should be inside if
statement.
>
>> +
>> + return 0;
>> +}
>> +
>> +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\n");
>> + ret = -ENODEV;
>> + goto error;
>> + }
>> +
>> + /* HPD not supported */
>> + pdata->connector.polled = 0;
>> +
>> + ret = drm_connector_init(bridge->dev, &pdata->connector,
>> + &ti_sn_bridge_connector_funcs,
>> + DRM_MODE_CONNECTOR_eDP);
>> + if (ret) {
>> + DRM_ERROR("Failed to initialize connector with drm\n");
>> + goto error;
>> + }
>> +
>> + 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");
>> + ret = -ENODEV;
>> + goto error;
>> + }
>> +
>> + 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 error;
>
> All of these error labels just return ret. So, why not just return
> directly?
>
>> + }
>> +
>> + /* 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);
>> +error:
>> + 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;
>> + unsigned int bit_rate_mhz, clk_freq_mhz;
>> + unsigned int val = 0;
>> +
>> + if (!pdata->num_modes)
>> + return;
>
> This relies on get_modes() to be called before enable, which is not
> guaranteed
> (user defined modes). You also are racing with get_modes(), so it's
> possible
> this is inaccurate anyways. Is there a good reason to gate enable on
> num_modes?
>
I will remove the num_modes check from here, framework should ensure
bridge_enable
gets called only after set_mode is called.
>> +
>> + ti_sn_bridge_power_ctrl(pdata, true);
>> +
>> + if (panel)
>> + drm_panel_prepare(panel);
>> +
>> + /* CLK_SRC config */
>> + regmap_write(pdata->regmap, SN_REFCLK_FREQ_REG,
>> + (DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
>> +
>> + /* DSI_A lane config */
>> + regmap_read(pdata->regmap, SN_DSI_LANES_REG, &val);
>> + val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
>
> These masks should be #defined, and imo the 4 would read better if it
> were not
> listed in hex. Same goes below, masks and offsets need to be #defined.
Ok will remove hex notation.
>
> You're also doing a bitwise OR on the current value of lanes. So since
> the
> default value is 0x3, you'll never actually change anything. You need
> to clear
> the current value before setting the new value. This applies below as
> well.
yes, i should mask the bits first and then program the new value.
>
>
>> + regmap_write(pdata->regmap, SN_DSI_LANES_REG, val);
>> +
>> + /* DP lane config */
>> + regmap_read(pdata->regmap, SN_SSC_CONFIG_REG, &val);
>> + val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
>> + regmap_write(pdata->regmap, 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);
>> + regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>> +
>> + regmap_write(pdata->regmap, SN_DATARATE_CONFIG_REG, 0x80); /* set
>> HBR */
>
> This should also be a function of the pclk, right?
Yes, will program this based on pclk.
>
>> +
>> + regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL
>> */
>> + usleep_range(10000, 10050);
>> +
>> + /* DPCD Register 0x0010A in Sink */
>> + regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
>> + regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
>> + regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
>> + regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
>> + regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
>> + regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
>
> What are you doing here and why?
As per the datasheet we need to enable ASSR((Alternate Scrambler Seed
Reset) in edp panel before we start link training.
This is step is done for that. It will common for all edp panels.
"The SN65DSI86 only supports ASSR Display Authentication method
and this method is enabled by default. An eDP panel
must support this Authentication method. Software will need to enable
this method in the eDP panel at DisplayPort address
0x0010A."
>
>> + usleep_range(10000, 10050);
>> +
>> + /* Semi auto link training mode */
>> + regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
>> + usleep_range(20000, 20050);
>
> You should use msleep if you need to sleep for ~20ms
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/timers/timers-howto.txt
>
>> +
>> + /* config video parameters */
>> + regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
>> + mode->hdisplay & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
>> + (mode->hdisplay >> 0x8) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
>> + mode->vdisplay & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
>> + (mode->vdisplay >> 0x8) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
>> + (mode->hsync_end - mode->hsync_start) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
>> + ((mode->hsync_end - mode->hsync_start) >> 0x8) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
>> + (mode->vsync_end - mode->vsync_start) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
>> + ((mode->vsync_end - mode->vsync_start) >> 0x8) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
>> + (mode->htotal - mode->hsync_end) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
>> + (mode->vtotal - mode->vsync_end) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
>> + (mode->hsync_start - mode->hdisplay) & 0xFF);
>> + regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
>> + (mode->vsync_start - mode->vdisplay) & 0xFF);
>> + if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
>> + regmap_write(pdata->regmap, SN_DATA_FORMAT_REG, 0x00);
>> + else
>> + regmap_write(pdata->regmap, SN_DATA_FORMAT_REG, 0x01);
>
> The format is hardcoded above.
>
>> + usleep_range(10000, 10050);
>> +
>> + /* enable video stream */
>> + regmap_read(pdata->regmap, SN_ENH_FRAME_REG, &val);
>> + val |= BIT(3);
>> + regmap_write(pdata->regmap, 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;
>> +
>> + 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;
>> + }
>> + /* initialize the value array */
>> + pdata->value_array = kmalloc(pdata->enable_gpios->ndescs *
>> sizeof(int),
>> + GFP_KERNEL);
>> + if (!pdata->value_array) {
>> + ret = -ENOMEM;
>> + goto exit;
>> + }
>> + memset(pdata->value_array, 0,
>> + pdata->enable_gpios->ndescs * sizeof(int));
>> +
>> +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 (!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->dev = &client->dev;
>> + pdata->i2c_client = client;
>> + pdata->intp_irq = client->irq;
>> + DRM_DEBUG("I2C address is %x\n", client->addr);
>> +
>> + pdata->regmap = devm_regmap_init_i2c(client,
>> + &ti_sn_bridge_regmap_config);
>> + if (IS_ERR(pdata->regmap))
>> + return PTR_ERR(pdata->regmap);
>> +
>> + ret = ti_sn_bridge_parse_dsi_host(pdata);
>> + if (ret)
>> + goto err_dt_parse;
>> +
>> + ret = ti_sn_bridge_parse_gpios(pdata);
>> + if (ret)
>> + goto err_dt_parse;
>> +
>> + ret = ti_sn_bridge_parse_regulators(pdata);
>> + if (ret) {
>> + DRM_ERROR("failed to parse regulators\n");
>> + goto err_reg_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)
>> + goto err_config;
>> + ti_sn_bridge_power_ctrl(pdata, false);
>> +
>> + i2c_set_clientdata(client, pdata);
>> + dev_set_drvdata(&client->dev, pdata);
>
> drvdata doesn't seem to be used anywhere.
>
>> +
>> + 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);
>
> Just turn it off before you check ret (similar to the of_node_put calls
> throughout the driver).
OK
>
>> +err_reg_parse:
>> + kfree(pdata->value_array);
>> +err_dt_parse:
>> + devm_kfree(&client->dev, pdata);
>
> If it's device managed, you don't need explicit frees.
Yes. will remove this.
>
>> + 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;
>> +
>> + mipi_dsi_detach(pdata->dsi);
>> + mipi_dsi_device_unregister(pdata->dsi);
>> +
>> + ti_sn_bridge_power_ctrl(pdata, false);
>
> If you want to refcount power_ctrl, you'll need to make sure that it's
> _actually_ off.
checked all the calls to power_ctrl in driver, we dont need a call to
power_ctrl.
bridge_disable should be the place where last call to power_ctrl needs
to be made.
>
>> + kfree(pdata->value_array);
>> +
>> + drm_bridge_remove(&pdata->bridge);
>> + 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},
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
>> +
>> +static const struct of_device_id ti_sn_bridge_match_table[] = {
>> + {.compatible = "ti,sn65dsi86"},
>> + {},
>> +};
>> +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");
>> +MODULE_LICENSE("GPL");
>
> GPL v2?
>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
More information about the dri-devel
mailing list