diff mbox

[v2,2/3] gpio: Add support for TPS68470 GPIOs

Message ID 1497161395-36504-3-git-send-email-rajmohan.mani@intel.com
State New
Headers show

Commit Message

Mani, Rajmohan June 11, 2017, 6:09 a.m. UTC
This patch adds support for TPS68470 GPIOs.
There are 7 GPIOs and a few sensor related GPIOs.
These GPIOs can be requested and configured as
appropriate.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/gpio/Kconfig         |  14 ++++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tps68470.c | 186 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)
 create mode 100644 drivers/gpio/gpio-tps68470.c

Comments

Andy Shevchenko June 11, 2017, 1:43 p.m. UTC | #1
On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
> This patch adds support for TPS68470 GPIOs.
> There are 7 GPIOs and a few sensor related GPIOs.
> These GPIOs can be requested and configured as
> appropriate.

Main points (some I already told in an answer to Sakari's mail):
1. Consider 2 GPIO chips over 1.
2. Fix FIXME(s).
3. If there is hardware bug we should work around it must be clarified.
4. You missed Linus' comments here (switch to the data pointer inside
GPIO chip and remove platform driver data stuff from the driver).
Mani, Rajmohan June 12, 2017, 9:14 a.m. UTC | #2
Hi Andy,

> Subject: Re: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs

> 

> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani

> <rajmohan.mani@intel.com> wrote:

> > This patch adds support for TPS68470 GPIOs.

> > There are 7 GPIOs and a few sensor related GPIOs.

> > These GPIOs can be requested and configured as appropriate.

> 

> Main points (some I already told in an answer to Sakari's mail):

> 1. Consider 2 GPIO chips over 1.


I am looking into this.

> 2. Fix FIXME(s).


Leaving this on, until I see how this can be fixed.

> 3. If there is hardware bug we should work around it must be clarified.


Ack
If this is about initializing the GPIOs with zero, I have removed this code for now.

> 4. You missed Linus' comments here (switch to the data pointer inside GPIO

> chip and remove platform driver data stuff from the driver).

> 


I have fixed this with v3
Mani, Rajmohan July 6, 2017, 1:49 a.m. UTC | #3
Hi Andy and all,

Thanks for your patience.

> >

> > Main points (some I already told in an answer to Sakari's mail):

> > 1. Consider 2 GPIO chips over 1.

> 

> I am looking into this.

> 


For the second GPIO chip, the call to acpi_attach_data() fails with AE_ALREADY_EXISTS, as below with 4.12 rc3 kernel.

acpi_gpiochip_add()
	acpi_attach_data() 
		acpi_ns_attach_data() fails with AE_ALREADY_EXISTS for the second GPIO chip.
		/* We only allow one attachment per handler */ drivers/acpi/acpica/nsobject.c

Also, I tried the following experiment, since we know the number of GPIOs for the second GPIO chip is 3, just to see if I can get further along after a successful call to acpi_attach_data().

Since the acpi_gpio_chip_dh is used as the handler in all acpi_gpiochip_* calls, I tried to use another handler (acpi_gpio_chip_dh2). But now, as expected, acpi_gpiochip_request_regions() failed inside acpi_gpiochip_add(), as only one OpRegion handler can be installed for a given OpRegion code. Due to this, when I try to set the GPIO 9 (which is now GPIO 2 of the second GPIO chip of 2 GPIO chips implementation), the OpRegion handler is called with the information / region_context pointer of the first GPIO chip, which is not desired.

It sounds like there are 2 issues with the 2 GPIO chips implementation.

1) How to get acpi_ns_attach_data() done successfully for the second GPIO chip, given the observation above and

2) How to get the GPIO OpRegion handler to be called with the right GPIO chip information / region_context pointer

I need pointers to make further progress here.

> > 2. Fix FIXME(s).

> 

> Leaving this on, until I see how this can be fixed.

> 


