[PATCH 00/10] Add RT5033 charger device driver

[PATCH 00/10] Add RT5033 charger device driver

[Jakob Hauser]

Works on rt5033-charger has already been quite far but phased out. The last
patchset version I could find was of March 2015 [1].

Let's pick this up again.

Those patches of March 2015 are now patch 6 and patch 10. On those two
patches I actually would prefer to use From: and Co-developed-by: tags. I
contacted Beomho Seo beforehand but didn't receive an answer. Therefore I
applied Cc: tag.

Looking through the old patchset, there were quite several things I changed.
A detailed list of changes on those patches 6 and 10 can be found further down,
going from top to bottom through the patches.

Some comments on the end-of-charge behavior. The rt5033 chip offers three
options. In the Android driver, a forth option was implemented.
- By default, the rt5033 chip charges indefinitely. The current goes down but
  there is always a charge voltage to the battery, which might not be too good
  for the battery lifetime.
- There is the possibility to enable a fast charge timer. The timer can be
  set to 4, 6, 8... 16 hours. After that time has elapsed, charging stops
  and the battery gets discharged. This option with a timer of 4 hours was
  chosen by Beomho Seo in the patchset of March 2015. However, that option
  is confusing to the user. It doesn't initiate a re-charge cycle. So when
  keeping plugged in the device over night, I find it discharging on the
  next morning.
- The third option of the rt5033 chip is enabling charging termination. This
  also enables a re-charge cycle. When the charging current sinks below the
  end-of-charge current, the chip stops charging. The sysfs state changes to
  "not charging". When the voltage gets 0.1 V below the end-of-charge constant
  voltage, re-charging starts. Then again, when charging current sinks below
  the end-of-charge current, the chip stops charging. And so on, going up and
  down in re-charge cycles. In case the power consumption is high (e.g. tuning
  on the display of the mobile device), the current goes into an equilibrium.
  The downside of this charging termination option: When reaching the end-of-
  charge current, the capacity might not have reached 100 % yet. The capacity
  to reach probably depends on power consumption and battery wear. On my mobile
  device, capacity reaches 98 %, drops to 96 % until re-charging kicks in,
  climbs to 98 %, drops to 96 %, and so on. Not reaching 100 % is a bit
  confusing to the user, too.
- In the Android driver, both timer and termination are turned off. Instead,
  a self-written re-charge logic is implemented in the driver infrastructure.
  On mobile device samsung-serranovelte, after passing the end-of-charge IRQ
  trigger, it keeps on charging for approx. 42 mins and then stops. When the
  voltage drops below 4.3 V, it starts charging again. 42 min later it stops
  again. When below 4.3 V starts again, etc. This way, the capacity reaches
  well 100 % and doesn't drop below. This behavior is not managed in
  drivers/battery/rt5033_charger.c [2] but by the Samsung battery
  infrastructure drivers/battery/sec_battery.c [3]. Some of the settings for
  the re-charge behavior are set in arch/arm/boot/dts/samsung/msm8916/
  msm8916-sec-serranovelte-battery-r01.dtsi [4] (for samsung-serranovelte
  mobile device).

The forth option would be the best. But it would require a lot of additional
coding and testing for the driver. For the rt5033-charger driver submited here,
option 3 "charging termination" was selected. It possibly doesn't reach 100 %
capacity, which is confusing to the user, but at least it offers a re-charge
cycle without extra effort in the driver.

Patch 2 fixes the bits to be read out to get the chip revision. While testing,
I noticed a nasty "hardware" bug in the chip revision read-out. I have two
devices samsung-serranove. Both have rt5033 chip revision 6 (register 0x03
value 0x86, the last four bits are the revision). However, when I remove the
battery, wait a bit, put the battery back in and boot, then one of the devices
show chip revision 1 (register 0x03 value 0x81). It stays that way even when
powering off and booting again. Once I put in the charging cable for the first
time, register 0x03 changes to the correct value 0x86 and stays there, even
when rebooting. This happens only on one of the two devices. Interestingly,
in the Android driver there seems to be a quirk to handle this issue [5]. In
register 0x6b (RT5033_REG_OFF_EVENT) they set bit 0x01, wait 100 microseconds,
read the chip revision, then unset bit 0x01. I was thinking about adding this
quirk to the patchset. But I decided not to do so (at least for the time being)
because I don't know what's register 0x6b and what exactly does bit 0x01. At
another location [6] it says this bit enables OSC, possibly OSC stand for
oscillator, which I think is used for the internal clock. I don't know if there
might be some side effects when applying this quirk. So I prefer to do nothing
about this. Having a wrong chip revision in dmesg until the first charge isn't
a severe issue. The quirk might be needed at a later date when other quirks
(see next paragraph) need to be added conditionally on the chip revision.

There are more of such quirks in the Android driver, e.g. [7][8][9]. I haven't
noticed any bugs except the chip revision bug described above. I didn't add
these quirks because I'm not fully sure what they do and if it's really needed.
However, there is a possibility that some devices run into issues. Still I'd
avoid adding all kind of quirks without knowing anything about it. The
rt5033-charger driver sets the bits for voltages, currents and end-of-charge
behavior. So from a safety point of view the most important boundaries should
be set.

Additionally something that's missing compared to the Android driver is the IRQ
implementation and infrastructure in rt5033-charger [10][11][12].

The rt5033-charger driver returs a dmesg warning "DMA mask not set". I've read
that it would be related to platform_set_drvdata() in the probe function. But I
couldn't spot anything wrong there. It could also be related to the
devm_kzalloc() of rt5033_charger_data *chg in rt5033_charger_dt_init().
I couldn't solve it. As it seems to have no effect, I didn't do anything more
about it.

The patchset is organized as follows:
- Patches 1-5: Fixes and preparatory changes on rt5033 mfd
- Patches 6-7: Add and extend rt5033-charger
- Patch 9: Add status property to rt5033-battery
- Patch 10: Add documentation

The first patch is a lost one from a previous series [13].

The patches depend on each other, it would be good to apply the patchset as a
whole. Not sure if this patchset is organized well enough in terms of touching
several subsystems. Let me know if it should be arranged or handled in a
different way.

The patchset is based on torvalds/linux v6.2-rc5. The result of the patchset
can be seen on my GitHub fork [14].

Changes to the version of March 2015:

Patch 6

Commit message
- Corrected typos "adds", "supports", "provides", "modes", "The", "pre-charge",
  "fast charge", "They vary in charge rate, the charge parameters...".

Makefile
- Changed "CONFIG_POWER_RT5033" to "CONFIG_CHARGER_RT5033", as noted by
  Sebastian.
- Placed CHARGER_RT5033 directly below BATTERY_RT5033, like in the Kconfig
  file.

Generally on rt5033_charger.c
- Added SPDX-License-Identifier tag to line 1.
- At the top of rt5033_charger.c, before "Free Software Foundation", added a
  space between "by the", as mentioned by Paul.
- In function rt5033_init_const_charge(), rt5033_init_fast_charge(),
  rt5033_init_pre_charge() and rt5033_charger_reg_init(), changed the pointer
  of "struct rt5033_charger" from *psy to *charger. Firstly to avoid confusion
  with "psy" within "struct rt5033_charger" [15]. Secondly to stay more
  consistant to other functions like rt5033_charger_probe() or
  rt5033_get_charger_state() where pointer *charger is used.
- At the end, added rt5033_charger_of_match[], MODULE_DEVICE_TABLE(of, ...)
  and .of_match_table to probe the driver by device-tree.
- At the end, changed MODULE_LICENSE to "GPL v2", as noted by Sebastian and
  Paul.

Function rt5033_get_charger_state()
- At declaration changed the order of "reg_data" and "state".
- Moved "state = POWER_SUPPLY_STATUS_UNKNOWN" from declaration area to
  switch "default:".
- Data type for "reg_data" doesn't need to be "u32". Changed it to
  "unsigned int". In the Android driver it's "int" [16].
- The RT5033_CHG_STAT_MASK needs to be 0x30 to cover the charge state options
  0x00, 0x10, 0x20, 0x30 [17]. In the Android driver it's 0x30 as well [18].
  Thus, changing that value in include/linux/mfd/rt5033-private.h is needed.
  Interestingly it overlaps with RT5033_CHG_STAT_TYPE_MASK which is 0x60.
  The STAT_TYPE is actually just one bit at 0x40. However, using 0x60 to
  overlap with the STAT mask 0x30 allows to detect more states. If the
  STAT is "charging" or "not charging" (failure), BIT(5) is 1 and the
  STAT_TYPE can be "fast" or "trickle". On the other hand, if STAT is
  "discharging" or "full", BIT(5) is 0 and that way STAT_TYPE can be set to
  "none" or "unknown".

Function rt5033_get_charger_type()
- At declaration changed the order of "reg_data" and "state".
- Again changed data type for "reg_data" from "u32" to "unsigned int". In
  the Android driver it's "int" [19].
- Moved "state =" from declaration area to switch "default:".
- Changed "state =" from UNKNOWN to NONE. This way the charger type is "none"
  when no cable is attached.

Function rt5033_get_charger_current_limit()
- Renamed function rt5033_get_charger_current() to
  rt5033_get_charger_current_limit(). It doesn't return a measured value, it
  returns the current limit that was set.
- Removed the psp check and psp parameter, as suggested by Sebastian.
- Replaced "state" calculation "(reg_data >> RT5033_CHGCTRL5_ICHG_SHIFT) & 0x0f"
  by "(reg_data & RT5033_CHGCTRL5_ICHG_MASK) >> RT5033_CHGCTRL5_ICHG_SHIFT".
  The first is a shift by >> 4 and mask 00001111. The second is a mask 11110000
  and a shift by >> 4. However, this way it's better represented by the values
  defined in include/linux/battery/charger/rt5033_charger.h.
- Removed the limitation to RT5033_CHG_MAX_CURRENT. This function is reading
  the current limit. If the current limit is higher than the defined max for
  whatever reason, this should be visible in sysfs.
- Replaced "data" calculation "state * 100 + 700" by a calculation using values
  RT5033_CHARGER_FAST_CURRENT_MIN and RT5033_CHARGER_FAST_CURRENT_STEP_NUM.

Function rt5033_get_charger_const_voltage()
- Renamed function rt5033_get_charge_voltage() to
  rt5033_get_charger_const_voltage(). It doesn't measure the charge voltage,
  it returns the const voltage that was set.
- Removed the psp check and psp parameter as suggested by Sebastian.
- Replaced "data" calculation "reg_data >> 2" by
  "(reg_data & RT5033_CHGCTRL2_CV_MASK) >> RT5033_CHGCTRL2_CV_SHIFT;". This
  is cleaner. However, the value RT5033_CHGCTRL2_CV_SHIFT needs to be added
  to include/linux/mfd/rt5033-private.h.
- Removed the limitation to RT5033_CHARGER_CONST_VOLTAGE_LIMIT_MAX. If the
  const voltage is set higher than the defined max for whatever reason, this
  should be visible in sysfs.

Function rt5033_init_const_charge()
- Added missing "int" to declaration "unsigned int val". Also adapted the
  order of declarations to be similar to functions rt5033_init_fast_charge()
  and rt5033_init_pre_charge().
- In remark "Set Constant voltage mode" wrote "constant" in lower case.
- Changed hex value 0x0 to two digits 0x00.
- In section "Set Constant voltage mode", the "reg_data" is wrongly set to
  0xfc (decimal 252). That's the value of the const voltage mask 11111100,
  which is RT5033_CHGCTRL2_CV_MASK. However, from 3,65 V to 4,4 V in
  0,025 steps [20] are 30 steps. Thus, the max value should be 0x1e (decimal
  30). Let's use a new define RT5033_CV_MAX_VOLTAGE here that contains that
  value. Needs to be added to include/linux/mfd/rt5033-private.h.
- In section "Set Constant voltage mode" at regmap_update_bits(), replaced
  shift value 2 by RT5033_CHGCTRL2_CV_SHIFT.
- In section "Set end of charge current" changed hex values 0x1 and 0x7 to
  two digits 0x01 and 0x07.

Function rt5033_init_fast_charge()
- Changed AICR mask name from RT5033_AICR_MODE_MASK to
  RT5033_CHGCTRL1_IAICR_MASK.
- Removed the block "Set internal timer". The fast charge timer stops charging
  when the time has elapsed (TIMER4 is 4 hours). There is no re-charging. Thus,
  this behavior is confusing because the device keeps discharging after the
  timer has elapsed.
- In remark "Set fast-charge mode Carging current" fixed the word "Carging"
  to "charging".
- In section "Set fast-charge mode charging current" changed hex value 0x0
  to two digits 0x00.
- Moved declaration "unsigned int val" to the beginning of the function.
- Replaced max value 0xd0 (decimal 208) by RT5033_CHG_MAX_CURRENT (which is
  0x0d, decimal 13). From 700 mA to 2000 mA in 100 mA steps [21] are 13 steps.
  In the Android driver the value is written as 0xd [22], which is decimal 13.
- Calculation of "reg_data" as "0x10 + val" is unneccesary. 0x10 adds BIT(4)
  but "val" needs to be shifted by 4 bits later on, thus the bit added here
  gets lost and is therefore not needed.
- In section "Set fast-charge mode charging current", on regmap_update_bits()
  the "reg_data" value has to be shifted by RT5033_CHGCTRL5_ICHG_SHIFT
  (shift << 4) to meet RT5033_CHGCTRL5_ICHG_MASK (mask 11110000).

Function rt5033_init_pre_charge()
- Moved declaration "unsigned int val" to the beginning of the function.
- In section "Set pre-charge threshold voltage" changed "reg_data"
  calculation from "0x00 + val" to simply "val", the 0x00 isn't needed.
- In section "Set pre-charge mode charging current", the min/max values are
  350 mA and 650 mA according to include/linux/mfd/rt5033-private.h. And the
  step size is 100 mA. These are 4 steps (350, 450, 550, 650 mA). The max
  "reg_data" value is therefore 0x03. The value 0x18, on the contrary, is
  the mask where that value needs to written into (mask 00011000). Therefore
  add a new define RT5033_CHG_MAX_PRE_CURRENT to rt5033-private.h and use
  this value for the "reg_data" max value.
- In section "Set pre-charge mode charging current", when writing the
  "reg_data" to the register, it needs a shift by 3 bit to fit the
  RT5033_CHGCTRL4_IPREC_MASK, which is mask 0x18 (00011000). Thus, replace
  the "reg_data" calculation "0x08 + val" by simply "val", add a new define
  RT5033_CHGCTRL4_IPREC_SHIFT to include/linux/mfd/rt5033-private.h and apply
  this on regmap_update_bits().

Function rt5033_charger_reg_init()
- Removed the regmap_update_bits() of RT5033_CHARGER_MODE,
  RT5033_CHARGER_UUG_ENABLE, RT5033_CFO_ENABLE and
  RT5033_CHARGER_HZ_DISABLE. They set the same values that are already the
  default settings of the chip. Therefore it's not neccessary to set them.
- Added new block "Enable charging termination". It stops charging when
  reaching the end-of-charge current and enters a re-charging cycle. The
  re-charging starts when the voltage gets 0.1 V below the constant voltage.
- Set minimum input voltage regulation (MIVR) to DISABLED. This increases
  charging speed when using weak cables.

Array rt5033_charger_props[]
- Removed property POWER_SUPPLY_PROP_CURRENT_NOW, see further below.
- Removed property POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, see below.
- Added property POWER_SUPPLY_PROP_ONLINE. Userspace layer UPower expects
  property "online" for a "line-power" device.

Function rt5033_charger_get_property()
- At declaration "struct rt5033_charger *charger =", replaced container_of()
  by power_supply_get_drvdata().
- Removed property POWER_SUPPLY_PROP_CURRENT_NOW. The regmap doesn't offer
  measured values of the charge current.
- Removed property POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX. The const
  voltage isn't configurable in userspace, therefore it's not relevant to
  the user.
- Assigned function rt5033_get_charger_current_limit() to property
  POWER_SUPPLY_PROP_CURRENT_MAX. It represents the current limit that was set.
- Assigned function rt5033_get_charger_const_voltage() to the property
  POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE. It represents the const voltage
  that was set.
- Added POWER_SUPPLY_PROP_ONLINE. For the time being, "online" is true when
  the state is "charging". This will be optimized when implementing extcon
  cable detection.

