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

2018-06-14 Thread spanda

On 2018-06-06 04:29, Sean Paul wrote:

On Tue, Jun 05, 2018 at 11:10:15AM +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).

Changes in v4:
 - Update License/Copyright (Sean Paul).
 - Add Kconfig and Makefile changes (Sean Paul).
 - Drop i2c gpio handling from this bridge driver, since i2c sda/scl 
gpios

   will be handled by i2c master.
 - Update required supplies names.
 - Remove unnecessary goto statements (Sean Paul).
 - Add mutex lock to power_ctrl API to avoid race conditions (Sean
   Paul).
 - Add support to parse reference clk frequency from dt(optional).
 - Update the bridge chip enable/disable sequence.

Changes in v5:
 - Fixed Kbuild test service reported warnings.

Changes in v6:
 - Use PM runtime based ref-counting instead of local ref_count 
mechanism

   (Stephen Boyd).
 - Clean up some debug logs and indentations (Sean Paul).
 - Simplify dp rate calculation (Sean Paul).
 - Add support to configure refclk based on input REFCLK pin or DACP/N
   pin (Stephen Boyd).

Changes in v7:
 - Use static supply entries instead of dynamic allocation (Andrzej
   Hajda).
 - Defer bridge driver probe if panel is not probed (Andrzej Hajda).
 - Update of_graph APIs for correct node reference management. 
(Andrzej

   Hajda).
 - Remove local display_mode object (Andrzej Hajda).
 - Remove version id check function from driver.

Changes in v8:
 - Move dsi register/attach function to bridge driver probe (Andrzej
   Hajda).
 - Introduce a new helper function to write 16bit words into 
consecutive

   registers (Andrzej Hajda).
 - Remove unnecessary macros (Andrzej Hajda).

Signed-off-by: Sandeep Panda 
---
 drivers/gpu/drm/bridge/Kconfig|   9 +
 drivers/gpu/drm/bridge/Makefile   |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 666 
++

 3 files changed, 676 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/drivers/gpu/drm/bridge/Kconfig 
b/drivers/gpu/drm/bridge/Kconfig

index 3b99d5a..8153150 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -108,6 +108,15 @@ config DRM_TI_TFP410
---help---
  Texas Instruments TFP410 DVI/HDMI Transmitter driver

+config DRM_TI_SN65DSI86
+   tristate "TI SN65DSI86 DSI to eDP bridge"
+   depends on OF
+   select DRM_KMS_HELPER
+   select REGMAP_I2C
+   select DRM_PANEL
+   ---help---
+ Texas Instruments SN65DSI86 DSI to eDP Bridge driver
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"

 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile 
b/drivers/gpu/drm/bridge/Makefile

index 373eb28..3711be8 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
+obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o


SN65DSI86 

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

2018-06-14 Thread spanda

On 2018-06-12 05:05, Stephen Boyd wrote:

Quoting Sandeep Panda (2018-06-04 22:40:15)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c

new file mode 100644
index 000..add6e0f
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -0,0 +1,666 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+

[...]

+
+static const struct regmap_config ti_sn_bridge_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+   .volatile_table = _sn_bridge_volatile_table,
+   .cache_type = REGCACHE_NONE,
+};
+
+static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
+  unsigned int reg, u16 val)
+{
+   regmap_write(pdata->regmap, reg, val & 0xFF);
+   regmap_write(pdata->regmap, reg + 1, val >> 8);
+}
+
+static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
+{
+   struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
+   int ret = 0;


Please don't assign variables and then reassign them again immediately
after. It hides use before real initialization bugs.


Ok.


+
+   ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, 
pdata->supplies);

[...]

+
+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;
+   struct mipi_dsi_host *host;
+   struct mipi_dsi_device *dsi;
+   int ret = 0;
+   const struct mipi_dsi_device_info info = { .type = 
"ti_sn_bridge",

+  .channel = 0,
+  .node = NULL,
+};
+
+   if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+   DRM_ERROR("device doesn't support I2C\n");
+   return -ENODEV;
+   }
+
+   ret = ti_sn_bridge_parse_dsi_host(pdata);
+   if (ret)
+   return ret;
+
+   host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+   if (!host) {
+   DRM_ERROR("failed to find dsi host\n");


Not sure we want to print an error and then return -EPROBE_DEFER.
Usually EPROBE_DEFER is silent.


+   ret = -EPROBE_DEFER;
+   goto err_dsi_host;
+   }
+

[...]

