mbox series

[00/10] Add RT5033 charger device driver

Message ID cover.1677620677.git.jahau@rocketmail.com
Headers show
Series Add RT5033 charger device driver | expand

Message

Jakob Hauser Feb. 28, 2023, 10:32 p.m. UTC 
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

Comments

Rob Herring Feb. 28, 2023, 11:15 p.m. UTC | #1 
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.
Rob Herring March 1, 2023, 2:35 a.m. UTC | #2 
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
>
Lee Jones March 5, 2023, 10:48 a.m. UTC | #3 
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 March 5, 2023, 10:52 a.m. UTC | #4 
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>
Jakob Hauser March 5, 2023, 3:54 p.m. UTC | #5 
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
Jakob Hauser March 5, 2023, 4:11 p.m. UTC | #6 
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
Lee Jones March 6, 2023, 9:15 a.m. UTC | #7 
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.
Pavel Machek March 25, 2023, 4:08 p.m. UTC | #8 
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
Jakob Hauser April 2, 2023, 10:14 a.m. UTC | #9 
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
Jakob Hauser April 2, 2023, 10:21 a.m. UTC | #10 
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