[PATCH 0/4] mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell

[PATCH 0/4] mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell

[Hans de Goede]

Hi All,

While doing a hobby project getting the mainline kernel to work on some
x86 tablets which come with Android pre-installed and which clearly have
never been meant to run Windows (or another generic OS) I encountered an
interesting setup for the bq24190 charger chip IRQ, it is not connected
to the main Bay Trail SoC, instead it runs through a special
external-charger IRQ pin on the CRC PMIC.

This series adds support for that.

Lee, patch 4/4 is included mostly FYI. It depends on current pdx86/for-next
and I plan to merge it through the pdx86 tree.

Patches 1-3 can be merged through the MFD tree indepdently.

Regards,

Hans


Hans de Goede (4):
  mfd: intel_soc_pmic_crc: Sort cells by IRQ order
  mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell to BYT cells
  mfd: intel_soc_pmic_crc: Set main IRQ domain bus token to
    DOMAIN_BUS_NEXUS
  platform/x86: Add crystal_cove_charger driver

 drivers/mfd/intel_soc_pmic_core.c           |   4 +
 drivers/mfd/intel_soc_pmic_crc.c            |  35 +++--
 drivers/platform/x86/Makefile               |   2 +-
 drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++
 4 files changed, 180 insertions(+), 14 deletions(-)
 create mode 100644 drivers/platform/x86/crystal_cove_charger.c

-- 
2.33.1

[PATCH 1/4] mfd: intel_soc_pmic_crc: Sort cells by IRQ order

[Hans de Goede]

The cells for the Crystal Cove PMIC are already mostly sorted by
function / IRQ order. Move the ADC cell so that they are fully sorted.

Also move some of the resource definitions so that their order matches
the (new) order of the cells.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/intel_soc_pmic_crc.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 38acb20e2d60..574cb8f9c70d 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -28,18 +28,10 @@
 #define CRYSTAL_COVE_IRQ_GPIO		5
 #define CRYSTAL_COVE_IRQ_VHDMIOCP	6
 
-static const struct resource gpio_resources[] = {
-	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_GPIO, "GPIO"),
-};
-
 static const struct resource pwrsrc_resources[] = {
 	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_PWRSRC, "PWRSRC"),
 };
 
-static const struct resource adc_resources[] = {
-	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_ADC, "ADC"),
-};
-
 static const struct resource thermal_resources[] = {
 	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_THRM, "THERMAL"),
 };
@@ -48,17 +40,20 @@ static const struct resource bcu_resources[] = {
 	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_BCU, "BCU"),
 };
 
