Re: [Intel-gfx] [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable
On Mon, Mar 16, 2015 at 5:42 AM, Shobhit Kumar shobhit.ku...@intel.com wrote: The CRC (Crystal Cove) PMIC, controls the panel enable and disable signals for BYT for dsi panels. This is indicated in the VBT fields. Use that to initialize and use GPIO based control for these signals. v2: Use the newer gpiod interface(Alexandre) v3: Remove the redundant checks and unused code (Ville) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com NACK. This is not a GPIO but a special-purpose register as can be seen from other discussions. Use a fixed voltage regulator spun from an MFD cell instead of shoehorning this into GPIO. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Thu, Mar 12, 2015 at 4:06 PM, Kumar, Shobhit shobhit.ku...@intel.com wrote: On Mon, 2015-03-09 at 18:15 +0100, Linus Walleij wrote: On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit shobhit.ku...@intel.com wrote: There are actually two lines for Panel Power control and Backlight enable/disable. I have already moved towards adding a new Cell device for PWM child device and a new pwm driver for the same. That will take care of backlight thingy. But for the Panel power control, I am at loss for how best to program that. Isn't it just a very simple regulator (just on/off of fixed voltage) cell? It is just behaving as an output GPIO line and for that reason I feel that rather than adding a new regulator driver, I disagree. GPIO reads general purpose input/output. This is not a GPIO, this is a special purpose thing and IMO it should be modeled directly as a regulator. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v5 2/9] gpio/crystalcove: Add additional GPIO for Panel control
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: Export PANEL_EN/DISABLE (offset 0x52) as additional GPIO. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable control is routed thorugh CRC PMIC CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com NACK. Spawn a separate MFD cell and write a fixed voltage regulator driver for this. @@ -39,6 +39,7 @@ #define GPIO0P0CTLI0x33 #define GPIO1P0CTLO0x3b #define GPIO1P0CTLI0x43 +#define GPIOPANELCTL 0x52 This is even a lie, you say in the commit message that the register is indeed named PANEL_EN/DISABLE and is not a GPIO. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit shobhit.ku...@intel.com wrote: There are actually two lines for Panel Power control and Backlight enable/disable. I have already moved towards adding a new Cell device for PWM child device and a new pwm driver for the same. That will take care of backlight thingy. But for the Panel power control, I am at loss for how best to program that. Isn't it just a very simple regulator (just on/off of fixed voltage) cell? Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two additional GPIOs. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable and Backlight control are routed thorugh CRC PMIC Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com This seems very unintuitive. I have a hard time believeing this: +#define GPIOPANELCTL 0x51 A special GPIO only designated to panel control? That is totally counter to the *meaning* of general purpose input/output. It is not general purpose at all, it is special-purpose. If the data sheet says a special-purpose pin is a GPIO when it isn't the map of the world (the datasheet terminology) is wrong, not the world, the usage is dedicated for the panel. Look at the usecase, not the name. Obviously that line is only for controlling the backlight, and you should spin a new child device from the Inte SoC PMIC MFD hub and put a driver for it in drivers/video/backlight or wherever this goes. Lee: do you agree? Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable
On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter dan...@ffwll.ch wrote:
The crystalcove pmic thing here really is just a dumb gpio line that for
the reference design gets routed to the panel (and hence has that as the
usual name).
So obviously the refman calls this register at offset 0x52
PANEL_EN/DISABLE not GPIO_FOO.
And what we want to do here is get some reasonable way to
route that gpio line control between two totally different drivers.
Also please note that x86 is a lot shittier than arm for this kind of
inter-driver-depencies since we don't even have board files. Hence also
why this series has some patches to expose the board file init stuff to
the pmic driver for setup.
We don't do board files for ARM anymore either. We do this
using device tree which is similar to how x86 use ACPI.
Anyway if you still think gpio is totally the wrong thing then we'll just
roll our own using the component framework.
I guess I better not say no if the alternative is even uglier.
The problem I have is GPIO being used as kitchen sink, and
a recent incident where someone instantiated generic GPIO
chips over some 32bit register just to basically poke bits in that
register to turn LEDs on/off. So a layer cake like:
GPIO LEDs - generic GPIO - Register.
It's nice and simple for that user as it's using existing kernel code
and just needs some device tree abstract stuff to get going, and
it works. The problem is that it takes something that is not
GPIO (rather a set of bits controlling LEDs) and pretends that
it is GPIO, while it is not, just to be able to use GPIO LEDs.
While the real solution is to write a pure LED driver, possibly
one using some random 32bit registers, LED MMIO or whatever.
Anyway. I still think a fixed voltage regulator would be suitable
for this.
Here is an example of how we do device tree:
en_3v3_reg: en_3v3 {
compatible = regulator-fixed;
regulator-name = en-3v3-fixed-supply;
regulator-min-microvolt = 330;
regulator-max-microvolt = 330;
gpio = ab8500_gpio 25 0x4;
startup-delay-us = 5000;
enable-active-high;
};
In this case the regulator is based on top of a GPIO pin,
so by setting that GPIO line high, some feature on the PCB
will drive a voltage at 3.3V.
A MFD cell spawned regulator can be a very smallish
thing in drivers/regulator. Sure, not 5 lines in the existing
GPIO driver, but still smallish.
You may actually *need* to use the fixed voltage regulator
too: if you have a power-on-delay for it, that the software need
to take into account, the regulator framework is your friend.
Else there will be kludgy code surrounding the GPIO enable()
operation to reimplement the same solution (I have seen this
happen).
So I imagine something like this in drivers/regulator/crystal-panel.c:
#include linux/platform_device.h
#include linux/bitops.h
#include linux/regmap.h
#include linux/mfd/intel_soc_pmic.h
#define PANEL_EN 0x52
static int crystal_enable_regulator(struct regulator_dev *reg)
{
struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
return regmap_update_bits(pmic-regmap, PANEL_EN, 1, 1);
}
static int crystal_disable_regulator(struct regulator_dev *reg)
{
struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
return regmap_update_bits(pmic-regmap, PANEL_EN, 1, 0);
}
static int crystal_is_enabled_regulator(struct regulator_dev *reg)
{
struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
int ret;
unsigned int val;
ret = regmap_read(pmic-regmap, PANEL_EN, val);
if (ret)
return ret;
return val 0x1;
}
static struct regulator_ops crystal_panel_reg_ops = {
.enable = crystal_enable_regulator,
.disable = crystal_disable_regulator,
.is_enabled = crystal_is_enabled_regulator,
};
static struct regulator_desc crystal_panel_regulator = {
.name = crystal-panel,
.fixed_uV = 330, /* Or whatever voltage the panel has */
.ops = crystal_panel_reg_ops,
.id = 1,
.type = REGULATOR_VOLTAGE,
.owner = THIS_MODULE,
.enable_time = NNN, /* This may be relevant! */
};
static int crystalcove_panel_regulator_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev.parent;
struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
struct regulator_config config = { };
config.dev = pdev-dev;
config.driver_data = pmic;
return devm_regulator_register(pdev-dev,
crystal_panel_regulator, config);
}
Untested, but to me simple and straight-forward.
Some stuff may be needed to associate the regulator with the right
device indeed but nothing horribly complicated.
Yours,
Linus Walleij
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable
On Tue, Mar 24, 2015 at 10:50 AM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 24, 2015 at 10:35:32AM +0100, Linus Walleij wrote: Some stuff may be needed to associate the regulator with the right device indeed but nothing horribly complicated. Nack, really. We've had an epic discussion at ks two years ago about how arm gpu drivers go overboard with abstraction. I remember it clearly as I was in the room. And yes IIRC that was indeed very much about the panel abstractions suggested by Laurent Pinchart. We have all the insanity with power domains, clock trees and whatnoelse in i915.ko, but since it's all mostly from the same company we have it integrated into one driver with our own infrastructure. Dave Airlie was telling this everyone and I fully agree with him - pushing abstraction in the middle of the driver without the need for it just causes tears. I fully understand this stance and I kind of understand why it came to this. I had the same discussion a bit with some different graphics people and DRM+panel drivers really are a special chapter when it comes to sequencing stuff. (As is ALSA-soundcard type audio it seems, or anything multimedia.) The problem I have here is that two different pieces on the same board from the same company which won't ever get reused anywhere else need to do cross-driver communication about a gpio line. I don't want any kind of additional abstraction to get into the way at all, the only thing I want are: - some function to switch that line without delays or cleverness interspersed. - dynamic lookup. GPIO seemed a perfect fit, but apparently it isn't. We'll roll our own. I'm not dealing in absolutes so I want to know what makes GPIO such a good fit compared to rolling your own. I guess the alternative is that the i915 driver gets a handle from another platform_device on the MFD cell (a different one, say named panel-power or whatever) and pokes that register itself (using regmap in the same way that my suggested regulator code does basically). I kind of like that because it makes the usecase all evident like I wanted. And arguably it's a better solution if the i915 driver want to be as self-contained as possible, using this pattern that the DRM drivers should take care of all sequencing business. The DRM driver can then also be used even if GPIO is configured out of the kernel. And yeah your code isn't any longer than the gpio version, but regulators drag in all that conceptual stuff about voltage/delays and depencies that imo just isn't controlled by the pmic. We've originally abused the panel interface for all this and Thierry (imo rightfully suggested) that this isn't a panel but it'd be better to expose the underlying stuff. Again imo faking all that stuff since you think this looks like a regulator is imo a worse lie than the exposing this as a gpio. Nah it's all lies :) I think you are right: if the DRM driver wants to control everything itself it should not use the regulator framework, neither the GPIO framework, it seems to be an established pattern to roll your own in these drivers so let's keep with that. I guess the use case is analogous to a monitor cable sticking out of a graphics card and sending these power-up/down sequences to that monitor in some magic way, and that is how DRM thinks of things. For what I know DRM frowns at the abstracted out backlight control used by framebuffers in drivers/video/backlight as well, and prefer to be on top of stuff, so then it should access stuff on as low level as possible. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Wed, Mar 18, 2015 at 2:27 PM, Daniel Vetter dan...@ffwll.ch wrote: On Wed, Mar 18, 2015 at 12:50:51PM +0100, Linus Walleij wrote: This is not a GPIO, this is a special purpose thing and IMO it should be modeled directly as a regulator. The idea behind reusing gpio was that we can reuse the dynamic lookup stuff so that i915 can find the gpio. (Fixed) regulators also have dynamic lookups so they provide the exact same mechanism of associating themselves to devices. And it is a gpio (there's lots more on that chip), Well the other GPIO registers seem to have 8 GPIOs each, so one register control 8 random lines. In this case they obviously put a single bit to control a line out of the PMIC. The register was intended to that purpose only, the register is named PANEL_EN/DISABLE in the refman (right?) and I bet a million to one that the pin on the PMIC and the rail connected to it is also named PANEL_EN/DISABLE or similar. So the hardware engineers had a special purpose in mind for this. Not general purpose (GPIO). it's just that intel tends to hand you recommended board layouts. And there's different ones so only really i915.ko can tell you if it's indeed used to control the panel or not (after consulting a bunch of vbios tables). What about the schematic or PMIC pinout. I suspect it tells you a lot about the intended usage by calling that pin PANEL_EN or so. I'm not saying other usecases are impossible, all Rube Goldberg machine variants are possible, but are other use cases really realistic amongst implementers? Regulator seems way too overkill for this, especially since it tends to be a lie on systems where the panel is not connected to that gpio line. And afaiui the point of the regulator subsystem is that you just ask for the regulator for your IP block and then magically get handed the right bit (or a dummy one). This is very much not the case, hw descriptions on x86 in this area are a kludge worse than board tables since we can't even fix up what the bios hands us anywhere. Depends on the intended use case I would say. Regulator isn't overkill if it fits the use case better. And I think you have a power-on delay, typically so that the panel driver cannot immediately start transmitting data to the panel after driving this line high. Probably the panel needs to settle first. This is referred to as a power-on-delay and the regulator framework can handle it for you. I strongly suspect that you would be planting some delay code in the site enabling the GPIO instead maybe already now or when later running into this while enabling suspend/resume or similar power saving mechanisms. That is basically reimplementing some helpful stuff from the regulator framework. So for me plan B is to just handroll our own thing using the component framework when reusing gpios isn't acceptable. Let's just figure out if GPIO is the best fit first, or if you actually could be helped by fixed regulators. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com So as I think it is better to use a fixed voltage regulator (see code example in the i915 patch) I think this should register a regulator lookup instead. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable
On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij linus.wall...@linaro.org wrote: So summary: - Reusing the dynamic gpio lookup stuff would be nice, and might be interesting as a new crazy use-case (or maybe not). But not a requirement since we have the component framework to handroll something. OK I guess you have me convinced, I will apply the patch from Shobhit. If it turns out ugly we can always revert it. If you believe in it, it's worth a try. Also as you say else it will be reinvented, let's go this way as it is likely the lesser of two evils. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two additional GPIOs. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable and Backlight control are routed thorugh CRC PMIC Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Patch applied, after Daniel convinced me that this is the lesser of several possible evils to the kernel code base. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v5 2/9] gpio/crystalcove: Add additional GPIO for Panel control
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: Export PANEL_EN/DISABLE (offset 0x52) as additional GPIO. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable control is routed thorugh CRC PMIC CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com OK applied *this* version of the patch... Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal
On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com I have changed my mind. Reviewed-by: Linus Walleij linus.wall...@linaro.org Lee, please apply this. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO
On Wed, Mar 25, 2015 at 3:15 PM, Kumar, Shobhit shobhit.ku...@intel.com wrote: On Wed, 2015-03-25 at 13:27 +0100, Linus Walleij wrote: On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two additional GPIOs. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable and Backlight control are routed thorugh CRC PMIC Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Patch applied, after Daniel convinced me that this is the lesser of several possible evils to the kernel code base. Thanks, but I hope you pulled in the patch from the latest series which only exports one of them not both. Ummm I'll double-check. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drivers/gpio/gpiolib: Add support for removing registered consumer lookup table
On Wed, Apr 29, 2015 at 3:59 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: In case we unload and load a driver module again that is registering a lookup table, without this it will result in multiple entries. Provide an option to remove the lookup table on driver unload v2: Ccing maintainers Cc: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Reviewed-by: Alexandre Courbot acour...@nvidia.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Reviewed-by: Linus Walleij linus.wall...@linaro.org Since it is core support I'd prefer to merge this into the GPIO git tree. Is it OK or do you need to merge it through some other means? Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/8] drivers/mfd: Add lookup table for Panel Control as GPIO signal
On Wed, Apr 29, 2015 at 4:00 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load v2: Make the lookup table static (Thierry) Remove the lookup table during driver remove (Thierry) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v6 3/8] drivers/mfd: Add lookup table for Panel Control as GPIO signal
On Fri, Apr 24, 2015 at 5:33 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load v2: Make the lookup table static (Thierry) Remove the lookup table during driver remove (Thierry) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v6 7/8] drm/i915: Use the CRC gpio for panel enable/disable
On Fri, Apr 24, 2015 at 5:33 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: The CRC (Crystal Cove) PMIC, controls the panel enable and disable signals for BYT for dsi panels. This is indicated in the VBT fields. Use that to initialize and use GPIO based control for these signals. v2: Use the newer gpiod interface(Alexandre) v3: Remove the redundant checks and unused code (Ville) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Acked-by: Linus Walleij linus.wall...@linaro.org + /* Panel Enable over CRC PMIC */ + if (intel_dsi-gpio_panel) + gpiod_set_value_cansleep(intel_dsi-gpio_panel, 1); + + msleep(intel_dsi-panel_on_delay); As I predicted, this reimplements .ramp_delay of the fixed regulator. No big disaster, just saying. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/i915: Use the CRC gpio for panel enable/disable
On Wed, Apr 29, 2015 at 4:00 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: The CRC (Crystal Cove) PMIC, controls the panel enable and disable signals for BYT for dsi panels. This is indicated in the VBT fields. Use that to initialize and use GPIO based control for these signals. v2: Use the newer gpiod interface(Alexandre) v3: Remove the redundant checks and unused code (Ville) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drivers/gpio/gpiolib: Add support for removing registered consumer lookup table
On Wed, May 6, 2015 at 5:09 PM, Daniel Vetter dan...@ffwll.ch wrote: On Wed, May 06, 2015 at 04:49:36PM +0200, Linus Walleij wrote: On Wed, Apr 29, 2015 at 3:59 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: In case we unload and load a driver module again that is registering a lookup table, without this it will result in multiple entries. Provide an option to remove the lookup table on driver unload v2: Ccing maintainers Cc: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Reviewed-by: Alexandre Courbot acour...@nvidia.com Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Reviewed-by: Linus Walleij linus.wall...@linaro.org I think the gpio/pwm and mfd parts would all need to go in through the same tree. i915 parts are decoupled. I guess I could do a branch with just those patches, tag it and then send a pull request to all 3 subsystems once it's reviewed. Would that be ok? That's an immutable branch I guess, and yeah I think it's OK. I usually only pull these in if/when there are conflicts in -next. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/8] mfd: intel_soc_pmic_core: Add lookup table for Panel Control as GPIO signal
On Tue, May 5, 2015 at 11:36 AM, Shobhit Kumar shobhit.ku...@intel.com wrote: On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load v2: Make the lookup table static (Thierry) Remove the lookup table during driver remove (Thierry) v3: Correct the subject line (Lee jones) CC: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Acked-by: Lee Jones lee.jo...@linaro.org Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [v2 1/7] gpiolib: Add support for removing registered consumer lookup table
On Mon, Jun 22, 2015 at 12:54 PM, Shobhit Kumar shobhit.ku...@intel.com wrote: In case we unload and load a driver module again that is registering a lookup table, without this it will result in multiple entries. Provide an option to remove the lookup table on driver unload v2: Ccing maintainers v3: Correct the subject line (Lee jones) Cc: Samuel Ortiz sa...@linux.intel.com Cc: Linus Walleij linus.wall...@linaro.org Cc: Alexandre Courbot gnu...@gmail.com Cc: Thierry Reding thierry.red...@gmail.com Reviewed-by: Alexandre Courbot acour...@nvidia.com Reviewed-by: Linus Walleij linus.wall...@linaro.org Signed-off-by: Shobhit Kumar shobhit.ku...@intel.com Do you plan to take this upstream for the next merge window or should I just merge this patch? Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] pinctrl: Intel: add RX invertion config
On Wed, Mar 16, 2016 at 2:34 PM, Daniel Vetter <dan...@ffwll.ch> wrote: > On Wed, Mar 16, 2016 at 01:27:49PM +0100, Linus Walleij wrote: >> - What is a HPD interrupt? > > hotplug interrupt, fires when you plug in a cable. > >> - What is a Type-C DP HPD? > > usb type C connector can multiplex both DisplayPort and USB, you need to > renegotiate the lane splitting every time the sink changes, i.e. on each > hotplug. OK I understand, thanks a lot! >> - Again why can't you just use a notifier or function call? > > Because windows sucks, hence all that coordination happens through hw > forwarded interrupts and magic registers. Same horror story on the sound > side, where the sound driver needs to know what kind of PCM stream the > monitor can take. It's hilarious. Except when they screw up the design and > then need to fake parts of it in software. So the story is something like that these IRQs have been put into hardware in order to compensate for flaws in Windows device driver model, I see. If there are such special registers in some hardware I guess I'm all for implementing some generic quirk in gpiolib for people who need to software-trigger GPIO IRQs. Could be good for testing too, as there are such registers in ARMs PL061 GPIO controller for test, just so as to simulate a GPIO IRQ. gpiod_trig_irq() would work with me, I'm happy to support whatever the GPIO hardware can do usually. > In sound we've switched over to a proper sw interface, and we tie the > different parts (drm graphics driver and alsa snd driver) using the > component.c framework. Hm is that solution or something similar proper for USB connector as well I wonder... I was thinking about just adding $random_notifier but maybe that is a bit ugly :/ >> What is VPG? Now it seems Intel's internal organization is being used as >> part of the argument to get this change in and that makes me a bit >> annoyed. (...) > There was chat of usb type C support for forever, but I was always > promised that we don't need any interactions on the sw side and it's all > magic in hw. There hasn't been any real design discussions in the open > source group. VPG is the hw/windows group and generally comes up with > "interesting" designs not suitable for upstream. > > Feel free to just nack this stuff, and please cc intel-gfx/dri-devel in > the future (since I tend to ignore everything that's not cc'ed to mailing > lists I don't care about, even when I'm on cc personally). I've added them > all to cc. Thanks a lot Daniel, I understand better now. I'm not really against adding this "interesting" workaround if that is how Windows works, we usually have to go by their standards. From the GPIO point of view it is OK, just something the GPIO can do. I would be more worried about what the USB PHY maintainer (Felipe) is going to say about this. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] mfd: intel_soc_pmic_core: Terminate panel control GPIO lookup table correctly
On Fri, Apr 22, 2016 at 9:38 PM, <ville.syrj...@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> GPIO lookup tables are supposed to be zero terminated. Let's do that
> and avoid accidentally walking off the end.
>
> Cc: Shobhit Kumar <shobhit.ku...@intel.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> Cc: Linus Walleij <linus.wall...@linaro.org>
> Cc: Alexandre Courbot <gnu...@gmail.com>
> Cc: Thierry Reding <thierry.red...@gmail.com>
> Cc: Lee Jones <lee.jo...@linaro.org>
> Cc: sta...@vger.kernel.org
> Fixes: 61dd2ca2d44e ("mfd: intel_soc_pmic_core: Add lookup table for Panel
> Control as GPIO signal")
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
Acked-by: Linus Walleij <linus.wall...@linaro.org>
Yours,
Linus Walleij
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH 04/11] drm: Split the display info into static and dynamic parts
On Tue, Feb 27, 2018 at 1:56 PM, Ville Syrjala <ville.syrj...@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > Currently we have a mix of static and dynamic information stored in > the display info structure. That makes it rather difficult to repopulate > the dynamic parts when a new EDID appears. Let's make life easier by > splitting the structure up into static and dynamic parts. > > The static part will consist of subpixel_order, panel_orientation, > and bus_formats. > > Actually I'm not sure where bus_formats & co. fit in all this. For some > drivers those seem to be static, even though they might fill them out > from .get_modes(). For other drivers this stuff even gets frobbed at > runtime, making it more some kind of a bastard encoder/connector state. > I'll just stick it into the static side so that the behaviour doesn't > change when I start clear out the entire dynamic state with memset(). > > Cc: Keith Packard <kei...@keithp.com> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > Cc: Hans de Goede <hdego...@redhat.com> > Cc: Shashank Sharma <shashank.sha...@intel.com> > Cc: Stefan Agner <ste...@agner.ch> > Cc: Thierry Reding <thierry.red...@gmail.com> > Cc: Boris Brezillon <boris.brezil...@bootlin.com> > Cc: Philipp Zabel <p.za...@pengutronix.de> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Manfred Schlaegl <manfred.schla...@gmx.at> > Cc: Marek Vasut <ma...@denx.de> > Cc: Archit Taneja <arch...@codeaurora.org> > Cc: Andrzej Hajda <a.ha...@samsung.com> > Cc: Alison Wang <alison.w...@freescale.com> > Cc: Eric Anholt <e...@anholt.net> > Cc: Linus Walleij <linus.wall...@linaro.org> > Cc: linux-renesas-...@vger.kernel.org > Cc: Maxime Ripard <maxime.rip...@bootlin.com> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> Acked-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/2] drm/panel: panel-innolux: drop unused variable
On Sun, May 26, 2019 at 8:05 PM Sam Ravnborg wrote: > The num_supplies variable is not used, delete it. > Build tested. > > Signed-off-by: Sam Ravnborg > Cc: Thierry Reding > Cc: David Airlie > Cc: Daniel Vetter Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/2] drm/panel: drop drmP.h usage
On Sun, May 26, 2019 at 8:05 PM Sam Ravnborg wrote: > Drop use of the deprecated drmP.h header file. > > While touching the list of include files: > - Divide include files in blocks of linux/* video/* drm/* etc. > Be consistent in the order of the blocks > - Sort individual blocks of include files > > Signed-off-by: Sam Ravnborg > Cc: Thierry Reding > Cc: Linus Walleij > Cc: Stefan Mavrodiev > Cc: David Airlie > Cc: Daniel Vetter Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 32/59] drm/tve2000: Drop drm_gem_prime_export/import
On Fri, Jun 14, 2019 at 10:36 PM Daniel Vetter wrote: > They're the default. > > Aside: Would be really nice to switch the others over to > drm_gem_object_funcs. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij When you write "Would be really nice to switch the others over to drm_gem_object_funcs." are you referring to the other drm driver function called by this one driver, or are you referring to other drivers not using these functions? If the former and if there is some nice upstream commit I should be looking at for inspiration I can take a stab at fixing this driver. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 18/59] drm/mcde: Drop drm_gem_prime_export/import
On Fri, Jun 14, 2019 at 10:36 PM Daniel Vetter wrote: > They're the default. > > Aside: Would be really nice to switch the others over to > drm_gem_object_funcs. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 0/8] drm/fb-helper: Move modesetting code to drm_client
On Thu, May 23, 2019 at 6:53 PM Sam Ravnborg wrote: > Could one of you take a look at this series. > Daniel already ack'ed the series on irc, but an extra pair of eyes > is never bad. I would if I had a chance of understanding them. I am still pretty novice with DRM so what I do is trace down to the calls I need and understand the small pieces I use. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT
On Mon, Dec 2, 2019 at 4:49 PM Hans de Goede 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915/dsi: Move Crystal Cove PMIC panel GPIO lookup from mfd to the i915 driver
On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede wrote: > Move the Crystal Cove PMIC panel GPIO lookup-table from > drivers/mfd/intel_soc_pmic_core.c to the i915 driver. > > The moved looked-up table is adding a GPIO lookup to the i915 PCI > device and the GPIO subsys allows only one lookup table per device, > > The intel_soc_pmic_core.c code only adds lookup-table entries for the > PMIC panel GPIO (as it deals only with the PMIC), but we also need to be > able to access some GPIOs on the SoC itself, which requires entries for > these GPIOs in the lookup-table. > > Since the lookup-table is attached to the i915 PCI device it really > should be part of the i915 driver, this will also allow us to extend > it with GPIOs from other sources when necessary. > > Signed-off-by: Hans de Goede Looks OK to me Acked-by: Linus Walleij But Lee & Andy should have a final word on this. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915/dsi: Control panel and backlight enable GPIOs on BYT
On Sun, Dec 15, 2019 at 5:38 PM 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-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 Looks good to me: Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915/dsi: Move poking of panel-enable GPIO to intel_dsi_vbt.c
On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede wrote: > On some older devices (BYT, CHT) which may use v2 VBT MIPI-sequences, > we need to manually control the panel enable GPIO as v2 sequences do > not do this. > > So far we have been carrying the code to do this on BYT/CHT devices > with a Crystal Cove PMIC in vlv_dsi.c, but as this really is a shortcoming > of the VBT MIPI-sequences, intel_dsi_vbt.c is a better place for this, > so move it there. > > This is a preparation patch for adding panel-enable and backlight-enable > GPIO support for BYT devices where instead of the PMIC the SoC is used > for backlight control. > > Signed-off-by: Hans de Goede The kernel looks prettier after than before and it seems correct so: Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/dsi: Init panel-enable GPIO to low when the LCD is initially off
On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede wrote: > When the LCD has not been turned on by the firmware/GOP, because e.g. the > device was booted with an external monitor connected over HDMI, we should > not turn on the panel-enable GPIO when we request it. > > Turning on the panel-enable GPIO when we request it, means we turn it on > too early in the init-sequence, which causes some panels to not correctly > light up. > > This commits adds a panel_is_on parameter to intel_dsi_vbt_gpio_init() > and makes intel_dsi_vbt_gpio_init() set the initial GPIO value accordingly. > > This fixes the panel not lighting up on a Thundersoft TST168 tablet when > booted with an external monitor connected over HDMI. > > Signed-off-by: Hans de Goede Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT
On Sun, Dec 15, 2019 at 5:38 PM Hans de Goede wrote: > Linus, this series starts with the already discussed pinctrl change to > export the function to unregister a pinctrl-map. We can either merge this > through drm-intel, or you could pick it up and then provide an immutable > branch with it for merging into drm-intel-next. Which option do you prefer? I have created an immutable branch with these changes and pulled it to my "devel" branch for v5.6: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-pinctrl-unreg-mappings Please pull this in and put the other patches on top of that. I had a bit of mess in my subsystems last kernel cycle so I want to avoid that by strictly including all larger commits in my trees. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] drm/i915/dsi: Control panel and backlight enable GPIOs from VBT
On Mon, Dec 16, 2019 at 12:11 PM Hans de Goede wrote: > Ugh, taking one last look at the "pinctrl: Export pinctrl_unregister_mappings" > patch it is no good, sorry. Ooops! > Linus, can you please drop this from your -next ? Sure, done. > So I see 2 options: > 1) Add an orig_map member to maps_node and use that in the comparison, > this is IMHO somewhat ugly > > 2) Add a new pinctrl_register_mappings_no_dup helper and document in > pinctrl_unregister_mappings kdoc that it can only be used together > with the no_dup variant. > > I believe that 2 is by far the best option. Linus do you agree or > do you have any other suggestions? What about (3) look for all calls to pinctrl_register_mappings() in the kernel. Hey it is 2 places in total: arch/arm/mach-u300/core.c: pinctrl_register_mappings(u300_pinmux_map, drivers/pinctrl/cirrus/pinctrl-madera-core.c: ret = pinctrl_register_mappings(pdata->gpio_configs, Delete __initdata from the u300 table, the other one seems safe. Fold this into your patch. Go with the original idea. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/simple-kms: Standardize arguments for callbacks
On Wed, Oct 23, 2019 at 12:13 PM Daniel Vetter wrote: > Passing the wrong type feels icky, everywhere else we use the pipe as > the first parameter. Spotted while discussing patches with Thomas > Zimmermann. > > v2: Make xen compile correctly > > Acked-By: Thomas Zimmermann (v1) > Cc: Thomas Zimmermann > Cc: Noralf Trønnes > Cc: Gerd Hoffmann > Cc: Eric Anholt > Cc: Emil Velikov > Cc: virtualizat...@lists.linux-foundation.org > Cc: Linus Walleij > Signed-off-by: Daniel Vetter Makes perfect sense. Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT
Hi Hans, thank you for your patch! On Fri, Nov 29, 2019 at 7:58 PM Hans de Goede 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] pinctrl: baytrail: Add GPIO lookup and pinctrl-map for i915 DSI panel ctrl
Hi Hans!
Thanks for your patch!
On Fri, Nov 29, 2019 at 7:58 PM 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.
>
> Signed-off-by: Hans de Goede
(...)
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 == _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm: Nuke mode->vrefresh
On Tue, Feb 25, 2020 at 8:27 PM Ville Syrjälä wrote: > OK, so I went ahead a wrote a bit of cocci [1] to find the bad apples. That's impressive :D > Unfortunately it found a lot of strange stuff: I will answer for the weirdness I caused. I have long suspected that a whole bunch of the "simple" displays are not simple but contains a display controller and memory. That means that the speed over the link to the display and actual refresh rate on the actual display is asymmetric because well we are just updating a RAM, the resolution just limits how much data we are sending, the clock limits the speed on the bus over to the RAM on the other side. In most cases I thing the clock is the way to go. > panel-sony-acx424akp.c:51/sony_acx424akp_vid_mode: 60 vs. 727 (.clock=33 > .htotal=480 + 15 + 0 + 15 .vtotal=864 + 14 + 1 + 11) I suspect clock should be adjusted after vfresh = 60 here instead of the other way around. I couldn't quite test the video mode, but the vendor driver (no documentation ) does state 330 MHz which seems a bit high. Just drop vrefresh for now. > panel-sony-acx424akp.c:71/sony_acx424akp_cmd_mode: 60 vs. 711 (.clock=420160 > .htotal=480 + 154 + 16 + 32 .vtotal=864 + 1 + 1 + 1) You can override this ignoring the vrefresh, this is a command-mode only, and in command mode the refresh doesn't come into play, or is very high and limited by a bunch of other overhead than just the resolution. The command mode HS clock is @420+ MHz indeed. Tests showed around 116 Hz for this particular display in practice with continuous updates. > panel-ilitek-ili9322.c:543/srgb_320x240_mode: 60 vs. 10168 (.clock=2453500 > .htotal=320 + 359 + 1 + 241 .vtotal=262) > panel-ilitek-ili9322.c:587/yuv_640x320_mode: 60 vs. 7768 (.clock=2454000 > .htotal=640 + 252 + 1 + 28 .vtotal=320 + 4 + 1 + 18) > panel-ilitek-ili9322.c:616/itu_r_bt_656_640_mode: 60 vs. 5358 (.clock=2454000 > .htotal=640 + 3 + 1 + 272 .vtotal=500) > panel-ilitek-ili9322.c:557/srgb_360x240_mode: 60 vs. 16178 (.clock=270 > .htotal=360 + 35 + 1 + 241 .vtotal=262) > panel-ilitek-ili9322.c:601/yuv_720x360_mode: 60 vs. 7071 (.clock=270 > .htotal=720 + 252 + 1 + 24 .vtotal=360 + 4 + 1 + 18) > panel-ilitek-ili9322.c:631/itu_r_bt_656_720_mode: 60 vs. 5422 (.clock=270 > .htotal=720 + 3 + 1 + 272 .vtotal=500) > panel-ilitek-ili9322.c:572/prgb_320x240_mode: 60 vs. 59725 (.clock=640 > .htotal=320 + 38 + 1 + 50 .vtotal=262) This is the datasheet if you want to take a look: https://dflund.se/~triad/krad/dlink-dir-685/ILI9322DS_V1.12.pdf The one platform using this is using the 8 bit ITU-R BT.656 640Y 320CbCr mode which (I think) is similar to DSI command mode again: there is a stream of data in a burst and you decide how often you want to send it because the panel always has backing memory and the speed out to the physical display is something completely different. You can safely delete vrefresh from all of these sites. > panel-arm-versatile.c:161/versatile_panels[]: 60 vs. 61 (.clock=25000 > .htotal=640 + 24 + 96 + 24 .vtotal=480 + 11 + 2 + 32) > panel-arm-versatile.c:208/versatile_panels[]: 116 vs. 59 (.clock=5400 > .htotal=240 + 10 + 10 + 20 .vtotal=320 + 2 + 2 + 2) > panel-arm-versatile.c:184/versatile_panels[]: 390 vs. 1523 (.clock=62500 > .htotal=176 + 2 + 3 + 3 .vtotal=220 + 0 + 2 + 1) The only driver drivers/gpu/drm/pl111/pl111_display.c Uses mode->clock so just drop vrefresh. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] (no subject)
On Wed, Feb 26, 2020 at 12:57 PM Ville Syrjälä wrote: > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote: > > I have long suspected that a whole bunch of the "simple" displays > > are not simple but contains a display controller and memory. > > That means that the speed over the link to the display and > > actual refresh rate on the actual display is asymmetric because > > well we are just updating a RAM, the resolution just limits how > > much data we are sending, the clock limits the speed on the > > bus over to the RAM on the other side. > > IMO even in command mode mode->clock should probably be the actual > dotclock used by the display. If there's another clock for the bus > speed/etc. it should be stored somewhere else. Good point. For the DSI panels we have the field hs_rate for the HS clock in struct mipi_dsi_device which is based on exactly this reasoning. And that is what I actually use for setting the HS clock. The problem is however that we in many cases have so substandard documentation of these panels that we have absolutely no idea about the dotclock. Maybe we should just set it to 0 in these cases? Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] (no subject)
On Wed, Feb 26, 2020 at 3:34 PM Ville Syrjälä
wrote:
> On Wed, Feb 26, 2020 at 01:08:06PM +0100, Linus Walleij wrote:
> > On Wed, Feb 26, 2020 at 12:57 PM Ville Syrjälä
> > wrote:
> > > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote:
> >
> > > > I have long suspected that a whole bunch of the "simple" displays
> > > > are not simple but contains a display controller and memory.
> > > > That means that the speed over the link to the display and
> > > > actual refresh rate on the actual display is asymmetric because
> > > > well we are just updating a RAM, the resolution just limits how
> > > > much data we are sending, the clock limits the speed on the
> > > > bus over to the RAM on the other side.
> > >
> > > IMO even in command mode mode->clock should probably be the actual
> > > dotclock used by the display. If there's another clock for the bus
> > > speed/etc. it should be stored somewhere else.
> >
> > Good point. For the DSI panels we have the field hs_rate
> > for the HS clock in struct mipi_dsi_device which is based
> > on exactly this reasoning. And that is what I actually use for
> > setting the HS clock.
> >
> > The problem is however that we in many cases have so
> > substandard documentation of these panels that we have
> > absolutely no idea about the dotclock. Maybe we should
> > just set it to 0 in these cases?
>
> Don't you always have a TE interrupt or something like that
> available? Could just measure it from that if no better
> information is available?
Yes and I did exactly that, so that is why this comment is in
the driver:
static const struct drm_display_mode sony_acx424akp_cmd_mode = {
(...)
/*
* Some desired refresh rate, experiments at the maximum "pixel"
* clock speed (HS clock 420 MHz) yields around 117Hz.
*/
.vrefresh = 60,
I got a review comment at the time saying 117 Hz was weird.
We didn't reach a proper conclusion on this:
https://lore.kernel.org/dri-devel/CACRpkdYW3YNPSNMY3A44GQn8DqK-n9TLvr7uipF7LM_DHZ5=l...@mail.gmail.com/
Thierry wasn't sure if 60Hz was good or not, so I just had to
go with something.
We could calculate the resulting pixel clock for ~117 Hz with
this resolution and put that in the clock field but ... don't know
what is the best?
Yours,
Linus Walleij
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 33/52] drm/mcde: Drop explicit drm_mode_config_cleanup call
On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter wrote: > Allows us to drop the drm_driver.release callback. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 34/52] drm/mcde: More devm_drm_dev_init
On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter wrote: > Auto-unwind ftw, now possible with the fixed drm_device related > management. > > Aside, clk/regulator seem to be missing devm versions for a bunch of > functions, preventing a pile of these simpler drivers from outright > losing their ->remove hook. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/52] drm/mcde: Use drmm_add_final_kfree
On Wed, Feb 19, 2020 at 11:21 AM Daniel Vetter wrote: > With this we can drop the final kfree from the release function. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] drm: Put drm_display_mode on diet
On Wed, Feb 19, 2020 at 9:35 PM Ville Syrjala wrote: > drm/exynos: Use mode->clock instead of reverse calculating it from the > vrefresh > drm: Nuke mode->vrefresh I'm sure this is fine. Acked-by: Linus Walleij We need one: either clock or refresh settings, so it does make sense to calculate one from the other. So is the reasoning such that we mostly know the clock and the resolutions and then calculate vrefresh? (It makes sense to me in a way.) What I am worried about here is that some of these (especially panels) are probably already out of sync so we need to ascertain that the clock is correct-ish everywhere, so let's be prepared for some regressions. For command-mode DSI panels the vrefresh can become quite weird I think, the vrefresh is different from what you can reverse calculate (I know this from experiments) but I doubt we care much. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/5] pinctrl: Allow modules to use pinctrl_[un]register_mappings
On Mon, Dec 16, 2019 at 9:51 PM Hans de Goede wrote: > Currently only the drivers/pinctrl/devicetree.c code allows registering > pinctrl-mappings which may later be unregistered, all other mappings > are assumed to be permanent. > > Non-dt platforms may also want to register pinctrl mappings from code which > is build as a module, which requires being able to unregister the mapping > when the module is unloaded to avoid dangling pointers. > > To allow unregistering the mappings the devicetree code uses 2 internal > functions: pinctrl_register_map and pinctrl_unregister_map. > > pinctrl_register_map allows the devicetree code to tell the core to > not memdup the mappings as it retains ownership of them and > pinctrl_unregister_map does the unregistering, note this only works > when the mappings where not memdupped. > > The only code relying on the memdup/shallow-copy done by > pinctrl_register_mappings is arch/arm/mach-u300/core.c this commit > replaces the __initdata with const, so that the shallow-copy is no > longer necessary. > > After that we can get rid of the internal pinctrl_unregister_map function > and just use pinctrl_register_mappings directly everywhere. > > This commit also renames pinctrl_unregister_map to > pinctrl_unregister_mappings so that its naming matches its > pinctrl_register_mappings counter-part and exports it. > > Together these 2 changes will allow non-dt platform code to > register pinctrl-mappings from modules without breaking things on > module unload (as they can now unregister the mapping on unload). > > Signed-off-by: Hans de Goede This v2 works fine for me, I applied it to this immutable branch in the pinctrl tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-pinctrl-unreg-mappings And pulled that into the pinctrl "devel" branch for v5.6. Please pull this immutable branch into the Intel DRM tree and apply the rest of the stuff on top! Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 28/59] drm/mcde: Use devm_drm_dev_alloc
On Wed, Apr 15, 2020 at 9:41 AM Daniel Vetter wrote: > Already using devm_drm_dev_init, so very simple replacment. > > v2: Move misplaced double-assignement to next patch (Sam) > > Acked-by: Sam Ravnborg > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 33/44] drm/mcde: Don't use drm_device->dev_private
On Fri, Apr 3, 2020 at 3:59 PM Daniel Vetter wrote: > Upcasting using a container_of macro is more typesafe, faster and > easier for the compiler to optimize. > > Signed-off-by: Daniel Vetter > Cc: Linus Walleij Nice, thanks! Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 12/17] drm/mcde: Use mode->clock instead of reverse calculating it from the vrefresh
On Fri, Apr 3, 2020 at 10:41 PM Ville Syrjala wrote: > From: Ville Syrjälä > > htotal*vtotal*vrefresh ~= clock. So just say "clock" when we mean it. > > Cc: Linus Walleij > Cc: Sam Ravnborg > Signed-off-by: Ville Syrjälä Indeed :) Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [patch 15/30] pinctrl: nomadik: Use irq_has_action()
On Thu, Dec 10, 2020 at 8:42 PM Thomas Gleixner wrote: > Let the core code do the fiddling with irq_desc. > > Signed-off-by: Thomas Gleixner > Cc: Linus Walleij > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-g...@vger.kernel.org Reviewed-by: Linus Walleij I suppose you will funnel this directly to Torvalds, else tell me and I'll apply it to my tree. Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [patch 16/30] mfd: ab8500-debugfs: Remove the racy fiddling with irq_desc
On Thu, Dec 10, 2020 at 8:42 PM Thomas Gleixner wrote: > First of all drivers have absolutely no business to dig into the internals > of an irq descriptor. That's core code and subject to change. All of this > information is readily available to /proc/interrupts in a safe and race > free way. > > Remove the inspection code which is a blatant violation of subsystem > boundaries and racy against concurrent modifications of the interrupt > descriptor. > > Print the irq line instead so the information can be looked up in a sane > way in /proc/interrupts. > > Signed-off-by: Thomas Gleixner > Cc: Linus Walleij > Cc: Lee Jones > Cc: linux-arm-ker...@lists.infradead.org Reviewed-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/11] drm/tiny: drm_gem_simple_display_pipe_prepare_fb is the default
On Fri, May 21, 2021 at 11:10 AM Daniel Vetter wrote: > Goes through all the drivers and deletes the default hook since it's > the default now. > > Signed-off-by: Daniel Vetter > Cc: Joel Stanley > Cc: Andrew Jeffery > Cc: "Noralf Trønnes" > Cc: Linus Walleij > Cc: Emma Anholt > Cc: David Lechner > Cc: Kamlesh Gurudasani > Cc: Oleksandr Andrushchenko > Cc: Daniel Vetter > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Sam Ravnborg > Cc: Alex Deucher > Cc: Andy Shevchenko > Cc: linux-asp...@lists.ozlabs.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: xen-de...@lists.xenproject.org Acked-by: Linus Walleij Yours, Linus Walleij ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix typo 'the the' in comment
On Thu, Jul 21, 2022 at 8:24 AM Slark Xiao wrote: > Replace 'the the' with 'the' in the comment. > > Signed-off-by: Slark Xiao Patch applied! Yours, Linus Walleij
