diff mbox

[v1] drm/i915/bxt: use NULL for GPIO connection ID

Message ID 20170221151902.152620-1-andriy.shevchenko@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andy Shevchenko Feb. 21, 2017, 3:19 p.m. UTC
The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
support") enables GPIO support for Broxton based platforms.

While using that API we might get into troubles in the future, because
we can't rely on label "panel" in the driver since vendor firmware might
provide any GPIO pin there, e.g. "reset", and even mark it in _DSD (in
which case the request will fail).

To avoid inconsistency and potential issues we have two options:
a) generate GPIO ACPI mapping table and supply it via
acpi_dev_add_driver_gpios(), or
b) just pass NULL as connection ID.

The b) approach is much simplier and would work since the driver relies
on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
requesting GPIO, is going to be stricter, and supplying non-NULL
connection ID when neither _DSD, nor GPIO ACPI mapping is present, will
make request fail.

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jani Nikula Feb. 21, 2017, 4:26 p.m. UTC | #1
On Tue, 21 Feb 2017, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> support") enables GPIO support for Broxton based platforms.
>
> While using that API we might get into troubles in the future, because
> we can't rely on label "panel" in the driver since vendor firmware might
> provide any GPIO pin there, e.g. "reset", and even mark it in _DSD (in
> which case the request will fail).
>
> To avoid inconsistency and potential issues we have two options:
> a) generate GPIO ACPI mapping table and supply it via
> acpi_dev_add_driver_gpios(), or
> b) just pass NULL as connection ID.
>
> The b) approach is much simplier and would work since the driver relies
> on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
> requesting GPIO, is going to be stricter, and supplying non-NULL
> connection ID when neither _DSD, nor GPIO ACPI mapping is present, will
> make request fail.

The patch version log in the commit suggests otherwise; we'd tried and
failed with NULL, until Mika realized passing "panel" works:

    v2 by Mika: switch *NULL* to *"panel"* when requesting gpio for MIPI/DSI
    panel.

See also [1]. What has changed since then that should make this work
now? We shouldn't apply until we get Tested-by's.


BR,
Jani.


[1] http://mid.mail-archive.com/1480597671.26172.82.camel@intel.com


>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 8f683b8b1816..493d5ec2b53a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -315,7 +315,7 @@ static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
>  
>  	if (!gpio_desc) {
>  		gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
> -						 "panel", gpio_index,
> +						 NULL, gpio_index,
>  						 value ? GPIOD_OUT_LOW :
>  						 GPIOD_OUT_HIGH);
Andy Shevchenko Feb. 21, 2017, 4:52 p.m. UTC | #2
On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> On Tue, 21 Feb 2017, Andy Shevchenko <andriy.shevchenko@linux.intel.co
> m> wrote:
> > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > support") enables GPIO support for Broxton based platforms.
> > 
> > While using that API we might get into troubles in the future,
> > because
> > we can't rely on label "panel" in the driver since vendor firmware
> > might
> > provide any GPIO pin there, e.g. "reset", and even mark it in _DSD
> > (in
> > which case the request will fail).
> > 
> > To avoid inconsistency and potential issues we have two options:
> > a) generate GPIO ACPI mapping table and supply it via
> > acpi_dev_add_driver_gpios(), or
> > b) just pass NULL as connection ID.
> > 
> > The b) approach is much simplier and would work since the driver
> > relies
> > on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
> > requesting GPIO, is going to be stricter, and supplying non-NULL
> > connection ID when neither _DSD, nor GPIO ACPI mapping is present,
> > will
> > make request fail.
> 
> The patch version log in the commit suggests otherwise; we'd tried and
> failed with NULL,

Can I see DSDT excerpts of the platform that fails?

>  until Mika realized passing "panel" works:
> 
>     v2 by Mika: switch *NULL* to *"panel"* when requesting gpio for
> MIPI/DSI
>     panel.

> 
> See also [1]. What has changed since then that should make this work
> now? We shouldn't apply until we get Tested-by's.

Not changed yet, but *going to be*. See my repository here [2].
To fix the mess with GPIO ACPI stuff we are going to make request
stricter as I pointed in commit message above, i.e. asking for a GPIO by
connection ID without _DSD present will guarantee -ENOENT since it will
be no fallback to _CRS. You may follow discussion in our internal
mailing list for drivers.

> [1] http://mid.mail-archive.com/1480597671.26172.82.camel@intel.com

