Re: [PATCH v5] Support for ns2501-DVO

Re: [PATCH v5] Support for ns2501-DVO

[Daniel Vetter]

On Wed, Jul 18, 2012 at 07:22:30PM +0200, Thomas Richter wrote:
> Hi Daniel, dear intel-drm developers,
> 
> attached again the ns2501 patch contributed a while ago. Sorry for
> taking so long to bring this into the correct form, I wasn't in town
> for the last 3 weeks.
> 
> So long,
> 	Thomas

> This patch adds support for the ns2501 DVO, found in some older Fujitsu/Siemens Labtops. 
> It is in the state of "works for me".
> Includes now proper DPMS support. Includes switching between resolutions - 
> from 640x480 to 1024x768. 
> Currently assumes that the native display resolution is 1024x768.
> 
> The ns2501 seems to be rather critical - if the output PLL is not
> running, the chip doesn't seem to be clocked and then doesn't react
> on i2c messages. Thus, a quick'n-dirty trick ensures that the DVO
> is active before submitting any i2c messages to it. This is
> probably to be reviewed.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=17902
> 
> Signed-off-by: Thomas Richter <thor@math.tu-berlin.de> 

Patch queued for -next, with the whitespace fail fixed up - please consult
Documentation/CodingStyle and scripts/checkpatch.pl for your next patch
submission.

I've also looked a bit at your enable_pll quirk and I plan to kill that in
some later patches because:
- We need to enable pll A unconditionally on i830M, otherwise parts of the
  chip will power down that shouldn't. For some strange reason we have the
  wrong entry in our quirk table.
- Simply enabling the pll without correct register values will hang the
  hw. This can happen after e.g. resume.

