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@xxxxxxxxxx>
> >> ---
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
