| Message ID | 20201210212534.216197-1-krzk@kernel.org |
|---|---|
| State | Accepted |
| Commit | 46799802136670e00498f19898f1635fbc85f583 |
| Headers | show |
| Series | [01/18] ARM: dts: exynos: correct fuel gauge interrupt trigger level on GT-I9100 | expand |
(I shortened the Cc) Hi, On Thu, Dec 10, 2020 at 10:25:34PM +0100, Krzysztof Kozlowski wrote: > [...] > index d956c67d5155..f737de0470de 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -367,7 +367,7 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip) > > flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > + max17040_thread_handler, IRQF_ONESHOT, > chip->battery->desc->name, chip); > > return ret; 'flags' is unused after this change. -- Sebastian
On Thu, Dec 10, 2020 at 10:25:34PM +0100, Krzysztof Kozlowski wrote: > Interrupt line can be configured on different hardware in different way, > even inverted. Therefore driver should not enforce specific trigger > type - edge falling - but instead rely on Devicetree to configure it. > > The Maxim 14577/77836 datasheets describe the interrupt line as active > low with a requirement of acknowledge from the CPU therefore the edge > falling is not correct. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > This patch should wait till DTS changes are merged, as it relies on > proper Devicetree. > --- > .../devicetree/bindings/power/supply/max17040_battery.txt | 2 +- > drivers/power/supply/max17040_battery.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > index c802f664b508..194eb9fe574d 100644 > --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -39,7 +39,7 @@ Example: > reg = <0x36>; > maxim,alert-low-soc-level = <10>; > interrupt-parent = <&gpio7>; > - interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > + interrupts = <2 IRQ_TYPE_LEVEL_LOW>; > wakeup-source; > }; > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index d956c67d5155..f737de0470de 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -367,7 +367,7 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip) > > flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; This has to be removed. I will fix it in v2. Best regards, Krzysztof > ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > - max17040_thread_handler, flags, > + max17040_thread_handler, IRQF_ONESHOT, > chip->battery->desc->name, chip); > > return ret; > -- > 2.25.1 >
On Thu, Dec 10, 2020 at 11:16:00PM +0100, Sebastian Reichel wrote: > (I shortened the Cc) > > Hi, > > On Thu, Dec 10, 2020 at 10:25:34PM +0100, Krzysztof Kozlowski wrote: > > [...] > > index d956c67d5155..f737de0470de 100644 > > --- a/drivers/power/supply/max17040_battery.c > > +++ b/drivers/power/supply/max17040_battery.c > > @@ -367,7 +367,7 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip) > > > > flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > - max17040_thread_handler, flags, > > + max17040_thread_handler, IRQF_ONESHOT, > > chip->battery->desc->name, chip); > > > > return ret; > > 'flags' is unused after this change. Indeed, thanks. Kbuild also complained now on my github branch. I'll fix it in v2.
On 12/10/20 11:25 PM, Krzysztof Kozlowski wrote: > The Maxim fuel gauge datasheets describe the interrupt line as active > low with a requirement of acknowledge from the CPU. The falling edge > interrupt will mostly work but it's not correct. > > Fixes: 45dfa741df86 ("ARM: dts: qcom: msm8974-lge-nexus5: Add fuel gauge") > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts > index e769f638f205..4c6f54aa9f66 100644 > --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts > +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts > @@ -575,7 +575,7 @@ fuelgauge: max17048@36 { > maxim,rcomp = /bits/ 8 <0x4d>; > > interrupt-parent = <&msmgpio>; > - interrupts = <9 IRQ_TYPE_EDGE_FALLING>; > + interrupts = <9 IRQ_TYPE_LEVEL_LOW>; > > pinctrl-names = "default"; > pinctrl-0 = <&fuelgauge_pin>; According to the datasheet for max17048 the ALRT pin is active low, so that looks good. The reason it was implemented EDGE_FALLING is mostly due to fragments taken from downstream, and the fact that it worked :) Acked-by: Iskren Chernev <iskren.chernev@gmail.com>
On Fri, Dec 11, 2020 at 05:44:26PM +0200, Iskren Chernev wrote: > > On 12/11/20 9:47 AM, Krzysztof Kozlowski wrote: > > On Thu, Dec 10, 2020 at 10:25:34PM +0100, Krzysztof Kozlowski wrote: > >> Interrupt line can be configured on different hardware in different way, > >> even inverted. Therefore driver should not enforce specific trigger > >> type - edge falling - but instead rely on Devicetree to configure it. > >> > >> The Maxim 14577/77836 datasheets describe the interrupt line as active > >> low with a requirement of acknowledge from the CPU therefore the edge > >> falling is not correct. > >> > >> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > >> > >> --- > >> > >> This patch should wait till DTS changes are merged, as it relies on > >> proper Devicetree. > >> --- > >> .../devicetree/bindings/power/supply/max17040_battery.txt | 2 +- > >> drivers/power/supply/max17040_battery.c | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git > a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> index c802f664b508..194eb9fe574d 100644 > >> --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > >> @@ -39,7 +39,7 @@ Example: > >> reg = <0x36>; > >> maxim,alert-low-soc-level = <10>; > >> interrupt-parent = <&gpio7>; > >> - interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > >> + interrupts = <2 IRQ_TYPE_LEVEL_LOW>; > >> wakeup-source; > >> }; > >> > >> diff --git a/drivers/power/supply/max17040_battery.c > b/drivers/power/supply/max17040_battery.c > >> index d956c67d5155..f737de0470de 100644 > >> --- a/drivers/power/supply/max17040_battery.c > >> +++ b/drivers/power/supply/max17040_battery.c > >> @@ -367,7 +367,7 @@ static int max17040_enable_alert_irq(struct > max17040_chip *chip) > >> > >> flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT; > > > > This has to be removed. I will fix it in v2. > > > > Best regards, > > Krzysztof > > I removed the IRQF_TRIGGER_FALLING, tweaked the DT as per the DT patch, and > it worked on the samsung klte. > > I don't understand how the DT irq flag ends up being used by the kernel. It > is never explicitly read from DT or passed to interrupt API, only i2c->irq, > which is a pure int. The core __setup_irq() calls irqd_get_trigger_type() on IRQ data matching the IRQ. Best regards, Krzysztof
On Thu, 10 Dec 2020 22:25:29 +0100, Krzysztof Kozlowski wrote: > Interrupt line can be configured on different hardware in different way, > even inverted. Therefore driver should not enforce specific trigger > type - edge falling - but instead rely on Devicetree to configure it. > > The Maxim 77686 datasheet describes the interrupt line as active low > with a requirement of acknowledge from the CPU therefore the edge > falling is not correct. > > The interrupt line is shared between PMIC and RTC driver, so using level > sensitive interrupt is here especially important to avoid races. With > an edge configuration in case if first PMIC signals interrupt followed > shortly after by the RTC, the interrupt might not be yet cleared/acked > thus the second one would not be noticed. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > This patch should wait till DTS changes are merged, as it relies on > proper Devicetree. > --- > Documentation/devicetree/bindings/clock/maxim,max77686.txt | 4 ++-- > Documentation/devicetree/bindings/mfd/max77686.txt | 2 +- > Documentation/devicetree/bindings/regulator/max77686.txt | 2 +- > drivers/mfd/max77686.c | 3 +-- > 4 files changed, 5 insertions(+), 6 deletions(-) > Acked-by: Rob Herring <robh@kernel.org>
On Thu, Dec 10, 2020 at 10:25:17PM +0100, Krzysztof Kozlowski wrote: > The Maxim fuel gauge datasheets describe the interrupt line as active > low with a requirement of acknowledge from the CPU. The falling edge > interrupt will mostly work but it's not correct. > > Fixes: 8620cc2f99b7 ("ARM: dts: exynos: Add devicetree file for the Galaxy S2") > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > arch/arm/boot/dts/exynos4210-i9100.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Applied 1-10 (Exynos and S5P dts patches). Best regards, Krzysztof
diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts index a0c3bab382ae..e56b64e237d3 100644 --- a/arch/arm/boot/dts/exynos4210-i9100.dts +++ b/arch/arm/boot/dts/exynos4210-i9100.dts @@ -136,7 +136,7 @@ battery@36 { compatible = "maxim,max17042"; interrupt-parent = <&gpx2>; - interrupts = <3 IRQ_TYPE_EDGE_FALLING>; + interrupts = <3 IRQ_TYPE_LEVEL_LOW>; pinctrl-0 = <&max17042_fuel_irq>; pinctrl-names = "default";
The Maxim fuel gauge datasheets describe the interrupt line as active low with a requirement of acknowledge from the CPU. The falling edge interrupt will mostly work but it's not correct. Fixes: 8620cc2f99b7 ("ARM: dts: exynos: Add devicetree file for the Galaxy S2") Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- arch/arm/boot/dts/exynos4210-i9100.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)