Re: [PATCH 3/5] drm/i915: index gmbus tables using the pin pair number

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 27 Mar 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 27, 2015 at 12:20:21AM +0200, Jani Nikula wrote:
>> Index the gmbus tables directly using the pin instead of having a
>> confusing "port = i + 1" mapping. This finishes off removing the "gmbus
>> port" as a notion, and leaves us with just the "gmbus pin".
>> 
>> As pin 0 is invalid by definition and the gmbus tables will have a gap
>> at that index, add pin validity check to all the loops. This will be
>> benefitial for supporting platforms that have different numbers of pins,
>> or gaps.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  3 +-
>>  drivers/gpu/drm/i915/i915_reg.h  |  2 +-
>>  drivers/gpu/drm/i915/intel_i2c.c | 65 +++++++++++++++++++++++-----------------
>>  3 files changed, 40 insertions(+), 30 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 3ba5de19e039..0c6024101eb9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1574,8 +1574,7 @@ struct drm_i915_private {
>>  
>>  	struct i915_virtual_gpu vgpu;
>>  
>> -	struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>> -
>> +	struct intel_gmbus gmbus[GMBUS_PIN_MAX];
>>  
>>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>>  	 * controller on different i2c buses. */
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b6113c9a803b..cdc071cff001 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1797,7 +1797,7 @@ enum skl_disp_power_wells {
>>  #define   GMBUS_PIN_DPB		5 /* SDVO, HDMIB */
>>  #define   GMBUS_PIN_DPD		6 /* HDMID */
>>  #define   GMBUS_PIN_RESERVED	7 /* 7 reserved */
>> -#define   GMBUS_NUM_PORTS	(GMBUS_PIN_DPD - GMBUS_PIN_SSC + 1)
>> +#define   GMBUS_PIN_MAX		7 /* not inclusive */
>
> PIN_MAX makes me think it's inclusive. NUM_PINS maybe?

I wanted to clearly distinguish it from the old NUM_PORTS which included
only the valid ones, while this is used for the gmbus[] array field size
in struct drm_i915_private only.

An alternative would be to make GMBUS_PIN_MAX inclusive and to use
gmbus[GMBUS_PIN_MAX + 1] in struct drm_i915_private.

> w/ or w/o that particular bikeshed patches 1-4 look fine to me, so:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review.

BR,
Jani.

>
>>  #define GMBUS1			0x5104 /* command/status */
>>  #define   GMBUS_SW_CLR_INT	(1<<31)
>>  #define   GMBUS_SW_RDY		(1<<30)
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index b0003a2bd854..ff47a8fdcb6d 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -34,18 +34,19 @@
>>  #include <drm/i915_drm.h>
>>  #include "i915_drv.h"
>>  
>> -struct gmbus_port {
>> +struct gmbus_pin {
>>  	const char *name;
>>  	int reg;
>>  };
>>  
>> -static const struct gmbus_port gmbus_ports[] = {
>> -	{ "ssc", GPIOB },
>> -	{ "vga", GPIOA },
>> -	{ "panel", GPIOC },
>> -	{ "dpc", GPIOD },
>> -	{ "dpb", GPIOE },
>> -	{ "dpd", GPIOF },
>> +/* Map gmbus pin pairs to names and registers. */
>> +static const struct gmbus_pin gmbus_pins[] = {
>> +	[GMBUS_PIN_SSC] = { "ssc", GPIOB },
>> +	[GMBUS_PIN_VGADDC] = { "vga", GPIOA },
>> +	[GMBUS_PIN_PANEL] = { "panel", GPIOC },
>> +	[GMBUS_PIN_DPC] = { "dpc", GPIOD },
>> +	[GMBUS_PIN_DPB] = { "dpb", GPIOE },
>> +	[GMBUS_PIN_DPD] = { "dpd", GPIOF },
>>  };
>>  
>>  /* Intel GPIO access functions */
>> @@ -182,15 +183,14 @@ intel_gpio_post_xfer(struct i2c_adapter *adapter)
>>  }
>>  
>>  static void
>> -intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>> +intel_gpio_setup(struct intel_gmbus *bus, unsigned int pin)
>>  {
>>  	struct drm_i915_private *dev_priv = bus->dev_priv;
>>  	struct i2c_algo_bit_data *algo;
>>  
>>  	algo = &bus->bit_algo;
>>  
>> -	/* -1 to map pin pair to gmbus index */
>> -	bus->gpio_reg = dev_priv->gpio_mmio_base + gmbus_ports[pin - 1].reg;
>> +	bus->gpio_reg = dev_priv->gpio_mmio_base + gmbus_pins[pin].reg;
>>  
>>  	bus->adapter.algo_data = algo;
>>  	algo->setsda = set_data;
>> @@ -517,7 +517,9 @@ static const struct i2c_algorithm gmbus_algorithm = {
>>  int intel_setup_gmbus(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	int ret, i;
>> +	struct intel_gmbus *bus;
>> +	unsigned int pin;
>> +	int ret;
>>  
>>  	if (HAS_PCH_NOP(dev))
>>  		return 0;
>> @@ -531,16 +533,18 @@ int intel_setup_gmbus(struct drm_device *dev)
>>  	mutex_init(&dev_priv->gmbus_mutex);
>>  	init_waitqueue_head(&dev_priv->gmbus_wait_queue);
>>  
>> -	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
>> -		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>> -		u32 port = i + 1; /* +1 to map gmbus index to pin pair */
>> +	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
>> +		if (!intel_gmbus_is_valid_pin(pin))
>> +			continue;
>> +
>> +		bus = &dev_priv->gmbus[pin];
>>  
>>  		bus->adapter.owner = THIS_MODULE;
>>  		bus->adapter.class = I2C_CLASS_DDC;
>>  		snprintf(bus->adapter.name,
>>  			 sizeof(bus->adapter.name),
>>  			 "i915 gmbus %s",
>> -			 gmbus_ports[i].name);
>> +			 gmbus_pins[pin].name);
>>  
>>  		bus->adapter.dev.parent = &dev->pdev->dev;
>>  		bus->dev_priv = dev_priv;
>> @@ -548,13 +552,13 @@ int intel_setup_gmbus(struct drm_device *dev)
>>  		bus->adapter.algo = &gmbus_algorithm;
>>  
>>  		/* By default use a conservative clock rate */
>> -		bus->reg0 = port | GMBUS_RATE_100KHZ;
>> +		bus->reg0 = pin | GMBUS_RATE_100KHZ;
>>  
>>  		/* gmbus seems to be broken on i830 */
>>  		if (IS_I830(dev))
>>  			bus->force_bit = 1;
>>  
>> -		intel_gpio_setup(bus, port);
>> +		intel_gpio_setup(bus, pin);
>>  
>>  		ret = i2c_add_adapter(&bus->adapter);
>>  		if (ret)
>> @@ -566,8 +570,11 @@ int intel_setup_gmbus(struct drm_device *dev)
>>  	return 0;
>>  
>>  err:
>> -	while (--i) {
>> -		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>> +	while (--pin) {
>> +		if (!intel_gmbus_is_valid_pin(pin))
>> +			continue;
>> +
>> +		bus = &dev_priv->gmbus[pin];
>>  		i2c_del_adapter(&bus->adapter);
>>  	}
>>  	return ret;
>> @@ -576,10 +583,10 @@ err:
>>  struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
>>  					    unsigned int pin)
>>  {
>> -	WARN_ON(!intel_gmbus_is_valid_pin(pin));
>> -	/* -1 to map pin pair to gmbus index */
>> -	return (intel_gmbus_is_valid_pin(pin)) ?
>> -		&dev_priv->gmbus[pin - 1].adapter : NULL;
>> +	if (WARN_ON(!intel_gmbus_is_valid_pin(pin)))
>> +		return NULL;
>> +
>> +	return &dev_priv->gmbus[pin].adapter;
>>  }
>>  
>>  void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
>> @@ -602,10 +609,14 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>>  void intel_teardown_gmbus(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	int i;
>> +	struct intel_gmbus *bus;
>> +	unsigned int pin;
>> +
>> +	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
>> +		if (!intel_gmbus_is_valid_pin(pin))
>> +			continue;
>>  
>> -	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
>> -		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>> +		bus = &dev_priv->gmbus[pin];
>>  		i2c_del_adapter(&bus->adapter);
>>  	}
>>  }
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx