[PATCH 0/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[PATCH 0/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

Hi All,

On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
do not control the LCD panel- and backlight-enable GPIOs. So far, when
the VBT indicates we should use the SoC for backlight control, 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 series contains 2 patches which together fix this.

To avoid new errors in the intel-gfx CI (assuming there is atleast 1
BYT device there with a DSI panel), we need both of these patches to
be merged through the drm-intel tree.

Unfortunately there is some churn currently going on in the
pinctrl-baytrail.c code, but not in the part of the file which this
touches, so merging the pinctrl-baytrail.c changes through the
drm-intel tree should not lead to conflicts later.

Andy, Mika, assuming you are happy with the changes, can I get your ack
for merging the pinctrl-baytrail patch throught the drm-inteol tree?

Regards,

Hans

[PATCH 1/2] pinctrl: baytrail: Add GPIO lookup and pinctrl-map for i915 DSI panel ctrl

[Hans de Goede]

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(+)

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);
 
-- 
2.23.0

[PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
do not control the LCD panel- and backlight-enable GPIOs. So far, when
the VBT indicates we should use the SoC for backlight control, 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 makes the DSI code control the SoC GPIOs for panel- and
backlight-enable on BYT, when the VBT indicates the SoC should be used
for backlight control. It also ensures that the PWM0 pin is muxed to the
PWM controller in this case.

This fixes the LCD panel not lighting up on various devices when booted
with a HDMI monitor connected. This has been tested to fix this on the
following devices:

Peaq C1010
Point of View MOBII TAB-P800W
Point of View MOBII TAB-P1005W
Terra Pad 1061
Yours Y8W81

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_dsi.h |  3 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c   | 44 ++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
index b15be5814599..6ff1b59b0f6f 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi.h
+++ b/drivers/gpu/drm/i915/display/intel_dsi.h
@@ -45,8 +45,9 @@ struct intel_dsi {
 	struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS];
 	intel_wakeref_t io_wakeref[I915_MAX_PORTS];
 
-	/* GPIO Desc for CRC based Panel control */
+	/* GPIO Desc for panel and backlight control */
 	struct gpio_desc *gpio_panel;
+	struct gpio_desc *gpio_backlight;
 
 	struct intel_connector *attached_connector;
 
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
index fc63e2f6a511..bd081a3bb54b 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -24,6 +24,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/slab.h>
 
 #include <drm/drm_atomic_helper.h>
@@ -797,7 +798,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	if (!IS_GEMINILAKE(dev_priv))
 		intel_dsi_prepare(encoder, pipe_config);
 
-	/* Power on, try both CRC pmic gpio and VBT */
+	/* Power on, try both the panel-enable GPIO and the MIPI seq. */
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
 	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
@@ -843,6 +844,9 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	}
 
 	intel_panel_enable_backlight(pipe_config, conn_state);
+	/* Backlight on, try both the backlight-enable GPIO and the MIPI seq. */
+	if (intel_dsi->gpio_backlight)
+		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 1);
 	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 }
 
@@ -859,7 +863,10 @@ static void intel_dsi_disable(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("\n");
 
+	/* Backlight off, try both the enable GPIO and the MIPI seq. */
 	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
+	if (intel_dsi->gpio_backlight)
+		gpiod_set_value_cansleep(intel_dsi->gpio_backlight, 0);
 	intel_panel_disable_backlight(old_conn_state);
 
 	/*
@@ -943,7 +950,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	/* Assert reset */
 	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 
-	/* Power off, try both CRC pmic gpio and VBT */
+	/* Power off, try both the panel-enable GPIO and the MIPI seq. */
 	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
 	intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 	if (intel_dsi->gpio_panel)
@@ -1542,6 +1549,8 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
 	/* dispose of the gpios */
 	if (intel_dsi->gpio_panel)
 		gpiod_put(intel_dsi->gpio_panel);
+	if (intel_dsi->gpio_backlight)
+		gpiod_put(intel_dsi->gpio_backlight);
 
 	intel_encoder_destroy(encoder);
 }
