[Intel-gfx] [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch