| Message ID | 20170221151902.152620-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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
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
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
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 --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);
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(-)