@@ -1821,6 +1830,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
 	struct intel_connector *intel_connector;
 	struct drm_connector *connector;
 	struct drm_display_mode *current_mode, *fixed_mode;
+	struct pinctrl *pinctrl;
 	enum port port;
 
 	DRM_DEBUG_KMS("\n");
@@ -1921,7 +1931,7 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
 	vlv_dphy_param_init(intel_dsi);
 
 	/*
-	 * In case of BYT with CRC PMIC, we need to use GPIO for
+	 * In case of BYT or CHT with CRC PMIC, we need to use GPIO for
 	 * Panel control.
 	 */
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
@@ -1934,6 +1944,34 @@ void vlv_dsi_init(struct drm_i915_private *dev_priv)
 			intel_dsi->gpio_panel = NULL;
 		}
 	}
+	/*
+	 * In case of BYT (and only BYT) and using the SoC for PWM, we need
+	 * to use the SoC's GPIOs for panel and backlight enable. When the
+	 * GOP did not initialize the panel (HDMI inserted) we may need to
+	 * also change the pinmux for the SoC's PWM0 pin from GPIO to PWM.
+	 */
+	if (IS_VALLEYVIEW(dev_priv) &&
+	    dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_SOC) {
+		intel_dsi->gpio_panel = gpiod_get(dev->dev,
+						  "soc_panel_enable",
+						  GPIOD_OUT_HIGH);
+		if (IS_ERR(intel_dsi->gpio_panel)) {
+			DRM_ERROR("Failed to own gpio for panel control\n");
+			intel_dsi->gpio_panel = NULL;
+		}
+
+		intel_dsi->gpio_backlight = gpiod_get(dev->dev,
+						      "soc_backlight_enable",
+						      GPIOD_OUT_HIGH);
+		if (IS_ERR(intel_dsi->gpio_backlight)) {
+			DRM_ERROR("Failed to own gpio for backlight control\n");
+			intel_dsi->gpio_backlight = NULL;
+		}
+
+		pinctrl = devm_pinctrl_get_select(dev->dev, "soc_pwm0");
+		if (IS_ERR(pinctrl))
+			DRM_ERROR("Failed to set pinmux to PWM\n");
+	}
 
 	drm_connector_init(dev, connector, &intel_dsi_connector_funcs,
 			   DRM_MODE_CONNECTOR_DSI);
-- 
2.23.0

Re: [PATCH 0/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Andy Shevchenko]

On Fri, Nov 29, 2019 at 07:58:34PM +0100, Hans de Goede wrote:
> Hi All,
> 
> On Bay Trail devices the MIPI power on/off sequences for DSI LCD panels
> do not control the LCD panel- and backlight-enable GPIOs. So far, when
> the VBT indicates we should use the SoC for backlight control, 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 series contains 2 patches which together fix this.
> 
> To avoid new errors in the intel-gfx CI (assuming there is atleast 1
> BYT device there with a DSI panel), we need both of these patches to
> be merged through the drm-intel tree.
> 
> Unfortunately there is some churn currently going on in the
> pinctrl-baytrail.c code, but not in the part of the file which this
> touches, so merging the pinctrl-baytrail.c changes through the
> drm-intel tree should not lead to conflicts later.
> 
> Andy, Mika, assuming you are happy with the changes, can I get your ack
> for merging the pinctrl-baytrail patch throught the drm-inteol tree?

I'm not okay with this. I will tell more next week how I see this to be
implemented in a better way.

Happy Black Friday! :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Linus Walleij]

Hi Hans,

thank you for your patch!

On Fri, Nov 29, 2019 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:

> -       /* GPIO Desc for CRC based Panel control */
> +       /* GPIO Desc for panel and backlight control */
>         struct gpio_desc *gpio_panel;
> +       struct gpio_desc *gpio_backlight;

I think what happens here is that you reimplement
drivers/video/backlight/gpio_backlight.c

The existing power control GPIO also reimplements
drivers/regulator/fixed.c in a sense but I am under the
impression that x86 in general and ACPI in particular
has a problem with the regulator subsystem so I am
uncertain about that one.

When I look at the code I get more confused because
nominally panels should have their own drivers in
drivers/gpu/drm/panel/* especially DSI panels, and the
panels control their own GPIOs or regulators for power
and backlight.

I was under the impression that Heikki's and Dmitry's
recent additions to software nodes would make it
possible to actually spawn devices like the GPIO backlight
and/or fixed regulator and put references to them so
that the drivers can pick them up from the respective
frameworks and manage them?

Maybe I misunderstood things here though, I am a bit
under the impression that elder DRM drivers are
considered "elder gods" and do not need to use separate
panel drivers, backlight abstraction etc, and in that
case just go ahead, I guess.

But I suspect some separation
would help the day the i915 driver wants to reuse some
really complex DSI panel from drivers/gpu/drm/panel/*
though.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: baytrail: Add GPIO lookup and pinctrl-map for i915 DSI panel ctrl

[Linus Walleij]

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

Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Jani Nikula]

On Mon, 02 Dec 2019, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Hans,
>
> thank you for your patch!
>
> On Fri, Nov 29, 2019 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
>> -       /* GPIO Desc for CRC based Panel control */
>> +       /* GPIO Desc for panel and backlight control */
>>         struct gpio_desc *gpio_panel;
>> +       struct gpio_desc *gpio_backlight;
>
> I think what happens here is that you reimplement
> drivers/video/backlight/gpio_backlight.c
>
> The existing power control GPIO also reimplements
> drivers/regulator/fixed.c in a sense but I am under the
> impression that x86 in general and ACPI in particular
> has a problem with the regulator subsystem so I am
> uncertain about that one.
>
> When I look at the code I get more confused because
> nominally panels should have their own drivers in
> drivers/gpu/drm/panel/* especially DSI panels, and the
> panels control their own GPIOs or regulators for power
> and backlight.
>
> I was under the impression that Heikki's and Dmitry's
> recent additions to software nodes would make it
> possible to actually spawn devices like the GPIO backlight
> and/or fixed regulator and put references to them so
> that the drivers can pick them up from the respective
> frameworks and manage them?
>
> Maybe I misunderstood things here though, I am a bit
> under the impression that elder DRM drivers are
> considered "elder gods" and do not need to use separate
> panel drivers, backlight abstraction etc, and in that
> case just go ahead, I guess.
>
> But I suspect some separation
> would help the day the i915 driver wants to reuse some
> really complex DSI panel from drivers/gpu/drm/panel/*
> though.

Sadly I don't think that's going to happen, though.

For i915 the panels are described by VBT, or Video BIOS Tables. We don't
really know the make or model of the panels, we just get the timings and
sequences etc. from there. There's no info to probe a separate panel
driver. So we just have a "generic" DSI driver that's part of i915 which
uses the data from the VBT to drive all the possible panels.

We do use some of the drm DSI framework as a library to abstract things
a bit better, but that's about it.

The sequences should include details about GPIOs to toggle as well, so
in that sense I feel like the gpio calls should be bolted there.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 1/2] pinctrl: baytrail: Add GPIO lookup and pinctrl-map for i915 DSI panel ctrl

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

HI,

On 02-12-2019 12:53, Jani Nikula wrote:
> On Mon, 02 Dec 2019, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Hi Hans,
>>
>> thank you for your patch!
>>
>> On Fri, Nov 29, 2019 at 7:58 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> -       /* GPIO Desc for CRC based Panel control */
>>> +       /* GPIO Desc for panel and backlight control */
>>>          struct gpio_desc *gpio_panel;
>>> +       struct gpio_desc *gpio_backlight;
>>
>> I think what happens here is that you reimplement
>> drivers/video/backlight/gpio_backlight.c
>>
>> The existing power control GPIO also reimplements
>> drivers/regulator/fixed.c in a sense but I am under the
>> impression that x86 in general and ACPI in particular
>> has a problem with the regulator subsystem so I am
>> uncertain about that one.
>>
>> When I look at the code I get more confused because
>> nominally panels should have their own drivers in
>> drivers/gpu/drm/panel/* especially DSI panels, and the
>> panels control their own GPIOs or regulators for power
>> and backlight.
>>
>> I was under the impression that Heikki's and Dmitry's
>> recent additions to software nodes would make it
>> possible to actually spawn devices like the GPIO backlight
>> and/or fixed regulator and put references to them so
>> that the drivers can pick them up from the respective
>> frameworks and manage them?
>>
>> Maybe I misunderstood things here though, I am a bit
>> under the impression that elder DRM drivers are
>> considered "elder gods" and do not need to use separate
>> panel drivers, backlight abstraction etc, and in that
>> case just go ahead, I guess.
>>
>> But I suspect some separation
>> would help the day the i915 driver wants to reuse some
>> really complex DSI panel from drivers/gpu/drm/panel/*
>> though.
> 
> Sadly I don't think that's going to happen, though.
> 
> For i915 the panels are described by VBT, or Video BIOS Tables. We don't
> really know the make or model of the panels, we just get the timings and
> sequences etc. from there. There's no info to probe a separate panel
> driver. So we just have a "generic" DSI driver that's part of i915 which
> uses the data from the VBT to drive all the possible panels.
> 
> We do use some of the drm DSI framework as a library to abstract things
> a bit better, but that's about it.
> 
> The sequences should include details about GPIOs to toggle as well, so
> in that sense I feel like the gpio calls should be bolted there.

Hmm, yes that makes sense, the problem really is that the VBT sequences
for DSI panels on BYT are incomplete (*) and the MIPI_SEQ_POWER_ON / _OFF
and MIPI_SEQ_BACKLIGHT_ON / _OFF sequences do not containt the
"instructions" to toggle the panel-enable, resp. backlight enable
GPIOs as they do on CHT and newer devices.

So if we see all this of a shortcoming of the VBT tables, then it makes
sense to move all special handling for this to intel_dsi_vbt.c, so we
put the adding of the lookup table, the getting of the pins and the
controlling of the pins all in there.

Jani, is that what you have in mind ?

This would also solve the problem of Linus and Andy not liking the
polution of the pinctrl driver with the adding of the platform
specific lookups; and then we could only add the lookups when the
VBT says the SoC is used for backlight control, leaving things
as is (not even adding the lookups) when the PMIC is used for
backlight control.

There is only one problem, currently is is not possible to
unregister a mapping added with pinctrl_register_mappings
and the i915 driver is typically a module which can be unloaded
and I believe actually is unloaded as part of the i915 CI.

pinctrl_register_mappings copies the passed in mapping, but
it is a shallow copy, so it contains pointers to the modules
const segment and we do not want to re-add another copy of
the mapping when the module loads a second time.

Fixing this is easy though, there already is a pinctrl_unregister_map()
function, we just need to export it so that the i915 driver can
remove the mapping when it is unbound.

Linus would exporting this function be ok with you?

Linus, question what is the purpose of the "dupping" / shallow
copying of the argument passed to pinctrl_register_map ? Since
it is shallow the mem for any pointers contained within there need
to be kept around by the caller, so why not let the caller keep
the pinctrl_map struct itself around too?

If we are going to export pinctrl_unregister_map() we need to make it
do the right thing for dupped maps too, we can just store the dup flag
in struct pinctrl_maps. So this is easy, but I wonder if we cannot
get rid of the dupping all together ?

Regards,

Hans




*) I suspect these are all version 2 VBT sequences (I guess I should check
this) and according to the comment in vlv_dsi.c which documents the sequences,
for v2 these sequences are not used, e.g. instead of executing the
MIPIPanelPowerOn sequence the "panel-on" step is described simply as "power on".
and the MIPIBacklightOn/Off sequences are described as being VBT v3+ only

Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Linus Walleij]

On Mon, Dec 2, 2019 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:

> There is only one problem, currently is is not possible to
> unregister a mapping added with pinctrl_register_mappings
> and the i915 driver is typically a module which can be unloaded
> and I believe actually is unloaded as part of the i915 CI.
>
> pinctrl_register_mappings copies the passed in mapping, but
> it is a shallow copy, so it contains pointers to the modules
> const segment and we do not want to re-add another copy of
> the mapping when the module loads a second time.
>
> Fixing this is easy though, there already is a pinctrl_unregister_map()
> function, we just need to export it so that the i915 driver can
> remove the mapping when it is unbound.
>
> Linus would exporting this function be ok with you?

Yep!

> Linus, question what is the purpose of the "dupping" / shallow
> copying of the argument passed to pinctrl_register_map ?

The initial commit contained this comment later removed:

+       /*
+        * Make a copy of the map array - string pointers will end up in the
+        * kernel const section anyway so these do not need to be deep copied.
+        */

