Hi,
The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
of GPIOs accessible. Some boards use these to control their backlight
or a few LEDs.
There's supposed to be 4 of them, but the fourth one has a different
configuration register scheme, and I couldn't find any board that was
using this GPIO. It will be probably be supported eventually, but
until then, we support only the first 3 GPIOs.
Let me know what you think,
Maxime
Maxime Ripard (4):
gpio: Add AXP209 GPIO driver
mfd: axp20x: Add AXP209 GPIO driver to the mfd
ARM: dt: axp209: Add AXP209 GPIO driver
ARM: sun5i: chip: Add status LED
.../devicetree/bindings/gpio/gpio-axp209.txt | 30 ++++
arch/arm/boot/dts/axp209.dtsi | 6 +
arch/arm/boot/dts/sun5i-r8-chip.dts | 10 ++
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-axp209.c | 166 +++++++++++++++++++++
drivers/mfd/axp20x.c | 3 +
7 files changed, 222 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-axp209.txt
create mode 100644 drivers/gpio/gpio-axp209.c
--
2.7.2
The AXP209 PMIC has a bunch of GPIOs accessible, that are usually used to
control LEDs or backlight.
Add a driver for them
Signed-off-by: Maxime Ripard <[email protected]>
---
.../devicetree/bindings/gpio/gpio-axp209.txt | 30 ++++
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-axp209.c | 166 +++++++++++++++++++++
4 files changed, 203 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-axp209.txt
create mode 100644 drivers/gpio/gpio-axp209.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-axp209.txt b/Documentation/devicetree/bindings/gpio/gpio-axp209.txt
new file mode 100644
index 000000000000..a6611304dd3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-axp209.txt
@@ -0,0 +1,30 @@
+AXP209 GPIO controller
+
+This driver follows the usual GPIO bindings found in
+Documentation/devicetree/bindings/gpio/gpio.txt
+
+Required properties:
+- compatible: Should be "x-powers,axp209-gpio"
+- #gpio-cells: Should be two. The first cell is the pin number and the
+ second is the GPIO flags.
+- gpio-controller: Marks the device node as a GPIO controller.
+
+This node must be a subnode of the axp20x PMIC, documented in
+Documentation/devicetree/bindings/mfd/axp20x.txt
+
+Example:
+
+axp209: pmic@34 {
+ compatible = "x-powers,axp209";
+ reg = <0x34>;
+ interrupt-parent = <&nmi_intc>;
+ interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ axp_gpio: gpio {
+ compatible = "x-powers,axp209-gpio";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1118fef45a86..cd5ab93ac197 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -136,6 +136,12 @@ config GPIO_ATH79
Select this option to enable GPIO driver for
Atheros AR71XX/AR724X/AR913X SoC devices.
+config GPIO_AXP209
+ tristate "X-Powers AXP209 PMIC GPIO Support"
+ depends on MFD_AXP20X
+ help
+ Say yes to enable GPIO support for the AXP209 PMIC
+
config GPIO_BCM_KONA
bool "Broadcom Kona GPIO"
depends on OF_GPIO && (ARCH_BCM_MOBILE || COMPILE_TEST)
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6f969df1431a..e9e7fbe80ab7 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_AMDPT) += gpio-amdpt.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
+obj-$(CONFIG_GPIO_AXP209) += gpio-axp209.o
obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
diff --git a/drivers/gpio/gpio-axp209.c b/drivers/gpio/gpio-axp209.c
new file mode 100644
index 000000000000..822f39faaadf
--- /dev/null
+++ b/drivers/gpio/gpio-axp209.c
@@ -0,0 +1,166 @@
+/*
+ * AXP20x GPIO driver
+ *
+ * Copyright (C) 2016 Maxime Ripard <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define AXP20X_GPIO_FUNCTIONS 0x7
+#define AXP20X_GPIO_FUNCTION_OUT_LOW 0
+#define AXP20X_GPIO_FUNCTION_OUT_HIGH 1
+#define AXP20X_GPIO_FUNCTION_INPUT 2
+
+struct axp20x_gpio {
+ struct gpio_chip chip;
+ struct regmap *regmap;
+};
+
+struct axp20x_gpio *to_axp20x_gpio(struct gpio_chip *chip)
+{
+ return container_of(chip, struct axp20x_gpio, chip);
+}
+
+static int axp20x_gpio_get_reg(unsigned offset)
+{
+ switch (offset) {
+ case 0:
+ return AXP20X_GPIO0_CTRL;
+ case 1:
+ return AXP20X_GPIO1_CTRL;
+ case 2:
+ return AXP20X_GPIO2_CTRL;
+ }
+
+ return -EINVAL;
+}
+
+static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset)
+{
+ struct axp20x_gpio *gpio = to_axp20x_gpio(chip);
+ int reg;
+
+ reg = axp20x_gpio_get_reg(offset);
+ if (reg < 0)
+ return reg;
+
+ return regmap_update_bits(gpio->regmap, reg,
+ AXP20X_GPIO_FUNCTIONS,
+ AXP20X_GPIO_FUNCTION_INPUT);
+}
+
+static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ struct axp20x_gpio *gpio = to_axp20x_gpio(chip);
+ unsigned int val;
+ int reg, ret;
+
+ reg = axp20x_gpio_get_reg(offset);
+ if (reg < 0)
+ return reg;
+
+ ret = regmap_read(gpio->regmap, reg, &val);
+ if (ret)
+ return ret;
+
+ return val & (1 << (offset + 4));
+}
+
+static int axp20x_gpio_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ struct axp20x_gpio *gpio = to_axp20x_gpio(chip);
+ int reg;
+
+ reg = axp20x_gpio_get_reg(offset);
+ if (reg < 0)
+ return reg;
+
+ return regmap_update_bits(gpio->regmap, reg,
+ AXP20X_GPIO_FUNCTIONS,
+ value ? AXP20X_GPIO_FUNCTION_OUT_HIGH
+ : AXP20X_GPIO_FUNCTION_OUT_LOW);
+}
+
+static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ axp20x_gpio_output(chip, offset, value);
+}
+
+static int axp20x_gpio_probe(struct platform_device *pdev)
+{
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+ struct axp20x_gpio *gpio;
+ int ret;
+
+ if (!of_device_is_available(pdev->dev.of_node))
+ return -ENODEV;
+
+ if (!axp20x) {
+ dev_err(&pdev->dev, "Parent drvdata not set\n");
+ return -EINVAL;
+ }
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->chip.base = -1;
+ gpio->chip.can_sleep = true;
+ gpio->chip.dev = &pdev->dev;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.owner = THIS_MODULE;
+ gpio->chip.get = axp20x_gpio_get;
+ gpio->chip.set = axp20x_gpio_set;
+ gpio->chip.direction_input = axp20x_gpio_input;
+ gpio->chip.direction_output = axp20x_gpio_output;
+ gpio->chip.ngpio = 3;
+
+ gpio->regmap = axp20x->regmap;
+
+ ret = gpiochip_add(&gpio->chip);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register GPIO chip\n");
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "AXP209 GPIO driver loaded\n");
+
+ return 0;
+}
+
+static const struct of_device_id axp20x_gpio_match[] = {
+ { .compatible = "x-powers,axp209-gpio" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, axp20x_gpio_match);
+
+static struct platform_driver axp20x_gpio_driver = {
+ .probe = axp20x_gpio_probe,
+ .driver = {
+ .name = "axp20x-gpio",
+ .of_match_table = axp20x_gpio_match,
+ },
+};
+
+module_platform_driver(axp20x_gpio_driver);
+
+MODULE_AUTHOR("Maxime Ripard <[email protected]>");
+MODULE_DESCRIPTION("AXP20x PMIC GPIO driver");
+MODULE_LICENSE("GPL");
--
2.7.2
Now that we have a GPIO driver for the AXP209, we can add it to our MFD.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/mfd/axp20x.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index a57d6e940610..89518ee3be33 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -430,6 +430,9 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
static struct mfd_cell axp20x_cells[] = {
{
+ .name = "axp20x-gpio",
+ .of_compatible = "x-powers,axp209-gpio",
+ }, {
.name = "axp20x-pek",
.num_resources = ARRAY_SIZE(axp20x_pek_resources),
.resources = axp20x_pek_resources,
--
2.7.2
Add the AXP209 GPIO node to our AXP209 DTSI so that boards can use it.
Signed-off-by: Maxime Ripard <[email protected]>
---
arch/arm/boot/dts/axp209.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
index 051ab3ba9a65..5430054f7c60 100644
--- a/arch/arm/boot/dts/axp209.dtsi
+++ b/arch/arm/boot/dts/axp209.dtsi
@@ -53,6 +53,12 @@
interrupt-controller;
#interrupt-cells = <1>;
+ axp_gpio: gpio {
+ compatible = "x-powers,axp209-gpio";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
regulators {
/* Default work frequency for buck regulators */
x-powers,dcdc-freq = <1500>;
--
2.7.2
The CHIP has a status LED connected to one of the AXP GPIOs. Add the
gpio-leds node to be able to use the proper LED framework to control it.
Signed-off-by: Maxime Ripard <[email protected]>
---
arch/arm/boot/dts/sun5i-r8-chip.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
index f6898c6b84d4..9a54df81ece7 100644
--- a/arch/arm/boot/dts/sun5i-r8-chip.dts
+++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
@@ -64,6 +64,16 @@
chosen {
stdout-path = "serial0:115200n8";
};
+
+ leds {
+ compatible = "gpio-leds";
+
+ status {
+ label = "chip:white:status";
+ gpios = <&axp_gpio 2 GPIO_ACTIVE_HIGH>;
+ default-state = "on";
+ };
+ };
};
&codec {
--
2.7.2
Hi,
On 09-03-16 11:50, Maxime Ripard wrote:
> Hi,
>
> The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
> of GPIOs accessible. Some boards use these to control their backlight
> or a few LEDs.
Thanks for working on this, but IMHO this cannot go upstream like this,
the gpio pins on the axp pmics need a pinctrl driver, not a gpio
driver. I.E. on the axp209 gpio0 and gpio1 can also be used to output
an additional low-noise ldo (so as a regulator), or as an adc input.
I've been working on gsl1680 touchscreen support lately and on at least
a few a23 tablets, the low-noise ldo is used as AVCC for the touchscreen
controller. Now these use an axp223 pmic, but nothing is stopping
someone from doing something similar with an axp209 and I think it
would be best to support this from day one, rather then hope we can
retro-fit this later without breaking dts.
> There's supposed to be 4 of them, but the fourth one has a different
> configuration register scheme, and I couldn't find any board that was
> using this GPIO. It will be probably be supported eventually, but
> until then, we support only the first 3 GPIOs.
He he, this matches what I've done in u-boot, the 4th gpio indeed
is somewhat funky (and the docs are not quite clear on how it works
exactly IIRC), leaving it out for now indeed seems for the best.
Regards,
Hans
Hi Maxime,
[auto build test ERROR on gpio/for-next]
[also build test ERROR on next-20160309]
[cannot apply to v4.5-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Maxime-Ripard/Add-AXP209-GPIO-driver/20160309-190907
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpio/gpio-axp209.c: In function 'axp20x_gpio_probe':
>> drivers/gpio/gpio-axp209.c:126:12: error: 'struct gpio_chip' has no member named 'dev'
gpio->chip.dev = &pdev->dev;
^
vim +126 drivers/gpio/gpio-axp209.c
120 gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
121 if (!gpio)
122 return -ENOMEM;
123
124 gpio->chip.base = -1;
125 gpio->chip.can_sleep = true;
> 126 gpio->chip.dev = &pdev->dev;
127 gpio->chip.label = dev_name(&pdev->dev);
128 gpio->chip.owner = THIS_MODULE;
129 gpio->chip.get = axp20x_gpio_get;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
>>>>> "Maxime" == Maxime Ripard <[email protected]> writes:
> The AXP209 PMIC has a bunch of GPIOs accessible, that are usually used to
> control LEDs or backlight.
Do you find 3 'a bunch'? ;)
> +static int axp20x_gpio_probe(struct platform_device *pdev)
> +{
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> + struct axp20x_gpio *gpio;
> + int ret;
> +
> + if (!of_device_is_available(pdev->dev.of_node))
> + return -ENODEV;
> +
> + if (!axp20x) {
> + dev_err(&pdev->dev, "Parent drvdata not set\n");
> + return -EINVAL;
> + }
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->chip.base = -1;
> + gpio->chip.can_sleep = true;
> + gpio->chip.dev = &pdev->dev;
> + gpio->chip.label = dev_name(&pdev->dev);
> + gpio->chip.owner = THIS_MODULE;
> + gpio->chip.get = axp20x_gpio_get;
> + gpio->chip.set = axp20x_gpio_set;
> + gpio->chip.direction_input = axp20x_gpio_input;
> + gpio->chip.direction_output = axp20x_gpio_output;
> + gpio->chip.ngpio = 3;
> +
> + gpio->regmap = axp20x->regmap;
This could just use dev_get_regmap(pdev.dev->parent, NULL) instead of
fiddling in the parent driver data.
> +
> + ret = gpiochip_add(&gpio->chip);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register GPIO chip\n");
> + return ret;
> + }
> +
> + dev_info(&pdev->dev, "AXP209 GPIO driver loaded\n");
Any reason to be so noisy?
--
Bye, Peter Korsgaard
Hi,
On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
> Hi,
>
> On 09-03-16 11:50, Maxime Ripard wrote:
> >Hi,
> >
> >The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
> >of GPIOs accessible. Some boards use these to control their backlight
> >or a few LEDs.
>
> Thanks for working on this, but IMHO this cannot go upstream like this,
> the gpio pins on the axp pmics need a pinctrl driver, not a gpio
> driver. I.E. on the axp209 gpio0 and gpio1 can also be used to output
> an additional low-noise ldo (so as a regulator), or as an adc input.
Eventually, yes, it needs both. But they don't even have to be the
same driver, since they provide two different features. The only
reason we have that construct in the pio case is because they share
the same address space, but in the AXP case, the regmap and our mfd
take care of that already.
> I've been working on gsl1680 touchscreen support lately and on at least
> a few a23 tablets, the low-noise ldo is used as AVCC for the touchscreen
> controller.
Yeah, the AXP209 also has an ADC connected to these pins.
> Now these use an axp223 pmic, but nothing is stopping someone from
> doing something similar with an axp209 and I think it would be best
> to support this from day one, rather then hope we can retro-fit this
> later without breaking dts.
I considered that, but I don't see how it would break the DT later. If
someone wants to enable say the ADC, he will of course have to add the
pinctrl driver, and the pinctrl handles, but the old DT will only
reference the gpio driver directly, which would still be something
that would work.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi,
On 09-03-16 16:28, Maxime Ripard wrote:
> Hi,
>
> On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 09-03-16 11:50, Maxime Ripard wrote:
>>> Hi,
>>>
>>> The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
>>> of GPIOs accessible. Some boards use these to control their backlight
>>> or a few LEDs.
>>
>> Thanks for working on this, but IMHO this cannot go upstream like this,
>> the gpio pins on the axp pmics need a pinctrl driver, not a gpio
>> driver. I.E. on the axp209 gpio0 and gpio1 can also be used to output
>> an additional low-noise ldo (so as a regulator), or as an adc input.
>
> Eventually, yes, it needs both. But they don't even have to be the
> same driver, since they provide two different features. The only
> reason we have that construct in the pio case is because they share
> the same address space, but in the AXP case, the regmap and our mfd
> take care of that already.
Hmm, so your suggesting to have mfd instantiate 2 platform devices
for this, a gpio and a pinctrl device, each with their own
driver. Yes that would work, but I'm a bit worried about the 2
racing or some such since they both will end up touching
bit 0-2 of register 0x90 / 0x92, more-over since they are both
touching the exact same bits I've the feeling that this really
should be one driver.
I guess that in a proper written dts we either use pinctrl to enable
a special function, or gpio, but still.
>> I've been working on gsl1680 touchscreen support lately and on at least
>> a few a23 tablets, the low-noise ldo is used as AVCC for the touchscreen
>> controller.
>
> Yeah, the AXP209 also has an ADC connected to these pins.
>
>> Now these use an axp223 pmic, but nothing is stopping someone from
>> doing something similar with an axp209 and I think it would be best
>> to support this from day one, rather then hope we can retro-fit this
>> later without breaking dts.
>
> I considered that, but I don't see how it would break the DT later. If
> someone wants to enable say the ADC, he will of course have to add the
> pinctrl driver, and the pinctrl handles, but the old DT will only
> reference the gpio driver directly, which would still be something
> that would work.
I was assuming we would use one mfd-child(-platform)-device for this,
not two. I guess that with 2 devices you're right and there should
not be any problem, still as said it feels wrong-ish to have 2 drivers
poking bits 0-2 of reg 0x90 / reg 0x92.
Regards,
Hans
On Wed, Mar 9, 2016 at 11:50 AM, Maxime Ripard
<[email protected]> wrote:
> The AXP209 PMIC has a bunch of GPIOs accessible, that are usually used to
> control LEDs or backlight.
>
> Add a driver for them
>
> Signed-off-by: Maxime Ripard <[email protected]>
OK...
> +++ b/Documentation/devicetree/bindings/gpio/gpio-axp209.txt
Some people insist that bindings be sent separately from the
drivers but I don't care. Especially not for this simple binding.
> +AXP209 GPIO controller
Write something more about the hardware here. For example the
quite obvious fact that it is part of an bigger MFD device.
In some cases people put all the bindings inside a single
file in bindings/mfd/*, follow Lee's recommendation here, I have
no strong opinion.
> +axp209: pmic@34 {
> + compatible = "x-powers,axp209";
Doesn't this need "simple-mfd" if the GPIO subdriver shall
probe properly?
> + reg = <0x34>;
> + interrupt-parent = <&nmi_intc>;
> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + axp_gpio: gpio {
> + compatible = "x-powers,axp209-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + };
> +};
(...)
> +++ b/drivers/gpio/gpio-axp209.c
(...)
> +#include <linux/device.h>
> +#include <linux/gpio.h>
Should only need <linux/gpio/driver.h>
> +struct axp20x_gpio *to_axp20x_gpio(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct axp20x_gpio, chip);
> +}
No. Use devm_gpiochip_add_data() and gpiochip_get_data()
to get the pointer back.
> +static int axp20x_gpio_get_reg(unsigned offset)
> +{
> + switch (offset) {
> + case 0:
> + return AXP20X_GPIO0_CTRL;
> + case 1:
> + return AXP20X_GPIO1_CTRL;
> + case 2:
> + return AXP20X_GPIO2_CTRL;
> + }
> +
> + return -EINVAL;
> +}
Can't you just:
static u8 regs[] = {AXP20X_GPIO0_CTRL, AXP20X_GPIO1_CTRL, AXP20X_GPIO2_CTRL};
static int axp20x_gpio_get_reg(unsigned offset)
{
if (offset >= ARRAY_SIZE(regs))
return -EINVAL;
return regs[offset];
}
> +static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct axp20x_gpio *gpio = to_axp20x_gpio(chip);
> + unsigned int val;
> + int reg, ret;
> +
> + reg = axp20x_gpio_get_reg(offset);
> + if (reg < 0)
> + return reg;
> +
> + ret = regmap_read(gpio->regmap, reg, &val);
> + if (ret)
> + return ret;
> +
> + return val & (1 << (offset + 4));
This doesn't clamp to [0,1]. Please do this instead:
#include <linux/bitops.h>
return !!(val & BIT(offset+4));
> +static int axp20x_gpio_probe(struct platform_device *pdev)
> +{
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> + struct axp20x_gpio *gpio;
> + int ret;
> +
> + if (!of_device_is_available(pdev->dev.of_node))
> + return -ENODEV;
> +
> + if (!axp20x) {
> + dev_err(&pdev->dev, "Parent drvdata not set\n");
> + return -EINVAL;
> + }
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->chip.base = -1;
> + gpio->chip.can_sleep = true;
> + gpio->chip.dev = &pdev->dev;
This is renamed .parent upstream, ick use latest kernel as base for
your patches ;)
> + gpio->chip.label = dev_name(&pdev->dev);
> + gpio->chip.owner = THIS_MODULE;
> + gpio->chip.get = axp20x_gpio_get;
> + gpio->chip.set = axp20x_gpio_set;
> + gpio->chip.direction_input = axp20x_gpio_input;
> + gpio->chip.direction_output = axp20x_gpio_output;
> + gpio->chip.ngpio = 3;
> +
> + gpio->regmap = axp20x->regmap;
> +
> + ret = gpiochip_add(&gpio->chip);
devm_gpiochip_add_data() as mentioned.
Yours,
Linus Walleij
On Wed, Mar 9, 2016 at 2:20 PM, Peter Korsgaard <[email protected]> wrote:
>>>>>> "Maxime" == Maxime Ripard <[email protected]> writes:
> > + dev_info(&pdev->dev, "AXP209 GPIO driver loaded\n");
>
> Any reason to be so noisy?
The GPIO maintainer likes boasty drivers.
I don't understand people who are dmesg minimalists, I want to
have positive indications that things work in there, not just bad news
about what went wrong.
Yours,
Linus Walleij
On Wed, Mar 9, 2016 at 11:50 AM, Maxime Ripard
<[email protected]> wrote:
> Now that we have a GPIO driver for the AXP209, we can add it to our MFD.
>
> Signed-off-by: Maxime Ripard <[email protected]>
(...)
> + .name = "axp20x-gpio",
> + .of_compatible = "x-powers,axp209-gpio",
Acked-by: Linus Walleij <[email protected]>
Professor X must be behind this X-powers X men.
Yours,
Linus Walleij
On Wed, Mar 9, 2016 at 11:50 AM, Maxime Ripard
<[email protected]> wrote:
> Add the AXP209 GPIO node to our AXP209 DTSI so that boards can use it.
>
> Signed-off-by: Maxime Ripard <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Mar 9, 2016 at 4:44 PM, Hans de Goede <[email protected]> wrote:
>> On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
>> Eventually, yes, it needs both. But they don't even have to be the
>> same driver, since they provide two different features. The only
>> reason we have that construct in the pio case is because they share
>> the same address space, but in the AXP case, the regmap and our mfd
>> take care of that already.
>
> Hmm, so your suggesting to have mfd instantiate 2 platform devices
> for this, a gpio and a pinctrl device, each with their own
> driver. Yes that would work, but I'm a bit worried about the 2
> racing or some such since they both will end up touching
> bit 0-2 of register 0x90 / 0x92, more-over since they are both
> touching the exact same bits I've the feeling that this really
> should be one driver.
We can put the driver in drivers/pinctrl/pinctrl-axp209.c from day
one, add in comments stating that it only implements GPIO
for now and that the GPIO portion must call
pinctrl_request_gpio() and the pin controller must implement
.gpio_request_enable() the day it is added so the GPIOs do
not conflict with other use of the pins.
Requireing a huge slew of upfront code is a bit hard on simple
drivers I think.
Also we have the solution in drivers/mfd/stmpe.c that just add
a simple mux code when pins on mixsigs or simple expanders
can just mux some two-three different functions, then I think
pin control may be a bit too thick overhead. (Not sure where
the limit is though.)
Yours,
Linus Walleij
Hi,
On 16-03-16 11:10, Linus Walleij wrote:
> On Wed, Mar 9, 2016 at 4:44 PM, Hans de Goede <[email protected]> wrote:
>>> On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
>
>>> Eventually, yes, it needs both. But they don't even have to be the
>>> same driver, since they provide two different features. The only
>>> reason we have that construct in the pio case is because they share
>>> the same address space, but in the AXP case, the regmap and our mfd
>>> take care of that already.
>>
>> Hmm, so your suggesting to have mfd instantiate 2 platform devices
>> for this, a gpio and a pinctrl device, each with their own
>> driver. Yes that would work, but I'm a bit worried about the 2
>> racing or some such since they both will end up touching
>> bit 0-2 of register 0x90 / 0x92, more-over since they are both
>> touching the exact same bits I've the feeling that this really
>> should be one driver.
>
> We can put the driver in drivers/pinctrl/pinctrl-axp209.c from day
> one, add in comments stating that it only implements GPIO
> for now and that the GPIO portion must call
> pinctrl_request_gpio() and the pin controller must implement
> .gpio_request_enable() the day it is added so the GPIOs do
> not conflict with other use of the pins.
>
> Requireing a huge slew of upfront code is a bit hard on simple
> drivers I think.
>
> Also we have the solution in drivers/mfd/stmpe.c that just add
> a simple mux code when pins on mixsigs or simple expanders
> can just mux some two-three different functions, then I think
> pin control may be a bit too thick overhead. (Not sure where
> the limit is though.)
I actually found out this weekend, that we already have
a problem here. Regulator on/off is implemented as
2 mux settings, just like gpio high/low/input are mux settings:
On the axp209 the gpio pin mux bit table looks like this:
000: low output
001: output (3.3V)
010: universal input function
011: low noise LDO5
100: ADC input
1XX: floating
And on the axp221 / axp223:
000: output low
001: output high (3.3V)
010: input
011: LDOIO0 off
100: LDOIO0 on
1XX: floating
Note the LDOIO0 off / LDOIO0 on being 2 different mux
settings, and then existing axp20x regulator driver for
LD05 / LDOIO0 actually already implements regulator on / off
by changing the mux for both axp209 and axp22x.
This results in the following potentially happening.
0) kernel boots
1) The gpio driver is in use for these pins, sets the pin
to say output low
2) kernel is ready to start init, calls the regulators
subsys late_init call, which disables any unused regulators,
this results in the mux getting set to 7 (axp209) resp
4 (axp22x), and the gpio is no longer driven low as
the intend of the driver using the pin was.
So we really need either some coordination here, or a way
to tell the regulator-driver to not register ldo5 / ldoio#
at all, so that the regulator-core won't try to turn them
off changing the mux.
I've submitted a patch series this weekend which allows
putting "status = disabled" in regulator child-dts-nodes, which
will then result in not registering a regulator at all,
which would fix that. But that series has not received any
feedback yet...
Regards,
Hans
On Wed, Mar 09, 2016 at 11:50:11AM +0100, Maxime Ripard wrote:
> The AXP209 PMIC has a bunch of GPIOs accessible, that are usually used to
> control LEDs or backlight.
>
> Add a driver for them
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> .../devicetree/bindings/gpio/gpio-axp209.txt | 30 ++++
Acked-by: Rob Herring <[email protected]>
> drivers/gpio/Kconfig | 6 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-axp209.c | 166 +++++++++++++++++++++
> 4 files changed, 203 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-axp209.txt
> create mode 100644 drivers/gpio/gpio-axp209.c
On Wed, 09 Mar 2016, Maxime Ripard wrote:
> Now that we have a GPIO driver for the AXP209, we can add it to our MFD.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/mfd/axp20x.c | 3 +++
> 1 file changed, 3 insertions(+)
Applied, thanks.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index a57d6e940610..89518ee3be33 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -430,6 +430,9 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
>
> static struct mfd_cell axp20x_cells[] = {
> {
> + .name = "axp20x-gpio",
> + .of_compatible = "x-powers,axp209-gpio",
> + }, {
> .name = "axp20x-pek",
> .num_resources = ARRAY_SIZE(axp20x_pek_resources),
> .resources = axp20x_pek_resources,
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi,
On Wed, Mar 09, 2016 at 04:44:01PM +0100, Hans de Goede wrote:
> Hi,
>
> On 09-03-16 16:28, Maxime Ripard wrote:
> >Hi,
> >
> >On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 09-03-16 11:50, Maxime Ripard wrote:
> >>>Hi,
> >>>
> >>>The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
> >>>of GPIOs accessible. Some boards use these to control their backlight
> >>>or a few LEDs.
> >>
> >>Thanks for working on this, but IMHO this cannot go upstream like this,
> >>the gpio pins on the axp pmics need a pinctrl driver, not a gpio
> >>driver. I.E. on the axp209 gpio0 and gpio1 can also be used to output
> >>an additional low-noise ldo (so as a regulator), or as an adc input.
> >
> >Eventually, yes, it needs both. But they don't even have to be the
> >same driver, since they provide two different features. The only
> >reason we have that construct in the pio case is because they share
> >the same address space, but in the AXP case, the regmap and our mfd
> >take care of that already.
>
> Hmm, so your suggesting to have mfd instantiate 2 platform devices
> for this, a gpio and a pinctrl device, each with their own
> driver. Yes that would work, but I'm a bit worried about the 2
> racing or some such since they both will end up touching
> bit 0-2 of register 0x90 / 0x92, more-over since they are both
> touching the exact same bits I've the feeling that this really
> should be one driver.
I don't think that's an issue. We basically have two cases here:
either we have a driver using the pin or not.
If we have a driver using it, then pinctrl will make sure we have an
exclusive use of the pin, before the gpio is requested and we start
changing that value. If not, then no one cares anyway.
>
> I guess that in a proper written dts we either use pinctrl to enable
> a special function, or gpio, but still.
>
> >>I've been working on gsl1680 touchscreen support lately and on at least
> >>a few a23 tablets, the low-noise ldo is used as AVCC for the touchscreen
> >>controller.
> >
> >Yeah, the AXP209 also has an ADC connected to these pins.
> >
> >>Now these use an axp223 pmic, but nothing is stopping someone from
> >>doing something similar with an axp209 and I think it would be best
> >>to support this from day one, rather then hope we can retro-fit this
> >>later without breaking dts.
> >
> >I considered that, but I don't see how it would break the DT later. If
> >someone wants to enable say the ADC, he will of course have to add the
> >pinctrl driver, and the pinctrl handles, but the old DT will only
> >reference the gpio driver directly, which would still be something
> >that would work.
>
> I was assuming we would use one mfd-child(-platform)-device for this,
> not two. I guess that with 2 devices you're right and there should
> not be any problem, still as said it feels wrong-ish to have 2 drivers
> poking bits 0-2 of reg 0x90 / reg 0x92.
Note that the PIO is built on the same model and it works, while we
have 3 drivers in one. The only reason why we have everything linked
to the same device is because we have to share the address space, but
regmap takes care of that here.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi,
On Tue, Mar 22, 2016 at 3:22 PM, Maxime Ripard
<[email protected]> wrote:
> Hi,
>
> On Wed, Mar 09, 2016 at 04:44:01PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 09-03-16 16:28, Maxime Ripard wrote:
>> >Hi,
>> >
>> >On Wed, Mar 09, 2016 at 01:17:50PM +0100, Hans de Goede wrote:
>> >>Hi,
>> >>
>> >>On 09-03-16 11:50, Maxime Ripard wrote:
>> >>>Hi,
>> >>>
>> >>>The axp209 PMIC used in combination to some Allwinner SoCs has a bunch
>> >>>of GPIOs accessible. Some boards use these to control their backlight
>> >>>or a few LEDs.
>> >>
>> >>Thanks for working on this, but IMHO this cannot go upstream like this,
>> >>the gpio pins on the axp pmics need a pinctrl driver, not a gpio
>> >>driver. I.E. on the axp209 gpio0 and gpio1 can also be used to output
>> >>an additional low-noise ldo (so as a regulator), or as an adc input.
>> >
>> >Eventually, yes, it needs both. But they don't even have to be the
>> >same driver, since they provide two different features. The only
>> >reason we have that construct in the pio case is because they share
>> >the same address space, but in the AXP case, the regmap and our mfd
>> >take care of that already.
>>
>> Hmm, so your suggesting to have mfd instantiate 2 platform devices
>> for this, a gpio and a pinctrl device, each with their own
>> driver. Yes that would work, but I'm a bit worried about the 2
>> racing or some such since they both will end up touching
>> bit 0-2 of register 0x90 / 0x92, more-over since they are both
>> touching the exact same bits I've the feeling that this really
>> should be one driver.
>
> I don't think that's an issue. We basically have two cases here:
> either we have a driver using the pin or not.
>
> If we have a driver using it, then pinctrl will make sure we have an
> exclusive use of the pin, before the gpio is requested and we start
> changing that value. If not, then no one cares anyway.
AFAIK pinctrl binding happens before probe time, and is tied to the
DT. Each driver requesting pins would have to have a separate node.
Seems like there isn't one for the regulators.
Or we'd have to manually configure them for each regulator that needs
them, and list the relevant pinctrl properties under its own node.
One "on" and "off" setting pair for each LDO_IO regulator. We could
then tie the settings to the regulator device, not the underlying
mfd sub-device.
ChenYu
>>
>> I guess that in a proper written dts we either use pinctrl to enable
>> a special function, or gpio, but still.
>>
>> >>I've been working on gsl1680 touchscreen support lately and on at least
>> >>a few a23 tablets, the low-noise ldo is used as AVCC for the touchscreen
>> >>controller.
>> >
>> >Yeah, the AXP209 also has an ADC connected to these pins.
>> >
>> >>Now these use an axp223 pmic, but nothing is stopping someone from
>> >>doing something similar with an axp209 and I think it would be best
>> >>to support this from day one, rather then hope we can retro-fit this
>> >>later without breaking dts.
>> >
>> >I considered that, but I don't see how it would break the DT later. If
>> >someone wants to enable say the ADC, he will of course have to add the
>> >pinctrl driver, and the pinctrl handles, but the old DT will only
>> >reference the gpio driver directly, which would still be something
>> >that would work.
>>
>> I was assuming we would use one mfd-child(-platform)-device for this,
>> not two. I guess that with 2 devices you're right and there should
>> not be any problem, still as said it feels wrong-ish to have 2 drivers
>> poking bits 0-2 of reg 0x90 / reg 0x92.
>
> Note that the PIO is built on the same model and it works, while we
> have 3 drivers in one. The only reason why we have everything linked
> to the same device is because we have to share the address space, but
> regmap takes care of that here.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
Hi Lee,
On Mon, Mar 21, 2016 at 11:54:10AM +0000, Lee Jones wrote:
> On Wed, 09 Mar 2016, Maxime Ripard wrote:
>
> > Now that we have a GPIO driver for the AXP209, we can add it to our MFD.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > drivers/mfd/axp20x.c | 3 +++
> > 1 file changed, 3 insertions(+)
>
> Applied, thanks.
There's still some discussions about the whole design of the driver,
can you drop that patch for now?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Tue, 29 Mar 2016, Maxime Ripard wrote:
> Hi Lee,
>
> On Mon, Mar 21, 2016 at 11:54:10AM +0000, Lee Jones wrote:
> > On Wed, 09 Mar 2016, Maxime Ripard wrote:
> >
> > > Now that we have a GPIO driver for the AXP209, we can add it to our MFD.
> > >
> > > Signed-off-by: Maxime Ripard <[email protected]>
> > > ---
> > > drivers/mfd/axp20x.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> >
> > Applied, thanks.
>
> There's still some discussions about the whole design of the driver,
> can you drop that patch for now?
Sure. Done.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog