Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document

[Ondřej Jirman]

On Mon, Apr 08, 2024 at 10:12:30PM GMT, Krzysztof Kozlowski wrote:
> On 08/04/2024 17:17, Ondřej Jirman wrote:
> > 
> > Now for things to not fail during suspend/resume based on PM callbacks
> > invocation order, anx7688 driver needs to enable this regulator too, as long
> > as it needs it.
> 
> No, the I2C bus driver needs to manage it. Not one individual I2C
> device. Again, why anx7688 is specific? If you next phone has anx8867,
> using different driver, you also add there i2c-supply? And if it is
> nxp,ptn5100 as well?

Yes, that could work, if I2C core would manage this.

> > 
> > I can put bus-supply to I2C controller node, and read it from the ANX7688 driver
> > I guess, by going up a DT node. Whether that's going to be acceptable, I don't
> > know. 
> > 
> > 
> > VCONN regulator I don't know where else to put either. It doesn't seem to belong
> > anywhere. It's not something directly connected to Type-C connector, so
> > not part of connector bindings, and there's nothing else I can see, other
> > than anx7688 device which needs it for core functionality.
> 
> That sounds like a GPIO, not regulator. anx7688 has GPIOs, right? On
> Pinephone they go to regulator, but on FooPhone also using anx7688 they
> go somewhere else, so why this anx7688 assumes this is a regulator?

CC1/CC2_VCONN control pins are "GPIO" of anx7688, sort of. They have fixed
purpose of switching external 5V regulator output to one of the CC pins
on type-c port. I don't care what other purpose with some other firmware
someone puts to those pins. It's irrelevant to the use case of anx7688
as a type-c controller/HDMI bridge, which we're describing here.

VCONN regulator is an actual GPIO controlled regulator on the board, and
needs to be controlled by the anx7688 driver. So that CC1/CC2_VCONN control
pins driven by the firmware actually do what they're supposed to do.

Not sure why it would be a business of anything else but anx7688 driver
enabling this regulator, because only this driver knows and cares about this.
If some other board doesn't have the need to manually enable the regulator, or
doesn't have the regulator, it can simply be optional.

There are also some other funky supplies in the bindings, that are not connected
to the chip in any way, but need to be controlled by the driver:

+  vbus-supply: true
+  vbus-in-supply: true

First one can be on the connector node instead, where the driver can fetch
it from.

The purpose of the second one is to link the Phone's PMIC's USB power input with
the type-c controller (anx7688), to make sure the PMIC has information about how
much power it can draw from external PSU.

The second one can be replaced by rewriting the anx7688 driver so that it
creates a power supply representing the USB PSU connected to the phone, and by
linking to anx7688 DT node from x-powers,axp813-usb-power-supply via
a power-supplies property, which would mean that USB input of the phone is
supplied by the external USB PSU. PMIC driver can be modified to watch
the power supply provided by anx7688 driver for information it detected
via USB-PD and update its input current limit via a standard helper function.

This is how eg. fusb302 works. Not sure if that's any better from the PoV of
DT bindings, though. Because power-supplies = <&anx7688>; will not look any
greater in DT bindings, IMO. It will just link the same nodes in the other
direction.

Anyway, the HW is that there's an external PSU (detected by type c controller)
and internal USB power input, and they are connected and one has to respect the
limits of the other. I guess I shouldn't be adding a device node for external PSU,
since it's not part of the phone. But that's what's trully being linked on
HW level.

Kind regards,
	o.

> 
> > 
> > ANX7688 chip desing doesn't have integrated VCONN mosfet switches so it always
> > needs external supply + switches that are controlled by the chip itself. There's
> > no sensible design where someone would not want this and the driver needs
> > to get this regulator reference from somewhere. The switches are sort of an
> > extension of the chip.
> 
> 
> Best regards,
> Krzysztof
>