Re: [PATCH 10/12] drm/i915: Add lspcon core functions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 04, 2016 at 09:09:06PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-05-03 às 21:44 +0530, Sharma, Shashank escreveu:
> > 
> > On 5/3/2016 9:39 PM, Ville Syrjälä wrote:
> > > 
> > > On Tue, May 03, 2016 at 09:18:49PM +0530, Sharma, Shashank wrote:
> > > > 
> > > > Regards
> > > > Shashank
> > > > 
> > > > On 5/2/2016 7:21 PM, Ville Syrjälä wrote:
> > > > > 
> > > > > On Mon, Apr 04, 2016 at 05:31:46PM +0530, Shashank Sharma
> > > > > wrote:
> > > > > > 
> > > > > > This patch adds lspcon's internal functions, which work
> > > > > > on the probe layer, and indicate the working status of
> > > > > > lspcon, which are mostly:
> > > > > > 
> > > > > > probe: A lspcon device is probed only once, during boot
> > > > > > time, as its always present with the device, next to port.
> > > > > > So the i2c_over_aux channel is alwyas read/writeable if DC is
> > > > > > powered on. If VBT says that this port contains lspcon, we
> > > > > > check and probe the HW to verify and initialize it.
> > > > > > 
> > > > > > get_mode: This function indicates the current mode of
> > > > > > operation
> > > > > > of lspcon (ls or pcon mode)
> > > > > > 
> > > > > > change_mode: This function can change the lspcon's mode of
> > > > > > operation to desired mode.
> > > > > > 
> > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > > > > Signed-off-by: Akashdeep Sharma <Akashdeep.sharma@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/intel_lspcon.c | 145
> > > > > > ++++++++++++++++++++++++++++++++++++
> > > > > >    1 file changed, 145 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c
> > > > > > b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > > > index c3c1cd2..617fe3f 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > > > @@ -61,6 +61,144 @@ static struct intel_lspcon
> > > > > >    	return
> > > > > > enc_to_lspcon(&intel_attached_encoder(connector)->base);
> > > > > >    }
> > > > > > 
> > > > > > +enum lspcon_mode lspcon_get_current_mode(struct intel_lspcon
> > > > > > *lspcon)
> > > > > > +{
> > > > > > +	u8 data;
> > > > > > +	int err = 0;
> > > > > > +	struct intel_digital_port *dig_port =
> > > > > > lspcon_to_dig_port(lspcon);
> > > > > > +	struct i2c_adapter *adapter = &dig_port->dp.aux.ddc;
> > > > > > +
> > > > > > +	/* Read Status: i2c over aux */
> > > > > > +	err = drm_dp_dual_mode_ioa_read(adapter, &data,
> > > > > > +		LSPCON_MODE_CHECK_OFFSET, sizeof(data));
> > > > > > +	if (err < 0) {
> > > > > > +		DRM_ERROR("LSPCON read mode ioa (0x80, 0x41)
> > > > > > failed\n");
> > > > > > +		return lspcon_mode_invalid;
> > > > > > +	}
> > > > > > +
> > > > > > +	DRM_DEBUG_DRIVER("LSPCON mode (0x80, 0x41) = %x\n",
> > > > > > (unsigned int)data);
> > > > > > +	return data & LSPCON_MODE_MASK ? lspcon_mode_pcon :
> > > > > > lspcon_mode_ls;
> > > > > > +}
> > > > > > +
> > > > > > +int lspcon_change_mode(struct intel_lspcon *lspcon,
> > > > > > +	enum lspcon_mode mode, bool force)
> > > > > > +{
> > > > > > +	u8 data;
> > > > > > +	int err;
> > > > > > +	int time_out = 200;
> > > > > > +	enum lspcon_mode current_mode;
> > > > > > +	struct intel_digital_port *dig_port =
> > > > > > lspcon_to_dig_port(lspcon);
> > > > > > +
> > > > > > +	current_mode = lspcon_get_current_mode(lspcon);
> > > > > > +	if (current_mode == lspcon_mode_invalid) {
> > > > > > +		DRM_ERROR("Failed to get current LSPCON
> > > > > > mode\n");
> > > > > > +		return -EFAULT;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (current_mode == mode && !force) {
> > > > > > +		DRM_DEBUG_DRIVER("Current mode = desired
> > > > > > LSPCON mode\n");
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (mode == lspcon_mode_ls)
> > > > > > +		data = ~LSPCON_MODE_MASK;
> > > > > > +	else
> > > > > > +		data = LSPCON_MODE_MASK;
> > > > > > +
> > > > > > +	/* Change mode */
> > > > > > +	err = drm_dp_dual_mode_ioa_write(&dig_port-
> > > > > > >dp.aux.ddc, &data,
> > > > > > +			LSPCON_MODE_CHANGE_OFFSET,
> > > > > > sizeof(data));
> > > > > > +	if (err < 0) {
> > > > > > +		DRM_ERROR("LSPCON mode change failed\n");
> > > > > > +		return err;
> > > > > > +	}
> > > > > > +
> > > > > > +	/*
> > > > > > +	* Confirm mode change by reading the status bit.
> > > > > > +	* Sometimes, it takes a while to change the mode,
> > > > > > +	* so wait and retry until time out or done.
> > > > > > +	*/
> > > > > > +	while (time_out) {
> > > > > > +		current_mode =
> > > > > > lspcon_get_current_mode(lspcon);
> > > > > > +		if (current_mode != mode) {
> > > > > > +			mdelay(10);
> > > > > > +			time_out -= 10;
> > > > > > +		} else {
> > > > > > +			lspcon->mode_of_op = mode;
> > > > > > +			DRM_DEBUG_DRIVER("LSPCON mode
> > > > > > changed to %s\n",
> > > > > > +				mode == lspcon_mode_ls ?
> > > > > > "LS" : "PCON");
> > > > > > +			return 0;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	DRM_ERROR("LSPCON mode change timed out\n");
> > > > > > +	return -EFAULT;
> > > > > > +}
> > > > > I think we probably want to put most of this stuff into the
> > > > > helper. We
> > > > > already have the LSPCON identification there, so having a few
> > > > > helpers
> > > > > to set/get the ls vs. pconn mode would seem appropriate.
> > > > > 
> > > > Actually handling of LS/PCON modes are specific to MCA chips, and
> > > > some
> > > > of the mode change mechanism and few other control stuff may not
> > > > be same
> > > > on parade or some other vendor's chip, so I am not sure if we
> > > > should
> > > > create a helper for something which is specific to this chip. You
> > > > suggest so ?
> > > Well, if we already put the detection into the helper we already
> > > have
> > > some vendor specific stuff there, no? Or would the other vendor's
> > > chips
> > > be identifiable in the same way?
> > > 
> > > Anyways, I presume someone else than Intel might want to use these
> > > same
> > > chips in their products, so having the support in a central place
> > > would
> > > seem like a good idea. If we start to see incompatble LSPCON chips
> > > from
> > > multiple vendors we'll need to rethink how we support them all, but
> > > until we know how exactly they differ it's rather impossible to
> > > design
> > > the helper to deal with them. So as a first step I'd stuff it all
> > > into
> > > the helper.
> > > 
> > Yes, makes more sense this way. I will try to go through parade
> > specs 
> > once, and see if I can find something which needs immediate
> > discussion 
> > here. Else, I will move this into the central layer.
> 
> The big problem with LSPCON is that it uses bits that are reserved by
> the VESA specs, such as bit 3 of I2C address 80h offset 10h. Nothing
> prevents VESA from defining an actual meaning to that bit that will be
> incompatible with LSPCON.

Well, when/if that happens we'll have to change the way the detection
works, eg. just leave it up to the driver or something. As always, it's
hard to make predictions, especially about the future.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx