On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter <daniel@xxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
