| Message ID | 1408516009-22106-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 20 Aug 2014, Marek Szyprowski wrote: > Commit 6f1c1e71d93 ("mfd: max77686: Convert to use regmap_irq") broke > support for boards, which provide no irq for MAX 77686 PMIC. In such > case, not all functions of PMIC are available, but the driver still > should at least handle voltage regulators instead of failing to init. > This patch restores previous behavior. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > Hello, > > This patch fixes booting issues with recently merged support for > Exynos4412 based Odroid X2/U3 boards. See > http://www.spinics.net/lists/linux-samsung-soc/msg35773.html > thread for more details. > > Best regards > Marek Szyprowski > Samsung R&D Institute Poland > --- > drivers/mfd/max77686.c | 46 +++++++++++++++++++++++++++------------------- > 1 file changed, 27 insertions(+), 19 deletions(-) > > diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c > index 86e552348db4..525718c448c8 100644 > --- a/drivers/mfd/max77686.c > +++ b/drivers/mfd/max77686.c [...] > @@ -357,8 +363,10 @@ static int max77686_i2c_remove(struct i2c_client *i2c) > > mfd_remove_devices(max77686->dev); > > - regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); > - regmap_del_irq_chip(max77686->irq, max77686->irq_data); > + if (max77686->irq) > + regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); > + if (max77686->irq) > + regmap_del_irq_chip(max77686->irq, max77686->irq_data); Looks like you rushed this patch and went a bit crazy with copy-paste. :) Please put these two regmap_del_irq_chip()'s as results of the same conditional i.e: if (max77686->irq) { regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); regmap_del_irq_chip(max77686->irq, max77686->irq_data); } > > if (max77686->type == TYPE_MAX77686) > i2c_unregister_device(max77686->rtc);
Hello Lee, Marek, On 08/20/2014 10:19 AM, Lee Jones wrote: > On Wed, 20 Aug 2014, Marek Szyprowski wrote: > >> Commit 6f1c1e71d93 ("mfd: max77686: Convert to use regmap_irq") broke >> support for boards, which provide no irq for MAX 77686 PMIC. In such >> case, not all functions of PMIC are available, but the driver still >> should at least handle voltage regulators instead of failing to init. >> This patch restores previous behavior. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> Hello, >> >> This patch fixes booting issues with recently merged support for >> Exynos4412 based Odroid X2/U3 boards. See >> http://www.spinics.net/lists/linux-samsung-soc/msg35773.html >> thread for more details. >> >> Best regards >> Marek Szyprowski >> Samsung R&D Institute Poland I already gave my opinion about this patch on thread: "[PATCH v2 0/4] Add Exynos4412 based Odroid X2 and U2/U3/U3+ support" where the issue was reported by Olof but I'll add it here as well for completeness since I don't know if the same people are cc'ed on both threads. Bartlomiej Zolnierkiewicz posted a similar patch [0] before but as I said on that thread, I'm not sure if is correct to make that DT property optional. Or rather, if we need to make it optional then we should also change the Documentation/devicetree/bindings/mfd/max77686.txt that list "interrupts" DT property as required. It's true that before converting the max77686 to use the regmap irq API, the driver was loose on that regard but max77686_irq_init() in max77686-irq.c was returning -ENODEV when the IRQ was not provided and that error code was not checked in max77686_i2c_probe() (which was a bug IMHO). More importantly I wonder if a design where a IRQ line is not connected to the max77686 PMIC makes even sense. If such a design is possible then the "interrupt" property should be made optional but if not then I think that the driver should fail to probe in that case. Yes, it is a regression but the Odroid DTS was not following the DT binding and was just lucky that the driver was not checking the required DT properties. Daniel Drake posted the correct fix for the Odroid DTS [1]. So I tend to say that Daniel's fix should be added as a -rc fix and we should keep both the max77686 driver and its DT binding as it is now but as I said, if from a hardware point of view makes sense to leave that line unconnected then I don't oppose to this change. Best regards, Javier [0]: https://lkml.org/lkml/2014/8/7/374 [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272869.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi, On Wednesday, August 20, 2014 10:29:39 AM Javier Martinez Canillas wrote: > Hello Lee, Marek, > > On 08/20/2014 10:19 AM, Lee Jones wrote: > > On Wed, 20 Aug 2014, Marek Szyprowski wrote: > > > >> Commit 6f1c1e71d93 ("mfd: max77686: Convert to use regmap_irq") broke > >> support for boards, which provide no irq for MAX 77686 PMIC. In such > >> case, not all functions of PMIC are available, but the driver still > >> should at least handle voltage regulators instead of failing to init. > >> This patch restores previous behavior. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> Hello, > >> > >> This patch fixes booting issues with recently merged support for > >> Exynos4412 based Odroid X2/U3 boards. See > >> http://www.spinics.net/lists/linux-samsung-soc/msg35773.html > >> thread for more details. > >> > >> Best regards > >> Marek Szyprowski > >> Samsung R&D Institute Poland > > I already gave my opinion about this patch on thread: > > "[PATCH v2 0/4] Add Exynos4412 based Odroid X2 and U2/U3/U3+ support" > > where the issue was reported by Olof but I'll add it here as well for > completeness since I don't know if the same people are cc'ed on both threads. > > Bartlomiej Zolnierkiewicz posted a similar patch [0] before but as I said BTW my patch a bit simpler than this one. regmap_del_irq_chip() checks for second argument being NULL so it is not needed to explicitly check for max77686->irq not being zero on probe failures before calling regmap_del_irq_chip(). > on that thread, I'm not sure if is correct to make that DT property > optional. Or rather, if we need to make it optional then we should also > change the Documentation/devicetree/bindings/mfd/max77686.txt that list > "interrupts" DT property as required. > > It's true that before converting the max77686 to use the regmap irq API, > the driver was loose on that regard but max77686_irq_init() in > max77686-irq.c was returning -ENODEV when the IRQ was not provided and > that error code was not checked in max77686_i2c_probe() (which was a bug > IMHO). > > More importantly I wonder if a design where a IRQ line is not connected to > the max77686 PMIC makes even sense. If such a design is possible then the > "interrupt" property should be made optional but if not then I think that > the driver should fail to probe in that case. Yes, it is a regression but > the Odroid DTS was not following the DT binding and was just lucky that > the driver was not checking the required DT properties. > > Daniel Drake posted the correct fix for the Odroid DTS [1]. So I tend to > say that Daniel's fix should be added as a -rc fix and we should keep both > the max77686 driver and its DT binding as it is now but as I said, if from > a hardware point of view makes sense to leave that line unconnected then I > don't oppose to this change. > > Best regards, > Javier > > [0]: https://lkml.org/lkml/2014/8/7/374 > [1]: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272869.html Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, 20 Aug 2014, Javier Martinez Canillas wrote: > Hello Lee, Marek, > > On 08/20/2014 10:19 AM, Lee Jones wrote: > > On Wed, 20 Aug 2014, Marek Szyprowski wrote: > > > >> Commit 6f1c1e71d93 ("mfd: max77686: Convert to use regmap_irq") broke > >> support for boards, which provide no irq for MAX 77686 PMIC. In such > >> case, not all functions of PMIC are available, but the driver still > >> should at least handle voltage regulators instead of failing to init. > >> This patch restores previous behavior. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> Hello, > >> > >> This patch fixes booting issues with recently merged support for > >> Exynos4412 based Odroid X2/U3 boards. See > >> http://www.spinics.net/lists/linux-samsung-soc/msg35773.html > >> thread for more details. > >> > >> Best regards > >> Marek Szyprowski > >> Samsung R&D Institute Poland > > I already gave my opinion about this patch on thread: > > "[PATCH v2 0/4] Add Exynos4412 based Odroid X2 and U2/U3/U3+ support" > > where the issue was reported by Olof but I'll add it here as well for > completeness since I don't know if the same people are cc'ed on both threads. > > Bartlomiej Zolnierkiewicz posted a similar patch [0] before but as I said > on that thread, I'm not sure if is correct to make that DT property > optional. Or rather, if we need to make it optional then we should also > change the Documentation/devicetree/bindings/mfd/max77686.txt that list > "interrupts" DT property as required. > > It's true that before converting the max77686 to use the regmap irq API, > the driver was loose on that regard but max77686_irq_init() in > max77686-irq.c was returning -ENODEV when the IRQ was not provided and > that error code was not checked in max77686_i2c_probe() (which was a bug > IMHO). > > More importantly I wonder if a design where a IRQ line is not connected to > the max77686 PMIC makes even sense. If such a design is possible then the > "interrupt" property should be made optional but if not then I think that > the driver should fail to probe in that case. Yes, it is a regression but > the Odroid DTS was not following the DT binding and was just lucky that > the driver was not checking the required DT properties. > > Daniel Drake posted the correct fix for the Odroid DTS [1]. So I tend to > say that Daniel's fix should be added as a -rc fix and we should keep both > the max77686 driver and its DT binding as it is now but as I said, if from > a hardware point of view makes sense to leave that line unconnected then I > don't oppose to this change. I wasn't aware that this had already been discussed. FWIW, I agree with everything you just said, so unless someone insists that the device can be functional without an IRQ line, I'm going to shelve (not accept) this patch. > [0]: https://lkml.org/lkml/2014/8/7/374 > [1]: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272869.html
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c index 86e552348db4..525718c448c8 100644 --- a/drivers/mfd/max77686.c +++ b/drivers/mfd/max77686.c @@ -314,22 +314,26 @@ static int max77686_i2c_probe(struct i2c_client *i2c, } } - ret = regmap_add_irq_chip(max77686->regmap, max77686->irq, - IRQF_TRIGGER_FALLING | IRQF_ONESHOT | - IRQF_SHARED, 0, irq_chip, - &max77686->irq_data); - if (ret) { - dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret); - goto err_unregister_i2c; - } + if (max77686->irq) { + ret = regmap_add_irq_chip(max77686->regmap, max77686->irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT | + IRQF_SHARED, 0, irq_chip, + &max77686->irq_data); + if (ret) { + dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", + ret); + goto err_unregister_i2c; + } - ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq, - IRQF_TRIGGER_FALLING | IRQF_ONESHOT | - IRQF_SHARED, 0, rtc_irq_chip, - &max77686->rtc_irq_data); - if (ret) { - dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret); - goto err_del_irqc; + ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT | + IRQF_SHARED, 0, rtc_irq_chip, + &max77686->rtc_irq_data); + if (ret) { + dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", + ret); + goto err_del_irqc; + } } ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL); @@ -341,9 +345,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c, return 0; err_del_rtc_irqc: - regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); + if (max77686->irq) + regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); err_del_irqc: - regmap_del_irq_chip(max77686->irq, max77686->irq_data); + if (max77686->irq) + regmap_del_irq_chip(max77686->irq, max77686->irq_data); err_unregister_i2c: if (max77686->type == TYPE_MAX77686) i2c_unregister_device(max77686->rtc); @@ -357,8 +363,10 @@ static int max77686_i2c_remove(struct i2c_client *i2c) mfd_remove_devices(max77686->dev); - regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); - regmap_del_irq_chip(max77686->irq, max77686->irq_data); + if (max77686->irq) + regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data); + if (max77686->irq) + regmap_del_irq_chip(max77686->irq, max77686->irq_data); if (max77686->type == TYPE_MAX77686) i2c_unregister_device(max77686->rtc);
Commit 6f1c1e71d93 ("mfd: max77686: Convert to use regmap_irq") broke support for boards, which provide no irq for MAX 77686 PMIC. In such case, not all functions of PMIC are available, but the driver still should at least handle voltage regulators instead of failing to init. This patch restores previous behavior. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- Hello, This patch fixes booting issues with recently merged support for Exynos4412 based Odroid X2/U3 boards. See http://www.spinics.net/lists/linux-samsung-soc/msg35773.html thread for more details. Best regards Marek Szyprowski Samsung R&D Institute Poland --- drivers/mfd/max77686.c | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-)