Function rt5033_charger_dt_init()
- Removed the remark "Charging current is decided by ...". It's true that the
  fast charging speed is not solely the value imported from device-tree value
  "richtek,fast-uamp" but instead gets chip-interenally managed by additional
  factors. However, at the one hand that's not the ideal location to explain
  this. Secondly, the external sensing register value of 10 mili ohm doesn't
  get changed by the driver. I would just skip this comment.

Struct rt5033_charger_desc
- Added a power_supply_desc struct. Using this in probe for
  devm_power_supply_register().
- Replaced type POWER_SUPPLY_TYPE_MAINS by POWER_SUPPLY_TYPE_USB.

Function rt5033_charger_probe()
- Replaced power_supply_register() by devm_power_supply_register(), as refered
  by Sebastian. Accordingly, removed rt5033_charger_remove() and ".remove"
  further down. Implemented "psy_cfg" in rt5033_charger_probe(). In
  include/linux/mfd/rt5033.h, turned power_supply "psy" into a pointer.

Patch 10
- Changed the documentation to yaml format.
- Corrected lower-case/upper-case on some words ("Power Management ...",
  "multifunction device").
- At the description, added that the battery fuel gauge uses a separate I2C
  bus.
- Split out the regulator and charger documentation into a separate documents.
- In the example of the mfd, indicated that the battery fuel gauge is set to
  another I2C bus.
- In the charger yaml in the description of "richtek,eoc-uamp" fixed the upper
  level from 200 mA to 600 mA [23]. Also added description of the step sizes.
- In the charger yaml, added property "extcon" for phandle.
- In the charger yaml, generally minor wording fixes on the descriptions.
- Choose more careful charger values for the example.
- In the regulator yaml added the voltage ranges to the description.
- Skipped the change on Documentation/devicetree/bindings/vendor-prefixes.txt,
  Richtek is already implemented in the vendors list by now.

References:
[1] https://lore.kernel.org/lkml/1425864191-4121-1-git-send-email-beomho.seo@samsung.com/T/#t
[2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c
[3] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/sec_battery.c
[4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi
[5] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L482-L486
[6] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L364-L365
[7] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L146-L158
[8] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L350-L390
[9] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L622-L627
[10] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L995-L1250
[11] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/include/linux/battery/charger/rt5033_charger.h#L52-L58
[12] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_irq.c
[13] https://lore.kernel.org/linux-pm/YMeILEnjOCCzo61q@gerhold.net
[14] https://github.com/Jakko3/linux/blob/rt5033-charger_v1/drivers/power/supply/rt5033_charger.c
[15] https://github.com/torvalds/linux/blob/v6.1-rc1/include/linux/mfd/rt5033.h#L54
[16] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L657
[17] https://github.com/torvalds/linux/blob/v6.1-rc1/include/linux/mfd/rt5033-private.h#L59-L62
[18] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L671
[19] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L705
[20] https://github.com/torvalds/linux/blob/v6.1-rc1/include/linux/mfd/rt5033-private.h#L155-L158
[21] https://github.com/torvalds/linux/blob/v6.1-rc1/include/linux/mfd/rt5033-private.h#L165-L168
[22] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L377-L382
[23] https://github.com/torvalds/linux/blob/v6.0/include/linux/mfd/rt5033-private.h#L178

Jakob Hauser (9):
  mfd: rt5033: Fix chip revision readout
  mfd: rt5033: Fix comments and style in includes
  mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines
  mfd: rt5033: Apply preparatory changes before adding rt5033-charger
    driver
  power: supply: rt5033_charger: Add RT5033 charger device driver
  power: supply: rt5033_charger: Add cable detection and USB OTG supply
  power: supply: rt5033_charger: Make use of high impedance mode
  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          | 102 +++
 .../power/supply/richtek,rt5033-charger.yaml  |  76 ++
 .../regulator/richtek,rt5033-regulator.yaml   |  45 +
 drivers/mfd/rt5033.c                          |  11 +-
 drivers/power/supply/Kconfig                  |   8 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/rt5033_battery.c         |  24 +
 drivers/power/supply/rt5033_charger.c         | 769 ++++++++++++++++++
 include/linux/mfd/rt5033-private.h            |  81 +-
 include/linux/mfd/rt5033.h                    |  15 +-
 10 files changed, 1091 insertions(+), 41 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.1

[PATCH 01/10] mfd: rt5033: Drop rt5033-battery sub-device

[Jakob Hauser]

From: Stephan Gerhold <stephan@gerhold.net>

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 <beomho.seo@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 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.1

[PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Jakob Hauser]

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 <jahau@rocketmail.com>
---
 drivers/mfd/rt5033.c               | 8 +++++---
 include/linux/mfd/rt5033-private.h | 4 ++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
index 8029d444b794..d32467174cb5 100644
--- a/drivers/mfd/rt5033.c
+++ b/drivers/mfd/rt5033.c
@@ -55,7 +55,8 @@ 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 data;
+	unsigned int chip_rev;
 	int ret;
 
 	rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
@@ -73,12 +74,13 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
 		return PTR_ERR(rt5033->regmap);
 	}
 
-	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &dev_id);
+	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &data);
 	if (ret) {
 		dev_err(&i2c->dev, "Device not found\n");
 		return -ENODEV;
 	}
-	dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
+	chip_rev = data & 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 2d1895c3efbf..d18cd4572208 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.1

[PATCH 03/10] mfd: rt5033: Fix comments and style in includes

[Jakob Hauser]

Fix comments and remove some empty lines in rt5033-private.h. Align struct
rt5033_charger in rt5033.h.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 include/linux/mfd/rt5033-private.h | 17 +++++++----------
 include/linux/mfd/rt5033.h         |  7 +++----
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
index d18cd4572208..b035a67cec73 100644
--- a/include/linux/mfd/rt5033-private.h
+++ b/include/linux/mfd/rt5033-private.h
@@ -111,14 +111,13 @@ enum rt5033_reg {
 #define RT5033_LDO_CTRL_MASK			0x1f
 
 /* RT5033 charger property - model, manufacturer */
-
 #define RT5033_CHARGER_MODEL	"RT5033WSC Charger"
 #define RT5033_MANUFACTURER	"Richtek Technology Corporation"
 
 /*
- * RT5033 charger fast-charge current lmits (as in CHGCTRL1 register),
- * AICR mode limits the input current for example,
- * the AIRC 100 mode limits the input current to 100 mA.
+ * While RT5033 charger can limit the fast-charge current (as in CHGCTRL1
+ * register), AICR mode limits the input current. For example, the AIRC 100
+ * mode limits the input current to 100 mA.
  */
 #define RT5033_AICR_100_MODE			0x20
 #define RT5033_AICR_500_MODE			0x40
@@ -143,10 +142,9 @@ enum rt5033_reg {
 #define RT5033_TE_ENABLE_MASK			0x08
 
 /*
- * RT5033 charger opa mode. RT50300 have two opa mode charger mode
- * and boost mode for OTG
+ * RT5033 charger opa mode. RT5033 has two opa modes for OTG: charger mode
+ * and boost mode.
  */
-
 #define RT5033_CHARGER_MODE			0x00
 #define RT5033_BOOST_MODE			0x01
 
@@ -185,18 +183,17 @@ enum rt5033_reg {
  * RT5033 charger pre-charge threshold volt limits
  * (as in CHGCTRL5 register), uV
  */
-
 #define RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MIN	2300000U
 #define RT5033_CHARGER_PRE_THRESHOLD_STEP_NUM	100000U
 #define RT5033_CHARGER_PRE_THRESHOLD_LIMIT_MAX	3800000U
 
 /*
- * RT5033 charger enable UUG, If UUG enable MOS auto control by H/W charger
+ * RT5033 charger UUG. It enables MOS auto control by H/W charger
  * circuit.
  */
 #define RT5033_CHARGER_UUG_ENABLE		0x02
 
-/* RT5033 charger High impedance mode */
+/* RT5033 charger high impedance mode */
 #define RT5033_CHARGER_HZ_DISABLE		0x00
 #define RT5033_CHARGER_HZ_ENABLE		0x01
 
diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h
index 3c23b6220c04..8f306ac15a27 100644
--- a/include/linux/mfd/rt5033.h
+++ b/include/linux/mfd/rt5033.h
@@ -49,10 +49,9 @@ struct rt5033_charger_data {
 };
 
 struct rt5033_charger {
-	struct device		*dev;
-	struct rt5033_dev	*rt5033;
-	struct power_supply	psy;
-
+	struct device			*dev;
+	struct rt5033_dev		*rt5033;
+	struct power_supply		psy;
 	struct rt5033_charger_data	*chg;
 };
 
-- 
2.39.1

[PATCH 04/10] mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines

[Jakob Hauser]

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 <jahau@rocketmail.com>
---
 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.1

[PATCH 05/10] mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver

[Jakob Hauser]

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 <jahau@rocketmail.com>
---
 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.1

[PATCH 06/10] power: supply: rt5033_charger: Add RT5033 charger device driver

[Jakob Hauser]

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 <beomho.seo@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Raymond Hackley <raymondhackley@protonmail.com>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 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 0bbfe6a7ce4d..2719c51b6fca 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -785,6 +785,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 0ee8653e882e..0a3303176503 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_BATTERY_MAX17042)	+= max17042_battery.o
 obj-$(CONFIG_BATTERY_MAX1721X)	+= max1721x_battery.o
 obj-$(CONFIG_BATTERY_Z2)	+= z2_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_BATTERY_S3C_ADC)	+= s3c_adc_battery.o
 obj-$(CONFIG_BATTERY_TWL4030_MADC)	+= twl4030_madc_battery.o
diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
new file mode 100644
index 000000000000..6bb3d45a5e9e
--- /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 <beomho.seo@samsung.com>
+ *
+ * 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, &reg_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, &reg_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, &reg_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, &reg_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-uamp", &chg->pre_uamp);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_property_read_u32(np, "richtek,pre-threshold-uvolt",
+			&chg->pre_uvolt);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_property_read_u32(np, "richtek,fast-uamp", &chg->fast_uamp);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_property_read_u32(np, "richtek,const-uvolt",
+			&chg->const_uvolt);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_property_read_u32(np, "richtek,eoc-uamp", &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 <beomho.seo@samsung.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.39.1

[PATCH 07/10] power: supply: rt5033_charger: Add cable detection and USB OTG supply

[Jakob Hauser]

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 <raymondhackley@protonmail.com>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 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 6bb3d45a5e9e..79e7f75fe634 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.1

[PATCH 08/10] power: supply: rt5033_charger: Make use of high impedance mode

[Jakob Hauser]

Enable high impedance mode to reduce power consumption. However, it needs to be
disabled in case of charging or OTG mode.

Tested-by: Raymond Hackley <raymondhackley@protonmail.com>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 drivers/power/supply/rt5033_charger.c | 47 ++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/rt5033_charger.c b/drivers/power/supply/rt5033_charger.c
index 79e7f75fe634..ab406fc9fa19 100644
--- a/drivers/power/supply/rt5033_charger.c
+++ b/drivers/power/supply/rt5033_charger.c
@@ -298,6 +298,17 @@ static int rt5033_charger_reg_init(struct rt5033_charger *charger)
 		return -EINVAL;
 	}
 
