diff mbox series

[4/5] drm/i915/gmbus: alloc intel_gmbus dynamically

Message ID 20220303181931.1661767-4-jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/5] drm/i915/gmbus: combine gmbus pin lookups to one function | expand

Commit Message

Jani Nikula March 3, 2022, 6:19 p.m. UTC 
Allocate the individual intel_gmbus structs dynamically. This lets us
hide struct intel_gmbus inside intel_gmbus.c completely. Also use the
cleanup function on the error path to avoid duplication.

Leave #include <linux/i2c.h> in i915_drv.h for now, as it pulls in a
bunch of implicit dependencies.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_gmbus.c | 42 +++++++++++++++-------
 drivers/gpu/drm/i915/i915_drv.h            | 14 ++------
 2 files changed, 31 insertions(+), 25 deletions(-)

Comments

Ville Syrjälä March 3, 2022, 8:17 p.m. UTC | #1 
On Thu, Mar 03, 2022 at 08:19:30PM +0200, Jani Nikula wrote:
> Allocate the individual intel_gmbus structs dynamically. This lets us
> hide struct intel_gmbus inside intel_gmbus.c completely. Also use the
> cleanup function on the error path to avoid duplication.
> 
> Leave #include <linux/i2c.h> in i915_drv.h for now, as it pulls in a
> bunch of implicit dependencies.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_gmbus.c | 42 +++++++++++++++-------
>  drivers/gpu/drm/i915/i915_drv.h            | 14 ++------
>  2 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
> index fd908e524875..2bb3494b93e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
> @@ -38,6 +38,16 @@
>  #include "intel_display_types.h"
>  #include "intel_gmbus.h"
>  
> +struct intel_gmbus {
> +	struct i2c_adapter adapter;
> +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
> +	u32 force_bit;
> +	u32 reg0;
> +	i915_reg_t gpio_reg;
> +	struct i2c_algo_bit_data bit_algo;
> +	struct drm_i915_private *dev_priv;
> +};
> +
>  struct gmbus_pin {
>  	const char *name;
>  	enum i915_gpio gpio;
> @@ -881,7 +891,11 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>  		if (!gmbus_pin)
>  			continue;
>  
> -		bus = &dev_priv->gmbus[pin];
> +		bus = kzalloc(sizeof(*bus), GFP_KERNEL);
> +		if (!bus) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
>  
>  		bus->adapter.owner = THIS_MODULE;
>  		bus->adapter.class = I2C_CLASS_DDC;
> @@ -911,8 +925,12 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>  		intel_gpio_setup(bus, GPIO(gmbus_pin->gpio));
>  
>  		ret = i2c_add_adapter(&bus->adapter);
> -		if (ret)
> +		if (ret) {
> +			kfree(bus);
>  			goto err;
> +		}
> +
> +		dev_priv->gmbus[pin] = bus;
>  	}
>  
>  	intel_gmbus_reset(dev_priv);
> @@ -920,24 +938,19 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>  	return 0;
>  
>  err:
> -	while (pin--) {
> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
> -			continue;
> +	intel_gmbus_teardown(dev_priv);
>  
> -		bus = &dev_priv->gmbus[pin];
> -		i2c_del_adapter(&bus->adapter);
> -	}
>  	return ret;
>  }
>  
>  struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
>  					    unsigned int pin)
>  {
> -	if (drm_WARN_ON(&dev_priv->drm,
> -			!intel_gmbus_is_valid_pin(dev_priv, pin)))
> +	if (drm_WARN_ON(&dev_priv->drm, pin >= ARRAY_SIZE(dev_priv->gmbus) ||
> +			!dev_priv->gmbus[pin]))
>  		return NULL;
>  
> -	return &dev_priv->gmbus[pin].adapter;
> +	return &dev_priv->gmbus[pin]->adapter;
>  }
>  
>  void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
> @@ -969,10 +982,13 @@ void intel_gmbus_teardown(struct drm_i915_private *dev_priv)
>  	unsigned int pin;
>  
>  	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
> +		bus = dev_priv->gmbus[pin];

nit: Would like the 'bus' variable to be declared inside the loop.
Same for intel_gmbus_setup().