Note that a pretty massive rewrite of the entire i915 modeset code is in
the pipeline for 3.7 - consider yourself volunteered to test some neat new
code ;-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/Makefile        |    1 +
>  drivers/gpu/drm/i915/dvo.h           |    1 +
>  drivers/gpu/drm/i915/dvo_ns2501.c    |  566 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |    4 +-
>  drivers/gpu/drm/i915/intel_dvo.c     |   10 +-
>  5 files changed, 580 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/dvo_ns2501.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 2e9268d..8e8e41f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -39,6 +39,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  dvo_ivch.o \
>  	  dvo_tfp410.o \
>  	  dvo_sil164.o \
> +	  dvo_ns2501.o \
>  	  i915_gem_dmabuf.o
>  
>  i915-$(CONFIG_COMPAT)   += i915_ioc32.o
> diff --git a/drivers/gpu/drm/i915/dvo.h b/drivers/gpu/drm/i915/dvo.h
> index 8c2ad01..a2af75c 100644
> --- a/drivers/gpu/drm/i915/dvo.h
> +++ b/drivers/gpu/drm/i915/dvo.h
> @@ -140,5 +140,6 @@ extern struct intel_dvo_dev_ops ch7xxx_ops;
>  extern struct intel_dvo_dev_ops ivch_ops;
>  extern struct intel_dvo_dev_ops tfp410_ops;
>  extern struct intel_dvo_dev_ops ch7017_ops;
> +extern struct intel_dvo_dev_ops ns2501_ops;
>  
>  #endif /* _INTEL_DVO_H */
> diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c b/drivers/gpu/drm/i915/dvo_ns2501.c
> new file mode 100644
> index 0000000..1b15a23
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/dvo_ns2501.c
> @@ -0,0 +1,566 @@
> +/**************************************************************************
> +
> +Copyright ? 2012 Gilles Dartiguelongue, Thomas Richter
> +
> +All Rights Reserved.
> +
> +Permission is hereby granted, free of charge, to any person obtaining a
> +copy of this software and associated documentation files (the
> +"Software"), to deal in the Software without restriction, including
> +without limitation the rights to use, copy, modify, merge, publish,
> +distribute, sub license, and/or sell copies of the Software, and to
> +permit persons to whom the Software is furnished to do so, subject to
> +the following conditions:
> +
> +The above copyright notice and this permission notice (including the
> +next paragraph) shall be included in all copies or substantial portions
> +of the Software.
> +
> +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> +OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> +IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> +TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> +SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> +
> +**************************************************************************/
> +
> +#include "dvo.h"
> +#include "i915_reg.h"
> +#include "i915_drv.h"
> +
> +#define NS2501_VID 0x1305
> +#define NS2501_DID 0x6726
> +
> +#define NS2501_VID_LO 0x00
> +#define NS2501_VID_HI 0x01
> +#define NS2501_DID_LO 0x02
> +#define NS2501_DID_HI 0x03
> +#define NS2501_REV 0x04
> +#define NS2501_RSVD 0x05
> +#define NS2501_FREQ_LO 0x06
> +#define NS2501_FREQ_HI 0x07
> +
> +#define NS2501_REG8 0x08
> +#define NS2501_8_VEN (1<<5)
> +#define NS2501_8_HEN (1<<4)
> +#define NS2501_8_DSEL (1<<3)
> +#define NS2501_8_BPAS (1<<2)
> +#define NS2501_8_RSVD (1<<1)
> +#define NS2501_8_PD (1<<0)
> +
> +#define NS2501_REG9 0x09
> +#define NS2501_9_VLOW (1<<7)
> +#define NS2501_9_MSEL_MASK (0x7<<4)
> +#define NS2501_9_TSEL (1<<3)
> +#define NS2501_9_RSEN (1<<2)
> +#define NS2501_9_RSVD (1<<1)
> +#define NS2501_9_MDI (1<<0)
> +
> +#define NS2501_REGC 0x0c
> +
> +struct ns2501_priv {
> +  //I2CDevRec d;
> +  bool quiet;
> +  int reg_8_shadow;
> +  int reg_8_set;
> +  // Shadow registers for i915
> +  int dvoc;
> +  int pll_a;
> +  int srcdim;
> +  int fw_blc;
> +};
> +
> +#define NSPTR(d) ((NS2501Ptr)(d->DriverPrivate.ptr))
> +
> +/*
> +** Include the PLL launcher prototype
> +*/
> +extern void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> +
> +/*
> +** For reasons unclear to me, the ns2501 at least on the Fujitsu/Siemens
> +** laptops does not react on the i2c bus unless
> +** both the PLL is running and the display is configured in its native
> +** resolution.
> +** This function forces the DVO on, and stores the registers it touches.
> +** Afterwards, registers are restored to regular values.
> +**
> +** This is pretty much a hack, though it works.
> +** Without that, ns2501_readb and ns2501_writeb fail
> +** when switching the resolution.
> +*/
> +
> +static void enable_dvo(struct intel_dvo_device *dvo)
> +{
> +  struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
> +  struct i2c_adapter *adapter = dvo->i2c_bus;
> +  struct intel_gmbus *bus     = container_of(adapter,
> +                                             struct intel_gmbus,
> +                                             adapter);
> +  struct drm_i915_private *dev_priv = bus->dev_priv;
> +
> +  DRM_DEBUG_KMS("%s: Trying to re-enable the DVO\n",__FUNCTION__);
> +
> +  ns->dvoc   = I915_READ(DVO_C);
> +  ns->pll_a  = I915_READ(_DPLL_A);
> +  ns->srcdim = I915_READ(DVOC_SRCDIM);
> +  ns->fw_blc = I915_READ(FW_BLC);
> +
> +  I915_WRITE(DVOC, 0x10004084);
> +  I915_WRITE(_DPLL_A,0xd0820000);
> +  I915_WRITE(DVOC_SRCDIM,0x400300); // 1024x768
> +  I915_WRITE(FW_BLC,0x1080304);     
> +
> +  intel_enable_pll(dev_priv,0);
> +
> +  I915_WRITE(DVOC, 0x90004084);
> +}
> +
> +/*
> +** Restore the I915 registers modified by the above
> +** trigger function.
> +*/
> +static void restore_dvo(struct intel_dvo_device *dvo)
> +{ 
> +  struct i2c_adapter *adapter = dvo->i2c_bus;
> +  struct intel_gmbus *bus     = container_of(adapter,
> +                                             struct intel_gmbus,
> +                                             adapter);
> +  struct drm_i915_private *dev_priv = bus->dev_priv;
> +  struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
> +
> +  I915_WRITE(DVOC       ,ns->dvoc);
> +  I915_WRITE(_DPLL_A    ,ns->pll_a);
> +  I915_WRITE(DVOC_SRCDIM,ns->srcdim);
> +  I915_WRITE(FW_BLC     ,ns->fw_blc);
> +}
> +
> +/*
> +** Read a register from the ns2501.
> +** Returns true if successful, false otherwise.
> +** If it returns false, it might be wise to enable the
> +** DVO with the above function.
> +*/
> +static bool ns2501_readb(struct intel_dvo_device *dvo, int addr, uint8_t *ch)
> +{
> +  struct ns2501_priv *ns = dvo->dev_priv;
> +  struct i2c_adapter *adapter = dvo->i2c_bus;
> +  u8 out_buf[2];
> +  u8 in_buf[2];
> +
> +  struct i2c_msg msgs[] = {
> +    {
> +      .addr = dvo->slave_addr,
> +      .flags = 0,
> +      .len = 1,
> +      .buf = out_buf,
> +    },
> +    {
> +      .addr = dvo->slave_addr,
> +      .flags = I2C_M_RD,
> +      .len = 1,
> +      .buf = in_buf,
> +    }
> +  };
> +
> +  out_buf[0] = addr;
> +  out_buf[1] = 0;
> +
> +  if (i2c_transfer(adapter, msgs, 2) == 2) {
> +    *ch = in_buf[0];
> +    return true;
> +  };
> +
> +  if (!ns->quiet) {
> +    DRM_DEBUG_KMS("Unable to read register 0x%02x from %s:0x%02x.\n",
> +		  addr, adapter->name, dvo->slave_addr);
> +  }
> +
> +  return false;
> +}
> +
> +/*
> +** Write a register to the ns2501.
> +** Returns true if successful, false otherwise.
> +** If it returns false, it might be wise to enable the
> +** DVO with the above function.
> +*/
> +static bool ns2501_writeb(struct intel_dvo_device *dvo, int addr, uint8_t ch)
> +{
> +  struct ns2501_priv *ns = dvo->dev_priv;
> +  struct i2c_adapter *adapter = dvo->i2c_bus;
> +  uint8_t out_buf[2];
> +
> +  struct i2c_msg msg = {
> +    .addr = dvo->slave_addr,
> +    .flags = 0,
> +    .len = 2,
> +    .buf = out_buf,
> +  };
> +
> +  out_buf[0] = addr;
> +  out_buf[1] = ch;
> +
> +  if (i2c_transfer(adapter, &msg, 1) == 1) {
> +    return true;
> +  }
> +
> +  if (!ns->quiet) {
> +    DRM_DEBUG_KMS("Unable to write register 0x%02x to %s:%d\n",
> +                  addr, adapter->name, dvo->slave_addr);
> +  }
> +
> +  return false;
> +}
> +
> +/* National Semiconductor 2501 driver for chip on i2c bus 
> +** scan for the chip on the bus.
> +** Hope the VBIOS initialized the PLL correctly so we can
> +** talk to it. If not, it will not be seen and not detected.
> +** Bummer!
> +*/
> +static bool ns2501_init(struct intel_dvo_device *dvo,
> +			struct i2c_adapter *adapter)
> +{
> +  /* this will detect the NS2501 chip on the specified i2c bus */
> +  struct ns2501_priv *ns;
> +  unsigned char ch;
> +
> +  ns = kzalloc(sizeof(struct ns2501_priv), GFP_KERNEL);
> +  if (ns == NULL)
> +    return false;
> +
> +  dvo->i2c_bus = adapter;
> +  dvo->dev_priv = ns;
> +  ns->quiet = true;
> +
> +  if (!ns2501_readb(dvo, NS2501_VID_LO, &ch))
> +    goto out;
> +
> +  if (ch != (NS2501_VID & 0xff)) {
> +    DRM_DEBUG_KMS("ns2501 not detected got %d: from %s Slave %d.\n",
> +		  ch, adapter->name, dvo->slave_addr);
> +    goto out;
> +  }
> +
> +  if (!ns2501_readb(dvo, NS2501_DID_LO, &ch))
> +    goto out;
> +
> +  if (ch != (NS2501_DID & 0xff)) {
> +    DRM_DEBUG_KMS("ns2501 not detected got %d: from %s Slave %d.\n",
> +		  ch, adapter->name, dvo->slave_addr);
> +    goto out;
> +  }
> +  ns->quiet        = false;
> +  ns->reg_8_set    = 0;
> +  ns->reg_8_shadow = NS2501_8_PD | NS2501_8_BPAS | NS2501_8_VEN | NS2501_8_HEN;
> +
> +  DRM_DEBUG_KMS("init ns2501 dvo controller successfully!\n");
> +  return true;
> +
> + out:
> +  kfree(ns);
> +  return false;
> +}
> +
> +static enum drm_connector_status ns2501_detect(struct intel_dvo_device *dvo)
> +{
> +  /*
> +  ** This is a Laptop display, it doesn't have hotplugging.
> +  ** Even if not, the detection bit of the 2501 is unreliable as
> +  ** it only works for some display types.
> +  ** It is even more unreliable as the PLL must be active for
> +  ** allowing reading from the chiop.
> +  */
> +  return connector_status_connected;
> +}
> +
> +static enum drm_mode_status ns2501_mode_valid(struct intel_dvo_device *dvo,
> +					      struct drm_display_mode *mode)
> +{
> +  DRM_DEBUG_KMS("%s: is mode valid (hdisplay=%d,htotal=%d,vdisplay=%d,vtotal=%d)\n",__FUNCTION__,
> +		mode->hdisplay,mode->htotal,mode->vdisplay,mode->vtotal);
> +
> +
> +  /*
> +  ** Currently, these are all the modes I have data from.
> +  ** More might exist. Unclear how to find the native resolution
> +  ** of the panel in here so we could always accept it
> +  ** by disabling the scaler.
> +  */
> +  if ((mode->hdisplay == 800 && mode->vdisplay == 600) ||
> +      (mode->hdisplay == 640 && mode->vdisplay == 480) ||
> +      (mode->hdisplay == 1024 && mode->vdisplay == 768)) {
> +    return MODE_OK;
> +  } else {
> +    return MODE_ONE_SIZE; /* Is this a reasonable error? */
> +  }    
> +}
> +
> +static void ns2501_mode_set(struct intel_dvo_device *dvo,
> +			    struct drm_display_mode *mode,
> +			    struct drm_display_mode *adjusted_mode)
> +{
> +  bool ok;
> +  bool restore = false;
> +  struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
> +
> +  DRM_DEBUG_KMS("%s: set mode (hdisplay=%d,htotal=%d,vdisplay=%d,vtotal=%d).\n",__FUNCTION__,
> +		mode->hdisplay,mode->htotal,mode->vdisplay,mode->vtotal);
> +
> +  /*
> +  ** Where do I find the native resolution for which scaling is not required???
> +  **
> +  ** First trigger the DVO on as otherwise the chip does not appear on the i2c
> +  ** bus.
> +  */
> +  do {
> +    ok = true;
> +    
> +    if (mode->hdisplay == 800 && mode->vdisplay == 600) {
> +      /* mode 277 */
> +      ns->reg_8_shadow &= ~NS2501_8_BPAS;
> +      DRM_DEBUG_KMS("%s: switching to 800x600\n",__FUNCTION__);
> +      
> +      /*
> +      ** No, I do not know where this data comes from.
> +      ** It is just what the video bios left in the DVO, so
> +      ** I'm just copying it here over.
> +      ** This also means that I cannot support any other modes
> +      ** except the ones supported by the bios.
> +      */
> +      ok &= ns2501_writeb(dvo, 0x11, 0xc8); // 0xc7 also works.
> +      ok &= ns2501_writeb(dvo, 0x1b, 0x19);
> +      ok &= ns2501_writeb(dvo, 0x1c, 0x62); // VBIOS left 0x64 here, but 0x62 works nicer
> +      ok &= ns2501_writeb(dvo, 0x1d, 0x02);
> +      
> +      ok &= ns2501_writeb(dvo, 0x34, 0x03);
> +      ok &= ns2501_writeb(dvo, 0x35, 0xff);
> +      
> +      ok &= ns2501_writeb(dvo, 0x80, 0x27);
> +      ok &= ns2501_writeb(dvo, 0x81, 0x03);
> +      ok &= ns2501_writeb(dvo, 0x82, 0x41);
> +      ok &= ns2501_writeb(dvo, 0x83, 0x05);
> +      
> +      ok &= ns2501_writeb(dvo, 0x8d, 0x02);
> +      ok &= ns2501_writeb(dvo, 0x8e, 0x04);
> +      ok &= ns2501_writeb(dvo, 0x8f, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0x90, 0xfe); /* vertical. VBIOS left 0xff here, but 0xfe works better */
> +      ok &= ns2501_writeb(dvo, 0x91, 0x07);
> +      ok &= ns2501_writeb(dvo, 0x94, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x95, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0x96, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0x99, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x9a, 0x88);
> +      
> +      ok &= ns2501_writeb(dvo, 0x9c, 0x23); /* Looks like first and last line of the image. */
> +      ok &= ns2501_writeb(dvo, 0x9d, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x9e, 0x25);
> +      ok &= ns2501_writeb(dvo, 0x9f, 0x03);
> +      
> +      ok &= ns2501_writeb(dvo, 0xa4, 0x80);
> +      
> +      ok &= ns2501_writeb(dvo, 0xb6, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0xb9, 0xc8); /* horizontal? */
> +      ok &= ns2501_writeb(dvo, 0xba, 0x00); /* horizontal? */
> +      
> +      ok &= ns2501_writeb(dvo, 0xc0, 0x05); /* horizontal? */
> +      ok &= ns2501_writeb(dvo, 0xc1, 0xd7);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc2, 0x00);
> +      ok &= ns2501_writeb(dvo, 0xc3, 0xf8);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc4, 0x03);
> +      ok &= ns2501_writeb(dvo, 0xc5, 0x1a);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc6, 0x00);
> +      ok &= ns2501_writeb(dvo, 0xc7, 0x73);
> +      ok &= ns2501_writeb(dvo, 0xc8, 0x02);
> +
> +    } else if (mode->hdisplay == 640 && mode->vdisplay == 480) {
> +      /* mode 274 */
> +      DRM_DEBUG_KMS("%s: switching to 640x480\n",__FUNCTION__);
> +      /*
> +      ** No, I do not know where this data comes from.
> +      ** It is just what the video bios left in the DVO, so
> +      ** I'm just copying it here over.
> +      ** This also means that I cannot support any other modes
> +      ** except the ones supported by the bios.
> +      */
> +      ns->reg_8_shadow &= ~NS2501_8_BPAS;   
> +      
> +      ok &= ns2501_writeb(dvo, 0x11, 0xa0);
> +      ok &= ns2501_writeb(dvo, 0x1b, 0x11);
> +      ok &= ns2501_writeb(dvo, 0x1c, 0x54);
> +      ok &= ns2501_writeb(dvo, 0x1d, 0x03);
> +      
> +      ok &= ns2501_writeb(dvo, 0x34, 0x03);
> +      ok &= ns2501_writeb(dvo, 0x35, 0xff);
> +      
> +      ok &= ns2501_writeb(dvo, 0x80, 0xff);
> +      ok &= ns2501_writeb(dvo, 0x81, 0x07);
> +      ok &= ns2501_writeb(dvo, 0x82, 0x3d);
> +      ok &= ns2501_writeb(dvo, 0x83, 0x05);
> +      
> +      ok &= ns2501_writeb(dvo, 0x8d, 0x02);
> +      ok &= ns2501_writeb(dvo, 0x8e, 0x10);
> +      ok &= ns2501_writeb(dvo, 0x8f, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0x90, 0xff); /* vertical */
> +      ok &= ns2501_writeb(dvo, 0x91, 0x07);
> +      ok &= ns2501_writeb(dvo, 0x94, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x95, 0x00);
> +      
> +      ok &= ns2501_writeb(dvo, 0x96, 0x05);
> +      
> +      ok &= ns2501_writeb(dvo, 0x99, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x9a, 0x88);
> +      
> +      ok &= ns2501_writeb(dvo, 0x9c, 0x24);
> +      ok &= ns2501_writeb(dvo, 0x9d, 0x00);
> +      ok &= ns2501_writeb(dvo, 0x9e, 0x25);
> +      ok &= ns2501_writeb(dvo, 0x9f, 0x03);
> +
> +      ok &= ns2501_writeb(dvo, 0xa4, 0x84);
> +      
> +      ok &= ns2501_writeb(dvo, 0xb6, 0x09);
> +      
> +      ok &= ns2501_writeb(dvo, 0xb9, 0xa0); /* horizontal? */
> +      ok &= ns2501_writeb(dvo, 0xba, 0x00); /* horizontal? */
> +      
> +      ok &= ns2501_writeb(dvo, 0xc0, 0x05); /* horizontal? */
> +      ok &= ns2501_writeb(dvo, 0xc1, 0x90);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc2, 0x00);
> +      ok &= ns2501_writeb(dvo, 0xc3, 0x0f);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc4, 0x03);
> +      ok &= ns2501_writeb(dvo, 0xc5, 0x16);
> +      
> +      ok &= ns2501_writeb(dvo, 0xc6, 0x00);
> +      ok &= ns2501_writeb(dvo, 0xc7, 0x02);
> +      ok &= ns2501_writeb(dvo, 0xc8, 0x02);
> +      
> +    } else if (mode->hdisplay == 1024 && mode->vdisplay == 768) {
> +      /* mode 280 */ 
> +      DRM_DEBUG_KMS("%s: switching to 1024x768\n",__FUNCTION__);
> +      /*
> +      ** This might or might not work, actually. I'm silently
> +      ** assuming here that the native panel resolution is
> +      ** 1024x768. If not, then this leaves the scaler disabled
> +      ** generating a picture that is likely not the expected.
> +      **
> +      ** Problem is that I do not know where to take the panel
> +      ** dimensions from.
> +      **
> +      ** Enable the bypass, scaling not required. 
> +      **
> +      ** The scaler registers are irrelevant here....
> +      **
> +      */
> +      ns->reg_8_shadow |= NS2501_8_BPAS;
> +      ok &= ns2501_writeb(dvo, 0x37, 0x44);
> +    } else {
> +      /* Data not known. Bummer! 
> +      ** Hopefully, the code should not go here
> +      ** as mode_OK delivered no other modes.
> +      */
> +      ns->reg_8_shadow |= NS2501_8_BPAS;
> +    }
> +    ok &= ns2501_writeb(dvo, NS2501_REG8, ns->reg_8_shadow); 
> +
> +    if (!ok) {
> +      if (restore)
> +	restore_dvo(dvo);
> +      enable_dvo(dvo);
> +      restore = true;
> +    }
> +  } while(!ok);
> +  /*
> +  ** Restore the old i915 registers before
> +  ** forcing the ns2501 on.
> +  */
> +  if (restore)
> +    restore_dvo(dvo);
> +}
> +
> +/* set the NS2501 power state */
> +static void ns2501_dpms(struct intel_dvo_device *dvo, int mode)
> +{
> +  bool ok;
> +  bool restore = false;
> +  struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv);
> +  unsigned char ch;
> +
> +  DRM_DEBUG_KMS("%s: Trying set the dpms of the DVO to %d\n",__FUNCTION__,mode);
> +
> +  ch = ns->reg_8_shadow;
> +  
> +  if (mode == DRM_MODE_DPMS_ON)
> +    ch |= NS2501_8_PD;
> +  else
> +    ch &= ~NS2501_8_PD;
> +
> +  if (ns->reg_8_set == 0 || ns->reg_8_shadow != ch) {
> +    ns->reg_8_set    = 1;
> +    ns->reg_8_shadow = ch;
> +
> +    do {
> +      ok = true;
> +      ok &= ns2501_writeb(dvo, NS2501_REG8, ch);
> +      ok &= ns2501_writeb(dvo, 0x34       ,(mode == DRM_MODE_DPMS_ON)?(0x03):(0x00));
> +      ok &= ns2501_writeb(dvo, 0x35       ,(mode == DRM_MODE_DPMS_ON)?(0xff):(0x00));
> +      if (!ok) {
> +	if (restore)
> +	  restore_dvo(dvo);
> +	enable_dvo(dvo);
> +	restore = true;
> +      }
> +    } while(!ok);
> +
> +    if (restore)
> +      restore_dvo(dvo);
> +  }
> +}
> +
> +static void ns2501_dump_regs(struct intel_dvo_device *dvo)
> +{
> +  uint8_t val;
> +
> +  ns2501_readb(dvo, NS2501_FREQ_LO, &val);
> +  DRM_LOG_KMS("NS2501_FREQ_LO: 0x%02x\n", val);
> +  ns2501_readb(dvo, NS2501_FREQ_HI, &val);
> +  DRM_LOG_KMS("NS2501_FREQ_HI: 0x%02x\n", val);
> +  ns2501_readb(dvo, NS2501_REG8, &val);
> +  DRM_LOG_KMS("NS2501_REG8: 0x%02x\n", val);
> +  ns2501_readb(dvo, NS2501_REG9, &val);
> +  DRM_LOG_KMS("NS2501_REG9: 0x%02x\n", val);
> +  ns2501_readb(dvo, NS2501_REGC, &val);
> +  DRM_LOG_KMS("NS2501_REGC: 0x%02x\n", val);
> +}
> +
> +static void ns2501_destroy(struct intel_dvo_device *dvo)
> +{
> +  struct ns2501_priv *ns = dvo->dev_priv;
> +
> +  if (ns) {
> +    kfree(ns);
> +    dvo->dev_priv = NULL;
> +  }
> +}
> +
> +struct intel_dvo_dev_ops ns2501_ops = {
> +  .init = ns2501_init,
> +  .detect = ns2501_detect,
> +  .mode_valid = ns2501_mode_valid,
> +  .mode_set = ns2501_mode_set,
> +  .dpms = ns2501_dpms,
> +  .dump_regs = ns2501_dump_regs,
> +  .destroy = ns2501_destroy,
> +};
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0161d94..70b83a1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1280,8 +1280,10 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
>   * protect mechanism may be enabled.
>   *
>   * Note!  This is for pre-ILK only.
> + *
> + * Unfortunately needed by dvo_ns2501 since the dvo depends on it running.
>   */
> -static void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> +void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
>  	int reg;
>  	u32 val;
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 60ba50b9..68c1893 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -37,6 +37,7 @@
>  #define SIL164_ADDR	0x38
>  #define CH7xxx_ADDR	0x76
>  #define TFP410_ADDR	0x38
> +#define NS2501_ADDR     0x38
>  
>  static const struct intel_dvo_device intel_dvo_devices[] = {
>  	{
> @@ -74,7 +75,14 @@ static const struct intel_dvo_device intel_dvo_devices[] = {
>  		.slave_addr = 0x75,
>  		.gpio = GMBUS_PORT_DPB,
>  		.dev_ops = &ch7017_ops,
> -	}
> +	},
> +	{
> +	        .type = INTEL_DVO_CHIP_TMDS,
> +		.name = "ns2501",
> +		.dvo_reg = DVOC,
> +		.slave_addr = NS2501_ADDR,
> +		.dev_ops = &ns2501_ops,
> +       }
>  };
>  
>  struct intel_dvo {
> -- 
> 1.7.2.5
> 


-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v5] Support for ns2501-DVO

