| Message ID | 1426480967-20029-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 16, 2015 at 5:42 AM, Shobhit Kumar <shobhit.kumar@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 <sameo@linux.intel.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Cc: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Shobhit Kumar <shobhit.kumar@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
On Wed, Mar 18, 2015 at 01:19:33PM +0100, Linus Walleij wrote: > On Mon, Mar 16, 2015 at 5:42 AM, Shobhit Kumar <shobhit.kumar@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 <sameo@linux.intel.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Alexandre Courbot <gnurou@gmail.com> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Signed-off-by: Shobhit Kumar <shobhit.kumar@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. Hm, somehow my reply here got eaten. I've looked at the regulator stuff and that doesn't make sense. The problem here is that i915.ko is in control of the regulator crap like on/off delays and proper ordering with everything else that must happen to enable/disable the panel without it falling over. Also i915.ko is the only thing that actually knows which gpio line to use (there's atm 3 different board designs afaik). 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). 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. Anyway if you still think gpio is totally the wrong thing then we'll just roll our own using the component framework. regulator is definitely a even bigger mismatch at least for our usage. I just figured it would make sense to reuse existing common infrastructure where it fits. Thanks, Daniel
On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter <daniel@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 = <3300000>; regulator-max-microvolt = <3300000>; 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 = 3300000, /* 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
On Tue, Mar 24, 2015 at 10:35:32AM +0100, Linus Walleij wrote: > On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter <daniel@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. Maybe I wasnt' clear: This is the i915 gfx driver, we noodle this information out of an even internally underspecified vbios table. It _is_ worse than board files. Also these tables specify the precise power sequence using forth (that part isn't in this series). Implementing a regulator for this would be a even bigger lie than whatever kind of lie use see in exposing this as a gpio. > > 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 = <3300000>; > regulator-max-microvolt = <3300000>; > 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 = 3300000, /* 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. Nack, really. We've had an epic discussion at ks two years ago about how arm gpu drivers go overboard with abstraction. 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. 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. 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. -Daniel
On Tue, Mar 24, 2015 at 10:50 AM, Daniel Vetter <daniel@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
On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 24, 2015 at 10:50 AM, Daniel Vetter <daniel@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. Yeah the discussion started because of Laurent's panel framework, but it was very much a general concern. A bunch of the arm gfx drivers are a pile of modules, and the resulting coordinatino chaos makes suspend/resume and driver load a real challenge to get right. Drivers which only modularize where it's absolutely needed are much easier to understand. >> 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). The alternative would be to use the component framework with some hardcoded names. Think board file distributed through all drivers with no abstraction to get at handles for different pieces. We already have that between i915 and snd-hda since the coordination to get audio-over-dp/hdmi right is a bit tricky. We could have partially reused e.g. power domains, but since the power domain/clock tree i915 manages isn't using generic infrastructure (yet, who knows) that didn't look like a good idea. power domains are actually a nice example for the tradeoffs: Currently the i915 power domain code doen't implement hystersis. So for now we just implement it ourselves for the one domain that really needs it. But it might make sense to rebase the i915 power domain code to use core power domains internally if we need hystersis in more places. > 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. So the main reason I thought reusing something common would make sense here is that we wouldn't need to roll our own lookup structure stuff and could reuse the logic provided by gpio for board files. Longer-term we might even have switched to gpio abstraction internally (maybe, not sure) since there's a bunch more board design floating around (and sometimes shipping already like this one here) where the panel line isn't controlled by i915 directly. Also I just wanted to figure out whether use-cases like this (irrespective of gpio, pwm, regulator or whatever) which are in some sense worse than board files would be even acceptable. At least I expect even more fun in the future since intel socs get integrated ever tighter. Most of the interactions are handled by magic in some power/clock firmware controllers (to make windows work), but there's always small rifts in the seams like here where they didn't remap some random register across the entire chip. Which the hw people generally do, e.g. we can access the southbridge i2c engine through the same mmio window as we access the gpu on the main die ;-) Fundamentally we just don't have a nice description of the hw like in dt with all the power domains/regulators and depencies described in an abstract and unified way. It's a bad mess of hard-coded knowledge in the driver, magic firmware to orchestrate cross-driver interactions and the occasional explicit cross-driver communication in software like this one where it all falls apart. >> 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. Yeah backlight is similar, for most panels we need to frob it at precise points or otherwise the source port or panel sink falls over. So overall it's a judgement call, but here I've thought the benefits of using the gpio lookup stuff (and maybe the benefits to the subsystem for accepting yet another crazy use-case and learning some interesting things) outweight the costs - also because the gpio abstraction is a very dumb/simple one. What I wanted is something that magically gives me a driver to flip a line without asking questions or imposing anything, that seemed to fit the gpio abstraction. Of course there's a full panel driver sitting on top, but in a way that's one layer up. At least to me. And in that upper layer the abstraction will most likely get in the way sooner or later. 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. - More abstraction than "pls flick this line for me" is probably too much since all the higher level logic must be in i915 because of the already shipping design of the vbios tables and all that. - I expect more of this kind of remote panel stuff and if we handroll we'll probably end up reinventing gpio&pwm partially. Cheers, Daniel
On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij > <linus.walleij@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
On Wed, Mar 25, 2015 at 01:24:01PM +0100, Linus Walleij wrote: > On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij > > <linus.walleij@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. Thanks for reconsidering. I quickly checked out your linux-gpio and it only has patch 2 to implement the gpio. We also need patch 1 (but with the leak Thierry spotted fixed). Should we drop Shobit's patch until that's done? Wrt merging the 4.1 window on the drm side is pretty much closed so I think I'll have to postpone the i915 side to 4.2 anyway. Luckily there's no direct depencies because we look up everything dynamically, so patches can go in in any order. Cheers, Daniel
On 03/25/2015 06:43 PM, Daniel Vetter wrote: > On Wed, Mar 25, 2015 at 01:24:01PM +0100, Linus Walleij wrote: >> On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij >>> <linus.walleij@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. > > Thanks for reconsidering. > > I quickly checked out your linux-gpio and it only has patch 2 to implement > the gpio. We also need patch 1 (but with the leak Thierry spotted fixed). > Should we drop Shobit's patch until that's done? Will work on this. Regards Shobhit > > Wrt merging the 4.1 window on the drm side is pretty much closed so I > think I'll have to postpone the i915 side to 4.2 anyway. Luckily there's > no direct depencies because we look up everything dynamically, so patches > can go in in any order. > > Cheers, Daniel >
On Wed, Mar 25, 2015 at 2:13 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > I quickly checked out your linux-gpio and it only has patch 2 to implement > the gpio. We also need patch 1 (but with the leak Thierry spotted fixed). > Should we drop Shobit's patch until that's done? Nah I trust that Lee will merge it to MFD in due time, there is no compile-time dependence anyway. I Reviewed/ACKed the patch anyway, probably should have pointed out to fix the leak pointed out by Thierry but I count on you folks to fix it. Can I add you ACK tag on the GPIO patch? Yours, Linus Wallei
On Wed, Mar 25, 2015 at 03:55:43PM +0100, Linus Walleij wrote: > On Wed, Mar 25, 2015 at 2:13 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > I quickly checked out your linux-gpio and it only has patch 2 to implement > > the gpio. We also need patch 1 (but with the leak Thierry spotted fixed). > > Should we drop Shobit's patch until that's done? > > Nah I trust that Lee will merge it to MFD in due time, there is no > compile-time dependence anyway. I Reviewed/ACKed the patch > anyway, probably should have pointed out to fix the leak > pointed out by Thierry but I count on you folks to fix it. > > Can I add you ACK tag on the GPIO patch? Sure. And yes Shobit is already working on the leak fix, so either it'll be a fixup or new patch version. Shobit, to make sure that your patch will apply please base the series on top of linux-next and not drm-intel-nightly for the next iteration. https://www.kernel.org/doc/man-pages/linux-next.html Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c8c8b24..06a5c30 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -31,6 +31,7 @@ #include <drm/drm_panel.h> #include <drm/drm_mipi_dsi.h> #include <linux/slab.h> +#include <linux/gpio/consumer.h> #include "i915_drv.h" #include "intel_drv.h" #include "intel_dsi.h" @@ -415,6 +416,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder) DRM_DEBUG_KMS("\n"); + /* 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); + /* Disable DPOunit clock gating, can stall pipe * and we need DPLL REFA always enabled */ tmp = I915_READ(DPLL(pipe)); @@ -432,8 +439,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder) /* put device in ready state */ intel_dsi_device_ready(encoder); - msleep(intel_dsi->panel_on_delay); - drm_panel_prepare(intel_dsi->panel); for_each_dsi_port(port, intel_dsi->ports) @@ -576,6 +581,10 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder) msleep(intel_dsi->panel_off_delay); msleep(intel_dsi->panel_pwr_cycle_delay); + + /* Panel Disable over CRC PMIC */ + if (intel_dsi->gpio_panel) + gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0); } static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, @@ -955,6 +964,11 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder) /* XXX: Logically this call belongs in the panel driver. */ drm_panel_remove(intel_dsi->panel); } + + /* dispose of the gpios */ + if (intel_dsi->gpio_panel) + gpiod_put(intel_dsi->gpio_panel); + intel_encoder_destroy(encoder); } @@ -1070,6 +1084,20 @@ void intel_dsi_init(struct drm_device *dev) goto err; } + /* + * In case of BYT with CRC PMIC, we need to use GPIO for + * Panel control. + */ + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { + intel_dsi->gpio_panel = + gpiod_get(dev->dev, "panel", 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_encoder->type = INTEL_OUTPUT_DSI; intel_encoder->cloneable = 0; drm_connector_init(dev, connector, &intel_dsi_connector_funcs, diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 2784ac4..bf1bade 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -29,6 +29,9 @@ #include <drm/drm_mipi_dsi.h> #include "intel_drv.h" +#define PPS_BLC_PMIC 0 +#define PPS_BLC_SOC 1 + /* Dual Link support */ #define DSI_DUAL_LINK_NONE 0 #define DSI_DUAL_LINK_FRONT_BACK 1 @@ -42,6 +45,9 @@ struct intel_dsi { struct drm_panel *panel; struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS]; + /* GPIO Desc for CRC based Panel control */ + struct gpio_desc *gpio_panel; + struct intel_connector *attached_connector; /* bit mask of ports being driven */
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 <sameo@linux.intel.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Cc: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> --- drivers/gpu/drm/i915/intel_dsi.c | 32 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_dsi.h | 6 ++++++ 2 files changed, 36 insertions(+), 2 deletions(-)