Hi,

On 29-06-18 13:51, Ville Syrjälä wrote:

On Fri, Jun 29, 2018 at 01:32:58PM +0200, Hans de Goede wrote:

...

Add acpi_gpio_mapping for the panel-enable GPIO, this fixes the following error: "Failed to own gpio for panel control" on BYT/CHT devices where pwm_blc == PPS_BLC_PMIC.

Note this patch is untested as I don't have hardware to test this, but it should fix things.

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 3b7acb5a70b3..b2b75ed3cbf9 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -29,6 +29,7 @@ #include <drm/drm_edid.h> #include <drm/i915_drm.h> #include <drm/drm_mipi_dsi.h> +#include <linux/acpi.h> #include <linux/slab.h> #include <linux/gpio/consumer.h> #include "i915_drv.h" @@ -1713,6 +1714,13 @@ static void intel_dsi_add_properties(struct intel_connector *connector) } }

+static const struct acpi_gpio_params panel_gpio = { 0, 0, false };

+static const struct acpi_gpio_mapping panel_gpios[] = {

• { "panel", &panel_gpio, 1 },
• { },

+};

Named initializers please.

These structs are used in many other drivers without using named initializers and using it with named-initializers will make the mapping table much harder to read if there is more then 1 entry.

I don't believe named initializers are necessary / useful here, on the contrary I believe them to be counter-productive in this case.

...

• void intel_dsi_init(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm;

@@ -1811,6 +1819,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv) */ if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC)) {

• devm_acpi_dev_add_driver_gpios(dev->dev, panel_gpios);
  

Some explanation on what this actually does would be nice. There is no documentation that I can see so it's totally unclear why this is needed.

Also IIRC this gpio comes straight from the pmic driver and not from acpi. So I don't really understand why acpi stuff must be involved here.

It has always come through ACPI, without adding code to manually search for a GPIO chip (and using a different way to get the gpio_desc) all GPIOs are always looked up through ACPI resource tables on x86.

Now it might point to a GPIO on the PMIC in some cases. But it does not always point to the PMIC, e.g. here are the GFX0 resources from the Microsoft Surface 3 (non pro version) :

Name (RBUF, ResourceTemplate () { I2cSerialBus (0x002C, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\_SB.PCI0.I2C6", 0x00, ResourceConsumer, , ) GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOut "\_SB.GPO1", 0x00, ResourceConsumer, , ) { // Pin list 0x003F } })

Notice how it is using a GPIO on GPO1, so not on the PMIC.

As for why this is necessary ACPI based GPIO lookups so far where unique in that they ignored the passed in name, relying on the index instead and in the i915 code, since no index is passed in simply blindly taking the first GPIO in the resources table.

While doing various cleanups to the ACPI GPIO code Andy introduced *mandatory* GPIO mappings for ACPI to map resource indexes to names as used on other platforms.

Regards,

Hans

Hi,

On 29-06-18 14:10, Ville Syrjälä wrote:

On Fri, Jun 29, 2018 at 02:05:58PM +0200, Hans de Goede wrote:

...

Hi,

On 29-06-18 13:51, Ville Syrjälä wrote:

...

On Fri, Jun 29, 2018 at 01:32:58PM +0200, Hans de Goede wrote:

...

Add acpi_gpio_mapping for the panel-enable GPIO, this fixes the following error: "Failed to own gpio for panel control" on BYT/CHT devices where pwm_blc == PPS_BLC_PMIC.

Note this patch is untested as I don't have hardware to test this, but it should fix things.

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 3b7acb5a70b3..b2b75ed3cbf9 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -29,6 +29,7 @@ #include <drm/drm_edid.h> #include <drm/i915_drm.h> #include <drm/drm_mipi_dsi.h> +#include <linux/acpi.h> #include <linux/slab.h> #include <linux/gpio/consumer.h> #include "i915_drv.h" @@ -1713,6 +1714,13 @@ static void intel_dsi_add_properties(struct intel_connector *connector) } }

+static const struct acpi_gpio_params panel_gpio = { 0, 0, false };

+static const struct acpi_gpio_mapping panel_gpios[] = {

• { "panel", &panel_gpio, 1 },
• { },

+};

Named initializers please.

These structs are used in many other drivers without using named initializers and using it with named-initializers will make the mapping table much harder to read if there is more then 1 entry.

I don't believe named initializers are necessary / useful here, on the contrary I believe them to be counter-productive in this case.

I have no idea what these magic numbers mean, and I don't want to have to look up the struct definition everyt time I read this code.

Ok, fair enough.

...

...

...

• void intel_dsi_init(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm;

@@ -1811,6 +1819,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv) */ if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC)) {

• devm_acpi_dev_add_driver_gpios(dev->dev, panel_gpios);
  

Some explanation on what this actually does would be nice. There is no documentation that I can see so it's totally unclear why this is needed.

Also IIRC this gpio comes straight from the pmic driver and not from acpi. So I don't really understand why acpi stuff must be involved here.

It has always come through ACPI, without adding code to manually search for a GPIO chip (and using a different way to get the gpio_desc) all GPIOs are always looked up through ACPI resource tables on x86.

So what is the gpio lookup thing in intel_soc_pmic_core.c ?

Oh, right <stunned silence>. I had forgotten all about that.

I got contacted by an user of a Surface 3 which is seeing a whole bunch of new errors after jumping from a somewhat old kernel to 4.18, of which not being able to get the panel-gpio is one error, so I thought I would go and fix that one, since the acpi_gpio_mapping stuff is fairly new. He could not test because the Surface 3 won't boot because of the other errors, I guess those other errors are also causing issues with the PMIC code.

You are right that in this case we are already manually adding a non ACPI based mapping, so we should probably not be adding the ACPI based mapping, my bad.

TL;DR: You are right we already have a hardcoded mapping to the PMIC for this and my patch is bogus and should be dropped.

Regards,

Hans

...

Now it might point to a GPIO on the PMIC in some cases. But it does not always point to the PMIC, e.g. here are the GFX0 resources from the Microsoft Surface 3 (non pro version) :

          Name (RBUF, ResourceTemplate ()
          {
              I2cSerialBus (0x002C, ControllerInitiated, 0x00061A80,
                  AddressingMode7Bit, "\\_SB.PCI0.I2C6",
                  0x00, ResourceConsumer, ,
                  )
              GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOut
                  "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                  )
                  {   // Pin list
                      0x003F
                  }
          })

Notice how it is using a GPIO on GPO1, so not on the PMIC.

As for why this is necessary ACPI based GPIO lookups so far where unique in that they ignored the passed in name, relying on the index instead and in the i915 code, since no index is passed in simply blindly taking the first GPIO in the resources table.

While doing various cleanups to the ACPI GPIO code Andy introduced *mandatory* GPIO mappings for ACPI to map resource indexes to names as used on other platforms.

Regards,

Hans