[PATCH 0/3] Dollar Cove TI PMIC support for Intel Cherry Trail

[PATCH 0/3] Dollar Cove TI PMIC support for Intel Cherry Trail

[Takashi Iwai]

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

[PATCH 1/3] mfd: Add support for Cherry Trail Dollar Cove TI PMIC

[Takashi Iwai]

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 <tiwai@suse.de>
---
 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 <ramakrishna.pallala@intel.com>
+ *
+ * Cleanup and forward-ported
+ *   Copyright (c) 2017 Takashi Iwai <tiwai@suse.de>
+ */
+
+#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

Re: [PATCH 1/3] mfd: Add support for Cherry Trail Dollar Cove TI PMIC

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/3] mfd: Add support for Cherry Trail Dollar Cove TI PMIC

[Takashi Iwai]

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

[PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Takashi Iwai]

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 <tiwai@suse.de>
---
 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 <tiwai@suse.de>
+ */
+
+#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

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Takashi Iwai]

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

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Johannes Stezenbach]

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

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Dmitry Torokhov]

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 <tiwai@suse.de>
> ---
>  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 <tiwai@suse.de>
> + */
> +
> +#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

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Takashi Iwai]

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 <tiwai@suse.de>
> > ---
> >  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

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] input/keyboard: Add support for Dollar Cove TI power button

[Takashi Iwai]

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

[PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Takashi Iwai]

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 <tiwai@suse.de>
---
 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 <tiwai@suse.de>
+ */
+
+#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

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Takashi Iwai]

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

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Takashi Iwai]

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

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Takashi Iwai]

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

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Takashi Iwai]

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

Re: [PATCH 3/3] ACPI / PMIC: Add opregion driver for Intel Dollar Cove TI PMIC

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy