Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <34b4f396-ecf3-576d-69e5-f8eac2a5d488@redhat.com>
Date:   Thu, 2 Nov 2023 16:47:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind
 the driver's back
Content-Language: en-US, nl
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Jani Nikula <jani.nikula@intel.com>,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Cc:     Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
References: <20231102151228.668842-1-andriy.shevchenko@linux.intel.com>
 <20231102151228.668842-15-andriy.shevchenko@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20231102151228.668842-15-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 11/2/23 16:12, Andy Shevchenko wrote:
> It's a dirty hack in the driver that pokes GPIO registers behind
> the driver's back. Moreoever it might be problematic as simultaneous
> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> cherryview: prevent concurrent access to GPIO controllers") for
> the details. Taking all this into consideration replace the hack
> with proper GPIO APIs being used.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index b1736c1301ea..ffc65c943b11 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
>  #define CHV_GPIO_IDX_START_SW		100
>  #define CHV_GPIO_IDX_START_SE		198
>  
> -#define CHV_VBT_MAX_PINS_PER_FMLY	15
> -
> -#define CHV_GPIO_PAD_CFG0(f, i)		(0x4400 + (f) * 0x400 + (i) * 8)
> -#define  CHV_GPIO_GPIOEN		(1 << 15)
> -#define  CHV_GPIO_GPIOCFG_GPIO		(0 << 8)
> -#define  CHV_GPIO_GPIOCFG_GPO		(1 << 8)
> -#define  CHV_GPIO_GPIOCFG_GPI		(2 << 8)
> -#define  CHV_GPIO_GPIOCFG_HIZ		(3 << 8)
> -#define  CHV_GPIO_GPIOTXSTATE(state)	((!!(state)) << 1)
> -
> -#define CHV_GPIO_PAD_CFG1(f, i)		(0x4400 + (f) * 0x400 + (i) * 8 + 4)
> -#define  CHV_GPIO_CFGLOCK		(1 << 31)
> -
>  /* ICL DSI Display GPIO Pins */
>  #define  ICL_GPIO_DDSP_HPD_A		0
>  #define  ICL_GPIO_L_VDDEN_1		1
> @@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector *connector,
>  			       u8 gpio_source, u8 gpio_index, bool value)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> -	u16 cfg0, cfg1;
> -	u16 family_num;
> -	u8 port;
>  
>  	if (connector->panel.vbt.dsi.seq_version >= 3) {
>  		if (gpio_index >= CHV_GPIO_IDX_START_SE) {
>  			/* XXX: it's unclear whether 255->57 is part of SE. */
> -			gpio_index -= CHV_GPIO_IDX_START_SE;
> -			port = CHV_IOSF_PORT_GPIO_SE;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
> +					     gpio_index - CHV_GPIO_IDX_START_SW, value);

The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - CHV_GPIO_IDX_START_SE".

Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to compile ...

Regards,

Hans


>  		} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
> -			gpio_index -= CHV_GPIO_IDX_START_SW;
> -			port = CHV_IOSF_PORT_GPIO_SW;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
> +					     gpio_index - CHV_GPIO_IDX_START_SW, value);
>  		} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
> -			gpio_index -= CHV_GPIO_IDX_START_E;
> -			port = CHV_IOSF_PORT_GPIO_E;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
> +					     gpio_index - CHV_GPIO_IDX_START_E, value);
>  		} else {
> -			port = CHV_IOSF_PORT_GPIO_N;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> +					     gpio_index - CHV_GPIO_IDX_START_N, value);
>  		}
>  	} else {
>  		/* XXX: The spec is unclear about CHV GPIO on seq v2 */
> @@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector *connector,
>  			return;
>  		}
>  
> -		port = CHV_IOSF_PORT_GPIO_N;
> +		soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> +				     gpio_index - CHV_GPIO_IDX_START_N, value);
>  	}
> -
> -	family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
> -	gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
> -
> -	cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
> -	cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
> -
> -	vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> -	vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
> -	vlv_iosf_sb_write(dev_priv, port, cfg0,
> -			  CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
> -			  CHV_GPIO_GPIOTXSTATE(value));
> -	vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
>  }
>  
>  static void bxt_gpio_set_value(struct intel_connector *connector,