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-10 02:10 +0200 |
| Articles | 20 on this page of 21 — 5 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 "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 00: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 22: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 22: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 23:20 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 01:40 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 02:20 +0200
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Hans de Goede <hdegoede@redhat.com> - 2017-06-08 09:10 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 01:50 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 00: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: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
Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-07 22:10 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 02:10 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 02:10 +0200
RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-10 02:10 +0200
Page 1 of 2 [1] 2 Next page →
| 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 | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 00:30 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQAzM-7eY-5@gated-at.bofh.it> |
| In reply to | #1658867 |
Hi Hans, > -----Original Message----- > From: Hans de Goede [mailto:hdegoede@redhat.com] > Sent: Tuesday, June 06, 2017 8:22 AM > To: Andy Shevchenko <andy.shevchenko@gmail.com>; Mani, Rajmohan > <rajmohan.mani@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij > <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J. > Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation > region driver > > 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. > Thanks for the reviews and time.
[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 22:20 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPPAS-2SK-3@gated-at.bofh.it> |
| In reply to | #1659819 |
Hi Andy, On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: > 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? This is still a different driver from the tps68470 driver which is an MFD driver. For clarity, I'd keep pmic as part of the name (and I'd use tps68470_pmic_ prefix for internal symbols, too). As PMICs are typically linked to the kernel (vs. being modules), there's no issue with the module name. I would suppose few if any PMICs will be compiled as modules in general. It's not a big deal though. I'm fine either way. -- 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 22:50 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPQ3U-34T-15@gated-at.bofh.it> |
| In reply to | #1660179 |
On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: >> 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 >> >> Would it work for you? > > This is still a different driver from the tps68470 driver which is an MFD > driver. For clarity, I'd keep pmic as part of the name (and I'd use > tps68470_pmic_ prefix for internal symbols, too). > > As PMICs are typically linked to the kernel (vs. being modules), there's no > issue with the module name. I would suppose few if any PMICs will be > compiled as modules in general. > > It's not a big deal though. I'm fine either way. Okay, let's agree on tps68470_pmic for internal prefix and for file name? -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 23:20 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPQwV-3ya-9@gated-at.bofh.it> |
| In reply to | #1660196 |
On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote: > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: > >> 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 > >> > >> Would it work for you? > > > > This is still a different driver from the tps68470 driver which is an MFD > > driver. For clarity, I'd keep pmic as part of the name (and I'd use > > tps68470_pmic_ prefix for internal symbols, too). > > > > As PMICs are typically linked to the kernel (vs. being modules), there's no > > issue with the module name. I would suppose few if any PMICs will be > > compiled as modules in general. > > > > It's not a big deal though. I'm fine either way. > > Okay, let's agree on tps68470_pmic for internal prefix and for file name? Ack. Thanks! -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 01:40 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQBFv-7Sa-1@gated-at.bofh.it> |
| In reply to | #1660234 |
Hi Sakari, Andy, > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@iki.fi] > Sent: Wednesday, June 07, 2017 2:13 PM > To: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Mani, Rajmohan <rajmohan.mani@intel.com>; Hans de Goede > <hdegoede@redhat.com>; linux-kernel@vger.kernel.org; linux- > gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Lee Jones > <lee.jones@linaro.org>; Linus Walleij <linus.walleij@linaro.org>; Alexandre > Courbot <gnurou@gmail.com>; Rafael J. Wysocki <rjw@rjwysocki.net>; Len > Brown <lenb@kernel.org> > Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation > region driver > > On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote: > > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: > > >> 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 > > >> > > >> Would it work for you? > > > > > > This is still a different driver from the tps68470 driver which is > > > an MFD driver. For clarity, I'd keep pmic as part of the name (and > > > I'd use tps68470_pmic_ prefix for internal symbols, too). > > > > > > As PMICs are typically linked to the kernel (vs. being modules), > > > there's no issue with the module name. I would suppose few if any > > > PMICs will be compiled as modules in general. > > > > > > It's not a big deal though. I'm fine either way. > > > > Okay, let's agree on tps68470_pmic for internal prefix and for file name? > > Ack. Thanks! > Ack
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 02:20 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQCid-8lG-11@gated-at.bofh.it> |
| In reply to | #1660234 |
Hi Sakari, Andy, > Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation > region driver > > On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote: > > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: > > >> 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 > > >> > > >> Would it work for you? > > > > > > This is still a different driver from the tps68470 driver which is > > > an MFD driver. For clarity, I'd keep pmic as part of the name (and > > > I'd use tps68470_pmic_ prefix for internal symbols, too). > > > > > > As PMICs are typically linked to the kernel (vs. being modules), > > > there's no issue with the module name. I would suppose few if any > > > PMICs will be compiled as modules in general. > > > > > > It's not a big deal though. I'm fine either way. > > > > Okay, let's agree on tps68470_pmic for internal prefix and for file name? > > Ack. Thanks! > Ack
[toc] | [prev] | [next] | [standalone]
| From | Hans de Goede <hdegoede@redhat.com> |
|---|---|
| Date | 2017-06-08 09:10 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPZJU-16j-27@gated-at.bofh.it> |
| In reply to | #1660179 |
Hi, On 07-06-17 22:10, Sakari Ailus wrote: > Hi Andy, > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote: >> 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? > > This is still a different driver from the tps68470 driver which is an MFD > driver. For clarity, I'd keep pmic as part of the name (and I'd use > tps68470_pmic_ prefix for internal symbols, too). > > As PMICs are typically linked to the kernel (vs. being modules), there's no > issue with the module name. I would suppose few if any PMICs will be > compiled as modules in general. Good point about the OpRegion driver usually being built-in, in my experience it MUST always be built-in, so the Kconfig option should be a bool. Note this is useless unless the mfd driver is also a bool (I would advice to go that route) and the mfd driver's Kconfig should select the right i2c bus driver to make sure that is built-in too, see for example: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0 https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=c5065d8625ebdc164199b99d838ac0636faa7f0b https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=5f125f1f570568a29edf783fba1ebb606d5c6b24 Which are all recent commits from me dealing with making the mfd driver built-in / selecting the i2c bus driver. Regards, Hans
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 01:50 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQBPb-7Vf-1@gated-at.bofh.it> |
| In reply to | #1660811 |
Hi Hans, > > > > As PMICs are typically linked to the kernel (vs. being modules), > > there's no issue with the module name. I would suppose few if any > > PMICs will be compiled as modules in general. > > Good point about the OpRegion driver usually being built-in, in my experience it > MUST always be built-in, so the Kconfig option should be a bool. Note this is > useless unless the mfd driver is also a bool (I would advice to go that > route) and the mfd driver's Kconfig should select the right i2c bus driver to > make sure that is built-in too, see for example: > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for- > mfd-next&id=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0 > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for- > mfd-next&id=c5065d8625ebdc164199b99d838ac0636faa7f0b > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for- > mfd-next&id=5f125f1f570568a29edf783fba1ebb606d5c6b24 > > Which are all recent commits from me dealing with making the mfd driver built- > in / selecting the i2c bus driver. > Thanks for these links. I will update the Kconfig and commit messages with relevant description around this.
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 00:30 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQAzL-7eY-1@gated-at.bofh.it> |
| In reply to | #1658803 |
Hi Andy,
Thanks for the reviews and patience.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan <rajmohan.mani@intel.com>; Hans de Goede
> <hdegoede@redhat.com>
> Cc: linux-kernel@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org; Lee Jones <lee.jones@linaro.org>; Linus Walleij
> <linus.walleij@linaro.org>; Alexandre Courbot <gnurou@gmail.com>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
>
> +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
>
I will reply to this, on the later threads on the same subject
> > 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.
>
Ack
> > 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.
>
Ack
> > + 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.
>
Ack
Since the caller is from within the same file and we know the parameters will be set properly, I can change this to dev_warn()
> > +
> > + 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 | 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] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-06-07 22:10 +0200 |
| Subject | Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tPPrd-2PB-45@gated-at.bofh.it> |
| In reply to | #1659805 |
On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote: > >> +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. I'm not really worried about the two callbacks --- you have the compexity, which is agruably rather manageable, split into a number of caller functions. I'd rather keep it as-is. -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 02:10 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQC8y-8iq-15@gated-at.bofh.it> |
| In reply to | #1660172 |
Hi Sakari, Andy, > Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation > region driver > > On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote: > > >> +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. > > I'm not really worried about the two callbacks --- you have the compexity, > which is agruably rather manageable, split into a number of caller functions. I'd > rather keep it as-is. > Ack
[toc] | [prev] | [next] | [standalone]
| From | "Mani, Rajmohan" <rajmohan.mani@intel.com> |
|---|---|
| Date | 2017-06-10 02:10 +0200 |
| Subject | RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver |
| Message-ID | <tQC8y-8iq-13@gated-at.bofh.it> |
| In reply to | #1659805 |
Hi Andy,
Thanks for the reviews and patience.
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
>
> 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.
>
Ack
> >> +static acpi_status ti_pmic_common_handler(u32 function,
> > + acpi_physical_address address,
> > + u32 bits, u64 *value,
> > + void *handler_context,
>
> > handler_context is unused.
>
Ack
> >> + 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.
>
I have responded on top of Sakari's response.
> >> +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.
>
Ack
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web