Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1658691 > unrolled thread
| Started by | Rajmohan Mani <rajmohan.mani@intel.com> |
|---|---|
| First post | 2017-06-06 14:10 +0200 |
| Last post | 2017-06-07 15:40 +0200 |
| Articles | 8 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v1 0/3] TPS68470 PMIC drivers Rajmohan Mani <rajmohan.mani@intel.com> - 2017-06-06 14:10 +0200
[PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Rajmohan Mani <rajmohan.mani@intel.com> - 2017-06-06 14:10 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-06 16:30 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Hans de Goede <hdegoede@redhat.com> - 2017-06-06 17:30 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 14:20 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-07 15:50 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 14:10 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-07 15:40 +0200
| From | Rajmohan Mani <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-06 14:10 +0200 |
| Subject | [PATCH v1 0/3] TPS68470 PMIC drivers |
| Message-ID | <tPlt8-aF-9@gated-at.bofh.it> |
This is the patch series for TPS68470 PMIC that works as a camera PMIC. The patch series provide the following 3 drivers. 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. --- 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 | 12 + drivers/acpi/Makefile | 2 + drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++ drivers/gpio/Kconfig | 10 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-tps68470.c | 185 ++++++++++++++++ drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 1 + drivers/mfd/tps68470.c | 227 +++++++++++++++++++ include/linux/mfd/tps68470.h | 167 ++++++++++++++ 10 files changed, 1071 insertions(+) create mode 100644 drivers/acpi/pmic/pmic_tps68470.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
[toc] | [next] | [standalone]
| From | Rajmohan Mani <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-06 14:10 +0200 |
| Subject | [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPlt9-aF-31@gated-at.bofh.it> |
| In reply to | #1658691 |
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 <rajmohan.mani@intel.com>
---
drivers/acpi/Kconfig | 12 +
drivers/acpi/Makefile | 2 +
drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
3 files changed, 468 insertions(+)
create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1ce52f8..218d22d 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -535,4 +535,16 @@ 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.
+
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc..7113d05 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/pmic_tps68470.o
+
video-objs += acpi_video.o video_detect.o
obj-y += dptf/
diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
new file mode 100644
index 0000000..b2d608b
--- /dev/null
+++ b/drivers/acpi/pmic/pmic_tps68470.c
@@ -0,0 +1,454 @@
+/*
+ * TI TPS68470 PMIC operation region driver
+ *
+ * Copyright (C) 2017 Intel Corporation. All rights reserved.
+ * Author: Rajmohan Mani <rajmohan.mani@intel.com>
+ *
+ * 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 ti_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 ti_pmic_opregion {
+ struct mutex lock;
+ struct regmap *regmap;
+};
+
+#define S_IO_I2C_EN (BIT(0) | BIT(1))
+
+static const struct ti_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 ti_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 ti_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 ti_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, struct ti_pmic_table *table,
+ int count, int *reg, int *bitmask)
+{
+ u64 i;
+
+ i = address / 4;
+
+ if (i >= count)
+ return -ENOENT;
+
+ if (!reg || !bitmask)
+ return -EINVAL;
+
+ *reg = table[i].reg;
+ *bitmask = table[i].bitmask;
+
+ return 0;
+}
+
+static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & bitmask) ? 1 : 0;
+ return 0;
+}
+
+static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = data & bitmask;
+ return 0;
+}
+
+static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & bitmask) ? 1 : 0;
+ return 0;
+}
+
+static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
+ int bitmask, u64 *value)
+{
+ 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 ti_pmic_common_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context,
+ int (*get)(struct regmap *,
+ int, int, u64 *),
+ int (*update)(struct regmap *,
+ int, int, u64),
+ struct ti_pmic_table *table,
+ int table_size)
+{
+ struct ti_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, table,
+ table_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 ti_pmic_clk_freq_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context)
+{
+ return ti_pmic_common_handler(function, address, bits, value,
+ handler_context, region_context,
+ ti_tps68470_pmic_get_clk_freq,
+ ti_tps68470_regmap_update_bits,
+ (struct ti_pmic_table *) &clk_freq_table,
+ ARRAY_SIZE(clk_freq_table));
+}
+
+static acpi_status ti_pmic_clk_handler(u32 function,
+ acpi_physical_address address, u32 bits,
+ u64 *value, void *handler_context,
+ void *region_context)
+{
+ return ti_pmic_common_handler(function, address, bits, value,
+ handler_context, region_context,
+ ti_tps68470_pmic_get_clk,
+ ti_tps68470_regmap_update_bits,
+ (struct ti_pmic_table *) &clk_table,
+ ARRAY_SIZE(clk_table));
+}
+
+static acpi_status ti_pmic_vr_val_handler(u32 function,
+ acpi_physical_address address,
+ u32 bits, u64 *value,
+ void *handler_context,
+ void *region_context)
+{
+ return ti_pmic_common_handler(function, address, bits, value,
+ handler_context, region_context,
+ ti_tps68470_pmic_get_vr_val,
+ ti_tps68470_regmap_update_bits,
+ (struct ti_pmic_table *) &vr_val_table,
+ ARRAY_SIZE(vr_val_table));
+}
+
+static acpi_status ti_pmic_power_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 ti_pmic_common_handler(function, address, bits, value,
+ handler_context, region_context,
+ ti_tps68470_pmic_get_power,
+ ti_tps68470_regmap_update_bits,
+ (struct ti_pmic_table *) &power_table,
+ ARRAY_SIZE(power_table));
+}
+
+static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
+ acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct ti_pmic_opregion *opregion;
+ acpi_status status;
+
+ if (!dev || !pmic->regmap) {
+ WARN(1, "dev or regmap is NULL\n");
+ return -EINVAL;
+ }
+
+ if (!handle) {
+ WARN(1, "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 = pmic->regmap;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_POWER_OPREGION_ID,
+ ti_pmic_power_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_VR_VAL_OPREGION_ID,
+ ti_pmic_vr_val_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ goto out_remove_power_handler;
+
+ status = acpi_install_address_space_handler(handle,
+ TI_PMIC_CLOCK_OPREGION_ID,
+ ti_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,
+ ti_pmic_clk_freq_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,
+ ti_pmic_clk_handler);
+out_remove_vr_val_handler:
+ acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
+ ti_pmic_vr_val_handler);
+out_remove_power_handler:
+ acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
+ ti_pmic_power_handler);
+ return -ENODEV;
+}
+
+static struct platform_driver ti_tps68470_pmic_opregion_driver = {
+ .probe = ti_tps68470_pmic_opregion_probe,
+ .driver = {
+ .name = "tps68470_pmic_opregion",
+ },
+};
+
+builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-06 16:30 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPnEB-1x7-3@gated-at.bofh.it> |
| In reply to | #1658694 |
+Cc Hans (that's why didn't delete anything from original mail, just
adding my comments).
Hans, if you have few minutes it would be appreciated to glance on the
below for some issues if any since you did pass quite a good quest
with other PMIC drivers.
On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
> 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 <rajmohan.mani@intel.com>
> ---
> drivers/acpi/Kconfig | 12 +
> drivers/acpi/Makefile | 2 +
> drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
Follow the pattern, please, I suppose
ti_pmic_tps68470.c
> 3 files changed, 468 insertions(+)
> create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 1ce52f8..218d22d 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -535,4 +535,16 @@ 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.
> +
> +
Extra line, remove.
> endif # ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..7113d05 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/pmic_tps68470.o
> +
> video-objs += acpi_video.o video_detect.o
> obj-y += dptf/
>
> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
> new file mode 100644
> index 0000000..b2d608b
> --- /dev/null
> +++ b/drivers/acpi/pmic/pmic_tps68470.c
> @@ -0,0 +1,454 @@
> +/*
> + * TI TPS68470 PMIC operation region driver
> + *
> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> + *
> + * 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 ti_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 ti_pmic_opregion {
> + struct mutex lock;
> + struct regmap *regmap;
> +};
> +
> +#define S_IO_I2C_EN (BIT(0) | BIT(1))
> +
> +static const struct ti_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 ti_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 ti_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 ti_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, struct ti_pmic_table *table,
> + int count, int *reg, int *bitmask)
> +{
> + u64 i;
> +
> + i = address / 4;
> +
Remove this line.
> + if (i >= count)
> + return -ENOENT;
> +
> + if (!reg || !bitmask)
> + return -EINVAL;
> +
> + *reg = table[i].reg;
> + *bitmask = table[i].bitmask;
> +
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = (data & bitmask) ? 1 : 0;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = data & bitmask;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = (data & bitmask) ? 1 : 0;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + 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 ti_pmic_common_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> + void *region_context,
> + int (*get)(struct regmap *,
> + int, int, u64 *),
> + int (*update)(struct regmap *,
> + int, int, u64),
> + struct ti_pmic_table *table,
> + int table_size)
> +{
> + struct ti_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, table,
> + table_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 ti_pmic_clk_freq_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_clk_freq,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &clk_freq_table,
> + ARRAY_SIZE(clk_freq_table));
> +}
> +
> +static acpi_status ti_pmic_clk_handler(u32 function,
> + acpi_physical_address address, u32 bits,
> + u64 *value, void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_clk,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &clk_table,
> + ARRAY_SIZE(clk_table));
> +}
> +
> +static acpi_status ti_pmic_vr_val_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_vr_val,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &vr_val_table,
> + ARRAY_SIZE(vr_val_table));
> +}
> +
> +static acpi_status ti_pmic_power_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 ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_power,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &power_table,
> + ARRAY_SIZE(power_table));
> +}
> +
> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
> +{
> + struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> + acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct ti_pmic_opregion *opregion;
> + acpi_status status;
> +
> + if (!dev || !pmic->regmap) {
> + WARN(1, "dev or regmap is NULL\n");
> + return -EINVAL;
> + }
> +
> + if (!handle) {
> + WARN(1, "acpi handle is NULL\n");
> + return -ENODEV;
> + }
I dunno if WARNs make user experience any better.
Besides that I would double check you may have such cases.
> +
> + opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
> + if (!opregion)
> + return -ENOMEM;
> +
> + mutex_init(&opregion->lock);
> + opregion->regmap = pmic->regmap;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_POWER_OPREGION_ID,
> + ti_pmic_power_handler,
> + NULL, opregion);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_VR_VAL_OPREGION_ID,
> + ti_pmic_vr_val_handler,
> + NULL, opregion);
> + if (ACPI_FAILURE(status))
> + goto out_remove_power_handler;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_CLOCK_OPREGION_ID,
> + ti_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,
> + ti_pmic_clk_freq_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,
> + ti_pmic_clk_handler);
> +out_remove_vr_val_handler:
> + acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
> + ti_pmic_vr_val_handler);
> +out_remove_power_handler:
> + acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
> + ti_pmic_power_handler);
> + return -ENODEV;
> +}
> +
> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> + .probe = ti_tps68470_pmic_opregion_probe,
> + .driver = {
> + .name = "tps68470_pmic_opregion",
> + },
> +};
> +
> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
> --
> 1.9.1
>
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2017-06-06 17:30 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPoAG-27S-27@gated-at.bofh.it> |
| In reply to | #1658803 |
Hi,
On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
>
> Hans, if you have few minutes it would be appreciated to glance on the
> below for some issues if any since you did pass quite a good quest
> with other PMIC drivers.
I've gone over this driver, nothing stands out in a bad way to me,
IOW this seems like a normal PMIC OpRegion handler to me.
Regards,
Hans
>
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com> wrote:
>> 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 <rajmohan.mani@intel.com>
>> ---
>> drivers/acpi/Kconfig | 12 +
>> drivers/acpi/Makefile | 2 +
>
>> drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
>
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
>
>> 3 files changed, 468 insertions(+)
>> create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 1ce52f8..218d22d 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -535,4 +535,16 @@ 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.
>> +
>
>> +
>
> Extra line, remove.
>
>> endif # ACPI
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index b1aacfc..7113d05 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/pmic_tps68470.o
>> +
>> video-objs += acpi_video.o video_detect.o
>> obj-y += dptf/
>>
>> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
>> new file mode 100644
>> index 0000000..b2d608b
>> --- /dev/null
>> +++ b/drivers/acpi/pmic/pmic_tps68470.c
>> @@ -0,0 +1,454 @@
>> +/*
>> + * TI TPS68470 PMIC operation region driver
>> + *
>> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
>> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
>> + *
>> + * 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 ti_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 ti_pmic_opregion {
>> + struct mutex lock;
>> + struct regmap *regmap;
>> +};
>> +
>> +#define S_IO_I2C_EN (BIT(0) | BIT(1))
>> +
>> +static const struct ti_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 ti_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 ti_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 ti_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, struct ti_pmic_table *table,
>> + int count, int *reg, int *bitmask)
>> +{
>> + u64 i;
>> +
>
>> + i = address / 4;
>
>
>> +
>
> Remove this line.
>
>> + if (i >= count)
>> + return -ENOENT;
>> +
>> + if (!reg || !bitmask)
>> + return -EINVAL;
>> +
>> + *reg = table[i].reg;
>> + *bitmask = table[i].bitmask;
>> +
>> + return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
>> + int bitmask, u64 *value)
>> +{
>> + int data;
>> +
>> + if (regmap_read(regmap, reg, &data))
>> + return -EIO;
>> +
>> + *value = (data & bitmask) ? 1 : 0;
>> + return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
>> + int bitmask, u64 *value)
>> +{
>> + int data;
>> +
>> + if (regmap_read(regmap, reg, &data))
>> + return -EIO;
>> +
>> + *value = data & bitmask;
>> + return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
>> + int bitmask, u64 *value)
>> +{
>> + int data;
>> +
>> + if (regmap_read(regmap, reg, &data))
>> + return -EIO;
>> +
>> + *value = (data & bitmask) ? 1 : 0;
>> + return 0;
>> +}
>> +
>> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
>> + int bitmask, u64 *value)
>> +{
>> + 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 ti_pmic_common_handler(u32 function,
>> + acpi_physical_address address,
>> + u32 bits, u64 *value,
>> + void *handler_context,
>> + void *region_context,
>> + int (*get)(struct regmap *,
>> + int, int, u64 *),
>> + int (*update)(struct regmap *,
>> + int, int, u64),
>> + struct ti_pmic_table *table,
>> + int table_size)
>> +{
>> + struct ti_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, table,
>> + table_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 ti_pmic_clk_freq_handler(u32 function,
>> + acpi_physical_address address,
>> + u32 bits, u64 *value,
>> + void *handler_context,
>> + void *region_context)
>> +{
>> + return ti_pmic_common_handler(function, address, bits, value,
>> + handler_context, region_context,
>> + ti_tps68470_pmic_get_clk_freq,
>> + ti_tps68470_regmap_update_bits,
>> + (struct ti_pmic_table *) &clk_freq_table,
>> + ARRAY_SIZE(clk_freq_table));
>> +}
>> +
>> +static acpi_status ti_pmic_clk_handler(u32 function,
>> + acpi_physical_address address, u32 bits,
>> + u64 *value, void *handler_context,
>> + void *region_context)
>> +{
>> + return ti_pmic_common_handler(function, address, bits, value,
>> + handler_context, region_context,
>> + ti_tps68470_pmic_get_clk,
>> + ti_tps68470_regmap_update_bits,
>> + (struct ti_pmic_table *) &clk_table,
>> + ARRAY_SIZE(clk_table));
>> +}
>> +
>> +static acpi_status ti_pmic_vr_val_handler(u32 function,
>> + acpi_physical_address address,
>> + u32 bits, u64 *value,
>> + void *handler_context,
>> + void *region_context)
>> +{
>> + return ti_pmic_common_handler(function, address, bits, value,
>> + handler_context, region_context,
>> + ti_tps68470_pmic_get_vr_val,
>> + ti_tps68470_regmap_update_bits,
>> + (struct ti_pmic_table *) &vr_val_table,
>> + ARRAY_SIZE(vr_val_table));
>> +}
>> +
>> +static acpi_status ti_pmic_power_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 ti_pmic_common_handler(function, address, bits, value,
>> + handler_context, region_context,
>> + ti_tps68470_pmic_get_power,
>> + ti_tps68470_regmap_update_bits,
>> + (struct ti_pmic_table *) &power_table,
>> + ARRAY_SIZE(power_table));
>> +}
>> +
>> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
>> +{
>> + struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
>> + acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
>> + struct device *dev = &pdev->dev;
>> + struct ti_pmic_opregion *opregion;
>> + acpi_status status;
>> +
>
>> + if (!dev || !pmic->regmap) {
>> + WARN(1, "dev or regmap is NULL\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!handle) {
>> + WARN(1, "acpi handle is NULL\n");
>> + return -ENODEV;
>> + }
>
> I dunno if WARNs make user experience any better.
> Besides that I would double check you may have such cases.
>
>> +
>> + opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
>> + if (!opregion)
>> + return -ENOMEM;
>> +
>> + mutex_init(&opregion->lock);
>> + opregion->regmap = pmic->regmap;
>> +
>> + status = acpi_install_address_space_handler(handle,
>> + TI_PMIC_POWER_OPREGION_ID,
>> + ti_pmic_power_handler,
>> + NULL, opregion);
>> + if (ACPI_FAILURE(status))
>> + return -ENODEV;
>> +
>> + status = acpi_install_address_space_handler(handle,
>> + TI_PMIC_VR_VAL_OPREGION_ID,
>> + ti_pmic_vr_val_handler,
>> + NULL, opregion);
>> + if (ACPI_FAILURE(status))
>> + goto out_remove_power_handler;
>> +
>> + status = acpi_install_address_space_handler(handle,
>> + TI_PMIC_CLOCK_OPREGION_ID,
>> + ti_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,
>> + ti_pmic_clk_freq_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,
>> + ti_pmic_clk_handler);
>> +out_remove_vr_val_handler:
>> + acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
>> + ti_pmic_vr_val_handler);
>> +out_remove_power_handler:
>> + acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
>> + ti_pmic_power_handler);
>> + return -ENODEV;
>> +}
>> +
>> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
>> + .probe = ti_tps68470_pmic_opregion_probe,
>> + .driver = {
>> + .name = "tps68470_pmic_opregion",
>> + },
>> +};
>> +
>> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
>> --
>> 1.9.1
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 14:20 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPI6m-6wb-13@gated-at.bofh.it> |
| In reply to | #1658803 |
On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote: > Follow the pattern, please, I suppose > ti_pmic_tps68470.c This pattern is weird. "ti" in front of the file name is redundant, and in very few places the vendor prefix is used anyway. Especially when the chip has a proper name --- as this one does. I assume for the Intel PMICs it could be there for a couple of reasons which are 1) lack of a clearly unique chip ID and 2) the use of common frameworklet for Intel PMICs. There are also no other PMIC chips supported currently. The pmic_tps68470 naming is in line with the GPIO driver (apart from the dash / underscore difference). -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-07 15:50 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPJvs-7jj-27@gated-at.bofh.it> |
| In reply to | #1659737 |
On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote: >> Follow the pattern, please, I suppose >> ti_pmic_tps68470.c > > This pattern is weird. "ti" in front of the file name is redundant, and in > very few places the vendor prefix is used anyway. Especially when the chip > has a proper name --- as this one does. > > I assume for the Intel PMICs it could be there for a couple of reasons which > are > > 1) lack of a clearly unique chip ID and > > 2) the use of common frameworklet for Intel PMICs. > > There are also no other PMIC chips supported currently. > > The pmic_tps68470 naming is in line with the GPIO driver (apart from the > dash / underscore difference). Since % git ls-files *pmic* returns somewhat interesting results, I would even go further and use tps68470.c here and s/ti_pmic/tps6840/g inside the file. Would it work for you? -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 14:10 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPHWF-6rv-9@gated-at.bofh.it> |
| In reply to | #1658694 |
Hi Rajmohan,
Thanks for removing the redundant struct definition. A couple more comments
below. Not really necessarily bugs but a few things to clean things up a
bit.
On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote:
> 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 <rajmohan.mani@intel.com>
> ---
> drivers/acpi/Kconfig | 12 +
> drivers/acpi/Makefile | 2 +
> drivers/acpi/pmic/pmic_tps68470.c | 454 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 468 insertions(+)
> create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 1ce52f8..218d22d 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -535,4 +535,16 @@ 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.
> +
Extra newline.
> +
> endif # ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..7113d05 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/pmic_tps68470.o
> +
> video-objs += acpi_video.o video_detect.o
> obj-y += dptf/
>
> diff --git a/drivers/acpi/pmic/pmic_tps68470.c b/drivers/acpi/pmic/pmic_tps68470.c
> new file mode 100644
> index 0000000..b2d608b
> --- /dev/null
> +++ b/drivers/acpi/pmic/pmic_tps68470.c
> @@ -0,0 +1,454 @@
> +/*
> + * TI TPS68470 PMIC operation region driver
> + *
> + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> + * Author: Rajmohan Mani <rajmohan.mani@intel.com>
> + *
> + * 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 ti_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 ti_pmic_opregion {
> + struct mutex lock;
> + struct regmap *regmap;
> +};
> +
> +#define S_IO_I2C_EN (BIT(0) | BIT(1))
> +
> +static const struct ti_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 ti_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 ti_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 ti_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, struct ti_pmic_table *table,
> + int count, int *reg, int *bitmask)
unsigned int count?
> +{
> + u64 i;
> +
> + i = address / 4;
> +
> + if (i >= count)
> + return -ENOENT;
> +
> + if (!reg || !bitmask)
> + return -EINVAL;
> +
> + *reg = table[i].reg;
> + *bitmask = table[i].bitmask;
> +
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
Shouldn't you use unsigned int here? Same in the functions below.
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = (data & bitmask) ? 1 : 0;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_vr_val(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = data & bitmask;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + int data;
> +
> + if (regmap_read(regmap, reg, &data))
> + return -EIO;
> +
> + *value = (data & bitmask) ? 1 : 0;
> + return 0;
> +}
> +
> +static int ti_tps68470_pmic_get_clk_freq(struct regmap *regmap, int reg,
> + int bitmask, u64 *value)
> +{
> + 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 ti_pmic_common_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
handler_context is unused.
> + void *region_context,
> + int (*get)(struct regmap *,
> + int, int, u64 *),
> + int (*update)(struct regmap *,
> + int, int, u64),
> + struct ti_pmic_table *table,
> + int table_size)
unsigned int. (Or size_t or whatever.)
> +{
> + struct ti_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, table,
> + table_size, ®, &bitmask);
> + if (ret < 0)
> + return AE_BAD_PARAMETER;
> +
> + if (function == ACPI_WRITE && (*value > bitmask))
Extra parentheses.
> + 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 ti_pmic_clk_freq_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_clk_freq,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &clk_freq_table,
You shouldn't use an explicit cast here. Instead make the function argument
const as well and you're fine.
> + ARRAY_SIZE(clk_freq_table));
> +}
> +
> +static acpi_status ti_pmic_clk_handler(u32 function,
> + acpi_physical_address address, u32 bits,
> + u64 *value, void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_clk,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &clk_table,
> + ARRAY_SIZE(clk_table));
> +}
> +
> +static acpi_status ti_pmic_vr_val_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> + void *region_context)
> +{
> + return ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_vr_val,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &vr_val_table,
> + ARRAY_SIZE(vr_val_table));
> +}
> +
> +static acpi_status ti_pmic_power_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 ti_pmic_common_handler(function, address, bits, value,
> + handler_context, region_context,
> + ti_tps68470_pmic_get_power,
> + ti_tps68470_regmap_update_bits,
> + (struct ti_pmic_table *) &power_table,
> + ARRAY_SIZE(power_table));
> +}
> +
> +static int ti_tps68470_pmic_opregion_probe(struct platform_device *pdev)
> +{
> + struct tps68470 *pmic = dev_get_drvdata(pdev->dev.parent);
> + acpi_handle handle = ACPI_HANDLE(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct ti_pmic_opregion *opregion;
> + acpi_status status;
> +
> + if (!dev || !pmic->regmap) {
> + WARN(1, "dev or regmap is NULL\n");
> + return -EINVAL;
> + }
> +
> + if (!handle) {
> + WARN(1, "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 = pmic->regmap;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_POWER_OPREGION_ID,
> + ti_pmic_power_handler,
> + NULL, opregion);
> + if (ACPI_FAILURE(status))
mutex_destroy() after mutex_init() --- please add a label for this.
> + return -ENODEV;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_VR_VAL_OPREGION_ID,
> + ti_pmic_vr_val_handler,
> + NULL, opregion);
> + if (ACPI_FAILURE(status))
> + goto out_remove_power_handler;
> +
> + status = acpi_install_address_space_handler(handle,
> + TI_PMIC_CLOCK_OPREGION_ID,
> + ti_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,
> + ti_pmic_clk_freq_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,
> + ti_pmic_clk_handler);
> +out_remove_vr_val_handler:
> + acpi_remove_address_space_handler(handle, TI_PMIC_VR_VAL_OPREGION_ID,
> + ti_pmic_vr_val_handler);
> +out_remove_power_handler:
> + acpi_remove_address_space_handler(handle, TI_PMIC_POWER_OPREGION_ID,
> + ti_pmic_power_handler);
> + return -ENODEV;
> +}
> +
> +static struct platform_driver ti_tps68470_pmic_opregion_driver = {
> + .probe = ti_tps68470_pmic_opregion_probe,
> + .driver = {
> + .name = "tps68470_pmic_opregion",
> + },
> +};
> +
> +builtin_platform_driver(ti_tps68470_pmic_opregion_driver)
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-07 15:40 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPJlM-7eC-25@gated-at.bofh.it> |
| In reply to | #1659730 |
On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
>> + int bitmask, u64 *value)
>> +{
>> + int data;
>
> Shouldn't you use unsigned int here? Same in the functions below.
+1, regmap_read() returns unsigned int.
>> +static acpi_status ti_pmic_common_handler(u32 function,
> + acpi_physical_address address,
> + u32 bits, u64 *value,
> + void *handler_context,
> handler_context is unused.
>> + int, int, u64 *),
>> + int (*update)(struct regmap *,
>> + int, int, u64),
>> + struct ti_pmic_table *table,
>> + int table_size)
I would even split this to have separate update() and get() paths
instead of having such a monster of parameters.
>> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
>> + acpi_physical_address address,
>> + u32 bits, u64 *value,
>> + void *handler_context,
>> + void *region_context)
>> +{
>> + return ti_pmic_common_handler(function, address, bits, value,
>> + handler_context, region_context,
>> + ti_tps68470_pmic_get_clk_freq,
>> + ti_tps68470_regmap_update_bits,
>> + (struct ti_pmic_table *) &clk_freq_table,
>
> You shouldn't use an explicit cast here. Instead make the function argument
> const as well and you're fine.
+1.
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web