+static const struct resource adc_resources[] = {
+	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_ADC, "ADC"),
+};
+
+static const struct resource gpio_resources[] = {
+	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_GPIO, "GPIO"),
+};
+
 static struct mfd_cell crystal_cove_byt_dev[] = {
 	{
 		.name = "crystal_cove_pwrsrc",
 		.num_resources = ARRAY_SIZE(pwrsrc_resources),
 		.resources = pwrsrc_resources,
 	},
-	{
-		.name = "crystal_cove_adc",
-		.num_resources = ARRAY_SIZE(adc_resources),
-		.resources = adc_resources,
-	},
 	{
 		.name = "crystal_cove_thermal",
 		.num_resources = ARRAY_SIZE(thermal_resources),
@@ -69,6 +64,11 @@ static struct mfd_cell crystal_cove_byt_dev[] = {
 		.num_resources = ARRAY_SIZE(bcu_resources),
 		.resources = bcu_resources,
 	},
+	{
+		.name = "crystal_cove_adc",
+		.num_resources = ARRAY_SIZE(adc_resources),
+		.resources = adc_resources,
+	},
 	{
 		.name = "crystal_cove_gpio",
 		.num_resources = ARRAY_SIZE(gpio_resources),
-- 
2.33.1

Re: [PATCH 1/4] mfd: intel_soc_pmic_crc: Sort cells by IRQ order

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 1:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The cells for the Crystal Cove PMIC are already mostly sorted by
> function / IRQ order. Move the ADC cell so that they are fully sorted.
>
> Also move some of the resource definitions so that their order matches
> the (new) order of the cells.


...

>  #define CRYSTAL_COVE_IRQ_GPIO          5


>         {

As Lee commented once in p2sb patch series I think it makes sense here
as well, i.e.

[CRYSTAL_COVE_IRQ_GPIO] = {

>                 .name = "crystal_cove_gpio",
>                 .num_resources = ARRAY_SIZE(gpio_resources),

What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] mfd: intel_soc_pmic_crc: Sort cells by IRQ order

[Hans de Goede]

Hi Andy,

On 12/25/21 15:20, Andy Shevchenko wrote:
> On Sat, Dec 25, 2021 at 1:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The cells for the Crystal Cove PMIC are already mostly sorted by
>> function / IRQ order. Move the ADC cell so that they are fully sorted.
>>
>> Also move some of the resource definitions so that their order matches
>> the (new) order of the cells.
> 
> 
> ...
> 
>>  #define CRYSTAL_COVE_IRQ_GPIO          5
> 
> 
>>         {
> 
> As Lee commented once in p2sb patch series I think it makes sense here
> as well, i.e.
> 
> [CRYSTAL_COVE_IRQ_GPIO] = {
> 
>>                 .name = "crystal_cove_gpio",
>>                 .num_resources = ARRAY_SIZE(gpio_resources),
> 
> What do you think?

There are also cells without IRQs, e.g. for the ACPI PMIC OpRegion
driver to bind to. So we cannot (consistently) do this.

Regards,

Hans

Re: [PATCH 1/4] mfd: intel_soc_pmic_crc: Sort cells by IRQ order

[Lee Jones]

On Sat, 25 Dec 2021, Hans de Goede wrote:

> The cells for the Crystal Cove PMIC are already mostly sorted by
> function / IRQ order. Move the ADC cell so that they are fully sorted.
> 
> Also move some of the resource definitions so that their order matches
> the (new) order of the cells.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/mfd/intel_soc_pmic_crc.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/4] mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell to BYT cells

[Hans de Goede]

The Crystal Cove PMIC has a pin which can be used to connect the IRQ of
an external charger IC. On some boards this is used and we need to have
a cell for this, with a driver which creates its own irqchip with
a single IRQ for the charger driver to consume.

The charger driver cannot directly consume the IRQ from the MFD level
irqchip because the PMIC has 2 levels of interrupts and the second
level interrupt status register, which is handled by the cell drivers,
needs to have the IRQ acked to avoid an IRQ storm.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/intel_soc_pmic_crc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 574cb8f9c70d..5bb0367bd974 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -44,6 +44,10 @@ static const struct resource adc_resources[] = {
 	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_ADC, "ADC"),
 };
 
+static const struct resource charger_resources[] = {
+	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_CHGR, "CHGR"),
+};
+
 static const struct resource gpio_resources[] = {
 	DEFINE_RES_IRQ_NAMED(CRYSTAL_COVE_IRQ_GPIO, "GPIO"),
 };
@@ -69,6 +73,11 @@ static struct mfd_cell crystal_cove_byt_dev[] = {
 		.num_resources = ARRAY_SIZE(adc_resources),
 		.resources = adc_resources,
 	},
+	{
+		.name = "crystal_cove_charger",
+		.num_resources = ARRAY_SIZE(charger_resources),
+		.resources = charger_resources,
+	},
 	{
 		.name = "crystal_cove_gpio",
 		.num_resources = ARRAY_SIZE(gpio_resources),
-- 
2.33.1

Re: [PATCH 2/4] mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell to BYT cells

[Lee Jones]

On Sat, 25 Dec 2021, Hans de Goede wrote:

> The Crystal Cove PMIC has a pin which can be used to connect the IRQ of
> an external charger IC. On some boards this is used and we need to have
> a cell for this, with a driver which creates its own irqchip with
> a single IRQ for the charger driver to consume.
> 
> The charger driver cannot directly consume the IRQ from the MFD level
> irqchip because the PMIC has 2 levels of interrupts and the second
> level interrupt status register, which is handled by the cell drivers,
> needs to have the IRQ acked to avoid an IRQ storm.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/mfd/intel_soc_pmic_crc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4] mfd: intel_soc_pmic_crc: Set main IRQ domain bus token to DOMAIN_BUS_NEXUS

[Hans de Goede]

For the CRC PMIC we end up with multiple IRQ domains with the same fwnode.
One for the irqchip which demultiplexes the actual PMIC interrupt into
interrupts for the various cells (known as the level 1 interrupts);

And 2 more for the irqchips which are part of the crystal_cove_gpio
and crystal_cove_charger cells.

This leads to the following error being printed when
CONFIG_GENERIC_IRQ_DEBUGFS is enabled:
 debugfs: File '\_SB.I2C7.PMIC' in directory 'domains' already present!

Set the bus token of the main IRQ domain to DOMAIN_BUS_NEXUS to avoid
this error, this also allows irq_find_matching_fwspec() to find the
right domain if necessary.

Note all 3 domain registering drivers need to set the IRQ domain bus token.
This is necessary because the IRQ domain code defaults to creating
the debugfs dir with just the fwnode name and then renames it when
the bus token is set. So each one starts with the same default name and
all 3 must be given a different name to avoid problems when one of the
other drivers loads and starts with the same default name.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/intel_soc_pmic_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index 47cb7f00dfcf..5e8c94e008ed 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -64,6 +64,10 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 	/* Add lookup table for crc-pwm */
 	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 
+	/* To distuingish this domain from the GPIO/charger's irqchip domains */
+	irq_domain_update_bus_token(regmap_irq_get_domain(pmic->irq_chip_data),
+				    DOMAIN_BUS_NEXUS);
+
 	ret = mfd_add_devices(dev, -1, config->cell_dev,
 			      config->n_cell_devs, NULL, 0,
 			      regmap_irq_get_domain(pmic->irq_chip_data));
-- 
2.33.1

Re: [PATCH 3/4] mfd: intel_soc_pmic_crc: Set main IRQ domain bus token to DOMAIN_BUS_NEXUS

[Lee Jones]

On Sat, 25 Dec 2021, Hans de Goede wrote:

> For the CRC PMIC we end up with multiple IRQ domains with the same fwnode.
> One for the irqchip which demultiplexes the actual PMIC interrupt into
> interrupts for the various cells (known as the level 1 interrupts);
> 
> And 2 more for the irqchips which are part of the crystal_cove_gpio
> and crystal_cove_charger cells.
> 
> This leads to the following error being printed when
> CONFIG_GENERIC_IRQ_DEBUGFS is enabled:
>  debugfs: File '\_SB.I2C7.PMIC' in directory 'domains' already present!
> 
> Set the bus token of the main IRQ domain to DOMAIN_BUS_NEXUS to avoid
> this error, this also allows irq_find_matching_fwspec() to find the
> right domain if necessary.
> 
> Note all 3 domain registering drivers need to set the IRQ domain bus token.
> This is necessary because the IRQ domain code defaults to creating
> the debugfs dir with just the fwnode name and then renames it when
> the bus token is set. So each one starts with the same default name and
> all 3 must be given a different name to avoid problems when one of the
> other drivers loads and starts with the same default name.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/mfd/intel_soc_pmic_core.c | 4 ++++
>  1 file changed, 4 insertions(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] platform/x86: Add crystal_cove_charger driver

[Hans de Goede]

Driver for the external-charger IRQ pass-through function of the
Bay Trail Crystal Cove PMIC.

Note this is NOT a power_supply class driver, it just deals with IRQ
pass-through, this requires this separate driver because the PMIC's
level 2 interrupt for this must be explicitly acked.

This new driver gets enabled by the existing X86_ANDROID_TABLETS Kconfig
option because the x86-android-tablets module is the only user of the
exported external-charger IRQ.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Makefile               |   2 +-
 drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++
 2 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/crystal_cove_charger.c

diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index bd20b435c22b..cce124e3acab 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -114,7 +114,7 @@ obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
 obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
 obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
 obj-$(CONFIG_WIRELESS_HOTKEY)		+= wireless-hotkey.o
-obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets.o
+obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets.o crystal_cove_charger.o
 
 # Intel uncore drivers
 obj-$(CONFIG_INTEL_IPS)				+= intel_ips.o
diff --git a/drivers/platform/x86/crystal_cove_charger.c b/drivers/platform/x86/crystal_cove_charger.c
new file mode 100644
index 000000000000..382c19806b12
--- /dev/null
+++ b/drivers/platform/x86/crystal_cove_charger.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for the external-charger IRQ pass-through function of the
+ * Bay Trail Crystal Cove PMIC.
+ *
+ * Note this is NOT a power_supply class driver, it just deals with IRQ
+ * pass-through, this requires this separate driver because the PMIC's
+ * level 2 interrupt for this must be explicitly acked.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define CHGRIRQ_REG					0x0a
+
+struct crystal_cove_charger_data {
+	struct mutex buslock; /* irq_bus_lock */
+	struct irq_chip irqchip;
+	struct regmap *regmap;
+	struct irq_domain *irq_domain;
+	int irq;
+	int charger_irq;
+	bool irq_enabled;
+	bool irq_is_enabled;
+};
+
+static irqreturn_t crystal_cove_charger_irq(int irq, void *data)
+{
+	struct crystal_cove_charger_data *charger = data;
+
+	/* No need to read CHGRIRQ_REG as there is only 1 IRQ */
+	handle_nested_irq(charger->charger_irq);
+
+	/* Ack CHGRIRQ 0 */
+	regmap_write(charger->regmap, CHGRIRQ_REG, BIT(0));
+
+	return IRQ_HANDLED;
+}
+
+static void crystal_cove_charger_irq_bus_lock(struct irq_data *data)
+{
+	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&charger->buslock);
+}
+
+static void crystal_cove_charger_irq_bus_sync_unlock(struct irq_data *data)
+{
+	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
+
+	if (charger->irq_is_enabled != charger->irq_enabled) {
+		if (charger->irq_enabled)
+			enable_irq(charger->irq);
+		else
+			disable_irq(charger->irq);
+
+		charger->irq_is_enabled = charger->irq_enabled;
+	}
+
+	mutex_unlock(&charger->buslock);
+}
+
+static void crystal_cove_charger_irq_unmask(struct irq_data *data)
+{
+	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
+
+	charger->irq_enabled = true;
+}
+
+static void crystal_cove_charger_irq_mask(struct irq_data *data)
+{
+	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
+
+	charger->irq_enabled = false;
+}
+
+static void crystal_cove_charger_rm_irq_domain(void *data)
+{
+	struct crystal_cove_charger_data *charger = data;
+
+	irq_domain_remove(charger->irq_domain);
+}
+
+static int crystal_cove_charger_probe(struct platform_device *pdev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	struct crystal_cove_charger_data *charger;
+	int ret;
+
+	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
+	charger->regmap = pmic->regmap;
+	mutex_init(&charger->buslock);
+
+	charger->irq = platform_get_irq(pdev, 0);
+	if (charger->irq < 0)
+		return charger->irq;
+
+	charger->irq_domain = irq_domain_create_linear(dev_fwnode(pdev->dev.parent), 1,
+						       &irq_domain_simple_ops, NULL);
+	if (!charger->irq_domain)
+		return -ENOMEM;
+
+	/* Distuingish IRQ domain from others sharing (MFD) the same fwnode */
+	irq_domain_update_bus_token(charger->irq_domain, DOMAIN_BUS_WAKEUP);
+
+	ret = devm_add_action_or_reset(&pdev->dev, crystal_cove_charger_rm_irq_domain, charger);
+	if (ret)
+		return ret;
+
+	charger->charger_irq = irq_create_mapping(charger->irq_domain, 0);
+	if (!charger->charger_irq)
+		return -ENOMEM;
+
+	charger->irqchip.name = KBUILD_MODNAME;
+	charger->irqchip.irq_unmask = crystal_cove_charger_irq_unmask;
+	charger->irqchip.irq_mask = crystal_cove_charger_irq_mask;
+	charger->irqchip.irq_bus_lock = crystal_cove_charger_irq_bus_lock;
+	charger->irqchip.irq_bus_sync_unlock = crystal_cove_charger_irq_bus_sync_unlock;
+
+	irq_set_chip_data(charger->charger_irq, charger);
+	irq_set_chip_and_handler(charger->charger_irq, &charger->irqchip, handle_simple_irq);
+	irq_set_nested_thread(charger->charger_irq, true);
+	irq_set_noprobe(charger->charger_irq);
+
+	ret = devm_request_threaded_irq(&pdev->dev, charger->irq, NULL,
+					crystal_cove_charger_irq,
+					IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					KBUILD_MODNAME, charger);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "requesting irq\n");
+
+	return 0;
+}
+
+static struct platform_driver crystal_cove_charger_driver = {
+	.probe = crystal_cove_charger_probe,
+	.driver = {
+		.name = "crystal_cove_charger",
+	},
+};
+module_platform_driver(crystal_cove_charger_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com");
+MODULE_DESCRIPTION("Bay Trail Crystal Cove external charger IRQ pass-through");
+MODULE_LICENSE("GPL");
-- 
2.33.1

Re: [PATCH 4/4] platform/x86: Add crystal_cove_charger driver

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 1:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Driver for the external-charger IRQ pass-through function of the
> Bay Trail Crystal Cove PMIC.

Intel Bay Trail (same in the code)

> Note this is NOT a power_supply class driver, it just deals with IRQ
> pass-through, this requires this separate driver because the PMIC's
> level 2 interrupt for this must be explicitly acked.
>
> This new driver gets enabled by the existing X86_ANDROID_TABLETS Kconfig
> option because the x86-android-tablets module is the only user of the
> exported external-charger IRQ.

...

>  drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++

I'm wondering why it's not under the intel/ subfolder. Do we expect to
have the same PMIC used on other x86 vendors?

...

> +static int crystal_cove_charger_probe(struct platform_device *pdev)
> +{

Adding

  struct device *dev = &pdev->dev;

may increase readability a bit and perhaps reduce the amount of LOCs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] platform/x86: Add crystal_cove_charger driver

[Hans de Goede]

Hi,

On 12/25/21 15:37, Andy Shevchenko wrote:
> On Sat, Dec 25, 2021 at 1:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Driver for the external-charger IRQ pass-through function of the
>> Bay Trail Crystal Cove PMIC.
> 
> Intel Bay Trail (same in the code)

Ack will fix before merging.

>> Note this is NOT a power_supply class driver, it just deals with IRQ
>> pass-through, this requires this separate driver because the PMIC's
>> level 2 interrupt for this must be explicitly acked.
>>
>> This new driver gets enabled by the existing X86_ANDROID_TABLETS Kconfig
>> option because the x86-android-tablets module is the only user of the
>> exported external-charger IRQ.
> 
> ...
> 
>>  drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++
> 
> I'm wondering why it's not under the intel/ subfolder. Do we expect to
> have the same PMIC used on other x86 vendors?

I was wondering about doing this myself before submitting this upstream,
since you have the same idea, lets go for it. I'll move this before
merging it.

> 
> ...
> 
>> +static int crystal_cove_charger_probe(struct platform_device *pdev)
>> +{
> 
> Adding
> 
>   struct device *dev = &pdev->dev;
> 
> may increase readability a bit and perhaps reduce the amount of LOCs.

Normally I'm a fan of doing that myself, but it doesn't really help
here since there are only a few references and those fit in 1 line.

Regards,

Hans

Re: [PATCH 4/4] platform/x86: Add crystal_cove_charger driver

[Hans de Goede]

Hi all,

On 12/25/21 12:55, Hans de Goede wrote:
> Driver for the external-charger IRQ pass-through function of the
> Bay Trail Crystal Cove PMIC.
> 
> Note this is NOT a power_supply class driver, it just deals with IRQ
> pass-through, this requires this separate driver because the PMIC's
> level 2 interrupt for this must be explicitly acked.
> 
> This new driver gets enabled by the existing X86_ANDROID_TABLETS Kconfig
> option because the x86-android-tablets module is the only user of the
> exported external-charger IRQ.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've added this to my review-hans (soon to be for-next) branch now.

(with the discussed move to the drivers/platform/x86/intel folder)

Regards,

Hans



> ---
>  drivers/platform/x86/Makefile               |   2 +-
>  drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++
>  2 files changed, 154 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/platform/x86/crystal_cove_charger.c
> 
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index bd20b435c22b..cce124e3acab 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -114,7 +114,7 @@ obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
>  obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
>  obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
>  obj-$(CONFIG_WIRELESS_HOTKEY)		+= wireless-hotkey.o
> -obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets.o
> +obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets.o crystal_cove_charger.o
>  
>  # Intel uncore drivers
>  obj-$(CONFIG_INTEL_IPS)				+= intel_ips.o
> diff --git a/drivers/platform/x86/crystal_cove_charger.c b/drivers/platform/x86/crystal_cove_charger.c
> new file mode 100644
> index 000000000000..382c19806b12
> --- /dev/null
> +++ b/drivers/platform/x86/crystal_cove_charger.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for the external-charger IRQ pass-through function of the
> + * Bay Trail Crystal Cove PMIC.
> + *
> + * Note this is NOT a power_supply class driver, it just deals with IRQ
> + * pass-through, this requires this separate driver because the PMIC's
> + * level 2 interrupt for this must be explicitly acked.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define CHGRIRQ_REG					0x0a
> +
> +struct crystal_cove_charger_data {
> +	struct mutex buslock; /* irq_bus_lock */
> +	struct irq_chip irqchip;
> +	struct regmap *regmap;
> +	struct irq_domain *irq_domain;
> +	int irq;
> +	int charger_irq;
> +	bool irq_enabled;
> +	bool irq_is_enabled;
> +};
> +
> +static irqreturn_t crystal_cove_charger_irq(int irq, void *data)
> +{
> +	struct crystal_cove_charger_data *charger = data;
> +
> +	/* No need to read CHGRIRQ_REG as there is only 1 IRQ */
> +	handle_nested_irq(charger->charger_irq);
> +
> +	/* Ack CHGRIRQ 0 */
> +	regmap_write(charger->regmap, CHGRIRQ_REG, BIT(0));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void crystal_cove_charger_irq_bus_lock(struct irq_data *data)
> +{
> +	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&charger->buslock);
> +}
> +
> +static void crystal_cove_charger_irq_bus_sync_unlock(struct irq_data *data)
> +{
> +	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
> +
> +	if (charger->irq_is_enabled != charger->irq_enabled) {
> +		if (charger->irq_enabled)
> +			enable_irq(charger->irq);
> +		else
> +			disable_irq(charger->irq);
> +
> +		charger->irq_is_enabled = charger->irq_enabled;
> +	}
> +
> +	mutex_unlock(&charger->buslock);
> +}
> +
> +static void crystal_cove_charger_irq_unmask(struct irq_data *data)
> +{
> +	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
> +
> +	charger->irq_enabled = true;
> +}
> +
> +static void crystal_cove_charger_irq_mask(struct irq_data *data)
> +{
> +	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
> +
> +	charger->irq_enabled = false;
> +}
> +
> +static void crystal_cove_charger_rm_irq_domain(void *data)
> +{
> +	struct crystal_cove_charger_data *charger = data;
> +
> +	irq_domain_remove(charger->irq_domain);
> +}
> +
> +static int crystal_cove_charger_probe(struct platform_device *pdev)
> +{
> +	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> +	struct crystal_cove_charger_data *charger;
> +	int ret;
> +
> +	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
> +	charger->regmap = pmic->regmap;
> +	mutex_init(&charger->buslock);
> +
> +	charger->irq = platform_get_irq(pdev, 0);
> +	if (charger->irq < 0)
> +		return charger->irq;
> +
> +	charger->irq_domain = irq_domain_create_linear(dev_fwnode(pdev->dev.parent), 1,
> +						       &irq_domain_simple_ops, NULL);
> +	if (!charger->irq_domain)
> +		return -ENOMEM;
> +
> +	/* Distuingish IRQ domain from others sharing (MFD) the same fwnode */
> +	irq_domain_update_bus_token(charger->irq_domain, DOMAIN_BUS_WAKEUP);
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, crystal_cove_charger_rm_irq_domain, charger);
> +	if (ret)
> +		return ret;
> +
> +	charger->charger_irq = irq_create_mapping(charger->irq_domain, 0);
> +	if (!charger->charger_irq)
> +		return -ENOMEM;
> +
> +	charger->irqchip.name = KBUILD_MODNAME;
> +	charger->irqchip.irq_unmask = crystal_cove_charger_irq_unmask;
> +	charger->irqchip.irq_mask = crystal_cove_charger_irq_mask;
> +	charger->irqchip.irq_bus_lock = crystal_cove_charger_irq_bus_lock;
> +	charger->irqchip.irq_bus_sync_unlock = crystal_cove_charger_irq_bus_sync_unlock;
> +
> +	irq_set_chip_data(charger->charger_irq, charger);
> +	irq_set_chip_and_handler(charger->charger_irq, &charger->irqchip, handle_simple_irq);
> +	irq_set_nested_thread(charger->charger_irq, true);
> +	irq_set_noprobe(charger->charger_irq);
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, charger->irq, NULL,
> +					crystal_cove_charger_irq,
> +					IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					KBUILD_MODNAME, charger);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "requesting irq\n");
> +
> +	return 0;
> +}
> +
> +static struct platform_driver crystal_cove_charger_driver = {
> +	.probe = crystal_cove_charger_probe,
> +	.driver = {
> +		.name = "crystal_cove_charger",
> +	},
> +};
> +module_platform_driver(crystal_cove_charger_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com");
> +MODULE_DESCRIPTION("Bay Trail Crystal Cove external charger IRQ pass-through");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 0/4] mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 1:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> While doing a hobby project getting the mainline kernel to work on some
> x86 tablets which come with Android pre-installed and which clearly have
> never been meant to run Windows (or another generic OS) I encountered an
> interesting setup for the bq24190 charger chip IRQ, it is not connected
> to the main Bay Trail SoC, instead it runs through a special
> external-charger IRQ pin on the CRC PMIC.
>
> This series adds support for that.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

There are a few non-showstopper comments, though.

> Lee, patch 4/4 is included mostly FYI. It depends on current pdx86/for-next
> and I plan to merge it through the pdx86 tree.
>
> Patches 1-3 can be merged through the MFD tree indepdently.
>
> Regards,
>
> Hans
>
>
> Hans de Goede (4):
>   mfd: intel_soc_pmic_crc: Sort cells by IRQ order
>   mfd: intel_soc_pmic_crc: Add crystal_cove_charger cell to BYT cells
>   mfd: intel_soc_pmic_crc: Set main IRQ domain bus token to
>     DOMAIN_BUS_NEXUS
>   platform/x86: Add crystal_cove_charger driver
>
>  drivers/mfd/intel_soc_pmic_core.c           |   4 +
>  drivers/mfd/intel_soc_pmic_crc.c            |  35 +++--
>  drivers/platform/x86/Makefile               |   2 +-
>  drivers/platform/x86/crystal_cove_charger.c | 153 ++++++++++++++++++++
>  4 files changed, 180 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/platform/x86/crystal_cove_charger.c
>
> --
> 2.33.1
>


-- 
With Best Regards,
Andy Shevchenko