On Tue, Nov 19, 2019 at 04:18:18PM +0100, Hans de Goede wrote: > At least Bay Trail (BYT) and Cherry Trail (CHT) devices can use 1 of 2 > different PWM controllers for controlling the LCD's backlight brightness. > Either the one integrated into the PMIC or the one integrated into the > SoC (the 1st LPSS PWM controller). > > So far in the LPSS code on BYT we have skipped registering the LPSS PWM > controller "pwm_backlight" lookup entry when a Crystal Cove PMIC is > present, assuming that in this case the PMIC PWM controller will be used. > > On CHT we have been relying on only 1 of the 2 PWM controllers being > enabled in the DSDT at the same time; and always registered the lookup. > > So far this has been working, but the correct way to determine which PWM > controller needs to be used is by checking a bit in the VBT table and > recently I've learned about 2 different BYT devices: > Point of View MOBII TAB-P800W > Acer Switch 10 SW5-012 > > Which use a Crystal Cove PMIC, yet the LCD is connected to the SoC/LPSS > PWM controller (and the VBT correctly indicates this), so here our old > heuristics fail. > > This commit fixes using the wrong PWM controller on these devices by > calling pwm_get() for the right PWM controller based on the > VBT dsi.config.pwm_blc bit. > > Note this is part of a series which contains 2 other patches which renames > the PWM lookup for the 1st SoC/LPSS PWM from "pwm_backlight" to > "pwm_pmic_backlight" and the PWM lookup for the Crystal Cove PMIC PWM > from "pwm_backlight" to "pwm_pmic_backlight". > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_panel.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c > index bc14e9c0285a..ddcf311d1114 100644 > --- a/drivers/gpu/drm/i915/display/intel_panel.c > +++ b/drivers/gpu/drm/i915/display/intel_panel.c > @@ -1840,13 +1840,22 @@ static int pwm_setup_backlight(struct intel_connector *connector, > enum pipe pipe) > { > struct drm_device *dev = connector->base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_panel *panel = &connector->panel; > + const char *desc; > int retval; > > - /* Get the PWM chip for backlight control */ > - panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight"); > + /* Get the right PWM chip for DSI backlight according to VBT */ > + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { > + panel->backlight.pwm = pwm_get(dev->dev, "pwm_pmic_backlight"); > + desc = "PMIC"; > + } else { > + panel->backlight.pwm = pwm_get(dev->dev, "pwm_soc_backlight"); > + desc = "SoC"; > + } Might we want the same thing for the panel enable gpio? > + > if (IS_ERR(panel->backlight.pwm)) { > - DRM_ERROR("Failed to own the pwm chip\n"); > + DRM_ERROR("Failed to get the %s PWM chip\n", desc); > panel->backlight.pwm = NULL; > return -ENODEV; > } > @@ -1873,6 +1882,7 @@ static int pwm_setup_backlight(struct intel_connector *connector, > CRC_PMIC_PWM_PERIOD_NS); > panel->backlight.enabled = panel->backlight.level != 0; > > + DRM_INFO("Using %s PWM for LCD backlight control\n", desc); > return 0; > } > > -- > 2.23.0 -- Ville Syrjälä Intel