[2] http://bitbucket.org/andy-shev/linux/branch/topic%2Fuart%2frpm
Daniel Vetter Feb. 26, 2017, 9:45 p.m. UTC | #3
On Tue, Feb 21, 2017 at 06:52:24PM +0200, Andy Shevchenko wrote:
> On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> > On Tue, 21 Feb 2017, Andy Shevchenko <andriy.shevchenko@linux.intel.co
> > m> wrote:
> > > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > > support") enables GPIO support for Broxton based platforms.
> > > 
> > > While using that API we might get into troubles in the future,
> > > because
> > > we can't rely on label "panel" in the driver since vendor firmware
> > > might
> > > provide any GPIO pin there, e.g. "reset", and even mark it in _DSD
> > > (in
> > > which case the request will fail).
> > > 
> > > To avoid inconsistency and potential issues we have two options:
> > > a) generate GPIO ACPI mapping table and supply it via
> > > acpi_dev_add_driver_gpios(), or
> > > b) just pass NULL as connection ID.
> > > 
> > > The b) approach is much simplier and would work since the driver
> > > relies
> > > on GPIO indeces only. Moreover, the _CRS fallback mechanism, when
> > > requesting GPIO, is going to be stricter, and supplying non-NULL
> > > connection ID when neither _DSD, nor GPIO ACPI mapping is present,
> > > will
> > > make request fail.
> > 
> > The patch version log in the commit suggests otherwise; we'd tried and
> > failed with NULL,
> 
> Can I see DSDT excerpts of the platform that fails?
> 
> >  until Mika realized passing "panel" works:
> > 
> >     v2 by Mika: switch *NULL* to *"panel"* when requesting gpio for
> > MIPI/DSI
> >     panel.
> 
> > 
> > See also [1]. What has changed since then that should make this work
> > now? We shouldn't apply until we get Tested-by's.
> 
> Not changed yet, but *going to be*. See my repository here [2].
> To fix the mess with GPIO ACPI stuff we are going to make request
> stricter as I pointed in commit message above, i.e. asking for a GPIO by
> connection ID without _DSD present will guarantee -ENOENT since it will
> be no fallback to _CRS. You may follow discussion in our internal
> mailing list for drivers.

Why exactly is this being discussed on an internal mailing list? Upstream
happens in public ...
-Daniel
Andy Shevchenko March 7, 2017, 10:48 a.m. UTC | #4
On Sun, 2017-02-26 at 22:45 +0100, Daniel Vetter wrote:
> On Tue, Feb 21, 2017 at 06:52:24PM +0200, Andy Shevchenko wrote:
> > On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> > > On Tue, 21 Feb 2017, Andy Shevchenko <andriy.shevchenko@linux.inte
> > > l.co
> > > m> wrote:
> > > > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > > > support") enables GPIO support for Broxton based platforms.
> > > > 
> > > > While using that API we might get into troubles in the future,
> > > > because
> > > > we can't rely on label "panel" in the driver since vendor
> > > > firmware
> > > > might
> > > > provide any GPIO pin there, e.g. "reset", and even mark it in
> > > > _DSD
> > > > (in
> > > > which case the request will fail).
> > > > 
> > > > To avoid inconsistency and potential issues we have two options:
> > > > a) generate GPIO ACPI mapping table and supply it via
> > > > acpi_dev_add_driver_gpios(), or
> > > > b) just pass NULL as connection ID.
> > > > 
> > > > The b) approach is much simplier and would work since the driver
> > > > relies
> > > > on GPIO indeces only. Moreover, the _CRS fallback mechanism,
> > > > when
> > > > requesting GPIO, is going to be stricter, and supplying non-NULL
> > > > connection ID when neither _DSD, nor GPIO ACPI mapping is
> > > > present,
> > > > will
> > > > make request fail.
> > > 
> > > The patch version log in the commit suggests otherwise; we'd tried
> > > and
> > > failed with NULL,
> > 
> > Can I see DSDT excerpts of the platform that fails?
> > 
> > >  until Mika realized passing "panel" works:
> > > 
> > >     v2 by Mika: switch *NULL* to *"panel"* when requesting gpio
> > > for
> > > MIPI/DSI
> > >     panel.
> > > 
> > > See also [1]. What has changed since then that should make this
> > > work
> > > now? We shouldn't apply until we get Tested-by's.
> > 
> > Not changed yet, but *going to be*. See my repository here [2].
> > To fix the mess with GPIO ACPI stuff we are going to make request
> > stricter as I pointed in commit message above, i.e. asking for a
> > GPIO by
> > connection ID without _DSD present will guarantee -ENOENT since it
> > will
> > be no fallback to _CRS. You may follow discussion in our internal
> > mailing list for drivers.
> 
> Why exactly is this being discussed on an internal mailing list?
> Upstream
> happens in public ...