+	/*
+	 * Enable high impedance mode. It stops charging or boosting and
+	 * operates at a low current sinking to reduce power consumption.
+	 */
+	ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+			RT5033_CHGCTRL1_HZ_MASK, RT5033_CHARGER_HZ_ENABLE);
+	if (ret) {
+		dev_err(charger->dev, "Failed to enable high impedance mode.\n");
+		return -EINVAL;
+	}
+
 	ret = rt5033_init_pre_charge(charger);
 	if (ret)
 		return ret;
@@ -319,6 +330,14 @@ static int rt5033_charger_set_otg(struct rt5033_charger *charger)
 
 	mutex_lock(&charger->lock);
 
+	/* Disable high impedance mode to allow OTG mode */
+	ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+			RT5033_CHGCTRL1_HZ_MASK, RT5033_CHARGER_HZ_DISABLE);
+	if (ret) {
+		dev_err(charger->dev, "Failed to disable high impedance mode.\n");
+		return -EINVAL;
+	}
+
 	/* Set OTG boost v_out to 5 volts */
 	ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL2,
 			RT5033_CHGCTRL2_CV_MASK,
@@ -381,6 +400,14 @@ static int rt5033_charger_set_charging(struct rt5033_charger *charger)
 
 	mutex_lock(&charger->lock);
 
