| Message ID | 20201202203516.43053-1-timon.baetz@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] extcon: max8997: Add CHGINS and CHGRM interrupt handling | expand |
On Wed, Dec 02, 2020 at 09:07:28PM +0000, Timon Baetz wrote: > Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 kernel > fork. > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > --- > arch/arm/boot/dts/exynos4210-i9100.dts | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts > index 9f8d927e0d21..2700d53ea01b 100644 > --- a/arch/arm/boot/dts/exynos4210-i9100.dts > +++ b/arch/arm/boot/dts/exynos4210-i9100.dts > @@ -558,14 +558,14 @@ safe2_sreg: ESAFEOUT2 { > > charger_reg: CHARGER { > regulator-name = "CHARGER"; > - regulator-min-microamp = <60000>; > - regulator-max-microamp = <2580000>; > + regulator-min-microamp = <200000>; > + regulator-max-microamp = <950000>; > }; > > chargercv_reg: CHARGER_CV { > regulator-name = "CHARGER_CV"; > - regulator-min-microvolt = <3800000>; > - regulator-max-microvolt = <4100000>; > + regulator-min-microvolt = <4200000>; > + regulator-max-microvolt = <4200000>; I am looking at my sources of Android 3.0 for GT-I9100 but I cannot find charger voltages for it. Where did you find it? Best regards, Krzysztof
On Wednesday, December 2, 2020 11:04 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Wed, Dec 02, 2020 at 09:07:28PM +0000, Timon Baetz wrote: > > > Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 kernel > > fork. > > > > Signed-off-by: Timon Baetz timon.baetz@protonmail.com > > > > ------------------------------------------------------ > > > > arch/arm/boot/dts/exynos4210-i9100.dts | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts > > index 9f8d927e0d21..2700d53ea01b 100644 > > --- a/arch/arm/boot/dts/exynos4210-i9100.dts > > +++ b/arch/arm/boot/dts/exynos4210-i9100.dts > > @@ -558,14 +558,14 @@ safe2_sreg: ESAFEOUT2 { > > > > charger_reg: CHARGER { > > regulator-name = "CHARGER"; > > > > > > - regulator-min-microamp = <60000>; > > > > > > - regulator-max-microamp = <2580000>; > > > > > > > > - regulator-min-microamp = <200000>; > > > > > > - regulator-max-microamp = <950000>; > > }; > > > > chargercv_reg: CHARGER_CV { > > regulator-name = "CHARGER_CV"; > > > > > > > > - regulator-min-microvolt = <3800000>; > > > > > > - regulator-max-microvolt = <4100000>; > > > > > > > > - regulator-min-microvolt = <4200000>; > > > > > > - regulator-max-microvolt = <4200000>; > > > > > > I am looking at my sources of Android 3.0 for GT-I9100 but I cannot find > charger voltages for it. Where did you find it? > > Best regards, > Krzysztof Thanks all the feedback Krzysztof, Voltage is set in the charger probe function of the downstream kernel fork: https://github.com/LineageOS/android_kernel_samsung_smdk4412/blob/lineage-17.0/drivers/power/max8997_charger_u1.c#L390-L391 Mainline uses the regulator: https://github.com/torvalds/linux/blob/master/drivers/regulator/max8997-regulator.c#L418-L419
On Thu, Dec 03, 2020 at 05:46:03AM +0000, Timon Bätz wrote: > On Wednesday, December 2, 2020 11:04 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > On Wed, Dec 02, 2020 at 09:07:28PM +0000, Timon Baetz wrote: > > > > > Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 kernel > > > fork. > > > > > > Signed-off-by: Timon Baetz timon.baetz@protonmail.com > > > > > > ------------------------------------------------------ > > > > > > arch/arm/boot/dts/exynos4210-i9100.dts | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts > > > index 9f8d927e0d21..2700d53ea01b 100644 > > > --- a/arch/arm/boot/dts/exynos4210-i9100.dts > > > +++ b/arch/arm/boot/dts/exynos4210-i9100.dts > > > @@ -558,14 +558,14 @@ safe2_sreg: ESAFEOUT2 { > > > > > > charger_reg: CHARGER { > > > regulator-name = "CHARGER"; > > > > > > > > > - regulator-min-microamp = <60000>; > > > > > > > > > - regulator-max-microamp = <2580000>; > > > > > > > > > > > > - regulator-min-microamp = <200000>; > > > > > > > > > - regulator-max-microamp = <950000>; > > > }; > > > > > > chargercv_reg: CHARGER_CV { > > > regulator-name = "CHARGER_CV"; > > > > > > > > > > > > - regulator-min-microvolt = <3800000>; > > > > > > > > > - regulator-max-microvolt = <4100000>; > > > > > > > > > > > > - regulator-min-microvolt = <4200000>; > > > > > > > > > - regulator-max-microvolt = <4200000>; > > > > > > > > > > I am looking at my sources of Android 3.0 for GT-I9100 but I cannot find > > charger voltages for it. Where did you find it? > > > > Best regards, > > Krzysztof > > Thanks all the feedback Krzysztof, > > Voltage is set in the charger probe function of the downstream kernel fork: https://github.com/LineageOS/android_kernel_samsung_smdk4412/blob/lineage-17.0/drivers/power/max8997_charger_u1.c#L390-L391 You need to fix your email client to wrap lines. The fork cannot be used as a reference because of poor quality of explanations for origins of the code. The commit which added 4.2 V is described as "samsung update 1" which basically means nothing. If at least it was "drop sources of GT-I9105"... but in this form it is useless. For the things we are not sure how they should be implemented, we sometimes accept the reason "vendor sources do like this". However Lineage or any other fork are not vendor sources. Therefore you need to provide a valid explanation for this voltage change. Best regards, Krzysztof
On Thu, Dec 03, 2020 at 10:23:01AM +0200, Krzysztof Kozlowski wrote: > On Thu, Dec 03, 2020 at 05:46:03AM +0000, Timon Bätz wrote: > > On Wednesday, December 2, 2020 11:04 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > On Wed, Dec 02, 2020 at 09:07:28PM +0000, Timon Baetz wrote: > > > > > > > Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 kernel > > > > fork. > > > > > > > > Signed-off-by: Timon Baetz timon.baetz@protonmail.com > > > > > > > > ------------------------------------------------------ > > > > > > > > arch/arm/boot/dts/exynos4210-i9100.dts | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts > > > > index 9f8d927e0d21..2700d53ea01b 100644 > > > > --- a/arch/arm/boot/dts/exynos4210-i9100.dts > > > > +++ b/arch/arm/boot/dts/exynos4210-i9100.dts > > > > @@ -558,14 +558,14 @@ safe2_sreg: ESAFEOUT2 { > > > > > > > > charger_reg: CHARGER { > > > > regulator-name = "CHARGER"; > > > > > > > > > > > > - regulator-min-microamp = <60000>; > > > > > > > > > > > > - regulator-max-microamp = <2580000>; > > > > > > > > > > > > > > > > - regulator-min-microamp = <200000>; > > > > > > > > > > > > - regulator-max-microamp = <950000>; > > > > }; > > > > > > > > chargercv_reg: CHARGER_CV { > > > > regulator-name = "CHARGER_CV"; > > > > > > > > > > > > > > > > - regulator-min-microvolt = <3800000>; > > > > > > > > > > > > - regulator-max-microvolt = <4100000>; > > > > > > > > > > > > > > > > - regulator-min-microvolt = <4200000>; > > > > > > > > > > > > - regulator-max-microvolt = <4200000>; > > > > > > > > > > > > > > I am looking at my sources of Android 3.0 for GT-I9100 but I cannot find > > > charger voltages for it. Where did you find it? > > > > > > Best regards, > > > Krzysztof > > > > Thanks all the feedback Krzysztof, > > > > Voltage is set in the charger probe function of the downstream kernel fork: https://github.com/LineageOS/android_kernel_samsung_smdk4412/blob/lineage-17.0/drivers/power/max8997_charger_u1.c#L390-L391 > > You need to fix your email client to wrap lines. > > The fork cannot be used as a reference because of poor quality of > explanations for origins of the code. > > The commit which added 4.2 V is described as "samsung update 1" which > basically means nothing. If at least it was "drop sources of > GT-I9105"... but in this form it is useless. > > For the things we are not sure how they should be implemented, we > sometimes accept the reason "vendor sources do like this". However Lineage > or any other fork are not vendor sources. > > Therefore you need to provide a valid explanation for this voltage > change. I checked vendor sources for Samsung Galaxy S2 Epic 4G Touch (SPH-D710) and indeed it uses the max8997 charger U1 which sets v4.2 volts. You can use it to fix up the commit msg. Unfortunately it seems Samsung started to remove most of older kernel source code from their OS compliance page. S1, S2 and S3 are mostly gone. I was able to find just few remaining sources and I am now updating my vendor-dump with them. I'll upload them later to https://github.com/krzk/linux-vendor-backup . Best regards, Krzysztof
On Thu, Dec 03, 2020 at 11:50:41AM +0200, Krzysztof Kozlowski wrote: > > Unfortunately it seems Samsung started to remove most of older > kernel source code from their OS compliance page. S1, S2 and S3 are > mostly gone. I was able to find just few remaining sources and I am now > updating my vendor-dump with them. I'll upload them later to > https://github.com/krzk/linux-vendor-backup . > I don't know why they keep removing older kernel sources (it's pretty stupid), but so far they added all of them back within a couple of days after I made an inquiry on https://opensource.samsung.com/requestInquiry (Note: They remove them again after a while so backups are always necessary...) Might be worth a try if you need some of them :) Stephan
On Mon, 21 Dec 2020, Timon Baetz wrote: > Register for extcon notification and set charging current depending on > the detected cable type. Current values are taken from vendor kernel, > where most charger types end up setting 650mA [0]. > > Also enable and disable the CHARGER regulator based on extcon events. > > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678 > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > --- > drivers/mfd/max8997.c | 4 +- Please split this out into a separate patch. > drivers/power/supply/max8997_charger.c | 94 ++++++++++++++++++++++++++ > 2 files changed, 96 insertions(+), 2 deletions(-)
On Mon, Dec 21, 2020 at 09:53:08AM +0000, Timon Baetz wrote: > This allows the MAX8997 charger to set the current limit depending on > the detected extcon charger type. > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> Don't do this: In-Reply-To: <20201202203516.43053-1-timon.baetz@protonmail.com> It's a v2, so new thread. If you want to reference previous work, paste a link from lore.kernel.org. Best regards, Krzysztof
On Mon, Dec 21, 2020 at 09:53:22AM +0000, Timon Baetz wrote: > Set CHARGER current and CHARGER_CV voltage according to Galaxy S2 vendor > sources [0,1]. Mention that the vendor sources are for Galaxy S2 Epic 4G Touch SPH-D710 Android. This seems to depend on driver changes, so it will have to wait till they reach mainline. Best regards, Krzysztof
On Mon, Dec 21, 2020 at 09:53:35AM +0000, Timon Baetz wrote: > Value taken from Galaxy S2 vendor kernel [0] which always sets 200mA. Subject: "Add", not "Added", as in Linux coding style. > > Also rearrange regulators based on definition in max8997.h. > > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/power/sec_battery_u1.c#L1525 > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > --- > arch/arm/boot/dts/exynos4210-i9100.dts | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts b/arch/arm/boot/dts/exynos4210-i9100.dts > index 586d801af0b5..fec6da64f7c1 100644 > --- a/arch/arm/boot/dts/exynos4210-i9100.dts > +++ b/arch/arm/boot/dts/exynos4210-i9100.dts > @@ -560,6 +560,16 @@ safe2_sreg: ESAFEOUT2 { > regulator-boot-on; > }; > > + EN32KHZ_AP { > + regulator-name = "EN32KHZ_AP"; > + regulator-always-on; > + }; > + > + EN32KHZ_CP { > + regulator-name = "EN32KHZ_CP"; > + regulator-always-on; > + }; > + > charger_reg: CHARGER { > regulator-name = "CHARGER"; > regulator-min-microamp = <200000>; > @@ -573,13 +583,10 @@ chargercv_reg: CHARGER_CV { > regulator-always-on; > }; > > - EN32KHZ_AP { > - regulator-name = "EN32KHZ_AP"; > - regulator-always-on; > - }; > - > - EN32KHZ_CP { > - regulator-name = "EN32KHZ_CP"; > + chargertopoff_reg: CHARGER_TOPOFF { No need for label "chargertopoff_reg". Best regards, Krzysztof
On Mon, Dec 21, 2020 at 03:35:07PM +0000, Timon Baetz wrote: > On Mon, 21 Dec 2020 15:16:27 +0100, Krzysztof Kozlowski wrote: > > On Mon, Dec 21, 2020 at 09:53:15AM +0000, Timon Baetz wrote: > > > Register for extcon notification and set charging current depending on > > > the detected cable type. Current values are taken from vendor kernel, > > > where most charger types end up setting 650mA [0]. > > > > > > Also enable and disable the CHARGER regulator based on extcon events. > > > > > > [0] https://github.com/krzk/linux-vendor-backup/blob/samsung/galaxy-s2-epic-4g-touch-sph-d710-exynos4210-dump/drivers/misc/max8997-muic.c#L1675-L1678 > > > > > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > > > --- > > > drivers/mfd/max8997.c | 4 +- > > > drivers/power/supply/max8997_charger.c | 94 ++++++++++++++++++++++++++ > > > 2 files changed, 96 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mfd/max8997.c b/drivers/mfd/max8997.c > > > index 68d8f2b95287..55d3a6f97783 100644 > > > --- a/drivers/mfd/max8997.c > > > +++ b/drivers/mfd/max8997.c > > > @@ -29,9 +29,9 @@ > > > static const struct mfd_cell max8997_devs[] = { > > > { .name = "max8997-pmic", }, > > > { .name = "max8997-rtc", }, > > > - { .name = "max8997-battery", }, > > > + { .name = "max8997-battery", .of_compatible = "maxim,max8997-battery", }, > > > { .name = "max8997-haptic", }, > > > - { .name = "max8997-muic", }, > > > + { .name = "max8997-muic", .of_compatible = "maxim,max8997-muic", }, > > > > Undocumented bindings. The checkpatch should complain about it, so I > > assume you did not run it. Please run the checkpatch. > > > > > { .name = "max8997-led", .id = 1 }, > > > { .name = "max8997-led", .id = 2 }, > > > }; > > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > > > index 1947af25879a..6e8750e455ea 100644 > > > --- a/drivers/power/supply/max8997_charger.c > > > +++ b/drivers/power/supply/max8997_charger.c > > > @@ -6,12 +6,14 @@ > > > // MyungJoo Ham <myungjoo.ham@samsung.com> > > > > > > #include <linux/err.h> > > > +#include <linux/extcon.h> > > > #include <linux/module.h> > > > #include <linux/slab.h> > > > #include <linux/platform_device.h> > > > #include <linux/power_supply.h> > > > #include <linux/mfd/max8997.h> > > > #include <linux/mfd/max8997-private.h> > > > +#include <linux/regulator/consumer.h> > > > > > > /* MAX8997_REG_STATUS4 */ > > > #define DCINOK_SHIFT 1 > > > @@ -31,6 +33,10 @@ struct charger_data { > > > struct device *dev; > > > struct max8997_dev *iodev; > > > struct power_supply *battery; > > > + struct regulator *reg; > > > + struct extcon_dev *edev; > > > + struct notifier_block extcon_nb; > > > + struct work_struct extcon_work; > > > }; > > > > > > static enum power_supply_property max8997_battery_props[] = { > > > @@ -88,6 +94,67 @@ static int max8997_battery_get_property(struct power_supply *psy, > > > return 0; > > > } > > > > > > +static void max8997_battery_extcon_evt_stop_work(void *data) > > > +{ > > > + struct charger_data *charger = data; > > > + > > > + cancel_work_sync(&charger->extcon_work); > > > +} > > > + > > > +static void max8997_battery_extcon_evt_worker(struct work_struct *work) > > > +{ > > > + struct charger_data *charger = > > > + container_of(work, struct charger_data, extcon_work); > > > + struct extcon_dev *edev = charger->edev; > > > + int current_limit, ret; > > > + > > > + if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) { > > > + dev_dbg(charger->dev, "USB SDP charger is connected\n"); > > > + current_limit = 450000; > > > + } else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) { > > > + dev_dbg(charger->dev, "USB DCP charger is connected\n"); > > > + current_limit = 650000; > > > + } else if (extcon_get_state(edev, EXTCON_CHG_USB_FAST) > 0) { > > > + dev_dbg(charger->dev, "USB FAST charger is connected\n"); > > > + current_limit = 650000; > > > + } else if (extcon_get_state(edev, EXTCON_CHG_USB_SLOW) > 0) { > > > + dev_dbg(charger->dev, "USB SLOW charger is connected\n"); > > > + current_limit = 650000; > > > + } else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) { > > > + dev_dbg(charger->dev, "USB CDP charger is connected\n"); > > > + current_limit = 650000; > > > + } else { > > > + dev_dbg(charger->dev, "USB charger is diconnected\n"); > > > + current_limit = -1; > > > + } > > > + > > > + if (current_limit > 0) { > > > + ret = regulator_set_current_limit(charger->reg, current_limit, current_limit); > > > + if (ret) { > > > + dev_err(charger->dev, "failed to set current limit: %d\n", ret); > > > + goto regulator_disable; > > > > Unusual error path... if regulator was not enabled before and > > regulator_set_current_limit() failed, you disable the regulator? Why? > > Wasn't it already disabled? > > Because I thought you asked me to in v1 of this patch: > > Failure of setting the current should rather disable the charging. > > I probably misunderstood you comment then. So I guess it should just > return? Yes, I was not specific enough. In v1 you enabled the charging even in case of regulator_set_current_limit() error. Instead, the charging should not be enabled, so just return here with error. Best regards, Krzysztof
diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c index 172e116ac1ce..70ffcef12e3e 100644 --- a/drivers/extcon/extcon-max8997.c +++ b/drivers/extcon/extcon-max8997.c @@ -44,6 +44,8 @@ static struct max8997_muic_irq muic_irqs[] = { { MAX8997_MUICIRQ_ChgDetRun, "muic-CHGDETRUN" }, { MAX8997_MUICIRQ_ChgTyp, "muic-CHGTYP" }, { MAX8997_MUICIRQ_OVP, "muic-OVP" }, + { MAX8997_PMICIRQ_CHGINS, "pmic-CHGINS" }, + { MAX8997_PMICIRQ_CHGRM, "pmic-CHGRM" }, }; /* Define supported cable type */ @@ -538,6 +540,9 @@ static void max8997_muic_irq_work(struct work_struct *work) case MAX8997_MUICIRQ_DCDTmr: case MAX8997_MUICIRQ_ChgDetRun: case MAX8997_MUICIRQ_ChgTyp: + case MAX8997_PMICIRQ_CHGINS: + case MAX8997_PMICIRQ_CHGRM: + /* Handle charger cable */ ret = max8997_muic_chg_handler(info); break;
Allows the MAX8997 charger to set the current limit depending on the detected extcon charger type. Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> --- drivers/extcon/extcon-max8997.c | 5 +++++ 1 file changed, 5 insertions(+)