[Daniel Vetter]

On Thu, Jul 19, 2012 at 09:44:02AM +0200, Thomas Richter wrote:
> Am 18.07.2012 19:59, schrieb Daniel Vetter:
> >
> >Patch queued for -next, with the whitespace fail fixed up - please consult
> >Documentation/CodingStyle and scripts/checkpatch.pl for your next patch
> >submission.
> >
> >I've also looked a bit at your enable_pll quirk and I plan to kill that in
> >some later patches because:
> >- We need to enable pll A unconditionally on i830M, otherwise parts of the
> >   chip will power down that shouldn't. For some strange reason we have the
> >   wrong entry in our quirk table.
> >- Simply enabling the pll without correct register values will hang the
> >   hw. This can happen after e.g. resume.
> 
> Agreed. This "enable" hack is a sledgehammer, unfortunately, I
> haven't had any more precise
> instrument available at the time of writing (I do not know enough on
> the i915 inner workings
> to allow that). I'm not sure it is only the PipeA that needs to be
> enabled to make this working,
> though. I found that the i915 driver includes a workaround list that
> enables pipe A on some models,
> and I seem to remember that I added mine without much success on
> this issue - namely the DVO
> remaining unresponsive.
> 
> >Note that a pretty massive rewrite of the entire i915 modeset code is in
> >the pipeline for 3.7 - consider yourself volunteered to test some neat new
> >code ;-)
> No problem, but please don't expect fast turnaround times here as I
> only have occasional access to the
> machines (one intel R31, and an equally old Fujitsu Siemens whose
> model number I forgot).
> 
> As a side question, have you been able to look into the video
> overlay hang I reported lately? I haven't
> had the time yet to update the i915 driver from the latest
> repository, but the version from three weeks
> ago hung completely when enabling overlays - the one before worked
> at least *once*, and hung the second
> time an overlay was created.

Hm, that could be related to the pipe A quirk - if the pipe A isn't
enabled, we enable it while switching the overlay. But only on i830M, so
this code has a rather high chance of being untested and hence broken.
Unfortunately I have to rebase my modeset rework on top of your dvo
branch, otherwise you don't have much to test. I'll try to get around to
that in the next few days.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v5] Support for ns2501-DVO

[Daniel Vetter]

On Thu, Jul 19, 2012 at 02:20:11PM +0200, Thomas Richter wrote:
> Hi Daniel,
> 
> >>As a side question, have you been able to look into the video
> >>overlay hang I reported lately? I haven't
> >>had the time yet to update the i915 driver from the latest
> >>repository, but the version from three weeks
> >>ago hung completely when enabling overlays - the one before worked
> >>at least *once*, and hung the second
> >>time an overlay was created.
> >Hm, that could be related to the pipe A quirk - if the pipe A isn't
> >enabled, we enable it while switching the overlay. But only on i830M, so
> >this code has a rather high chance of being untested and hence broken.
> >Unfortunately I have to rebase my modeset rework on top of your dvo
> >branch, otherwise you don't have much to test. I'll try to get around to
> >that in the next few days.
> 
> I wouldn't need the DVO patch integrated into that since the overlay
> problem arises on two machines,
> the Fujitsu Siemens (requiring the DVO patch) and the IBM R31 (of
> which the DVO is already supported
> out of the box).

