diff mbox

[1/4] gpio: Add AXP209 GPIO driver

Message ID 1457520614-32239-2-git-send-email-maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard March 9, 2016, 10:50 a.m. UTC
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 <maxime.ripard@free-electrons.com>
---
 .../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

Comments

kernel test robot March 9, 2016, 1:07 p.m. UTC | #1
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
Peter Korsgaard March 9, 2016, 1:20 p.m. UTC | #2
>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> 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?
Linus Walleij March 16, 2016, 9:56 a.m. UTC | #3
On Wed, Mar 9, 2016 at 11:50 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> 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 <maxime.ripard@free-electrons.com>

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
Linus Walleij March 16, 2016, 9:57 a.m. UTC | #4
On Wed, Mar 9, 2016 at 2:20 PM, Peter Korsgaard <peter@korsgaard.com> wrote:
>>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> 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
Rob Herring (Arm) March 17, 2016, 7:10 p.m. UTC | #5
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 <maxime.ripard@free-electrons.com>
> ---
>  .../devicetree/bindings/gpio/gpio-axp209.txt       |  30 ++++

Acked-by: Rob Herring <robh@kernel.org>

>  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 mbox

Patch

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 <maxime.ripard@free-electrons.com>
+ *
+ * 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 <maxime.ripard@free-electrons.com>");
+MODULE_DESCRIPTION("AXP20x PMIC GPIO driver");
+MODULE_LICENSE("GPL");