From: Christophe JAILLET <[email protected]>
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
Tested-by: Hans de Goede <[email protected]>
Acked-for-MFD-by: Lee Jones <[email protected]>
---
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 <[email protected]>
*/
-#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
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 <[email protected]>
Tested-by: Hans de Goede <[email protected]>
Acked-for-MFD-by: Lee Jones <[email protected]>
---
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
Hi,
On 7/31/22 22:12, Andy Shevchenko wrote:
> From: Christophe JAILLET <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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
On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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
Hi,
On 8/1/22 11:29, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> <[email protected]> wrote:
>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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.
On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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
On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <[email protected]> wrote:
> On 8/1/22 11:29, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> > <[email protected]> wrote:
> >> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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
Hi,
On 8/1/22 12:38, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <[email protected]> wrote:
>> On 8/1/22 11:29, Andy Shevchenko wrote:
>>> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
>>> <[email protected]> wrote:
>>>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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 <[email protected]>
to the original v2 1/10 patch.
Regards,
Hans
On Mon, Aug 1, 2022 at 12:53 PM Hans de Goede <[email protected]> wrote:
> On 8/1/22 12:38, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 11:52 AM Hans de Goede <[email protected]> wrote:
> >> On 8/1/22 11:29, Andy Shevchenko wrote:
> >>> On Mon, Aug 1, 2022 at 11:14 AM Andy Shevchenko
> >>> <[email protected]> wrote:
> >>>> On Mon, Aug 1, 2022 at 10:43 AM Hans de Goede <[email protected]> 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 <[email protected]>
>
> to the original v2 1/10 patch.
Thanks!
--
With Best Regards,
Andy Shevchenko