These FIXME code will be removed from the driver, as we are working to get this done through platform firmware.

> > 3. If there is hardware bug we should work around it must be clarified.

> 

> Ack

> If this is about initializing the GPIOs with zero, I have removed this code for

> now.

> 

> > 4. You missed Linus' comments here (switch to the data pointer inside

> > GPIO chip and remove platform driver data stuff from the driver).

> >

> 

> I have fixed this with v3
diff mbox

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 23ca51e..9ba647e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1041,6 +1041,20 @@  config GPIO_TPS65912
 	help
 	  This driver supports TPS65912 gpio chip
 
+config GPIO_TPS68470
+	bool "TPS68470 GPIO"
+	depends on MFD_TPS68470
+	help
+	  Select this option to enable GPIO driver for the TPS68470
+	  chip family.
+	  There are 7 GPIOs and few sensor related GPIOs supported
+	  by the TPS68470. While the 7 GPIOs can be configured as
+	  input or output as appropriate, the sensor related GPIOs
+	  are "output only" GPIOs.
+
+	  This driver config is bool, as the GPIO functionality
+	  of the TPS68470 must be available before dependent
+	  drivers are loaded.
 config GPIO_TWL4030
 	tristate "TWL4030, TWL5030, and TPS659x0 GPIOs"
 	depends on TWL4030_CORE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 68b9627..a2659cb 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -120,6 +120,7 @@  obj-$(CONFIG_GPIO_TPS65218)	+= gpio-tps65218.o
 obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
+obj-$(CONFIG_GPIO_TPS68470)	+= gpio-tps68470.o
 obj-$(CONFIG_GPIO_TS4800)	+= gpio-ts4800.o
 obj-$(CONFIG_GPIO_TS4900)	+= gpio-ts4900.o
 obj-$(CONFIG_GPIO_TS5500)	+= gpio-ts5500.o
diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
new file mode 100644
index 0000000..548eea9
--- /dev/null
+++ b/drivers/gpio/gpio-tps68470.c
@@ -0,0 +1,186 @@ 
+/*
+ * GPIO driver for TPS68470 PMIC
+ *
+ * Copyright (C) 2017 Intel Corporation
+ * Authors:
+ * Antti Laakso <antti.laakso@intel.com>
+ * Tianshu Qiu <tian.shu.qiu@intel.com>
+ * Jian Xu Zheng <jian.xu.zheng@intel.com>
+ * Yuning Pu <yuning.pu@intel.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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define TPS68470_N_LOGIC_OUTPUT	3
+#define TPS68470_N_REGULAR_GPIO	7
+#define TPS68470_N_GPIO	(TPS68470_N_LOGIC_OUTPUT + TPS68470_N_REGULAR_GPIO)
+
+struct tps68470_gpio_data {
+	struct regmap *tps68470_regmap;
+	struct gpio_chip gc;
+};
+
+static inline struct tps68470_gpio_data *to_gpio_data(struct gpio_chip *gpiochp)
+{
+	return container_of(gpiochp, struct tps68470_gpio_data, gc);
+}
+
+static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
+	unsigned int reg = TPS68470_REG_GPDO;
+	int val, ret;
+
+	if (offset >= TPS68470_N_REGULAR_GPIO) {
+		offset -= TPS68470_N_REGULAR_GPIO;
+		reg = TPS68470_REG_SGPO;
+	}
+
+	ret = regmap_read(regmap, reg, &val);
+	if (ret) {
+		dev_err(tps68470_gpio->gc.parent, "reg 0x%x read failed\n",
+			TPS68470_REG_SGPO);
+		return ret;
+	}
+	return !!(val & BIT(offset));
+}
+
+/* Return 0 if output, 1 if input */
+static int tps68470_gpio_get_direction(struct gpio_chip *gc,
+				       unsigned int offset)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
+	int val, ret;
+
+	/* rest are always outputs */
+	if (offset >= TPS68470_N_REGULAR_GPIO)
+		return 0;
+
+	ret = regmap_read(regmap, TPS68470_GPIO_CTL_REG_A(offset), &val);
+	if (ret) {
+		dev_err(tps68470_gpio->gc.parent, "reg 0x%x read failed\n",
+			TPS68470_GPIO_CTL_REG_A(offset));
+		return ret;
+	}
+
+	val &= TPS68470_GPIO_MODE_MASK;
+	return val >= TPS68470_GPIO_MODE_OUT_CMOS ? 0 : 1;
+}
+
+static void tps68470_gpio_set(struct gpio_chip *gc, unsigned int offset,
+				int value)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
+	unsigned int reg = TPS68470_REG_GPDO;
+
+	if (offset >= TPS68470_N_REGULAR_GPIO) {
+		reg = TPS68470_REG_SGPO;
+		offset -= TPS68470_N_REGULAR_GPIO;
+	}
+
+	regmap_update_bits(regmap, reg, BIT(offset), value ? BIT(offset) : 0);
+}
+
+static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
+				int value)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
+
+	/* rest are always outputs */
+	if (offset >= TPS68470_N_REGULAR_GPIO)
+		return 0;
+
+	/* Set the initial value */
+	tps68470_gpio_set(gc, offset, value);
+
+	return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
+				 TPS68470_GPIO_MODE_MASK,
+				 TPS68470_GPIO_MODE_OUT_CMOS);
+}
+
+static int tps68470_gpio_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps68470_gpio_data *tps68470_gpio = to_gpio_data(gc);
+	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
+
+	/* rest are always outputs */
+	if (offset >= TPS68470_N_REGULAR_GPIO)
+		return -EINVAL;
+
+	return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
+				   TPS68470_GPIO_MODE_MASK, 0x00);
+}
+
+static const char *tps68470_names[TPS68470_N_GPIO] = {
+	"gpio.0", "gpio.1", "gpio.2", "gpio.3",
+	"gpio.4", "gpio.5", "gpio.6",
+	"s_enable", "s_idle", "s_resetn",
+};
+
+static int tps68470_gpio_probe(struct platform_device *pdev)
+{
+	struct tps68470_gpio_data *tps68470_gpio;
+	int i, ret;
+
+	tps68470_gpio = devm_kzalloc(&pdev->dev, sizeof(*tps68470_gpio),
+				     GFP_KERNEL);
+	if (!tps68470_gpio)
+		return -ENOMEM;
+
+	tps68470_gpio->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
+	tps68470_gpio->gc.label = "tps68470-gpio";
+	tps68470_gpio->gc.owner = THIS_MODULE;
+	tps68470_gpio->gc.direction_input = tps68470_gpio_input;
+	tps68470_gpio->gc.direction_output = tps68470_gpio_output;
+	tps68470_gpio->gc.get = tps68470_gpio_get;
+	tps68470_gpio->gc.get_direction = tps68470_gpio_get_direction;
+	tps68470_gpio->gc.set = tps68470_gpio_set;
+	tps68470_gpio->gc.can_sleep = true;
+	tps68470_gpio->gc.names = tps68470_names;
+	tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
+	tps68470_gpio->gc.base = -1;
+	tps68470_gpio->gc.parent = &pdev->dev;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &tps68470_gpio->gc, NULL);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register gpio_chip: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, tps68470_gpio);
+
+	/*
+	 * Initialize all the GPIOs to 0, just to make sure all
+	 * GPIOs start with known default values. This protects against
+	 * any GPIOs getting set with a value of 1, after TPS68470 reset
+	 */
+	for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
+		tps68470_gpio_set(&tps68470_gpio->gc, i, 0);
+
+	return ret;
+}
+
+static struct platform_driver tps68470_gpio_driver = {
+	.driver = {
+		   .name = "tps68470-gpio",
+	},
+	.probe = tps68470_gpio_probe,
+};
+
+builtin_platform_driver(tps68470_gpio_driver)