| Message ID | 20191129185836.2789-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT | expand |
Hi Hans! Thanks for your patch! On Fri, Nov 29, 2019 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote: > On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels > do not control the LCD panel and backlight GPIOs. So far we have been > relying on these GPIOs being configured as output and driven high by > the Video BIOS (GOP) when it initializes the panel. > > This does not work when the device is booted with a HDMI monitor connected > as then the GOP will initialize the HDMI instead of the panel, leaving the > panel black, even though the i915 driver tries to output an image to it. > > Likewise on some device-models when the GOP does not initialize the DSI > panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead > of muxing it to the PWM controller. > > This commit adds GPIO lookups and a pinctrl-map which the i915 driver can > use to get the panel- and backlight-enable GPIOs and to mux the PWM0 pin > to the PWM controller. > > Note it may seem a bit weird to add a pinctrl-map for the i915 driver, > so that it can set the PWM0 pinmux. Doing this from the LPSS PWM driver > would be more logical. But the only thing telling us that the pin should > definitely be muxed to the PWM controller is the VBT to which the PWM > driver does not have access. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> (...) The code setting up the pinctrl map and the GPIO machine descriptor essentially looks good to me. This looks a bit weird: + if (soc_data == &byt_score_soc_data) { Can you do this with explicit platform ID string comparison instead? if (!strcmp(soc_data->uid, BYT_SCORE_ACPI_UID)) { ... It seems more appropriate to me. What is puzzling is the placement inside the pinctrl driver: normally the thing that spawns the devices on the platform such as the "byt_gpio" should register this table too. However I see that the device comes from the ACPI match. Two questions: - Is this one of those cases where ACPI "should have thought of this" (a common phrase) and we have to mop up the mess in the kernel because ACPI didn't and now we have to quirk around it? - Can we in that case handle this with a "boardfile" of quirks under say drivers/platform where we register some extra stuff rather than inside the pinctrl driver? It sort of connects to the other review comments where I feel we should be spawning gpio backlight devices from somewhere. I understand those things may be a bit big, if the intel pinctrl maintainers are fine with this solution I am fine with it too, it's not like it is the biggest deal, I am just worried that we might be stockpiling these quirks all over the place while they should perhaps be centralized. Yours, Linus Walleij
On Fri, Nov 29, 2019 at 07:58:35PM +0100, Hans de Goede wrote: > On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels > do not control the LCD panel and backlight GPIOs. So far we have been > relying on these GPIOs being configured as output and driven high by > the Video BIOS (GOP) when it initializes the panel. > > This does not work when the device is booted with a HDMI monitor connected > as then the GOP will initialize the HDMI instead of the panel, leaving the > panel black, even though the i915 driver tries to output an image to it. > > Likewise on some device-models when the GOP does not initialize the DSI > panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead > of muxing it to the PWM controller. > > This commit adds GPIO lookups and a pinctrl-map which the i915 driver can > use to get the panel- and backlight-enable GPIOs and to mux the PWM0 pin > to the PWM controller. > > Note it may seem a bit weird to add a pinctrl-map for the i915 driver, > so that it can set the PWM0 pinmux. Doing this from the LPSS PWM driver > would be more logical. But the only thing telling us that the pin should > definitely be muxed to the PWM controller is the VBT to which the PWM > driver does not have access. My concern here, as one of Linus', is a pollution the driver with board code. Aren't we able to split this to a separate file under PDx86 realm and do nasty quirks there?
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index 3a6404b6fd7e..a13ca7c672fa 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -9,6 +9,7 @@ #include <linux/acpi.h> #include <linux/bitops.h> #include <linux/gpio/driver.h> +#include <linux/gpio/machine.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/io.h> @@ -1601,6 +1602,39 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = { { } }; +/* + * On Bay Trail devices with a DSI LCD panel and using the SoC (LPSS) pwm for + * backlight control, the i915 driver needs to control the backlight and LCD + * enable GPIOs, which are always pin 10 and 11 on the NCORE. Without this + * the LCD panel will not light-up when the system is booted with a HDMI cable + * inserted and the video BIOS / GOP did not init the LCD because of this. + * + * Likewise in some cases when a HDMI cable is inserted the firmware does not + * even properly mux the PWM0 pin. We add a pinctrl-map for this so that the + * i915 driver can fix this. Note that the map is for the i915 device not for + * the PWM device. This is a bit weird, but the only thing telling us that + * the pin should definitely be muxed to the PWM controller is the VBT bit + * which tells the i915 driver to use the SoC's PWM for backlight control. + */ + +static char score_name[32]; +static char ncore_name[32]; + +static const struct pinctrl_map byt_panel_pwm_pinctrl_map[] = { + PIN_MAP_MUX_GROUP("0000:00:02.0", "soc_pwm0", score_name, + "pwm0_grp", "pwm"), +}; + +static struct gpiod_lookup_table byt_panel_gpio_table = { + .dev_id = "0000:00:02.0", + .table = { + /* "soc_" prefix to distuingish these from those on the PMIC */ + GPIO_LOOKUP(ncore_name, 10, "soc_backlight_enable", GPIO_ACTIVE_HIGH), + GPIO_LOOKUP(ncore_name, 11, "soc_panel_enable", GPIO_ACTIVE_HIGH), + { }, + }, +}; + static int byt_pinctrl_probe(struct platform_device *pdev) { const struct intel_pinctrl_soc_data *soc_data = NULL; @@ -1651,6 +1685,16 @@ static int byt_pinctrl_probe(struct platform_device *pdev) if (ret) return ret; + if (soc_data == &byt_score_soc_data) { + strscpy(score_name, dev_name(&pdev->dev), sizeof(score_name)); + ret = pinctrl_register_mappings(byt_panel_pwm_pinctrl_map, 1); + if (ret) + dev_err(&pdev->dev, "failed to register pinctrl-map\n"); + } else if (soc_data == &byt_ncore_soc_data) { + strscpy(ncore_name, dev_name(&pdev->dev), sizeof(ncore_name)); + gpiod_add_lookup_table(&byt_panel_gpio_table); + } + platform_set_drvdata(pdev, vg); pm_runtime_enable(&pdev->dev);
On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels do not control the LCD panel and backlight GPIOs. So far we have been relying on these GPIOs being configured as output and driven high by the Video BIOS (GOP) when it initializes the panel. This does not work when the device is booted with a HDMI monitor connected as then the GOP will initialize the HDMI instead of the panel, leaving the panel black, even though the i915 driver tries to output an image to it. Likewise on some device-models when the GOP does not initialize the DSI panel it also leaves the mux of the PWM0 pin in generic GPIO mode instead of muxing it to the PWM controller. This commit adds GPIO lookups and a pinctrl-map which the i915 driver can use to get the panel- and backlight-enable GPIOs and to mux the PWM0 pin to the PWM controller. Note it may seem a bit weird to add a pinctrl-map for the i915 driver, so that it can set the PWM0 pinmux. Doing this from the LPSS PWM driver would be more logical. But the only thing telling us that the pin should definitely be muxed to the PWM controller is the VBT to which the PWM driver does not have access. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/pinctrl/intel/pinctrl-baytrail.c | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+)