Re: [PATCH v5 3/3] Documentation: power: rt9471: Document exported sysfs entries

[ChiYuan Huang <u0084500@gmail.com>]

Hi,
ChiYuan Huang <u0084500@gmail.com> 於 2022年10月3日 週一 上午10:00寫道：
>
> Sebastian Reichel <sebastian.reichel@collabora.com> 於 2022年10月2日 週日 凌晨4:58寫道：
> >
> > Hi,
> >
> > On Mon, Sep 19, 2022 at 09:11:09AM +0800, ChiYuan Huang wrote:
> > > Sebastian Reichel <sebastian.reichel@collabora.com> 於 2022年9月17日 週六 上午9:19寫道：
> > > > On Thu, Sep 15, 2022 at 12:30:15AM +0800, cy_huang wrote:
> > > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > > >
> > > > > Document the settings exported by rt9471 charger driver through sysfs entries:
> > > > > - sysoff_enable
> > > > > - port_detect_enable
> > > > >
> > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > > ---
> > > > > Since v5:
> > > > > - Recover all the change in sysfs-class-power.
> > > > > - New a sysfs-class-power-rt9471 file.
> > > > > - Remove 'charge_term_enable' sysfs entry, directly integrate it in
> > > > >   'charge_term_current' power supply property control.
> > > > >
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-class-power-rt9471 | 29 ++++++++++++++++++++++
> > > > >  1 file changed, 29 insertions(+)
> > > > >  create mode 100644 Documentation/ABI/testing/sysfs-class-power-rt9471
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-class-power-rt9471 b/Documentation/ABI/testing/sysfs-class-power-rt9471
> > > > > new file mode 100644
> > > > > index 00000000..ad5b049
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-class-power-rt9471
> > > > > @@ -0,0 +1,29 @@
> > > > > +What:                /sys/class/power_supply/rt9471-*/sysoff_enable
> > > > > +Date:                Oct 2022
> > > > > +KernelVersion:       6.1
> > > > > +Contact:     ChiYuan Huang <cy_huang@richtek.com>
> > > > > +Description:
> > > > > +             This entry allows enabling the sysoff mode of rt9471 charger devices.
> > > > > +             If enabled and the input is removed, the internal battery FET is turned
> > > > > +             off to reduce the leakage from the BAT pin. See device datasheet for details.
> > > > > +             It's commonly used when the product enter shipping stage.
> > > > > +
> > > > > +             Access: Read, Write
> > > > > +             Valid values:
> > > > > +             - 1: enabled
> > > > > +             - 0: disabled
> > > >
> > > > I still fail to see why this needs to be controllable at runtime.
> > > > This looks like a hardware property. Are there any known products,
> > > > which need this disabled?
> > > It's just a switch, actually 'disabled' is not needed.
> > > For the enabled case, mostly used in below scenarios
> > > 1. Online testing, USB IN -> Factory testing -> write 1 to enable ->
> > > USB out -> immediately VSYS off -> pack
> > > 2. Offline testing no vbus -> Factory testing -> write 1 to enable ->
> > > immediately VSYS off -> pack
> > >
> > > The 'disable" can use to cancel the shipping mode in case 1 before USB out.
> > > It's more like the testing.
> > >
> > > Like as you said, shipping BATFET_OFF is all the hardware behavior.
> > > To leave this mode after VSYS off, there're three ways
> > > 1. power key pressed
> > > 2. VBUS IN
> > > 3. control BATFET_OFF to 0 (But it need SOC to be alive, at the time,
> > > VSYS off, no one can execute this I2C command)
> >
> > If factory testing and preperation is the only use case, I don't
> > think exposing this in sysfs and creating userspace ABI is worth
> > it. Just tell factory to use i2c-dev and poke the correct registers.
> >
> I agree your comment if there's only this case will use it.
>
> So I ask our HW members about this.
> They said there's still one case I didn't consider about.
> It's the dual charger scenario.
> If the charging process is entering CV mode, the slave charger is no
> need to join the charging.
> Then in common case, slave charger need to minimize the battery leakage.
> And the BATFET_OFF is needed to lower the battery leakage.
>
> They think this sysfs entry is needed. Can this persuade you?
>
> > > If what you care is no need to mention 'disable', then just remove it.
> > > It's fine.
> > > >
> > > > > +What:                /sys/class/power_supply/rt9471-*/port_detect_enable
> > > > > +Date:                Oct 2022
> > > > > +KernelVersion:       6.1
> > > > > +Contact:     ChiYuan Huang <cy_huang@richtek.com>
> > > > > +Description:
> > > > > +             This entry allows enabling the USB BC12 port detect function of rt9471 charger
> > > > > +             devices. If enabled and VBUS is inserted, device will start to do the BC12
> > > > > +             port detect and report the usb port type when port detect is done. See
> > > > > +             datasheet for details. Normally controlled when TypeC/USBPD port integrated.
> > > > > +
> > > > > +             Access: Read, Write
> > > > > +             Valid values:
> > > > > +             - 1: enabled
> > > > > +             - 0: disabled
> > > >
> > > > So basically this depends on the hardware integration (e.g. it
> > > > should be disabled when power source is a DC barrel jack instead
> > > > of USB) and is not supposed to change at all during runtime? Then
> > > > the information wether it needs to be enabled should be derived
> > > > from the device tree.
> > >
> > > It's a switching charger integrates OTG boost.
> > > For the case 'DC Jack', there's no need to use this kind of product.
> > >
> > > With typec integration, at most time, it still need bc12 to be enabled
> > > by default. Just in some case, like as power role swap (SNK -> SRC -> SNK),
> > > to automatically identify the USB port, this may interrupt USB communication.
> > >
> > > So as my understanding, keep it enabled by default, just in some case,
> > > it my need to control at runtime.
> >
> > This should be part of the description. You can drop the sentence
> > "Normally controlled when TypeC/USBPD port integrated.", since
> > that's hard to comprehend. Instead add the information that this
> > is supposed to be always enabled, but can be disabled to avoid
> > USB link loss (?) when doing a USB PD role swap.
> >
> Thanks for the comment.
>
> I'll rewrite it as below
> 'It's supposed to be always enabled, but can be disabled to avoid usb
> link interruption especially when doing a USBPD 'power' role swap.'
>
> Sorry, due to the long reply period, I already sent v6/v7/v8 to fix
> some coding like as missing header and irq wakeup check.
> Actually only missing header change is needed.
> Please ignore these noise.
>
> And after the sysfs reviewing is finished, you can just review the revision v9.

I'm preparing the final patch.
2 more weeks past, now only one question is left for rt9471 ABI document.
After my explanation, is 'to keep sysoff_enable" acceptible?
Or no more discussion, just delete it?

> Thanks.
> > -- Sebastian