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().
> + 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
--
Ville Syrjälä
Intel
