This is the patch series for TPS68470 PMIC that works as a camera PMIC.
The patch series provide the following 3 drivers, to help configure
the voltage regulators, clocks and GPIOs provided by the TPS68470
PMIC, to be able to use the camera sensors connected to this PMIC.
TPS68470 MFD driver:
This is the multi function driver that initializes the TPS68470
PMIC and supports the GPIO and Op Region functions.
TPS68470 GPIO driver:
This is the PMIC GPIO driver that will be used by the OS GPIO layer,
when the BIOS / firmware triggered GPIO access is done.
TPS68470 Op Region driver:
This is the driver that will be invoked, when the BIOS / firmware
configures the voltage / clock for the sensors / vcm devices
connected to the PMIC.
---
Changes in v2:
- MFD driver:
- Removed tps68470_* wrappers around regmap_* calls
- Removed "struct tps68470"
- used devm_mfd_add_devices and removed mutex in mfd driver
- Added reasoning about the need of having mfd driver
as bool/builtin
- Opregion driver:
- renamed opregion driver file / internal symbol names
with tps68470_pmic*
- Made opregion driver tables as const
- Removed unused *handler_context in common handler
- Replaced "int" with "unsigned int"
- Changed to WARN macro to dev_warn()
- Destroyed mutex on error
- Added reasoning about the need of having Opregion driver
as bool/builtin
- GPIO driver:
- Implemented get_direction() in the GPIO driver
- Setup gpio_chip.names
- Moved the GPIO lookup table code inside mfd driver
- Added reasoning about the need of having GPIO driver
as bool/builtin
---
Rajmohan Mani (3):
mfd: Add new mfd device TPS68470
gpio: Add support for TPS68470 GPIOs
ACPI / PMIC: Add TI PMIC TPS68470 operation region driver
drivers/acpi/Kconfig | 15 ++
drivers/acpi/Makefile | 2 +
drivers/acpi/pmic/tps68470_pmic.c | 456 ++++++++++++++++++++++++++++++++++++++
drivers/gpio/Kconfig | 14 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-tps68470.c | 186 ++++++++++++++++
drivers/mfd/Kconfig | 18 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/tps68470.c | 192 ++++++++++++++++
include/linux/mfd/tps68470.h | 144 ++++++++++++
10 files changed, 1029 insertions(+)
create mode 100644 drivers/acpi/pmic/tps68470_pmic.c
create mode 100644 drivers/gpio/gpio-tps68470.c
create mode 100644 drivers/mfd/tps68470.c
create mode 100644 include/linux/mfd/tps68470.h
--
1.9.1
The TPS68470 device is an advanced power management
unit that powers a Compact Camera Module (CCM),
generates clocks for image sensors, drives a dual
LED for Flash and incorporates two LED drivers for
general purpose indicators.
This patch adds support for TPS68470 mfd device.
Signed-off-by: Rajmohan Mani <[email protected]>
---
drivers/mfd/Kconfig | 18 ++++
drivers/mfd/Makefile | 1 +
drivers/mfd/tps68470.c | 192 +++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/tps68470.h | 144 ++++++++++++++++++++++++++++++++
4 files changed, 355 insertions(+)
create mode 100644 drivers/mfd/tps68470.c
create mode 100644 include/linux/mfd/tps68470.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3eb5c93..2081248 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1311,6 +1311,24 @@ config MFD_TPS65217
This driver can also be built as a module. If so, the module
will be called tps65217.
+config MFD_TPS68470
+ bool "TI TPS68470 Power Management / LED chips"
+ depends on ACPI && I2C
+ select MFD_CORE
+ select REGMAP_I2C
+ select I2C_DESIGNWARE_PLATFORM
+ help
+ If you say yes here you get support for the TPS68470 series of
+ Power Management / LED chips.
+
+ These include voltage regulators, led and other features
+ that are often used in portable devices.
+
+ This option is a bool as it provides an ACPI operation
+ region, which must be available before any of the devices
+ using this, are probed. This option also configures the
+ designware-i2c driver to be builtin, for the same reason.
+
config MFD_TI_LP873X
tristate "TI LP873X Power Management IC"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c16bf1e..6dd2b94 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
+obj-$(CONFIG_MFD_TPS68470) += tps68470.o
obj-$(CONFIG_MFD_TPS80031) += tps80031.o
obj-$(CONFIG_MENELAUS) += menelaus.o
diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
new file mode 100644
index 0000000..ff4606b
--- /dev/null
+++ b/drivers/mfd/tps68470.c
@@ -0,0 +1,192 @@
+/*
+ * TPS68470 chip family multi-function driver
+ *
+ * Copyright (C) 2017 Intel Corporation
+ * Authors:
+ * Rajmohan Mani <[email protected]>
+ * Tianshu Qiu <[email protected]>
+ * Jian Xu Zheng <[email protected]>
+ * Yuning Pu <[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 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/acpi.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/machine.h>
+#include <linux/init.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell tps68470s[] = {
+ {
+ .name = "tps68470-gpio",
+ },
+ {
+ .name = "tps68470_pmic_opregion",
+ },
+};
+
+/*
+ * This lookup table for the TPS68470 GPIOs, lists
+ * the 7 GPIOs (that can be configured as input or output
+ * as appropriate) and 3 special purpose GPIOs that are
+ * "output only". Exporting these GPIOs in a system mounted
+ * with the TPS68470, in conjunction with the gpio-tps68470
+ * driver, allows the platform firmware to configure these
+ * GPIOs appropriately, through the ACPI operation region.
+ * These 7 configurable GPIOs can be connected to power rails,
+ * sensor control (e.g sensor reset), while the 3 GPIOs can
+ * be used for sensor control.
+ */
+struct gpiod_lookup_table gpios_table = {
+ .dev_id = NULL,
+ .table = {
+ GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", GPIO_ACTIVE_HIGH),
+ {},
+ },
+};
+
+static const struct regmap_config tps68470_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = TPS68470_REG_MAX,
+};
+
+static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
+{
+ unsigned int version;
+ int ret;
+
+ ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
+ if (ret < 0) {
+ dev_err(dev, "Failed to read revision register: %d\n", ret);
+ return ret;
+ }
+
+ dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
+
+ ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
+ if (ret < 0)
+ return ret;
+
+ /* FIXME: configure these dynamically */
+ /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
+ ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 2);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(regmap, TPS68470_REG_GPCTL4A, 2);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(regmap, TPS68470_REG_GPCTL5A, 2);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(regmap, TPS68470_REG_GPCTL6A, 2);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * When SDA and SCL are routed to GPIO1 and GPIO2, the mode
+ * for these GPIOs must be configured using their respective
+ * GPCTLxA registers as inputs with no pull-ups.
+ */
+ ret = regmap_write(regmap, TPS68470_REG_GPCTL1A, 0);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_write(regmap, TPS68470_REG_GPCTL2A, 0);
+ if (ret < 0)
+ return ret;
+
+ /* Enable daisy chain */
+ ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
+ TPS68470_DAISY_CHAIN_DELAY_US + 10);
+ return 0;
+}
+
+static int tps68470_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct regmap *regmap;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
+ PTR_ERR(regmap));
+ return PTR_ERR(regmap);
+ }
+
+ i2c_set_clientdata(client, regmap);
+
+ gpiod_add_lookup_table(&gpios_table);
+
+ ret = devm_mfd_add_devices(dev, -1, tps68470s,
+ ARRAY_SIZE(tps68470s), NULL, 0, NULL);
+ if (ret < 0) {
+ dev_err(dev, "mfd_add_devices failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = tps68470_chip_init(dev, regmap);
+ if (ret < 0) {
+ dev_err(dev, "TPS68470 Init Error %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id tps68470_id_table[] = {
+ {},
+};
+
+MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
+
+static const struct acpi_device_id tps68470_acpi_ids[] = {
+ {"INT3472"},
+ {},
+};
+
+MODULE_DEVICE_TABLE(acpi, tps68470_acpi_ids);
+
+static void tps68470_shutdown(struct i2c_client *client)
+{
+ gpiod_remove_lookup_table(&gpios_table);
+}
+
+static struct i2c_driver tps68470_driver = {
+ .driver = {
+ .name = "tps68470",
+ .acpi_match_table = ACPI_PTR(tps68470_acpi_ids),
+ },
+ .id_table = tps68470_id_table,
+ .probe_new = tps68470_probe,
+ .shutdown = tps68470_shutdown,
+};
+builtin_i2c_driver(tps68470_driver);
diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
new file mode 100644
index 0000000..3fdfc75
--- /dev/null
+++ b/include/linux/mfd/tps68470.h
@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * Functions to access TPS68470 power management chip.
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_MFD_TPS68470_H
+#define __LINUX_MFD_TPS68470_H
+
+#include <linux/i2c.h>
+
+/* All register addresses */
+#define TPS68470_REG_GSTAT 0x01
+#define TPS68470_REG_VRSTA 0x02
+#define TPS68470_REG_VRSHORT 0x03
+#define TPS68470_REG_INTMASK 0x04
+#define TPS68470_REG_VCOSPEED 0x05
+#define TPS68470_REG_POSTDIV2 0x06
+#define TPS68470_REG_BOOSTDIV 0x07
+#define TPS68470_REG_BUCKDIV 0x08
+#define TPS68470_REG_PLLSWR 0x09
+#define TPS68470_REG_XTALDIV 0x0A
+#define TPS68470_REG_PLLDIV 0x0B
+#define TPS68470_REG_POSTDIV 0x0C
+#define TPS68470_REG_PLLCTL 0x0D
+#define TPS68470_REG_PLLCTL2 0x0E
+#define TPS68470_REG_CLKCFG1 0x0F
+#define TPS68470_REG_CLKCFG2 0x10
+#define TPS68470_REG_GPCTL0A 0x14
+#define TPS68470_REG_GPCTL0B 0x15
+#define TPS68470_REG_GPCTL1A 0x16
+#define TPS68470_REG_GPCTL1B 0x17
+#define TPS68470_REG_GPCTL2A 0x18
+#define TPS68470_REG_GPCTL2B 0x19
+#define TPS68470_REG_GPCTL3A 0x1A
+#define TPS68470_REG_GPCTL3B 0x1B
+#define TPS68470_REG_GPCTL4A 0x1C
+#define TPS68470_REG_GPCTL4B 0x1D
+#define TPS68470_REG_GPCTL5A 0x1E
+#define TPS68470_REG_GPCTL5B 0x1F
+#define TPS68470_REG_GPCTL6A 0x20
+#define TPS68470_REG_GPCTL6B 0x21
+#define TPS68470_REG_SGPO 0x22
+#define TPS68470_REG_PITCTL 0x23
+#define TPS68470_REG_WAKECFG 0x24
+#define TPS68470_REG_IOWAKESTAT 0x25
+#define TPS68470_REG_GPDI 0x26
+#define TPS68470_REG_GPDO 0x27
+#define TPS68470_REG_ILEDCTL 0x28
+#define TPS68470_REG_WLEDSTAT 0x29
+#define TPS68470_REG_VWLEDILIM 0x2A
+#define TPS68470_REG_VWLEDVAL 0x2B
+#define TPS68470_REG_WLEDMAXRER 0x2C
+#define TPS68470_REG_WLEDMAXT 0x2D
+#define TPS68470_REG_WLEDMAXAF 0x2E
+#define TPS68470_REG_WLEDMAXF 0x2F
+#define TPS68470_REG_WLEDTO 0x30
+#define TPS68470_REG_VWLEDCTL 0x31
+#define TPS68470_REG_WLEDTIMER_MSB 0x32
+#define TPS68470_REG_WLEDTIMER_LSB 0x33
+#define TPS68470_REG_WLEDC1 0x34
+#define TPS68470_REG_WLEDC2 0x35
+#define TPS68470_REG_WLEDCTL 0x36
+#define TPS68470_REG_VCMVAL 0x3C
+#define TPS68470_REG_VAUX1VAL 0x3D
+#define TPS68470_REG_VAUX2VAL 0x3E
+#define TPS68470_REG_VIOVAL 0x3F
+#define TPS68470_REG_VSIOVAL 0x40
+#define TPS68470_REG_VAVAL 0x41
+#define TPS68470_REG_VDVAL 0x42
+#define TPS68470_REG_S_I2C_CTL 0x43
+#define TPS68470_REG_VCMCTL 0x44
+#define TPS68470_REG_VAUX1CTL 0x45
+#define TPS68470_REG_VAUX2CTL 0x46
+#define TPS68470_REG_VACTL 0x47
+#define TPS68470_REG_VDCTL 0x48
+#define TPS68470_REG_RESET 0x50
+#define TPS68470_REG_REVID 0xFF
+
+#define TPS68470_REG_MAX TPS68470_REG_REVID
+
+/* Register field definitions */
+
+#define TPS68470_VAVAL_AVOLT_MASK GENMASK(6, 0)
+
+#define TPS68470_VDVAL_DVOLT_MASK GENMASK(5, 0)
+#define TPS68470_VCMVAL_VCVOLT_MASK GENMASK(6, 0)
+#define TPS68470_VIOVAL_IOVOLT_MASK GENMASK(6, 0)
+#define TPS68470_VSIOVAL_IOVOLT_MASK GENMASK(6, 0)
+#define TPS68470_VAUX1VAL_AUX1VOLT_MASK GENMASK(6, 0)
+#define TPS68470_VAUX2VAL_AUX2VOLT_MASK GENMASK(6, 0)
+
+#define TPS68470_VACTL_EN_MASK GENMASK(0, 0)
+#define TPS68470_VDCTL_EN_MASK GENMASK(0, 0)
+#define TPS68470_VCMCTL_EN_MASK GENMASK(0, 0)
+#define TPS68470_S_I2C_CTL_EN_MASK GENMASK(1, 0)
+#define TPS68470_VAUX1CTL_EN_MASK GENMASK(0, 0)
+#define TPS68470_VAUX2CTL_EN_MASK GENMASK(0, 0)
+#define TPS68470_PLL_EN_MASK GENMASK(0, 0)
+
+#define TPS68470_OSC_EXT_CAP_SHIFT 4
+#define TPS68470_OSC_EXT_CAP_DEFAULT 0x05 /* 10pf */
+
+#define TPS68470_CLK_SRC_SHIFT 7
+#define TPS68470_CLK_SRC_GPIO3 0
+#define TPS68470_CLK_SRC_XTAL 1
+
+#define TPS68470_DRV_STR_1MA 0
+#define TPS68470_DRV_STR_2MA 1
+#define TPS68470_DRV_STR_4MA 2
+#define TPS68470_DRV_STR_8MA 3
+#define TPS68470_DRV_STR_A_SHIFT 0
+#define TPS68470_DRV_STR_B_SHIFT 2
+
+#define TPS68470_OUTPUT_XTAL_BUFFERED 1
+#define TPS68470_PLL_OUTPUT_ENABLE 2
+#define TPS68470_PLL_OUTPUT_SS_ENABLE 3
+#define TPS68470_OUTPUT_A_SHIFT 0
+#define TPS68470_OUTPUT_B_SHIFT 2
+
+#define TPS68470_CLKCFG1_MODE_A_MASK GENMASK(1, 0)
+#define TPS68470_CLKCFG1_MODE_B_MASK GENMASK(3, 2)
+
+#define TPS68470_GPIO_CTL_REG_A(x) (TPS68470_REG_GPCTL0A + (x) * 2)
+#define TPS68470_GPIO_CTL_REG_B(x) (TPS68470_REG_GPCTL0B + (x) * 2)
+#define TPS68470_GPIO_MODE_MASK GENMASK(1, 0)
+#define TPS68470_GPIO_MODE_IN 0
+#define TPS68470_GPIO_MODE_IN_PULLUP 1
+#define TPS68470_GPIO_MODE_OUT_CMOS 2
+#define TPS68470_GPIO_MODE_OUT_ODRAIN 3
+
+#define TPS68470_PLL_STARTUP_DELAY_US 1000
+#define TPS68470_DAISY_CHAIN_DELAY_US 3000
+
+#endif /* __LINUX_MFD_TPS68470_H */
--
1.9.1
The Kabylake platform coreboot (Chrome OS equivalent of
BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
These operation regions are to enable/disable voltage
regulators, configure voltage regulators, enable/disable
clocks and to configure clocks.
This config adds ACPI operation region support for TI TPS68470 PMIC.
TPS68470 device is an advanced power management unit that powers
a Compact Camera Module (CCM), generates clocks for image sensors,
drives a dual LED for flash and incorporates two LED drivers for
general purpose indicators.
This driver enables ACPI operation region support to control voltage
regulators and clocks for the TPS68470 PMIC.
Signed-off-by: Rajmohan Mani <[email protected]>
---
drivers/acpi/Kconfig | 15 ++
drivers/acpi/Makefile | 2 +
drivers/acpi/pmic/tps68470_pmic.c | 456 ++++++++++++++++++++++++++++++++++++++
3 files changed, 473 insertions(+)
create mode 100644 drivers/acpi/pmic/tps68470_pmic.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1ce52f8..88fc6c9 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -535,4 +535,19 @@ if ARM64
source "drivers/acpi/arm64/Kconfig"
endif
+config TPS68470_PMIC_OPREGION
+ bool "ACPI operation region support for TPS68470 PMIC"
+ help
+ This config adds ACPI operation region support for TI TPS68470 PMIC.
+ TPS68470 device is an advanced power management unit that powers
+ a Compact Camera Module (CCM), generates clocks for image sensors,
+ drives a dual LED for flash and incorporates two LED drivers for
+ general purpose indicators.
+ This driver enables ACPI operation region support control voltage
+ regulators and clocks.
+
+ This option is a bool as it provides an ACPI operation
+ region, which must be available before any of the devices
+ using this, are probed.
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc..2d70a7f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
obj-$(CONFIG_ACPI_CONFIGFS) += acpi_configfs.o
+obj-$(CONFIG_TPS68470_PMIC_OPREGION) += pmic/tps68470_pmic.o
+
video-objs += acpi_video.o video_detect.o
obj-y += dptf/
diff --git a/drivers/acpi/pmic/tps68470_pmic.c b/drivers/acpi/pmic/tps68470_pmic.c
new file mode 100644
index 0000000..1352c13
--- /dev/null
+++ b/drivers/acpi/pmic/tps68470_pmic.c
@@ -0,0 +1,456 @@
+/*
+ * TI TPS68470 PMIC operation region driver
+ *
+ * Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Author: Rajmohan Mani <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Based on drivers/acpi/pmic/intel_pmic* drivers
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct tps68470_pmic_table {
+ u32 address; /* operation region address */
+ u32 reg; /* corresponding register */
+ u32 bitmask; /* bit mask for power, clock */
+};
+
+#define TI_PMIC_POWER_OPREGION_ID 0xB0
+#define TI_PMIC_VR_VAL_OPREGION_ID 0xB1
+#define TI_PMIC_CLOCK_OPREGION_ID 0xB2
+#define TI_PMIC_CLKFREQ_OPREGION_ID 0xB3
+
+struct tps68470_pmic_opregion {
+ struct mutex lock;
+ struct regmap *regmap;
+};
+
+#define S_IO_I2C_EN (BIT(0) | BIT(1))
+
+static const struct tps68470_pmic_table power_table[] = {
+ {
+ .address = 0x00,
+ .reg = TPS68470_REG_S_I2C_CTL,
+ .bitmask = S_IO_I2C_EN,
+ /* S_I2C_CTL */
+ },
+ {
+ .address = 0x04,
+ .reg = TPS68470_REG_VCMCTL,
+ .bitmask = BIT(0),
+ /* VCMCTL */
+ },
+ {
+ .address = 0x08,
+ .reg = TPS68470_REG_VAUX1CTL,
+ .bitmask = BIT(0),
+ /* VAUX1_CTL */
+ },
+ {
+ .address = 0x0C,
+ .reg = TPS68470_REG_VAUX2CTL,
+ .bitmask = BIT(0),
+ /* VAUX2CTL */
+ },
+ {
+ .address = 0x10,
+ .reg = TPS68470_REG_VACTL,
+ .bitmask = BIT(0),
+ /* VACTL */
+ },
+ {
+ .address = 0x14,
+ .reg = TPS68470_REG_VDCTL,
+ .bitmask = BIT(0),
+ /* VDCTL */
+ },
+};
+
+/* Table to set voltage regulator value */
+static const struct tps68470_pmic_table vr_val_table[] = {
+ {
+ .address = 0x00,
+ .reg = TPS68470_REG_VSIOVAL,
+ .bitmask = TPS68470_VSIOVAL_IOVOLT_MASK,
+ /* TPS68470_REG_VSIOVAL */
+ },
+ {
+ .address = 0x04,
+ .reg = TPS68470_REG_VIOVAL,
+ .bitmask = TPS68470_VIOVAL_IOVOLT_MASK,
+ /* TPS68470_REG_VIOVAL */
+ },
+ {
+ .address = 0x08,
+ .reg = TPS68470_REG_VCMVAL,
+ .bitmask = TPS68470_VCMVAL_VCVOLT_MASK,
+ /* TPS68470_REG_VCMVAL */
+ },
+ {
+ .address = 0x0C,
+ .reg = TPS68470_REG_VAUX1VAL,
+ .bitmask = TPS68470_VAUX1VAL_AUX1VOLT_MASK,
+ /* TPS68470_REG_VAUX1VAL */
+ },
+ {
+ .address = 0x10,
+ .reg = TPS68470_REG_VAUX2VAL,
+ .bitmask = TPS68470_VAUX2VAL_AUX2VOLT_MASK,
+ /* TPS68470_REG_VAUX2VAL */
+ },
+ {
+ .address = 0x14,
+ .reg = TPS68470_REG_VAVAL,
+ .bitmask = TPS68470_VAVAL_AVOLT_MASK,
+ /* TPS68470_REG_VAVAL */
+ },
+ {
+ .address = 0x18,
+ .reg = TPS68470_REG_VDVAL,
+ .bitmask = TPS68470_VDVAL_DVOLT_MASK,
+ /* TPS68470_REG_VDVAL */
+ },
+};
+
+/* Table to configure clock frequency */
+static const struct tps68470_pmic_table clk_freq_table[] = {
+ {
+ .address = 0x00,
+ .reg = TPS68470_REG_POSTDIV2,
+ .bitmask = BIT(0) | BIT(1),
+ /* TPS68470_REG_POSTDIV2 */
+ },
+ {
+ .address = 0x04,
+ .reg = TPS68470_REG_BOOSTDIV,
+ .bitmask = 0x1F,
+ /* TPS68470_REG_BOOSTDIV */
+ },
+ {
+ .address = 0x08,
+ .reg = TPS68470_REG_BUCKDIV,
+ .bitmask = 0x0F,
+ /* TPS68470_REG_BUCKDIV */
+ },
+ {
+ .address = 0x0C,
+ .reg = TPS68470_REG_PLLSWR,
+ .bitmask = 0x13,
+ /* TPS68470_REG_PLLSWR */
+ },
+ {
+ .address = 0x10,
+ .reg = TPS68470_REG_XTALDIV,
+ .bitmask = 0xFF,
+ /* TPS68470_REG_XTALDIV */
+ },
+ {
+ .address = 0x14,
+ .reg = TPS68470_REG_PLLDIV,
+ .bitmask = 0xFF,
+ /* TPS68470_REG_PLLDIV */
+ },
+ {
+ .address = 0x18,
+ .reg = TPS68470_REG_POSTDIV,
+ .bitmask = 0x83,
+ /* TPS68470_REG_POSTDIV */
+ },
+};
+
+/* Table to configure and enable clocks */
+static const struct tps68470_pmic_table clk_table[] = {
+ {
+ .address = 0x00,
+ .reg = TPS68470_REG_PLLCTL,
+ .bitmask = 0xF5,
+ /* TPS68470_REG_PLLCTL */
+ },
+ {
+ .address = 0x04,
+ .reg = TPS68470_REG_PLLCTL2,
+ .bitmask = BIT(0),
+ /* TPS68470_REG_PLLCTL2 */
+ },
+ {
+ .address = 0x08,
+ .reg = TPS68470_REG_CLKCFG1,
+ .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
+ TPS68470_CLKCFG1_MODE_B_MASK,
+ /* TPS68470_REG_CLKCFG1 */
+ },
+ {
+ .address = 0x0C,
+ .reg = TPS68470_REG_CLKCFG2,
+ .bitmask = TPS68470_CLKCFG1_MODE_A_MASK |
+ TPS68470_CLKCFG1_MODE_B_MASK,
+ /* TPS68470_REG_CLKCFG2 */
+ },
+};
+
+static int pmic_get_reg_bit(u64 address,
+ const struct tps68470_pmic_table *table,
+ const unsigned int table_size, int *reg,
+ int *bitmask)
+{
+ u64 i;
+
+ i = address / 4;
+ if (i >= table_size)
+ return -ENOENT;
+
+ if (!reg || !bitmask)
+ return -EINVAL;
+
+ *reg = table[i].reg;
+ *bitmask = table[i].bitmask;
+
+ return 0;
+}
+
+static int tps68470_pmic_get_power(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ unsigned int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & bitmask) ? 1 : 0;
+ return 0;
+}
+
+static int tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ unsigned int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = data & bitmask;
+ return 0;
+}
+
+static int tps68470_pmic_get_clk(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ unsigned int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & bitmask) ? 1 : 0;
+ return 0;
+}
+
+static int tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ unsigned int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = data & bitmask;
+ return 0;
+}
+
+static int ti_tps68470_regmap_update_bits(struct regmap *regmap, int reg,
+ int bitmask, u64 value)
+{
+ return regmap_update_bits(regmap, reg, bitmask, value);
+}
+
+static acpi_status tps68470_pmic_common_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *region_context,
+ int (*get)(struct regmap *,
+ int, int, u64 *),
+ int (*update)(struct regmap *,
+ int, int, u64),
+ const struct tps68470_pmic_table *tbl,
+ unsigned int tbl_size)
+{
+ struct tps68470_pmic_opregion *opregion = region_context;
+ struct regmap *regmap = opregion->regmap;
+ int reg, ret, bitmask;
+
+ if (bits != 32)
+ return AE_BAD_PARAMETER;
+
+ ret = pmic_get_reg_bit(address, tbl,
+ tbl_size, ®, &bitmask);
+ if (ret < 0)
+ return AE_BAD_PARAMETER;
+
+ if (function == ACPI_WRITE && *value > bitmask)
+ return AE_BAD_PARAMETER;
+
+ mutex_lock(&opregion->lock);
+
+ ret = (function == ACPI_READ) ?
+ get(regmap, reg, bitmask, value) :
+ update(regmap, reg, bitmask, *value);
+
+ mutex_unlock(&opregion->lock);
+
+ return ret ? AE_ERROR : AE_OK;
+}
+
+static acpi_status tps68470_pmic_cfreq_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context)
+{
+ return tps68470_pmic_common_handler(function, address, bits, value,
+ region_context,
+ tps68470_pmic_get_clk_freq,
+ ti_tps68470_regmap_update_bits,
+ clk_freq_table,
+ ARRAY_SIZE(clk_freq_table));
+}
+
+static acpi_status tps68470_pmic_clk_handler(u32 function,
+ acpi_physical_address address, u32 bits,
+ u64 *value, void *handler_context,
+ void *region_context)
+{
+ return tps68470_pmic_common_handler(function, address, bits, value,
+ region_context,
+ tps68470_pmic_get_clk,
+ ti_tps68470_regmap_update_bits,
+ clk_table,
+ ARRAY_SIZE(clk_table));
+}
+
+static acpi_status tps68470_pmic_vrval_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context)
+{
+ return tps68470_pmic_common_handler(function, address, bits, value,
+ region_context,
+ tps68470_pmic_get_vr_val,
+ ti_tps68470_regmap_update_bits,
+ vr_val_table,
+ ARRAY_SIZE(vr_val_table));
+}
+
+static acpi_status tps68470_pmic_pwr_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context)
+{
+ if (bits != 32)
+ return AE_BAD_PARAMETER;
+
+ /* set/clear for bit 0, bits 0 and 1 together */
+ if (function == ACPI_WRITE &&
+ !(*value == 0 || *value == 1 || *value == 3)) {
+ return AE_BAD_PARAMETER;
+ }
+
+ return tps68470_pmic_common_handler(function, address, bits, value,
+ region_context,
+ tps68470_pmic_get_power,
+ ti_tps68470_regmap_update_bits,
+ power_table,
+ ARRAY_SIZE(power_table));
+}
+
+static int tps68470_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct regmap *tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
+ acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct tps68470_pmic_opregion *opregion;
+ acpi_status status;
+
+ if (!dev || !tps68470_regmap) {
+ dev_warn(dev, "dev or regmap is NULL\n");
+ return -EINVAL;
+ }
+
+ if (!handle) {
+ dev_warn(dev, "acpi handle is NULL\n");
+ return -ENODEV;
+ }
+
+ opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
+ if (!opregion)
+ return -ENOMEM;
+
+ mutex_init(&opregion->lock);
+ opregion->regmap = tps68470_regmap;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_POWER_OPREGION_ID,
+ tps68470_pmic_pwr_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ goto out_mutex_destroy;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_VR_VAL_OPREGION_ID,
+ tps68470_pmic_vrval_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ goto out_remove_power_handler;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_CLOCK_OPREGION_ID,
+ tps68470_pmic_clk_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ goto out_remove_vr_val_handler;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_CLKFREQ_OPREGION_ID,
+ tps68470_pmic_cfreq_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ goto out_remove_clk_handler;
+
+ return 0;
+
+out_remove_clk_handler:
+ acpi_remove_address_space_handler(handle, TI_PMIC_CLOCK_OPREGION_ID,
+ tps68470_pmic_clk_handler);
+out_remove_vr_val_handler:
+ acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
+ tps68470_pmic_vrval_handler);
+out_remove_power_handler:
+ acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
+ tps68470_pmic_pwr_handler);
+out_mutex_destroy:
+ mutex_destroy(&opregion->lock);
+ return -ENODEV;
+}
+
+static struct platform_driver tps68470_pmic_opregion_driver = {
+ .probe = tps68470_pmic_opregion_probe,
+ .driver = {
+ .name = "tps68470_pmic_opregion",
+ },
+};
+
+builtin_platform_driver(tps68470_pmic_opregion_driver)
--
1.9.1
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 <[email protected]>
---
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
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 <[email protected]>
+ * Tianshu Qiu <[email protected]>
+ * Jian Xu Zheng <[email protected]>
+ * Yuning Pu <[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 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)
--
1.9.1
On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani <[email protected]> wrote:
> The TPS68470 device is an advanced power management
> unit that powers a Compact Camera Module (CCM),
> generates clocks for image sensors, drives a dual
> LED for Flash and incorporates two LED drivers for
> general purpose indicators.
>
> This patch adds support for TPS68470 mfd device.
Thanks! This looks much better, though see my few comments below.
> +/*
> + * This lookup table for the TPS68470 GPIOs, lists
> + * the 7 GPIOs (that can be configured as input or output
> + * as appropriate) and 3 special purpose GPIOs that are
> + * "output only". Exporting these GPIOs in a system mounted
> + * with the TPS68470, in conjunction with the gpio-tps68470
> + * driver, allows the platform firmware to configure these
> + * GPIOs appropriately, through the ACPI operation region.
> + * These 7 configurable GPIOs can be connected to power rails,
> + * sensor control (e.g sensor reset), while the 3 GPIOs can
> + * be used for sensor control.
> + */
> +struct gpiod_lookup_table gpios_table = {
> + .dev_id = NULL,
Why dev_id is NULL?
> + .table = {
> + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn", GPIO_ACTIVE_HIGH),
> + {},
> + },
> +};
I don't remember if I asked already why this table exists at all in
the driver. Shouldn't it be provided by ACPI _DSD?
> +static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
> +{
> + unsigned int version;
> + int ret;
> +
> + ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> + if (ret < 0) {
> + dev_err(dev, "Failed to read revision register: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
This will confuse user when probe fails. Should be printed only when
we return 0 for sure.
> + ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
> + if (ret < 0)
> + return ret;
> +
> + /* FIXME: configure these dynamically */
Please, either fix or remove this comment.
> + /* Enable daisy chain */
> + ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> + TPS68470_DAISY_CHAIN_DELAY_US + 10);
This might require a comment, though I'm fine with it as long as it
close to previous excerpt.
> + return 0;
> +}
> +static int tps68470_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> + PTR_ERR(regmap));
1. Indentation.
2. Do we really need this message?
> + return PTR_ERR(regmap);
> + }
> +
> + i2c_set_clientdata(client, regmap);
> +
> + gpiod_add_lookup_table(&gpios_table);
> +
> + ret = devm_mfd_add_devices(dev, -1, tps68470s,
> + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
-1 has a definition for such case, use it instead.
> + if (ret < 0) {
> + dev_err(dev, "mfd_add_devices failed: %d\n", ret);
> + return ret;
> + }
> +static const struct i2c_device_id tps68470_id_table[] = {
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
Either choose ->probe() over ->probe_new() or remove above.
> +static struct i2c_driver tps68470_driver = {
> + .driver = {
> + .name = "tps68470",
> + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids),
ACPI_PTR() is redundant.
> +#include <linux/i2c.h>
And this is for...?
...
> +#define TPS68470_PLL_STARTUP_DELAY_US 1000
> +#define TPS68470_DAISY_CHAIN_DELAY_US 3000
> +
> +#endif /* __LINUX_MFD_TPS68470_H */
--
With Best Regards,
Andy Shevchenko
On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani <[email protected]> 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).
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for the reviews.
> Subject: Re: [PATCH v2 1/3] mfd: Add new mfd device TPS68470
>
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
> <[email protected]> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
>
> Thanks! This looks much better, though see my few comments below.
>
> > +/*
> > + * This lookup table for the TPS68470 GPIOs, lists
> > + * the 7 GPIOs (that can be configured as input or output
> > + * as appropriate) and 3 special purpose GPIOs that are
> > + * "output only". Exporting these GPIOs in a system mounted
> > + * with the TPS68470, in conjunction with the gpio-tps68470
> > + * driver, allows the platform firmware to configure these
> > + * GPIOs appropriately, through the ACPI operation region.
> > + * These 7 configurable GPIOs can be connected to power rails,
> > + * sensor control (e.g sensor reset), while the 3 GPIOs can
> > + * be used for sensor control.
> > + */
>
> > +struct gpiod_lookup_table gpios_table = {
> > + .dev_id = NULL,
>
> Why dev_id is NULL?
>
I have removed the GPIO lookup tables in the driver.
> > + .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > + },
> > +};
>
> I don't remember if I asked already why this table exists at all in the driver.
> Shouldn't it be provided by ACPI _DSD?
>
Ack. I have removed the GPIO lookup tables in the driver.
> > +static int tps68470_chip_init(struct device *dev, struct regmap
> > +*regmap) {
> > + unsigned int version;
> > + int ret;
> > +
> > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to read revision register: %d\n", ret);
> > + return ret;
> > + }
> > +
>
> > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
>
> This will confuse user when probe fails. Should be printed only when we return
> 0 for sure.
>
Ack.
> > + ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
> > + if (ret < 0)
> > + return ret;
> > +
>
> > + /* FIXME: configure these dynamically */
>
> Please, either fix or remove this comment.
>
Will keep this comment, until I see how this can be fixed.
> > + /* Enable daisy chain */
> > + ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
> > + if (ret < 0)
> > + return ret;
> > +
>
> > + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> > + TPS68470_DAISY_CHAIN_DELAY_US + 10);
>
> This might require a comment, though I'm fine with it as long as it close to
> previous excerpt.
>
Ack. Added comment.
> > + return 0;
> > +}
>
> > +static int tps68470_probe(struct i2c_client *client) {
> > + struct device *dev = &client->dev;
> > + struct regmap *regmap;
> > + int ret;
> > +
> > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> > + if (IS_ERR(regmap)) {
>
> > + dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > + PTR_ERR(regmap));
>
> 1. Indentation.
Ack
> 2. Do we really need this message?
>
Since the driver probe fails, it would be useful to know more info on that.
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + i2c_set_clientdata(client, regmap);
> > +
> > + gpiod_add_lookup_table(&gpios_table);
> > +
>
> > + ret = devm_mfd_add_devices(dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
>
> -1 has a definition for such case, use it instead.
>
Ack
> > + if (ret < 0) {
> > + dev_err(dev, "mfd_add_devices failed: %d\n", ret);
> > + return ret;
> > + }
>
> > +static const struct i2c_device_id tps68470_id_table[] = {
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
>
> Either choose ->probe() over ->probe_new() or remove above.
>
Ack. Chose the former.
> > +static struct i2c_driver tps68470_driver = {
> > + .driver = {
> > + .name = "tps68470",
>
> > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids),
>
> ACPI_PTR() is redundant.
>
Ack
> > +#include <linux/i2c.h>
>
> And this is for...?
>
use by tps68470.c which uses i2c_driver and other such i2c defines.
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
> <[email protected]> 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
On Mon, Jun 12, 2017 at 12:12 PM, Mani, Rajmohan
<[email protected]> wrote:
>> > +#include <linux/i2c.h>
>>
>> And this is for...?
> use by tps68470.c which uses i2c_driver and other such i2c defines.
It should be in C-file, not in the header which has nothing to do with I2C APIs.
--
With Best Regards,
Andy Shevchenko
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