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

[Hans de Goede]

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