This patchset adds the charger driver "rt5033-charger". It is part of the
multifunction device rt5033. The patchset is based on an older version by
Beomho Seo of March 2015. For more information on the history and setup of
the patchset see the cover sheet of version v1, there is a link further down
below the changelog.
In patch 9 I didn't change the extcon phandle, I haven't received any answer
on this.
Changes in v2:
- Rebased to linux-next (20230413), as suggested by Lee.
- The v1 patch 3 "mfd: rt5033: Fix comments and style in includes" vanished
as it got applied already.
- Dropped the v1 patch 8 "power: supply: rt5033_charger: Make use of high
impedance mode". The high impedance mode is kind of a sleep mode for power
saving. It turned out that it might complicate a future implementation of
an rt5033 flash LED driver. Therefore drop it for now. The high impedance
mode could be added at a later date as a power saving improvement.
- Patch 2: Changed variable name "data" back to original "dev_id".
- New patch 5: Changed name of regulators to lowercase, as suggested by Rob.
- Patch 6: In function "rt5033_charger_dt_init" replaced the devicetree units
"uamp" to "microamp" and "uvolt" to "microvolt". However, I didn't change
the unit names of the driver-internal variables in order to keep the
variable names short. Let me know if you think they should be changed too.
- Patch 9: Removed '|' after all "description" blocks in all three files.
- Patch 9: In the example of "mfd/richtek,rt5033.yaml" changed "i2c@0"
to "i2c".
- Patch 9: In the example of "mfd/richtek,rt5033.yaml" removed the last part
on the battery fuelgauge. It has its own I2C line and is therefore not a
subsidiary of the rt5033 MFD driver.
- Patch 9: Replaced units "uamp" by "microamp" and "uvolt" by "microvolt"
in the example of "mfd/richtek,rt5033.yaml" and the file
"power/supply/richtek,rt5033-charger.yaml".
- Patch 9: Changed name of regulators to lowercase in
"regulator/richtek,rt5033-regulator.yaml" and in the example of
"mfd/richtek,rt5033.yaml" (related to patch 5).
- Patch 9: Removed example from "regulator/richtek,rt5033-regulator.yaml".
It is already part of the example in "mfd/richtek,rt5033.yaml".
v1: https://lore.kernel.org/linux-pm/[email protected]/T/#t
The result of the patchset v2 can be seen at:
https://github.com/Jakko3/linux/blob/rt5033-charger_v2/drivers/power/supply/rt5033_charger.c
Jakob Hauser (8):
mfd: rt5033: Fix chip revision readout
mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines
mfd: rt5033: Apply preparatory changes before adding rt5033-charger
driver
regulator: rt5033: Change regulator names to lowercase
power: supply: rt5033_charger: Add RT5033 charger device driver
power: supply: rt5033_charger: Add cable detection and USB OTG supply
power: supply: rt5033_battery: Adopt status property from charger
dt-bindings: Add documentation for rt5033 mfd, regulator and charger
Stephan Gerhold (1):
mfd: rt5033: Drop rt5033-battery sub-device
.../bindings/mfd/richtek,rt5033.yaml | 90 +++
.../power/supply/richtek,rt5033-charger.yaml | 76 ++
.../regulator/richtek,rt5033-regulator.yaml | 24 +
drivers/mfd/rt5033.c | 8 +-
drivers/power/supply/Kconfig | 8 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/rt5033_battery.c | 24 +
drivers/power/supply/rt5033_charger.c | 724 ++++++++++++++++++
drivers/regulator/rt5033-regulator.c | 12 +-
include/linux/mfd/rt5033-private.h | 64 +-
include/linux/mfd/rt5033.h | 10 +-
11 files changed, 1008 insertions(+), 33 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
create mode 100644 drivers/power/supply/rt5033_charger.c
--
2.39.2
From: Stephan Gerhold <[email protected]>
The fuel gauge in the RT5033 PMIC (rt5033-battery) has its own I2C bus
and interrupt lines. Therefore, it is not part of the MFD device
and needs to be specified separately in the device tree.
Cc: Beomho Seo <[email protected]>
Cc: Chanwoo Choi <[email protected]>
Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
Signed-off-by: Stephan Gerhold <[email protected]>
Acked-by: Lee Jones <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/mfd/rt5033.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
index a5e520fe50a1..8029d444b794 100644
--- a/drivers/mfd/rt5033.c
+++ b/drivers/mfd/rt5033.c
@@ -40,9 +40,6 @@ static const struct mfd_cell rt5033_devs[] = {
{
.name = "rt5033-charger",
.of_compatible = "richtek,rt5033-charger",
- }, {
- .name = "rt5033-battery",
- .of_compatible = "richtek,rt5033-battery",
}, {
.name = "rt5033-led",
.of_compatible = "richtek,rt5033-led",
--
2.39.2
After reading the data from the DEVICE_ID register, mask 0x0f needs to be
applied to extract the revision of the chip [1].
The other part of the DEVICE_ID register, mask 0xf0, is a vendor identification
code. That's how it is set up at similar products of Richtek, e.g. RT9455 [2]
page 21 top.
[1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L484
[2] https://www.richtek.com/assets/product_file/RT9455/DS9455-00.pdf
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/mfd/rt5033.c | 5 +++--
include/linux/mfd/rt5033-private.h | 4 ++++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
index 8029d444b794..3eee4242ee02 100644
--- a/drivers/mfd/rt5033.c
+++ b/drivers/mfd/rt5033.c
@@ -55,7 +55,7 @@ static const struct regmap_config rt5033_regmap_config = {
static int rt5033_i2c_probe(struct i2c_client *i2c)
{
struct rt5033_dev *rt5033;
- unsigned int dev_id;
+ unsigned int dev_id, chip_rev;
int ret;
rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
@@ -78,7 +78,8 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
dev_err(&i2c->dev, "Device not found\n");
return -ENODEV;
}
- dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
+ chip_rev = dev_id & RT5033_CHIP_REV_MASK;
+ dev_info(&i2c->dev, "Device found (rev. %d)\n", chip_rev);
ret = regmap_add_irq_chip(rt5033->regmap, rt5033->irq,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
index 6bb432f6a96c..b035a67cec73 100644
--- a/include/linux/mfd/rt5033-private.h
+++ b/include/linux/mfd/rt5033-private.h
@@ -71,6 +71,10 @@ enum rt5033_reg {
/* RT5033 CHGCTRL2 register */
#define RT5033_CHGCTRL2_CV_MASK 0xfc
+/* RT5033 DEVICE_ID register */
+#define RT5033_VENDOR_ID_MASK 0xf0
+#define RT5033_CHIP_REV_MASK 0x0f
+
/* RT5033 CHGCTRL3 register */
#define RT5033_CHGCTRL3_CFO_EN_MASK 0x40
#define RT5033_CHGCTRL3_TIMER_MASK 0x38
--
2.39.2
Lowercase is preferred for node names.
Cc: Beomho Seo <[email protected]>
Cc: Chanwoo Choi <[email protected]>
Cc: Axel Lin <[email protected]>
Cc: ChiYuan Huang <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/regulator/rt5033-regulator.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/regulator/rt5033-regulator.c b/drivers/regulator/rt5033-regulator.c
index 2ba74f205543..dee272dc81df 100644
--- a/drivers/regulator/rt5033-regulator.c
+++ b/drivers/regulator/rt5033-regulator.c
@@ -41,8 +41,8 @@ static const struct regulator_ops rt5033_buck_ops = {
static const struct regulator_desc rt5033_supported_regulators[] = {
[RT5033_BUCK] = {
- .name = "BUCK",
- .of_match = of_match_ptr("BUCK"),
+ .name = "buck",
+ .of_match = of_match_ptr("buck"),
.regulators_node = of_match_ptr("regulators"),
.id = RT5033_BUCK,
.ops = &rt5033_buck_ops,
@@ -57,8 +57,8 @@ static const struct regulator_desc rt5033_supported_regulators[] = {
.vsel_mask = RT5033_BUCK_CTRL_MASK,
},
[RT5033_LDO] = {
- .name = "LDO",
- .of_match = of_match_ptr("LDO"),
+ .name = "ldo",
+ .of_match = of_match_ptr("ldo"),
.regulators_node = of_match_ptr("regulators"),
.id = RT5033_LDO,
.ops = &rt5033_buck_ops,
@@ -73,8 +73,8 @@ static const struct regulator_desc rt5033_supported_regulators[] = {
.vsel_mask = RT5033_LDO_CTRL_MASK,
},
[RT5033_SAFE_LDO] = {
- .name = "SAFE_LDO",
- .of_match = of_match_ptr("SAFE_LDO"),
+ .name = "safe_ldo",
+ .of_match = of_match_ptr("safe_ldo"),
.regulators_node = of_match_ptr("regulators"),
.id = RT5033_SAFE_LDO,
.ops = &rt5033_safe_ldo_ops,
--
2.39.2
The charger state mask RT5033_CHG_STAT_MASK should be 0x30 [1][2].
The high impedance mask RT5033_RT_HZ_MASK is actually value 0x02 [3] and is
assosiated to the RT5033 CHGCTRL1 register [4]. Accordingly also change
RT5033_CHARGER_HZ_ENABLE to 0x02 to avoid the need of a bit shift upon
application.
For input current limiting AICR mode, the define for the 1000 mA step was
missing [5]. Additionally add the define for DISABLE option. Concerning the
mask, remove RT5033_AICR_MODE_MASK because there is already
RT5033_CHGCTRL1_IAICR_MASK further up. They are redundant and the upper one
makes more sense to have the masks of a register colleted there as an
overview.
[1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L669-L682
[2] https://github.com/torvalds/linux/blob/v6.0/include/linux/mfd/rt5033-private.h#L59-L62
[3] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/include/linux/battery/charger/rt5033_charger.h#L44
[4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L223
[5] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L278
Signed-off-by: Jakob Hauser <[email protected]>
Acked-for-MFD-by: Lee Jones <[email protected]>
---
include/linux/mfd/rt5033-private.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
index b035a67cec73..b6773ebf4e6b 100644
--- a/include/linux/mfd/rt5033-private.h
+++ b/include/linux/mfd/rt5033-private.h
@@ -55,7 +55,7 @@ enum rt5033_reg {
};
/* RT5033 Charger state register */
-#define RT5033_CHG_STAT_MASK 0x20
+#define RT5033_CHG_STAT_MASK 0x30
#define RT5033_CHG_STAT_DISCHARGING 0x00
#define RT5033_CHG_STAT_FULL 0x10
#define RT5033_CHG_STAT_CHARGING 0x20
@@ -67,6 +67,7 @@ enum rt5033_reg {
/* RT5033 CHGCTRL1 register */
#define RT5033_CHGCTRL1_IAICR_MASK 0xe0
#define RT5033_CHGCTRL1_MODE_MASK 0x01
+#define RT5033_CHGCTRL1_HZ_MASK 0x02
/* RT5033 CHGCTRL2 register */
#define RT5033_CHGCTRL2_CV_MASK 0xfc
@@ -92,7 +93,6 @@ enum rt5033_reg {
/* RT5033 RT CTRL1 register */
#define RT5033_RT_CTRL1_UUG_MASK 0x02
-#define RT5033_RT_HZ_MASK 0x01
/* RT5033 control register */
#define RT5033_CTRL_FCCM_BUCK_MASK BIT(0)
@@ -119,13 +119,14 @@ enum rt5033_reg {
* register), AICR mode limits the input current. For example, the AIRC 100
* mode limits the input current to 100 mA.
*/
+#define RT5033_AICR_DISABLE 0x00
#define RT5033_AICR_100_MODE 0x20
#define RT5033_AICR_500_MODE 0x40
#define RT5033_AICR_700_MODE 0x60
#define RT5033_AICR_900_MODE 0x80
+#define RT5033_AICR_1000_MODE 0xa0
#define RT5033_AICR_1500_MODE 0xc0
#define RT5033_AICR_2000_MODE 0xe0
-#define RT5033_AICR_MODE_MASK 0xe0
/* RT5033 use internal timer need to set time */
#define RT5033_FAST_CHARGE_TIMER4 0x00
@@ -195,7 +196,7 @@ enum rt5033_reg {
/* RT5033 charger high impedance mode */
#define RT5033_CHARGER_HZ_DISABLE 0x00
-#define RT5033_CHARGER_HZ_ENABLE 0x01
+#define RT5033_CHARGER_HZ_ENABLE 0x02
/* RT5033 regulator BUCK output voltage uV */
#define RT5033_REGULATOR_BUCK_VOLTAGE_MIN 1000000U
--
2.39.2
Order the register blocks to have the masks in descending manner.
Add new defines for constant voltage shift (RT5033_CHGCTRL2_CV_SHIFT),
MIVR mask (RT5033_CHGCTRL4_MIVR_MASK), pre-charge current shift
(RT5033_CHGCTRL4_IPREC_SHIFT), internal timer disable
(RT5033_INT_TIMER_DISABLE), termination disable (RT5033_TE_DISABLE),
CFO disable (RT5033_CFO_DISABLE), UUG disable (RT5033_CHARGER_UUG_DISABLE).
The fast charge timer type needs to be written on mask 0x38
(RT5033_CHGCTRL3_TIMER_MASK). To avoid a bit shift on application, change the
values of the timer types to fit the mask. Added the timout duration as a
comment. And the timer between TIMER8 and TIMER12 is most likely TIMER10, see
e.g. RT5036 [1] page 28 bottom.
Add value options for MIVR (Minimum Input Voltage Regulation).
Move RT5033_TE_ENABLE_MASK to the block "RT5033 CHGCTRL1 register", in order
to have the masks of the register collected there. To fit the naming scheme,
rename it to RT5033_CHGCTRL1_TE_EN_MASK.
Move RT5033_CHG_MAX_CURRENT to the block "RT5033 charger fast-charge current".
Add new defines RT5033_CV_MAX_VOLTAGE and RT5033_CHG_MAX_PRE_CURRENT to the
blocks "RT5033 charger constant charge voltage" and "RT5033 charger pre-charge
current limits".
In include/linux/mfd/rt5033.h, turn power_supply "psy" into a pointer in order
to use it in devm_power_supply_register().
[1] https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
Signed-off-by: Jakob Hauser <[email protected]>
---
include/linux/mfd/rt5033-private.h | 53 ++++++++++++++++++++----------
include/linux/mfd/rt5033.h | 2 +-
2 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
index b6773ebf4e6b..0221f806d139 100644
--- a/include/linux/mfd/rt5033-private.h
+++ b/include/linux/mfd/rt5033-private.h
@@ -55,22 +55,24 @@ enum rt5033_reg {
};
/* RT5033 Charger state register */
+#define RT5033_CHG_STAT_TYPE_MASK 0x60
+#define RT5033_CHG_STAT_TYPE_PRE 0x20
+#define RT5033_CHG_STAT_TYPE_FAST 0x60
#define RT5033_CHG_STAT_MASK 0x30
#define RT5033_CHG_STAT_DISCHARGING 0x00
#define RT5033_CHG_STAT_FULL 0x10
#define RT5033_CHG_STAT_CHARGING 0x20
#define RT5033_CHG_STAT_NOT_CHARGING 0x30
-#define RT5033_CHG_STAT_TYPE_MASK 0x60
-#define RT5033_CHG_STAT_TYPE_PRE 0x20
-#define RT5033_CHG_STAT_TYPE_FAST 0x60
/* RT5033 CHGCTRL1 register */
#define RT5033_CHGCTRL1_IAICR_MASK 0xe0
-#define RT5033_CHGCTRL1_MODE_MASK 0x01
+#define RT5033_CHGCTRL1_TE_EN_MASK 0x08
#define RT5033_CHGCTRL1_HZ_MASK 0x02
+#define RT5033_CHGCTRL1_MODE_MASK 0x01
/* RT5033 CHGCTRL2 register */
#define RT5033_CHGCTRL2_CV_MASK 0xfc
+#define RT5033_CHGCTRL2_CV_SHIFT 0x02
/* RT5033 DEVICE_ID register */
#define RT5033_VENDOR_ID_MASK 0xf0
@@ -82,14 +84,15 @@ enum rt5033_reg {
#define RT5033_CHGCTRL3_TIMER_EN_MASK 0x01
/* RT5033 CHGCTRL4 register */
-#define RT5033_CHGCTRL4_EOC_MASK 0x07
+#define RT5033_CHGCTRL4_MIVR_MASK 0xe0
#define RT5033_CHGCTRL4_IPREC_MASK 0x18
+#define RT5033_CHGCTRL4_IPREC_SHIFT 0x03
+#define RT5033_CHGCTRL4_EOC_MASK 0x07
/* RT5033 CHGCTRL5 register */
-#define RT5033_CHGCTRL5_VPREC_MASK 0x0f
#define RT5033_CHGCTRL5_ICHG_MASK 0xf0
#define RT5033_CHGCTRL5_ICHG_SHIFT 0x04
-#define RT5033_CHG_MAX_CURRENT 0x0d
+#define RT5033_CHGCTRL5_VPREC_MASK 0x0f
/* RT5033 RT CTRL1 register */
#define RT5033_RT_CTRL1_UUG_MASK 0x02
@@ -128,20 +131,28 @@ enum rt5033_reg {
#define RT5033_AICR_1500_MODE 0xc0
#define RT5033_AICR_2000_MODE 0xe0
-/* RT5033 use internal timer need to set time */
-#define RT5033_FAST_CHARGE_TIMER4 0x00
-#define RT5033_FAST_CHARGE_TIMER6 0x01
-#define RT5033_FAST_CHARGE_TIMER8 0x02
-#define RT5033_FAST_CHARGE_TIMER9 0x03
-#define RT5033_FAST_CHARGE_TIMER12 0x04
-#define RT5033_FAST_CHARGE_TIMER14 0x05
-#define RT5033_FAST_CHARGE_TIMER16 0x06
+/* RT5033 charger minimum input voltage regulation */
+#define RT5033_CHARGER_MIVR_DISABLE 0x00
+#define RT5033_CHARGER_MIVR_4200MV 0x20
+#define RT5033_CHARGER_MIVR_4300MV 0x40
+#define RT5033_CHARGER_MIVR_4400MV 0x60
+#define RT5033_CHARGER_MIVR_4500MV 0x80
+#define RT5033_CHARGER_MIVR_4600MV 0xa0
+#define RT5033_CHARGER_MIVR_4700MV 0xc0
+#define RT5033_CHARGER_MIVR_4800MV 0xe0
+/* RT5033 use internal timer need to set time */
+#define RT5033_FAST_CHARGE_TIMER4 0x00 /* 4 hrs */
+#define RT5033_FAST_CHARGE_TIMER6 0x08 /* 6 hrs */
+#define RT5033_FAST_CHARGE_TIMER8 0x10 /* 8 hrs */
+#define RT5033_FAST_CHARGE_TIMER10 0x18 /* 10 hrs */
+#define RT5033_FAST_CHARGE_TIMER12 0x20 /* 12 hrs */
+#define RT5033_FAST_CHARGE_TIMER14 0x28 /* 14 hrs */
+#define RT5033_FAST_CHARGE_TIMER16 0x30 /* 16 hrs */
+
+#define RT5033_INT_TIMER_DISABLE 0x00
#define RT5033_INT_TIMER_ENABLE 0x01
-/* RT5033 charger termination enable mask */
-#define RT5033_TE_ENABLE_MASK 0x08
-
/*
* RT5033 charger opa mode. RT5033 has two opa modes for OTG: charger mode
* and boost mode.
@@ -150,25 +161,30 @@ enum rt5033_reg {
#define RT5033_BOOST_MODE 0x01
/* RT5033 charger termination enable */
+#define RT5033_TE_DISABLE 0x00
#define RT5033_TE_ENABLE 0x08
/* RT5033 charger CFO enable */
+#define RT5033_CFO_DISABLE 0x00
#define RT5033_CFO_ENABLE 0x40
/* RT5033 charger constant charge voltage (as in CHGCTRL2 register), uV */
#define RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN 3650000U
#define RT5033_CHARGER_CONST_VOLTAGE_STEP_NUM 25000U
#define RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX 4400000U
+#define RT5033_CV_MAX_VOLTAGE 0x1e
/* RT5033 charger pre-charge current limits (as in CHGCTRL4 register), uA */
#define RT5033_CHARGER_PRE_CURRENT_LIMIT_MIN 350000U
#define RT5033_CHARGER_PRE_CURRENT_STEP_NUM 100000U
#define RT5033_CHARGER_PRE_CURRENT_LIMIT_MAX 650000U
+#define RT5033_CHG_MAX_PRE_CURRENT 0x03
/* RT5033 charger fast-charge current (as in CHGCTRL5 register), uA */
#define RT5033_CHARGER_FAST_CURRENT_MIN 700000U
#define RT5033_CHARGER_FAST_CURRENT_STEP_NUM 100000U
#define RT5033_CHARGER_FAST_CURRENT_MAX 2000000U
+#define RT5033_CHG_MAX_CURRENT 0x0d
/*
* RT5033 charger const-charge end of charger current (
@@ -192,6 +208,7 @@ enum rt5033_reg {
* RT5033 charger UUG. It enables MOS auto control by H/W charger
* circuit.
*/
+#define RT5033_CHARGER_UUG_DISABLE 0x00
#define RT5033_CHARGER_UUG_ENABLE 0x02
/* RT5033 charger high impedance mode */
diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h
index 8f306ac15a27..e99e2ab0c1c1 100644
--- a/include/linux/mfd/rt5033.h
+++ b/include/linux/mfd/rt5033.h
@@ -51,7 +51,7 @@ struct rt5033_charger_data {
struct rt5033_charger {
struct device *dev;
struct rt5033_dev *rt5033;
- struct power_supply psy;
+ struct power_supply *psy;
struct rt5033_charger_data *chg;
};
--
2.39.2
This patch adds device driver of Richtek RT5033 PMIC. The driver supports
switching charger. rt5033 charger provides three charging modes. The charging
modes are pre-charge mode, fast charge mode and constant voltage mode. They
vary in charge rate, the charge parameters can be controlled by i2c interface.
Cc: Beomho Seo <[email protected]>
Cc: Chanwoo Choi <[email protected]>
Tested-by: Raymond Hackley <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/power/supply/Kconfig | 8 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/rt5033_charger.c | 463 ++++++++++++++++++++++++++
3 files changed, 472 insertions(+)
create mode 100644 drivers/power/supply/rt5033_charger.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index c78be9f322e6..ea11797670ca 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -766,6 +766,14 @@ config BATTERY_RT5033
The fuelgauge calculates and determines the battery state of charge
according to battery open circuit voltage.
+config CHARGER_RT5033
+ tristate "RT5033 battery charger support"
+ depends on MFD_RT5033
+ help
+ This adds support for battery charger in Richtek RT5033 PMIC.
+ The device supports pre-charge mode, fast charge mode and
+ constant voltage mode.
+
config CHARGER_RT9455
tristate "Richtek RT9455 battery charger driver"
depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 4adbfba02d05..dfc624bbcf1d 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o
obj-$(CONFIG_BATTERY_MAX17042) += max17042_battery.o
obj-$(CONFIG_BATTERY_MAX1721X) += max1721x_battery.o
obj-$(CONFIG_BATTERY_RT5033) += rt5033_battery.o
+obj-$(CONFIG_CHARGER_RT5033) += rt5033_charger.o
obj-$(CONFIG_CHARGER_RT9455) += rt9455_charger.o
obj-$(CONFIG_CHARGER_RT9467) += rt9467-charger.o
obj-$(CONFIG_CHARGER_RT9471) += rt9471.o
diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
new file mode 100644
index 000000000000..b5be43e6ebf5
--- /dev/null
+++ b/drivers/power/supply/rt5033_charger.c
@@ -0,0 +1,463 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Battery charger driver for RT5033
+ *
+ * Copyright (C) 2014 Samsung Electronics, Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/mfd/rt5033-private.h>
+#include <linux/mfd/rt5033.h>
+
+static int rt5033_get_charger_state(struct rt5033_charger *charger)
+{
+ struct regmap *regmap = charger->rt5033->regmap;
+ unsigned int reg_data;
+ int state;
+
+ if (!regmap)
+ return state;
+
+ regmap_read(regmap, RT5033_REG_CHG_STAT, ®_data);
+
+ switch (reg_data & RT5033_CHG_STAT_MASK) {
+ case RT5033_CHG_STAT_DISCHARGING:
+ state = POWER_SUPPLY_STATUS_DISCHARGING;
+ break;
+ case RT5033_CHG_STAT_CHARGING:
+ state = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case RT5033_CHG_STAT_FULL:
+ state = POWER_SUPPLY_STATUS_FULL;
+ break;
+ case RT5033_CHG_STAT_NOT_CHARGING:
+ state = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ break;
+ default:
+ state = POWER_SUPPLY_STATUS_UNKNOWN;
+ }
+
+ return state;
+}
+
+static int rt5033_get_charger_type(struct rt5033_charger *charger)
+{
+ struct regmap *regmap = charger->rt5033->regmap;
+ unsigned int reg_data;
+ int state;
+
+ regmap_read(regmap, RT5033_REG_CHG_STAT, ®_data);
+
+ switch (reg_data & RT5033_CHG_STAT_TYPE_MASK) {
+ case RT5033_CHG_STAT_TYPE_FAST:
+ state = POWER_SUPPLY_CHARGE_TYPE_FAST;
+ break;
+ case RT5033_CHG_STAT_TYPE_PRE:
+ state = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+ break;
+ default:
+ state = POWER_SUPPLY_CHARGE_TYPE_NONE;
+ }
+
+ return state;
+}
+
+static int rt5033_get_charger_current_limit(struct rt5033_charger *charger)
+{
+ struct regmap *regmap = charger->rt5033->regmap;
+ unsigned int state, reg_data, data;
+
+ regmap_read(regmap, RT5033_REG_CHG_CTRL5, ®_data);
+
+ state = (reg_data & RT5033_CHGCTRL5_ICHG_MASK)
+ >> RT5033_CHGCTRL5_ICHG_SHIFT;
+
+ data = RT5033_CHARGER_FAST_CURRENT_MIN +
+ RT5033_CHARGER_FAST_CURRENT_STEP_NUM * state;
+
+ return data;
+}
+
+static int rt5033_get_charger_const_voltage(struct rt5033_charger *charger)
+{
+ struct regmap *regmap = charger->rt5033->regmap;
+ unsigned int state, reg_data, data;
+
+ regmap_read(regmap, RT5033_REG_CHG_CTRL2, ®_data);
+
+ state = (reg_data & RT5033_CHGCTRL2_CV_MASK)
+ >> RT5033_CHGCTRL2_CV_SHIFT;
+
+ data = RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN +
+ RT5033_CHARGER_CONST_VOLTAGE_STEP_NUM * state;
+
+ return data;
+}
+
+static inline int rt5033_init_const_charge(struct rt5033_charger *charger)
+{
+ struct rt5033_charger_data *chg = charger->chg;
+ int ret;
+ unsigned int val;
+ u8 reg_data;
+
+ /* Set constant voltage mode */
+ if (chg->const_uvolt < RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN ||
+ chg->const_uvolt > RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX)
+ return -EINVAL;
+
+ if (chg->const_uvolt == RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN)
+ reg_data = 0x00;
+ else if (chg->const_uvolt == RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX)
+ reg_data = RT5033_CV_MAX_VOLTAGE;
+ else {
+ val = chg->const_uvolt;
+ val -= RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MIN;
+ val /= RT5033_CHARGER_CONST_VOLTAGE_STEP_NUM;
+ reg_data = val;
+ }
+
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL2,
+ RT5033_CHGCTRL2_CV_MASK,
+ reg_data << RT5033_CHGCTRL2_CV_SHIFT);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ /* Set end of charge current */
+ if (chg->eoc_uamp < RT5033_CHARGER_EOC_MIN ||
+ chg->eoc_uamp > RT5033_CHARGER_EOC_MAX)
+ return -EINVAL;
+
+ if (chg->eoc_uamp == RT5033_CHARGER_EOC_MIN)
+ reg_data = 0x01;
+ else if (chg->eoc_uamp == RT5033_CHARGER_EOC_MAX)
+ reg_data = 0x07;
+ else {
+ val = chg->eoc_uamp;
+ if (val < RT5033_CHARGER_EOC_REF) {
+ val -= RT5033_CHARGER_EOC_MIN;
+ val /= RT5033_CHARGER_EOC_STEP_NUM1;
+ reg_data = 0x01 + val;
+ } else if (val > RT5033_CHARGER_EOC_REF) {
+ val -= RT5033_CHARGER_EOC_REF;
+ val /= RT5033_CHARGER_EOC_STEP_NUM2;
+ reg_data = 0x04 + val;
+ } else {
+ reg_data = 0x04;
+ }
+ }
+
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL4,
+ RT5033_CHGCTRL4_EOC_MASK, reg_data);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static inline int rt5033_init_fast_charge(struct rt5033_charger *charger)
+{
+ struct rt5033_charger_data *chg = charger->chg;
+ int ret;
+ unsigned int val;
+ u8 reg_data;
+
+ /* Set limit input current */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+ RT5033_CHGCTRL1_IAICR_MASK, RT5033_AICR_2000_MODE);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ /* Set fast-charge mode charging current */
+ if (chg->fast_uamp < RT5033_CHARGER_FAST_CURRENT_MIN ||
+ chg->fast_uamp > RT5033_CHARGER_FAST_CURRENT_MAX)
+ return -EINVAL;
+
+ if (chg->fast_uamp == RT5033_CHARGER_FAST_CURRENT_MIN)
+ reg_data = 0x00;
+ else if (chg->fast_uamp == RT5033_CHARGER_FAST_CURRENT_MAX)
+ reg_data = RT5033_CHG_MAX_CURRENT;
+ else {
+ val = chg->fast_uamp;
+ val -= RT5033_CHARGER_FAST_CURRENT_MIN;
+ val /= RT5033_CHARGER_FAST_CURRENT_STEP_NUM;
+ reg_data = val;
+ }
+
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL5,
+ RT5033_CHGCTRL5_ICHG_MASK,
+ reg_data << RT5033_CHGCTRL5_ICHG_SHIFT);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static inline int rt5033_init_pre_charge(struct rt5033_charger *charger)
+{
+ struct rt5033_charger_data *chg = charger->chg;
+ int ret;
+ unsigned int val;
+ u8 reg_data;
+
+ /* Set pre-charge threshold voltage */
+ if (chg->pre_uvolt < RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MIN ||
+ chg->pre_uvolt > RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MAX)
+ return -EINVAL;
+
+ if (chg->pre_uvolt == RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MIN)
+ reg_data = 0x00;
+ else if (chg->pre_uvolt == RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MAX)
+ reg_data = 0x0f;
+ else {
+ val = chg->pre_uvolt;
+ val -= RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MIN;
+ val /= RT5033_CHARGER_PRE_THRESHOLD_STEP_NUM;
+ reg_data = val;
+ }
+
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL5,
+ RT5033_CHGCTRL5_VPREC_MASK, reg_data);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ /* Set pre-charge mode charging current */
+ if (chg->pre_uamp < RT5033_CHARGER_PRE_CURRENT_LIMIT_MIN ||
+ chg->pre_uamp > RT5033_CHARGER_PRE_CURRENT_LIMIT_MAX)
+ return -EINVAL;
+
+ if (chg->pre_uamp == RT5033_CHARGER_PRE_CURRENT_LIMIT_MIN)
+ reg_data = 0x00;
+ else if (chg->pre_uamp == RT5033_CHARGER_PRE_CURRENT_LIMIT_MAX)
+ reg_data = RT5033_CHG_MAX_PRE_CURRENT;
+ else {
+ val = chg->pre_uamp;
+ val -= RT5033_CHARGER_PRE_CURRENT_LIMIT_MIN;
+ val /= RT5033_CHARGER_PRE_CURRENT_STEP_NUM;
+ reg_data = val;
+ }
+
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL4,
+ RT5033_CHGCTRL4_IPREC_MASK,
+ reg_data << RT5033_CHGCTRL4_IPREC_SHIFT);
+ if (ret) {
+ dev_err(charger->dev, "Failed regmap update\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rt5033_charger_reg_init(struct rt5033_charger *charger)
+{
+ int ret = 0;
+
+ /* Enable charging termination */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+ RT5033_CHGCTRL1_TE_EN_MASK, RT5033_TE_ENABLE);
+ if (ret) {
+ dev_err(charger->dev, "Failed to enable charging termination.\n");
+ return -EINVAL;
+ }
+
+ /*
+ * Disable minimum input voltage regulation (MIVR), this improves
+ * the charging performance.
+ */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL4,
+ RT5033_CHGCTRL4_MIVR_MASK, RT5033_CHARGER_MIVR_DISABLE);
+ if (ret) {
+ dev_err(charger->dev, "Failed to disable MIVR.\n");
+ return -EINVAL;
+ }
+
+ ret = rt5033_init_pre_charge(charger);
+ if (ret)
+ return ret;
+
+ ret = rt5033_init_fast_charge(charger);
+ if (ret)
+ return ret;
+
+ ret = rt5033_init_const_charge(charger);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static enum power_supply_property rt5033_charger_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_CHARGE_TYPE,
+ POWER_SUPPLY_PROP_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
+ POWER_SUPPLY_PROP_MODEL_NAME,
+ POWER_SUPPLY_PROP_MANUFACTURER,
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int rt5033_charger_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct rt5033_charger *charger = power_supply_get_drvdata(psy);
+ int ret = 0;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_STATUS:
+ val->intval = rt5033_get_charger_state(charger);
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ val->intval = rt5033_get_charger_type(charger);
+ break;
+ case POWER_SUPPLY_PROP_CURRENT_MAX:
+ val->intval = rt5033_get_charger_current_limit(charger);
+ break;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+ val->intval = rt5033_get_charger_const_voltage(charger);
+ break;
+ case POWER_SUPPLY_PROP_MODEL_NAME:
+ val->strval = RT5033_CHARGER_MODEL;
+ break;
+ case POWER_SUPPLY_PROP_MANUFACTURER:
+ val->strval = RT5033_MANUFACTURER;
+ break;
+ case POWER_SUPPLY_PROP_ONLINE:
+ val->intval = (rt5033_get_charger_state(charger) ==
+ POWER_SUPPLY_STATUS_CHARGING);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static struct rt5033_charger_data *rt5033_charger_dt_init(
+ struct platform_device *pdev)
+{
+ struct rt5033_charger_data *chg;
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ if (!np) {
+ dev_err(&pdev->dev, "No charger of_node\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ chg = devm_kzalloc(&pdev->dev, sizeof(*chg), GFP_KERNEL);
+ if (!chg)
+ return ERR_PTR(-ENOMEM);
+
+ ret = of_property_read_u32(np, "richtek,pre-microamp", &chg->pre_uamp);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = of_property_read_u32(np, "richtek,pre-threshold-microvolt",
+ &chg->pre_uvolt);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = of_property_read_u32(np, "richtek,fast-microamp", &chg->fast_uamp);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = of_property_read_u32(np, "richtek,const-microvolt",
+ &chg->const_uvolt);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = of_property_read_u32(np, "richtek,eoc-microamp", &chg->eoc_uamp);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return chg;
+}
+
+static const struct power_supply_desc rt5033_charger_desc = {
+ .name = "rt5033-charger",
+ .type = POWER_SUPPLY_TYPE_USB,
+ .properties = rt5033_charger_props,
+ .num_properties = ARRAY_SIZE(rt5033_charger_props),
+ .get_property = rt5033_charger_get_property,
+};
+
+static int rt5033_charger_probe(struct platform_device *pdev)
+{
+ struct rt5033_charger *charger;
+ struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
+ struct power_supply_config psy_cfg = {};
+ int ret;
+
+ charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
+ if (!charger)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, charger);
+ charger->dev = &pdev->dev;
+ charger->rt5033 = rt5033;
+
+ charger->chg = rt5033_charger_dt_init(pdev);
+ if (IS_ERR_OR_NULL(charger->chg))
+ return -ENODEV;
+
+ ret = rt5033_charger_reg_init(charger);
+ if (ret)
+ return ret;
+
+ psy_cfg.of_node = pdev->dev.of_node;
+ psy_cfg.drv_data = charger;
+
+ charger->psy = devm_power_supply_register(&pdev->dev,
+ &rt5033_charger_desc,
+ &psy_cfg);
+ if (IS_ERR(charger->psy)) {
+ dev_err(&pdev->dev, "failed: power supply register\n");
+ return PTR_ERR(charger->psy);
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id rt5033_charger_id[] = {
+ { "rt5033-charger", },
+ { }
+};
+MODULE_DEVICE_TABLE(platform, rt5033_charger_id);
+
+static const struct of_device_id rt5033_charger_of_match[] = {
+ { .compatible = "richtek,rt5033-charger", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, rt5033_charger_of_match);
+
+static struct platform_driver rt5033_charger_driver = {
+ .driver = {
+ .name = "rt5033-charger",
+ .of_match_table = rt5033_charger_of_match,
+ },
+ .probe = rt5033_charger_probe,
+ .id_table = rt5033_charger_id,
+};
+module_platform_driver(rt5033_charger_driver);
+
+MODULE_DESCRIPTION("Richtek RT5033 charger driver");
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.39.2
Add device tree binding documentation for rt5033 multifunction device, voltage
regulator and battery charger.
Cc: Beomho Seo <[email protected]>
Cc: Chanwoo Choi <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
The patch is based on linux-next (tag "next-20230413").
.../bindings/mfd/richtek,rt5033.yaml | 90 +++++++++++++++++++
.../power/supply/richtek,rt5033-charger.yaml | 76 ++++++++++++++++
.../regulator/richtek,rt5033-regulator.yaml | 24 +++++
3 files changed, 190 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
new file mode 100644
index 000000000000..158036a57291
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/richtek,rt5033.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5033 Power Management Integrated Circuit
+
+maintainers:
+ - Jakob Hauser <[email protected]>
+
+description:
+ RT5033 is a multifunction device which includes battery charger, fuel gauge,
+ flash LED current source, LDO and synchronous Buck converter for portable
+ applications. It is interfaced to host controller using I2C interface. The
+ battery fuel gauge uses a separate I2C bus.
+
+properties:
+ compatible:
+ const: richtek,rt5033
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ regulators:
+ type: object
+ $ref: /schemas/regulator/richtek,rt5033-regulator.yaml#
+
+ charger:
+ type: object
+ $ref: /schemas/power/supply/richtek,rt5033-charger.yaml#
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@34 {
+ compatible = "richtek,rt5033";
+ reg = <0x34>;
+
+ interrupt-parent = <&msmgpio>;
+ interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pmic_int_default>;
+
+ regulators {
+ safe_ldo_reg: safe_ldo {
+ regulator-name = "safe_ldo";
+ regulator-min-microvolt = <4900000>;
+ regulator-max-microvolt = <4900000>;
+ regulator-always-on;
+ };
+ ldo_reg: ldo {
+ regulator-name = "ldo";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ };
+ buck_reg: buck {
+ regulator-name = "buck";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ };
+ };
+
+ charger {
+ compatible = "richtek,rt5033-charger";
+ richtek,pre-microamp = <450000>;
+ richtek,fast-microamp = <1000000>;
+ richtek,eoc-microamp = <150000>;
+ richtek,pre-threshold-microvolt = <3500000>;
+ richtek,const-microvolt = <4350000>;
+ extcon = <&muic>;
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
new file mode 100644
index 000000000000..439e0b7962f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5033 PIMC Battery Charger
+
+maintainers:
+ - Jakob Hauser <[email protected]>
+
+description:
+ The battery charger of the multifunction device RT5033 has to be instantiated
+ under sub-node named "charger" using the following format.
+
+properties:
+ compatible:
+ const: richtek,rt5033-charger
+
+ richtek,pre-microamp:
+ description:
+ Current of pre-charge mode. The pre-charge current levels are 350 mA to
+ 650 mA programmed by I2C per 100 mA.
+ maxItems: 1
+
+ richtek,fast-microamp:
+ description:
+ Current of fast-charge mode. The fast-charge current levels are 700 mA
+ to 2000 mA programmed by I2C per 100 mA.
+ maxItems: 1
+
+ richtek,eoc-microamp:
+ description:
+ This property is end of charge current. Its level ranges from 150 mA to
+ 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
+ in 100 mA steps.
+ maxItems: 1
+
+ richtek,pre-threshold-microvolt:
+ description:
+ Voltage of pre-charge mode. If the battery voltage is below the pre-charge
+ threshold voltage, the charger is in pre-charge mode with pre-charge current.
+ Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
+ maxItems: 1
+
+ richtek,const-microvolt:
+ description:
+ Battery regulation voltage of constant voltage mode. This voltage levels from
+ 3.65 V to 4.4 V by I2C per 0.025 V.
+ maxItems: 1
+
+ extcon:
+ description:
+ Phandle to the extcon device.
+ maxItems: 1
+
+required:
+ - richtek,pre-microamp
+ - richtek,fast-microamp
+ - richtek,eoc-microamp
+ - richtek,pre-threshold-microvolt
+ - richtek,const-microvolt
+
+additionalProperties: false
+
+examples:
+ - |
+ charger {
+ compatible = "richtek,rt5033-charger";
+ richtek,pre-microamp = <450000>;
+ richtek,fast-microamp = <1000000>;
+ richtek,eoc-microamp = <150000>;
+ richtek,pre-threshold-microvolt = <3500000>;
+ richtek,const-microvolt = <4350000>;
+ extcon = <&muic>;
+ };
diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
new file mode 100644
index 000000000000..66c8a0692e10
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rt5033-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5033 PIMC Voltage Regulator
+
+maintainers:
+ - Jakob Hauser <[email protected]>
+
+description:
+ The regulators of RT5033 have to be instantiated under a sub-node named
+ "regulators". For "safe_ldo" voltage there is only one value of 4.9 V. "ldo"
+ voltage ranges from 1.2 V to 3.0 V in 0.1 V steps. "buck" voltage ranges from
+ 1.0 V to 3.0 V in 0.1 V steps.
+
+patternProperties:
+ "^(safe_ldo|ldo|buck)$":
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ unevaluatedProperties: false
+
+additionalProperties: false
--
2.39.2
The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
can, let's get this value.
Tested-by: Raymond Hackley <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
index 5c04cf305219..48d4cccce4f6 100644
--- a/drivers/power/supply/rt5033_battery.c
+++ b/drivers/power/supply/rt5033_battery.c
@@ -12,6 +12,26 @@
#include <linux/mfd/rt5033-private.h>
#include <linux/mfd/rt5033.h>
+static int rt5033_battery_get_status(struct i2c_client *client)
+{
+ struct power_supply *charger;
+ union power_supply_propval val;
+ int ret;
+
+ charger = power_supply_get_by_name("rt5033-charger");
+ if (!charger)
+ return -ENODEV;
+
+ ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
+ if (ret) {
+ power_supply_put(charger);
+ return POWER_SUPPLY_STATUS_UNKNOWN;
+ }
+
+ power_supply_put(charger);
+ return val.intval;
+}
+
static int rt5033_battery_get_capacity(struct i2c_client *client)
{
struct rt5033_battery *battery = i2c_get_clientdata(client);
@@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_CAPACITY:
val->intval = rt5033_battery_get_capacity(battery->client);
break;
+ case POWER_SUPPLY_PROP_STATUS:
+ val->intval = rt5033_battery_get_status(battery->client);
+ break;
default:
return -EINVAL;
}
@@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
POWER_SUPPLY_PROP_VOLTAGE_OCV,
POWER_SUPPLY_PROP_PRESENT,
POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_STATUS,
};
static const struct regmap_config rt5033_battery_regmap_config = {
--
2.39.2
Implement cable detection by extcon and handle the driver according to the
connector type.
There are basically three types of action: "set_charging", "set_otg" and
"set_disconnect".
A forth helper function to "unset_otg" was added because this is used in both
"set_charging" and "set_disconnect". In the first case it covers the rather
rare event that someone changes from OTG to charging without disconnect. In
the second case, when disconnecting, the values are set back to the ones from
initialization to return into a defined state.
Additionally, there is "set_mivr". When connecting to e.g. a laptop/PC, the
minimum input voltage regulation (MIVR) shall prevent a voltage drop if the
cable or the supply is weak. The MIVR value is set to 4600MV, same as in the
Android driver [1]. When disconnecting, MIVR is set back to DISABLED.
In the function rt5033_get_charger_state(): When in OTG mode, the chip
reports status "charging". Change this to "discharging" because there is
no charging going on in OTG mode [2].
[1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L499
[2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L686-L687
Tested-by: Raymond Hackley <[email protected]>
Signed-off-by: Jakob Hauser <[email protected]>
---
drivers/power/supply/rt5033_charger.c | 265 +++++++++++++++++++++++++-
include/linux/mfd/rt5033.h | 8 +
2 files changed, 271 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
index b5be43e6ebf5..dd71233d554b 100644
--- a/drivers/power/supply/rt5033_charger.c
+++ b/drivers/power/supply/rt5033_charger.c
@@ -10,7 +10,10 @@
* published by the Free Software Foundation.
*/
+#include <linux/devm-helpers.h>
+#include <linux/extcon.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
#include <linux/mfd/rt5033-private.h>
@@ -44,6 +47,10 @@ static int rt5033_get_charger_state(struct rt5033_charger *charger)
state = POWER_SUPPLY_STATUS_UNKNOWN;
}
+ /* For OTG mode, RT5033 would still report "charging" */
+ if (charger->otg)
+ state = POWER_SUPPLY_STATUS_DISCHARGING;
+
return state;
}
@@ -132,6 +139,9 @@ static inline int rt5033_init_const_charge(struct rt5033_charger *charger)
return -EINVAL;
}
+ /* Store that value for later usage */
+ charger->cv_regval = reg_data;
+
/* Set end of charge current */
if (chg->eoc_uamp < RT5033_CHARGER_EOC_MIN ||
chg->eoc_uamp > RT5033_CHARGER_EOC_MAX)
@@ -303,6 +313,152 @@ static int rt5033_charger_reg_init(struct rt5033_charger *charger)
return 0;
}
+static int rt5033_charger_set_otg(struct rt5033_charger *charger)
+{
+ int ret;
+
+ mutex_lock(&charger->lock);
+
+ /* Set OTG boost v_out to 5 volts */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL2,
+ RT5033_CHGCTRL2_CV_MASK,
+ 0x37 << RT5033_CHGCTRL2_CV_SHIFT);
+ if (ret) {
+ dev_err(charger->dev, "Failed set OTG boost v_out\n");
+ return -EINVAL;
+ }
+
+ /* Set operation mode to OTG */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+ RT5033_CHGCTRL1_MODE_MASK, RT5033_BOOST_MODE);
+ if (ret) {
+ dev_err(charger->dev, "Failed to update OTG mode.\n");
+ return -EINVAL;
+ }
+
+ /* In case someone switched from charging to OTG directly */
+ if (charger->online)
+ charger->online = false;
+
+ charger->otg = true;
+
+ mutex_unlock(&charger->lock);
+
+ return 0;
+}
+
+static int rt5033_charger_unset_otg(struct rt5033_charger *charger)
+{
+ int ret;
+ u8 data;
+
+ /* Restore constant voltage for charging */
+ data = charger->cv_regval;
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL2,
+ RT5033_CHGCTRL2_CV_MASK,
+ data << RT5033_CHGCTRL2_CV_SHIFT);
+ if (ret) {
+ dev_err(charger->dev, "Failed to restore constant voltage\n");
+ return -EINVAL;
+ }
+
+ /* Set operation mode to charging */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+ RT5033_CHGCTRL1_MODE_MASK, RT5033_CHARGER_MODE);
+ if (ret) {
+ dev_err(charger->dev, "Failed to update charger mode.\n");
+ return -EINVAL;
+ }
+
+ charger->otg = false;
+
+ return 0;
+}
+
+static int rt5033_charger_set_charging(struct rt5033_charger *charger)
+{
+ int ret;
+
+ mutex_lock(&charger->lock);
+
+ /* In case someone switched from OTG to charging directly */
+ if (charger->otg) {
+ ret = rt5033_charger_unset_otg(charger);
+ if (ret)
+ return -EINVAL;
+ }
+
+ charger->online = true;
+
+ mutex_unlock(&charger->lock);
+
+ return 0;
+}
+
+static int rt5033_charger_set_mivr(struct rt5033_charger *charger)
+{
+ int ret;
+
+ mutex_lock(&charger->lock);
+
+ /*
+ * When connected via USB connector type SDP (Standard Downstream Port),
+ * the minimum input voltage regulation (MIVR) should be enabled. It
+ * prevents an input voltage drop due to insufficient current provided
+ * by the adapter or USB input. As a downside, it may reduces the
+ * charging current and thus slows the charging.
+ */
+ ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL4,
+ RT5033_CHGCTRL4_MIVR_MASK, RT5033_CHARGER_MIVR_4600MV);
+ if (ret) {
+ dev_err(charger->dev, "Failed to set MIVR level.\n");
+ return -EINVAL;
+ }
+
+ charger->mivr_enabled = true;
+
+ mutex_unlock(&charger->lock);
+
+ /* Beyond this, do the same steps like setting charging */
+ rt5033_charger_set_charging(charger);
+
+ return 0;
+}
+
+static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
+{
+ int ret;
+
+ mutex_lock(&charger->lock);
+
+ /* Disable MIVR if enabled */
+ if (charger->mivr_enabled) {
+ ret = regmap_update_bits(charger->rt5033->regmap,
+ RT5033_REG_CHG_CTRL4,
+ RT5033_CHGCTRL4_MIVR_MASK,
+ RT5033_CHARGER_MIVR_DISABLE);
+ if (ret) {
+ dev_err(charger->dev, "Failed to disable MIVR.\n");
+ return -EINVAL;
+ }
+
+ charger->mivr_enabled = false;
+ }
+
+ if (charger->otg) {
+ ret = rt5033_charger_unset_otg(charger);
+ if (ret)
+ return -EINVAL;
+ }
+
+ if (charger->online)
+ charger->online = false;
+
+ mutex_unlock(&charger->lock);
+
+ return 0;
+}
+
static enum power_supply_property rt5033_charger_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_CHARGE_TYPE,
@@ -340,8 +496,7 @@ static int rt5033_charger_get_property(struct power_supply *psy,
val->strval = RT5033_MANUFACTURER;
break;
case POWER_SUPPLY_PROP_ONLINE:
- val->intval = (rt5033_get_charger_state(charger) ==
- POWER_SUPPLY_STATUS_CHARGING);
+ val->intval = charger->online;
break;
default:
return -EINVAL;
@@ -391,6 +546,86 @@ static struct rt5033_charger_data *rt5033_charger_dt_init(
return chg;
}
+static void rt5033_charger_extcon_work(struct work_struct *work)
+{
+ struct rt5033_charger *charger =
+ container_of(work, struct rt5033_charger, extcon_work);
+ struct extcon_dev *edev = charger->edev;
+ int connector, state;
+ int ret;
+
+ for (connector = EXTCON_USB_HOST; connector <= EXTCON_CHG_USB_PD;
+ connector++) {
+ state = extcon_get_state(edev, connector);
+ if (state == 1)
+ break;
+ }
+
+ /*
+ * Adding a delay between extcon notification and extcon action. This
+ * makes extcon action execution more reliable. Without the delay the
+ * execution sometimes fails, possibly because the chip is busy or not
+ * ready.
+ */
+ msleep(100);
+
+ switch (connector) {
+ case EXTCON_CHG_USB_SDP:
+ ret = rt5033_charger_set_mivr(charger);
+ if (ret) {
+ dev_err(charger->dev, "failed to set USB mode\n");
+ break;
+ }
+ dev_info(charger->dev, "USB mode. connector type: %d\n",
+ connector);
+ break;
+ case EXTCON_CHG_USB_DCP:
+ case EXTCON_CHG_USB_CDP:
+ case EXTCON_CHG_USB_ACA:
+ case EXTCON_CHG_USB_FAST:
+ case EXTCON_CHG_USB_SLOW:
+ case EXTCON_CHG_WPT:
+ case EXTCON_CHG_USB_PD:
+ ret = rt5033_charger_set_charging(charger);
+ if (ret) {
+ dev_err(charger->dev, "failed to set charging\n");
+ break;
+ }
+ dev_info(charger->dev, "charging. connector type: %d\n",
+ connector);
+ break;
+ case EXTCON_USB_HOST:
+ ret = rt5033_charger_set_otg(charger);
+ if (ret) {
+ dev_err(charger->dev, "failed to set OTG\n");
+ break;
+ }
+ dev_info(charger->dev, "OTG enabled\n");
+ break;
+ default:
+ ret = rt5033_charger_set_disconnect(charger);
+ if (ret) {
+ dev_err(charger->dev, "failed to set disconnect\n");
+ break;
+ }
+ dev_info(charger->dev, "disconnected\n");
+ break;
+ }
+
+ power_supply_changed(charger->psy);
+}
+
+static int rt5033_charger_extcon_notifier(struct notifier_block *nb,
+ unsigned long event, void *param)
+{
+ struct rt5033_charger *charger =
+ container_of(nb, struct rt5033_charger, extcon_nb);
+
+ schedule_work(&charger->extcon_work);
+
+ return NOTIFY_OK;
+}
+
static const struct power_supply_desc rt5033_charger_desc = {
.name = "rt5033-charger",
.type = POWER_SUPPLY_TYPE_USB,
@@ -413,6 +648,7 @@ static int rt5033_charger_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, charger);
charger->dev = &pdev->dev;
charger->rt5033 = rt5033;
+ mutex_init(&charger->lock);
charger->chg = rt5033_charger_dt_init(pdev);
if (IS_ERR_OR_NULL(charger->chg))
@@ -433,6 +669,31 @@ static int rt5033_charger_probe(struct platform_device *pdev)
return PTR_ERR(charger->psy);
}
+ /*
+ * Extcon support is not vital for the charger to work. If no extcon
+ * is available, just emit a warning and leave the probe function.
+ */
+ charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0);
+ if (IS_ERR(charger->edev)) {
+ dev_warn(&pdev->dev, "no extcon phandle found in device-tree\n");
+ goto out;
+ }
+
+ ret = devm_work_autocancel(&pdev->dev, &charger->extcon_work,
+ rt5033_charger_extcon_work);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to initialize extcon work\n");
+ return ret;
+ }
+
+ charger->extcon_nb.notifier_call = rt5033_charger_extcon_notifier;
+ ret = devm_extcon_register_notifier_all(&pdev->dev, charger->edev,
+ &charger->extcon_nb);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register extcon notifier\n");
+ return ret;
+ }
+out:
return 0;
}
diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h
index e99e2ab0c1c1..d2c613764756 100644
--- a/include/linux/mfd/rt5033.h
+++ b/include/linux/mfd/rt5033.h
@@ -53,6 +53,14 @@ struct rt5033_charger {
struct rt5033_dev *rt5033;
struct power_supply *psy;
struct rt5033_charger_data *chg;
+ struct extcon_dev *edev;
+ struct notifier_block extcon_nb;
+ struct work_struct extcon_work;
+ struct mutex lock;
+ bool online;
+ bool otg;
+ bool mivr_enabled;
+ u8 cv_regval;
};
#endif /* __RT5033_H__ */
--
2.39.2
On 16/04/2023 14:44, Jakob Hauser wrote:
> This patchset adds the charger driver "rt5033-charger". It is part of the
> multifunction device rt5033. The patchset is based on an older version by
> Beomho Seo of March 2015. For more information on the history and setup of
> the patchset see the cover sheet of version v1, there is a link further down
> below the changelog.
>
> In patch 9 I didn't change the extcon phandle, I haven't received any answer
> on this.
>
> Changes in v2:
> - Rebased to linux-next (20230413), as suggested by Lee.
> - The v1 patch 3 "mfd: rt5033: Fix comments and style in includes" vanished
> as it got applied already.
> - Dropped the v1 patch 8 "power: supply: rt5033_charger: Make use of high
> impedance mode". The high impedance mode is kind of a sleep mode for power
> saving. It turned out that it might complicate a future implementation of
> an rt5033 flash LED driver. Therefore drop it for now. The high impedance
> mode could be added at a later date as a power saving improvement.
> - Patch 2: Changed variable name "data" back to original "dev_id".
> - New patch 5: Changed name of regulators to lowercase, as suggested by Rob.
> - Patch 6: In function "rt5033_charger_dt_init" replaced the devicetree units
> "uamp" to "microamp" and "uvolt" to "microvolt". However, I didn't change
> the unit names of the driver-internal variables in order to keep the
> variable names short. Let me know if you think they should be changed too.
> - Patch 9: Removed '|' after all "description" blocks in all three files.
> - Patch 9: In the example of "mfd/richtek,rt5033.yaml" changed "i2c@0"
> to "i2c".
> - Patch 9: In the example of "mfd/richtek,rt5033.yaml" removed the last part
> on the battery fuelgauge. It has its own I2C line and is therefore not a
> subsidiary of the rt5033 MFD driver.
> - Patch 9: Replaced units "uamp" by "microamp" and "uvolt" by "microvolt"
> in the example of "mfd/richtek,rt5033.yaml" and the file
> "power/supply/richtek,rt5033-charger.yaml".
> - Patch 9: Changed name of regulators to lowercase in
> "regulator/richtek,rt5033-regulator.yaml" and in the example of
> "mfd/richtek,rt5033.yaml" (related to patch 5).
> - Patch 9: Removed example from "regulator/richtek,rt5033-regulator.yaml".
> It is already part of the example in "mfd/richtek,rt5033.yaml".
Thanks for describing the changes. It's much more readable for us if
such complex changelog is put in each patch under --- .
Best regards,
Krzysztof
On 16/04/2023 14:44, Jakob Hauser wrote:
> Lowercase is preferred for node names.
>
This will break all existing users. In-tree and out-of-tree. Where is
the binding update?
Best regards,
Krzysztof
On 16/04/2023 14:44, Jakob Hauser wrote:
> Add device tree binding documentation for rt5033 multifunction device, voltage
> regulator and battery charger.
Subject: drop second/last, redundant "documentation". The "dt-bindings"
prefix is already stating that these are documentation.
>
> Cc: Beomho Seo <[email protected]>
> Cc: Chanwoo Choi <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> The patch is based on linux-next (tag "next-20230413").
>
> .../bindings/mfd/richtek,rt5033.yaml | 90 +++++++++++++++++++
> .../power/supply/richtek,rt5033-charger.yaml | 76 ++++++++++++++++
> .../regulator/richtek,rt5033-regulator.yaml | 24 +++++
> 3 files changed, 190 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
> create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
> create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@34 {
> + compatible = "richtek,rt5033";
> + reg = <0x34>;
> +
> + interrupt-parent = <&msmgpio>;
> + interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pmic_int_default>;
> +
> + regulators {
> + safe_ldo_reg: safe_ldo {
If you could change it: No underscores in node names... but you cannot.
This is old driver so you will break the users.
> + regulator-name = "safe_ldo";
> + regulator-min-microvolt = <4900000>;
> + regulator-max-microvolt = <4900000>;
> + regulator-always-on;
> + };
> + ldo_reg: ldo {
> + regulator-name = "ldo";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + };
> + buck_reg: buck {
> + regulator-name = "buck";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + };
> + };
> +
> + charger {
> + compatible = "richtek,rt5033-charger";
> + richtek,pre-microamp = <450000>;
> + richtek,fast-microamp = <1000000>;
> + richtek,eoc-microamp = <150000>;
> + richtek,pre-threshold-microvolt = <3500000>;
> + richtek,const-microvolt = <4350000>;
> + extcon = <&muic>;
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
> new file mode 100644
> index 000000000000..439e0b7962f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5033 PIMC Battery Charger
> +
> +maintainers:
> + - Jakob Hauser <[email protected]>
> +
> +description:
> + The battery charger of the multifunction device RT5033 has to be instantiated
> + under sub-node named "charger" using the following format.
> +
> +properties:
> + compatible:
> + const: richtek,rt5033-charger
> +
> + richtek,pre-microamp:
> + description:
> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
> + 650 mA programmed by I2C per 100 mA.
minimum:
maximum:
multipleOf: 100
Same for other cases.
> + maxItems: 1
> +
> + richtek,fast-microamp:
> + description:
> + Current of fast-charge mode. The fast-charge current levels are 700 mA
> + to 2000 mA programmed by I2C per 100 mA.
> + maxItems: 1
> +
> + richtek,eoc-microamp:
> + description:
> + This property is end of charge current. Its level ranges from 150 mA to
> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
> + in 100 mA steps.
> + maxItems: 1
> +
> + richtek,pre-threshold-microvolt:
> + description:
> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
> + maxItems: 1
> +
> + richtek,const-microvolt:
> + description:
> + Battery regulation voltage of constant voltage mode. This voltage levels from
> + 3.65 V to 4.4 V by I2C per 0.025 V.
> + maxItems: 1
> +
> + extcon:
> + description:
> + Phandle to the extcon device.
> + maxItems: 1
> +
> +required:
> + - richtek,pre-microamp
> + - richtek,fast-microamp
> + - richtek,eoc-microamp
> + - richtek,pre-threshold-microvolt
> + - richtek,const-microvolt
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + charger {
> + compatible = "richtek,rt5033-charger";
> + richtek,pre-microamp = <450000>;
> + richtek,fast-microamp = <1000000>;
> + richtek,eoc-microamp = <150000>;
> + richtek,pre-threshold-microvolt = <3500000>;
> + richtek,const-microvolt = <4350000>;
> + extcon = <&muic>;
> + };
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
> new file mode 100644
> index 000000000000..66c8a0692e10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rt5033-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5033 PIMC Voltage Regulator
You should explain in commit msg that you document existing driver in
the Linux kernel. We would not cut some slack, e.g. stricter rules
applied to new bindings.
> +
> +maintainers:
> + - Jakob Hauser <[email protected]>
> +
> +description:
> + The regulators of RT5033 have to be instantiated under a sub-node named
> + "regulators". For "safe_ldo" voltage there is only one value of 4.9 V. "ldo"
> + voltage ranges from 1.2 V to 3.0 V in 0.1 V steps. "buck" voltage ranges from
> + 1.0 V to 3.0 V in 0.1 V steps.
> +
> +patternProperties:
> + "^(safe_ldo|ldo|buck)$":
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
Just squash it with parent schema. No real benefits of having regulators
separate - it's very small one.
Best regards,
Krzysztof
Hi Krzysztof,
On 16.04.23 20:32, Krzysztof Kozlowski wrote:
> On 16/04/2023 14:44, Jakob Hauser wrote:
>> Lowercase is preferred for node names.
>
> This will break all existing users. In-tree and out-of-tree. Where is
> the binding update?
In my reply to Rob's comments in v1 I was pointing out that this will
affect an existing driver. There was no reaction.
As far as I can see, there is no in-tree usage yet. Though I can't tell
about out-of-tree usage. Although if there is, adding the rt5033-charger
driver might already causes the need for changes.
Well, to stay on the safe side, I'll drop this patch in v3 and will
change the bindings (patch 9) back to uppercase.
Kind regards,
Jakob
Hi Krzysztof,
On 16.04.23 20:39, Krzysztof Kozlowski wrote:
> On 16/04/2023 14:44, Jakob Hauser wrote:
>> Add device tree binding documentation for rt5033 multifunction device, voltage
>> regulator and battery charger.
>
> Subject: drop second/last, redundant "documentation". The "dt-bindings"
> prefix is already stating that these are documentation.
If I understand correctly, the new subject would be:
"dt-bindings: Add rt5033 mfd, regulator and charger"
...
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + pmic@34 {
>> + compatible = "richtek,rt5033";
>> + reg = <0x34>;
>> +
>> + interrupt-parent = <&msmgpio>;
>> + interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pmic_int_default>;
>> +
>> + regulators {
>> + safe_ldo_reg: safe_ldo {
>
> If you could change it: No underscores in node names... but you cannot.
> This is old driver so you will break the users.
As discussed on patch 5, I'll leave the regulator names unchanged. Thus,
I'll reset them to the original uppercase spelling.
>> + regulator-name = "safe_ldo";
>> + regulator-min-microvolt = <4900000>;
>> + regulator-max-microvolt = <4900000>;
>> + regulator-always-on;
>> + };
>> + ldo_reg: ldo {
>> + regulator-name = "ldo";
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>> + };
>> + buck_reg: buck {
>> + regulator-name = "buck";
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + };
>> + };
...
>> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> new file mode 100644
>> index 000000000000..439e0b7962f3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> @@ -0,0 +1,76 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Battery Charger
>> +
>> +maintainers:
>> + - Jakob Hauser <[email protected]>
>> +
>> +description:
>> + The battery charger of the multifunction device RT5033 has to be instantiated
>> + under sub-node named "charger" using the following format.
>> +
>> +properties:
>> + compatible:
>> + const: richtek,rt5033-charger
>> +
>> + richtek,pre-microamp:
>> + description:
>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
>> + 650 mA programmed by I2C per 100 mA.
>
> minimum:
> maximum:
> multipleOf: 100
>
> Same for other cases.
The "multipleOf: 100" doesn't seen appropriate to me when the choice is
350, 450, 550, 650. Those are not multiples of 100. It's more of a step
size. I didn't find a general property for step size. Listing them as
"enum" would be another possibility, I guess, but not an elegant one.
Especially for property "richtek,const-microvolt" there are 30
possibilities.
Using "multipleOf" and unit microamp, the block would then look like this:
richtek,pre-microamp:
description:
Current of pre-charge mode. The pre-charge current levels are
350 mA to 650 mA programmed by I2C per 100 mA.
minimum: 350000
maximum: 650000
multipleOf: 100000
maxItems: 1
Or possibly better readable the following way:
richtek,pre-microamp:
description:
Current of pre-charge mode. The pre-charge current levels are
350 mA to 650 mA programmed by I2C per 100 mA.
maxItems: 1
items:
minimum: 350000
maximum: 650000
multipleOf: 100000
>> + maxItems: 1
>> +
>> + richtek,fast-microamp:
>> + description:
>> + Current of fast-charge mode. The fast-charge current levels are 700 mA
>> + to 2000 mA programmed by I2C per 100 mA.
>> + maxItems: 1
>> +
>> + richtek,eoc-microamp:
>> + description:
>> + This property is end of charge current. Its level ranges from 150 mA to
>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>> + in 100 mA steps.
>> + maxItems: 1
Here are two different step sizes. The first few are 50 mA steps (150,
200, 250, 300 mA) and then it changes to 100 mA steps (300, 400, 500,
600 mA). How to deal with that? Again I guess "enum" would be a
possibility, but again not a nice one.
>> +
>> + richtek,pre-threshold-microvolt:
>> + description:
>> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
>> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
>> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
>> + maxItems: 1
>> +
>> + richtek,const-microvolt:
>> + description:
>> + Battery regulation voltage of constant voltage mode. This voltage levels from
>> + 3.65 V to 4.4 V by I2C per 0.025 V.
>> + maxItems: 1
>> +
>> + extcon:
>> + description:
>> + Phandle to the extcon device.
>> + maxItems: 1
...
>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> new file mode 100644
>> index 000000000000..66c8a0692e10
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> @@ -0,0 +1,24 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5033-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Voltage Regulator
>
> You should explain in commit msg that you document existing driver in
> the Linux kernel. We would not cut some slack, e.g. stricter rules
> applied to new bindings.
>
>> +
>> +maintainers:
>> + - Jakob Hauser <[email protected]>
>> +
>> +description:
>> + The regulators of RT5033 have to be instantiated under a sub-node named
>> + "regulators". For "safe_ldo" voltage there is only one value of 4.9 V. "ldo"
>> + voltage ranges from 1.2 V to 3.0 V in 0.1 V steps. "buck" voltage ranges from
>> + 1.0 V to 3.0 V in 0.1 V steps.
>> +
>> +patternProperties:
>> + "^(safe_ldo|ldo|buck)$":
>> + type: object
>> + $ref: /schemas/regulator/regulator.yaml#
>
> Just squash it with parent schema. No real benefits of having regulators
> separate - it's very small one.
OK, I'll squash the regulator schema into the mfd schema.
Kind regards,
Jakob
On 18/04/2023 23:24, Jakob Hauser wrote:
> Hi Krzysztof,
>
> On 16.04.23 20:32, Krzysztof Kozlowski wrote:
>> On 16/04/2023 14:44, Jakob Hauser wrote:
>>> Lowercase is preferred for node names.
>>
>> This will break all existing users. In-tree and out-of-tree. Where is
>> the binding update?
>
> In my reply to Rob's comments in v1 I was pointing out that this will
> affect an existing driver. There was no reaction.
>
> As far as I can see, there is no in-tree usage yet. Though I can't tell
> about out-of-tree usage. Although if there is, adding the rt5033-charger
> driver might already causes the need for changes.
>
> Well, to stay on the safe side, I'll drop this patch in v3 and will
> change the bindings (patch 9) back to uppercase.
>
Your v1 binding patch did not explain that you document existing ABI, so
you got comments like for a new binding. This is not really new binding,
is it?
Best regards,
Krzysztof
On 18/04/2023 23:37, Jakob Hauser wrote:
>>> +properties:
>>> + compatible:
>>> + const: richtek,rt5033-charger
>>> +
>>> + richtek,pre-microamp:
>>> + description:
>>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
>>> + 650 mA programmed by I2C per 100 mA.
>>
>> minimum:
>> maximum:
>> multipleOf: 100
>>
>> Same for other cases.
>
> The "multipleOf: 100" doesn't seen appropriate to me when the choice is
> 350, 450, 550, 650. Those are not multiples of 100. It's more of a step
> size. I didn't find a general property for step size. Listing them as
> "enum" would be another possibility, I guess, but not an elegant one.
> Especially for property "richtek,const-microvolt" there are 30
> possibilities.
Ahh, right. You can use enum here and min/max for other cases, where
multipleOf cannot be used.
>>> + richtek,eoc-microamp:
>>> + description:
>>> + This property is end of charge current. Its level ranges from 150 mA to
>>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>>> + in 100 mA steps.
>>> + maxItems: 1
>
> Here are two different step sizes. The first few are 50 mA steps (150,
> 200, 250, 300 mA) and then it changes to 100 mA steps (300, 400, 500,
> 600 mA). How to deal with that? Again I guess "enum" would be a
> possibility, but again not a nice one.
enum
Best regards,
Krzysztof
Hi Krzysztof,
On 19.04.23 10:40, Krzysztof Kozlowski wrote:
> On 18/04/2023 23:24, Jakob Hauser wrote:
>> On 16.04.23 20:32, Krzysztof Kozlowski wrote:
>>> On 16/04/2023 14:44, Jakob Hauser wrote:
>>>> Lowercase is preferred for node names.
>>>
>>> This will break all existing users. In-tree and out-of-tree. Where is
>>> the binding update?
>>
>> In my reply to Rob's comments in v1 I was pointing out that this will
>> affect an existing driver. There was no reaction.
>>
>> As far as I can see, there is no in-tree usage yet. Though I can't tell
>> about out-of-tree usage. Although if there is, adding the rt5033-charger
>> driver might already causes the need for changes.
>>
>> Well, to stay on the safe side, I'll drop this patch in v3 and will
>> change the bindings (patch 9) back to uppercase.
>
> Your v1 binding patch did not explain that you document existing ABI, so
> you got comments like for a new binding. This is not really new binding,
> is it?
The bindings for the mfd and regulator are new, even though the drivers
are already existing. Sorry for not being clear on this in v1. This is
due to historic reasons of the old patchset, more information on that
further down.
The current situation in the mainline kernel is as follows.
drivers
-------
rt5033: drivers/mfd/rt5033.c
rt5033-regulator: drivers/regulator/rt5033-regulator.c
rt5033-charger: not existent
rt5033-battery: drivers/power/supply/rt5033_battery.c
rt5033-leds: not existent
bindings
--------
rt5033: not existent
rt5033-regulator: not existent
rt5033-charger: not existent
rt5033-battery: .../bindings/power/supply/richtek,rt5033-battery.yaml
rt5033-leds: not existent
The reason for that discrepancy:
RT5033 mfd, regulator and fuelgauge drivers were applied but charger &
documentation didn't make [1]. They were submitted again but it phased
out at that point, last known state is [2]. The LEDs are also a phased
out patchset [3]. The fuelgauge binding was added not so long ago by
Stephan [4].
[1]
https://lore.kernel.org/all/[email protected]/T/#t
[2]
https://lore.kernel.org/lkml/[email protected]/T/#t
[3]
https://lore.kernel.org/linux-leds/[email protected]/T/#u
[4]
https://lore.kernel.org/linux-devicetree/[email protected]/T/#m197b5719a5d37b17ba4ff9f3b3ff4bd4efcda71e
Kind regards,
Jakob
Hi Krzysztof,
On 19.04.23 10:42, Krzysztof Kozlowski wrote:
> On 18/04/2023 23:37, Jakob Hauser wrote:
>
>>>> +properties:
>>>> + compatible:
>>>> + const: richtek,rt5033-charger
>>>> +
>>>> + richtek,pre-microamp:
>>>> + description:
>>>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
>>>> + 650 mA programmed by I2C per 100 mA.
>>>
>>> minimum:
>>> maximum:
>>> multipleOf: 100
>>>
>>> Same for other cases.
>>
>> The "multipleOf: 100" doesn't seen appropriate to me when the choice is
>> 350, 450, 550, 650. Those are not multiples of 100. It's more of a step
>> size. I didn't find a general property for step size. Listing them as
>> "enum" would be another possibility, I guess, but not an elegant one.
>> Especially for property "richtek,const-microvolt" there are 30
>> possibilities.
>
> Ahh, right. You can use enum here and min/max for other cases, where
> multipleOf cannot be used.
>
>>>> + richtek,eoc-microamp:
>>>> + description:
>>>> + This property is end of charge current. Its level ranges from 150 mA to
>>>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>>>> + in 100 mA steps.
>>>> + maxItems: 1
>>
>> Here are two different step sizes. The first few are 50 mA steps (150,
>> 200, 250, 300 mA) and then it changes to 100 mA steps (300, 400, 500,
>> 600 mA). How to deal with that? Again I guess "enum" would be a
>> possibility, but again not a nice one.
>
> enum
Thanks for the reply. Looking through the properties, I get the
following result.
richtek,pre-microamp:
description:
Current of pre-charge mode. The pre-charge current levels are
350 mA to 650 mA programmed by I2C per 100 mA.
maxItems: 1
enum: [350000, 450000, 550000, 650000]
richtek,fast-microamp:
description:
Current of fast-charge mode. The fast-charge current levels are
700 mA to 2000 mA programmed by I2C per 100 mA.
maxItems: 1
minimum: 700000
maximum: 2000000
multipleOf: 100000
richtek,eoc-microamp:
description:
This property is end of charge current. Its level ranges from
150 mA to 600 mA. Between 150 mA and 300 mA in 50 mA steps,
between 300 mA and 600 mA in 100 mA steps.
maxItems: 1
enum: [150000, 200000, 250000, 300000, 400000, 500000, 600000]
richtek,pre-threshold-microvolt:
description:
Voltage of pre-charge mode. If the battery voltage is below the
pre-charge threshold voltage, the charger is in pre-charge mode
with pre-charge current. Its levels are 2.3 V to 3.8 V programmed
by I2C per 0.1 V.
maxItems: 1
minimum: 2300000
maximum: 3800000
multipleOf: 100000
richtek,const-microvolt:
description:
Battery regulation voltage of constant voltage mode. This voltage
levels from 3.65 V to 4.4 V by I2C per 0.025 V.
maxItems: 1
minimum: 3650000
maximum: 4400000
multipleOf: 25000
Kind regards,
Jakob
Hi Jakob,
thanks for your patch!
The following caught my eye:
On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <[email protected]> wrote:
> Add device tree binding documentation for rt5033 multifunction device, voltage
> regulator and battery charger.
>
> Cc: Beomho Seo <[email protected]>
> Cc: Chanwoo Choi <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> The patch is based on linux-next (tag "next-20230413").
(...)
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
(...)
> + richtek,pre-microamp:
> + description:
> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
> + 650 mA programmed by I2C per 100 mA.
> + maxItems: 1
> +
> + richtek,fast-microamp:
> + description:
> + Current of fast-charge mode. The fast-charge current levels are 700 mA
> + to 2000 mA programmed by I2C per 100 mA.
> + maxItems: 1
> +
> + richtek,eoc-microamp:
> + description:
> + This property is end of charge current. Its level ranges from 150 mA to
> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
> + in 100 mA steps.
> + maxItems: 1
> +
> + richtek,pre-threshold-microvolt:
> + description:
> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
> + maxItems: 1
> +
> + richtek,const-microvolt:
> + description:
> + Battery regulation voltage of constant voltage mode. This voltage levels from
> + 3.65 V to 4.4 V by I2C per 0.025 V.
> + maxItems: 1
These are very generic currents and voltages, and their usage is well known
and generic. So they should not be prefixed "richtek,".
Use the properties already defined in
Documentation/devicetree/bindings/power/supply/battery.yaml
for these:
precharge-current-microamp
constant-charge-current-max-microamp
charge-term-current-microamp
precharge-upper-limit-microvolt
constant-charge-voltage-max-microvolt
Please double-check, I think those are the ones you need.
Perhaps it is possible to just $ref these properties directly and add
the additional restrictions on top.
Yours,
Linus Walleij
On Sun, Apr 16, 2023 at 2:45 PM Jakob Hauser <[email protected]> wrote:
> From: Stephan Gerhold <[email protected]>
>
> The fuel gauge in the RT5033 PMIC (rt5033-battery) has its own I2C bus
> and interrupt lines. Therefore, it is not part of the MFD device
> and needs to be specified separately in the device tree.
>
> Cc: Beomho Seo <[email protected]>
> Cc: Chanwoo Choi <[email protected]>
> Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
> Signed-off-by: Stephan Gerhold <[email protected]>
> Acked-by: Lee Jones <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
Looks correct,
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <[email protected]> wrote:
>
> Hi Jakob,
>
> thanks for your patch!
>
> The following caught my eye:
>
> On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <[email protected]> wrote:
>
> > Add device tree binding documentation for rt5033 multifunction device, voltage
> > regulator and battery charger.
> >
> > Cc: Beomho Seo <[email protected]>
> > Cc: Chanwoo Choi <[email protected]>
> > Signed-off-by: Jakob Hauser <[email protected]>
> > ---
> > The patch is based on linux-next (tag "next-20230413").
> (...)
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
> (...)
> > + richtek,pre-microamp:
> > + description:
> > + Current of pre-charge mode. The pre-charge current levels are 350 mA to
> > + 650 mA programmed by I2C per 100 mA.
> > + maxItems: 1
> > +
> > + richtek,fast-microamp:
> > + description:
> > + Current of fast-charge mode. The fast-charge current levels are 700 mA
> > + to 2000 mA programmed by I2C per 100 mA.
> > + maxItems: 1
> > +
> > + richtek,eoc-microamp:
> > + description:
> > + This property is end of charge current. Its level ranges from 150 mA to
> > + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
> > + in 100 mA steps.
> > + maxItems: 1
> > +
> > + richtek,pre-threshold-microvolt:
> > + description:
> > + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
> > + threshold voltage, the charger is in pre-charge mode with pre-charge current.
> > + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
> > + maxItems: 1
> > +
> > + richtek,const-microvolt:
> > + description:
> > + Battery regulation voltage of constant voltage mode. This voltage levels from
> > + 3.65 V to 4.4 V by I2C per 0.025 V.
> > + maxItems: 1
>
> These are very generic currents and voltages, and their usage is well known
> and generic. So they should not be prefixed "richtek,".
>
> Use the properties already defined in
> Documentation/devicetree/bindings/power/supply/battery.yaml
> for these:
>
> precharge-current-microamp
> constant-charge-current-max-microamp
> charge-term-current-microamp
> precharge-upper-limit-microvolt
> constant-charge-voltage-max-microvolt
>
> Please double-check, I think those are the ones you need.
>
> Perhaps it is possible to just $ref these properties directly and add
> the additional restrictions on top.
On second thought, these are really weird properties to have on the
*charger* isn't it?
It is really *battery* restrictions.
A charger can charge many different batteries with different CC/CV
settings.
I think your charger should contain a phandle to a battery and the battery
node should contain these limits.
monitored-battery:
$ref: /schemas/types.yaml#/definitions/phandle
description: phandle to battery node
Then you can just use the standard battery bindings for these properties
on the battery.
See for example:
Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
There will be driver changes needed too, but this will be way cleaner.
Yours,
Linus Walleij
Hi Linus!
On 20.04.23 10:03, Linus Walleij wrote:
> On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <[email protected]> wrote:
>>
>> Hi Jakob,
>>
>> thanks for your patch!
>>
>> The following caught my eye:
>>
>> On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <[email protected]> wrote:
>>
>>> Add device tree binding documentation for rt5033 multifunction device, voltage
>>> regulator and battery charger.
>>>
>>> Cc: Beomho Seo <[email protected]>
>>> Cc: Chanwoo Choi <[email protected]>
>>> Signed-off-by: Jakob Hauser <[email protected]>
>>> ---
>>> The patch is based on linux-next (tag "next-20230413").
>> (...)
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> (...)
>>> + richtek,pre-microamp:
>>> + description:
>>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
>>> + 650 mA programmed by I2C per 100 mA.
>>> + maxItems: 1
>>> +
>>> + richtek,fast-microamp:
>>> + description:
>>> + Current of fast-charge mode. The fast-charge current levels are 700 mA
>>> + to 2000 mA programmed by I2C per 100 mA.
>>> + maxItems: 1
>>> +
>>> + richtek,eoc-microamp:
>>> + description:
>>> + This property is end of charge current. Its level ranges from 150 mA to
>>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>>> + in 100 mA steps.
>>> + maxItems: 1
>>> +
>>> + richtek,pre-threshold-microvolt:
>>> + description:
>>> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
>>> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
>>> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
>>> + maxItems: 1
>>> +
>>> + richtek,const-microvolt:
>>> + description:
>>> + Battery regulation voltage of constant voltage mode. This voltage levels from
>>> + 3.65 V to 4.4 V by I2C per 0.025 V.
>>> + maxItems: 1
>>
>> These are very generic currents and voltages, and their usage is well known
>> and generic. So they should not be prefixed "richtek,".
>>
>> Use the properties already defined in
>> Documentation/devicetree/bindings/power/supply/battery.yaml
>> for these:
>>
>> precharge-current-microamp
>> constant-charge-current-max-microamp
>> charge-term-current-microamp
>> precharge-upper-limit-microvolt
>> constant-charge-voltage-max-microvolt
>>
>> Please double-check, I think those are the ones you need.
>>
>> Perhaps it is possible to just $ref these properties directly and add
>> the additional restrictions on top.
>
> On second thought, these are really weird properties to have on the
> *charger* isn't it?
>
> It is really *battery* restrictions.
>
> A charger can charge many different batteries with different CC/CV
> settings.
>
> I think your charger should contain a phandle to a battery and the battery
> node should contain these limits.
>
> monitored-battery:
> $ref: /schemas/types.yaml#/definitions/phandle
> description: phandle to battery node
>
> Then you can just use the standard battery bindings for these properties
> on the battery.
>
> See for example:
> Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
>
> There will be driver changes needed too, but this will be way cleaner.
>
> Yours,
> Linus Walleij
These are interesting hints!
I was first a bit confused by the term "battery". I associated that term
with the driver "rt5033-battery". But I think that thought was wrong.
The driver "rt5033-battery" is just the fuel gauge.
Hardware-wise, the mfd (incl. charger) and fuel gauge are on different
I2C lines. Therefore, the different drivers access different registers.
Charger registers:
https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L13-L23
Fuel gauge registers:
https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L215-L243
The things being set or read in those registers kind of determine which
things are done in which driver.
The properties we talk about here are the settings for the charger. They
tell the charger how it should behave. It makes sense to process those
settings within the charger driver. The fuel gauge, on the other hand,
returns information like actual voltage and percentage.
The only thing that seems not placed well is the "status" property like
charging/discharging/not-charging/etc. At RT5033 this information is
within the charger register. Userspace layer "UPower" expects this from
the "battery" device. Patch 8 of this series v2 carries that property
from the charger over to the fuel gauge. Though this is a bit of a
quirk. And it creates dependencies between two drivers which actually
would be independent from each other.
---------------------------------
Back to the topic. When we talk about "battery" here, it seems to me
that it's about the representation in the devicetree.
Currently it is:
pmic@34 {
compatible = "richtek,rt5033";
....
charger {
compatible = "richtek,rt5033-charger";
richtek,pre-microamp = <450000>;
richtek,fast-microamp = <1000000>;
richtek,eoc-microamp = <150000>;
richtek,pre-threshold-microvolt = <3500000>;
richtek,const-microvolt = <4350000>;
extcon = <&muic>;
};
};
According to your remarks, the properties could be "outsourced" into a
battery node. (Btw. I have double-checked the property names.)
battery: battery {
compatible = "simple-battery";
precharge-current-microamp = <450000>;
constant-charge-current-max-microamp = <1000000>;
charge-term-current-microamp = <150000>;
precharge-upper-limit-microvolt = <3500000>;
constant-charge-voltage-max-microvolt = <4350000>;
};
pmic@34 {
compatible = "richtek,rt5033";
....
charger {
compatible = "richtek,rt5033-charger";
monitored-battery = <&battery>;
extcon = <&muic>;
};
};
Personally I would choose the current implementation for two reasons
(possibly weak ones):
1) The original author of the driver and documentation is Beomho Seo. I
tried to preserve the original structure as far as possible. This is
probably rather a question of editing than a technical one.
2) At least in my mind it's still the setup for the charger. It sets up
a the charging behavior of a certain consumer device. And the choice of
their values is limited to the hardware of the charger. Accordingly the
dt-bindings would say what the charger hardware is capable to do.
Therefore I'd say it's reasonable to have those values in the charger
node and use vendor properties.
I agree to you that actually the physical battery is determining how
these values should be set. In the end, as far as I can see, it is a
representation thing in the devicetree. At least in our case here.
Not sure how to proceed here. I would stick to the current
implementation. If someone strongly prefers the "battery" representation
style, I'm open to switch to this.
However, I'm not sure how the dt-bindings would look like in that case.
Those battery properties would not be part of the RT5033 node, thus they
basically would not be part of the RT5033 documentation. Again I think
it makes sense to handle those properties within the charger node as
"charger settings" properties.
Kind regards,
Jakob
Hi Jakob,
On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <[email protected]> wrote:
> > On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <[email protected]> wrote:
> > On second thought, these are really weird properties to have on the
> > *charger* isn't it?
> >
> > It is really *battery* restrictions.
> >
> > A charger can charge many different batteries with different CC/CV
> > settings.
(...)
> I was first a bit confused by the term "battery". I associated that term
> with the driver "rt5033-battery". But I think that thought was wrong.
> The driver "rt5033-battery" is just the fuel gauge.
Yeah that is a different thing altogether, it should be named
rt5033-fuel-gauge if I could turn back time, I would review it
and say this, perhaps the confusion can be fixed. Mistakes
were made.
> The properties we talk about here are the settings for the charger. They
> tell the charger how it should behave. It makes sense to process those
> settings within the charger driver.
It may make sense to *parse* this in the charger driver, by
following the monitored-battery phandle to inspect the battery
properties and get the information you need.
The architecture of any Linux driver does not really concern
the DT bindings, the bindings should reflect the hardware.
The hardware has a charger, and the charger is monitoring
a battery, so it needs to be its own DT node.
> The fuel gauge, on the other hand,
> returns information like actual voltage and percentage.
The fuel gauge should probably have a phandle to the same
battery for compleness sake, but may not need it. If it ever needs
any battery properties, it should definately have that.
> According to your remarks, the properties could be "outsourced" into a
> battery node. (Btw. I have double-checked the property names.)
>
> battery: battery {
> compatible = "simple-battery";
> precharge-current-microamp = <450000>;
> constant-charge-current-max-microamp = <1000000>;
> charge-term-current-microamp = <150000>;
> precharge-upper-limit-microvolt = <3500000>;
> constant-charge-voltage-max-microvolt = <4350000>;
> };
>
> pmic@34 {
> compatible = "richtek,rt5033";
> ....
> charger {
> compatible = "richtek,rt5033-charger";
> monitored-battery = <&battery>;
> extcon = <&muic>;
> };
> };
Yups this is how it should look :)
> Personally I would choose the current implementation for two reasons
> (possibly weak ones):
The device tree binding isn't any "implementation", and make sure
to not go into the trap that DT bindings should be done to be
convenient for any specific Linux driver to use.
> 2) At least in my mind it's still the setup for the charger. It sets up
> a the charging behavior of a certain consumer device. And the choice of
> their values is limited to the hardware of the charger. Accordingly the
> dt-bindings would say what the charger hardware is capable to do.
> Therefore I'd say it's reasonable to have those values in the charger
> node and use vendor properties.
>
> I agree to you that actually the physical battery is determining how
> these values should be set. In the end, as far as I can see, it is a
> representation thing in the devicetree. At least in our case here.
The DT bindings should reflect the hardware, and not what some
or any driver "would like to see" (to make its life easier...)
As these things are programmed into registers, clearly the
hardware is adoptable for different batteries, and the purpose
of these registers is to support different batteries. Ergo: they
belong in a battery node.
> Not sure how to proceed here. I would stick to the current
> implementation. If someone strongly prefers the "battery" representation
> style, I'm open to switch to this.
Again this is not an implementation but a hardware description.
It should use a phandle to a montored-battery and follow that to
read the battery properties.
> However, I'm not sure how the dt-bindings would look like in that case.
Just like you sketched above, just reuse simple-battery if the battery
is hardcoded into the platform, such as soldered in or has a form
factor such that no different battery can be fitted.
> Those battery properties would not be part of the RT5033 node, thus they
> basically would not be part of the RT5033 documentation. Again I think
> it makes sense to handle those properties within the charger node as
> "charger settings" properties.
Why?
This is like saying that the number of pixels on your monitor should
be part of the graphics card DT node as "configuration". And we
clearly do not do that.
Yours,
Linus Walleij
Hi Linus,
On 21.04.23 11:20, Linus Walleij wrote:
> On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <[email protected]> wrote:
...
>> I agree to you that actually the physical battery is determining how
>> these values should be set. In the end, as far as I can see, it is a
>> representation thing in the devicetree. At least in our case here.
>
> The DT bindings should reflect the hardware, and not what some
> or any driver "would like to see" (to make its life easier...)
>
> As these things are programmed into registers, clearly the
> hardware is adoptable for different batteries, and the purpose
> of these registers is to support different batteries. Ergo: they
> belong in a battery node.
>
>> Not sure how to proceed here. I would stick to the current
>> implementation. If someone strongly prefers the "battery" representation
>> style, I'm open to switch to this.
>
> Again this is not an implementation but a hardware description.
>
> It should use a phandle to a montored-battery and follow that to
> read the battery properties.
OK, this is expressing a strong preference. I’ll switch to the "battery"
node in v3.
>> However, I'm not sure how the dt-bindings would look like in that case.
>
> Just like you sketched above, just reuse simple-battery if the battery
> is hardcoded into the platform, such as soldered in or has a form
> factor such that no different battery can be fitted.
>
>> Those battery properties would not be part of the RT5033 node, thus they
>> basically would not be part of the RT5033 documentation. Again I think
>> it makes sense to handle those properties within the charger node as
>> "charger settings" properties.
>
> Why?
>
> This is like saying that the number of pixels on your monitor should
> be part of the graphics card DT node as "configuration". And we
> clearly do not do that.
The charger driver needs five parameters. And the person writing the dts
file should know about what range and granularity is possible for those
values. This information could be put into the description of the
"monitored-battery", as shown below, but that's less strict and clear as
it is currently in patch 9.
title: Richtek RT5033 PIMC Battery Charger
maintainers:
- Jakob Hauser <[email protected]>
description:
The battery charger of the multifunction device RT5033 has to be
instantiated under sub-node named "charger" using the following
format.
properties:
compatible:
const: richtek,rt5033-charger
monitored-battery:
description: |
Phandle to the monitored battery according to battery.yaml.
The battery node needs to contain five parameters.
precharge-current-microamp:
Current of pre-charge mode. The pre-charge current levels are
350 mA to 650 mA programmed by I2C per 100 mA.
constant-charge-current-max-microamp:
Current of fast-charge mode. The fast-charge current levels
are 700 mA to 2000 mA programmed by I2C per 100 mA.
charge-term-current-microamp:
This property is end of charge current. Its level ranges from
150 mA to 600 mA. Between 150 mA and 300 mA in 50 mA steps,
between 300 mA and 600 mA in 100 mA steps.
precharge-upper-limit-microvolt:
Voltage of pre-charge mode. If the battery voltage is below
the pre-charge threshold voltage, the charger is in pre-charge
mode with pre-charge current. Its levels are 2.3 V to 3.8 V
programmed by I2C per 0.1 V.
constant-charge-voltage-max-microvolt:
Battery regulation voltage of constant voltage mode. This
voltage levels from 3.65 V to 4.4 V by I2C per 0.025 V.
extcon:
description:
Phandle to the extcon device.
maxItems: 1
required:
- monitored-battery
additionalProperties: false
examples:
- |
charger {
compatible = "richtek,rt5033-charger";
monitored-battery = <&battery>;
extcon = <&muic>;
};
Kind regards,
Jakob
Hi Jakob,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on next-20230421]
[cannot apply to sre-power-supply/for-next broonie-regulator/for-next linus/master lee-mfd/for-mfd-fixes v6.3-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link: https://lore.kernel.org/r/665d8906ea7b84e0a248315e8395a80007b8bafb.1681646904.git.jahau%40rocketmail.com
patch subject: [PATCH v2 6/9] power: supply: rt5033_charger: Add RT5033 charger device driver
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0fbd385f9a1acd55a8d943560428b9d783f8047f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
git checkout 0fbd385f9a1acd55a8d943560428b9d783f8047f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/xen/ drivers/power/supply/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/power/supply/rt5033_charger.c:26:10: warning: variable 'state' is uninitialized when used here [-Wuninitialized]
return state;
^~~~~
drivers/power/supply/rt5033_charger.c:23:11: note: initialize the variable 'state' to silence this warning
int state;
^
= 0
1 warning generated.
vim +/state +26 drivers/power/supply/rt5033_charger.c
18
19 static int rt5033_get_charger_state(struct rt5033_charger *charger)
20 {
21 struct regmap *regmap = charger->rt5033->regmap;
22 unsigned int reg_data;
23 int state;
24
25 if (!regmap)
> 26 return state;
27
28 regmap_read(regmap, RT5033_REG_CHG_STAT, ®_data);
29
30 switch (reg_data & RT5033_CHG_STAT_MASK) {
31 case RT5033_CHG_STAT_DISCHARGING:
32 state = POWER_SUPPLY_STATUS_DISCHARGING;
33 break;
34 case RT5033_CHG_STAT_CHARGING:
35 state = POWER_SUPPLY_STATUS_CHARGING;
36 break;
37 case RT5033_CHG_STAT_FULL:
38 state = POWER_SUPPLY_STATUS_FULL;
39 break;
40 case RT5033_CHG_STAT_NOT_CHARGING:
41 state = POWER_SUPPLY_STATUS_NOT_CHARGING;
42 break;
43 default:
44 state = POWER_SUPPLY_STATUS_UNKNOWN;
45 }
46
47 return state;
48 }
49
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Sebastian,
I noticed a small mistake in patch 8.
On 16.04.23 14:44, Jakob Hauser wrote:
> The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> can, let's get this value.
>
> Tested-by: Raymond Hackley <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> index 5c04cf305219..48d4cccce4f6 100644
> --- a/drivers/power/supply/rt5033_battery.c
> +++ b/drivers/power/supply/rt5033_battery.c
> @@ -12,6 +12,26 @@
> #include <linux/mfd/rt5033-private.h>
> #include <linux/mfd/rt5033.h>
>
> +static int rt5033_battery_get_status(struct i2c_client *client)
> +{
> + struct power_supply *charger;
> + union power_supply_propval val;
> + int ret;
> +
> + charger = power_supply_get_by_name("rt5033-charger");
> + if (!charger)
> + return -ENODEV;
> +
> + ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> + if (ret) {
> + power_supply_put(charger);
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> + }
> +
> + power_supply_put(charger);
> + return val.intval;
> +}
> +
If the rt5033-charger driver is not available, this function returns
"-ENODEV". Instead of an error, in fact the status node in sysfs just
reports "-19" then. Userspace layer UPower makes status "unknown" out of
this.
An error message would spam dmesg anyway, as it would be issued every
time the battery gets polled by UPower, which is quite regularly. The
scenario of a missing rt5033-charger driver is not unlikely for devices
where it's not yet implemented in the devicetree or in the configs of
the compiled kernel. For the displayed battery icon, UPower assumes
"discharging" for a single battery in "unknown" state.
It makes more sense to return "POWER_SUPPLY_STATUS_UNKNOWN" right away.
I'll change that line in v3.
> static int rt5033_battery_get_capacity(struct i2c_client *client)
> {
> struct rt5033_battery *battery = i2c_get_clientdata(client);
> @@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_CAPACITY:
> val->intval = rt5033_battery_get_capacity(battery->client);
> break;
> + case POWER_SUPPLY_PROP_STATUS:
> + val->intval = rt5033_battery_get_status(battery->client);
> + break;
> default:
> return -EINVAL;
> }
> @@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
> POWER_SUPPLY_PROP_VOLTAGE_OCV,
> POWER_SUPPLY_PROP_PRESENT,
> POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_STATUS,
> };
>
> static const struct regmap_config rt5033_battery_regmap_config = {
Kind regards,
Jakob
Hi Sebastian,
On 23.04.23 03:22, kernel test robot wrote:
> Hi Jakob,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on lee-mfd/for-mfd-next]
> [also build test WARNING on next-20230421]
> [cannot apply to sre-power-supply/for-next broonie-regulator/for-next linus/master lee-mfd/for-mfd-fixes v6.3-rc7]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
> base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
> patch link: https://lore.kernel.org/r/665d8906ea7b84e0a248315e8395a80007b8bafb.1681646904.git.jahau%40rocketmail.com
> patch subject: [PATCH v2 6/9] power: supply: rt5033_charger: Add RT5033 charger device driver
> config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/0fbd385f9a1acd55a8d943560428b9d783f8047f
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
> git checkout 0fbd385f9a1acd55a8d943560428b9d783f8047f
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/xen/ drivers/power/supply/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
>>> drivers/power/supply/rt5033_charger.c:26:10: warning: variable 'state' is uninitialized when used here [-Wuninitialized]
> return state;
> ^~~~~
> drivers/power/supply/rt5033_charger.c:23:11: note: initialize the variable 'state' to silence this warning
> int state;
> ^
> = 0
> 1 warning generated.
>
>
> vim +/state +26 drivers/power/supply/rt5033_charger.c
>
> 18
> 19 static int rt5033_get_charger_state(struct rt5033_charger *charger)
> 20 {
> 21 struct regmap *regmap = charger->rt5033->regmap;
> 22 unsigned int reg_data;
> 23 int state;
> 24
> 25 if (!regmap)
> > 26 return state;
> 27
> 28 regmap_read(regmap, RT5033_REG_CHG_STAT, ®_data);
> 29
> 30 switch (reg_data & RT5033_CHG_STAT_MASK) {
> 31 case RT5033_CHG_STAT_DISCHARGING:
> 32 state = POWER_SUPPLY_STATUS_DISCHARGING;
> 33 break;
> 34 case RT5033_CHG_STAT_CHARGING:
> 35 state = POWER_SUPPLY_STATUS_CHARGING;
> 36 break;
> 37 case RT5033_CHG_STAT_FULL:
> 38 state = POWER_SUPPLY_STATUS_FULL;
> 39 break;
> 40 case RT5033_CHG_STAT_NOT_CHARGING:
> 41 state = POWER_SUPPLY_STATUS_NOT_CHARGING;
> 42 break;
> 43 default:
> 44 state = POWER_SUPPLY_STATUS_UNKNOWN;
> 45 }
> 46
> 47 return state;
> 48 }
> 49
>
Here as well I'll change "return state;" into "return
POWER_SUPPLY_STATUS_UNKNOWN;". If the driver fails to get the regmap,
the status is "unknown".
Kind regards,
Jakob