Hi,
this is a patch set to add the support for Dollar Cove TI PMIC found
on some Intel Cherry Trail laptops / tablets. All drivers are based
on the original code from Intel downstream patches, with lots of
rewrites and cleanups. MFD driver is implemented as a stand-alone
like a few other variants, and the input driver got a diet in a
minimalistic form.
The patch set has been tested on ASUS E100H and E200H, as well as on
HP x210.
thanks,
Takashi
===
Takashi Iwai (3):
mfd: Add support for Cherry Trail Dollar Cove TI PMIC
input/keyboard: Add support for Dollar Cove TI power button
ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC
drivers/acpi/Kconfig | 6 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/pmic/intel_pmic_dc_ti.c | 135 ++++++++++++++++++++++++
drivers/input/keyboard/Kconfig | 7 ++
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++
drivers/mfd/Kconfig | 13 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/intel_soc_pmic_dc_ti.c | 189 ++++++++++++++++++++++++++++++++++
9 files changed, 438 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic_dc_ti.c
create mode 100644 drivers/input/keyboard/dc_ti_pwrbtn.c
create mode 100644 drivers/mfd/intel_soc_pmic_dc_ti.c
--
2.14.0
This patch adds the opregion driver for Dollar Cove TI PMIC on Intel
Cherry Trail devices. The patch is based on the original work by
Intel, found at:
https://github.com/01org/ProductionKernelQuilts
with many cleanups and rewrites.
The driver is currently provided only as built-in to follow other
PMIC opregion drivers convention.
The re-enumeration of devices at probe is required for fixing the
issues on HP x2 210 G2. See bug#195689.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195689
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/acpi/Kconfig | 6 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/pmic/intel_pmic_dc_ti.c | 135 +++++++++++++++++++++++++++++++++++
3 files changed, 142 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic_dc_ti.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1ce52f84dc23..9da8dcefde7b 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -521,6 +521,12 @@ config CHT_WC_PMIC_OPREGION
help
This config adds ACPI operation region support for CHT Whiskey Cove PMIC.
+config DC_TI_PMIC_OPREGION
+ bool "ACPI operation region support for Dollar Cove TI PMIC"
+ depends on INTEL_SOC_PMIC_DC_TI
+ help
+ This config adds ACPI operation region support for Dollar Cove TI PMIC.
+
endif
config ACPI_CONFIGFS
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc62b1f..0a9008b971af 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -103,6 +103,7 @@ obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
+obj-$(CONFIG_DC_TI_PMIC_OPREGION) += pmic/intel_pmic_dc_ti.o
obj-$(CONFIG_ACPI_CONFIGFS) += acpi_configfs.o
diff --git a/drivers/acpi/pmic/intel_pmic_dc_ti.c b/drivers/acpi/pmic/intel_pmic_dc_ti.c
new file mode 100644
index 000000000000..236ff31c5a79
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_dc_ti.c
@@ -0,0 +1,135 @@
+/*
+ * intel_pmic_dc_ti.c - TI Dollar Cove PMIC operation region driver
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * Rewritten and cleaned up
+ * Copyright (C) 2017 Takashi Iwai <[email protected]>
+ */
+
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include "intel_pmic.h"
+
+#define TI_DC_PMICTEMP_LOW 0x57
+#define TI_DC_BATTEMP_LOW 0x59
+#define TI_DC_GPADC_LOW 0x5b
+
+static struct pmic_table dc_ti_power_table[] = {
+ { .address = 0x00, .reg = 0x41 },
+ { .address = 0x04, .reg = 0x42 },
+ { .address = 0x08, .reg = 0x43 },
+ { .address = 0x0c, .reg = 0x45 },
+ { .address = 0x10, .reg = 0x46 },
+ { .address = 0x14, .reg = 0x47 },
+ { .address = 0x18, .reg = 0x48 },
+ { .address = 0x1c, .reg = 0x49 },
+ { .address = 0x20, .reg = 0x4a },
+ { .address = 0x24, .reg = 0x4b },
+ { .address = 0x28, .reg = 0x4c },
+ { .address = 0x2c, .reg = 0x4d },
+ { .address = 0x30, .reg = 0x4e },
+};
+
+static struct pmic_table dc_ti_thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = TI_DC_GPADC_LOW
+ },
+ {
+ .address = 0x0c,
+ .reg = TI_DC_GPADC_LOW
+ },
+ /* TMP2 -> SYSTEMP */
+ {
+ .address = 0x18,
+ .reg = TI_DC_GPADC_LOW
+ },
+ /* TMP3 -> BATTEMP */
+ {
+ .address = 0x24,
+ .reg = TI_DC_BATTEMP_LOW
+ },
+ {
+ .address = 0x30,
+ .reg = TI_DC_GPADC_LOW
+ },
+ /* TMP5 -> PMICTEMP */
+ {
+ .address = 0x3c,
+ .reg = TI_DC_PMICTEMP_LOW
+ },
+};
+
+static int dc_ti_pmic_get_power(struct regmap *regmap, int reg, int bit,
+ u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = data & 1;
+ return 0;
+}
+
+static int dc_ti_pmic_update_power(struct regmap *regmap, int reg, int bit,
+ bool on)
+{
+ return regmap_update_bits(regmap, reg, 1, on);
+}
+
+static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ int temp_l, temp_h;
+
+ if (regmap_read(regmap, reg, &temp_l) ||
+ regmap_read(regmap, reg - 1, &temp_h))
+ return -EIO;
+
+ return temp_l | (temp_h & 0x3) << 8;
+}
+
+static struct intel_pmic_opregion_data dc_ti_pmic_opregion_data = {
+ .get_power = dc_ti_pmic_get_power,
+ .update_power = dc_ti_pmic_update_power,
+ .get_raw_temp = dc_ti_pmic_get_raw_temp,
+ .power_table = dc_ti_power_table,
+ .power_table_count = ARRAY_SIZE(dc_ti_power_table),
+ .thermal_table = dc_ti_thermal_table,
+ .thermal_table_count = ARRAY_SIZE(dc_ti_thermal_table),
+};
+
+static int dc_ti_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+ int err;
+
+ err = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), pmic->regmap,
+ &dc_ti_pmic_opregion_data);
+ if (err < 0)
+ return err;
+
+ /* Re-enumerate devices depending on PMIC */
+ acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent));
+ return 0;
+}
+
+static struct platform_device_id dc_ti_pmic_opregion_id_table[] = {
+ { .name = "dc_ti_region" },
+ {},
+};
+
+static struct platform_driver dc_ti_pmic_opregion_driver = {
+ .probe = dc_ti_pmic_opregion_probe,
+ .driver = {
+ .name = "dollar_cove_ti_pmic",
+ },
+ .id_table = dc_ti_pmic_opregion_id_table,
+};
+module_platform_driver(dc_ti_pmic_opregion_driver);
+
+MODULE_DESCRIPTION("Dollar Cove TI PMIC opregion driver");
+MODULE_LICENSE("GPLv2");
--
2.14.0
This patch adds the MFD driver for Dollar Cove TI PMIC (ACPI INT33F5)
that is found on some Intel Cherry Trail devices.
The driver is based on the original work by Intel, found at:
https://github.com/01org/ProductionKernelQuilts
This is a minimal version for adding the basic resources. Currently,
only ACPI PMIC opregion and the external power-button are used.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/mfd/Kconfig | 13 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/intel_soc_pmic_dc_ti.c | 189 +++++++++++++++++++++++++++++++++++++
3 files changed, 203 insertions(+)
create mode 100644 drivers/mfd/intel_soc_pmic_dc_ti.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 94ad2c1c3d90..28c12bb50c9f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -496,6 +496,19 @@ config INTEL_SOC_PMIC_CHTWC
available before any devices using it are probed. This option also
causes the designware-i2c driver to be builtin for the same reason.
+config INTEL_SOC_PMIC_DC_TI
+ tristate "Support for Dollar Cove TI PMIC"
+ depends on GPIOLIB
+ depends on I2C
+ depends on ACPI
+ depends on X86
+ select MFD_CORE
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ help
+ Select this option for supporting Dollar Cove TI PMIC device that is
+ found on some Intel Cherry Trail systems.
+
config MFD_INTEL_LPSS
tristate
select COMMON_CLK
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 080793b3fd0e..16c4fe519d30 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -216,6 +216,7 @@ intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o
obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o
+obj-$(CONFIG_INTEL_SOC_PMIC_DC_TI) += intel_soc_pmic_dc_ti.o
obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
diff --git a/drivers/mfd/intel_soc_pmic_dc_ti.c b/drivers/mfd/intel_soc_pmic_dc_ti.c
new file mode 100644
index 000000000000..f1c2ac310bf0
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_dc_ti.c
@@ -0,0 +1,189 @@
+/*
+ * Device access for Dollar Cove TI PMIC
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: Ramakrishna Pallala <[email protected]>
+ *
+ * Cleanup and forward-ported
+ * Copyright (c) 2017 Takashi Iwai <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/consumer.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+
+#define DC_TI_IRQLVL1 0x01
+#define DC_TI_MASK_IRQLVL1 0x02
+
+/* Level 1 IRQs */
+enum {
+ DC_TI_PWRBTN = 0, /* power button */
+ DC_TI_DIETMPWARN, /* thermal */
+ DC_TI_ADCCMPL, /* ADC */
+ /* no irq 3 */
+ DC_TI_VBATLOW = 4, /* battery */
+ DC_TI_VBUSDET, /* power source */
+ /* no irq 6 */
+ DC_TI_CCEOCAL = 7, /* battery */
+};
+
+static struct resource power_button_resources[] = {
+ DEFINE_RES_IRQ(DC_TI_PWRBTN),
+};
+
+static struct resource thermal_resources[] = {
+ DEFINE_RES_IRQ(DC_TI_DIETMPWARN),
+};
+
+static struct resource adc_resources[] = {
+ DEFINE_RES_IRQ(DC_TI_ADCCMPL),
+};
+
+static struct resource pwrsrc_resources[] = {
+ DEFINE_RES_IRQ(DC_TI_VBUSDET),
+};
+
+static struct resource battery_resources[] = {
+ DEFINE_RES_IRQ(DC_TI_VBATLOW),
+ DEFINE_RES_IRQ(DC_TI_CCEOCAL),
+};
+
+static struct mfd_cell dc_ti_dev[] = {
+ {
+ .name = "dc_ti_pwrbtn",
+ .num_resources = ARRAY_SIZE(power_button_resources),
+ .resources = power_button_resources,
+ },
+ {
+ .name = "dc_ti_adc",
+ .num_resources = ARRAY_SIZE(adc_resources),
+ .resources = adc_resources,
+ },
+ {
+ .name = "dc_ti_thermal",
+ .num_resources = ARRAY_SIZE(thermal_resources),
+ .resources = thermal_resources,
+ },
+ {
+ .name = "dc_ti_pwrsrc",
+ .num_resources = ARRAY_SIZE(pwrsrc_resources),
+ .resources = pwrsrc_resources,
+ },
+ {
+ .name = "dc_ti_battery",
+ .num_resources = ARRAY_SIZE(battery_resources),
+ .resources = battery_resources,
+ },
+ {
+ .name = "dc_ti_region",
+ },
+};
+
+static const struct regmap_config dc_ti_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 128,
+ .cache_type = REGCACHE_NONE,
+};
+
+static const struct regmap_irq dc_ti_irqs[] = {
+ REGMAP_IRQ_REG(DC_TI_PWRBTN, 0, BIT(DC_TI_PWRBTN)),
+ REGMAP_IRQ_REG(DC_TI_DIETMPWARN, 0, BIT(DC_TI_DIETMPWARN)),
+ REGMAP_IRQ_REG(DC_TI_ADCCMPL, 0, BIT(DC_TI_ADCCMPL)),
+ REGMAP_IRQ_REG(DC_TI_VBATLOW, 0, BIT(DC_TI_VBATLOW)),
+ REGMAP_IRQ_REG(DC_TI_VBUSDET, 0, BIT(DC_TI_VBUSDET)),
+ REGMAP_IRQ_REG(DC_TI_CCEOCAL, 0, BIT(DC_TI_CCEOCAL)),
+};
+
+static const struct regmap_irq_chip dc_ti_irq_chip = {
+ .name = KBUILD_MODNAME,
+ .irqs = dc_ti_irqs,
+ .num_irqs = ARRAY_SIZE(dc_ti_irqs),
+ .num_regs = 1,
+ .status_base = DC_TI_IRQLVL1,
+ .mask_base = DC_TI_MASK_IRQLVL1,
+ .ack_base = DC_TI_IRQLVL1,
+};
+
+static int dc_ti_probe(struct i2c_client *i2c)
+{
+ struct device *dev = &i2c->dev;
+ struct intel_soc_pmic *pmic;
+ int ret;
+
+ pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+ if (!pmic)
+ return -ENOMEM;
+
+ i2c_set_clientdata(i2c, pmic);
+
+ pmic->regmap = devm_regmap_init_i2c(i2c, &dc_ti_regmap_config);
+ if (IS_ERR(pmic->regmap))
+ return PTR_ERR(pmic->regmap);
+ pmic->irq = i2c->irq;
+
+ ret = devm_regmap_add_irq_chip(dev, pmic->regmap, pmic->irq,
+ IRQF_ONESHOT, 0,
+ &dc_ti_irq_chip, &pmic->irq_chip_data);
+ if (ret)
+ return ret;
+
+ return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, dc_ti_dev,
+ ARRAY_SIZE(dc_ti_dev), NULL, 0,
+ regmap_irq_get_domain(pmic->irq_chip_data));
+}
+
+static void dc_ti_shutdown(struct i2c_client *i2c)
+{
+ struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);
+
+ disable_irq(pmic->irq);
+}
+
+static int __maybe_unused dc_ti_suspend(struct device *dev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+ disable_irq(pmic->irq);
+ return 0;
+}
+
+static int __maybe_unused dc_ti_resume(struct device *dev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+ enable_irq(pmic->irq);
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(dc_ti_pm_ops, dc_ti_suspend, dc_ti_resume);
+
+static const struct i2c_device_id dc_ti_i2c_id[] = {
+ { }
+};
+
+static const struct acpi_device_id dc_ti_acpi_ids[] = {
+ { "INT33F5" },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, dc_ti_acpi_ids);
+
+static struct i2c_driver dc_ti_i2c_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .pm = &dc_ti_pm_ops,
+ .acpi_match_table = ACPI_PTR(dc_ti_acpi_ids),
+ },
+ .probe_new = dc_ti_probe,
+ .shutdown = dc_ti_shutdown,
+ .id_table = dc_ti_i2c_id,
+};
+
+module_i2c_driver(dc_ti_i2c_driver);
+
+MODULE_DESCRIPTION("I2C driver for Intel SoC Dollar Cove TI PMIC");
+MODULE_LICENSE("GPL v2");
--
2.14.0
This provides a new input driver for supporting the power button on
Dollar Cove TI PMIC, found on Cherrytrail-based devices.
The patch is based on the original work by Intel, found at:
https://github.com/01org/ProductionKernelQuilts
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/input/keyboard/Kconfig | 7 +++
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+)
create mode 100644 drivers/input/keyboard/dc_ti_pwrbtn.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 4c4ab1ced235..673748b3cc34 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -756,4 +756,11 @@ config KEYBOARD_BCM
To compile this driver as a module, choose M here: the
module will be called bcm-keypad.
+config KEYBOARD_DC_TI_PWRBTN
+ tristate "Dollar Cove TI power button driver"
+ depends on INTEL_SOC_PMIC_DC_TI
+ help
+ Say Y here fi you want to have a power button driver for
+ Dollar Cove TI PMIC.
+
endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index d2338bacdad1..fa473d241e5c 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -66,3 +66,4 @@ obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
+obj-$(CONFIG_KEYBOARD_DC_TI_PWRBTN) += dc_ti_pwrbtn.o
diff --git a/drivers/input/keyboard/dc_ti_pwrbtn.c b/drivers/input/keyboard/dc_ti_pwrbtn.c
new file mode 100644
index 000000000000..a0900a440c92
--- /dev/null
+++ b/drivers/input/keyboard/dc_ti_pwrbtn.c
@@ -0,0 +1,85 @@
+/*
+ * Power button driver for Dollar Cove TI PMIC
+ * Copyright (C) 2014 Intel Corp
+ * Copyright (c) 2017 Takashi Iwai <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/mfd/intel_soc_pmic.h>
+
+#define DC_TI_SIRQ_REG 0x3
+#define SIRQ_PWRBTN_REL (1 << 0)
+
+#define DRIVER_NAME "dc_ti_pwrbtn"
+
+static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id)
+{
+ struct input_dev *pwrbtn_input = dev_id;
+ struct device *dev = pwrbtn_input->dev.parent;
+ struct regmap *regmap = dev_get_drvdata(dev);
+ int state;
+
+ if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) {
+ dev_dbg(dev, "SIRQ_REG=0x%x\n", state);
+ state &= SIRQ_PWRBTN_REL;
+ input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state);
+ input_sync(pwrbtn_input);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int dc_ti_pwrbtn_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
+ struct input_dev *pwrbtn_input;
+ int irq;
+ int ret;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return -EINVAL;
+ pwrbtn_input = devm_input_allocate_device(dev);
+ if (!pwrbtn_input)
+ return -ENOMEM;
+ pwrbtn_input->name = pdev->name;
+ pwrbtn_input->phys = "dc-ti-power/input0";
+ pwrbtn_input->id.bustype = BUS_HOST;
+ pwrbtn_input->dev.parent = dev;
+ input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER);
+ ret = input_register_device(pwrbtn_input);
+ if (ret)
+ return ret;
+
+ dev_set_drvdata(dev, pmic->regmap);
+
+ ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt,
+ 0, KBUILD_MODNAME, pwrbtn_input);
+ if (ret)
+ return ret;
+
+ ret = enable_irq_wake(irq);
+ if (ret)
+ dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
+
+ return 0;
+}
+
+static struct platform_driver dc_ti_pwrbtn_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+ .probe = dc_ti_pwrbtn_probe,
+};
+
+module_platform_driver(dc_ti_pwrbtn_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);
--
2.14.0
On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> This patch adds the MFD driver for Dollar Cove TI PMIC (ACPI INT33F5)
> that is found on some Intel Cherry Trail devices.
> The driver is based on the original work by Intel, found at:
> https://github.com/01org/ProductionKernelQuilts
>
> This is a minimal version for adding the basic resources. Currently,
> only ACPI PMIC opregion and the external power-button are used.
Overall looks good. Few comments below.
I hope Hans de Goede had been involved somehow to this. He did enormous
work to examine all this PMIC/ACPI/I2C stuff on CHT platforms.
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/acpi.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
Alphabetical order?
> +static const struct i2c_device_id dc_ti_i2c_id[] = {
> + { }
> +};
No need anymore. Moreover, you already are using ->probe_new().
> +static struct i2c_driver dc_ti_i2c_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .pm = &dc_ti_pm_ops,
> + .acpi_match_table = ACPI_PTR(dc_ti_acpi_ids),
ACPI_PTR is redundant here, driver is solely for ACPI case.
> + },
> + .probe_new = dc_ti_probe,
> + .shutdown = dc_ti_shutdown,
> + .id_table = dc_ti_i2c_id,
> +};
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> This provides a new input driver for supporting the power button on
> Dollar Cove TI PMIC, found on Cherrytrail-based devices.
> The patch is based on the original work by Intel, found at:
> https://github.com/01org/ProductionKernelQuilts
For now we have similar driver under drivers/platform/x86.
I think when we move to regmap API there we might even reuse
intel_mid_powerbtn.c.
Or other way around, move that one to input subsystem (which I
personally see less fit).
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> This patch adds the opregion driver for Dollar Cove TI PMIC on Intel
> Cherry Trail devices. The patch is based on the original work by
> Intel, found at:
> https://github.com/01org/ProductionKernelQuilts
> with many cleanups and rewrites.
>
> The driver is currently provided only as built-in to follow other
> PMIC opregion drivers convention.
>
> The re-enumeration of devices at probe is required for fixing the
> issues on HP x2 210 G2. See bug#195689.
>
>
> +static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
> +{
> + int temp_l, temp_h;
> +
> + if (regmap_read(regmap, reg, &temp_l) ||
> + regmap_read(regmap, reg - 1, &temp_h))
> + return -EIO;
> +
> + return temp_l | (temp_h & 0x3) << 8;
> +}
I'm not sure I understand this "- 1" part along with choice of l and h
suffixes.
Does it mean the register is big endian?
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 22 Aug 2017 11:58:35 +0200,
Andy Shevchenko wrote:
>
> On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > This patch adds the opregion driver for Dollar Cove TI PMIC on Intel
> > Cherry Trail devices. The patch is based on the original work by
> > Intel, found at:
> > https://github.com/01org/ProductionKernelQuilts
> > with many cleanups and rewrites.
> >
> > The driver is currently provided only as built-in to follow other
> > PMIC opregion drivers convention.
> >
> > The re-enumeration of devices at probe is required for fixing the
> > issues on HP x2 210 G2. See bug#195689.
> >
> >
>
> > +static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
> > +{
> > + int temp_l, temp_h;
> > +
> > + if (regmap_read(regmap, reg, &temp_l) ||
> > + regmap_read(regmap, reg - 1, &temp_h))
> > + return -EIO;
> > +
> > + return temp_l | (temp_h & 0x3) << 8;
> > +}
>
> I'm not sure I understand this "- 1" part along with choice of l and h
> suffixes.
>
> Does it mean the register is big endian?
Good point, I need to check the original code and the values.
This can be a typo of '+', of course.
thanks,
Takashi
On Tue, 22 Aug 2017 11:37:45 +0200,
Andy Shevchenko wrote:
>
> On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > This patch adds the MFD driver for Dollar Cove TI PMIC (ACPI INT33F5)
> > that is found on some Intel Cherry Trail devices.
> > The driver is based on the original work by Intel, found at:
> > https://github.com/01org/ProductionKernelQuilts
> >
> > This is a minimal version for adding the basic resources. Currently,
> > only ACPI PMIC opregion and the external power-button are used.
>
> Overall looks good. Few comments below.
>
> I hope Hans de Goede had been involved somehow to this. He did enormous
> work to examine all this PMIC/ACPI/I2C stuff on CHT platforms.
Sure, he was of a great help, although he didn't touch directly about
these codes.
> > +#include <linux/module.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/acpi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/intel_soc_pmic.h>
>
> Alphabetical order?
OK, will rearrange.
Also, gpio stuff was removed from this version, so it's superfluous.
Will rip off.
> > +static const struct i2c_device_id dc_ti_i2c_id[] = {
> > + { }
> > +};
>
> No need anymore. Moreover, you already are using ->probe_new().
OK.
> > +static struct i2c_driver dc_ti_i2c_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .pm = &dc_ti_pm_ops,
> > + .acpi_match_table = ACPI_PTR(dc_ti_acpi_ids),
>
> ACPI_PTR is redundant here, driver is solely for ACPI case.
Right.
thanks,
Takashi
On Tue, 22 Aug 2017 11:42:08 +0200,
Andy Shevchenko wrote:
>
> On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > This provides a new input driver for supporting the power button on
> > Dollar Cove TI PMIC, found on Cherrytrail-based devices.
> > The patch is based on the original work by Intel, found at:
> > https://github.com/01org/ProductionKernelQuilts
>
> For now we have similar driver under drivers/platform/x86.
>
> I think when we move to regmap API there we might even reuse
> intel_mid_powerbtn.c.
Ah, OK, that can be a better place. I'll move to there.
Also, from comparison with intel_mid_powerbtn.c, I noticed that the
irq wake isn't cleared at removal. Will address it as well.
> Or other way around, move that one to input subsystem (which I
> personally see less fit).
I don't think it's worth, either.
I updated the patches and now pushed to topic/dollar-cove-ti-4.13-v2
branch. Will resubmit v2 (tomorrow or later) once after gathering
reviews.
thanks,
Takashi
On Tue, 22 Aug 2017 12:25:12 +0200,
Takashi Iwai wrote:
>
> On Tue, 22 Aug 2017 11:58:35 +0200,
> Andy Shevchenko wrote:
> >
> > On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > > This patch adds the opregion driver for Dollar Cove TI PMIC on Intel
> > > Cherry Trail devices. The patch is based on the original work by
> > > Intel, found at:
> > > https://github.com/01org/ProductionKernelQuilts
> > > with many cleanups and rewrites.
> > >
> > > The driver is currently provided only as built-in to follow other
> > > PMIC opregion drivers convention.
> > >
> > > The re-enumeration of devices at probe is required for fixing the
> > > issues on HP x2 210 G2. See bug#195689.
> > >
> > >
> >
> > > +static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
> > > +{
> > > + int temp_l, temp_h;
> > > +
> > > + if (regmap_read(regmap, reg, &temp_l) ||
> > > + regmap_read(regmap, reg - 1, &temp_h))
> > > + return -EIO;
> > > +
> > > + return temp_l | (temp_h & 0x3) << 8;
> > > +}
> >
> > I'm not sure I understand this "- 1" part along with choice of l and h
> > suffixes.
> >
> > Does it mean the register is big endian?
>
> Good point, I need to check the original code and the values.
It's really big-endian, the order is hi:lo.
But, admittedly, the temperature code hasn't been tested, and it's
possibly missing something. So I'm fine to drop that part in the
first version, too.
thanks,
Takashi
On Tue, 2017-08-22 at 13:01 +0200, Takashi Iwai wrote:
> On Tue, 22 Aug 2017 12:25:12 +0200,
> Takashi Iwai wrote:
> >
> > On Tue, 22 Aug 2017 11:58:35 +0200,
> > Andy Shevchenko wrote:
> > >
> > > On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > > > This patch adds the opregion driver for Dollar Cove TI PMIC on
> > > > Intel
> > > > Cherry Trail devices. The patch is based on the original work
> > > > by
> > > > Intel, found at:
> > > > https://github.com/01org/ProductionKernelQuilts
> > > > with many cleanups and rewrites.
> > > >
> > > > The driver is currently provided only as built-in to follow
> > > > other
> > > > PMIC opregion drivers convention.
> > > >
> > > > The re-enumeration of devices at probe is required for fixing
> > > > the
> > > > issues on HP x2 210 G2. See bug#195689.
> > > >
> > > >
> > > > +static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int
> > > > reg)
> > > > +{
> > > > + int temp_l, temp_h;
> > > > +
> > > > + if (regmap_read(regmap, reg, &temp_l) ||
> > > > + regmap_read(regmap, reg - 1, &temp_h))
> > > > + return -EIO;
> > > > +
> > > > + return temp_l | (temp_h & 0x3) << 8;
> > > > +}
> > >
> > > I'm not sure I understand this "- 1" part along with choice of l
> > > and h
> > > suffixes.
> > >
> > > Does it mean the register is big endian?
> >
> > Good point, I need to check the original code and the values.
>
> It's really big-endian, the order is hi:lo.
>
> But, admittedly, the temperature code hasn't been tested, and it's
> possibly missing something. So I'm fine to drop that part in the
> first version, too.
I don't know if regmap allows you to define registers with different
sizes for same chip, perhaps it would make sense to start register from
hi part (and not doing non-intuitive "- 1", or maybe "+ 1" instead) and
mark it in comment that is BE16.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 22 Aug 2017 13:37:21 +0200,
Andy Shevchenko wrote:
>
> On Tue, 2017-08-22 at 13:01 +0200, Takashi Iwai wrote:
> > On Tue, 22 Aug 2017 12:25:12 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Tue, 22 Aug 2017 11:58:35 +0200,
> > > Andy Shevchenko wrote:
> > > >
> > > > On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote:
> > > > > This patch adds the opregion driver for Dollar Cove TI PMIC on
> > > > > Intel
> > > > > Cherry Trail devices. The patch is based on the original work
> > > > > by
> > > > > Intel, found at:
> > > > > https://github.com/01org/ProductionKernelQuilts
> > > > > with many cleanups and rewrites.
> > > > >
> > > > > The driver is currently provided only as built-in to follow
> > > > > other
> > > > > PMIC opregion drivers convention.
> > > > >
> > > > > The re-enumeration of devices at probe is required for fixing
> > > > > the
> > > > > issues on HP x2 210 G2. See bug#195689.
> > > > >
> > > > >
> > > > > +static int dc_ti_pmic_get_raw_temp(struct regmap *regmap, int
> > > > > reg)
> > > > > +{
> > > > > + int temp_l, temp_h;
> > > > > +
> > > > > + if (regmap_read(regmap, reg, &temp_l) ||
> > > > > + regmap_read(regmap, reg - 1, &temp_h))
> > > > > + return -EIO;
> > > > > +
> > > > > + return temp_l | (temp_h & 0x3) << 8;
> > > > > +}
> > > >
> > > > I'm not sure I understand this "- 1" part along with choice of l
> > > > and h
> > > > suffixes.
> > > >
> > > > Does it mean the register is big endian?
> > >
> > > Good point, I need to check the original code and the values.
> >
> > It's really big-endian, the order is hi:lo.
> >
> > But, admittedly, the temperature code hasn't been tested, and it's
> > possibly missing something. So I'm fine to drop that part in the
> > first version, too.
>
> I don't know if regmap allows you to define registers with different
> sizes for same chip, perhaps it would make sense to start register from
> hi part (and not doing non-intuitive "- 1", or maybe "+ 1" instead) and
> mark it in comment that is BE16.
I don't think regmap would allow different bit size, so combining
still needed. But yeah, it's better to start from the high register
with the lower address and use +1.
thanks,
Takashi
On Tue, 2017-08-22 at 14:37 +0300, Andy Shevchenko wrote:
> On Tue, 2017-08-22 at 13:01 +0200, Takashi Iwai wrote:
> > On Tue, 22 Aug 2017 12:25:12 +0200,
> > Takashi Iwai wrote:
> > > > Does it mean the register is big endian?
> > >
> > > Good point, I need to check the original code and the values.
> >
> > It's really big-endian, the order is hi:lo.
> >
> > But, admittedly, the temperature code hasn't been tested, and it's
> > possibly missing something. So I'm fine to drop that part in the
> > first version, too.
>
> I don't know if regmap allows you to define registers with different
> sizes for same chip, perhaps it would make sense to start register
> from
> hi part (and not doing non-intuitive "- 1", or maybe "+ 1" instead)
> and
> mark it in comment that is BE16.
I have just checked datasheet, so, there are 4 pairs of BE16 registers.
VBAT (Hi:Lo) 0x54
DIETEMP 0x56
BPTHERM 0x58
GPADC 0x5a
So, I would create a separate address mapping for them, dropping out
that _LOW suffix and put a comment that they are BE16 since ADC has 10-
bit resolution.
Or even do a separate ADC driver under drivers/iio/adc for PMIC(s).
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 22 Aug 2017 14:08:27 +0200,
Andy Shevchenko wrote:
>
> On Tue, 2017-08-22 at 14:37 +0300, Andy Shevchenko wrote:
> > On Tue, 2017-08-22 at 13:01 +0200, Takashi Iwai wrote:
> > > On Tue, 22 Aug 2017 12:25:12 +0200,
> > > Takashi Iwai wrote:
>
> > > > > Does it mean the register is big endian?
> > > >
> > > > Good point, I need to check the original code and the values.
> > >
> > > It's really big-endian, the order is hi:lo.
> > >
> > > But, admittedly, the temperature code hasn't been tested, and it's
> > > possibly missing something. So I'm fine to drop that part in the
> > > first version, too.
> >
> > I don't know if regmap allows you to define registers with different
> > sizes for same chip, perhaps it would make sense to start register
> > from
> > hi part (and not doing non-intuitive "- 1", or maybe "+ 1" instead)
> > and
> > mark it in comment that is BE16.
>
> I have just checked datasheet, so, there are 4 pairs of BE16 registers.
>
> VBAT (Hi:Lo) 0x54
> DIETEMP 0x56
> BPTHERM 0x58
> GPADC 0x5a
>
> So, I would create a separate address mapping for them, dropping out
> that _LOW suffix and put a comment that they are BE16 since ADC has 10-
> bit resolution.
Yep, done in that way now. Also I changed the register names to
follow your reference.
> Or even do a separate ADC driver under drivers/iio/adc for PMIC(s).
That's another option, but I took an easier path as the first step.
thanks,
Takashi
On Tue, 2017-08-22 at 14:26 +0200, Takashi Iwai wrote:
> On Tue, 22 Aug 2017 14:08:27 +0200,
> Andy Shevchenko wrote:
> >
>
> > Or even do a separate ADC driver under drivers/iio/adc for PMIC(s).
>
> That's another option, but I took an easier path as the first step.
Looking to the current pattern we have ADC being referred from ACPI PMIC
OpRegion drivers, so, I have to withdraw my proposal.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, Aug 22, 2017 at 12:58:07PM +0200, Takashi Iwai wrote:
> I updated the patches and now pushed to topic/dollar-cove-ti-4.13-v2
> branch. Will resubmit v2 (tomorrow or later) once after gathering
> reviews.
FWIW I tested current Linus's master + topic/dollar-cove-ti-4.13-v2
+ topic/soc-cx2072x-4.13 + my test patches, no observable
difference to topic/dollar-cove-ti-4.13 on E200HA.
Still hoping someone would give me a hint about possible
causes for the SoC entering S0i1 only instead of S0i3?
(https://bugzilla.kernel.org/show_bug.cgi?id=193891)
Where do I start looking?
Thanks,
Johannes
Hi Takashi,
On Tue, Aug 22, 2017 at 07:57:09AM +0200, Takashi Iwai wrote:
> This provides a new input driver for supporting the power button on
> Dollar Cove TI PMIC, found on Cherrytrail-based devices.
> The patch is based on the original work by Intel, found at:
> https://github.com/01org/ProductionKernelQuilts
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> drivers/input/keyboard/Kconfig | 7 +++
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++
Not sure if it ends up in drivers/input/keyboard, or drivers/input/misc/
(where most power buttons live) or in platform drivers, still a few
comments below.
> 3 files changed, 93 insertions(+)
> create mode 100644 drivers/input/keyboard/dc_ti_pwrbtn.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 4c4ab1ced235..673748b3cc34 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -756,4 +756,11 @@ config KEYBOARD_BCM
> To compile this driver as a module, choose M here: the
> module will be called bcm-keypad.
>
> +config KEYBOARD_DC_TI_PWRBTN
> + tristate "Dollar Cove TI power button driver"
> + depends on INTEL_SOC_PMIC_DC_TI
> + help
> + Say Y here fi you want to have a power button driver for
> + Dollar Cove TI PMIC.
If keeping in input we customarily call out the module name (see a few
lines above).
> +
> endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index d2338bacdad1..fa473d241e5c 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -66,3 +66,4 @@ obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
> obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
> obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
> +obj-$(CONFIG_KEYBOARD_DC_TI_PWRBTN) += dc_ti_pwrbtn.o
> diff --git a/drivers/input/keyboard/dc_ti_pwrbtn.c b/drivers/input/keyboard/dc_ti_pwrbtn.c
> new file mode 100644
> index 000000000000..a0900a440c92
> --- /dev/null
> +++ b/drivers/input/keyboard/dc_ti_pwrbtn.c
> @@ -0,0 +1,85 @@
> +/*
> + * Power button driver for Dollar Cove TI PMIC
> + * Copyright (C) 2014 Intel Corp
> + * Copyright (c) 2017 Takashi Iwai <[email protected]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/input.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +
> +#define DC_TI_SIRQ_REG 0x3
> +#define SIRQ_PWRBTN_REL (1 << 0)
BIT()?
> +
> +#define DRIVER_NAME "dc_ti_pwrbtn"
> +
> +static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id)
> +{
> + struct input_dev *pwrbtn_input = dev_id;
> + struct device *dev = pwrbtn_input->dev.parent;
> + struct regmap *regmap = dev_get_drvdata(dev);
> + int state;
> +
> + if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) {
> + dev_dbg(dev, "SIRQ_REG=0x%x\n", state);
> + state &= SIRQ_PWRBTN_REL;
> + input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state);
Why not
input_report_key(pwrbtn_input, KEY_POWER,
!(state & SIRQ_PWRBTN_REL));
> + input_sync(pwrbtn_input);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int dc_ti_pwrbtn_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> + struct input_dev *pwrbtn_input;
> + int irq;
> + int ret;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -EINVAL;
Why do you clobber return value? Simply return "irq".
> + pwrbtn_input = devm_input_allocate_device(dev);
> + if (!pwrbtn_input)
> + return -ENOMEM;
> + pwrbtn_input->name = pdev->name;
> + pwrbtn_input->phys = "dc-ti-power/input0";
> + pwrbtn_input->id.bustype = BUS_HOST;
> + pwrbtn_input->dev.parent = dev;
Not needed since devm_input_allocate_device() does it for us.
> + input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER);
> + ret = input_register_device(pwrbtn_input);
> + if (ret)
> + return ret;
If staying in input, can we please call this variable err or error?
> +
> + dev_set_drvdata(dev, pmic->regmap);
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt,
> + 0, KBUILD_MODNAME, pwrbtn_input);
> + if (ret)
> + return ret;
> +
> + ret = enable_irq_wake(irq);
> + if (ret)
> + dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
We do not normally enable wake IRQs in probe, but instead do:
device_init_wakeup(&pdev->dev, true);
in probe() and then check it in suspend/resume:
if (device_may_wakeup(dev)) {
err = enable_irq_wake(XXX->irq);
if (!err)
XXX->irq_wake_enabled = true;
}
...
if (XXX->irq_wake_enabled)
disable_irq_wake(XXX->irq);
This allows userspace to inhibit wakeup, if needed.
> +
> + return 0;
> +}
> +
> +static struct platform_driver dc_ti_pwrbtn_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + },
> + .probe = dc_ti_pwrbtn_probe,
> +};
> +
> +module_platform_driver(dc_ti_pwrbtn_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> --
> 2.14.0
>
Thanks!
--
Dmitry
On Thu, 31 Aug 2017 20:33:55 +0200,
Dmitry Torokhov wrote:
>
> Hi Takashi,
>
> On Tue, Aug 22, 2017 at 07:57:09AM +0200, Takashi Iwai wrote:
> > This provides a new input driver for supporting the power button on
> > Dollar Cove TI PMIC, found on Cherrytrail-based devices.
> > The patch is based on the original work by Intel, found at:
> > https://github.com/01org/ProductionKernelQuilts
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > drivers/input/keyboard/Kconfig | 7 +++
> > drivers/input/keyboard/Makefile | 1 +
> > drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++
>
> Not sure if it ends up in drivers/input/keyboard, or drivers/input/misc/
> (where most power buttons live) or in platform drivers, still a few
> comments below.
Oh sorry, I forgot to put you to Cc after v2 patch.
Per Andy's suggestion, the driver was moved to drivers/platform/x86
now. For v3 patchset, see the following thread.
https://marc.info/?i=20170825134443.14843-1-tiwai%40suse.de
> > +config KEYBOARD_DC_TI_PWRBTN
> > + tristate "Dollar Cove TI power button driver"
> > + depends on INTEL_SOC_PMIC_DC_TI
> > + help
> > + Say Y here fi you want to have a power button driver for
> > + Dollar Cove TI PMIC.
>
> If keeping in input we customarily call out the module name (see a few
> lines above).
I changed the driver and config names to follow the platform driver.
> > +#define DC_TI_SIRQ_REG 0x3
> > +#define SIRQ_PWRBTN_REL (1 << 0)
>
> BIT()?
OK.
> > +#define DRIVER_NAME "dc_ti_pwrbtn"
> > +
> > +static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id)
> > +{
> > + struct input_dev *pwrbtn_input = dev_id;
> > + struct device *dev = pwrbtn_input->dev.parent;
> > + struct regmap *regmap = dev_get_drvdata(dev);
> > + int state;
> > +
> > + if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) {
> > + dev_dbg(dev, "SIRQ_REG=0x%x\n", state);
> > + state &= SIRQ_PWRBTN_REL;
> > + input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state);
>
> Why not
>
> input_report_key(pwrbtn_input, KEY_POWER,
> !(state & SIRQ_PWRBTN_REL));
Sounds better, indeed.
> > + input_sync(pwrbtn_input);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int dc_ti_pwrbtn_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> > + struct input_dev *pwrbtn_input;
> > + int irq;
> > + int ret;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return -EINVAL;
>
> Why do you clobber return value? Simply return "irq".
OK.
> > + pwrbtn_input = devm_input_allocate_device(dev);
> > + if (!pwrbtn_input)
> > + return -ENOMEM;
> > + pwrbtn_input->name = pdev->name;
> > + pwrbtn_input->phys = "dc-ti-power/input0";
> > + pwrbtn_input->id.bustype = BUS_HOST;
> > + pwrbtn_input->dev.parent = dev;
>
> Not needed since devm_input_allocate_device() does it for us.
OK.
> > + input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER);
> > + ret = input_register_device(pwrbtn_input);
> > + if (ret)
> > + return ret;
>
> If staying in input, can we please call this variable err or error?
OK, I don't mind to change it.
> > +
> > + dev_set_drvdata(dev, pmic->regmap);
> > +
> > + ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt,
> > + 0, KBUILD_MODNAME, pwrbtn_input);
> > + if (ret)
> > + return ret;
> > +
> > + ret = enable_irq_wake(irq);
> > + if (ret)
> > + dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
>
> We do not normally enable wake IRQs in probe, but instead do:
>
> device_init_wakeup(&pdev->dev, true);
> in probe()
Right, this was already changed in the later version.
But...
> and then check it in suspend/resume:
>
> if (device_may_wakeup(dev)) {
> err = enable_irq_wake(XXX->irq);
> if (!err)
> XXX->irq_wake_enabled = true;
> }
>
> ...
>
> if (XXX->irq_wake_enabled)
> disable_irq_wake(XXX->irq);
>
> This allows userspace to inhibit wakeup, if needed.
... this wasn't. Will put it.
Thank your for the review!
Takashi
On Thu, 2017-08-31 at 11:33 -0700, Dmitry Torokhov wrote:
> > +
> > + ret = enable_irq_wake(irq);
> > + if (ret)
> > + dev_warn(dev, "Can't enable IRQ as wake source:
> > %d\n", ret);
>
> We do not normally enable wake IRQs in probe, but instead do:
>
> device_init_wakeup(&pdev->dev, true);
>
> in probe() and then check it in suspend/resume:
>
> if (device_may_wakeup(dev)) {
> err = enable_irq_wake(XXX->irq);
> if (!err)
> XXX->irq_wake_enabled = true;
> }
>
> ...
>
> if (XXX->irq_wake_enabled)
No need to duplicate a flag which IRQ core already has.
See, for example, commit
aef3ad103a68 ("serial: core: remove unneeded irq_wake flag")
> disable_irq_wake(XXX->irq);
>
> This allows userspace to inhibit wakeup, if needed.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Fri, 01 Sep 2017 13:09:36 +0200,
Andy Shevchenko wrote:
>
> On Thu, 2017-08-31 at 11:33 -0700, Dmitry Torokhov wrote:
>
> > > +
> > > + ret = enable_irq_wake(irq);
> > > + if (ret)
> > > + dev_warn(dev, "Can't enable IRQ as wake source:
> > > %d\n", ret);
> >
> > We do not normally enable wake IRQs in probe, but instead do:
> >
> > device_init_wakeup(&pdev->dev, true);
> >
> > in probe() and then check it in suspend/resume:
> >
> > if (device_may_wakeup(dev)) {
> > err = enable_irq_wake(XXX->irq);
> > if (!err)
> > XXX->irq_wake_enabled = true;
> > }
> >
> > ...
> >
> > if (XXX->irq_wake_enabled)
>
> No need to duplicate a flag which IRQ core already has.
>
> See, for example, commit
> aef3ad103a68 ("serial: core: remove unneeded irq_wake flag")
I couldn't find the commit, but checked the web search.
So everything is set up and done by dev_pm_set_wake_irq(), no need to
create the own PM ops just for that, right? That stuff was already in
v3 patch.
thanks,
Takashi