> +		if (!bus)
>  			continue;
>  
> -		bus = &dev_priv->gmbus[pin];
>  		i2c_del_adapter(&bus->adapter);
> +
> +		kfree(bus);
> +		dev_priv->gmbus[pin] = NULL;

I see we don't actually check intel_gmbus_setup() return value at all so
intel_gmbus_teardown() can get called twice. So this NULLing is essential
should intel_gmbus_setup() ever fail.

Series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 457bc1993d19..869a2bda347b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -35,7 +35,6 @@
>  #include <asm/hypervisor.h>
>  
>  #include <linux/i2c.h>
> -#include <linux/i2c-algo-bit.h>
>  #include <linux/intel-iommu.h>
>  #include <linux/pm_qos.h>
>  
> @@ -99,6 +98,7 @@ struct intel_dpll_funcs;
>  struct intel_encoder;
>  struct intel_fbdev;
>  struct intel_fdi_funcs;
> +struct intel_gmbus;
>  struct intel_hotplug_funcs;
>  struct intel_initial_plane_config;
>  struct intel_limit;
> @@ -231,16 +231,6 @@ struct i915_drrs {
>  #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7)
>  #define QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8)
>  
> -struct intel_gmbus {
> -	struct i2c_adapter adapter;
> -#define GMBUS_FORCE_BIT_RETRY (1U << 31)
> -	u32 force_bit;
> -	u32 reg0;
> -	i915_reg_t gpio_reg;
> -	struct i2c_algo_bit_data bit_algo;
> -	struct drm_i915_private *dev_priv;
> -};
> -
>  struct i915_suspend_saved_registers {
>  	u32 saveDSPARB;
>  	u32 saveSWF0[16];
> @@ -510,7 +500,7 @@ struct drm_i915_private {
>  
>  	struct intel_dmc dmc;
>  
> -	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
> +	struct intel_gmbus *gmbus[GMBUS_NUM_PINS];
>  
>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>  	 * controller on different i2c buses. */
> -- 
> 2.30.2
Jani Nikula March 4, 2022, 9:55 a.m. UTC | #2 
On Thu, 03 Mar 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 03, 2022 at 08:19:30PM +0200, Jani Nikula wrote:
>> Allocate the individual intel_gmbus structs dynamically. This lets us
>> hide struct intel_gmbus inside intel_gmbus.c completely. Also use the
>> cleanup function on the error path to avoid duplication.
>> 
>> Leave #include <linux/i2c.h> in i915_drv.h for now, as it pulls in a
>> bunch of implicit dependencies.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_gmbus.c | 42 +++++++++++++++-------
>>  drivers/gpu/drm/i915/i915_drv.h            | 14 ++------
>>  2 files changed, 31 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index fd908e524875..2bb3494b93e2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -38,6 +38,16 @@
>>  #include "intel_display_types.h"
>>  #include "intel_gmbus.h"
>>  
>> +struct intel_gmbus {
>> +	struct i2c_adapter adapter;
>> +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>> +	u32 force_bit;
>> +	u32 reg0;
>> +	i915_reg_t gpio_reg;
>> +	struct i2c_algo_bit_data bit_algo;
>> +	struct drm_i915_private *dev_priv;
>> +};
>> +
>>  struct gmbus_pin {
>>  	const char *name;
>>  	enum i915_gpio gpio;
>> @@ -881,7 +891,11 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  		if (!gmbus_pin)
>>  			continue;
>>  
>> -		bus = &dev_priv->gmbus[pin];
>> +		bus = kzalloc(sizeof(*bus), GFP_KERNEL);
>> +		if (!bus) {
>> +			ret = -ENOMEM;
>> +			goto err;
>> +		}
>>  
>>  		bus->adapter.owner = THIS_MODULE;
>>  		bus->adapter.class = I2C_CLASS_DDC;
>> @@ -911,8 +925,12 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  		intel_gpio_setup(bus, GPIO(gmbus_pin->gpio));
>>  
>>  		ret = i2c_add_adapter(&bus->adapter);
>> -		if (ret)
>> +		if (ret) {
>> +			kfree(bus);
>>  			goto err;
>> +		}
>> +
>> +		dev_priv->gmbus[pin] = bus;
>>  	}
>>  
>>  	intel_gmbus_reset(dev_priv);
>> @@ -920,24 +938,19 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  	return 0;
>>  
>>  err:
>> -	while (pin--) {
>> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>> -			continue;
>> +	intel_gmbus_teardown(dev_priv);
>>  
>> -		bus = &dev_priv->gmbus[pin];
>> -		i2c_del_adapter(&bus->adapter);
>> -	}
>>  	return ret;
>>  }
>>  
>>  struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
>>  					    unsigned int pin)
>>  {
>> -	if (drm_WARN_ON(&dev_priv->drm,
>> -			!intel_gmbus_is_valid_pin(dev_priv, pin)))
>> +	if (drm_WARN_ON(&dev_priv->drm, pin >= ARRAY_SIZE(dev_priv->gmbus) ||
>> +			!dev_priv->gmbus[pin]))
>>  		return NULL;
>>  
>> -	return &dev_priv->gmbus[pin].adapter;
>> +	return &dev_priv->gmbus[pin]->adapter;
>>  }
>>  
>>  void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>> @@ -969,10 +982,13 @@ void intel_gmbus_teardown(struct drm_i915_private *dev_priv)
>>  	unsigned int pin;
>>  
>>  	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
>> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>> +		bus = dev_priv->gmbus[pin];
>
> nit: Would like the 'bus' variable to be declared inside the loop.
> Same for intel_gmbus_setup().

Thanks for the review, pushed to din. I'll follow up with a few
cleanups.

BR,
Jani.


>
>> +		if (!bus)
>>  			continue;
>>  
>> -		bus = &dev_priv->gmbus[pin];
>>  		i2c_del_adapter(&bus->adapter);
>> +
>> +		kfree(bus);
>> +		dev_priv->gmbus[pin] = NULL;
>
> I see we don't actually check intel_gmbus_setup() return value at all so
> intel_gmbus_teardown() can get called twice. So this NULLing is essential
> should intel_gmbus_setup() ever fail.
>
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>>  	}
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 457bc1993d19..869a2bda347b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -35,7 +35,6 @@
>>  #include <asm/hypervisor.h>
>>  
>>  #include <linux/i2c.h>
>> -#include <linux/i2c-algo-bit.h>
>>  #include <linux/intel-iommu.h>
>>  #include <linux/pm_qos.h>
>>  
>> @@ -99,6 +98,7 @@ struct intel_dpll_funcs;
>>  struct intel_encoder;
>>  struct intel_fbdev;
>>  struct intel_fdi_funcs;
>> +struct intel_gmbus;
>>  struct intel_hotplug_funcs;
>>  struct intel_initial_plane_config;
>>  struct intel_limit;
>> @@ -231,16 +231,6 @@ struct i915_drrs {
>>  #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7)
>>  #define QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8)
>>  
>> -struct intel_gmbus {
>> -	struct i2c_adapter adapter;
>> -#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>> -	u32 force_bit;
>> -	u32 reg0;
>> -	i915_reg_t gpio_reg;
>> -	struct i2c_algo_bit_data bit_algo;
>> -	struct drm_i915_private *dev_priv;
>> -};
>> -
>>  struct i915_suspend_saved_registers {
>>  	u32 saveDSPARB;
>>  	u32 saveSWF0[16];
>> @@ -510,7 +500,7 @@ struct drm_i915_private {
>>  
>>  	struct intel_dmc dmc;
>>  
>> -	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
>> +	struct intel_gmbus *gmbus[GMBUS_NUM_PINS];
>>  
>>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>>  	 * controller on different i2c buses. */
>> -- 
>> 2.30.2
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
index fd908e524875..2bb3494b93e2 100644
--- a/drivers/gpu/drm/i915/display/intel_gmbus.c
+++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
@@ -38,6 +38,16 @@ 
 #include "intel_display_types.h"
 #include "intel_gmbus.h"
 
+struct intel_gmbus {
+	struct i2c_adapter adapter;
+#define GMBUS_FORCE_BIT_RETRY (1U << 31)
+	u32 force_bit;
+	u32 reg0;
+	i915_reg_t gpio_reg;
+	struct i2c_algo_bit_data bit_algo;
+	struct drm_i915_private *dev_priv;
+};
+
 struct gmbus_pin {
 	const char *name;
 	enum i915_gpio gpio;
@@ -881,7 +891,11 @@  int intel_gmbus_setup(struct drm_i915_private *dev_priv)
 		if (!gmbus_pin)
 			continue;
 
-		bus = &dev_priv->gmbus[pin];
+		bus = kzalloc(sizeof(*bus), GFP_KERNEL);
+		if (!bus) {
+			ret = -ENOMEM;
+			goto err;
+		}
 
 		bus->adapter.owner = THIS_MODULE;
 		bus->adapter.class = I2C_CLASS_DDC;
@@ -911,8 +925,12 @@  int intel_gmbus_setup(struct drm_i915_private *dev_priv)
 		intel_gpio_setup(bus, GPIO(gmbus_pin->gpio));
 
 		ret = i2c_add_adapter(&bus->adapter);
-		if (ret)
+		if (ret) {
+			kfree(bus);
 			goto err;
+		}
+
+		dev_priv->gmbus[pin] = bus;
 	}
 
 	intel_gmbus_reset(dev_priv);
@@ -920,24 +938,19 @@  int intel_gmbus_setup(struct drm_i915_private *dev_priv)
 	return 0;
 
 err:
-	while (pin--) {
-		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
-			continue;
+	intel_gmbus_teardown(dev_priv);
 
-		bus = &dev_priv->gmbus[pin];
-		i2c_del_adapter(&bus->adapter);
-	}
 	return ret;
 }
 
 struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
 					    unsigned int pin)
 {
-	if (drm_WARN_ON(&dev_priv->drm,
-			!intel_gmbus_is_valid_pin(dev_priv, pin)))
+	if (drm_WARN_ON(&dev_priv->drm, pin >= ARRAY_SIZE(dev_priv->gmbus) ||
+			!dev_priv->gmbus[pin]))
 		return NULL;
 
-	return &dev_priv->gmbus[pin].adapter;
+	return &dev_priv->gmbus[pin]->adapter;
 }
 
 void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
@@ -969,10 +982,13 @@  void intel_gmbus_teardown(struct drm_i915_private *dev_priv)
 	unsigned int pin;
 
 	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
-		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
+		bus = dev_priv->gmbus[pin];
+		if (!bus)
 			continue;
 
-		bus = &dev_priv->gmbus[pin];
 		i2c_del_adapter(&bus->adapter);
+
+		kfree(bus);
+		dev_priv->gmbus[pin] = NULL;
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 457bc1993d19..869a2bda347b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -35,7 +35,6 @@ 
 #include <asm/hypervisor.h>
 
 #include <linux/i2c.h>
-#include <linux/i2c-algo-bit.h>
 #include <linux/intel-iommu.h>
 #include <linux/pm_qos.h>
 
@@ -99,6 +98,7 @@  struct intel_dpll_funcs;
 struct intel_encoder;
 struct intel_fbdev;
 struct intel_fdi_funcs;
+struct intel_gmbus;
 struct intel_hotplug_funcs;
 struct intel_initial_plane_config;
 struct intel_limit;
@@ -231,16 +231,6 @@  struct i915_drrs {
 #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7)
 #define QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8)
 
-struct intel_gmbus {
-	struct i2c_adapter adapter;
-#define GMBUS_FORCE_BIT_RETRY (1U << 31)
-	u32 force_bit;
-	u32 reg0;
-	i915_reg_t gpio_reg;
-	struct i2c_algo_bit_data bit_algo;
-	struct drm_i915_private *dev_priv;
-};
-
 struct i915_suspend_saved_registers {
 	u32 saveDSPARB;
 	u32 saveSWF0[16];
@@ -510,7 +500,7 @@  struct drm_i915_private {
 
 	struct intel_dmc dmc;
 
-	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
+	struct intel_gmbus *gmbus[GMBUS_NUM_PINS];
 
 	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
 	 * controller on different i2c buses. */