The use was to free up memory for platforms using boardfiles
with a gazillion variants and huge pin control tables, so these
could be marked  __initdata and discarded after boot.
As the strings would anyway stay around we didn't need to
deep copy.

See for example in arch/arm/mach-u300/core.c
static struct pinctrl_map __initdata u300_pinmux_map[]

> Since
> it is shallow the mem for any pointers contained within there need
> to be kept around by the caller, so why not let the caller keep
> the pinctrl_map struct itself around too?

So the strings will be kept around because the kernel can't get
rid of strings. (Yeah it is silly, should haven been fixed ages
ago, but not by me, haha :)

> If we are going to export pinctrl_unregister_map() we need to make it
> do the right thing for dupped maps too, we can just store the dup flag
> in struct pinctrl_maps. So this is easy, but I wonder if we cannot
> get rid of the dupping all together ?

Maybe ... I don't know. What do you think? I suppose you could
make u300 crash if you do that.

Yours,
Linus Walleij

Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

[Hans de Goede]

Hi,

On 11-12-2019 01:24, Linus Walleij wrote:
> On Mon, Dec 2, 2019 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> There is only one problem, currently is is not possible to
>> unregister a mapping added with pinctrl_register_mappings
>> and the i915 driver is typically a module which can be unloaded
>> and I believe actually is unloaded as part of the i915 CI.
>>
>> pinctrl_register_mappings copies the passed in mapping, but
>> it is a shallow copy, so it contains pointers to the modules
>> const segment and we do not want to re-add another copy of
>> the mapping when the module loads a second time.
>>
>> Fixing this is easy though, there already is a pinctrl_unregister_map()
>> function, we just need to export it so that the i915 driver can
>> remove the mapping when it is unbound.
>>
>> Linus would exporting this function be ok with you?
> 
> Yep!
> 
>> Linus, question what is the purpose of the "dupping" / shallow
>> copying of the argument passed to pinctrl_register_map ?
> 
> The initial commit contained this comment later removed:
> 
> +       /*
> +        * Make a copy of the map array - string pointers will end up in the
> +        * kernel const section anyway so these do not need to be deep copied.
> +        */
> 
> The use was to free up memory for platforms using boardfiles
> with a gazillion variants and huge pin control tables, so these
> could be marked  __initdata and discarded after boot.
> As the strings would anyway stay around we didn't need to
> deep copy.
> 
> See for example in arch/arm/mach-u300/core.c
> static struct pinctrl_map __initdata u300_pinmux_map[]
> 
>> Since
>> it is shallow the mem for any pointers contained within there need
>> to be kept around by the caller, so why not let the caller keep
>> the pinctrl_map struct itself around too?
> 
> So the strings will be kept around because the kernel can't get
> rid of strings. (Yeah it is silly, should haven been fixed ages
> ago, but not by me, haha :)
> 
>> If we are going to export pinctrl_unregister_map() we need to make it
>> do the right thing for dupped maps too, we can just store the dup flag
>> in struct pinctrl_maps. So this is easy, but I wonder if we cannot
>> get rid of the dupping all together ?
> 
> Maybe ... I don't know. What do you think? I suppose you could
> make u300 crash if you do that.

I've prepared a patch which makes pinctrl_register_mappings remember
if the mapping is dupped or not (store the dup value in struct pinctrl_maps);
and which modifies pinctrl_unregister_map() to do the right thing
depending on the stored dup value.

I still need to test the new series and then I will post it.

Regards,

Hans