+
+   /* 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;
+
+   pdata->refclk = devm_clk_get(pdata->dev, "refclk");


We need to check for error

if (IS_ERR(pdata->refclk))

And then if it's EPROBE_DEFER I suppose we would bail out, otherwise
assume it's not present?


Yes found this issue while testing the driver on actual sn65dsi86 HW, i 
will fix this in next patchset.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-06-12 Thread Stephen Boyd
Quoting Sandeep Panda (2018-06-04 22:40:15)
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> new file mode 100644
> index 000..add6e0f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,666 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
[...]
> +
> +static const struct regmap_config ti_sn_bridge_regmap_config = {
> +   .reg_bits = 8,
> +   .val_bits = 8,
> +   .volatile_table = _sn_bridge_volatile_table,
> +   .cache_type = REGCACHE_NONE,
> +};
> +
> +static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
> +  unsigned int reg, u16 val)
> +{
> +   regmap_write(pdata->regmap, reg, val & 0xFF);
> +   regmap_write(pdata->regmap, reg + 1, val >> 8);
> +}
> +
> +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
> +{
> +   struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> +   int ret = 0;

Please don't assign variables and then reassign them again immediately
after. It hides use before real initialization bugs.

> +
> +   ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);
[...]
> +
> +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;
> +   struct mipi_dsi_host *host;
> +   struct mipi_dsi_device *dsi;
> +   int ret = 0;
> +   const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
> +  .channel = 0,
> +  .node = NULL,
> +};
> +
> +   if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +   DRM_ERROR("device doesn't support I2C\n");
> +   return -ENODEV;
> +   }
> +
> +   ret = ti_sn_bridge_parse_dsi_host(pdata);
> +   if (ret)
> +   return ret;
> +
> +   host = of_find_mipi_dsi_host_by_node(pdata->host_node);
> +   if (!host) {
> +   DRM_ERROR("failed to find dsi host\n");

Not sure we want to print an error and then return -EPROBE_DEFER.
Usually EPROBE_DEFER is silent.

> +   ret = -EPROBE_DEFER;
> +   goto err_dsi_host;
> +   }
> +
[...]
> +
> +   /* 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;
> +
> +   pdata->refclk = devm_clk_get(pdata->dev, "refclk");

We need to check for error

if (IS_ERR(pdata->refclk))

And then if it's EPROBE_DEFER I suppose we would bail out, otherwise
assume it's not present?

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-06-05 Thread Sean Paul
On Tue, Jun 05, 2018 at 11:10:15AM +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).
> 
> Changes in v4:
>  - Update License/Copyright (Sean Paul).
>  - Add Kconfig and Makefile changes (Sean Paul).
>  - Drop i2c gpio handling from this bridge driver, since i2c sda/scl gpios
>will be handled by i2c master.
>  - Update required supplies names.
>  - Remove unnecessary goto statements (Sean Paul).
>  - Add mutex lock to power_ctrl API to avoid race conditions (Sean
>Paul).
>  - Add support to parse reference clk frequency from dt(optional).
>  - Update the bridge chip enable/disable sequence.
> 
> Changes in v5:
>  - Fixed Kbuild test service reported warnings.
> 
> Changes in v6:
>  - Use PM runtime based ref-counting instead of local ref_count mechanism
>(Stephen Boyd).
>  - Clean up some debug logs and indentations (Sean Paul).
>  - Simplify dp rate calculation (Sean Paul).
>  - Add support to configure refclk based on input REFCLK pin or DACP/N
>pin (Stephen Boyd).
> 
> Changes in v7:
>  - Use static supply entries instead of dynamic allocation (Andrzej
>Hajda).
>  - Defer bridge driver probe if panel is not probed (Andrzej Hajda).
>  - Update of_graph APIs for correct node reference management. (Andrzej
>Hajda).
>  - Remove local display_mode object (Andrzej Hajda).
>  - Remove version id check function from driver.
> 
> Changes in v8:
>  - Move dsi register/attach function to bridge driver probe (Andrzej
>Hajda).
>  - Introduce a new helper function to write 16bit words into consecutive
>registers (Andrzej Hajda).
>  - Remove unnecessary macros (Andrzej Hajda).
> 
> Signed-off-by: Sandeep Panda 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   9 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 666 
> ++
>  3 files changed, 676 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..8153150 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -108,6 +108,15 @@ config DRM_TI_TFP410
>   ---help---
> Texas Instruments TFP410 DVI/HDMI Transmitter driver
>  
> +config DRM_TI_SN65DSI86
> + tristate "TI SN65DSI86 DSI to eDP bridge"
> + depends on OF
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + select DRM_PANEL
> + ---help---
> +   Texas Instruments SN65DSI86 DSI to eDP Bridge driver
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>  
>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..3711be8 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += 

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

2018-06-05 Thread Sean Paul
On Tue, Jun 05, 2018 at 04:47:15PM +0530, Vinod wrote:
> On 05-06-18, 11:10, 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).
> 
> This is pretty useless for changelog. This is useful for review but not
> down the line when this is applied
> 
> Since you have cover letter, you may add it there. Or after sob and ---
> tag in the patch, that way it is skipped while applying..

FWIW, in drm we prefer the changelog is included in the commit message. It gives
credit to the reviewers, sometimes explains why decisions were made, and commit
message space is cheap :-).

Sean

> 
> > +#define SN_ENABLE_VID_STREAM_BIT   BIT(3)
> > +#define SN_DSIA_NUM_LANES_BITS (BIT(4) | BIT(3))
> > +#define SN_DP_NUM_LANES_BITS   (BIT(5) | BIT(4))
> > +#define SN_DP_DATA_RATE_BITS   (BIT(7) | BIT(6) | BIT(5))
> 
> GENMASK(7, 5)
> 
> > +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
> > +{
> > +   struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> > +   int ret = 0;
> 
> superfluous initialization
> 
> > +static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
> > +{
> > +   struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> > +   int ret = 0;
> 
> here as well
> 
> > +static int ti_sn_bridge_connector_get_modes(struct drm_connector 
> > *connector)
> > +{
> > +   struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > +   struct edid *edid;
> > +   u32 num_modes;
> > +
> > +   if (pdata->panel) {
> > +   DRM_DEBUG_KMS("get mode from connected drm_panel\n");
> > +   return drm_panel_get_modes(pdata->panel);
> > +   }
> > +
> > +   if (!pdata->ddc)
> > +   return 0;
> > +
> > +   pm_runtime_get_sync(pdata->dev);
> 
> you should check return of this
> 
> > +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
> > +{
> > +   int i = 0;
> 
> superfluous initialization
> 
> > +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > +{
> > +   struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +   unsigned int val = 0;
> 
> here as well
> 
> > +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +   struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +
> > +   pm_runtime_get_sync(pdata->dev);
> 
> error check required
> -- 
> ~Vinod

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

2018-06-05 Thread Vinod
On 05-06-18, 11:10, 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).

This is pretty useless for changelog. This is useful for review but not
down the line when this is applied

Since you have cover letter, you may add it there. Or after sob and ---
tag in the patch, that way it is skipped while applying..

> +#define SN_ENABLE_VID_STREAM_BIT BIT(3)
> +#define SN_DSIA_NUM_LANES_BITS   (BIT(4) | BIT(3))
> +#define SN_DP_NUM_LANES_BITS (BIT(5) | BIT(4))
> +#define SN_DP_DATA_RATE_BITS (BIT(7) | BIT(6) | BIT(5))

GENMASK(7, 5)

> +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
> +{
> + struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> + int ret = 0;

superfluous initialization

> +static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
> +{
> + struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> + int ret = 0;

here as well

> +static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> +{
> + struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> + struct edid *edid;
> + u32 num_modes;
> +
> + if (pdata->panel) {
> + DRM_DEBUG_KMS("get mode from connected drm_panel\n");
> + return drm_panel_get_modes(pdata->panel);
> + }
> +
> + if (!pdata->ddc)
> + return 0;
> +
> + pm_runtime_get_sync(pdata->dev);

you should check return of this

> +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
> +{
> + int i = 0;

superfluous initialization

> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> + unsigned int val = 0;

here as well

> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + pm_runtime_get_sync(pdata->dev);

error check required
-- 
~Vinod
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH v8 1/4] drm/bridge: add support for sn65dsi86 bridge driver

2018-06-05 Thread Sandeep Panda
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).

Changes in v4:
 - Update License/Copyright (Sean Paul).
 - Add Kconfig and Makefile changes (Sean Paul).
 - Drop i2c gpio handling from this bridge driver, since i2c sda/scl gpios
   will be handled by i2c master.
 - Update required supplies names.
 - Remove unnecessary goto statements (Sean Paul).
 - Add mutex lock to power_ctrl API to avoid race conditions (Sean
   Paul).
 - Add support to parse reference clk frequency from dt(optional).
 - Update the bridge chip enable/disable sequence.

Changes in v5:
 - Fixed Kbuild test service reported warnings.

Changes in v6:
 - Use PM runtime based ref-counting instead of local ref_count mechanism
   (Stephen Boyd).
 - Clean up some debug logs and indentations (Sean Paul).
 - Simplify dp rate calculation (Sean Paul).
 - Add support to configure refclk based on input REFCLK pin or DACP/N
   pin (Stephen Boyd).

Changes in v7:
 - Use static supply entries instead of dynamic allocation (Andrzej
   Hajda).
 - Defer bridge driver probe if panel is not probed (Andrzej Hajda).
 - Update of_graph APIs for correct node reference management. (Andrzej
   Hajda).
 - Remove local display_mode object (Andrzej Hajda).
 - Remove version id check function from driver.

Changes in v8:
 - Move dsi register/attach function to bridge driver probe (Andrzej
   Hajda).
 - Introduce a new helper function to write 16bit words into consecutive
   registers (Andrzej Hajda).
 - Remove unnecessary macros (Andrzej Hajda).

Signed-off-by: Sandeep Panda 
---
 drivers/gpu/drm/bridge/Kconfig|   9 +
 drivers/gpu/drm/bridge/Makefile   |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 666 ++
 3 files changed, 676 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3b99d5a..8153150 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -108,6 +108,15 @@ config DRM_TI_TFP410
---help---
  Texas Instruments TFP410 DVI/HDMI Transmitter driver
 
+config DRM_TI_SN65DSI86
+   tristate "TI SN65DSI86 DSI to eDP bridge"
+   depends on OF
+   select DRM_KMS_HELPER
+   select REGMAP_I2C
+   select DRM_PANEL
+   ---help---
+ Texas Instruments SN65DSI86 DSI to eDP Bridge driver
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..3711be8 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
+obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
 obj-y += synopsys/
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
new file mode 100644
index