[PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Andy Shevchenko]

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

The commit in Fixes: has added a pwm_add_table() call in the probe() and
a pwm_remove_table() call in the remove(), but forget to update the error
handling path of the probe.

Add the missing pwm_remove_table() call.

Fixes: a3aa9a93df9f ("mfd: intel_soc_pmic_core: ADD PWM lookup table for CRC PMIC based PWM")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch

 drivers/mfd/intel_soc_pmic_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index 5e8c94e008ed..85d070bce0e2 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -77,6 +77,7 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 	return 0;
 
 err_del_irq_chip:
+	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
 	return ret;
 }
-- 
2.35.1

[PATCH v2 02/10] mfd: intel_soc_pmic_crc: Merge Intel PMIC core to crc

[Andy Shevchenko]

The core part is misleading since its only purpose to serve Crystal Cove PMIC,
although for couple of different platforms. Merge core part into crc one.

Advantages among others are:
- speed up a compilation and build
- decreasing the code base
- reducing noise in the namespace by making some data static and const

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/Makefile              |   3 +-
 drivers/mfd/intel_soc_pmic_core.h |  25 -----
 drivers/mfd/intel_soc_pmic_crc.c  | 163 ++++++++++++++++++++++++++++--
 3 files changed, 158 insertions(+), 33 deletions(-)
 delete mode 100644 drivers/mfd/intel_soc_pmic_core.h

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..61db669f864c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -236,8 +236,7 @@ obj-$(CONFIG_MFD_RT4831)	+= rt4831.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
-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)		+= intel_soc_pmic_crc.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_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
diff --git a/drivers/mfd/intel_soc_pmic_core.h b/drivers/mfd/intel_soc_pmic_core.h
deleted file mode 100644
index d490685845eb..000000000000
--- a/drivers/mfd/intel_soc_pmic_core.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Intel SoC PMIC MFD Driver
- *
- * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
- *
- * Author: Yang, Bin <bin.yang@intel.com>
- * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
- */
-
-#ifndef __INTEL_SOC_PMIC_CORE_H__
-#define __INTEL_SOC_PMIC_CORE_H__
-
-struct intel_soc_pmic_config {
-	unsigned long irq_flags;
-	struct mfd_cell *cell_dev;
-	int n_cell_devs;
-	const struct regmap_config *regmap_config;
-	const struct regmap_irq_chip *irq_chip;
-};
-
-extern struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc;
-extern struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc;
-
-#endif	/* __INTEL_SOC_PMIC_CORE_H__ */
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 5bb0367bd974..c4e6456976f5 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -2,18 +2,21 @@
 /*
  * Device access for Crystal Cove PMIC
  *
- * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
  *
  * Author: Yang, Bin <bin.yang@intel.com>
  * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
  */
 
+#include <linux/acpi.h>
+#include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/regmap.h>
+#include <linux/module.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/intel_soc_pmic.h>
-
-#include "intel_soc_pmic_core.h"
+#include <linux/platform_data/x86/soc.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
 
 #define CRYSTAL_COVE_MAX_REGISTER	0xC6
 
@@ -132,7 +135,20 @@ static const struct regmap_irq_chip crystal_cove_irq_chip = {
 	.mask_base = CRYSTAL_COVE_REG_MIRQLVL1,
 };
 
-struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc = {
+/* PWM consumed by the Intel GFX */
+static struct pwm_lookup crc_pwm_lookup[] = {
+	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
+};
+
+struct intel_soc_pmic_config {
+	unsigned long irq_flags;
+	struct mfd_cell *cell_dev;
+	int n_cell_devs;
+	const struct regmap_config *regmap_config;
+	const struct regmap_irq_chip *irq_chip;
+};
+
+static const struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc = {
 	.irq_flags = IRQF_TRIGGER_RISING,
 	.cell_dev = crystal_cove_byt_dev,
 	.n_cell_devs = ARRAY_SIZE(crystal_cove_byt_dev),
@@ -140,10 +156,145 @@ struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc = {
 	.irq_chip = &crystal_cove_irq_chip,
 };
 
-struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc = {
+static const struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc = {
 	.irq_flags = IRQF_TRIGGER_RISING,
 	.cell_dev = crystal_cove_cht_dev,
 	.n_cell_devs = ARRAY_SIZE(crystal_cove_cht_dev),
 	.regmap_config = &crystal_cove_regmap_config,
 	.irq_chip = &crystal_cove_irq_chip,
 };
+
+static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
+				    const struct i2c_device_id *i2c_id)
+{
+	const struct intel_soc_pmic_config *config;
+	struct device *dev = &i2c->dev;
+	struct intel_soc_pmic *pmic;
+	int ret;
+
+	if (soc_intel_is_byt())
+		config = &intel_soc_pmic_config_byt_crc;
+	else
+		config = &intel_soc_pmic_config_cht_crc;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, pmic);
+
+	pmic->regmap = devm_regmap_init_i2c(i2c, config->regmap_config);
+	if (IS_ERR(pmic->regmap))
+		return PTR_ERR(pmic->regmap);
+
+	pmic->irq = i2c->irq;
+
+	ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
+				  config->irq_flags | IRQF_ONESHOT,
+				  0, config->irq_chip,
+				  &pmic->irq_chip_data);
+	if (ret)
+		return ret;
+
+	ret = enable_irq_wake(pmic->irq);
+	if (ret)
+		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
+
+	/* 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));
+	if (ret)
+		goto err_del_irq_chip;
+
+	return 0;
+
+err_del_irq_chip:
+	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
+	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
+	return ret;
+}
+
+static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
+
+	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
+
+	/* remove crc-pwm lookup table */
+	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
+
+	mfd_remove_devices(&i2c->dev);
+
+	return 0;
+}
+
+static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
+
+	disable_irq(pmic->irq);
+
+	return;
+}
+
+#if defined(CONFIG_PM_SLEEP)
+static int intel_soc_pmic_suspend(struct device *dev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+	disable_irq(pmic->irq);
+
+	return 0;
+}
+
+static int intel_soc_pmic_resume(struct device *dev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+	enable_irq(pmic->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(intel_soc_pmic_pm_ops, intel_soc_pmic_suspend,
+			 intel_soc_pmic_resume);
+
+static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
+
+#if defined(CONFIG_ACPI)
+static const struct acpi_device_id intel_soc_pmic_acpi_match[] = {
+	{ "INT33FD" },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
+#endif
+
+static struct i2c_driver intel_soc_pmic_i2c_driver = {
+	.driver = {
+		.name = "intel_soc_pmic_i2c",
+		.pm = &intel_soc_pmic_pm_ops,
+		.acpi_match_table = ACPI_PTR(intel_soc_pmic_acpi_match),
+	},
+	.probe = intel_soc_pmic_i2c_probe,
+	.remove = intel_soc_pmic_i2c_remove,
+	.id_table = intel_soc_pmic_i2c_id,
+	.shutdown = intel_soc_pmic_shutdown,
+};
+
+module_i2c_driver(intel_soc_pmic_i2c_driver);
+
+MODULE_DESCRIPTION("I2C driver for Intel SoC PMIC");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Yang, Bin <bin.yang@intel.com>");
+MODULE_AUTHOR("Zhu, Lejun <lejun.zhu@linux.intel.com>");
-- 
2.35.1

Re: [PATCH v2 02/10] mfd: intel_soc_pmic_crc: Merge Intel PMIC core to crc

[Christophe JAILLET]

Le 31/07/2022 à 22:12, Andy Shevchenko a écrit :
> The core part is misleading since its only purpose to serve Crystal Cove PMIC,
> although for couple of different platforms. Merge core part into crc one.
> 
> Advantages among others are:
> - speed up a compilation and build
> - decreasing the code base
> - reducing noise in the namespace by making some data static and const
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> v2: added tags and rebased on top of new patch 1
> 
>   drivers/mfd/Makefile              |   3 +-
>   drivers/mfd/intel_soc_pmic_core.h |  25 -----
>   drivers/mfd/intel_soc_pmic_crc.c  | 163 ++++++++++++++++++++++++++++--
>   3 files changed, 158 insertions(+), 33 deletions(-)
>   delete mode 100644 drivers/mfd/intel_soc_pmic_core.h
> 

Hi,

in v1, there was a:
    drivers/mfd/intel_soc_pmic_core.c | 160 -----------------------------
    delete mode 100644 drivers/mfd/intel_soc_pmic_core.c

Is it expected not to have it in v2?

CJ

Re: [PATCH v2 02/10] mfd: intel_soc_pmic_crc: Merge Intel PMIC core to crc

[Andy Shevchenko]

On Sun, Jul 31, 2022 at 10:29 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Le 31/07/2022 à 22:12, Andy Shevchenko a écrit :

...

> >   drivers/mfd/Makefile              |   3 +-
> >   drivers/mfd/intel_soc_pmic_core.h |  25 -----
> >   drivers/mfd/intel_soc_pmic_crc.c  | 163 ++++++++++++++++++++++++++++--
> >   3 files changed, 158 insertions(+), 33 deletions(-)
> >   delete mode 100644 drivers/mfd/intel_soc_pmic_core.h

> in v1, there was a:
>     drivers/mfd/intel_soc_pmic_core.c | 160 -----------------------------
>     delete mode 100644 drivers/mfd/intel_soc_pmic_core.c
>
> Is it expected not to have it in v2?

No, it's not. Good catch, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 03/10] mfd: intel_soc_pmic: Move non-Intel Makefile entries to their own group

[Andy Shevchenko]

It looks like a random position for couple of Makefile entries that are
disrupting Intel PMIC group. Move them to their own group.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 61db669f864c..30f5de657e88 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -172,6 +172,10 @@ obj-$(CONFIG_MFD_MAX8998)	+= max8998.o max8998-irq.o
 
 obj-$(CONFIG_MFD_MP2629)	+= mp2629.o
 
+obj-$(CONFIG_MFD_MT6360)	+= mt6360-core.o
+mt6397-objs			:= mt6397-core.o mt6397-irq.o mt6358-irq.o
+obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
+
 pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
 obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
 obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
@@ -240,9 +244,6 @@ obj-$(CONFIG_INTEL_SOC_PMIC)		+= intel_soc_pmic_crc.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_CHTDC_TI)	+= intel_soc_pmic_chtdc_ti.o
-obj-$(CONFIG_MFD_MT6360)	+= mt6360-core.o
-mt6397-objs			:= mt6397-core.o mt6397-irq.o mt6358-irq.o
-obj-$(CONFIG_MFD_MT6397)	+= mt6397.o
 obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
-- 
2.35.1

[PATCH v2 04/10] mfd: intel_soc_pmic_crc: Use devm_regmap_add_irq_chip()

[Andy Shevchenko]

Use devm_regmap_add_irq_chip() to simplify the code.

While at it, replace -1 magic parameter by PLATFORM_DEVID_NONE when
calling mfd_add_devices().

Note, the mfd_add_devices() left in non-devm variant here due to
potentially increased churn while wrapping pwm_remove_table().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index c4e6456976f5..64cdd435f8c5 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -189,10 +189,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 
 	pmic->irq = i2c->irq;
 
-	ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
-				  config->irq_flags | IRQF_ONESHOT,
-				  0, config->irq_chip,
-				  &pmic->irq_chip_data);
+	ret = devm_regmap_add_irq_chip(dev, pmic->regmap, pmic->irq,
+				       config->irq_flags | IRQF_ONESHOT,
+				       0, config->irq_chip, &pmic->irq_chip_data);
 	if (ret)
 		return ret;
 
@@ -207,26 +206,17 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 	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,
+	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, config->cell_dev,
 			      config->n_cell_devs, NULL, 0,
 			      regmap_irq_get_domain(pmic->irq_chip_data));
 	if (ret)
