Re: [PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
Hi!
> > From: Ondrej Jirman
> >
> > This is driver for ANX7688 USB-C HDMI, with flashing and debugging
> > features removed. ANX7688 is rather criticial piece on PinePhone,
> > there's no display and no battery charging without it.
>
> Don't remove the flashing part. Some Pinephones come without the firmware
> in the past. Even recently, I've seen some people in the Pine chat
> asking how to flash the firmware on some old PinePhone.
>
> It's a safe operation that can be done at any time and can only be done
> from the kernel driver.
I can re-add it later, but initial merge will be tricky enough as-is.
> > There's likely more work to be done here, but having basic support
> > in mainline is needed to be able to work on the other stuff
> > (networking, cameras, power management).
> >
> > Signed-off-by: Ondrej Jirman
>
> I should be second in order of sign-offs. Martijn wrote a non-working skeleton
> https://megous.com/git/linux/commit/?h=pp-5.7=30e33cefd7956a2b49fb27008b4af9d868974e58
> driver. Then I picked it up and developed it over years to a working thing.
> Almost none of the skeleton remains.
I believe From: should match First signed-off. I guess Martijn should
be marked as co-developed-by or something like that?
> License is GPLv2.
Ok.
> > + ret = of_property_read_variable_u32_array(dev->of_node, "sink-caps",
> > + anx7688->snk_caps,
> > + 1,
> > ARRAY_SIZE(anx7688->snk_caps));
> > + if (ret < 0) {
> > + dev_err(dev, "failed to get sink-caps from DT\n");
> > + return ret;
> > + }
>
> ^^^ The driver will need to follow usb-c-connector bindings and it will need
> a bindings documentation for itself.
>
> That's one of the missing things that I did not implement, yet.
Yep, I fought with device trees for few days. I should have yaml
ready.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
signature.asc
Description: PGP signature
Re: [PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
Hi Pavel,
On Thu, Feb 01, 2024 at 11:41:56AM +0100, Pavel Machek wrote:
> From: Ondrej Jirman
>
> This is driver for ANX7688 USB-C HDMI, with flashing and debugging
> features removed. ANX7688 is rather criticial piece on PinePhone,
> there's no display and no battery charging without it.
Don't remove the flashing part. Some Pinephones come without the firmware
in the past. Even recently, I've seen some people in the Pine chat
asking how to flash the firmware on some old PinePhone.
It's a safe operation that can be done at any time and can only be done
from the kernel driver.
> There's likely more work to be done here, but having basic support
> in mainline is needed to be able to work on the other stuff
> (networking, cameras, power management).
>
> Signed-off-by: Ondrej Jirman
I should be second in order of sign-offs. Martijn wrote a non-working skeleton
https://megous.com/git/linux/commit/?h=pp-5.7=30e33cefd7956a2b49fb27008b4af9d868974e58
driver. Then I picked it up and developed it over years to a working thing.
Almost none of the skeleton remains.
License is GPLv2.
> Signed-off-by: Martijn Braam
> Signed-off-by: Samuel Holland
> Signed-off-by: Pavel Machek
>
> [...]
>
> +static int anx7688_i2c_probe(struct i2c_client *client)
> +{
> +struct anx7688 *anx7688;
> +struct device *dev = >dev;
> +struct typec_capability typec_cap = { };
> +int i, vid_h, vid_l;
> +int irq_cabledet;
> +int ret = 0;
> +
> +anx7688 = devm_kzalloc(dev, sizeof(*anx7688), GFP_KERNEL);
> +if (!anx7688)
> +return -ENOMEM;
> +
> +i2c_set_clientdata(client, anx7688);
> +anx7688->client = client;
> +anx7688->dev = >dev;
> +mutex_init(>lock);
> +INIT_DELAYED_WORK(>work, anx7688_work);
> + anx7688->last_extcon_state = -1;
> +
> + ret = of_property_read_variable_u32_array(dev->of_node, "source-caps",
> + anx7688->src_caps,
> + 1,
> ARRAY_SIZE(anx7688->src_caps));
> + if (ret < 0) {
> + dev_err(dev, "failed to get source-caps from DT\n");
> + return ret;
> + }
> + anx7688->n_src_caps = ret;
> +
> + ret = of_property_read_variable_u32_array(dev->of_node, "sink-caps",
> + anx7688->snk_caps,
> + 1,
> ARRAY_SIZE(anx7688->snk_caps));
> + if (ret < 0) {
> + dev_err(dev, "failed to get sink-caps from DT\n");
> + return ret;
> + }
^^^ The driver will need to follow usb-c-connector bindings and it will need
a bindings documentation for itself.
That's one of the missing things that I did not implement, yet.
Kind regards,
o.
Re: [PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
Hi! > > --- /dev/null > > +++ b/drivers/usb/typec/anx7688.c > > @@ -0,0 +1,1866 @@ > > +/* > > + * ANX7688 USB-C HDMI bridge/PD driver > > + * > > > > Did this pass checkpatch? I need a spdx line for new files please, > don't force us to go back and guess in the future, that isn't nice. Thanks for the review. Fixing checkpatch issues is easy, but it looks like closer look will be needed at the devicetree probing. Also, what are the authors preferences about the license? I'd prefer GPLv2+. Best regards, Pavel
Re: [PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
On Thu, Feb 01, 2024 at 11:41:56AM +0100, Pavel Machek wrote:
> +#define DEBUG
Please don't. This is dynamic, use the dynamic debugging and set it
from userspace if you want to debug the driver.
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* firmware regs */
> +
> +#define ANX7688_REG_VBUS_OFF_DELAY_TIME 0x22
> +#define ANX7688_REG_FEATURE_CTRL0x27
> +#define ANX7688_REG_EEPROM_LOAD_STATUS1 0x11
> +#define ANX7688_REG_EEPROM_LOAD_STATUS0 0x12
> +#define ANX7688_REG_FW_VERSION1 0x15
> +#define ANX7688_REG_FW_VERSION0 0x16
> +
> +#define ANX7688_EEPROM_FW_LOADED 0x01
Mix of tabs and spaces, please just use tabs.
> +static const char * const anx7688_supply_names[] = {
> +"avdd33",
> +"avdd18",
> +"dvdd18",
> +"avdd10",
> +"dvdd10",
> + "i2c",
> +"hdmi_vt",
> +
> +"vconn", // power for VCONN1/VCONN2 switches
> +"vbus", // vbus power
> +};
Again, tabs vs. spaces, please use checkpatch.
> +#define ANX7688_NUM_SUPPLIES ARRAY_SIZE(anx7688_supply_names)
> +#define ANX7688_NUM_ALWAYS_ON_SUPPLIES (ANX7688_NUM_SUPPLIES - 1)
> +
> +#define ANX7688_I2C_INDEX (ANX7688_NUM_SUPPLIES - 4)
> +#define ANX7688_VCONN_INDEX (ANX7688_NUM_SUPPLIES - 2)
> +#define ANX7688_VBUS_INDEX (ANX7688_NUM_SUPPLIES - 1)
> +
> +enum {
> + ANX7688_F_POWERED,
> + ANX7688_F_CONNECTED,
> + ANX7688_F_FW_FAILED,
> + ANX7688_F_PWRSUPPLY_CHANGE,
> + ANX7688_F_CURRENT_UPDATE,
> +};
> +
> +struct anx7688 {
> +struct device *dev;
> +struct i2c_client *client;
> +struct i2c_client *client_tcpc;
> +struct regulator_bulk_data supplies[ANX7688_NUM_SUPPLIES];
> + struct power_supply *vbus_in_supply;
> + struct notifier_block vbus_in_nb;
> + int input_current_limit; // mA
> +struct gpio_desc *gpio_enable;
> +struct gpio_desc *gpio_reset;
> +struct gpio_desc *gpio_cabledet;
I'm stopping here, again, tabs, you know this :(
greg k-h
Re: [PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
On Thu, Feb 01, 2024 at 11:41:56AM +0100, Pavel Machek wrote: > --- /dev/null > +++ b/drivers/usb/typec/anx7688.c > @@ -0,0 +1,1866 @@ > +/* > + * ANX7688 USB-C HDMI bridge/PD driver > + * Did this pass checkpatch? I need a spdx line for new files please, don't force us to go back and guess in the future, that isn't nice. thanks, greg k-h
[PATCH] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge
From: Ondrej Jirman This is driver for ANX7688 USB-C HDMI, with flashing and debugging features removed. ANX7688 is rather criticial piece on PinePhone, there's no display and no battery charging without it. There's likely more work to be done here, but having basic support in mainline is needed to be able to work on the other stuff (networking, cameras, power management). Signed-off-by: Ondrej Jirman Signed-off-by: Martijn Braam Signed-off-by: Samuel Holland Signed-off-by: Pavel Machek diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig index 2f80c2792dbd..982b7c444a1f 100644 --- a/drivers/usb/typec/Kconfig +++ b/drivers/usb/typec/Kconfig @@ -64,6 +65,17 @@ config TYPEC_ANX7411 If you choose to build this driver as a dynamically linked module, the module will be called anx7411.ko. +config TYPEC_ANX7688 + tristate "Analogix ANX7688 Type-C DRP Port controller and mux driver" + depends on I2C + depends on USB_ROLE_SWITCH + help + Say Y or M here if your system has Analogix ANX7688 Type-C Bridge + controller driver. + + If you choose to build this driver as a dynamically linked module, the + module will be called anx7688.ko. + config TYPEC_RT1719 tristate "Richtek RT1719 Sink Only Type-C controller driver" depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile index 7a368fea61bc..3f8ff94ad294 100644 --- a/drivers/usb/typec/Makefile +++ b/drivers/usb/typec/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_TYPEC_TCPM)+= tcpm/ obj-$(CONFIG_TYPEC_UCSI) += ucsi/ obj-$(CONFIG_TYPEC_TPS6598X) += tipd/ obj-$(CONFIG_TYPEC_ANX7411)+= anx7411.o +obj-$(CONFIG_TYPEC_ANX7688)+= anx7688.o obj-$(CONFIG_TYPEC_HD3SS3220) += hd3ss3220.o obj-$(CONFIG_TYPEC_STUSB160X) += stusb160x.o obj-$(CONFIG_TYPEC_RT1719) += rt1719.o diff --git a/drivers/usb/typec/anx7688.c b/drivers/usb/typec/anx7688.c new file mode 100644 index ..9b693c9fd085 --- /dev/null +++ b/drivers/usb/typec/anx7688.c @@ -0,0 +1,1866 @@ +/* + * ANX7688 USB-C HDMI bridge/PD driver + * + * Warning, this driver is somewhat PinePhone specific. + * + * How this works: + * - this driver allows to program firmware into ANX7688 EEPROM, and + * initialize it + * - it then communicates with the firmware running on the OCM (on-chip + * microcontroller) + * - it detects whether there is cable plugged in or not and powers + * up or down the ANX7688 based on that + * - when the cable is connected the firmware on the OCM will handle + * the detection of the nature of the device on the other end + * of the USB-C cable + * - this driver then communicates with the USB phy to let it swap + * data roles accordingly + * - it also enables VBUS and VCONN regulators as appropriate + * - USB phy driver (Allwinner) needs to know whether to switch to + * device or host mode, or whether to turn off + * - when the firmware detects SRC.1.5A or SRC.3.0A via CC pins + * or something else via PD, it notifies this driver via software + * interrupt and this driver will determine how to update the TypeC + * port status and what input current limit is appropriate + * - input current limit determination happens 500ms after cable + * insertion or hard reset (delay is necessary to determine whether + * the remote end is PD capable or not) + * - this driver tells to the PMIC driver that the input current limit + * needs to be changed + * - this driver also monitors PMIC status and re-sets the input current + * limit if it changes for some reason (due to PMIC internal decision + * making) (this is disabled for now) + * + * ANX7688 FW behavior as observed: + * + * - DO NOT SET MORE THAN 1 SINK CAPABILITY! Firmware will ignore what + * you set and send hardcoded PDO_BATT 5-21V 30W message! + * + * Product brief: + * https://www.analogix.com/en/system/files/AA-002281-PB-6-ANX7688_Product_Brief_0.pdf + * Development notes: + * https://xnux.eu/devices/feature/anx7688.html + */ + +#define DEBUG + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* firmware regs */ + +#define ANX7688_REG_VBUS_OFF_DELAY_TIME 0x22 +#define ANX7688_REG_FEATURE_CTRL0x27 +#define ANX7688_REG_EEPROM_LOAD_STATUS1 0x11 +#define ANX7688_REG_EEPROM_LOAD_STATUS0 0x12 +#define ANX7688_REG_FW_VERSION1 0x15 +#define ANX7688_REG_FW_VERSION0 0x16 + +#define ANX7688_EEPROM_FW_LOADED 0x01 + +#define ANX7688_REG_STATUS_INT_MASK 0x17 +#define ANX7688_REG_STATUS_INT 0x28 +#define ANX7688_IRQS_RECEIVED_MSG BIT(0) +#define ANX7688_IRQS_RECEIVED_ACK BIT(1) +#define ANX7688_IRQS_VCONN_CHANGE BIT(2) +#define ANX7688_IRQS_VBUS_CHANGEBIT(3) +#define ANX7688_IRQS_CC_STATUS_CHANGE BIT(4) +#define