It was a prelininary discussion and it's sad you didn't notice it.

Nevertheless, Hans started it in public mailing list here [1].

I would include you in Cc list for my further replies there.

Mika K., my branch is here [2] with above patch included. Can you,
please, test your use case? Because it sounds too strange to me that
connection ID in there affects somehow the PM flow. It very likely
unveils a bug somewhere else.

[1] https://www.spinics.net/lists/linux-input/msg49127.html
[2] http://bitbucket.org/andy-shev/linux/branch/topic%2Fuart%2frpm
Daniel Vetter March 7, 2017, 10:52 a.m. UTC | #5
On Tue, Mar 07, 2017 at 12:48:26PM +0200, Andy Shevchenko wrote:
> On Sun, 2017-02-26 at 22:45 +0100, Daniel Vetter wrote:
> > On Tue, Feb 21, 2017 at 06:52:24PM +0200, Andy Shevchenko wrote:
> > > On Tue, 2017-02-21 at 18:26 +0200, Jani Nikula wrote:
> > > > On Tue, 21 Feb 2017, Andy Shevchenko <andriy.shevchenko@linux.inte
> > > > l.co
> > > > m> wrote:
> > > > > The commit 213e08ad60ba ("drm/i915/bxt: add bxt dsi gpio element
> > > > > support") enables GPIO support for Broxton based platforms.
> > > > > 
> > > > > While using that API we might get into troubles in the future,
> > > > > because
> > > > > we can't rely on label "panel" in the driver since vendor
> > > > > firmware
> > > > > might
> > > > > provide any GPIO pin there, e.g. "reset", and even mark it in
> > > > > _DSD
> > > > > (in
> > > > > which case the request will fail).
> > > > > 
> > > > > To avoid inconsistency and potential issues we have two options:
> > > > > a) generate GPIO ACPI mapping table and supply it via
> > > > > acpi_dev_add_driver_gpios(), or
> > > > > b) just pass NULL as connection ID.
> > > > > 
> > > > > The b) approach is much simplier and would work since the driver
> > > > > relies
> > > > > on GPIO indeces only. Moreover, the _CRS fallback mechanism,
> > > > > when
> > > > > requesting GPIO, is going to be stricter, and supplying non-NULL
> > > > > connection ID when neither _DSD, nor GPIO ACPI mapping is
> > > > > present,
> > > > > will
> > > > > make request fail.
> > > > 
> > > > The patch version log in the commit suggests otherwise; we'd tried
> > > > and
> > > > failed with NULL,
> > > 
> > > Can I see DSDT excerpts of the platform that fails?
> > > 
> > > >  until Mika realized passing "panel" works:
> > > > 
> > > >     v2 by Mika: switch *NULL* to *"panel"* when requesting gpio
> > > > for
> > > > MIPI/DSI
> > > >     panel.
> > > > 
> > > > See also [1]. What has changed since then that should make this
> > > > work
> > > > now? We shouldn't apply until we get Tested-by's.
> > > 
> > > Not changed yet, but *going to be*. See my repository here [2].
> > > To fix the mess with GPIO ACPI stuff we are going to make request
> > > stricter as I pointed in commit message above, i.e. asking for a
> > > GPIO by
> > > connection ID without _DSD present will guarantee -ENOENT since it
> > > will
> > > be no fallback to _CRS. You may follow discussion in our internal
> > > mailing list for drivers.
> > 
> > Why exactly is this being discussed on an internal mailing list?
> > Upstream
> > happens in public ...
> 
> It was a prelininary discussion and it's sad you didn't notice it.

The problem isn't that I didn't notice (I don't think I can provide
anything of value here), but that technical discussion should happen in
the open, on public mailing lists, because otherwise we just have a big
coordination chaos. GFX is huge, and just the automatic public archiving
mailing lists provides is super important to get people up to speed when
suddenly you realize you need them.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 8f683b8b1816..493d5ec2b53a 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -315,7 +315,7 @@  static void bxt_exec_gpio(struct drm_i915_private *dev_priv,
 
 	if (!gpio_desc) {
 		gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-						 "panel", gpio_index,
+						 NULL, gpio_index,
 						 value ? GPIOD_OUT_LOW :
 						 GPIOD_OUT_HIGH);