On Thu, 03 Mar 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
>
>> }
>> }
>> 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, Intel Open Source Graphics Center
