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

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

[Andy Shevchenko]

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(-)

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);
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/bxt: use NULL for GPIO connection ID

[Patchwork]

== Series Details ==

Series: drm/i915/bxt: use NULL for GPIO connection ID
URL   : https://patchwork.freedesktop.org/series/20011/
State : success

== Summary ==

Series 20011v1 drm/i915/bxt: use NULL for GPIO connection ID
https://patchwork.freedesktop.org/api/1.0/series/20011/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup gtt-render:
                incomplete -> PASS       (fi-byt-j1900)

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:253  pass:225  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

3d47f9476bdc68afdb73ddbc96375366ea689a79 drm-tip: 2017y-02m-21d-14h-13m-06s UTC integration manifest
fbeb942 drm/i915/bxt: use NULL for GPIO connection ID

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3914/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Andy Shevchenko]

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1] drm/i915/bxt: use NULL for GPIO connection ID

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andy Shevchenko]

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx