* [PATCH v4 0/3] Initial support for XPowers AXP288 PMIC
@ 2014-09-17 0:11 Jacob Pan
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-17 0:11 UTC (permalink / raw)
To: IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Jean Delvare, Samuel Ortiz, Liam Girdwood, Mark Brown,
Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki, Jacob Pan
X-Powers AXP288 is a customized PMIC found on some Intel Baytrail-CR platforms.
It comes with sub-functions such as USB charging, fuel gauge, ADC, and many LDO
and BUCK channels.
By extending the existing AXP20x driver, this patchset adds basic support
for AXP288 PMIC with ADC as one MFD cell device driver.
Currently, the PMIC driver in this patchset does not support platform data
enumeration. But when ACPI _DSD and unified device properties become available,
cell devices with platform data will be added.
This patch does not use intel_soc_pmic core for i2c and regmap handling in that
axp288 shares similar programming interface with other X-Power PMICs supported in
axp20x.c. Therefore, extending axp20x.c to include axp288 makes more sense.
Changes
v4: - removed rename patch, use Kconfig description to list supported devices
- misc clean up in ADC code, use regmap_bulk_read and improve error
handling, etc.
- remove IIO ADC scale, treat raw data as processed since the unit is
already in IIO expected milliamps.
v3: - put all file rename changes in 1/5
- add iio documentation for in_current_raw/scale
- removed global variables in axp2xx
- removed pm callbacks from GPADC
- removed ACPI opregion cell device
- added scales to ADC current and voltage
- removed ADC thermal sensor from sysfs, kernel internal use only
v2:
- use format -M for 1/4
- minor tweak based on Maxime's review
Jacob Pan (3):
mfd/axp20x: extend axp20x to support axp288 pmic
iio/adc: add support for axp288 adc
iio: add documentation for current attribute
Documentation/ABI/testing/sysfs-bus-iio | 9 +
drivers/iio/adc/Kconfig | 8 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/axp288_adc.c | 254 +++++++++++++++++++++++
drivers/mfd/Kconfig | 3 +-
drivers/mfd/axp20x.c | 353 ++++++++++++++++++++++++++------
include/linux/mfd/axp20x.h | 58 ++++++
7 files changed, 626 insertions(+), 60 deletions(-)
create mode 100644 drivers/iio/adc/axp288_adc.c
--
1.9.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-17 0:11 [PATCH v4 0/3] Initial support for XPowers AXP288 PMIC Jacob Pan
@ 2014-09-17 0:11 ` Jacob Pan
2014-09-18 5:39 ` Lee Jones
2014-09-21 13:01 ` Jonathan Cameron
2014-09-17 0:11 ` [PATCH v4 2/3] iio/adc: add support for axp288 adc Jacob Pan
2014-09-17 0:11 ` [PATCH v4 3/3] iio: add documentation for current attribute Jacob Pan
2 siblings, 2 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-17 0:11 UTC (permalink / raw)
To: IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Jean Delvare, Samuel Ortiz, Liam Girdwood, Mark Brown,
Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki, Jacob Pan
X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR platforms. Similar
to AXP202/209, AXP288 comes with USB charger, more LDO and BUCK channels, and
AD converters. It also provides extended status and interrupt reporting
capabilities than the devices currently supported in axp20x.c.
In addition to feature extension, this patch also adds ACPI binding for
enumeration.
This consolidated driver should support more X-Powers' PMICs in both device
tree and ACPI enumerated platforms.
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
drivers/mfd/Kconfig | 3 +-
drivers/mfd/axp20x.c | 353 +++++++++++++++++++++++++++++++++++++--------
include/linux/mfd/axp20x.h | 58 ++++++++
3 files changed, 354 insertions(+), 60 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index de5abf2..c183edb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -74,7 +74,8 @@ config MFD_AXP20X
select REGMAP_IRQ
depends on I2C=y
help
- If you say Y here you get support for the X-Powers AXP202 and AXP209.
+ If you say Y here you get support for the X-Powers AXP202, AXP209 and
+ AXP288 power management IC (PMIC).
This driver include only the core APIs. You have to select individual
components like regulators or the PEK (Power Enable Key) under the
corresponding menus.
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index dee6539..8e03e59 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -1,9 +1,9 @@
/*
- * axp20x.c - MFD core driver for the X-Powers AXP202 and AXP209
+ * axp20x.c - MFD core driver for the X-Powers' Power Management ICs
*
- * AXP20x comprises an adaptive USB-Compatible PWM charger, 2 BUCK DC-DC
- * converters, 5 LDOs, multiple 12-bit ADCs of voltage, current and temperature
- * as well as 4 configurable GPIOs.
+ * AXP20x typically comprises an adaptive USB-Compatible PWM charger, BUCK DC-DC
+ * converters, LDOs, multiple 12-bit ADCs of voltage, current and temperature
+ * as well as configurable GPIOs.
*
* Author: Carlo Caione <carlo@caione.org>
*
@@ -25,9 +25,16 @@
#include <linux/mfd/core.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
+#include <linux/acpi.h>
#define AXP20X_OFF 0x80
+static const char *axp20x_model_names[] = {
+ "AXP202",
+ "AXP209",
+ "AXP288",
+};
+
static const struct regmap_range axp20x_writeable_ranges[] = {
regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
@@ -47,6 +54,25 @@ static const struct regmap_access_table axp20x_volatile_table = {
.n_yes_ranges = ARRAY_SIZE(axp20x_volatile_ranges),
};
+static const struct regmap_range axp288_writeable_ranges[] = {
+ regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ6_STATE),
+ regmap_reg_range(AXP20X_DCDC_MODE, AXP288_FG_TUNE5),
+};
+
+static const struct regmap_range axp288_volatile_ranges[] = {
+ regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
+};
+
+static const struct regmap_access_table axp288_writeable_table = {
+ .yes_ranges = axp288_writeable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(axp288_writeable_ranges),
+};
+
+static const struct regmap_access_table axp288_volatile_table = {
+ .yes_ranges = axp288_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(axp288_volatile_ranges),
+};
+
static struct resource axp20x_pek_resources[] = {
{
.name = "PEK_DBR",
@@ -61,7 +87,40 @@ static struct resource axp20x_pek_resources[] = {
},
};
-static const struct regmap_config axp20x_regmap_config = {
+static struct resource axp288_battery_resources[] = {
+ {
+ .start = AXP288_IRQ_QWBTU,
+ .end = AXP288_IRQ_QWBTU,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_WBTU,
+ .end = AXP288_IRQ_WBTU,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_QWBTO,
+ .end = AXP288_IRQ_QWBTO,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_WBTO,
+ .end = AXP288_IRQ_WBTO,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_WL2,
+ .end = AXP288_IRQ_WL2,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_WL1,
+ .end = AXP288_IRQ_WL1,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct regmap_config axp20x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.wr_table = &axp20x_writeable_table,
@@ -70,47 +129,96 @@ static const struct regmap_config axp20x_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
-#define AXP20X_IRQ(_irq, _off, _mask) \
- [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
+static struct regmap_config axp288_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .wr_table = &axp288_writeable_table,
+ .volatile_table = &axp288_volatile_table,
+ .max_register = AXP288_FG_TUNE5,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+#define INIT_REGMAP_IRQ(_variant, _irq, _off, _mask) \
+ [_variant##_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
static const struct regmap_irq axp20x_regmap_irqs[] = {
- AXP20X_IRQ(ACIN_OVER_V, 0, 7),
- AXP20X_IRQ(ACIN_PLUGIN, 0, 6),
- AXP20X_IRQ(ACIN_REMOVAL, 0, 5),
- AXP20X_IRQ(VBUS_OVER_V, 0, 4),
- AXP20X_IRQ(VBUS_PLUGIN, 0, 3),
- AXP20X_IRQ(VBUS_REMOVAL, 0, 2),
- AXP20X_IRQ(VBUS_V_LOW, 0, 1),
- AXP20X_IRQ(BATT_PLUGIN, 1, 7),
- AXP20X_IRQ(BATT_REMOVAL, 1, 6),
- AXP20X_IRQ(BATT_ENT_ACT_MODE, 1, 5),
- AXP20X_IRQ(BATT_EXIT_ACT_MODE, 1, 4),
- AXP20X_IRQ(CHARG, 1, 3),
- AXP20X_IRQ(CHARG_DONE, 1, 2),
- AXP20X_IRQ(BATT_TEMP_HIGH, 1, 1),
- AXP20X_IRQ(BATT_TEMP_LOW, 1, 0),
- AXP20X_IRQ(DIE_TEMP_HIGH, 2, 7),
- AXP20X_IRQ(CHARG_I_LOW, 2, 6),
- AXP20X_IRQ(DCDC1_V_LONG, 2, 5),
- AXP20X_IRQ(DCDC2_V_LONG, 2, 4),
- AXP20X_IRQ(DCDC3_V_LONG, 2, 3),
- AXP20X_IRQ(PEK_SHORT, 2, 1),
- AXP20X_IRQ(PEK_LONG, 2, 0),
- AXP20X_IRQ(N_OE_PWR_ON, 3, 7),
- AXP20X_IRQ(N_OE_PWR_OFF, 3, 6),
- AXP20X_IRQ(VBUS_VALID, 3, 5),
- AXP20X_IRQ(VBUS_NOT_VALID, 3, 4),
- AXP20X_IRQ(VBUS_SESS_VALID, 3, 3),
- AXP20X_IRQ(VBUS_SESS_END, 3, 2),
- AXP20X_IRQ(LOW_PWR_LVL1, 3, 1),
- AXP20X_IRQ(LOW_PWR_LVL2, 3, 0),
- AXP20X_IRQ(TIMER, 4, 7),
- AXP20X_IRQ(PEK_RIS_EDGE, 4, 6),
- AXP20X_IRQ(PEK_FAL_EDGE, 4, 5),
- AXP20X_IRQ(GPIO3_INPUT, 4, 3),
- AXP20X_IRQ(GPIO2_INPUT, 4, 2),
- AXP20X_IRQ(GPIO1_INPUT, 4, 1),
- AXP20X_IRQ(GPIO0_INPUT, 4, 0),
+ INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V, 0, 7),
+ INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN, 0, 6),
+ INIT_REGMAP_IRQ(AXP20X, ACIN_REMOVAL, 0, 5),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_OVER_V, 0, 4),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_PLUGIN, 0, 3),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_REMOVAL, 0, 2),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_V_LOW, 0, 1),
+ INIT_REGMAP_IRQ(AXP20X, BATT_PLUGIN, 1, 7),
+ INIT_REGMAP_IRQ(AXP20X, BATT_REMOVAL, 1, 6),
+ INIT_REGMAP_IRQ(AXP20X, BATT_ENT_ACT_MODE, 1, 5),
+ INIT_REGMAP_IRQ(AXP20X, BATT_EXIT_ACT_MODE, 1, 4),
+ INIT_REGMAP_IRQ(AXP20X, CHARG, 1, 3),
+ INIT_REGMAP_IRQ(AXP20X, CHARG_DONE, 1, 2),
+ INIT_REGMAP_IRQ(AXP20X, BATT_TEMP_HIGH, 1, 1),
+ INIT_REGMAP_IRQ(AXP20X, BATT_TEMP_LOW, 1, 0),
+ INIT_REGMAP_IRQ(AXP20X, DIE_TEMP_HIGH, 2, 7),
+ INIT_REGMAP_IRQ(AXP20X, CHARG_I_LOW, 2, 6),
+ INIT_REGMAP_IRQ(AXP20X, DCDC1_V_LONG, 2, 5),
+ INIT_REGMAP_IRQ(AXP20X, DCDC2_V_LONG, 2, 4),
+ INIT_REGMAP_IRQ(AXP20X, DCDC3_V_LONG, 2, 3),
+ INIT_REGMAP_IRQ(AXP20X, PEK_SHORT, 2, 1),
+ INIT_REGMAP_IRQ(AXP20X, PEK_LONG, 2, 0),
+ INIT_REGMAP_IRQ(AXP20X, N_OE_PWR_ON, 3, 7),
+ INIT_REGMAP_IRQ(AXP20X, N_OE_PWR_OFF, 3, 6),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_VALID, 3, 5),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_NOT_VALID, 3, 4),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_SESS_VALID, 3, 3),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_SESS_END, 3, 2),
+ INIT_REGMAP_IRQ(AXP20X, LOW_PWR_LVL1, 3, 1),
+ INIT_REGMAP_IRQ(AXP20X, LOW_PWR_LVL2, 3, 0),
+ INIT_REGMAP_IRQ(AXP20X, TIMER, 4, 7),
+ INIT_REGMAP_IRQ(AXP20X, PEK_RIS_EDGE, 4, 6),
+ INIT_REGMAP_IRQ(AXP20X, PEK_FAL_EDGE, 4, 5),
+ INIT_REGMAP_IRQ(AXP20X, GPIO3_INPUT, 4, 3),
+ INIT_REGMAP_IRQ(AXP20X, GPIO2_INPUT, 4, 2),
+ INIT_REGMAP_IRQ(AXP20X, GPIO1_INPUT, 4, 1),
+ INIT_REGMAP_IRQ(AXP20X, GPIO0_INPUT, 4, 0),
+};
+
+/* some IRQs are compatible with axp20x models */
+static const struct regmap_irq axp288_regmap_irqs[] = {
+ INIT_REGMAP_IRQ(AXP20X, VBUS_REMOVAL, 0, 2),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_PLUGIN, 0, 3),
+ INIT_REGMAP_IRQ(AXP20X, VBUS_OVER_V, 0, 4),
+
+ INIT_REGMAP_IRQ(AXP20X, CHARG_DONE, 1, 2),
+ INIT_REGMAP_IRQ(AXP20X, CHARG, 1, 3),
+ INIT_REGMAP_IRQ(AXP288, SAFE_QUIT, 1, 4),
+ INIT_REGMAP_IRQ(AXP288, SAFE_ENTER, 1, 5),
+ INIT_REGMAP_IRQ(AXP20X, BATT_REMOVAL, 1, 6),
+ INIT_REGMAP_IRQ(AXP20X, BATT_PLUGIN, 1, 7),
+
+ INIT_REGMAP_IRQ(AXP288, QWBTU, 2, 0),
+ INIT_REGMAP_IRQ(AXP288, WBTU, 2, 1),
+ INIT_REGMAP_IRQ(AXP288, QWBTO, 2, 2),
+ INIT_REGMAP_IRQ(AXP288, WBTU, 2, 3),
+ INIT_REGMAP_IRQ(AXP288, QCBTU, 2, 4),
+ INIT_REGMAP_IRQ(AXP288, CBTU, 2, 5),
+ INIT_REGMAP_IRQ(AXP288, QCBTO, 2, 6),
+ INIT_REGMAP_IRQ(AXP288, CBTO, 2, 7),
+
+ INIT_REGMAP_IRQ(AXP288, WL2, 3, 0),
+ INIT_REGMAP_IRQ(AXP288, WL1, 3, 1),
+ INIT_REGMAP_IRQ(AXP288, GPADC, 3, 2),
+ INIT_REGMAP_IRQ(AXP288, OT, 3, 7),
+
+ INIT_REGMAP_IRQ(AXP288, GPIO0, 4, 0),
+ INIT_REGMAP_IRQ(AXP288, GPIO1, 4, 1),
+ INIT_REGMAP_IRQ(AXP288, POKO, 4, 2),
+ INIT_REGMAP_IRQ(AXP288, POKL, 4, 3),
+ INIT_REGMAP_IRQ(AXP288, POKS, 4, 4),
+ INIT_REGMAP_IRQ(AXP288, POKN, 4, 5),
+ INIT_REGMAP_IRQ(AXP288, POKP, 4, 6),
+ INIT_REGMAP_IRQ(AXP20X, TIMER, 4, 7),
+
+ INIT_REGMAP_IRQ(AXP288, MV_CHNG, 5, 0),
+ INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG, 5, 1),
};
static const struct of_device_id axp20x_of_match[] = {
@@ -128,14 +236,21 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
};
MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id);
-static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
+static struct acpi_device_id axp20x_acpi_match[] = {
+ {
+ .id = "INT33F4",
+ .driver_data = (kernel_ulong_t)AXP288_ID,
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
+
+/* common irq chip attributes only */
+static struct regmap_irq_chip axp20x_regmap_irq_chip = {
.name = "axp20x_irq_chip",
.status_base = AXP20X_IRQ1_STATE,
.ack_base = AXP20X_IRQ1_STATE,
.mask_base = AXP20X_IRQ1_EN,
- .num_regs = 5,
- .irqs = axp20x_regmap_irqs,
- .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs),
.mask_invert = true,
.init_ack_masked = true,
};
@@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
},
};
+static struct resource axp288_adc_resources[] = {
+ {
+ .name = "GPADC",
+ .start = AXP288_IRQ_GPADC,
+ .end = AXP288_IRQ_GPADC,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct resource axp288_charger_resources[] = {
+ {
+ .start = AXP288_IRQ_OV,
+ .end = AXP288_IRQ_OV,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_DONE,
+ .end = AXP288_IRQ_DONE,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_CHARGING,
+ .end = AXP288_IRQ_CHARGING,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_SAFE_QUIT,
+ .end = AXP288_IRQ_SAFE_QUIT,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_SAFE_ENTER,
+ .end = AXP288_IRQ_SAFE_ENTER,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_QCBTU,
+ .end = AXP288_IRQ_QCBTU,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_CBTU,
+ .end = AXP288_IRQ_CBTU,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_QCBTO,
+ .end = AXP288_IRQ_QCBTO,
+ .flags = IORESOURCE_IRQ,
+ },
+ {
+ .start = AXP288_IRQ_CBTO,
+ .end = AXP288_IRQ_CBTO,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct mfd_cell axp288_cells[] = {
+ {
+ .name = "axp288_adc",
+ .num_resources = ARRAY_SIZE(axp288_adc_resources),
+ .resources = axp288_adc_resources,
+ },
+ {
+ .name = "axp288_charger",
+ .num_resources = ARRAY_SIZE(axp288_charger_resources),
+ .resources = axp288_charger_resources,
+ },
+ {
+ .name = "axp288_battery",
+ .num_resources = ARRAY_SIZE(axp288_battery_resources),
+ .resources = axp288_battery_resources,
+ },
+};
+
static struct axp20x_dev *axp20x_pm_power_off;
static void axp20x_power_off(void)
{
+ if (axp20x_pm_power_off->variant == AXP288_ID)
+ return;
regmap_write(axp20x_pm_power_off->regmap, AXP20X_OFF_CTRL,
AXP20X_OFF);
}
+static int axp20x_match_device(struct axp20x_dev *axp20x, struct device *dev)
+{
+ const struct acpi_device_id *acpi_id;
+ const struct of_device_id *of_id;
+
+ of_id = of_match_device(axp20x_of_match, dev);
+ if (of_id)
+ axp20x->variant = (long) of_id->data;
+ else {
+ acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+ if (!acpi_id || !acpi_id->driver_data) {
+ dev_err(dev, "Unable to determine ID\n");
+ return -ENODEV;
+ }
+ axp20x->variant = (long) acpi_id->driver_data;
+ }
+ switch (axp20x->variant) {
+ case AXP202_ID:
+ case AXP209_ID:
+ axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
+ axp20x->cells = axp20x_cells;
+ axp20x->regmap_cfg = &axp20x_regmap_config;
+ axp20x_regmap_irq_chip.num_regs = 5;
+ axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
+ axp20x_regmap_irq_chip.num_irqs =
+ ARRAY_SIZE(axp20x_regmap_irqs);
+ break;
+ case AXP288_ID:
+ axp20x->cells = axp288_cells;
+ axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
+ axp20x->regmap_cfg = &axp288_regmap_config;
+ axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
+ axp20x_regmap_irq_chip.num_irqs =
+ ARRAY_SIZE(axp288_regmap_irqs);
+ axp20x_regmap_irq_chip.num_regs = 6;
+ break;
+ default:
+ dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
+ return -ENODEV;
+ }
+ dev_info(dev, "AXP20x variant %s found\n",
+ axp20x_model_names[axp20x->variant]);
+
+ return 0;
+}
+
static int axp20x_i2c_probe(struct i2c_client *i2c,
- const struct i2c_device_id *id)
+ const struct i2c_device_id *id)
{
struct axp20x_dev *axp20x;
- const struct of_device_id *of_id;
int ret;
axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
if (!axp20x)
return -ENOMEM;
- of_id = of_match_device(axp20x_of_match, &i2c->dev);
- if (!of_id) {
- dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
- return -ENODEV;
- }
- axp20x->variant = (long) of_id->data;
+ ret = axp20x_match_device(axp20x, &i2c->dev);
+ if (ret)
+ return ret;
axp20x->i2c_client = i2c;
axp20x->dev = &i2c->dev;
dev_set_drvdata(axp20x->dev, axp20x);
- axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
+ axp20x->regmap = devm_regmap_init_i2c(i2c, axp20x->regmap_cfg);
if (IS_ERR(axp20x->regmap)) {
ret = PTR_ERR(axp20x->regmap);
dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
@@ -206,8 +440,8 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
return ret;
}
- ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
- ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
+ ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
+ axp20x->nr_cells, NULL, 0, NULL);
if (ret) {
dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
@@ -245,6 +479,7 @@ static struct i2c_driver axp20x_i2c_driver = {
.name = "axp20x",
.owner = THIS_MODULE,
.of_match_table = of_match_ptr(axp20x_of_match),
+ .acpi_match_table = ACPI_PTR(axp20x_acpi_match),
},
.probe = axp20x_i2c_probe,
.remove = axp20x_i2c_remove,
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index d0e31a2..ffc69e2 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -14,6 +14,8 @@
enum {
AXP202_ID = 0,
AXP209_ID,
+ AXP288_ID,
+ NR_AXP20X_VARIANTS,
};
#define AXP20X_DATACACHE(m) (0x04 + (m))
@@ -49,11 +51,13 @@ enum {
#define AXP20X_IRQ3_EN 0x42
#define AXP20X_IRQ4_EN 0x43
#define AXP20X_IRQ5_EN 0x44
+#define AXP20X_IRQ6_EN 0x45
#define AXP20X_IRQ1_STATE 0x48
#define AXP20X_IRQ2_STATE 0x49
#define AXP20X_IRQ3_STATE 0x4a
#define AXP20X_IRQ4_STATE 0x4b
#define AXP20X_IRQ5_STATE 0x4c
+#define AXP20X_IRQ6_STATE 0x4d
/* ADC */
#define AXP20X_ACIN_V_ADC_H 0x56
@@ -116,6 +120,15 @@ enum {
#define AXP20X_CC_CTRL 0xb8
#define AXP20X_FG_RES 0xb9
+/* AXP288 specific registers */
+#define AXP288_PMIC_ADC_H 0x56
+#define AXP288_PMIC_ADC_L 0x57
+#define AXP288_ADC_TS_PIN_CTRL 0x84
+
+#define AXP288_PMIC_ADC_EN 0x84
+#define AXP288_FG_TUNE5 0xed
+
+
/* Regulators IDs */
enum {
AXP20X_LDO1 = 0,
@@ -169,12 +182,57 @@ enum {
AXP20X_IRQ_GPIO0_INPUT,
};
+enum axp288_irqs {
+ AXP288_IRQ_VBUS_FALL = 2,
+ AXP288_IRQ_VBUS_RISE,
+ AXP288_IRQ_OV,
+ AXP288_IRQ_FALLING_ALT,
+ AXP288_IRQ_RISING_ALT,
+ AXP288_IRQ_OV_ALT,
+ AXP288_IRQ_DONE = 10,
+ AXP288_IRQ_CHARGING,
+ AXP288_IRQ_SAFE_QUIT,
+ AXP288_IRQ_SAFE_ENTER,
+ AXP288_IRQ_ABSENT,
+ AXP288_IRQ_APPEND,
+ AXP288_IRQ_QWBTU,
+ AXP288_IRQ_WBTU,
+ AXP288_IRQ_QWBTO,
+ AXP288_IRQ_WBTO,
+ AXP288_IRQ_QCBTU,
+ AXP288_IRQ_CBTU,
+ AXP288_IRQ_QCBTO,
+ AXP288_IRQ_CBTO,
+ AXP288_IRQ_WL2,
+ AXP288_IRQ_WL1,
+ AXP288_IRQ_GPADC,
+ AXP288_IRQ_OT = 31,
+ AXP288_IRQ_GPIO0,
+ AXP288_IRQ_GPIO1,
+ AXP288_IRQ_POKO,
+ AXP288_IRQ_POKL,
+ AXP288_IRQ_POKS,
+ AXP288_IRQ_POKN,
+ AXP288_IRQ_POKP,
+ AXP288_IRQ_TIMER,
+ AXP288_IRQ_MV_CHNG,
+ AXP288_IRQ_BC_USB_CHNG,
+};
+
+#define AXP288_TS_ADC_H 0x58
+#define AXP288_TS_ADC_L 0x59
+#define AXP288_GP_ADC_H 0x5a
+#define AXP288_GP_ADC_L 0x5b
+
struct axp20x_dev {
struct device *dev;
struct i2c_client *i2c_client;
struct regmap *regmap;
struct regmap_irq_chip_data *regmap_irqc;
long variant;
+ int nr_cells;
+ struct mfd_cell *cells;
+ struct regmap_config *regmap_cfg;
};
#endif /* __LINUX_MFD_AXP20X_H */
--
1.9.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 0:11 [PATCH v4 0/3] Initial support for XPowers AXP288 PMIC Jacob Pan
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
@ 2014-09-17 0:11 ` Jacob Pan
2014-09-17 22:20 ` Hartmut Knaack
2014-09-21 13:14 ` Jonathan Cameron
2014-09-17 0:11 ` [PATCH v4 3/3] iio: add documentation for current attribute Jacob Pan
2 siblings, 2 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-17 0:11 UTC (permalink / raw)
To: IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Jean Delvare, Samuel Ortiz, Liam Girdwood, Mark Brown,
Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki, Jacob Pan
Platform driver for X-Powers AXP288 ADC, which is a sub-device of the
customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC device
enumerates as one of the MFD cell devices. It uses IIO infrastructure
to communicate with userspace and consumer drivers.
Usages of ADC channels include battery charging and thermal sensors.
Based on initial work by:
Ramakrishna Pallala <ramakrishna.pallala@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
drivers/iio/adc/Kconfig | 8 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/axp288_adc.c | 254 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 263 insertions(+)
create mode 100644 drivers/iio/adc/axp288_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 11b048a..db2681b 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -127,6 +127,14 @@ config AT91_ADC
help
Say yes here to build support for Atmel AT91 ADC.
+config AXP288_ADC
+ tristate "X-Powers AXP288 ADC driver"
+ depends on MFD_AXP20X
+ help
+ Say yes here to have support for X-Powers power management IC (PMIC) ADC
+ device. Depending on platform configuration, this general purpose ADC can
+ be used for sampling sensors such as thermal resistors.
+
config EXYNOS_ADC
tristate "Exynos ADC driver support"
depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ad81b51..19640f9 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AD799X) += ad799x.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1027) += max1027.o
diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
new file mode 100644
index 0000000..333c624
--- /dev/null
+++ b/drivers/iio/adc/axp288_adc.c
@@ -0,0 +1,254 @@
+/*
+ * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
+ *
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/platform_device.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
+
+#define AXP288_ADC_EN_MASK 0xF1
+#define AXP288_ADC_TS_PIN_GPADC 0xF2
+#define AXP288_ADC_TS_PIN_ON 0xF3
+
+enum axp288_adc_id {
+ AXP288_ADC_TS,
+ AXP288_ADC_PMIC,
+ AXP288_ADC_GP,
+ AXP288_ADC_BATT_CHRG_I,
+ AXP288_ADC_BATT_DISCHRG_I,
+ AXP288_ADC_BATT_V,
+ AXP288_ADC_NR_CHAN,
+};
+
+struct axp288_adc_info {
+ int irq;
+ struct device *dev;
+ struct regmap *regmap;
+};
+
+static const struct iio_chan_spec const axp288_adc_channels[] = {
+ {
+ .indexed = 1,
+ .type = IIO_TEMP,
+ .channel = 0,
+ .address = AXP288_TS_ADC_H,
+ .datasheet_name = "CH0",
+ }, {
+ .indexed = 1,
+ .type = IIO_TEMP,
+ .channel = 1,
+ .address = AXP288_PMIC_ADC_H,
+ .datasheet_name = "CH1",
+ }, {
+ .indexed = 1,
+ .type = IIO_TEMP,
+ .channel = 2,
+ .address = AXP288_GP_ADC_H,
+ .datasheet_name = "CH2",
+ }, {
+ .indexed = 1,
+ .type = IIO_CURRENT,
+ .channel = 3,
+ .address = AXP20X_BATT_CHRG_I_H,
+ .datasheet_name = "CH3",
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ }, {
+ .indexed = 1,
+ .type = IIO_CURRENT,
+ .channel = 4,
+ .address = AXP20X_BATT_DISCHRG_I_H,
+ .datasheet_name = "CH4",
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ }, {
+ .indexed = 1,
+ .type = IIO_VOLTAGE,
+ .channel = 5,
+ .address = AXP20X_BATT_V_H,
+ .datasheet_name = "CH5",
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+};
+
+#define AXP288_ADC_MAP(_adc_channel_label, _consumer_dev_name, \
+ _consumer_channel) \
+ { \
+ .adc_channel_label = _adc_channel_label, \
+ .consumer_dev_name = _consumer_dev_name, \
+ .consumer_channel = _consumer_channel, \
+ }
+
+/* for consumer drivers */
+static struct iio_map axp288_adc_default_maps[] = {
+ AXP288_ADC_MAP("TS_PIN", "axp288-batt", "axp288-batt-temp"),
+ AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic", "axp288-pmic-temp"),
+ AXP288_ADC_MAP("GPADC", "axp288-gpadc", "axp288-system-temp"),
+ AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg", "axp288-chrg-curr"),
+ AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg", "axp288-chrg-d-curr"),
+ AXP288_ADC_MAP("BATT_V", "axp288-batt", "axp288-batt-volt"),
+ {},
+};
+
+static int axp288_adc_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ int ret;
+ struct axp288_adc_info *info = iio_priv(indio_dev);
+ u8 buf[2];
+
+ mutex_lock(&indio_dev->mlock);
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ /* special case for GPADC sample */
+ if (chan->address == AXP288_GP_ADC_H) {
+ ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
+ AXP288_ADC_TS_PIN_GPADC);
+ if (ret) {
+ dev_err(&indio_dev->dev, "GPADC mode\n");
+ mutex_unlock(&indio_dev->mlock);
+ return ret;
+ }
+ }
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = regmap_bulk_read(info->regmap, chan->address, buf, 2);
+ if (ret) {
+ dev_err(&indio_dev->dev, "sample raw data failed\n");
+ break;
+ }
+ *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
+ ret = IIO_VAL_INT;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ if (mask == IIO_CHAN_INFO_RAW && chan->address == AXP288_GP_ADC_H)
+ ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
+ AXP288_ADC_TS_PIN_ON);
+
+ mutex_unlock(&indio_dev->mlock);
+
+ return ret;
+}
+
+static int axp288_adc_enable(struct regmap *regmap, bool enable)
+{
+ unsigned int pin_on, adc_en;
+
+ if (enable) {
+ pin_on = AXP288_ADC_TS_PIN_ON;
+ adc_en = AXP288_ADC_EN_MASK;
+ } else {
+ pin_on = ~AXP288_ADC_TS_PIN_ON;
+ adc_en = ~AXP288_ADC_EN_MASK;
+ }
+ if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
+ return -EIO;
+
+ return regmap_write(regmap, AXP20X_ADC_EN1, ~AXP288_ADC_EN_MASK);
+}
+
+static const struct iio_info axp288_adc_iio_info = {
+ .read_raw = &axp288_adc_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static int axp288_adc_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct axp288_adc_info *info;
+ struct iio_dev *indio_dev;
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ info = iio_priv(indio_dev);
+ info->irq = platform_get_irq(pdev, 0);
+ if (info->irq < 0) {
+ dev_err(&pdev->dev, "no irq resource?\n");
+ return info->irq;
+ }
+ platform_set_drvdata(pdev, indio_dev);
+ info->regmap = axp20x->regmap;
+ ret = axp288_adc_enable(axp20x->regmap, true);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to enable ADC device\n");
+ return ret;
+ }
+
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->name = pdev->name;
+ indio_dev->channels = axp288_adc_channels;
+ indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
+ indio_dev->info = &axp288_adc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ ret = iio_map_array_register(indio_dev, axp288_adc_default_maps);
+ if (ret < 0)
+ goto err_disable_dev;
+
+ ret = iio_device_register(indio_dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "unable to register iio device\n");
+ goto err_array_unregister;
+ }
+
+ return 0;
+
+err_array_unregister:
+ iio_map_array_unregister(indio_dev);
+err_disable_dev:
+ ret = axp288_adc_enable(axp20x->regmap, false);
+
+ return ret;
+}
+
+static int axp288_adc_remove(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+
+ if (axp288_adc_enable(axp20x->regmap, false))
+ dev_err(&pdev->dev, "Unable to disable ADC device\n");
+ iio_device_unregister(indio_dev);
+ iio_map_array_unregister(indio_dev);
+
+ return 0;
+}
+
+static struct platform_driver axp288_adc_driver = {
+ .probe = axp288_adc_probe,
+ .remove = axp288_adc_remove,
+ .driver = {
+ .name = "axp288_adc",
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(axp288_adc_driver);
+
+MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
+MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 3/3] iio: add documentation for current attribute
2014-09-17 0:11 [PATCH v4 0/3] Initial support for XPowers AXP288 PMIC Jacob Pan
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
2014-09-17 0:11 ` [PATCH v4 2/3] iio/adc: add support for axp288 adc Jacob Pan
@ 2014-09-17 0:11 ` Jacob Pan
2014-09-21 13:03 ` Jonathan Cameron
2 siblings, 1 reply; 17+ messages in thread
From: Jacob Pan @ 2014-09-17 0:11 UTC (permalink / raw)
To: IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Jean Delvare, Samuel Ortiz, Liam Girdwood, Mark Brown,
Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki, Jacob Pan
Add documentation for input current raw sysfs attribute.
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index d760b02..1ac9ac2 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1028,3 +1028,12 @@ Contact: linux-iio@vger.kernel.org
Description:
Raw value of rotation from true/magnetic north measured with
or without compensation from tilt sensors.
+
+What: /sys/bus/iio/devices/iio:deviceX/in_currentX_raw
+KernelVersion: 3.18
+Contact: linux-iio@vger.kernel.org
+Description:
+ Raw current measurement from channel X. Units are in milliamps
+ after application of scale and offset. If no offset or scale is
+ present, output should be considered as processed with the
+ unit in milliamps.
--
1.9.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 0:11 ` [PATCH v4 2/3] iio/adc: add support for axp288 adc Jacob Pan
@ 2014-09-17 22:20 ` Hartmut Knaack
2014-09-17 22:30 ` Hartmut Knaack
2014-09-17 23:13 ` Jacob Pan
2014-09-21 13:14 ` Jonathan Cameron
1 sibling, 2 replies; 17+ messages in thread
From: Hartmut Knaack @ 2014-09-17 22:20 UTC (permalink / raw)
To: Jacob Pan, IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Samuel Ortiz, Liam Girdwood, Mark Brown, Grant Likely,
Greg Kroah-Hartman, Rob Herring, Lars-Peter Clausen, Fugang Duan,
Arnd Bergmann, Zubair Lutfullah, Sebastian Reichel,
Johannes Thumshirn, Philippe Reynes, Angelo Compagnucci,
Doug Anderson, Ramakrishna Pallala, Peter Meerwald,
Maxime Ripard, Rafael Wysocki
Jacob Pan schrieb, Am 17.09.2014 02:11:
> Platform driver for X-Powers AXP288 ADC, which is a sub-device of the
> customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC device
> enumerates as one of the MFD cell devices. It uses IIO infrastructure
> to communicate with userspace and consumer drivers.
>
> Usages of ADC channels include battery charging and thermal sensors.
>
You are progressing, but still a few issues left. One thing is indentation in some places (I feel like being pretty annoying about this), others are commented in line.
> Based on initial work by:
> Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
> drivers/iio/adc/Kconfig | 8 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/axp288_adc.c | 254 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 263 insertions(+)
> create mode 100644 drivers/iio/adc/axp288_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 11b048a..db2681b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -127,6 +127,14 @@ config AT91_ADC
> help
> Say yes here to build support for Atmel AT91 ADC.
>
> +config AXP288_ADC
> + tristate "X-Powers AXP288 ADC driver"
> + depends on MFD_AXP20X
> + help
> + Say yes here to have support for X-Powers power management IC (PMIC) ADC
> + device. Depending on platform configuration, this general purpose ADC can
> + be used for sampling sensors such as thermal resistors.
> +
> config EXYNOS_ADC
> tristate "Exynos ADC driver support"
> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ad81b51..19640f9 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AD799X) += ad799x.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1027) += max1027.o
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> new file mode 100644
> index 0000000..333c624
> --- /dev/null
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -0,0 +1,254 @@
> +/*
> + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +
> +#define AXP288_ADC_EN_MASK 0xF1
> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
> +#define AXP288_ADC_TS_PIN_ON 0xF3
> +
> +enum axp288_adc_id {
> + AXP288_ADC_TS,
> + AXP288_ADC_PMIC,
> + AXP288_ADC_GP,
> + AXP288_ADC_BATT_CHRG_I,
> + AXP288_ADC_BATT_DISCHRG_I,
> + AXP288_ADC_BATT_V,
> + AXP288_ADC_NR_CHAN,
> +};
> +
> +struct axp288_adc_info {
> + int irq;
> + struct device *dev;
> + struct regmap *regmap;
> +};
> +
> +static const struct iio_chan_spec const axp288_adc_channels[] = {
> + {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 0,
> + .address = AXP288_TS_ADC_H,
> + .datasheet_name = "CH0",
> + }, {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 1,
> + .address = AXP288_PMIC_ADC_H,
> + .datasheet_name = "CH1",
> + }, {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 2,
> + .address = AXP288_GP_ADC_H,
> + .datasheet_name = "CH2",
> + }, {
> + .indexed = 1,
> + .type = IIO_CURRENT,
> + .channel = 3,
> + .address = AXP20X_BATT_CHRG_I_H,
> + .datasheet_name = "CH3",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + }, {
> + .indexed = 1,
> + .type = IIO_CURRENT,
> + .channel = 4,
> + .address = AXP20X_BATT_DISCHRG_I_H,
> + .datasheet_name = "CH4",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + }, {
> + .indexed = 1,
> + .type = IIO_VOLTAGE,
> + .channel = 5,
> + .address = AXP20X_BATT_V_H,
> + .datasheet_name = "CH5",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> +};
> +
> +#define AXP288_ADC_MAP(_adc_channel_label, _consumer_dev_name, \
> + _consumer_channel) \
> + { \
> + .adc_channel_label = _adc_channel_label, \
> + .consumer_dev_name = _consumer_dev_name, \
> + .consumer_channel = _consumer_channel, \
> + }
> +
> +/* for consumer drivers */
> +static struct iio_map axp288_adc_default_maps[] = {
> + AXP288_ADC_MAP("TS_PIN", "axp288-batt", "axp288-batt-temp"),
> + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic", "axp288-pmic-temp"),
> + AXP288_ADC_MAP("GPADC", "axp288-gpadc", "axp288-system-temp"),
> + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg", "axp288-chrg-curr"),
> + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg", "axp288-chrg-d-curr"),
> + AXP288_ADC_MAP("BATT_V", "axp288-batt", "axp288-batt-volt"),
> + {},
> +};
> +
> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int ret;
> + struct axp288_adc_info *info = iio_priv(indio_dev);
> + u8 buf[2];
You could use __be16 instead. Then you just need to use be16_to_cpu(), shift 4 bits to the right and mask the result with 0xFF.
> +
> + mutex_lock(&indio_dev->mlock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + /* special case for GPADC sample */
> + if (chan->address == AXP288_GP_ADC_H) {
> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_PIN_GPADC);
> + if (ret) {
> + dev_err(&indio_dev->dev, "GPADC mode\n");
> + mutex_unlock(&indio_dev->mlock);
> + return ret;
You may even just goto the mutex_unlock at the end of the function. Would safe a few bytes here in the source - up to you. More interesting is, if the error message is required, as you don't have one below, when switching to TS_PIN_ON.
> + }
> + }
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = regmap_bulk_read(info->regmap, chan->address, buf, 2);
> + if (ret) {
> + dev_err(&indio_dev->dev, "sample raw data failed\n");
> + break;
> + }
> + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + if (mask == IIO_CHAN_INFO_RAW && chan->address == AXP288_GP_ADC_H)
> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_PIN_ON);
> +
> + mutex_unlock(&indio_dev->mlock);
> +
> + return ret;
> +}
> +
> +static int axp288_adc_enable(struct regmap *regmap, bool enable)
> +{
> + unsigned int pin_on, adc_en;
> +
> + if (enable) {
> + pin_on = AXP288_ADC_TS_PIN_ON;
> + adc_en = AXP288_ADC_EN_MASK;
> + } else {
> + pin_on = ~AXP288_ADC_TS_PIN_ON;
> + adc_en = ~AXP288_ADC_EN_MASK;
> + }
> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
> + return -EIO;
> +
> + return regmap_write(regmap, AXP20X_ADC_EN1, ~AXP288_ADC_EN_MASK);
So, what's the deal about the variable adc_en?
> +}
> +
> +static const struct iio_info axp288_adc_iio_info = {
> + .read_raw = &axp288_adc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int axp288_adc_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct axp288_adc_info *info;
> + struct iio_dev *indio_dev;
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(&pdev->dev, "no irq resource?\n");
> + return info->irq;
> + }
> + platform_set_drvdata(pdev, indio_dev);
> + info->regmap = axp20x->regmap;
> + ret = axp288_adc_enable(axp20x->regmap, true);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable ADC device\n");
> + return ret;
> + }
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = pdev->name;
> + indio_dev->channels = axp288_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
> + indio_dev->info = &axp288_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + ret = iio_map_array_register(indio_dev, axp288_adc_default_maps);
> + if (ret < 0)
> + goto err_disable_dev;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "unable to register iio device\n");
> + goto err_array_unregister;
> + }
> +
> + return 0;
> +
> +err_array_unregister:
> + iio_map_array_unregister(indio_dev);
> +err_disable_dev:
> + ret = axp288_adc_enable(axp20x->regmap, false);
Don't set ret here. We want to return the error code from the function that failed above.
> +
> + return ret;
> +}
> +
> +static int axp288_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +
> + if (axp288_adc_enable(axp20x->regmap, false))
> + dev_err(&pdev->dev, "Unable to disable ADC device\n");
> + iio_device_unregister(indio_dev);
> + iio_map_array_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver axp288_adc_driver = {
> + .probe = axp288_adc_probe,
> + .remove = axp288_adc_remove,
> + .driver = {
> + .name = "axp288_adc",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(axp288_adc_driver);
> +
> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
> +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 22:20 ` Hartmut Knaack
@ 2014-09-17 22:30 ` Hartmut Knaack
2014-09-17 23:13 ` Jacob Pan
1 sibling, 0 replies; 17+ messages in thread
From: Hartmut Knaack @ 2014-09-17 22:30 UTC (permalink / raw)
To: Jacob Pan, IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Samuel Ortiz, Liam Girdwood, Mark Brown, Grant Likely,
Greg Kroah-Hartman, Rob Herring, Lars-Peter Clausen, Fugang Duan,
Arnd Bergmann, Zubair Lutfullah, Sebastian Reichel,
Johannes Thumshirn, Philippe Reynes, Angelo Compagnucci,
Doug Anderson, Ramakrishna Pallala, Peter Meerwald,
Maxime Ripard, Rafael Wysocki
Hartmut Knaack schrieb, Am 18.09.2014 00:20:
> Jacob Pan schrieb, Am 17.09.2014 02:11:
>> Platform driver for X-Powers AXP288 ADC, which is a sub-device of the
>> customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC device
>> enumerates as one of the MFD cell devices. It uses IIO infrastructure
>> to communicate with userspace and consumer drivers.
>>
>> Usages of ADC channels include battery charging and thermal sensors.
>>
> You are progressing, but still a few issues left. One thing is indentation in some places (I feel like being pretty annoying about this), others are commented in line.
>> Based on initial work by:
>> Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>
>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> ---
>> drivers/iio/adc/Kconfig | 8 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/axp288_adc.c | 254 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 263 insertions(+)
>> create mode 100644 drivers/iio/adc/axp288_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 11b048a..db2681b 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -127,6 +127,14 @@ config AT91_ADC
>> help
>> Say yes here to build support for Atmel AT91 ADC.
>>
>> +config AXP288_ADC
>> + tristate "X-Powers AXP288 ADC driver"
>> + depends on MFD_AXP20X
>> + help
>> + Say yes here to have support for X-Powers power management IC (PMIC) ADC
>> + device. Depending on platform configuration, this general purpose ADC can
>> + be used for sampling sensors such as thermal resistors.
>> +
>> config EXYNOS_ADC
>> tristate "Exynos ADC driver support"
>> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index ad81b51..19640f9 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>> obj-$(CONFIG_AD7887) += ad7887.o
>> obj-$(CONFIG_AD799X) += ad799x.o
>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>> +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>> obj-$(CONFIG_MAX1027) += max1027.o
>> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
>> new file mode 100644
>> index 0000000..333c624
>> --- /dev/null
>> +++ b/drivers/iio/adc/axp288_adc.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
>> + *
>> + * Copyright (C) 2014 Intel Corporation
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/axp20x.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/driver.h>
>> +
>> +#define AXP288_ADC_EN_MASK 0xF1
>> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
>> +#define AXP288_ADC_TS_PIN_ON 0xF3
>> +
>> +enum axp288_adc_id {
>> + AXP288_ADC_TS,
>> + AXP288_ADC_PMIC,
>> + AXP288_ADC_GP,
>> + AXP288_ADC_BATT_CHRG_I,
>> + AXP288_ADC_BATT_DISCHRG_I,
>> + AXP288_ADC_BATT_V,
>> + AXP288_ADC_NR_CHAN,
>> +};
>> +
>> +struct axp288_adc_info {
>> + int irq;
>> + struct device *dev;
>> + struct regmap *regmap;
>> +};
>> +
>> +static const struct iio_chan_spec const axp288_adc_channels[] = {
>> + {
>> + .indexed = 1,
>> + .type = IIO_TEMP,
>> + .channel = 0,
>> + .address = AXP288_TS_ADC_H,
>> + .datasheet_name = "CH0",
>> + }, {
>> + .indexed = 1,
>> + .type = IIO_TEMP,
>> + .channel = 1,
>> + .address = AXP288_PMIC_ADC_H,
>> + .datasheet_name = "CH1",
>> + }, {
>> + .indexed = 1,
>> + .type = IIO_TEMP,
>> + .channel = 2,
>> + .address = AXP288_GP_ADC_H,
>> + .datasheet_name = "CH2",
>> + }, {
>> + .indexed = 1,
>> + .type = IIO_CURRENT,
>> + .channel = 3,
>> + .address = AXP20X_BATT_CHRG_I_H,
>> + .datasheet_name = "CH3",
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> + }, {
>> + .indexed = 1,
>> + .type = IIO_CURRENT,
>> + .channel = 4,
>> + .address = AXP20X_BATT_DISCHRG_I_H,
>> + .datasheet_name = "CH4",
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> + }, {
>> + .indexed = 1,
>> + .type = IIO_VOLTAGE,
>> + .channel = 5,
>> + .address = AXP20X_BATT_V_H,
>> + .datasheet_name = "CH5",
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> + },
>> +};
>> +
>> +#define AXP288_ADC_MAP(_adc_channel_label, _consumer_dev_name, \
>> + _consumer_channel) \
>> + { \
>> + .adc_channel_label = _adc_channel_label, \
>> + .consumer_dev_name = _consumer_dev_name, \
>> + .consumer_channel = _consumer_channel, \
>> + }
>> +
>> +/* for consumer drivers */
>> +static struct iio_map axp288_adc_default_maps[] = {
>> + AXP288_ADC_MAP("TS_PIN", "axp288-batt", "axp288-batt-temp"),
>> + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic", "axp288-pmic-temp"),
>> + AXP288_ADC_MAP("GPADC", "axp288-gpadc", "axp288-system-temp"),
>> + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg", "axp288-chrg-curr"),
>> + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg", "axp288-chrg-d-curr"),
>> + AXP288_ADC_MAP("BATT_V", "axp288-batt", "axp288-batt-volt"),
>> + {},
>> +};
>> +
>> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + int ret;
>> + struct axp288_adc_info *info = iio_priv(indio_dev);
>> + u8 buf[2];
> You could use __be16 instead. Then you just need to use be16_to_cpu(), shift 4 bits to the right and mask the result with 0xFF.
On second thought, there is not even a need for masking (was not clear to me before, that the device is 12 bit - my bad).
>> +
>> + mutex_lock(&indio_dev->mlock);
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + /* special case for GPADC sample */
>> + if (chan->address == AXP288_GP_ADC_H) {
>> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
>> + AXP288_ADC_TS_PIN_GPADC);
>> + if (ret) {
>> + dev_err(&indio_dev->dev, "GPADC mode\n");
>> + mutex_unlock(&indio_dev->mlock);
>> + return ret;
> You may even just goto the mutex_unlock at the end of the function. Would safe a few bytes here in the source - up to you. More interesting is, if the error message is required, as you don't have one below, when switching to TS_PIN_ON.
>> + }
>> + }
>> + case IIO_CHAN_INFO_PROCESSED:
>> + ret = regmap_bulk_read(info->regmap, chan->address, buf, 2);
>> + if (ret) {
>> + dev_err(&indio_dev->dev, "sample raw data failed\n");
>> + break;
>> + }
>> + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
>> + ret = IIO_VAL_INT;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + if (mask == IIO_CHAN_INFO_RAW && chan->address == AXP288_GP_ADC_H)
>> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
>> + AXP288_ADC_TS_PIN_ON);
>> +
>> + mutex_unlock(&indio_dev->mlock);
>> +
>> + return ret;
>> +}
>> +
>> +static int axp288_adc_enable(struct regmap *regmap, bool enable)
>> +{
>> + unsigned int pin_on, adc_en;
>> +
>> + if (enable) {
>> + pin_on = AXP288_ADC_TS_PIN_ON;
>> + adc_en = AXP288_ADC_EN_MASK;
>> + } else {
>> + pin_on = ~AXP288_ADC_TS_PIN_ON;
>> + adc_en = ~AXP288_ADC_EN_MASK;
>> + }
>> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
>> + return -EIO;
>> +
>> + return regmap_write(regmap, AXP20X_ADC_EN1, ~AXP288_ADC_EN_MASK);
> So, what's the deal about the variable adc_en?
>> +}
>> +
>> +static const struct iio_info axp288_adc_iio_info = {
>> + .read_raw = &axp288_adc_read_raw,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int axp288_adc_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct axp288_adc_info *info;
>> + struct iio_dev *indio_dev;
>> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + info = iio_priv(indio_dev);
>> + info->irq = platform_get_irq(pdev, 0);
>> + if (info->irq < 0) {
>> + dev_err(&pdev->dev, "no irq resource?\n");
>> + return info->irq;
>> + }
>> + platform_set_drvdata(pdev, indio_dev);
>> + info->regmap = axp20x->regmap;
>> + ret = axp288_adc_enable(axp20x->regmap, true);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Unable to enable ADC device\n");
>> + return ret;
>> + }
>> +
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->name = pdev->name;
>> + indio_dev->channels = axp288_adc_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
>> + indio_dev->info = &axp288_adc_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + ret = iio_map_array_register(indio_dev, axp288_adc_default_maps);
>> + if (ret < 0)
>> + goto err_disable_dev;
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "unable to register iio device\n");
>> + goto err_array_unregister;
>> + }
>> +
>> + return 0;
>> +
>> +err_array_unregister:
>> + iio_map_array_unregister(indio_dev);
>> +err_disable_dev:
>> + ret = axp288_adc_enable(axp20x->regmap, false);
> Don't set ret here. We want to return the error code from the function that failed above.
>> +
>> + return ret;
>> +}
>> +
>> +static int axp288_adc_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +
>> + if (axp288_adc_enable(axp20x->regmap, false))
>> + dev_err(&pdev->dev, "Unable to disable ADC device\n");
>> + iio_device_unregister(indio_dev);
>> + iio_map_array_unregister(indio_dev);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver axp288_adc_driver = {
>> + .probe = axp288_adc_probe,
>> + .remove = axp288_adc_remove,
>> + .driver = {
>> + .name = "axp288_adc",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +module_platform_driver(axp288_adc_driver);
>> +
>> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
>> +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
>> +MODULE_LICENSE("GPL");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 22:20 ` Hartmut Knaack
2014-09-17 22:30 ` Hartmut Knaack
@ 2014-09-17 23:13 ` Jacob Pan
2014-09-18 21:52 ` Hartmut Knaack
1 sibling, 1 reply; 17+ messages in thread
From: Jacob Pan @ 2014-09-17 23:13 UTC (permalink / raw)
To: Hartmut Knaack
Cc: IIO, LKML, DEVICE TREE, Lee Jones, Carlo Caione,
Srinivas Pandruvada, Aaron Lu, Alan Cox, Samuel Ortiz,
Liam Girdwood, Mark Brown, Grant Likely, Greg Kroah-Hartman,
Rob Herring, Lars-Peter Clausen, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Thu, 18 Sep 2014 00:20:00 +0200
Hartmut Knaack <knaack.h@gmx.de> wrote:
> Jacob Pan schrieb, Am 17.09.2014 02:11:
> > Platform driver for X-Powers AXP288 ADC, which is a sub-device of
> > the customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC
> > device enumerates as one of the MFD cell devices. It uses IIO
> > infrastructure to communicate with userspace and consumer drivers.
> >
> > Usages of ADC channels include battery charging and thermal sensors.
> >
> You are progressing, but still a few issues left. One thing is
> indentation in some places (I feel like being pretty annoying about
> this), others are commented in line.
I don't see the indentation issue, perhaps it is the difference in the
editors we use?
> > Based on initial work by:
> > Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> >
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> > drivers/iio/adc/Kconfig | 8 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/axp288_adc.c | 254
> > +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 263
> > insertions(+) create mode 100644 drivers/iio/adc/axp288_adc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 11b048a..db2681b 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -127,6 +127,14 @@ config AT91_ADC
> > help
> > Say yes here to build support for Atmel AT91 ADC.
> >
> > +config AXP288_ADC
> > + tristate "X-Powers AXP288 ADC driver"
> > + depends on MFD_AXP20X
> > + help
> > + Say yes here to have support for X-Powers power
> > management IC (PMIC) ADC
> > + device. Depending on platform configuration, this
> > general purpose ADC can
> > + be used for sampling sensors such as thermal resistors.
> > +
> > config EXYNOS_ADC
> > tristate "Exynos ADC driver support"
> > depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index ad81b51..19640f9 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> > obj-$(CONFIG_AD7887) += ad7887.o
> > obj-$(CONFIG_AD799X) += ad799x.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > diff --git a/drivers/iio/adc/axp288_adc.c
> > b/drivers/iio/adc/axp288_adc.c new file mode 100644
> > index 0000000..333c624
> > --- /dev/null
> > +++ b/drivers/iio/adc/axp288_adc.c
> > @@ -0,0 +1,254 @@
> > +/*
> > + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + *
> > + *
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/axp20x.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/machine.h>
> > +#include <linux/iio/driver.h>
> > +
> > +#define AXP288_ADC_EN_MASK 0xF1
> > +#define AXP288_ADC_TS_PIN_GPADC 0xF2
> > +#define AXP288_ADC_TS_PIN_ON 0xF3
> > +
> > +enum axp288_adc_id {
> > + AXP288_ADC_TS,
> > + AXP288_ADC_PMIC,
> > + AXP288_ADC_GP,
> > + AXP288_ADC_BATT_CHRG_I,
> > + AXP288_ADC_BATT_DISCHRG_I,
> > + AXP288_ADC_BATT_V,
> > + AXP288_ADC_NR_CHAN,
> > +};
> > +
> > +struct axp288_adc_info {
> > + int irq;
> > + struct device *dev;
> > + struct regmap *regmap;
> > +};
> > +
> > +static const struct iio_chan_spec const axp288_adc_channels[] = {
> > + {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 0,
> > + .address = AXP288_TS_ADC_H,
> > + .datasheet_name = "CH0",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 1,
> > + .address = AXP288_PMIC_ADC_H,
> > + .datasheet_name = "CH1",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 2,
> > + .address = AXP288_GP_ADC_H,
> > + .datasheet_name = "CH2",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_CURRENT,
> > + .channel = 3,
> > + .address = AXP20X_BATT_CHRG_I_H,
> > + .datasheet_name = "CH3",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_CURRENT,
> > + .channel = 4,
> > + .address = AXP20X_BATT_DISCHRG_I_H,
> > + .datasheet_name = "CH4",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_VOLTAGE,
> > + .channel = 5,
> > + .address = AXP20X_BATT_V_H,
> > + .datasheet_name = "CH5",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + },
> > +};
> > +
> > +#define AXP288_ADC_MAP(_adc_channel_label,
> > _consumer_dev_name, \
> > + _consumer_channel) \
> > + { \
> > + .adc_channel_label = _adc_channel_label, \
> > + .consumer_dev_name = _consumer_dev_name, \
> > + .consumer_channel =
> > _consumer_channel, \
> > + }
> > +
> > +/* for consumer drivers */
> > +static struct iio_map axp288_adc_default_maps[] = {
> > + AXP288_ADC_MAP("TS_PIN", "axp288-batt",
> > "axp288-batt-temp"),
> > + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic",
> > "axp288-pmic-temp"),
> > + AXP288_ADC_MAP("GPADC", "axp288-gpadc",
> > "axp288-system-temp"),
> > + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg",
> > "axp288-chrg-curr"),
> > + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg",
> > "axp288-chrg-d-curr"),
> > + AXP288_ADC_MAP("BATT_V", "axp288-batt",
> > "axp288-batt-volt"),
> > + {},
> > +};
> > +
> > +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int ret;
> > + struct axp288_adc_info *info = iio_priv(indio_dev);
> > + u8 buf[2];
> You could use __be16 instead. Then you just need to use
> be16_to_cpu(), shift 4 bits to the right and mask the result with
> 0xFF.
> > +
> > + mutex_lock(&indio_dev->mlock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + /* special case for GPADC sample */
> > + if (chan->address == AXP288_GP_ADC_H) {
> > + ret = regmap_write(info->regmap,
> > AXP288_ADC_TS_PIN_CTRL,
> > + AXP288_ADC_TS_PIN_GPADC);
> > + if (ret) {
> > + dev_err(&indio_dev->dev, "GPADC
> > mode\n");
> > + mutex_unlock(&indio_dev->mlock);
> > + return ret;
> You may even just goto the mutex_unlock at the end of the function.
> Would safe a few bytes here in the source - up to you. More
> interesting is, if the error message is required, as you don't have
> one below, when switching to TS_PIN_ON.
i don't have preference, the previous reviews don't like goto.
> > + }
> > + }
> > + case IIO_CHAN_INFO_PROCESSED:
> > + ret = regmap_bulk_read(info->regmap,
> > chan->address, buf, 2);
> > + if (ret) {
> > + dev_err(&indio_dev->dev, "sample raw data
> > failed\n");
> > + break;
> > + }
> > + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> > + ret = IIO_VAL_INT;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> > + if (mask == IIO_CHAN_INFO_RAW && chan->address ==
> > AXP288_GP_ADC_H)
> > + ret = regmap_write(info->regmap,
> > AXP288_ADC_TS_PIN_CTRL,
> > + AXP288_ADC_TS_PIN_ON);
> > +
> > + mutex_unlock(&indio_dev->mlock);
> > +
> > + return ret;
> > +}
> > +
> > +static int axp288_adc_enable(struct regmap *regmap, bool enable)
> > +{
> > + unsigned int pin_on, adc_en;
> > +
> > + if (enable) {
> > + pin_on = AXP288_ADC_TS_PIN_ON;
> > + adc_en = AXP288_ADC_EN_MASK;
> > + } else {
> > + pin_on = ~AXP288_ADC_TS_PIN_ON;
> > + adc_en = ~AXP288_ADC_EN_MASK;
> > + }
> > + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
> > + return -EIO;
> > +
> > + return regmap_write(regmap, AXP20X_ADC_EN1,
> > ~AXP288_ADC_EN_MASK);
> So, what's the deal about the variable adc_en?
well, I am trying to be consistent with the data sheet. in axp202
datasheet, register 82h is named "ADC Enable 1". In axp288 datasheet,
the same register 82h is named ADC Enable. I am going with adc_en1 to
better scale for more "ADC Enable X".
> > +}
> > +
> > +static const struct iio_info axp288_adc_iio_info = {
> > + .read_raw = &axp288_adc_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int axp288_adc_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct axp288_adc_info *info;
> > + struct iio_dev *indio_dev;
> > + struct axp20x_dev *axp20x =
> > dev_get_drvdata(pdev->dev.parent); +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev,
> > sizeof(*info));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + info = iio_priv(indio_dev);
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(&pdev->dev, "no irq resource?\n");
> > + return info->irq;
> > + }
> > + platform_set_drvdata(pdev, indio_dev);
> > + info->regmap = axp20x->regmap;
> > + ret = axp288_adc_enable(axp20x->regmap, true);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to enable ADC
> > device\n");
> > + return ret;
> > + }
> > +
> > + indio_dev->dev.parent = &pdev->dev;
> > + indio_dev->name = pdev->name;
> > + indio_dev->channels = axp288_adc_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
> > + indio_dev->info = &axp288_adc_iio_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + ret = iio_map_array_register(indio_dev,
> > axp288_adc_default_maps);
> > + if (ret < 0)
> > + goto err_disable_dev;
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "unable to register iio
> > device\n");
> > + goto err_array_unregister;
> > + }
> > +
> > + return 0;
> > +
> > +err_array_unregister:
> > + iio_map_array_unregister(indio_dev);
> > +err_disable_dev:
> > + ret = axp288_adc_enable(axp20x->regmap, false);
> Don't set ret here. We want to return the error code from the
> function that failed above.
> > +
good point. thanks for the review.
> > + return ret;
> > +}
> > +
> > +static int axp288_adc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct axp20x_dev *axp20x =
> > dev_get_drvdata(pdev->dev.parent); +
> > + if (axp288_adc_enable(axp20x->regmap, false))
> > + dev_err(&pdev->dev, "Unable to disable ADC
> > device\n");
> > + iio_device_unregister(indio_dev);
> > + iio_map_array_unregister(indio_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver axp288_adc_driver = {
> > + .probe = axp288_adc_probe,
> > + .remove = axp288_adc_remove,
> > + .driver = {
> > + .name = "axp288_adc",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +module_platform_driver(axp288_adc_driver);
> > +
> > +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
> > +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
> > +MODULE_LICENSE("GPL");
> >
>
[Jacob Pan]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
@ 2014-09-18 5:39 ` Lee Jones
2014-09-18 12:32 ` Jacob Pan
2014-09-21 13:01 ` Jonathan Cameron
1 sibling, 1 reply; 17+ messages in thread
From: Lee Jones @ 2014-09-18 5:39 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, DEVICE TREE, Carlo Caione, Srinivas Pandruvada,
Aaron Lu, Alan Cox, Jean Delvare, Samuel Ortiz, Liam Girdwood,
Mark Brown, Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Tue, 16 Sep 2014, Jacob Pan wrote:
> X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR platforms. Similar
> to AXP202/209, AXP288 comes with USB charger, more LDO and BUCK channels, and
> AD converters. It also provides extended status and interrupt reporting
> capabilities than the devices currently supported in axp20x.c.
>
> In addition to feature extension, this patch also adds ACPI binding for
> enumeration.
>
> This consolidated driver should support more X-Powers' PMICs in both device
> tree and ACPI enumerated platforms.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
> drivers/mfd/Kconfig | 3 +-
> drivers/mfd/axp20x.c | 353 +++++++++++++++++++++++++++++++++++++--------
> include/linux/mfd/axp20x.h | 58 ++++++++
> 3 files changed, 354 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..c183edb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -74,7 +74,8 @@ config MFD_AXP20X
> select REGMAP_IRQ
> depends on I2C=y
> help
> - If you say Y here you get support for the X-Powers AXP202 and AXP209.
> + If you say Y here you get support for the X-Powers AXP202, AXP209 and
> + AXP288 power management IC (PMIC).
> This driver include only the core APIs. You have to select individual
> components like regulators or the PEK (Power Enable Key) under the
> corresponding menus.
[...]
> -static const struct regmap_config axp20x_regmap_config = {
[...]
> +static struct regmap_config axp20x_regmap_config = {
Why have you removed const status?
> .reg_bits = 8,
> .val_bits = 8,
> .wr_table = &axp20x_writeable_table,
> @@ -70,47 +129,96 @@ static const struct regmap_config axp20x_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> -#define AXP20X_IRQ(_irq, _off, _mask) \
> - [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
> +static struct regmap_config axp288_regmap_config = {
const?
> + .reg_bits = 8,
> + .val_bits = 8,
> + .wr_table = &axp288_writeable_table,
> + .volatile_table = &axp288_volatile_table,
> + .max_register = AXP288_FG_TUNE5,
> + .cache_type = REGCACHE_RBTREE,
> +};
[...]
> -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> +static struct acpi_device_id axp20x_acpi_match[] = {
> + {
> + .id = "INT33F4",
> + .driver_data = (kernel_ulong_t)AXP288_ID,
Why do you need to cast this?
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
> +
> +/* common irq chip attributes only */
> +static struct regmap_irq_chip axp20x_regmap_irq_chip = {
> .name = "axp20x_irq_chip",
> .status_base = AXP20X_IRQ1_STATE,
> .ack_base = AXP20X_IRQ1_STATE,
> .mask_base = AXP20X_IRQ1_EN,
> - .num_regs = 5,
> - .irqs = axp20x_regmap_irqs,
> - .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs),
> .mask_invert = true,
> .init_ack_masked = true,
> };
> @@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
> },
> };
[...]
> +static int axp20x_match_device(struct axp20x_dev *axp20x, struct device *dev)
> +{
> + const struct acpi_device_id *acpi_id;
> + const struct of_device_id *of_id;
> +
> + of_id = of_match_device(axp20x_of_match, dev);
> + if (of_id)
> + axp20x->variant = (long) of_id->data;
> + else {
> + acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!acpi_id || !acpi_id->driver_data) {
> + dev_err(dev, "Unable to determine ID\n");
> + return -ENODEV;
> + }
> + axp20x->variant = (long) acpi_id->driver_data;
> + }
We can do better error handling here and give the user a better sense
of what happened if anything were to go wrong. Do:
if (dev->of_node)
of_id = of_match_device()
if (!of_id)
error()
variant = of_id->data;
else
acpi_id = acpi_match_device()
if (!acpi_id)
error()
variant = acpi_id->driver_data;
> + switch (axp20x->variant) {
> + case AXP202_ID:
> + case AXP209_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> + axp20x->cells = axp20x_cells;
> + axp20x->regmap_cfg = &axp20x_regmap_config;
> + axp20x_regmap_irq_chip.num_regs = 5;
> + axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp20x_regmap_irqs);
> + break;
> + case AXP288_ID:
> + axp20x->cells = axp288_cells;
> + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> + axp20x->regmap_cfg = &axp288_regmap_config;
> + axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp288_regmap_irqs);
> + axp20x_regmap_irq_chip.num_regs = 6;
> + break;
> + default:
> + dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> + return -ENODEV;
-EINVAL might be better here.
> + }
> + dev_info(dev, "AXP20x variant %s found\n",
> + axp20x_model_names[axp20x->variant]);
> +
> + return 0;
> +}
> +
> static int axp20x_i2c_probe(struct i2c_client *i2c,
> - const struct i2c_device_id *id)
> + const struct i2c_device_id *id)
Sneaky. ;)
> {
> struct axp20x_dev *axp20x;
> - const struct of_device_id *of_id;
> int ret;
>
> axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
> if (!axp20x)
> return -ENOMEM;
>
> - of_id = of_match_device(axp20x_of_match, &i2c->dev);
> - if (!of_id) {
> - dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
> - return -ENODEV;
> - }
> - axp20x->variant = (long) of_id->data;
> + ret = axp20x_match_device(axp20x, &i2c->dev);
> + if (ret)
> + return ret;
>
> axp20x->i2c_client = i2c;
> axp20x->dev = &i2c->dev;
> dev_set_drvdata(axp20x->dev, axp20x);
>
> - axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
> + axp20x->regmap = devm_regmap_init_i2c(i2c, axp20x->regmap_cfg);
> if (IS_ERR(axp20x->regmap)) {
> ret = PTR_ERR(axp20x->regmap);
> dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
> @@ -206,8 +440,8 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
> return ret;
> }
>
> - ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> - ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
> + ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> + axp20x->nr_cells, NULL, 0, NULL);
>
> if (ret) {
> dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
[...]
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index d0e31a2..ffc69e2 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
[...]
> struct axp20x_dev {
> struct device *dev;
> struct i2c_client *i2c_client;
> struct regmap *regmap;
> struct regmap_irq_chip_data *regmap_irqc;
> long variant;
> + int nr_cells;
> + struct mfd_cell *cells;
> + struct regmap_config *regmap_cfg;
const?
> };
>
> #endif /* __LINUX_MFD_AXP20X_H */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-18 5:39 ` Lee Jones
@ 2014-09-18 12:32 ` Jacob Pan
2014-09-24 11:02 ` Lee Jones
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Pan @ 2014-09-18 12:32 UTC (permalink / raw)
To: Lee Jones
Cc: IIO, LKML, DEVICE TREE, Carlo Caione, Srinivas Pandruvada,
Aaron Lu, Alan Cox, Jean Delvare, Samuel Ortiz, Liam Girdwood,
Mark Brown, Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Wed, 17 Sep 2014 22:39:26 -0700
Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 16 Sep 2014, Jacob Pan wrote:
>
> > X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR
> > platforms. Similar to AXP202/209, AXP288 comes with USB charger,
> > more LDO and BUCK channels, and AD converters. It also provides
> > extended status and interrupt reporting capabilities than the
> > devices currently supported in axp20x.c.
> >
> > In addition to feature extension, this patch also adds ACPI binding
> > for enumeration.
> >
> > This consolidated driver should support more X-Powers' PMICs in
> > both device tree and ACPI enumerated platforms.
> >
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> > drivers/mfd/Kconfig | 3 +-
> > drivers/mfd/axp20x.c | 353
> > +++++++++++++++++++++++++++++++++++++--------
> > include/linux/mfd/axp20x.h | 58 ++++++++ 3 files changed, 354
> > insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index de5abf2..c183edb 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -74,7 +74,8 @@ config MFD_AXP20X
> > select REGMAP_IRQ
> > depends on I2C=y
> > help
> > - If you say Y here you get support for the X-Powers
> > AXP202 and AXP209.
> > + If you say Y here you get support for the X-Powers
> > AXP202, AXP209 and
> > + AXP288 power management IC (PMIC).
> > This driver include only the core APIs. You have to
> > select individual components like regulators or the PEK (Power
> > Enable Key) under the corresponding menus.
>
> [...]
>
> > -static const struct regmap_config axp20x_regmap_config = {
>
> [...]
>
> > +static struct regmap_config axp20x_regmap_config = {
>
> Why have you removed const status?
I did not use const * in axp20x_dev, let me add that. good point.
>
> > .reg_bits = 8,
> > .val_bits = 8,
> > .wr_table = &axp20x_writeable_table,
> > @@ -70,47 +129,96 @@ static const struct regmap_config
> > axp20x_regmap_config = { .cache_type = REGCACHE_RBTREE,
> > };
> >
> > -#define AXP20X_IRQ(_irq, _off, _mask) \
> > - [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask =
> > BIT(_mask) } +static struct regmap_config axp288_regmap_config = {
>
> const?
>
ditto
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .wr_table = &axp288_writeable_table,
> > + .volatile_table = &axp288_volatile_table,
> > + .max_register = AXP288_FG_TUNE5,
> > + .cache_type = REGCACHE_RBTREE,
> > +};
>
> [...]
>
> > -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> > +static struct acpi_device_id axp20x_acpi_match[] = {
> > + {
> > + .id = "INT33F4",
> > + .driver_data = (kernel_ulong_t)AXP288_ID,
>
> Why do you need to cast this?
>
to make sure match driver_data which is different in acpi_device_id than
of_device_id.
> > + },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
> > +
> > +/* common irq chip attributes only */
> > +static struct regmap_irq_chip axp20x_regmap_irq_chip = {
> > .name = "axp20x_irq_chip",
> > .status_base = AXP20X_IRQ1_STATE,
> > .ack_base = AXP20X_IRQ1_STATE,
> > .mask_base = AXP20X_IRQ1_EN,
> > - .num_regs = 5,
> > - .irqs = axp20x_regmap_irqs,
> > - .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs),
> > .mask_invert = true,
> > .init_ack_masked = true,
> > };
> > @@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
> > },
> > };
>
> [...]
>
> > +static int axp20x_match_device(struct axp20x_dev *axp20x, struct
> > device *dev) +{
> > + const struct acpi_device_id *acpi_id;
> > + const struct of_device_id *of_id;
> > +
> > + of_id = of_match_device(axp20x_of_match, dev);
> > + if (of_id)
> > + axp20x->variant = (long) of_id->data;
> > + else {
> > + acpi_id =
> > acpi_match_device(dev->driver->acpi_match_table, dev);
> > + if (!acpi_id || !acpi_id->driver_data) {
> > + dev_err(dev, "Unable to determine ID\n");
> > + return -ENODEV;
> > + }
> > + axp20x->variant = (long) acpi_id->driver_data;
> > + }
>
> We can do better error handling here and give the user a better sense
> of what happened if anything were to go wrong. Do:
>
> if (dev->of_node)
> of_id = of_match_device()
> if (!of_id)
> error()
this will give false error on ACPI based platforms, right? in reality
this driver will only run on ACPI or OF platform not both, so IMHO
there is not need to give user a specific error since they already know
what platform I am running.
> variant = of_id->data;
> else
> acpi_id = acpi_match_device()
> if (!acpi_id)
> error()
> variant = acpi_id->driver_data;
>
> > + switch (axp20x->variant) {
> > + case AXP202_ID:
> > + case AXP209_ID:
> > + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> > + axp20x->cells = axp20x_cells;
> > + axp20x->regmap_cfg = &axp20x_regmap_config;
> > + axp20x_regmap_irq_chip.num_regs = 5;
> > + axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> > + axp20x_regmap_irq_chip.num_irqs =
> > + ARRAY_SIZE(axp20x_regmap_irqs);
> > + break;
> > + case AXP288_ID:
> > + axp20x->cells = axp288_cells;
> > + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> > + axp20x->regmap_cfg = &axp288_regmap_config;
> > + axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> > + axp20x_regmap_irq_chip.num_irqs =
> > + ARRAY_SIZE(axp288_regmap_irqs);
> > + axp20x_regmap_irq_chip.num_regs = 6;
> > + break;
> > + default:
> > + dev_err(dev, "unsupported AXP20X ID %lu\n",
> > axp20x->variant);
> > + return -ENODEV;
>
> -EINVAL might be better here.
I was considering the return value gets propagated to probe function
which is used to query the existence of a device per driver model. But
I have no strong preference.
>
> > + }
> > + dev_info(dev, "AXP20x variant %s found\n",
> > + axp20x_model_names[axp20x->variant]);
> > +
> > + return 0;
> > +}
> > +
> > static int axp20x_i2c_probe(struct i2c_client *i2c,
> > - const struct i2c_device_id *id)
> > + const struct i2c_device_id *id)
>
> Sneaky. ;)
I should not fix the extra white spaces here, unrelated to this patch.
will remove.
>
> > {
> > struct axp20x_dev *axp20x;
> > - const struct of_device_id *of_id;
> > int ret;
> >
> > axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x),
> > GFP_KERNEL); if (!axp20x)
> > return -ENOMEM;
> >
> > - of_id = of_match_device(axp20x_of_match, &i2c->dev);
> > - if (!of_id) {
> > - dev_err(&i2c->dev, "Unable to setup AXP20X
> > data\n");
> > - return -ENODEV;
> > - }
> > - axp20x->variant = (long) of_id->data;
> > + ret = axp20x_match_device(axp20x, &i2c->dev);
> > + if (ret)
> > + return ret;
> >
> > axp20x->i2c_client = i2c;
> > axp20x->dev = &i2c->dev;
> > dev_set_drvdata(axp20x->dev, axp20x);
> >
> > - axp20x->regmap = devm_regmap_init_i2c(i2c,
> > &axp20x_regmap_config);
> > + axp20x->regmap = devm_regmap_init_i2c(i2c,
> > axp20x->regmap_cfg); if (IS_ERR(axp20x->regmap)) {
> > ret = PTR_ERR(axp20x->regmap);
> > dev_err(&i2c->dev, "regmap init failed: %d\n",
> > ret); @@ -206,8 +440,8 @@ static int axp20x_i2c_probe(struct
> > i2c_client *i2c, return ret;
> > }
> >
> > - ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> > - ARRAY_SIZE(axp20x_cells), NULL, 0,
> > NULL);
> > + ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> > + axp20x->nr_cells, NULL, 0, NULL);
> >
> > if (ret) {
> > dev_err(&i2c->dev, "failed to add MFD devices:
> > %d\n", ret);
>
> [...]
>
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index d0e31a2..ffc69e2 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
>
> [...]
>
> > struct axp20x_dev {
> > struct device *dev;
> > struct i2c_client *i2c_client;
> > struct regmap *regmap;
> > struct regmap_irq_chip_data *regmap_irqc;
> > long variant;
> > + int nr_cells;
> > + struct mfd_cell *cells;
> > + struct regmap_config *regmap_cfg;
>
> const?
>
yes
> > };
> >
> > #endif /* __LINUX_MFD_AXP20X_H */
>
thanks for the review.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 23:13 ` Jacob Pan
@ 2014-09-18 21:52 ` Hartmut Knaack
2014-09-18 22:35 ` Jacob Pan
0 siblings, 1 reply; 17+ messages in thread
From: Hartmut Knaack @ 2014-09-18 21:52 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, DEVICE TREE, Lee Jones, Carlo Caione,
Srinivas Pandruvada, Aaron Lu, Alan Cox, Samuel Ortiz,
Liam Girdwood, Mark Brown, Grant Likely, Greg Kroah-Hartman,
Rob Herring, Lars-Peter Clausen, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
Jacob Pan schrieb, Am 18.09.2014 01:13:
> On Thu, 18 Sep 2014 00:20:00 +0200
> Hartmut Knaack <knaack.h@gmx.de> wrote:
>
>> Jacob Pan schrieb, Am 17.09.2014 02:11:
>>> Platform driver for X-Powers AXP288 ADC, which is a sub-device of
>>> the customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC
>>> device enumerates as one of the MFD cell devices. It uses IIO
>>> infrastructure to communicate with userspace and consumer drivers.
>>>
>>> Usages of ADC channels include battery charging and thermal sensors.
>>>
>> You are progressing, but still a few issues left. One thing is
>> indentation in some places (I feel like being pretty annoying about
>> this), others are commented in line.
> I don't see the indentation issue, perhaps it is the difference in the
> editors we use?
>
>>> Based on initial work by:
>>> Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>>
>>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>> ---
>>> drivers/iio/adc/Kconfig | 8 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/axp288_adc.c | 254
>>> +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 263
>>> insertions(+) create mode 100644 drivers/iio/adc/axp288_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 11b048a..db2681b 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -127,6 +127,14 @@ config AT91_ADC
>>> help
>>> Say yes here to build support for Atmel AT91 ADC.
>>>
>>> +config AXP288_ADC
>>> + tristate "X-Powers AXP288 ADC driver"
>>> + depends on MFD_AXP20X
>>> + help
>>> + Say yes here to have support for X-Powers power
>>> management IC (PMIC) ADC
>>> + device. Depending on platform configuration, this
>>> general purpose ADC can
>>> + be used for sampling sensors such as thermal resistors.
>>> +
>>> config EXYNOS_ADC
>>> tristate "Exynos ADC driver support"
>>> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index ad81b51..19640f9 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>>> obj-$(CONFIG_AD7887) += ad7887.o
>>> obj-$(CONFIG_AD799X) += ad799x.o
>>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>>> +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>> obj-$(CONFIG_MAX1027) += max1027.o
>>> diff --git a/drivers/iio/adc/axp288_adc.c
>>> b/drivers/iio/adc/axp288_adc.c new file mode 100644
>>> index 0000000..333c624
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/axp288_adc.c
>>> @@ -0,0 +1,254 @@
>>> +/*
>>> + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
>>> + *
>>> + * Copyright (C) 2014 Intel Corporation
>>> + *
>>> + *
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License as
>>> published by
>>> + * the Free Software Foundation; version 2 of the License.
>>> + *
>>> + * 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.
>>> + *
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/axp20x.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>>> +#include <linux/iio/driver.h>
>>> +
>>> +#define AXP288_ADC_EN_MASK 0xF1
>>> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
>>> +#define AXP288_ADC_TS_PIN_ON 0xF3
>>> +
>>> +enum axp288_adc_id {
>>> + AXP288_ADC_TS,
>>> + AXP288_ADC_PMIC,
>>> + AXP288_ADC_GP,
>>> + AXP288_ADC_BATT_CHRG_I,
>>> + AXP288_ADC_BATT_DISCHRG_I,
>>> + AXP288_ADC_BATT_V,
>>> + AXP288_ADC_NR_CHAN,
>>> +};
>>> +
>>> +struct axp288_adc_info {
>>> + int irq;
>>> + struct device *dev;
>>> + struct regmap *regmap;
>>> +};
>>> +
>>> +static const struct iio_chan_spec const axp288_adc_channels[] = {
>>> + {
>>> + .indexed = 1,
>>> + .type = IIO_TEMP,
>>> + .channel = 0,
>>> + .address = AXP288_TS_ADC_H,
>>> + .datasheet_name = "CH0",
>>> + }, {
>>> + .indexed = 1,
>>> + .type = IIO_TEMP,
>>> + .channel = 1,
>>> + .address = AXP288_PMIC_ADC_H,
>>> + .datasheet_name = "CH1",
>>> + }, {
>>> + .indexed = 1,
>>> + .type = IIO_TEMP,
>>> + .channel = 2,
>>> + .address = AXP288_GP_ADC_H,
>>> + .datasheet_name = "CH2",
>>> + }, {
>>> + .indexed = 1,
>>> + .type = IIO_CURRENT,
>>> + .channel = 3,
>>> + .address = AXP20X_BATT_CHRG_I_H,
>>> + .datasheet_name = "CH3",
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + }, {
>>> + .indexed = 1,
>>> + .type = IIO_CURRENT,
>>> + .channel = 4,
>>> + .address = AXP20X_BATT_DISCHRG_I_H,
>>> + .datasheet_name = "CH4",
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + }, {
>>> + .indexed = 1,
>>> + .type = IIO_VOLTAGE,
>>> + .channel = 5,
>>> + .address = AXP20X_BATT_V_H,
>>> + .datasheet_name = "CH5",
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> + },
>>> +};
>>> +
>>> +#define AXP288_ADC_MAP(_adc_channel_label,
>>> _consumer_dev_name, \
>>> + _consumer_channel) \
>>> + { \
>>> + .adc_channel_label = _adc_channel_label, \
>>> + .consumer_dev_name = _consumer_dev_name, \
>>> + .consumer_channel =
>>> _consumer_channel, \
>>> + }
>>> +
>>> +/* for consumer drivers */
>>> +static struct iio_map axp288_adc_default_maps[] = {
>>> + AXP288_ADC_MAP("TS_PIN", "axp288-batt",
>>> "axp288-batt-temp"),
>>> + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic",
>>> "axp288-pmic-temp"),
>>> + AXP288_ADC_MAP("GPADC", "axp288-gpadc",
>>> "axp288-system-temp"),
>>> + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg",
>>> "axp288-chrg-curr"),
>>> + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg",
>>> "axp288-chrg-d-curr"),
>>> + AXP288_ADC_MAP("BATT_V", "axp288-batt",
>>> "axp288-batt-volt"),
>>> + {},
>>> +};
>>> +
>>> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
Here is an indentation issue. See [1] how it can look like (although it is fine to have several parameters in every line).
[1] https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/max1363.c?h=testing#n414
>>> +{
>>> + int ret;
>>> + struct axp288_adc_info *info = iio_priv(indio_dev);
>>> + u8 buf[2];
>> You could use __be16 instead. Then you just need to use
>> be16_to_cpu(), shift 4 bits to the right and mask the result with
>> 0xFF.
>>> +
>>> + mutex_lock(&indio_dev->mlock);
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + /* special case for GPADC sample */
>>> + if (chan->address == AXP288_GP_ADC_H) {
>>> + ret = regmap_write(info->regmap,
>>> AXP288_ADC_TS_PIN_CTRL,
>>> + AXP288_ADC_TS_PIN_GPADC);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "GPADC
>>> mode\n");
>>> + mutex_unlock(&indio_dev->mlock);
>>> + return ret;
>> You may even just goto the mutex_unlock at the end of the function.
>> Would safe a few bytes here in the source - up to you. More
>> interesting is, if the error message is required, as you don't have
>> one below, when switching to TS_PIN_ON.
> i don't have preference, the previous reviews don't like goto.
Yeah, that's probably always a decision between quick exit path vs. low code duplication. They weight pretty equal in this case.
>>> + }
>>> + }
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + ret = regmap_bulk_read(info->regmap,
>>> chan->address, buf, 2);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "sample raw data
>>> failed\n");
>>> + break;
>>> + }
>>> + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
>>> + ret = IIO_VAL_INT;
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + }
>>> +
>>> + if (mask == IIO_CHAN_INFO_RAW && chan->address ==
>>> AXP288_GP_ADC_H)
>>> + ret = regmap_write(info->regmap,
>>> AXP288_ADC_TS_PIN_CTRL,
>>> + AXP288_ADC_TS_PIN_ON);
>>> +
>>> + mutex_unlock(&indio_dev->mlock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int axp288_adc_enable(struct regmap *regmap, bool enable)
>>> +{
>>> + unsigned int pin_on, adc_en;
>>> +
>>> + if (enable) {
>>> + pin_on = AXP288_ADC_TS_PIN_ON;
>>> + adc_en = AXP288_ADC_EN_MASK;
This one, adc_en ^^^^^ is what I meant. It gets set here and below, but not used again.
I would expect, that you intended to write it to AXP20X_ADC_EN1 at the end of this function.
>>> + } else {
>>> + pin_on = ~AXP288_ADC_TS_PIN_ON;
>>> + adc_en = ~AXP288_ADC_EN_MASK;
>>> + }
>>> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
>>> + return -EIO;
>>> +
>>> + return regmap_write(regmap, AXP20X_ADC_EN1,
>>> ~AXP288_ADC_EN_MASK);
>> So, what's the deal about the variable adc_en?
> well, I am trying to be consistent with the data sheet. in axp202
> datasheet, register 82h is named "ADC Enable 1". In axp288 datasheet,
> the same register 82h is named ADC Enable. I am going with adc_en1 to
> better scale for more "ADC Enable X".
>>> +}
>>> +
>>> +static const struct iio_info axp288_adc_iio_info = {
>>> + .read_raw = &axp288_adc_read_raw,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int axp288_adc_probe(struct platform_device *pdev)
>>> +{
>>> + int ret;
>>> + struct axp288_adc_info *info;
>>> + struct iio_dev *indio_dev;
>>> + struct axp20x_dev *axp20x =
>>> dev_get_drvdata(pdev->dev.parent); +
>>> + indio_dev = devm_iio_device_alloc(&pdev->dev,
>>> sizeof(*info));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + info = iio_priv(indio_dev);
>>> + info->irq = platform_get_irq(pdev, 0);
>>> + if (info->irq < 0) {
>>> + dev_err(&pdev->dev, "no irq resource?\n");
>>> + return info->irq;
>>> + }
>>> + platform_set_drvdata(pdev, indio_dev);
>>> + info->regmap = axp20x->regmap;
>>> + ret = axp288_adc_enable(axp20x->regmap, true);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Unable to enable ADC
>>> device\n");
>>> + return ret;
>>> + }
>>> +
>>> + indio_dev->dev.parent = &pdev->dev;
>>> + indio_dev->name = pdev->name;
>>> + indio_dev->channels = axp288_adc_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
>>> + indio_dev->info = &axp288_adc_iio_info;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + ret = iio_map_array_register(indio_dev,
>>> axp288_adc_default_maps);
>>> + if (ret < 0)
>>> + goto err_disable_dev;
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret < 0) {
>>> + dev_err(&pdev->dev, "unable to register iio
>>> device\n");
>>> + goto err_array_unregister;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +err_array_unregister:
>>> + iio_map_array_unregister(indio_dev);
>>> +err_disable_dev:
>>> + ret = axp288_adc_enable(axp20x->regmap, false);
>> Don't set ret here. We want to return the error code from the
>> function that failed above.
>>> +
> good point. thanks for the review.
>
>>> + return ret;
>>> +}
>>> +
>>> +static int axp288_adc_remove(struct platform_device *pdev)
>>> +{
>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> + struct axp20x_dev *axp20x =
>>> dev_get_drvdata(pdev->dev.parent); +
>>> + if (axp288_adc_enable(axp20x->regmap, false))
>>> + dev_err(&pdev->dev, "Unable to disable ADC
>>> device\n");
>>> + iio_device_unregister(indio_dev);
>>> + iio_map_array_unregister(indio_dev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver axp288_adc_driver = {
>>> + .probe = axp288_adc_probe,
>>> + .remove = axp288_adc_remove,
>>> + .driver = {
>>> + .name = "axp288_adc",
>>> + .owner = THIS_MODULE,
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(axp288_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
>>> +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>
> [Jacob Pan]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-18 21:52 ` Hartmut Knaack
@ 2014-09-18 22:35 ` Jacob Pan
0 siblings, 0 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-18 22:35 UTC (permalink / raw)
To: Hartmut Knaack
Cc: IIO, LKML, DEVICE TREE, Lee Jones, Carlo Caione,
Srinivas Pandruvada, Aaron Lu, Alan Cox, Samuel Ortiz,
Liam Girdwood, Mark Brown, Grant Likely, Greg Kroah-Hartman,
Rob Herring, Lars-Peter Clausen, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Thu, 18 Sep 2014 23:52:01 +0200
Hartmut Knaack <knaack.h@gmx.de> wrote:
> Jacob Pan schrieb, Am 18.09.2014 01:13:
> > On Thu, 18 Sep 2014 00:20:00 +0200
> > Hartmut Knaack <knaack.h@gmx.de> wrote:
> >
> >> Jacob Pan schrieb, Am 17.09.2014 02:11:
> >>> Platform driver for X-Powers AXP288 ADC, which is a sub-device of
> >>> the customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC
> >>> device enumerates as one of the MFD cell devices. It uses IIO
> >>> infrastructure to communicate with userspace and consumer drivers.
> >>>
> >>> Usages of ADC channels include battery charging and thermal
> >>> sensors.
> >>>
> >> You are progressing, but still a few issues left. One thing is
> >> indentation in some places (I feel like being pretty annoying about
> >> this), others are commented in line.
> > I don't see the indentation issue, perhaps it is the difference in
> > the editors we use?
> >
> >>> Based on initial work by:
> >>> Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> >>>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >>> ---
> >>> drivers/iio/adc/Kconfig | 8 ++
> >>> drivers/iio/adc/Makefile | 1 +
> >>> drivers/iio/adc/axp288_adc.c | 254
> >>> +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 263
> >>> insertions(+) create mode 100644 drivers/iio/adc/axp288_adc.c
> >>>
> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >>> index 11b048a..db2681b 100644
> >>> --- a/drivers/iio/adc/Kconfig
> >>> +++ b/drivers/iio/adc/Kconfig
> >>> @@ -127,6 +127,14 @@ config AT91_ADC
> >>> help
> >>> Say yes here to build support for Atmel AT91 ADC.
> >>>
> >>> +config AXP288_ADC
> >>> + tristate "X-Powers AXP288 ADC driver"
> >>> + depends on MFD_AXP20X
> >>> + help
> >>> + Say yes here to have support for X-Powers power
> >>> management IC (PMIC) ADC
> >>> + device. Depending on platform configuration, this
> >>> general purpose ADC can
> >>> + be used for sampling sensors such as thermal resistors.
> >>> +
> >>> config EXYNOS_ADC
> >>> tristate "Exynos ADC driver support"
> >>> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> >>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >>> index ad81b51..19640f9 100644
> >>> --- a/drivers/iio/adc/Makefile
> >>> +++ b/drivers/iio/adc/Makefile
> >>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> >>> obj-$(CONFIG_AD7887) += ad7887.o
> >>> obj-$(CONFIG_AD799X) += ad799x.o
> >>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> >>> +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> >>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> >>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> >>> obj-$(CONFIG_MAX1027) += max1027.o
> >>> diff --git a/drivers/iio/adc/axp288_adc.c
> >>> b/drivers/iio/adc/axp288_adc.c new file mode 100644
> >>> index 0000000..333c624
> >>> --- /dev/null
> >>> +++ b/drivers/iio/adc/axp288_adc.c
> >>> @@ -0,0 +1,254 @@
> >>> +/*
> >>> + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
> >>> + *
> >>> + * Copyright (C) 2014 Intel Corporation
> >>> + *
> >>> + *
> >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >>> modify
> >>> + * it under the terms of the GNU General Public License as
> >>> published by
> >>> + * the Free Software Foundation; version 2 of the License.
> >>> + *
> >>> + * 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.
> >>> + *
> >>> + */
> >>> +
> >>> +#include <linux/module.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/device.h>
> >>> +#include <linux/regmap.h>
> >>> +#include <linux/mfd/axp20x.h>
> >>> +#include <linux/platform_device.h>
> >>> +
> >>> +#include <linux/iio/iio.h>
> >>> +#include <linux/iio/machine.h>
> >>> +#include <linux/iio/driver.h>
> >>> +
> >>> +#define AXP288_ADC_EN_MASK 0xF1
> >>> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
> >>> +#define AXP288_ADC_TS_PIN_ON 0xF3
> >>> +
> >>> +enum axp288_adc_id {
> >>> + AXP288_ADC_TS,
> >>> + AXP288_ADC_PMIC,
> >>> + AXP288_ADC_GP,
> >>> + AXP288_ADC_BATT_CHRG_I,
> >>> + AXP288_ADC_BATT_DISCHRG_I,
> >>> + AXP288_ADC_BATT_V,
> >>> + AXP288_ADC_NR_CHAN,
> >>> +};
> >>> +
> >>> +struct axp288_adc_info {
> >>> + int irq;
> >>> + struct device *dev;
> >>> + struct regmap *regmap;
> >>> +};
> >>> +
> >>> +static const struct iio_chan_spec const axp288_adc_channels[] = {
> >>> + {
> >>> + .indexed = 1,
> >>> + .type = IIO_TEMP,
> >>> + .channel = 0,
> >>> + .address = AXP288_TS_ADC_H,
> >>> + .datasheet_name = "CH0",
> >>> + }, {
> >>> + .indexed = 1,
> >>> + .type = IIO_TEMP,
> >>> + .channel = 1,
> >>> + .address = AXP288_PMIC_ADC_H,
> >>> + .datasheet_name = "CH1",
> >>> + }, {
> >>> + .indexed = 1,
> >>> + .type = IIO_TEMP,
> >>> + .channel = 2,
> >>> + .address = AXP288_GP_ADC_H,
> >>> + .datasheet_name = "CH2",
> >>> + }, {
> >>> + .indexed = 1,
> >>> + .type = IIO_CURRENT,
> >>> + .channel = 3,
> >>> + .address = AXP20X_BATT_CHRG_I_H,
> >>> + .datasheet_name = "CH3",
> >>> + .info_mask_separate =
> >>> BIT(IIO_CHAN_INFO_PROCESSED),
> >>> + }, {
> >>> + .indexed = 1,
> >>> + .type = IIO_CURRENT,
> >>> + .channel = 4,
> >>> + .address = AXP20X_BATT_DISCHRG_I_H,
> >>> + .datasheet_name = "CH4",
> >>> + .info_mask_separate =
> >>> BIT(IIO_CHAN_INFO_PROCESSED),
> >>> + }, {
> >>> + .indexed = 1,
> >>> + .type = IIO_VOLTAGE,
> >>> + .channel = 5,
> >>> + .address = AXP20X_BATT_V_H,
> >>> + .datasheet_name = "CH5",
> >>> + .info_mask_separate =
> >>> BIT(IIO_CHAN_INFO_PROCESSED),
> >>> + },
> >>> +};
> >>> +
> >>> +#define AXP288_ADC_MAP(_adc_channel_label,
> >>> _consumer_dev_name, \
> >>> +
> >>> _consumer_channel) \
> >>> + {
> >>> \
> >>> + .adc_channel_label = _adc_channel_label, \
> >>> + .consumer_dev_name = _consumer_dev_name, \
> >>> + .consumer_channel =
> >>> _consumer_channel, \
> >>> + }
> >>> +
> >>> +/* for consumer drivers */
> >>> +static struct iio_map axp288_adc_default_maps[] = {
> >>> + AXP288_ADC_MAP("TS_PIN", "axp288-batt",
> >>> "axp288-batt-temp"),
> >>> + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic",
> >>> "axp288-pmic-temp"),
> >>> + AXP288_ADC_MAP("GPADC", "axp288-gpadc",
> >>> "axp288-system-temp"),
> >>> + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg",
> >>> "axp288-chrg-curr"),
> >>> + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg",
> >>> "axp288-chrg-d-curr"),
> >>> + AXP288_ADC_MAP("BATT_V", "axp288-batt",
> >>> "axp288-batt-volt"),
> >>> + {},
> >>> +};
> >>> +
> >>> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> >>> + struct iio_chan_spec const *chan,
> >>> + int *val, int *val2, long mask)
> Here is an indentation issue. See [1] how it can look like (although
> it is fine to have several parameters in every line).
>
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/adc/max1363.c?h=testing#n414
>
> >>> +{
> >>> + int ret;
> >>> + struct axp288_adc_info *info = iio_priv(indio_dev);
> >>> + u8 buf[2];
> >> You could use __be16 instead. Then you just need to use
> >> be16_to_cpu(), shift 4 bits to the right and mask the result with
> >> 0xFF.
> >>> +
> >>> + mutex_lock(&indio_dev->mlock);
> >>> + switch (mask) {
> >>> + case IIO_CHAN_INFO_RAW:
> >>> + /* special case for GPADC sample */
> >>> + if (chan->address == AXP288_GP_ADC_H) {
> >>> + ret = regmap_write(info->regmap,
> >>> AXP288_ADC_TS_PIN_CTRL,
> >>> + AXP288_ADC_TS_PIN_GPADC);
> >>> + if (ret) {
> >>> + dev_err(&indio_dev->dev, "GPADC
> >>> mode\n");
> >>> + mutex_unlock(&indio_dev->mlock);
> >>> + return ret;
> >> You may even just goto the mutex_unlock at the end of the function.
> >> Would safe a few bytes here in the source - up to you. More
> >> interesting is, if the error message is required, as you don't have
> >> one below, when switching to TS_PIN_ON.
> > i don't have preference, the previous reviews don't like goto.
> Yeah, that's probably always a decision between quick exit path vs.
> low code duplication. They weight pretty equal in this case.
> >>> + }
> >>> + }
> >>> + case IIO_CHAN_INFO_PROCESSED:
> >>> + ret = regmap_bulk_read(info->regmap,
> >>> chan->address, buf, 2);
> >>> + if (ret) {
> >>> + dev_err(&indio_dev->dev, "sample raw data
> >>> failed\n");
> >>> + break;
> >>> + }
> >>> + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> >>> + ret = IIO_VAL_INT;
> >>> + break;
> >>> + default:
> >>> + ret = -EINVAL;
> >>> + }
> >>> +
> >>> + if (mask == IIO_CHAN_INFO_RAW && chan->address ==
> >>> AXP288_GP_ADC_H)
> >>> + ret = regmap_write(info->regmap,
> >>> AXP288_ADC_TS_PIN_CTRL,
> >>> + AXP288_ADC_TS_PIN_ON);
> >>> +
> >>> + mutex_unlock(&indio_dev->mlock);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static int axp288_adc_enable(struct regmap *regmap, bool enable)
> >>> +{
> >>> + unsigned int pin_on, adc_en;
> >>> +
> >>> + if (enable) {
> >>> + pin_on = AXP288_ADC_TS_PIN_ON;
> >>> + adc_en = AXP288_ADC_EN_MASK;
> This one, adc_en ^^^^^ is what I meant. It gets set here and below,
> but not used again. I would expect, that you intended to write it to
> AXP20X_ADC_EN1 at the end of this function.
you are right, my mistake. thanks.
> >>> + } else {
> >>> + pin_on = ~AXP288_ADC_TS_PIN_ON;
> >>> + adc_en = ~AXP288_ADC_EN_MASK;
> >>> + }
> >>> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
> >>> + return -EIO;
> >>> +
> >>> + return regmap_write(regmap, AXP20X_ADC_EN1,
> >>> ~AXP288_ADC_EN_MASK);
> >> So, what's the deal about the variable adc_en?
> > well, I am trying to be consistent with the data sheet. in axp202
> > datasheet, register 82h is named "ADC Enable 1". In axp288
> > datasheet, the same register 82h is named ADC Enable. I am going
> > with adc_en1 to better scale for more "ADC Enable X".
> >>> +}
> >>> +
> >>> +static const struct iio_info axp288_adc_iio_info = {
> >>> + .read_raw = &axp288_adc_read_raw,
> >>> + .driver_module = THIS_MODULE,
> >>> +};
> >>> +
> >>> +static int axp288_adc_probe(struct platform_device *pdev)
> >>> +{
> >>> + int ret;
> >>> + struct axp288_adc_info *info;
> >>> + struct iio_dev *indio_dev;
> >>> + struct axp20x_dev *axp20x =
> >>> dev_get_drvdata(pdev->dev.parent); +
> >>> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> >>> sizeof(*info));
> >>> + if (!indio_dev)
> >>> + return -ENOMEM;
> >>> +
> >>> + info = iio_priv(indio_dev);
> >>> + info->irq = platform_get_irq(pdev, 0);
> >>> + if (info->irq < 0) {
> >>> + dev_err(&pdev->dev, "no irq resource?\n");
> >>> + return info->irq;
> >>> + }
> >>> + platform_set_drvdata(pdev, indio_dev);
> >>> + info->regmap = axp20x->regmap;
> >>> + ret = axp288_adc_enable(axp20x->regmap, true);
> >>> + if (ret) {
> >>> + dev_err(&pdev->dev, "Unable to enable ADC
> >>> device\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + indio_dev->dev.parent = &pdev->dev;
> >>> + indio_dev->name = pdev->name;
> >>> + indio_dev->channels = axp288_adc_channels;
> >>> + indio_dev->num_channels =
> >>> ARRAY_SIZE(axp288_adc_channels);
> >>> + indio_dev->info = &axp288_adc_iio_info;
> >>> + indio_dev->modes = INDIO_DIRECT_MODE;
> >>> + ret = iio_map_array_register(indio_dev,
> >>> axp288_adc_default_maps);
> >>> + if (ret < 0)
> >>> + goto err_disable_dev;
> >>> +
> >>> + ret = iio_device_register(indio_dev);
> >>> + if (ret < 0) {
> >>> + dev_err(&pdev->dev, "unable to register iio
> >>> device\n");
> >>> + goto err_array_unregister;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +
> >>> +err_array_unregister:
> >>> + iio_map_array_unregister(indio_dev);
> >>> +err_disable_dev:
> >>> + ret = axp288_adc_enable(axp20x->regmap, false);
> >> Don't set ret here. We want to return the error code from the
> >> function that failed above.
> >>> +
> > good point. thanks for the review.
> >
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static int axp288_adc_remove(struct platform_device *pdev)
> >>> +{
> >>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >>> + struct axp20x_dev *axp20x =
> >>> dev_get_drvdata(pdev->dev.parent); +
> >>> + if (axp288_adc_enable(axp20x->regmap, false))
> >>> + dev_err(&pdev->dev, "Unable to disable ADC
> >>> device\n");
> >>> + iio_device_unregister(indio_dev);
> >>> + iio_map_array_unregister(indio_dev);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static struct platform_driver axp288_adc_driver = {
> >>> + .probe = axp288_adc_probe,
> >>> + .remove = axp288_adc_remove,
> >>> + .driver = {
> >>> + .name = "axp288_adc",
> >>> + .owner = THIS_MODULE,
> >>> + },
> >>> +};
> >>> +
> >>> +module_platform_driver(axp288_adc_driver);
> >>> +
> >>> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
> >>> +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
> >>> +MODULE_LICENSE("GPL");
> >>>
> >>
> >
> > [Jacob Pan]
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-iio" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
[Jacob Pan]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
2014-09-18 5:39 ` Lee Jones
@ 2014-09-21 13:01 ` Jonathan Cameron
1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2014-09-21 13:01 UTC (permalink / raw)
To: Jacob Pan, IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Samuel Ortiz, Liam Girdwood, Mark Brown, Grant Likely,
Greg Kroah-Hartman, Rob Herring, Lars-Peter Clausen,
Hartmut Knaack, Fugang Duan, Arnd Bergmann, Zubair Lutfullah,
Sebastian Reichel, Johannes Thumshirn, Philippe Reynes,
Angelo Compagnucci, Doug Anderson, Ramakrishna Pallala,
Peter Meerwald, Maxime Ripard, Rafael Wysocki
On 17/09/14 01:11, Jacob Pan wrote:
> X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR platforms. Similar
> to AXP202/209, AXP288 comes with USB charger, more LDO and BUCK channels, and
> AD converters. It also provides extended status and interrupt reporting
> capabilities than the devices currently supported in axp20x.c.
>
> In addition to feature extension, this patch also adds ACPI binding for
> enumeration.
>
> This consolidated driver should support more X-Powers' PMICs in both device
> tree and ACPI enumerated platforms.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
One suggested improvement inline. When you a driver moves from supporting
one part to a couple with different configurations, a good pattern is
to pick between const configuration structures rather than having one
that is modified (or copied and then modified).
Jonathan
> ---
> drivers/mfd/Kconfig | 3 +-
> drivers/mfd/axp20x.c | 353 +++++++++++++++++++++++++++++++++++++--------
> include/linux/mfd/axp20x.h | 58 ++++++++
> 3 files changed, 354 insertions(+), 60 deletions(-)
>
...
> -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> +static struct acpi_device_id axp20x_acpi_match[] = {
> + {
> + .id = "INT33F4",
> + .driver_data = (kernel_ulong_t)AXP288_ID,
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
> +
> +/* common irq chip attributes only */
> +static struct regmap_irq_chip axp20x_regmap_irq_chip = {
> .name = "axp20x_irq_chip",
> .status_base = AXP20X_IRQ1_STATE,
> .ack_base = AXP20X_IRQ1_STATE,
> .mask_base = AXP20X_IRQ1_EN,
> - .num_regs = 5,
> - .irqs = axp20x_regmap_irqs,
> - .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs),
> .mask_invert = true,
> .init_ack_masked = true,
> };
> @@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
> },
> };
>
> +static struct resource axp288_adc_resources[] = {
> + {
> + .name = "GPADC",
> + .start = AXP288_IRQ_GPADC,
> + .end = AXP288_IRQ_GPADC,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct resource axp288_charger_resources[] = {
> + {
> + .start = AXP288_IRQ_OV,
> + .end = AXP288_IRQ_OV,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_DONE,
> + .end = AXP288_IRQ_DONE,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_CHARGING,
> + .end = AXP288_IRQ_CHARGING,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_SAFE_QUIT,
> + .end = AXP288_IRQ_SAFE_QUIT,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_SAFE_ENTER,
> + .end = AXP288_IRQ_SAFE_ENTER,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_QCBTU,
> + .end = AXP288_IRQ_QCBTU,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_CBTU,
> + .end = AXP288_IRQ_CBTU,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_QCBTO,
> + .end = AXP288_IRQ_QCBTO,
> + .flags = IORESOURCE_IRQ,
> + },
> + {
> + .start = AXP288_IRQ_CBTO,
> + .end = AXP288_IRQ_CBTO,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct mfd_cell axp288_cells[] = {
> + {
> + .name = "axp288_adc",
> + .num_resources = ARRAY_SIZE(axp288_adc_resources),
> + .resources = axp288_adc_resources,
> + },
> + {
> + .name = "axp288_charger",
> + .num_resources = ARRAY_SIZE(axp288_charger_resources),
> + .resources = axp288_charger_resources,
> + },
> + {
> + .name = "axp288_battery",
> + .num_resources = ARRAY_SIZE(axp288_battery_resources),
> + .resources = axp288_battery_resources,
> + },
> +};
> +
> static struct axp20x_dev *axp20x_pm_power_off;
> static void axp20x_power_off(void)
> {
> + if (axp20x_pm_power_off->variant == AXP288_ID)
> + return;
> regmap_write(axp20x_pm_power_off->regmap, AXP20X_OFF_CTRL,
> AXP20X_OFF);
> }
>
> +static int axp20x_match_device(struct axp20x_dev *axp20x, struct device *dev)
> +{
> + const struct acpi_device_id *acpi_id;
> + const struct of_device_id *of_id;
> +
> + of_id = of_match_device(axp20x_of_match, dev);
> + if (of_id)
> + axp20x->variant = (long) of_id->data;
> + else {
> + acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!acpi_id || !acpi_id->driver_data) {
> + dev_err(dev, "Unable to determine ID\n");
> + return -ENODEV;
> + }
> + axp20x->variant = (long) acpi_id->driver_data;
> + }
> + switch (axp20x->variant) {
> + case AXP202_ID:
> + case AXP209_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> + axp20x->cells = axp20x_cells;
> + axp20x->regmap_cfg = &axp20x_regmap_config;
> + axp20x_regmap_irq_chip.num_regs = 5;
> + axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp20x_regmap_irqs);
You would be better off having two regmap_irq_chip structures and
just picking the relevant one here. Then they can remain const.
> + break;
> + case AXP288_ID:
> + axp20x->cells = axp288_cells;
> + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> + axp20x->regmap_cfg = &axp288_regmap_config;
> + axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp288_regmap_irqs);
> + axp20x_regmap_irq_chip.num_regs = 6;
> + break;
> + default:
> + dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> + return -ENODEV;
> + }
> + dev_info(dev, "AXP20x variant %s found\n",
> + axp20x_model_names[axp20x->variant]);
> +
> + return 0;
> +}
> +
...
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/3] iio: add documentation for current attribute
2014-09-17 0:11 ` [PATCH v4 3/3] iio: add documentation for current attribute Jacob Pan
@ 2014-09-21 13:03 ` Jonathan Cameron
0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2014-09-21 13:03 UTC (permalink / raw)
To: Jacob Pan, IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Samuel Ortiz, Liam Girdwood, Mark Brown, Grant Likely,
Greg Kroah-Hartman, Rob Herring, Lars-Peter Clausen,
Hartmut Knaack, Fugang Duan, Arnd Bergmann, Zubair Lutfullah,
Sebastian Reichel, Johannes Thumshirn, Philippe Reynes,
Angelo Compagnucci, Doug Anderson, Ramakrishna Pallala,
Peter Meerwald, Maxime Ripard, Rafael Wysocki
On 17/09/14 01:11, Jacob Pan wrote:
> Add documentation for input current raw sysfs attribute.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Applied to the togreg branch of iio.git
(initially pushed out as testing for autobuilders to play - though
obviously they aren't going to do much with this patch!)
Jonathan
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index d760b02..1ac9ac2 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1028,3 +1028,12 @@ Contact: linux-iio@vger.kernel.org
> Description:
> Raw value of rotation from true/magnetic north measured with
> or without compensation from tilt sensors.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_currentX_raw
> +KernelVersion: 3.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Raw current measurement from channel X. Units are in milliamps
> + after application of scale and offset. If no offset or scale is
> + present, output should be considered as processed with the
> + unit in milliamps.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-17 0:11 ` [PATCH v4 2/3] iio/adc: add support for axp288 adc Jacob Pan
2014-09-17 22:20 ` Hartmut Knaack
@ 2014-09-21 13:14 ` Jonathan Cameron
2014-09-22 17:46 ` Jacob Pan
1 sibling, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2014-09-21 13:14 UTC (permalink / raw)
To: Jacob Pan, IIO, LKML, DEVICE TREE, Lee Jones
Cc: Carlo Caione, Srinivas Pandruvada, Aaron Lu, Alan Cox,
Samuel Ortiz, Liam Girdwood, Mark Brown, Grant Likely,
Greg Kroah-Hartman, Rob Herring, Lars-Peter Clausen,
Hartmut Knaack, Fugang Duan, Arnd Bergmann, Zubair Lutfullah,
Sebastian Reichel, Johannes Thumshirn, Philippe Reynes,
Angelo Compagnucci, Doug Anderson, Ramakrishna Pallala,
Peter Meerwald, Maxime Ripard, Rafael Wysocki
On 17/09/14 01:11, Jacob Pan wrote:
> Platform driver for X-Powers AXP288 ADC, which is a sub-device of the
> customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC device
> enumerates as one of the MFD cell devices. It uses IIO infrastructure
> to communicate with userspace and consumer drivers.
>
> Usages of ADC channels include battery charging and thermal sensors.
>
> Based on initial work by:
> Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
A few more bits from me...
Jonathan
> ---
> drivers/iio/adc/Kconfig | 8 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/axp288_adc.c | 254 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 263 insertions(+)
> create mode 100644 drivers/iio/adc/axp288_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 11b048a..db2681b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -127,6 +127,14 @@ config AT91_ADC
> help
> Say yes here to build support for Atmel AT91 ADC.
>
> +config AXP288_ADC
> + tristate "X-Powers AXP288 ADC driver"
> + depends on MFD_AXP20X
> + help
> + Say yes here to have support for X-Powers power management IC (PMIC) ADC
> + device. Depending on platform configuration, this general purpose ADC can
> + be used for sampling sensors such as thermal resistors.
> +
> config EXYNOS_ADC
> tristate "Exynos ADC driver support"
> depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ad81b51..19640f9 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AD799X) += ad799x.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1027) += max1027.o
> diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
> new file mode 100644
> index 0000000..333c624
> --- /dev/null
> +++ b/drivers/iio/adc/axp288_adc.c
> @@ -0,0 +1,254 @@
> +/*
> + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +
> +#define AXP288_ADC_EN_MASK 0xF1
> +#define AXP288_ADC_TS_PIN_GPADC 0xF2
> +#define AXP288_ADC_TS_PIN_ON 0xF3
> +
> +enum axp288_adc_id {
> + AXP288_ADC_TS,
> + AXP288_ADC_PMIC,
> + AXP288_ADC_GP,
> + AXP288_ADC_BATT_CHRG_I,
> + AXP288_ADC_BATT_DISCHRG_I,
> + AXP288_ADC_BATT_V,
> + AXP288_ADC_NR_CHAN,
> +};
> +
> +struct axp288_adc_info {
> + int irq;
> + struct device *dev;
Is the dev element ever used?
> + struct regmap *regmap;
> +};
> +
> +static const struct iio_chan_spec const axp288_adc_channels[] = {
> + {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 0,
> + .address = AXP288_TS_ADC_H,
> + .datasheet_name = "CH0",
> + }, {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 1,
> + .address = AXP288_PMIC_ADC_H,
> + .datasheet_name = "CH1",
> + }, {
> + .indexed = 1,
> + .type = IIO_TEMP,
> + .channel = 2,
> + .address = AXP288_GP_ADC_H,
> + .datasheet_name = "CH2",
> + }, {
> + .indexed = 1,
> + .type = IIO_CURRENT,
> + .channel = 3,
> + .address = AXP20X_BATT_CHRG_I_H,
> + .datasheet_name = "CH3",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + }, {
> + .indexed = 1,
> + .type = IIO_CURRENT,
> + .channel = 4,
> + .address = AXP20X_BATT_DISCHRG_I_H,
> + .datasheet_name = "CH4",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + }, {
> + .indexed = 1,
> + .type = IIO_VOLTAGE,
> + .channel = 5,
> + .address = AXP20X_BATT_V_H,
> + .datasheet_name = "CH5",
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> +};
> +
> +#define AXP288_ADC_MAP(_adc_channel_label, _consumer_dev_name, \
> + _consumer_channel) \
> + { \
> + .adc_channel_label = _adc_channel_label, \
> + .consumer_dev_name = _consumer_dev_name, \
> + .consumer_channel = _consumer_channel, \
> + }
> +
> +/* for consumer drivers */
> +static struct iio_map axp288_adc_default_maps[] = {
> + AXP288_ADC_MAP("TS_PIN", "axp288-batt", "axp288-batt-temp"),
> + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic", "axp288-pmic-temp"),
> + AXP288_ADC_MAP("GPADC", "axp288-gpadc", "axp288-system-temp"),
> + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg", "axp288-chrg-curr"),
> + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg", "axp288-chrg-d-curr"),
> + AXP288_ADC_MAP("BATT_V", "axp288-batt", "axp288-batt-volt"),
> + {},
> +};
> +
> +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int ret;
> + struct axp288_adc_info *info = iio_priv(indio_dev);
> + u8 buf[2];
> +
> + mutex_lock(&indio_dev->mlock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + /* special case for GPADC sample */
> + if (chan->address == AXP288_GP_ADC_H) {
> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_PIN_GPADC);
> + if (ret) {
> + dev_err(&indio_dev->dev, "GPADC mode\n");
> + mutex_unlock(&indio_dev->mlock);
> + return ret;
> + }
> + }
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = regmap_bulk_read(info->regmap, chan->address, buf, 2);
> + if (ret) {
> + dev_err(&indio_dev->dev, "sample raw data failed\n");
> + break;
> + }
> + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
Why is this statement down here rather than in the relevant case entry
above?
> + if (mask == IIO_CHAN_INFO_RAW && chan->address == AXP288_GP_ADC_H)
> + ret = regmap_write(info->regmap, AXP288_ADC_TS_PIN_CTRL,
> + AXP288_ADC_TS_PIN_ON);
> +
> + mutex_unlock(&indio_dev->mlock);
> +
> + return ret;
> +}
> +
> +static int axp288_adc_enable(struct regmap *regmap, bool enable)
It's fussy, but axp288_adc_set_state or similar would be slightly
less confusing as enable with a parameter of false, just means
don't enable, not disable as i would imagine you intend...
> +{
> + unsigned int pin_on, adc_en;
> +
> + if (enable) {
> + pin_on = AXP288_ADC_TS_PIN_ON;
> + adc_en = AXP288_ADC_EN_MASK;
> + } else {
> + pin_on = ~AXP288_ADC_TS_PIN_ON;
> + adc_en = ~AXP288_ADC_EN_MASK;
> + }
> + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
> + return -EIO;
> +
> + return regmap_write(regmap, AXP20X_ADC_EN1, ~AXP288_ADC_EN_MASK);
> +}
> +
> +static const struct iio_info axp288_adc_iio_info = {
> + .read_raw = &axp288_adc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int axp288_adc_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct axp288_adc_info *info;
> + struct iio_dev *indio_dev;
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(&pdev->dev, "no irq resource?\n");
> + return info->irq;
> + }
> + platform_set_drvdata(pdev, indio_dev);
> + info->regmap = axp20x->regmap;
> + ret = axp288_adc_enable(axp20x->regmap, true);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable ADC device\n");
> + return ret;
> + }
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = pdev->name;
> + indio_dev->channels = axp288_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
> + indio_dev->info = &axp288_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + ret = iio_map_array_register(indio_dev, axp288_adc_default_maps);
> + if (ret < 0)
> + goto err_disable_dev;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "unable to register iio device\n");
> + goto err_array_unregister;
> + }
> +
> + return 0;
> +
> +err_array_unregister:
> + iio_map_array_unregister(indio_dev);
> +err_disable_dev:
> + ret = axp288_adc_enable(axp20x->regmap, false);
> +
> + return ret;
> +}
> +
> +static int axp288_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
Would feel a little more consistent to get the regmap from info->regmap,
but doesn't really matter I suppose...
> +
> + if (axp288_adc_enable(axp20x->regmap, false))
> + dev_err(&pdev->dev, "Unable to disable ADC device\n");
> + iio_device_unregister(indio_dev);
> + iio_map_array_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver axp288_adc_driver = {
> + .probe = axp288_adc_probe,
> + .remove = axp288_adc_remove,
> + .driver = {
> + .name = "axp288_adc",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(axp288_adc_driver);
> +
> +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
> +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/3] iio/adc: add support for axp288 adc
2014-09-21 13:14 ` Jonathan Cameron
@ 2014-09-22 17:46 ` Jacob Pan
0 siblings, 0 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-22 17:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: IIO, LKML, DEVICE TREE, Lee Jones, Carlo Caione,
Srinivas Pandruvada, Aaron Lu, Alan Cox, Samuel Ortiz,
Liam Girdwood, Mark Brown, Grant Likely, Greg Kroah-Hartman,
Rob Herring, Lars-Peter Clausen, Hartmut Knaack, Fugang Duan,
Arnd Bergmann, Zubair Lutfullah, Sebastian Reichel,
Johannes Thumshirn, Philippe Reynes, Angelo Compagnucci,
Doug Anderson, Ramakrishna Pallala, Peter Meerwald,
Maxime Ripard, Rafael Wysocki
On Sun, 21 Sep 2014 14:14:17 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On 17/09/14 01:11, Jacob Pan wrote:
> > Platform driver for X-Powers AXP288 ADC, which is a sub-device of
> > the customized AXP288 PMIC for Intel Baytrail-CR platforms. GPADC
> > device enumerates as one of the MFD cell devices. It uses IIO
> > infrastructure to communicate with userspace and consumer drivers.
> >
> > Usages of ADC channels include battery charging and thermal sensors.
> >
> > Based on initial work by:
> > Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> >
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> A few more bits from me...
>
> Jonathan
> > ---
> > drivers/iio/adc/Kconfig | 8 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/axp288_adc.c | 254
> > +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 263
> > insertions(+) create mode 100644 drivers/iio/adc/axp288_adc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 11b048a..db2681b 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -127,6 +127,14 @@ config AT91_ADC
> > help
> > Say yes here to build support for Atmel AT91 ADC.
> >
> > +config AXP288_ADC
> > + tristate "X-Powers AXP288 ADC driver"
> > + depends on MFD_AXP20X
> > + help
> > + Say yes here to have support for X-Powers power
> > management IC (PMIC) ADC
> > + device. Depending on platform configuration, this
> > general purpose ADC can
> > + be used for sampling sensors such as thermal resistors.
> > +
> > config EXYNOS_ADC
> > tristate "Exynos ADC driver support"
> > depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index ad81b51..19640f9 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> > obj-$(CONFIG_AD7887) += ad7887.o
> > obj-$(CONFIG_AD799X) += ad799x.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > diff --git a/drivers/iio/adc/axp288_adc.c
> > b/drivers/iio/adc/axp288_adc.c new file mode 100644
> > index 0000000..333c624
> > --- /dev/null
> > +++ b/drivers/iio/adc/axp288_adc.c
> > @@ -0,0 +1,254 @@
> > +/*
> > + * axp288_adc.c - X-Powers AXP288 PMIC ADC Driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + *
> > + *
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/axp20x.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/machine.h>
> > +#include <linux/iio/driver.h>
> > +
> > +#define AXP288_ADC_EN_MASK 0xF1
> > +#define AXP288_ADC_TS_PIN_GPADC 0xF2
> > +#define AXP288_ADC_TS_PIN_ON 0xF3
> > +
> > +enum axp288_adc_id {
> > + AXP288_ADC_TS,
> > + AXP288_ADC_PMIC,
> > + AXP288_ADC_GP,
> > + AXP288_ADC_BATT_CHRG_I,
> > + AXP288_ADC_BATT_DISCHRG_I,
> > + AXP288_ADC_BATT_V,
> > + AXP288_ADC_NR_CHAN,
> > +};
> > +
> > +struct axp288_adc_info {
> > + int irq;
> > + struct device *dev;
> Is the dev element ever used?
>
> > + struct regmap *regmap;
> > +};
> > +
> > +static const struct iio_chan_spec const axp288_adc_channels[] = {
> > + {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 0,
> > + .address = AXP288_TS_ADC_H,
> > + .datasheet_name = "CH0",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 1,
> > + .address = AXP288_PMIC_ADC_H,
> > + .datasheet_name = "CH1",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_TEMP,
> > + .channel = 2,
> > + .address = AXP288_GP_ADC_H,
> > + .datasheet_name = "CH2",
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_CURRENT,
> > + .channel = 3,
> > + .address = AXP20X_BATT_CHRG_I_H,
> > + .datasheet_name = "CH3",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_CURRENT,
> > + .channel = 4,
> > + .address = AXP20X_BATT_DISCHRG_I_H,
> > + .datasheet_name = "CH4",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + }, {
> > + .indexed = 1,
> > + .type = IIO_VOLTAGE,
> > + .channel = 5,
> > + .address = AXP20X_BATT_V_H,
> > + .datasheet_name = "CH5",
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + },
> > +};
> > +
> > +#define AXP288_ADC_MAP(_adc_channel_label,
> > _consumer_dev_name, \
> > + _consumer_channel) \
> > + { \
> > + .adc_channel_label = _adc_channel_label, \
> > + .consumer_dev_name = _consumer_dev_name, \
> > + .consumer_channel =
> > _consumer_channel, \
> > + }
> > +
> > +/* for consumer drivers */
> > +static struct iio_map axp288_adc_default_maps[] = {
> > + AXP288_ADC_MAP("TS_PIN", "axp288-batt",
> > "axp288-batt-temp"),
> > + AXP288_ADC_MAP("PMIC_TEMP", "axp288-pmic",
> > "axp288-pmic-temp"),
> > + AXP288_ADC_MAP("GPADC", "axp288-gpadc",
> > "axp288-system-temp"),
> > + AXP288_ADC_MAP("BATT_CHG_I", "axp288-chrg",
> > "axp288-chrg-curr"),
> > + AXP288_ADC_MAP("BATT_DISCHRG_I", "axp288-chrg",
> > "axp288-chrg-d-curr"),
> > + AXP288_ADC_MAP("BATT_V", "axp288-batt",
> > "axp288-batt-volt"),
> > + {},
> > +};
> > +
> > +static int axp288_adc_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int ret;
> > + struct axp288_adc_info *info = iio_priv(indio_dev);
> > + u8 buf[2];
> > +
> > + mutex_lock(&indio_dev->mlock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + /* special case for GPADC sample */
> > + if (chan->address == AXP288_GP_ADC_H) {
> > + ret = regmap_write(info->regmap,
> > AXP288_ADC_TS_PIN_CTRL,
> > + AXP288_ADC_TS_PIN_GPADC);
> > + if (ret) {
> > + dev_err(&indio_dev->dev, "GPADC
> > mode\n");
> > + mutex_unlock(&indio_dev->mlock);
> > + return ret;
> > + }
> > + }
> > + case IIO_CHAN_INFO_PROCESSED:
> > + ret = regmap_bulk_read(info->regmap,
> > chan->address, buf, 2);
> > + if (ret) {
> > + dev_err(&indio_dev->dev, "sample raw data
> > failed\n");
> > + break;
> > + }
> > + *val = (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> > + ret = IIO_VAL_INT;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> Why is this statement down here rather than in the relevant case entry
> above?
I think you meant the line below, why I need to check the same
conditions again for INFO_RAW. The reason is that the regmap_write()
call is preparing a valid mode for the regmap_bulk_read below. I could
move the statement in the case statement and duplicate the
regmap_bulk_read() such that INFO_RAW and INFO_PROCESSED has its own
break statement. That should make a code more readable but with
duplicated code.
> > + if (mask == IIO_CHAN_INFO_RAW && chan->address ==
> > AXP288_GP_ADC_H)
> > + ret = regmap_write(info->regmap,
> > AXP288_ADC_TS_PIN_CTRL,
> > + AXP288_ADC_TS_PIN_ON);
> > +
> > + mutex_unlock(&indio_dev->mlock);
> > +
> > + return ret;
> > +}
> > +
> > +static int axp288_adc_enable(struct regmap *regmap, bool enable)
> It's fussy, but axp288_adc_set_state or similar would be slightly
> less confusing as enable with a parameter of false, just means
> don't enable, not disable as i would imagine you intend...
agreed.
>
> > +{
> > + unsigned int pin_on, adc_en;
> > +
> > + if (enable) {
> > + pin_on = AXP288_ADC_TS_PIN_ON;
> > + adc_en = AXP288_ADC_EN_MASK;
> > + } else {
> > + pin_on = ~AXP288_ADC_TS_PIN_ON;
> > + adc_en = ~AXP288_ADC_EN_MASK;
> > + }
> > + if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, pin_on))
> > + return -EIO;
> > +
> > + return regmap_write(regmap, AXP20X_ADC_EN1,
> > ~AXP288_ADC_EN_MASK); +}
> > +
> > +static const struct iio_info axp288_adc_iio_info = {
> > + .read_raw = &axp288_adc_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int axp288_adc_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct axp288_adc_info *info;
> > + struct iio_dev *indio_dev;
> > + struct axp20x_dev *axp20x =
> > dev_get_drvdata(pdev->dev.parent); +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev,
> > sizeof(*info));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + info = iio_priv(indio_dev);
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(&pdev->dev, "no irq resource?\n");
> > + return info->irq;
> > + }
> > + platform_set_drvdata(pdev, indio_dev);
> > + info->regmap = axp20x->regmap;
> > + ret = axp288_adc_enable(axp20x->regmap, true);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to enable ADC
> > device\n");
> > + return ret;
> > + }
> > +
> > + indio_dev->dev.parent = &pdev->dev;
> > + indio_dev->name = pdev->name;
> > + indio_dev->channels = axp288_adc_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(axp288_adc_channels);
> > + indio_dev->info = &axp288_adc_iio_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + ret = iio_map_array_register(indio_dev,
> > axp288_adc_default_maps);
> > + if (ret < 0)
> > + goto err_disable_dev;
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "unable to register iio
> > device\n");
> > + goto err_array_unregister;
> > + }
> > +
> > + return 0;
> > +
> > +err_array_unregister:
> > + iio_map_array_unregister(indio_dev);
> > +err_disable_dev:
> > + ret = axp288_adc_enable(axp20x->regmap, false);
> > +
> > + return ret;
> > +}
> > +
> > +static int axp288_adc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct axp20x_dev *axp20x =
> > dev_get_drvdata(pdev->dev.parent);
> Would feel a little more consistent to get the regmap from
> info->regmap, but doesn't really matter I suppose...
> > +
> > + if (axp288_adc_enable(axp20x->regmap, false))
> > + dev_err(&pdev->dev, "Unable to disable ADC
> > device\n");
> > + iio_device_unregister(indio_dev);
> > + iio_map_array_unregister(indio_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver axp288_adc_driver = {
> > + .probe = axp288_adc_probe,
> > + .remove = axp288_adc_remove,
> > + .driver = {
> > + .name = "axp288_adc",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +module_platform_driver(axp288_adc_driver);
> > +
> > +MODULE_AUTHOR("Jacob Pan <jacob.jun.pan@linux.intel.com>");
> > +MODULE_DESCRIPTION("X-Powers AXP288 ADC Driver");
> > +MODULE_LICENSE("GPL");
> >
[Jacob Pan]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-18 12:32 ` Jacob Pan
@ 2014-09-24 11:02 ` Lee Jones
2014-09-24 13:46 ` Jacob Pan
0 siblings, 1 reply; 17+ messages in thread
From: Lee Jones @ 2014-09-24 11:02 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, DEVICE TREE, Carlo Caione, Srinivas Pandruvada,
Aaron Lu, Alan Cox, Jean Delvare, Samuel Ortiz, Liam Girdwood,
Mark Brown, Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Thu, 18 Sep 2014, Jacob Pan wrote:
> Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 16 Sep 2014, Jacob Pan wrote:
> >
> > > X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR
> > > platforms. Similar to AXP202/209, AXP288 comes with USB charger,
> > > more LDO and BUCK channels, and AD converters. It also provides
> > > extended status and interrupt reporting capabilities than the
> > > devices currently supported in axp20x.c.
> > >
> > > In addition to feature extension, this patch also adds ACPI binding
> > > for enumeration.
> > >
> > > This consolidated driver should support more X-Powers' PMICs in
> > > both device tree and ACPI enumerated platforms.
> > >
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > ---
> > > drivers/mfd/Kconfig | 3 +-
> > > drivers/mfd/axp20x.c | 353
> > > +++++++++++++++++++++++++++++++++++++--------
> > > include/linux/mfd/axp20x.h | 58 ++++++++ 3 files changed, 354
> > > insertions(+), 60 deletions(-)
[...]
> > > -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> > > +static struct acpi_device_id axp20x_acpi_match[] = {
> > > + {
> > > + .id = "INT33F4",
> > > + .driver_data = (kernel_ulong_t)AXP288_ID,
> >
> > Why do you need to cast this?
> >
> to make sure match driver_data which is different in acpi_device_id than
> of_device_id.
You don't need the cast.
[...]
> > > +static int axp20x_match_device(struct axp20x_dev *axp20x, struct
> > > device *dev) +{
> > > + const struct acpi_device_id *acpi_id;
> > > + const struct of_device_id *of_id;
> > > +
> > > + of_id = of_match_device(axp20x_of_match, dev);
> > > + if (of_id)
> > > + axp20x->variant = (long) of_id->data;
> > > + else {
> > > + acpi_id =
> > > acpi_match_device(dev->driver->acpi_match_table, dev);
> > > + if (!acpi_id || !acpi_id->driver_data) {
> > > + dev_err(dev, "Unable to determine ID\n");
> > > + return -ENODEV;
> > > + }
> > > + axp20x->variant = (long) acpi_id->driver_data;
> > > + }
> >
> > We can do better error handling here and give the user a better sense
> > of what happened if anything were to go wrong. Do:
> >
> > if (dev->of_node)
> > of_id = of_match_device()
> > if (!of_id)
> > error()
> this will give false error on ACPI based platforms, right? in reality
Why would it? dev->of_node should be NULL if running ACPI?
[...]
> > > + switch (axp20x->variant) {
> > > + case AXP202_ID:
> > > + case AXP209_ID:
> > > + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> > > + axp20x->cells = axp20x_cells;
> > > + axp20x->regmap_cfg = &axp20x_regmap_config;
> > > + axp20x_regmap_irq_chip.num_regs = 5;
> > > + axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> > > + axp20x_regmap_irq_chip.num_irqs =
> > > + ARRAY_SIZE(axp20x_regmap_irqs);
> > > + break;
> > > + case AXP288_ID:
> > > + axp20x->cells = axp288_cells;
> > > + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> > > + axp20x->regmap_cfg = &axp288_regmap_config;
> > > + axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> > > + axp20x_regmap_irq_chip.num_irqs =
> > > + ARRAY_SIZE(axp288_regmap_irqs);
> > > + axp20x_regmap_irq_chip.num_regs = 6;
> > > + break;
> > > + default:
> > > + dev_err(dev, "unsupported AXP20X ID %lu\n",
> > > axp20x->variant);
> > > + return -ENODEV;
> >
> > -EINVAL might be better here.
> I was considering the return value gets propagated to probe function
> which is used to query the existence of a device per driver model. But
> I have no strong preference.
I think -EINVAL would be better as the argument passed in
axp20x->variant is invalid.
define EINVAL 22 /* Invalid argument */
> > > + }
> > > + dev_info(dev, "AXP20x variant %s found\n",
> > > + axp20x_model_names[axp20x->variant]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int axp20x_i2c_probe(struct i2c_client *i2c,
> > > - const struct i2c_device_id *id)
> > > + const struct i2c_device_id *id)
> >
> > Sneaky. ;)
> I should not fix the extra white spaces here, unrelated to this patch.
> will remove.
It's okay. I don't mind little things like this occasionally. I find
them more amusing than harmful.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
2014-09-24 11:02 ` Lee Jones
@ 2014-09-24 13:46 ` Jacob Pan
0 siblings, 0 replies; 17+ messages in thread
From: Jacob Pan @ 2014-09-24 13:46 UTC (permalink / raw)
To: Lee Jones
Cc: IIO, LKML, DEVICE TREE, Carlo Caione, Srinivas Pandruvada,
Aaron Lu, Alan Cox, Jean Delvare, Samuel Ortiz, Liam Girdwood,
Mark Brown, Grant Likely, Greg Kroah-Hartman, Rob Herring,
Lars-Peter Clausen, Hartmut Knaack, Fugang Duan, Arnd Bergmann,
Zubair Lutfullah, Sebastian Reichel, Johannes Thumshirn,
Philippe Reynes, Angelo Compagnucci, Doug Anderson,
Ramakrishna Pallala, Peter Meerwald, Maxime Ripard,
Rafael Wysocki
On Wed, 24 Sep 2014 12:02:12 +0100
Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 18 Sep 2014, Jacob Pan wrote:
> > Lee Jones <lee.jones@linaro.org> wrote:
> > > On Tue, 16 Sep 2014, Jacob Pan wrote:
> > >
> > > > X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR
> > > > platforms. Similar to AXP202/209, AXP288 comes with USB charger,
> > > > more LDO and BUCK channels, and AD converters. It also provides
> > > > extended status and interrupt reporting capabilities than the
> > > > devices currently supported in axp20x.c.
> > > >
> > > > In addition to feature extension, this patch also adds ACPI
> > > > binding for enumeration.
> > > >
> > > > This consolidated driver should support more X-Powers' PMICs in
> > > > both device tree and ACPI enumerated platforms.
> > > >
> > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > ---
> > > > drivers/mfd/Kconfig | 3 +-
> > > > drivers/mfd/axp20x.c | 353
> > > > +++++++++++++++++++++++++++++++++++++--------
> > > > include/linux/mfd/axp20x.h | 58 ++++++++ 3 files changed, 354
> > > > insertions(+), 60 deletions(-)
>
> [...]
>
> > > > -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> > > > +static struct acpi_device_id axp20x_acpi_match[] = {
> > > > + {
> > > > + .id = "INT33F4",
> > > > + .driver_data = (kernel_ulong_t)AXP288_ID,
> > >
> > > Why do you need to cast this?
> > >
> > to make sure match driver_data which is different in acpi_device_id
> > than of_device_id.
>
> You don't need the cast.
yes, agree in this case. my thinking is since AXP288_ID is a macro, so
extra careful. I can remove that.
>
> [...]
>
> > > > +static int axp20x_match_device(struct axp20x_dev *axp20x,
> > > > struct device *dev) +{
> > > > + const struct acpi_device_id *acpi_id;
> > > > + const struct of_device_id *of_id;
> > > > +
> > > > + of_id = of_match_device(axp20x_of_match, dev);
> > > > + if (of_id)
> > > > + axp20x->variant = (long) of_id->data;
> > > > + else {
> > > > + acpi_id =
> > > > acpi_match_device(dev->driver->acpi_match_table, dev);
> > > > + if (!acpi_id || !acpi_id->driver_data) {
> > > > + dev_err(dev, "Unable to determine
> > > > ID\n");
> > > > + return -ENODEV;
> > > > + }
> > > > + axp20x->variant = (long) acpi_id->driver_data;
> > > > + }
> > >
> > > We can do better error handling here and give the user a better
> > > sense of what happened if anything were to go wrong. Do:
> > >
> > > if (dev->of_node)
> > > of_id = of_match_device()
> > > if (!of_id)
> > > error()
> > this will give false error on ACPI based platforms, right? in
> > reality
>
> Why would it? dev->of_node should be NULL if running ACPI?
>
right, i missed that. I thought of_match_device() is already checking
of_node. Anyway. I can change the code to the flow you suggested, it is
more explicit. My point is that in both flows, the error() can be the
same and implied by the platform (ACPI/OF).
> [...]
>
> > > > + switch (axp20x->variant) {
> > > > + case AXP202_ID:
> > > > + case AXP209_ID:
> > > > + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> > > > + axp20x->cells = axp20x_cells;
> > > > + axp20x->regmap_cfg = &axp20x_regmap_config;
> > > > + axp20x_regmap_irq_chip.num_regs = 5;
> > > > + axp20x_regmap_irq_chip.irqs =
> > > > axp20x_regmap_irqs;
> > > > + axp20x_regmap_irq_chip.num_irqs =
> > > > + ARRAY_SIZE(axp20x_regmap_irqs);
> > > > + break;
> > > > + case AXP288_ID:
> > > > + axp20x->cells = axp288_cells;
> > > > + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> > > > + axp20x->regmap_cfg = &axp288_regmap_config;
> > > > + axp20x_regmap_irq_chip.irqs =
> > > > axp288_regmap_irqs;
> > > > + axp20x_regmap_irq_chip.num_irqs =
> > > > + ARRAY_SIZE(axp288_regmap_irqs);
> > > > + axp20x_regmap_irq_chip.num_regs = 6;
> > > > + break;
> > > > + default:
> > > > + dev_err(dev, "unsupported AXP20X ID %lu\n",
> > > > axp20x->variant);
> > > > + return -ENODEV;
> > >
> > > -EINVAL might be better here.
> > I was considering the return value gets propagated to probe function
> > which is used to query the existence of a device per driver model.
> > But I have no strong preference.
>
> I think -EINVAL would be better as the argument passed in
> axp20x->variant is invalid.
>
> define EINVAL 22 /* Invalid argument */
>
agreed, changed in v5 just sent.
> > > > + }
> > > > + dev_info(dev, "AXP20x variant %s found\n",
> > > > + axp20x_model_names[axp20x->variant]);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int axp20x_i2c_probe(struct i2c_client *i2c,
> > > > - const struct i2c_device_id *id)
> > > > + const struct i2c_device_id *id)
> > >
> > > Sneaky. ;)
> > I should not fix the extra white spaces here, unrelated to this
> > patch. will remove.
>
> It's okay. I don't mind little things like this occasionally. I find
> them more amusing than harmful.
>
fixed in v5 also.
> [...]
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-09-24 13:46 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-17 0:11 [PATCH v4 0/3] Initial support for XPowers AXP288 PMIC Jacob Pan
2014-09-17 0:11 ` [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic Jacob Pan
2014-09-18 5:39 ` Lee Jones
2014-09-18 12:32 ` Jacob Pan
2014-09-24 11:02 ` Lee Jones
2014-09-24 13:46 ` Jacob Pan
2014-09-21 13:01 ` Jonathan Cameron
2014-09-17 0:11 ` [PATCH v4 2/3] iio/adc: add support for axp288 adc Jacob Pan
2014-09-17 22:20 ` Hartmut Knaack
2014-09-17 22:30 ` Hartmut Knaack
2014-09-17 23:13 ` Jacob Pan
2014-09-18 21:52 ` Hartmut Knaack
2014-09-18 22:35 ` Jacob Pan
2014-09-21 13:14 ` Jonathan Cameron
2014-09-22 17:46 ` Jacob Pan
2014-09-17 0:11 ` [PATCH v4 3/3] iio: add documentation for current attribute Jacob Pan
2014-09-21 13:03 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).