+	/* Disable high impedance mode to allow charging mode */
+	ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+			RT5033_CHGCTRL1_HZ_MASK, RT5033_CHARGER_HZ_DISABLE);
+	if (ret) {
+		dev_err(charger->dev, "Failed to disable high impedance mode.\n");
+		return -EINVAL;
+	}
+
 	/* In case someone switched from OTG to charging directly */
 	if (charger->otg) {
 		ret = rt5033_charger_unset_otg(charger);
@@ -431,6 +458,14 @@ static int rt5033_charger_set_disconnect(struct rt5033_charger *charger)
 
 	mutex_lock(&charger->lock);
 
+	/* Enable high impedance mode to reduce power consumption */
+	ret = regmap_update_bits(charger->rt5033->regmap, RT5033_REG_CHG_CTRL1,
+			RT5033_CHGCTRL1_HZ_MASK, RT5033_CHARGER_HZ_ENABLE);
+	if (ret) {
+		dev_err(charger->dev, "Failed to enable high impedance mode.\n");
+		return -EINVAL;
+	}
+
 	/* Disable MIVR if enabled */
 	if (charger->mivr_enabled) {
 		ret = regmap_update_bits(charger->rt5033->regmap,
@@ -671,11 +706,21 @@ static int rt5033_charger_probe(struct platform_device *pdev)
 
 	/*
 	 * Extcon support is not vital for the charger to work. If no extcon
-	 * is available, just emit a warning and leave the probe function.
+	 * is available, just emit a warning, disable high impedance mode 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");
+		ret = regmap_update_bits(charger->rt5033->regmap,
+					 RT5033_REG_CHG_CTRL1,
+					 RT5033_CHGCTRL1_HZ_MASK,
+					 RT5033_CHARGER_HZ_DISABLE);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to disable high impedance mode.\n");
+			return -EINVAL;
+		}
 		goto out;
 	}
 
-- 
2.39.1

[PATCH 09/10] power: supply: rt5033_battery: Adopt status property from charger

[Jakob Hauser]

The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
can, let's get this value.

Tested-by: Raymond Hackley <raymondhackley@protonmail.com>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 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.1

[PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Jakob Hauser]

Add device tree binding documentation for rt5033 multifunction device, voltage
regulator and battery charger.

Cc: Beomho Seo <beomho.seo@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
 .../bindings/mfd/richtek,rt5033.yaml          | 102 ++++++++++++++++++
 .../power/supply/richtek,rt5033-charger.yaml  |  76 +++++++++++++
 .../regulator/richtek,rt5033-regulator.yaml   |  45 ++++++++
 3 files changed, 223 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..f1a58694c81e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
@@ -0,0 +1,102 @@
+# 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 <jahau@rocketmail.com>
+
+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@0 {
+        #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-uamp = <450000>;
+                richtek,fast-uamp = <1000000>;
+                richtek,eoc-uamp = <150000>;
+                richtek,pre-threshold-uvolt = <3500000>;
+                richtek,const-uvolt = <4350000>;
+                extcon = <&muic>;
+            };
+        };
+    };
+
+    i2c@1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        battery@35 {
+            compatible = "richtek,rt5033-battery";
+            reg = <0x35>;
+            interrupt-parent = <&msmgpio>;
+            interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
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..996c2932927d
--- /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 <jahau@rocketmail.com>
+
+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-uamp:
+    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-uamp:
+    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-uamp:
+    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-uvolt:
+    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-uvolt:
+    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-uamp
+  - richtek,fast-uamp
+  - richtek,eoc-uamp
+  - richtek,pre-threshold-uvolt
+  - richtek,const-uvolt
+
+additionalProperties: false
+
+examples:
+  - |
+    charger {
+        compatible = "richtek,rt5033-charger";
+        richtek,pre-uamp = <450000>;
+        richtek,fast-uamp = <1000000>;
+        richtek,eoc-uamp = <150000>;
+        richtek,pre-threshold-uvolt = <3500000>;
+        richtek,const-uvolt = <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..61b074488db4
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
@@ -0,0 +1,45 @@
+# 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 <jahau@rocketmail.com>
+
+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
+
+examples:
+  - |
+    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>;
+        };
+     };
-- 
2.39.1

Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Rob Herring]

On Tue, 28 Feb 2023 23:32:27 +0100, Jakob Hauser wrote:
> Add device tree binding documentation for rt5033 multifunction device, voltage
> regulator and battery charger.
> 
> Cc: Beomho Seo <beomho.seo@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  .../bindings/mfd/richtek,rt5033.yaml          | 102 ++++++++++++++++++
>  .../power/supply/richtek,rt5033-charger.yaml  |  76 +++++++++++++
>  .../regulator/richtek,rt5033-regulator.yaml   |  45 ++++++++
>  3 files changed, 223 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-threshold-uvolt: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-threshold-uvolt: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-threshold-uvolt: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,const-uvolt: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,const-uvolt: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,const-uvolt: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,eoc-uamp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,eoc-uamp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,eoc-uamp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,fast-uamp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,fast-uamp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,fast-uamp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-uamp: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-uamp: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml: properties:richtek,pre-uamp: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
Documentation/devicetree/bindings/mfd/richtek,rt5033.example.dts:23.15-66.11: Warning (unit_address_vs_reg): /example-0/i2c@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/mfd/richtek,rt5033.example.dts:68.15-78.11: Warning (unit_address_vs_reg): /example-0/i2c@1: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/a698f524106e0eb7db5cbd7e73e77ecd5ac8ad7f.1677620677.git.jahau@rocketmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Rob Herring]

On Tue, Feb 28, 2023 at 11:32:27PM +0100, Jakob Hauser wrote:
> Add device tree binding documentation for rt5033 multifunction device, voltage
> regulator and battery charger.
> 
> Cc: Beomho Seo <beomho.seo@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  .../bindings/mfd/richtek,rt5033.yaml          | 102 ++++++++++++++++++
>  .../power/supply/richtek,rt5033-charger.yaml  |  76 +++++++++++++
>  .../regulator/richtek,rt5033-regulator.yaml   |  45 ++++++++
>  3 files changed, 223 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..f1a58694c81e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
> @@ -0,0 +1,102 @@
> +# 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 <jahau@rocketmail.com>
> +
> +description: |

Don't need '|' unless you care about line endings.

> +  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@0 {

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-uamp = <450000>;
> +                richtek,fast-uamp = <1000000>;
> +                richtek,eoc-uamp = <150000>;
> +                richtek,pre-threshold-uvolt = <3500000>;
> +                richtek,const-uvolt = <4350000>;
> +                extcon = <&muic>;
> +            };
> +        };
> +    };
> +
> +    i2c@1 {

This should be a separate example entry.

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        battery@35 {
> +            compatible = "richtek,rt5033-battery";
> +            reg = <0x35>;
> +            interrupt-parent = <&msmgpio>;
> +            interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
> +        };
> +    };
> 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..996c2932927d
> --- /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 <jahau@rocketmail.com>
> +
> +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-uamp:

Use defined standard unit type suffixes.

> +    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-uamp:
> +    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-uamp:
> +    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-uvolt:
> +    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-uvolt:
> +    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:

This is deprecated. There's standard connector bindings now.

> +    description: |
> +      Phandle to the extcon device.
> +    maxItems: 1
> +
> +required:
> +  - richtek,pre-uamp
> +  - richtek,fast-uamp
> +  - richtek,eoc-uamp
> +  - richtek,pre-threshold-uvolt
> +  - richtek,const-uvolt
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    charger {
> +        compatible = "richtek,rt5033-charger";
> +        richtek,pre-uamp = <450000>;
> +        richtek,fast-uamp = <1000000>;
> +        richtek,eoc-uamp = <150000>;
> +        richtek,pre-threshold-uvolt = <3500000>;
> +        richtek,const-uvolt = <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..61b074488db4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
> @@ -0,0 +1,45 @@
> +# 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 <jahau@rocketmail.com>
> +
> +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)$":

Lowercase preferred for node names.

> +    type: object
> +    $ref: /schemas/regulator/regulator.yaml#
> +    unevaluatedProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    regulators {

Just 1 complete example in the MFD binding please.

> +        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>;
> +        };
> +     };
> -- 
> 2.39.1
> 

Re: [PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Lee Jones]

On Tue, 28 Feb 2023, Jakob Hauser wrote:

> 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 <jahau@rocketmail.com>
> ---
>  drivers/mfd/rt5033.c               | 8 +++++---
>  include/linux/mfd/rt5033-private.h | 4 ++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
> index 8029d444b794..d32467174cb5 100644
> --- a/drivers/mfd/rt5033.c
> +++ b/drivers/mfd/rt5033.c
> @@ -55,7 +55,8 @@ 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 data;

In terms of nomenclature, this is a regression.

'data' is a terrible variable name.  Why not keep it as-is?

> +	unsigned int chip_rev;
>  	int ret;
>  
>  	rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
> @@ -73,12 +74,13 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
>  		return PTR_ERR(rt5033->regmap);
>  	}
>  
> -	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &dev_id);
> +	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &data);
>  	if (ret) {
>  		dev_err(&i2c->dev, "Device not found\n");
>  		return -ENODEV;
>  	}
> -	dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
> +	chip_rev = data & RT5033_CHIP_REV_MASK;
> +	dev_info(&i2c->dev, "Device found (rev. %d)\n", chip_rev);

Why not print both?

>  	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 2d1895c3efbf..d18cd4572208 100644
> --- a/include/linux/mfd/rt5033-private.h
> +++ b/include/linux/mfd/rt5033-private.h
> @@ -71,6 +71,10 @@ enum rt5033_reg {














g
>  /* 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.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 03/10] mfd: rt5033: Fix comments and style in includes

[Lee Jones]

On Tue, 28 Feb 2023, Jakob Hauser wrote:

> Fix comments and remove some empty lines in rt5033-private.h. Align struct
> rt5033_charger in rt5033.h.
> 
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  include/linux/mfd/rt5033-private.h | 17 +++++++----------
>  include/linux/mfd/rt5033.h         |  7 +++----
>  2 files changed, 10 insertions(+), 14 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

Re: [PATCH 04/10] mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines

[Lee Jones]

On Tue, 28 Feb 2023, Jakob Hauser wrote:

> 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 <jahau@rocketmail.com>
> ---
>  include/linux/mfd/rt5033-private.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

For my own reference (apply this as-is to your sign-off block):

Acked-for-MFD-by: Lee Jones <lee@kernel.org>

-- 
Lee Jones [李琼斯]

Re: [PATCH 05/10] mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver

[Lee Jones]

On Tue, 28 Feb 2023, Jakob Hauser wrote:

> 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().

Are there no present users to account for?

> [1] https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
> 
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>  include/linux/mfd/rt5033-private.h | 53 ++++++++++++++++++++----------
>  include/linux/mfd/rt5033.h         |  2 +-
>  2 files changed, 36 insertions(+),` 19 deletions(-)

-- 
Lee Jones [李琼斯]

Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Jakob Hauser]

Hi Rob,

thanks for the remarks and sorry for not running 'make 
dt_binding_check'. I'm not familiar with devicetree bindings and 
obviously missed to read the file 
Documentation/devicetree/bindings/submitting-patches.rst.

On 01.03.23 03:35, Rob Herring wrote:
> On Tue, Feb 28, 2023 at 11:32:27PM +0100, Jakob Hauser wrote:
>> Add device tree binding documentation for rt5033 multifunction device, voltage
>> regulator and battery charger.
>>
>> Cc: Beomho Seo <beomho.seo@samsung.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
>> ---
>>   .../bindings/mfd/richtek,rt5033.yaml          | 102 ++++++++++++++++++
>>   .../power/supply/richtek,rt5033-charger.yaml  |  76 +++++++++++++
>>   .../regulator/richtek,rt5033-regulator.yaml   |  45 ++++++++
>>   3 files changed, 223 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..f1a58694c81e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> @@ -0,0 +1,102 @@
>> +# 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 <jahau@rocketmail.com>
>> +
>> +description: |
> 
> Don't need '|' unless you care about line endings.

OK

>> +  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@0 {
> 
> i2c {

OK

>> +        #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-uamp = <450000>;
>> +                richtek,fast-uamp = <1000000>;
>> +                richtek,eoc-uamp = <150000>;
>> +                richtek,pre-threshold-uvolt = <3500000>;
>> +                richtek,const-uvolt = <4350000>;
>> +                extcon = <&muic>;
>> +            };
>> +        };
>> +    };
>> +
>> +    i2c@1 {
> 
> This should be a separate example entry.

I'll skip it then.

The battery fuel gauge is not handled as a part of the MFD, it has a 
separate I2C line. Accordingly, it has a separate documentation 
including examples [1].

I had implemented into the MFD example to make clear this is separated. 
But as it is not part of the MFD, I guess it shouldn't show up in the 
MFD documentation.

In the description of the MFD there is the statement "The battery fuel 
gauge uses a separate I2C bus." I hope this is clear enough, I'm not 
sure if I should modify/extent that statement or add some kind of 
reference to the battery fuel gauge after removing it from the example.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml?h=v6.2

>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        battery@35 {
>> +            compatible = "richtek,rt5033-battery";
>> +            reg = <0x35>;
>> +            interrupt-parent = <&msmgpio>;
>> +            interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
>> +        };
>> +    };
>> 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..996c2932927d
>> --- /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 <jahau@rocketmail.com>
>> +
>> +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-uamp:
> 
> Use defined standard unit type suffixes.

That makes sense. I took the current names from the 2015 patchset and 
wasn't aware of the standard suffixes.

Just for the record: Chaning the names will also impact patch 06 "power: 
supply: rt5033_charger: Add RT5033 charger device driver", as the names 
are parsed there.

>> +    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-uamp:
>> +    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-uamp:
>> +    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-uvolt:
>> +    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-uvolt:
>> +    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:
> 
> This is deprecated. There's standard connector bindings now.

How does this work? I couldn't find an example.

I found Documentation/devicetree/bindings/connector/usb-connector.yaml 
[2] but I don't see how this would be applied here.

The extcon device entry in the samsung-serranove devicetree [3] looks like:

i2c-muic {
         compatible = "i2c-gpio";
         sda-gpios = <&msmgpio 105 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
         scl-gpios = <&msmgpio 106 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;

         pinctrl-names = "default";
         pinctrl-0 = <&muic_i2c_default>;

         #address-cells = <1>;
         #size-cells = <0>;

         muic: extcon@14 {
                 compatible = "siliconmitus,sm5504-muic";
                 reg = <0x14>;

                 interrupt-parent = <&msmgpio>;
                 interrupts = <12 IRQ_TYPE_EDGE_FALLING>;

                 pinctrl-names = "default";
                 pinctrl-0 = <&muic_irq_default>;
         };
};

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/connector/usb-connector.yaml?h=v6.2
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts?h=v6.2#n123

>> +    description: |
>> +      Phandle to the extcon device.
>> +    maxItems: 1
>> +
>> +required:
>> +  - richtek,pre-uamp
>> +  - richtek,fast-uamp
>> +  - richtek,eoc-uamp
>> +  - richtek,pre-threshold-uvolt
>> +  - richtek,const-uvolt
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    charger {
>> +        compatible = "richtek,rt5033-charger";
>> +        richtek,pre-uamp = <450000>;
>> +        richtek,fast-uamp = <1000000>;
>> +        richtek,eoc-uamp = <150000>;
>> +        richtek,pre-threshold-uvolt = <3500000>;
>> +        richtek,const-uvolt = <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..61b074488db4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> @@ -0,0 +1,45 @@
>> +# 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 <jahau@rocketmail.com>
>> +
>> +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)$":
> 
> Lowercase preferred for node names.

OK, I can change to lowercase.

Though I have to change the already existing driver rt5033-regulator as 
well then [4]. I'm not sure if this has an impact on already existing 
implementations. Although within the upstream kernel I think there is no 
usage of the rt5033-regulator driver yet.

[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/rt5033-regulator.c?h=v6.2#n42

>> +    type: object
>> +    $ref: /schemas/regulator/regulator.yaml#
>> +    unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    regulators {
> 
> Just 1 complete example in the MFD binding please.

OK, I'll skip the examples part here then.

>> +        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>;
>> +        };
>> +     };
>> -- 
>> 2.39.1
>>

I'll wait with implementing the changes as there will be likely further 
comments on the other patches.

Kind regards,
Jakob

Re: [PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Jakob Hauser]

Hi Lee,

On 05.03.23 11:47, Lee Jones wrote:
> On Tue, 28 Feb 2023, Jakob Hauser wrote:
> 
>> 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 <jahau@rocketmail.com>
>> ---
>>   drivers/mfd/rt5033.c               | 8 +++++---
>>   include/linux/mfd/rt5033-private.h | 4 ++++
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
>> index 8029d444b794..d32467174cb5 100644
>> --- a/drivers/mfd/rt5033.c
>> +++ b/drivers/mfd/rt5033.c
>> @@ -55,7 +55,8 @@ 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 data;
> 
> In terms of nomenclature, this is a regression.
> 
> 'data' is a terrible variable name.  Why not keep it as-is?

While not having a datasheet for RT5033 available, in similar products 
like RT9455 the register is called "Device ID", the first part of that 
is "VENDOR_ID" and the second part "CHIP_REV", [1] page 23 top. Or in 
RT5036 preliminary data sheet the register is called "ID", the first 
part "VENDOR_ID" and the second part "CHIP_REV_ID", [2] page 27 top.

I wanted to avoid confusion between "dev_id" and "chip_rev". Therefore 
in the patch it's written as getting some "data" from the register and 
extract "chip_rev" from that data.

I could change it to "reg_data"? Or something in that direction? I still 
think that getting "chip_rev" out of "dev_id" would be confusing.

[1] https://www.richtek.com/assets/product_file/RT9455/DS9455-00.pdf
[2] 
https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf

> 
>> +	unsigned int chip_rev;
>>   	int ret;
>>   
>>   	rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
>> @@ -73,12 +74,13 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
>>   		return PTR_ERR(rt5033->regmap);
>>   	}
>>   
>> -	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &dev_id);
>> +	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &data);
>>   	if (ret) {
>>   		dev_err(&i2c->dev, "Device not found\n");
>>   		return -ENODEV;
>>   	}
>> -	dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
>> +	chip_rev = data & RT5033_CHIP_REV_MASK;
>> +	dev_info(&i2c->dev, "Device found (rev. %d)\n", chip_rev);
> 
> Why not print both?

As described above, the data "dev_id" consists of a first part which is 
a vendor ID and a second part which is the chip revision.

The vendor ID is of no interest here. These bits[7:4] contain binary 
value 1000 (decimal value 8) and I'd expect that to be the same on all 
RT5033 devices.

Contrary to this, the chip revision is an important information. The 
downstream Android driver applies some quirks depending on the chip 
revision. This seemed not yet necessary in the upstream driver. So far 
I've seen chip rev. 6 on samsung-serranove & samsung-e7 and chip rev. 5 
on samsung-grandmax & samsung-fortuna, the behavior of the chip 
revisions are slightly different.

Accordingly, the downstream Android driver as well reads [3] and prints 
[4] the chip revision only – confusingly calling it "rev id".
[3] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L484
[4] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L486

>>   	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 2d1895c3efbf..d18cd4572208 100644
>> --- a/include/linux/mfd/rt5033-private.h
>> +++ b/include/linux/mfd/rt5033-private.h
>> @@ -71,6 +71,10 @@ enum rt5033_reg {
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> g

What does the "g" mean, was this on purpose? I didn't get the meaning of it.

>>   /* 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.1
>>
> 

Kind regards,
Jakob

Re: [PATCH 03/10] mfd: rt5033: Fix comments and style in includes

[Jakob Hauser]

Hi Lee,

On 05.03.23 11:48, Lee Jones wrote:
> On Tue, 28 Feb 2023, Jakob Hauser wrote:
> 
>> Fix comments and remove some empty lines in rt5033-private.h. Align struct
>> rt5033_charger in rt5033.h.
>>
>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
>> ---
>>   include/linux/mfd/rt5033-private.h | 17 +++++++----------
>>   include/linux/mfd/rt5033.h         |  7 +++----
>>   2 files changed, 10 insertions(+), 14 deletions(-)
> 
> Applied, thanks
> 

Thanks! Does this mean I should skip this patch in the next versions of 
the patchset? Or should I just add the Acked-for-MFD-by tag of yours? In 
the first case I'm not sure what base to use for the next versions of 
the patchset. Sorry, I'm not so much familiar with the procedures.

Kind regards,
Jakob

Re: [PATCH 05/10] mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver

[Jakob Hauser]

Hi Lee,

On 05.03.23 11:55, Lee Jones wrote:
> On Tue, 28 Feb 2023, Jakob Hauser wrote:
> 
>> 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().
> 
> Are there no present users to account for?

At least none I'm aware of. Within the upstream kernel the 
rt5033_charger power_supply didn't exist so far, the patchset is about 
to implement it.

>> [1] https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
>>
>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
>> ---
>>   include/linux/mfd/rt5033-private.h | 53 ++++++++++++++++++++----------
>>   include/linux/mfd/rt5033.h         |  2 +-
>>   2 files changed, 36 insertions(+),` 19 deletions(-)
> 

Kind regards,
Jakob

Re: [PATCH 03/10] mfd: rt5033: Fix comments and style in includes

[Lee Jones]

On Sun, 05 Mar 2023, Jakob Hauser wrote:

> Hi Lee,
> 
> On 05.03.23 11:48, Lee Jones wrote:
> > On Tue, 28 Feb 2023, Jakob Hauser wrote:
> > 
> > > Fix comments and remove some empty lines in rt5033-private.h. Align struct
> > > rt5033_charger in rt5033.h.
> > > 
> > > Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> > > ---
> > >   include/linux/mfd/rt5033-private.h | 17 +++++++----------
> > >   include/linux/mfd/rt5033.h         |  7 +++----
> > >   2 files changed, 10 insertions(+), 14 deletions(-)
> > 
> > Applied, thanks
> > 
> 
> Thanks! Does this mean I should skip this patch in the next versions of the
> patchset? Or should I just add the Acked-for-MFD-by tag of yours? In the
> first case I'm not sure what base to use for the next versions of the
> patchset. Sorry, I'm not so much familiar with the procedures.

You should rebase onto -next before sending out your next submission.

This patch should vanish from the set.

If it doesn't, please wait another 24hrs and try again.

-- 
Lee Jones [李琼斯]

Re: [PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Lee Jones]

On Sun, 05 Mar 2023, Jakob Hauser wrote:

> Hi Lee,
> 
> On 05.03.23 11:47, Lee Jones wrote:
> > On Tue, 28 Feb 2023, Jakob Hauser wrote:
> > 
> > > 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 <jahau@rocketmail.com>
> > > ---
> > >   drivers/mfd/rt5033.c               | 8 +++++---
> > >   include/linux/mfd/rt5033-private.h | 4 ++++
> > >   2 files changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
> > > index 8029d444b794..d32467174cb5 100644
> > > --- a/drivers/mfd/rt5033.c
> > > +++ b/drivers/mfd/rt5033.c
> > > @@ -55,7 +55,8 @@ 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 data;
> > 
> > In terms of nomenclature, this is a regression.
> > 
> > 'data' is a terrible variable name.  Why not keep it as-is?
> 
> While not having a datasheet for RT5033 available, in similar products like
> RT9455 the register is called "Device ID", the first part of that is
> "VENDOR_ID" and the second part "CHIP_REV", [1] page 23 top. Or in RT5036
> preliminary data sheet the register is called "ID", the first part
> "VENDOR_ID" and the second part "CHIP_REV_ID", [2] page 27 top.
> 
> I wanted to avoid confusion between "dev_id" and "chip_rev". Therefore in
> the patch it's written as getting some "data" from the register and extract
> "chip_rev" from that data.
> 
> I could change it to "reg_data"? Or something in that direction? I still
> think that getting "chip_rev" out of "dev_id" would be confusing.

You're reading from a register called RT5033_REG_DEVICE_ID.  I don't see
any reason why the variable you read into can't reflect that.

> [1] https://www.richtek.com/assets/product_file/RT9455/DS9455-00.pdf
> [2] https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
> 
> > 
> > > +	unsigned int chip_rev;
> > >   	int ret;
> > >   	rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
> > > @@ -73,12 +74,13 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
> > >   		return PTR_ERR(rt5033->regmap);
> > >   	}
> > > -	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &dev_id);
> > > +	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &data);
> > >   	if (ret) {
> > >   		dev_err(&i2c->dev, "Device not found\n");
> > >   		return -ENODEV;
> > >   	}
> > > -	dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
> > > +	chip_rev = data & RT5033_CHIP_REV_MASK;
> > > +	dev_info(&i2c->dev, "Device found (rev. %d)\n", chip_rev);
> > 
> > Why not print both?
> 
> As described above, the data "dev_id" consists of a first part which is a
> vendor ID and a second part which is the chip revision.
> 
> The vendor ID is of no interest here. These bits[7:4] contain binary value
> 1000 (decimal value 8) and I'd expect that to be the same on all RT5033
> devices.
> 
> Contrary to this, the chip revision is an important information. The
> downstream Android driver applies some quirks depending on the chip
> revision. This seemed not yet necessary in the upstream driver. So far I've
> seen chip rev. 6 on samsung-serranove & samsung-e7 and chip rev. 5 on
> samsung-grandmax & samsung-fortuna, the behavior of the chip revisions are
> slightly different.
> 
> Accordingly, the downstream Android driver as well reads [3] and prints [4]
> the chip revision only – confusingly calling it "rev id".
> [3] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L484
> [4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L486
> 
> > >   	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 2d1895c3efbf..d18cd4572208 100644
> > > --- a/include/linux/mfd/rt5033-private.h
> > > +++ b/include/linux/mfd/rt5033-private.h
> > > @@ -71,6 +71,10 @@ enum rt5033_reg {
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > g
> 
> What does the "g" mean, was this on purpose? I didn't get the meaning of it.
> 
> > >   /* 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.1
> > > 
> > 
> 
> Kind regards,
> Jakob

-- 
Lee Jones [李琼斯]

Re: [PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Jakob Hauser]

Hi Lee,

On 06.03.23 10:18, Lee Jones wrote:
> On Sun, 05 Mar 2023, Jakob Hauser wrote:
> 
>> Hi Lee,
>>
>> On 05.03.23 11:47, Lee Jones wrote:
>>> On Tue, 28 Feb 2023, Jakob Hauser wrote:
>>>
>>>> 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 <jahau@rocketmail.com>
>>>> ---
>>>>    drivers/mfd/rt5033.c               | 8 +++++---
>>>>    include/linux/mfd/rt5033-private.h | 4 ++++
>>>>    2 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
>>>> index 8029d444b794..d32467174cb5 100644
>>>> --- a/drivers/mfd/rt5033.c
>>>> +++ b/drivers/mfd/rt5033.c
>>>> @@ -55,7 +55,8 @@ 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 data;
>>>
>>> In terms of nomenclature, this is a regression.
>>>
>>> 'data' is a terrible variable name.  Why not keep it as-is?
>>
>> While not having a datasheet for RT5033 available, in similar products like
>> RT9455 the register is called "Device ID", the first part of that is
>> "VENDOR_ID" and the second part "CHIP_REV", [1] page 23 top. Or in RT5036
>> preliminary data sheet the register is called "ID", the first part
>> "VENDOR_ID" and the second part "CHIP_REV_ID", [2] page 27 top.
>>
>> I wanted to avoid confusion between "dev_id" and "chip_rev". Therefore in
>> the patch it's written as getting some "data" from the register and extract
>> "chip_rev" from that data.
>>
>> I could change it to "reg_data"? Or something in that direction? I still
>> think that getting "chip_rev" out of "dev_id" would be confusing.
> 
> You're reading from a register called RT5033_REG_DEVICE_ID.  I don't see
> any reason why the variable you read into can't reflect that.

OK, I'll use "dev_id" and "chip_rev" for the variable names.

...

Kind regards,
Jakob

Re: [PATCH 00/10] Add RT5033 charger device driver

[Pavel Machek]

Hi!

> Some comments on the end-of-charge behavior. The rt5033 chip offers three
> options. In the Android driver, a forth option was implemented.

Hmm. I'm working on that on motorola-cpcap driver, and I guess this is going
to be common problem for many drivers.

> - By default, the rt5033 chip charges indefinitely. The current goes down but
>   there is always a charge voltage to the battery, which might not be too good
>   for the battery lifetime.
> - There is the possibility to enable a fast charge timer. The timer can be
>   set to 4, 6, 8... 16 hours. After that time has elapsed, charging stops
>   and the battery gets discharged. This option with a timer of 4 hours was
>   chosen by Beomho Seo in the patchset of March 2015. However, that option
>   is confusing to the user. It doesn't initiate a re-charge cycle. So when
>   keeping plugged in the device over night, I find it discharging on the
>   next morning.
> - The third option of the rt5033 chip is enabling charging termination. This
>   also enables a re-charge cycle. When the charging current sinks below the
>   end-of-charge current, the chip stops charging. The sysfs state changes to
>   "not charging". When the voltage gets 0.1 V below the end-of-charge constant
>   voltage, re-charging starts. Then again, when charging current sinks below
>   the end-of-charge current, the chip stops charging. And so on, going up and
>   down in re-charge cycles. In case the power consumption is high (e.g. tuning
>   on the display of the mobile device), the current goes into an equilibrium.
>   The downside of this charging termination option: When reaching the end-of-
>   charge current, the capacity might not have reached 100 % yet. The capacity
>   to reach probably depends on power consumption and battery wear. On my mobile
>   device, capacity reaches 98 %, drops to 96 % until re-charging kicks in,
>   climbs to 98 %, drops to 96 %, and so on. Not reaching 100 % is a bit
>   confusing to the user, too.

Is the system powered from the battery in the not-charging case?

Anyway, we should teach userspace that "full battery" does not neccessary mean 100%,
as keeping battery at 4.3V wears it down quickly.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 00/10] Add RT5033 charger device driver

[Jakob Hauser]

Hi Pavel,

On 25.03.23 17:08, Pavel Machek wrote:
> Hi!

...

>> - The third option of the rt5033 chip is enabling charging termination. This
>>    also enables a re-charge cycle. When the charging current sinks below the
>>    end-of-charge current, the chip stops charging. The sysfs state changes to
>>    "not charging". When the voltage gets 0.1 V below the end-of-charge constant
>>    voltage, re-charging starts. Then again, when charging current sinks below
>>    the end-of-charge current, the chip stops charging. And so on, going up and
>>    down in re-charge cycles. In case the power consumption is high (e.g. tuning
>>    on the display of the mobile device), the current goes into an equilibrium.
>>    The downside of this charging termination option: When reaching the end-of-
>>    charge current, the capacity might not have reached 100 % yet. The capacity
>>    to reach probably depends on power consumption and battery wear. On my mobile
>>    device, capacity reaches 98 %, drops to 96 % until re-charging kicks in,
>>    climbs to 98 %, drops to 96 %, and so on. Not reaching 100 % is a bit
>>    confusing to the user, too.
> 
> Is the system powered from the battery in the not-charging case?

Yes, at RT5033 in the "not charging" state the system draws the power 
from the battery.

...

Kind regards,
Jakob

Re: [PATCH 05/10] mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver

[Jakob Hauser]

Hi Lee,

On 05.03.23 17:14, Jakob Hauser wrote:
> On 05.03.23 11:55, Lee Jones wrote:
>> On Tue, 28 Feb 2023, Jakob Hauser wrote:
>>
>>> 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().
>>
>> Are there no present users to account for?
> 
> At least none I'm aware of. Within the upstream kernel the 
> rt5033_charger power_supply didn't exist so far, the patchset is about 
> to implement it.

Is there anything you want me to change or improve on this patch?

>>> [1] 
>>> https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
>>>
>>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
>>> ---
>>>   include/linux/mfd/rt5033-private.h | 53 ++++++++++++++++++++----------
>>>   include/linux/mfd/rt5033.h         |  2 +-
>>>   2 files changed, 36 insertions(+),` 19 deletions(-)

Kind regards,
Jakob

Re: [PATCH 08/10] power: supply: rt5033_charger: Make use of high impedance mode

[Jakob Hauser]

Hi Sebastian,

On 28.02.23 23:32, Jakob Hauser wrote:
> Enable high impedance mode to reduce power consumption. However, it needs to be
> disabled in case of charging or OTG mode.
> 
> Tested-by: Raymond Hackley <raymondhackley@protonmail.com>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
>   drivers/power/supply/rt5033_charger.c | 47 ++++++++++++++++++++++++++-
>   1 file changed, 46 insertions(+), 1 deletion(-)

...

Raymond (in copy) did some tests on the flash LEDs, which are also 
managed by the rt5033 chip. There is no driver for leds-rt5033 yet but 
Raymond got the rt5033 LEDs running via the similar driver leds-sgm3140.

However, to get the flash LEDs working, he had to disable the high 
impedance mode of rt5033.

I implemented the use of high impedance mode by this patch to improve 
power saving. It's kind of a sleep mode. Although it's not clear how 
much power it does save, it's generally worth trying to improve power 
saving on mobile devices as far as possible.

As it now turns out that the use of high impedance mode might complicate 
the handling of the flash LEDs, I would drop this patch in the next 
version v2 of the patchset. Let's skip this power saving attempt for 
now. It still can be added at a later date as an improvement.

Kind regards,
Jakob

Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Jakob Hauser]

Hi Rob,

On 05.03.23 16:54, Jakob Hauser wrote:
...
> On 01.03.23 03:35, Rob Herring wrote:
>> On Tue, Feb 28, 2023 at 11:32:27PM +0100, Jakob Hauser wrote:

...

>>> +  richtek,pre-threshold-uvolt:
>>> +    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-uvolt:
>>> +    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:
>>
>> This is deprecated. There's standard connector bindings now.
> 
> How does this work? I couldn't find an example.
> 
> I found Documentation/devicetree/bindings/connector/usb-connector.yaml 
> [2] but I don't see how this would be applied here.
> 
> The extcon device entry in the samsung-serranove devicetree [3] looks like:
> 
> i2c-muic {
>          compatible = "i2c-gpio";
>          sda-gpios = <&msmgpio 105 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
>          scl-gpios = <&msmgpio 106 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> 
>          pinctrl-names = "default";
>          pinctrl-0 = <&muic_i2c_default>;
> 
>          #address-cells = <1>;
>          #size-cells = <0>;
> 
>          muic: extcon@14 {
>                  compatible = "siliconmitus,sm5504-muic";
>                  reg = <0x14>;
> 
>                  interrupt-parent = <&msmgpio>;
>                  interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> 
>                  pinctrl-names = "default";
>                  pinctrl-0 = <&muic_irq_default>;
>          };
> };
> 
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/connector/usb-connector.yaml?h=v6.2
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts?h=v6.2#n123

Could you add more information on what you mean by standard connector 
bindings? It's not clear to me.

>>> +    description: |
>>> +      Phandle to the extcon device.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - richtek,pre-uamp
>>> +  - richtek,fast-uamp
>>> +  - richtek,eoc-uamp
>>> +  - richtek,pre-threshold-uvolt
>>> +  - richtek,const-uvolt
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    charger {
>>> +        compatible = "richtek,rt5033-charger";
>>> +        richtek,pre-uamp = <450000>;
>>> +        richtek,fast-uamp = <1000000>;
>>> +        richtek,eoc-uamp = <150000>;
>>> +        richtek,pre-threshold-uvolt = <3500000>;
>>> +        richtek,const-uvolt = <4350000>;
>>> +        extcon = <&muic>;
>>> +    };

...

Kind regards,
Jakob

Re: [PATCH 05/10] mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver

[Lee Jones]

On Sun, 02 Apr 2023, Jakob Hauser wrote:

> Hi Lee,
>
> On 05.03.23 17:14, Jakob Hauser wrote:
> > On 05.03.23 11:55, Lee Jones wrote:
> > > On Tue, 28 Feb 2023, Jakob Hauser wrote:
> > >
> > > > 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().
> > >
> > > Are there no present users to account for?
> >
> > At least none I'm aware of. Within the upstream kernel the
> > rt5033_charger power_supply didn't exist so far, the patchset is about
> > to implement it.
>
> Is there anything you want me to change or improve on this patch?

If there were I would have said. :)  Please fix up the other review
comments in the set and submit the next revision.

> > > > [1] https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf
> > > >
> > > > Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> > > > ---
> > > >   include/linux/mfd/rt5033-private.h | 53 ++++++++++++++++++++----------
> > > >   include/linux/mfd/rt5033.h         |  2 +-
> > > >   2 files changed, 36 insertions(+),` 19 deletions(-)
>
> Kind regards,
> Jakob

--
Lee Jones [李琼斯]