-		goto err_del_irq_chip;
+		pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 
-	return 0;
-
-err_del_irq_chip:
-	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
-	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
 	return ret;
 }
 
 static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
 {
-	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
-
-	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
-
 	/* remove crc-pwm lookup table */
 	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
 
-- 
2.35.1

[PATCH v2 05/10] mfd: intel_soc_pmic_crc: Convert to use i2c_get/set_clientdata()

[Andy Shevchenko]

We have the specific helpers for I2C device to set and get its driver data.
Convert driver to use them instead of open coded variants.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 64cdd435f8c5..082007485cda 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -181,7 +181,7 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 	if (!pmic)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, pmic);
+	i2c_set_clientdata(i2c, pmic);
 
 	pmic->regmap = devm_regmap_init_i2c(i2c, config->regmap_config);
 	if (IS_ERR(pmic->regmap))
@@ -227,7 +227,7 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
 
 static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
 {
-	struct intel_soc_pmic *pmic = dev_get_drvdata(&i2c->dev);
+	struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);
 
 	disable_irq(pmic->irq);
 
-- 
2.35.1

[PATCH v2 06/10] mfd: intel_soc_pmic_crc: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() etc

[Andy Shevchenko]

Letting the compiler remove these functions when the kernel is built
without CONFIG_PM_SLEEP support is simpler and less error prone than the
use of #ifdef based kernel configuration guards.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 082007485cda..d68ed5b35fd9 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -234,7 +234,6 @@ static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
 	return;
 }
 
