Messages in this thread | | | Subject | Re: [PATCH 2/3] mfd: intel_soc_pmic: Rename pwm_backlight pwm-lookup to pwm_pmic_backlight | From | Hans de Goede <> | Date | Fri, 13 Dec 2019 13:40:37 +0100 |
| |
Hi,
On 13-12-2019 09:27, Lee Jones wrote: > On Thu, 12 Dec 2019, Hans de Goede wrote: > >> Hi, >> >> On 12-12-2019 16:52, Lee Jones wrote: >>> On Thu, 12 Dec 2019, Hans de Goede wrote: >>> >>>> Hi, >>>> >>>> On 12-12-2019 09:45, Lee Jones wrote: >>>>> On Wed, 11 Dec 2019, Hans de Goede wrote: >>>>> >>>>>> Hi Lee, >>>>>> >>>>>> On 10-12-2019 09:51, Lee Jones wrote: >>>>>>> On Tue, 19 Nov 2019, 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. >>>>>>>> >>>>>>>> Since only the i915 driver has access to the VBT, this commit renames >>>>>>>> the "pwm_backlight" lookup entries for the Crystal Cove PMIC's PWM >>>>>>>> controller to "pwm_pmic_backlight" so that the i915 driver can do a >>>>>>>> pwm_get() for the right controller depending on the VBT bit, instead of >>>>>>>> the i915 driver relying on a "pwm_backlight" lookup getting registered >>>>>>>> which magically points to the right controller. >>>>>>>> >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> --- >>>>>>>> drivers/mfd/intel_soc_pmic_core.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> For my own reference: >>>>>>> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> >>>>>> >>>>>> As mentioned in the cover-letter, to avoid breaking bi-sectability >>>>>> as well as to avoid breaking the intel-gfx CI we need to merge this series >>>>>> in one go through one tree. Specifically through the drm-intel tree. >>>>>> Is that ok with you ? >>>>>> >>>>>> If this is ok with you, then you do not have to do anything, I will just push >>>>>> the entire series to drm-intel. drivers/mfd/intel_soc_pmic_core.c >>>>>> does not see much changes so I do not expect this to lead to any conflicts. >>>>> >>>>> It's fine, so long as a minimal immutable pull-request is provided. >>>>> Whether it's pulled or not will depend on a number of factors, but it >>>>> needs to be an option. >>>> >>>> The way the drm subsys works that is not really a readily available >>>> option. The struct definition which this patch changes a single line in >>>> has not been touched since 2015-06-26 so I really doubt we will get a >>>> conflict from this. >>> >>> Always with the exceptions ... >>> >>> OOI, why does this *have* to go through the DRM tree? >> >> This patch renames the name used to lookup the pwm controller from >> "pwm_backlight" to "pwm_pmic_backlight" because there are 2 possible >> pwm controllers which may be used, one in the SoC itself and one >> in the PMIC. Which controller should be used is described in a table >> in the Video BIOS, so another part of this series adds this code to >> the i915 driver: >> >> - 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"; >> + } >> >> So both not to break bisectability, but also so as to not break the extensive >> CI system which is used to test the i915 driver we need the MFD change doing >> the rename to go upstrream through the same tree as the i915 change. >> >> I have even considered just squashing the 2 commits together as having only 1 >> present, but not the other breaks stuff left and right. > > That doesn't answer the question. > > Why do they all *have* to go in via the DRM tree specifically?
1. As explained these chanegs need to stay together 2. This change is primarily a drm/i915 change. Also the i915 code sees lots of changes every cycle, where as the change to the mfd code touches a block of code which has not been touched since 2015-06-26, so the chance of conflicts is much bigger if this goes on through another tree.
I honestly do not see the problem here? Let me reverse the question why should this NOT go in through the drm tree?
Regards,
Hans
| |