Re: [PATCH] drm/i915/intel_dsi: Add acpi_gpio_mapping for the panel-enable GPIO

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> 
> 
> >> +
> >>   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 ?

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

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