-#if defined(CONFIG_PM_SLEEP)
 static int intel_soc_pmic_suspend(struct device *dev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
@@ -252,10 +251,8 @@ static int intel_soc_pmic_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
-static SIMPLE_DEV_PM_OPS(intel_soc_pmic_pm_ops, intel_soc_pmic_suspend,
-			 intel_soc_pmic_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(crystal_cove_pm_ops, intel_soc_pmic_suspend, intel_soc_pmic_resume);
 
 static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
 	{ }
@@ -273,7 +270,7 @@ MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
 static struct i2c_driver intel_soc_pmic_i2c_driver = {
 	.driver = {
 		.name = "intel_soc_pmic_i2c",
-		.pm = &intel_soc_pmic_pm_ops,
+		.pm = pm_sleep_ptr(&crystal_cove_pm_ops),
 		.acpi_match_table = ACPI_PTR(intel_soc_pmic_acpi_match),
 	},
 	.probe = intel_soc_pmic_i2c_probe,
-- 
2.35.1

[PATCH v2 07/10] mfd: intel_soc_pmic_crc: Drop redundant ACPI_PTR() and ifdeffery

[Andy Shevchenko]

The driver depends on ACPI, ACPI_PTR() resolution is always the same.
Otherwise a compiler may produce a warning.

That said, the rule of thumb either ugly ifdeffery with ACPI_PTR or
none should be used in a driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/Kconfig              | 4 ++--
 drivers/mfd/intel_soc_pmic_crc.c | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index abb58ab1a1a4..60196bdc8a1d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -589,8 +589,8 @@ config LPC_SCH
 
 config INTEL_SOC_PMIC
 	bool "Support for Crystal Cove PMIC"
-	depends on ACPI && HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
-	depends on X86 || COMPILE_TEST
+	depends on HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
+	depends on (X86 && ACPI) || COMPILE_TEST
 	depends on I2C_DESIGNWARE_PLATFORM=y
 	select MFD_CORE
 	select REGMAP_I2C
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index d68ed5b35fd9..17fcb10930f6 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -8,9 +8,9 @@
  * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
  */
 
-#include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/intel_soc_pmic.h>
@@ -259,19 +259,17 @@ static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
 
-#if defined(CONFIG_ACPI)
 static const struct acpi_device_id intel_soc_pmic_acpi_match[] = {
 	{ "INT33FD" },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
-#endif
 
 static struct i2c_driver intel_soc_pmic_i2c_driver = {
 	.driver = {
 		.name = "intel_soc_pmic_i2c",
 		.pm = pm_sleep_ptr(&crystal_cove_pm_ops),
-		.acpi_match_table = ACPI_PTR(intel_soc_pmic_acpi_match),
+		.acpi_match_table = intel_soc_pmic_acpi_match,
 	},
 	.probe = intel_soc_pmic_i2c_probe,
 	.remove = intel_soc_pmic_i2c_remove,
-- 
2.35.1

[PATCH v2 08/10] mfd: intel_soc_pmic_crc: Convert driver to use ->probe_new()

[Andy Shevchenko]

Use the ->probe_new() callback.

The driver does not use const struct i2c_device_id * argument,
so convert it to utilise the simplified I²C driver registration.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 17fcb10930f6..d4780390fbea 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -164,8 +164,7 @@ static const struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc = {
 	.irq_chip = &crystal_cove_irq_chip,
 };
 
-static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
-				    const struct i2c_device_id *i2c_id)
+static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c)
 {
 	const struct intel_soc_pmic_config *config;
 	struct device *dev = &i2c->dev;
@@ -254,11 +253,6 @@ static int intel_soc_pmic_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(crystal_cove_pm_ops, intel_soc_pmic_suspend, intel_soc_pmic_resume);
 
-static const struct i2c_device_id intel_soc_pmic_i2c_id[] = {
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
-
 static const struct acpi_device_id intel_soc_pmic_acpi_match[] = {
 	{ "INT33FD" },
 	{ },
@@ -271,9 +265,8 @@ static struct i2c_driver intel_soc_pmic_i2c_driver = {
 		.pm = pm_sleep_ptr(&crystal_cove_pm_ops),
 		.acpi_match_table = intel_soc_pmic_acpi_match,
 	},
-	.probe = intel_soc_pmic_i2c_probe,
+	.probe_new = intel_soc_pmic_i2c_probe,
 	.remove = intel_soc_pmic_i2c_remove,
-	.id_table = intel_soc_pmic_i2c_id,
 	.shutdown = intel_soc_pmic_shutdown,
 };
 
-- 
2.35.1

[PATCH v2 09/10] mfd: intel_soc_pmic_crc: Replace intel_soc_pmic with crystal_cove

[Andy Shevchenko]

To reflect the point that this driver is only for one type of the PMICs,
replace intel_soc_pmic with crystal_cove (avoid using crc for possible
namespace collisions with CRC library APIs).

Note, also rename the driver name since we don't expect any user
that enumerates by it, only ACPI known so far.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 42 ++++++++++++++++----------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index d4780390fbea..bbb30060d2fb 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -140,7 +140,7 @@ static struct pwm_lookup crc_pwm_lookup[] = {
 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
 };
 
-struct intel_soc_pmic_config {
+struct crystal_cove_config {
 	unsigned long irq_flags;
 	struct mfd_cell *cell_dev;
 	int n_cell_devs;
@@ -148,7 +148,7 @@ struct intel_soc_pmic_config {
 	const struct regmap_irq_chip *irq_chip;
 };
 
-static const struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc = {
+static const struct crystal_cove_config crystal_cove_config_byt_crc = {
 	.irq_flags = IRQF_TRIGGER_RISING,
 	.cell_dev = crystal_cove_byt_dev,
 	.n_cell_devs = ARRAY_SIZE(crystal_cove_byt_dev),
@@ -156,7 +156,7 @@ static const struct intel_soc_pmic_config intel_soc_pmic_config_byt_crc = {
 	.irq_chip = &crystal_cove_irq_chip,
 };
 
-static const struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc = {
+static const struct crystal_cove_config crystal_cove_config_cht_crc = {
 	.irq_flags = IRQF_TRIGGER_RISING,
 	.cell_dev = crystal_cove_cht_dev,
 	.n_cell_devs = ARRAY_SIZE(crystal_cove_cht_dev),
@@ -164,17 +164,17 @@ static const struct intel_soc_pmic_config intel_soc_pmic_config_cht_crc = {
 	.irq_chip = &crystal_cove_irq_chip,
 };
 
-static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c)
+static int crystal_cove_i2c_probe(struct i2c_client *i2c)
 {
-	const struct intel_soc_pmic_config *config;
+	const struct crystal_cove_config *config;
 	struct device *dev = &i2c->dev;
 	struct intel_soc_pmic *pmic;
 	int ret;
 
 	if (soc_intel_is_byt())
-		config = &intel_soc_pmic_config_byt_crc;
+		config = &crystal_cove_config_byt_crc;
 	else
-		config = &intel_soc_pmic_config_cht_crc;
+		config = &crystal_cove_config_cht_crc;
 
 	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
 	if (!pmic)
@@ -214,7 +214,7 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
+static int crystal_cove_i2c_remove(struct i2c_client *i2c)
 {
 	/* remove crc-pwm lookup table */
 	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
@@ -224,7 +224,7 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)
 	return 0;
 }
 
-static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
+static void crystal_cove_shutdown(struct i2c_client *i2c)
 {
 	struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);
 
@@ -233,7 +233,7 @@ static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
 	return;
 }
 
-static int intel_soc_pmic_suspend(struct device *dev)
+static int crystal_cove_suspend(struct device *dev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
@@ -242,7 +242,7 @@ static int intel_soc_pmic_suspend(struct device *dev)
 	return 0;
 }
 
-static int intel_soc_pmic_resume(struct device *dev)
+static int crystal_cove_resume(struct device *dev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
@@ -251,26 +251,26 @@ static int intel_soc_pmic_resume(struct device *dev)
 	return 0;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(crystal_cove_pm_ops, intel_soc_pmic_suspend, intel_soc_pmic_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(crystal_cove_pm_ops, crystal_cove_suspend, crystal_cove_resume);
 
-static const struct acpi_device_id intel_soc_pmic_acpi_match[] = {
+static const struct acpi_device_id crystal_cove_acpi_match[] = {
 	{ "INT33FD" },
 	{ },
 };
-MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
+MODULE_DEVICE_TABLE(acpi, crystal_cove_acpi_match);
 
-static struct i2c_driver intel_soc_pmic_i2c_driver = {
+static struct i2c_driver crystal_cove_i2c_driver = {
 	.driver = {
-		.name = "intel_soc_pmic_i2c",
+		.name = "crystal_cove_i2c",
 		.pm = pm_sleep_ptr(&crystal_cove_pm_ops),
-		.acpi_match_table = intel_soc_pmic_acpi_match,
+		.acpi_match_table = crystal_cove_acpi_match,
 	},
-	.probe_new = intel_soc_pmic_i2c_probe,
-	.remove = intel_soc_pmic_i2c_remove,
-	.shutdown = intel_soc_pmic_shutdown,
+	.probe_new = crystal_cove_i2c_probe,
+	.remove = crystal_cove_i2c_remove,
+	.shutdown = crystal_cove_shutdown,
 };
 
-module_i2c_driver(intel_soc_pmic_i2c_driver);
+module_i2c_driver(crystal_cove_i2c_driver);
 
 MODULE_DESCRIPTION("I2C driver for Intel SoC PMIC");
 MODULE_LICENSE("GPL v2");
-- 
2.35.1

[PATCH v2 10/10] mfd: intel_soc_pmic_crc: Update the copyright year

[Andy Shevchenko]

Update the copyright year to be 2012-2014, 2022.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
v2: added tags and rebased on top of new patch 1

 drivers/mfd/intel_soc_pmic_crc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index bbb30060d2fb..40f14a0c0790 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -2,7 +2,7 @@
 /*
  * Device access for Crystal Cove PMIC
  *
- * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
+ * Copyright (C) 2012-2014, 2022 Intel Corporation. All rights reserved.
  *
  * Author: Yang, Bin <bin.yang@intel.com>
  * Author: Zhu, Lejun <lejun.zhu@linux.intel.com>
-- 
2.35.1

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Hans de Goede]

Hi,

On 7/31/22 22:12, Andy Shevchenko wrote:
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> The commit in Fixes: has added a pwm_add_table() call in the probe() and
> a pwm_remove_table() call in the remove(), but forget to update the error
> handling path of the probe.
> 
> Add the missing pwm_remove_table() call.
> 
> Fixes: a3aa9a93df9f ("mfd: intel_soc_pmic_core: ADD PWM lookup table for CRC PMIC based PWM")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
> 
>  drivers/mfd/intel_soc_pmic_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> index 5e8c94e008ed..85d070bce0e2 100644
> --- a/drivers/mfd/intel_soc_pmic_core.c
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -77,6 +77,7 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>  	return 0;
>  
>  err_del_irq_chip:
> +	pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>  	regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>  	return ret;
>  }


Note alternatively we could just move the pwm_add_table() to just before the "return 0",
there is no strict ordering between adding the mfd devices and the pwm_add_table()
(the pwm device only becomes available after the pwm-driver has bound to the mfd
instantiated platform device which happens later).

IMHO that would be a bit cleaner.

Regards,

Hans

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/31/22 22:12, Andy Shevchenko wrote:

...

> >  err_del_irq_chip:
> > +     pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
> >       regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
> >       return ret;
>
> Note alternatively we could just move the pwm_add_table() to just before the "return 0",
> there is no strict ordering between adding the mfd devices and the pwm_add_table()
> (the pwm device only becomes available after the pwm-driver has bound to the mfd
> instantiated platform device which happens later).
>
> IMHO that would be a bit cleaner.

Good suggestion!

Since I need to send a v3 anyway, I will fix this accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 7/31/22 22:12, Andy Shevchenko wrote:

...

> > >  err_del_irq_chip:
> > > +     pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
> > >       regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
> > >       return ret;
> >
> > Note alternatively we could just move the pwm_add_table() to just before the "return 0",
> > there is no strict ordering between adding the mfd devices and the pwm_add_table()
> > (the pwm device only becomes available after the pwm-driver has bound to the mfd
> > instantiated platform device which happens later).

Just to be sure... How is it guaranteed that that happens later?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Hans de Goede]

Hi,

On 8/1/22 11:29, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 7/31/22 22:12, Andy Shevchenko wrote:
> 
> ...
> 
>>>>  err_del_irq_chip:
>>>> +     pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
>>>>       regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);
>>>>       return ret;
>>>
>>> Note alternatively we could just move the pwm_add_table() to just before the "return 0",
>>> there is no strict ordering between adding the mfd devices and the pwm_add_table()
>>> (the pwm device only becomes available after the pwm-driver has bound to the mfd
>>> instantiated platform device which happens later).
> 
> Just to be sure... How is it guaranteed that that happens later?

Ah you are right, it could happen immediately if the driver is builtin and
has already registered (if the PWM driver is a module, as it is on Fedora,
then the driver will only bind once the module is loaded).

Regardless there are no ordering guarantees between the probe() function of
intel_soc_pmic and the consumer of the PWM device, so the consumer must
be prepared to deal with the lookup not being present yet when its probe()
function runs (*).

Regards,

Hans


*) ATM this is actually an unsolved problem and this works only because the PMIC
drivers are builtin and i915, which consumes the PWM for backlight control
is a module. Swapping the order does not impact this.

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8/1/22 11:29, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>> On 7/31/22 22:12, Andy Shevchenko wrote:

...

> >>> Note alternatively we could just move the pwm_add_table() to just before the "return 0",
> >>> there is no strict ordering between adding the mfd devices and the pwm_add_table()
> >>> (the pwm device only becomes available after the pwm-driver has bound to the mfd
> >>> instantiated platform device which happens later).
> >
> > Just to be sure... How is it guaranteed that that happens later?
>
> Ah you are right, it could happen immediately if the driver is builtin and
> has already registered (if the PWM driver is a module, as it is on Fedora,
> then the driver will only bind once the module is loaded).
>
> Regardless there are no ordering guarantees between the probe() function of
> intel_soc_pmic and the consumer of the PWM device, so the consumer must
> be prepared to deal with the lookup not being present yet when its probe()
> function runs (*).

Would be nice to have, but isn't it the issue with all lookup tables
so far, e.g. consumers of GPIO ones are also affected the very same
way?

> *) ATM this is actually an unsolved problem and this works only because the PMIC
> drivers are builtin and i915, which consumes the PWM for backlight control
> is a module. Swapping the order does not impact this.

Even so, I think we can't change order right now because the issue is
much broader.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Hans de Goede]

Hi,

On 8/1/22 12:38, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 8/1/22 11:29, Andy Shevchenko wrote:
>>> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> On 7/31/22 22:12, Andy Shevchenko wrote:
> 
> ...
> 
>>>>> Note alternatively we could just move the pwm_add_table() to just before the "return 0",
>>>>> there is no strict ordering between adding the mfd devices and the pwm_add_table()
>>>>> (the pwm device only becomes available after the pwm-driver has bound to the mfd
>>>>> instantiated platform device which happens later).
>>>
>>> Just to be sure... How is it guaranteed that that happens later?
>>
>> Ah you are right, it could happen immediately if the driver is builtin and
>> has already registered (if the PWM driver is a module, as it is on Fedora,
>> then the driver will only bind once the module is loaded).
>>
>> Regardless there are no ordering guarantees between the probe() function of
>> intel_soc_pmic and the consumer of the PWM device, so the consumer must
>> be prepared to deal with the lookup not being present yet when its probe()
>> function runs (*).
> 
> Would be nice to have, but isn't it the issue with all lookup tables
> so far, e.g. consumers of GPIO ones are also affected the very same
> way?
> 
>> *) ATM this is actually an unsolved problem and this works only because the PMIC
>> drivers are builtin and i915, which consumes the PWM for backlight control
>> is a module. Swapping the order does not impact this.
> 
> Even so, I think we can't change order right now because the issue is
> much broader.

Ok, lets go with the original v2 1/10 patch then. In that case, you
may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

to the original v2 1/10 patch.

Regards,

Hans

Re: [PATCH v2 01/10] mfd: intel_soc_pmic: Fix an error handling path in intel_soc_pmic_i2c_probe()

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 12:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8/1/22 12:38, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 8/1/22 11:29, Andy Shevchenko wrote:
> >>> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> >>> <andy.shevchenko@gmail.com> wrote:
> >>>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>> On 7/31/22 22:12, Andy Shevchenko wrote:
> >
> > ...
> >
> >>>>> Note alternatively we could just move the pwm_add_table() to just before the "return 0",
> >>>>> there is no strict ordering between adding the mfd devices and the pwm_add_table()
> >>>>> (the pwm device only becomes available after the pwm-driver has bound to the mfd
> >>>>> instantiated platform device which happens later).
> >>>
> >>> Just to be sure... How is it guaranteed that that happens later?
> >>
> >> Ah you are right, it could happen immediately if the driver is builtin and
> >> has already registered (if the PWM driver is a module, as it is on Fedora,
> >> then the driver will only bind once the module is loaded).
> >>
> >> Regardless there are no ordering guarantees between the probe() function of
> >> intel_soc_pmic and the consumer of the PWM device, so the consumer must
> >> be prepared to deal with the lookup not being present yet when its probe()
> >> function runs (*).
> >
> > Would be nice to have, but isn't it the issue with all lookup tables
> > so far, e.g. consumers of GPIO ones are also affected the very same
> > way?
> >
> >> *) ATM this is actually an unsolved problem and this works only because the PMIC
> >> drivers are builtin and i915, which consumes the PWM for backlight control
> >> is a module. Swapping the order does not impact this.
> >
> > Even so, I think we can't change order right now because the issue is
> > much broader.
>
> Ok, lets go with the original v2 1/10 patch then. In that case, you
> may add my:

Will do.

> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> to the original v2 1/10 patch.

Thanks!

-- 
With Best Regards,
Andy Shevchenko