Cool, so if you've got time to test it, please try out the modeset-rework
branch from my personal git repo at

http://cgit.freedesktop.org/~danvet/drm

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v5] Support for ns2501-DVO

[Daniel Vetter]

On Thu, Jul 19, 2012 at 02:20:11PM +0200, Thomas Richter wrote:
> Hi Daniel,
> 
> >>As a side question, have you been able to look into the video
> >>overlay hang I reported lately? I haven't
> >>had the time yet to update the i915 driver from the latest
> >>repository, but the version from three weeks
> >>ago hung completely when enabling overlays - the one before worked
> >>at least *once*, and hung the second
> >>time an overlay was created.
> >Hm, that could be related to the pipe A quirk - if the pipe A isn't
> >enabled, we enable it while switching the overlay. But only on i830M, so
> >this code has a rather high chance of being untested and hence broken.
> >Unfortunately I have to rebase my modeset rework on top of your dvo
> >branch, otherwise you don't have much to test. I'll try to get around to
> >that in the next few days.
> 
> I wouldn't need the DVO patch integrated into that since the overlay
> problem arises on two machines,
> the Fujitsu Siemens (requiring the DVO patch) and the IBM R31 (of
> which the DVO is already supported
> out of the box).

Ok, I've updated my modeset-rework branch and applied the required changes
to the ns2501 dvo driver. I've also killed the pll hack. Testing feedback
_very_ much appreciated.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48