* [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document @ 2024-04-08 10:51 Pavel Machek 2024-04-08 10:54 ` [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge Pavel Machek 2024-04-08 11:17 ` [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Krzysztof Kozlowski 0 siblings, 2 replies; 22+ messages in thread From: Pavel Machek @ 2024-04-08 10:51 UTC (permalink / raw) To: phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, megi [-- Attachment #1: Type: text/plain, Size: 4013 bytes --] Add binding for anx7688 usb type-c bridge. I don't have a datasheet, but I did best I could. Signed-off-by: Pavel Machek <pavel@ucw.cz> --- v2: implement review feedback v3: fix single character pointed by robot diff --git a/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml new file mode 100644 index 000000000000..48b9ae936cb5 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/analogix,anx7688.yaml @@ -0,0 +1,127 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/analogix,anx7688.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +# Pin names can be deduced from +# https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2b%20Released%20Schematic.pdf + +title: Analogix ANX7688 Type-C controller + +maintainers: + - Pavel Machek <pavel@ucw.cz> + +properties: + compatible: + enum: + - analogix,anx7688 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + description: GPIO controlling RESET_N (B7) pin. + + enable-gpios: + maxItems: 1 + description: GPIO controlling POWER_EN (D2) pin. + + cabledet-gpios: + maxItems: 1 + description: GPIO controlling CABLE_DET (C3) pin. + + avdd10-supply: + description: 1.0V power supply going to AVDD10 (A4, ...) pins + + dvdd10-supply: + description: 1.0V power supply going to DVDD10 (D6, ...) pins + + avdd18-supply: + description: 1.8V power supply going to AVDD18 (E3, ...) pins + + dvdd18-supply: + description: 1.8V power supply going to DVDD18 (G4, ...) pins + + avdd33-supply: + description: 3.3V power supply going to AVDD33 (C4, ...) pins + + i2c-supply: true + vconn-supply: true + hdmi-vt-supply: true + vbus-supply: true + vbus-in-supply: true + + connector: + type: object + $ref: /schemas/connector/usb-connector.yaml + + description: + Properties for usb c connector. + + properties: + compatible: + const: usb-c-connector + +required: + - compatible + - reg + - connector + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + typec@2c { + compatible = "analogix,anx7688"; + reg = <0x2c>; + interrupts = <8 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio0>; + + enable-gpios = <&pio 3 10 GPIO_ACTIVE_LOW>; /* PD10 */ + reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */ + cabledet-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */ + + avdd10-supply = <®_anx1v0>; + dvdd10-supply = <®_anx1v0>; + avdd18-supply = <®_ldo_io1>; + dvdd18-supply = <®_ldo_io1>; + avdd33-supply = <®_dcdc1>; + i2c-supply = <®_ldo_io0>; + vconn-supply = <®_vconn5v0>; + hdmi-vt-supply = <®_dldo1>; + + vbus-supply = <®_usb_5v>; + vbus-in-supply = <&usb_power_supply>; + + typec_con: connector { + compatible = "usb-c-connector"; + power-role = "dual"; + data-role = "dual"; + try-power-role = "source"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + typec_con_ep: endpoint { + remote-endpoint = <&usbotg_hs_ep>; + }; + }; + }; + }; + }; + }; +... -- People of Russia, stop Putin before his war on Ukraine escalates. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-08 10:51 [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Pavel Machek @ 2024-04-08 10:54 ` Pavel Machek 2024-04-09 9:56 ` Heikki Krogerus 2024-04-08 11:17 ` [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Pavel Machek @ 2024-04-08 10:54 UTC (permalink / raw) To: phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, megi [-- Attachment #1: Type: text/plain, Size: 55631 bytes --] From: Ondrej Jirman <megi@xff.cz> 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 <megi@xff.cz> Co-developed-by: Martijn Braam <martijn@brixit.nl> Co-developed-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Pavel Machek <pavel@ucw.cz> --- v2: Fix checkpatch stuff. Some cleanups, adapt to dts format in 1/2. v3: Turn down debugging, as requested during review. diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig index 2f80c2792dbd..c9043ae61546 100644 --- a/drivers/usb/typec/Kconfig +++ b/drivers/usb/typec/Kconfig @@ -64,6 +64,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 000000000000..407cbd5fedba --- /dev/null +++ b/drivers/usb/typec/anx7688.c @@ -0,0 +1,1830 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * 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 + */ + +#include <linux/debugfs.h> +#include <linux/delay.h> +#include <linux/extcon-provider.h> +#include <linux/firmware.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/power_supply.h> +#include <linux/regulator/consumer.h> +#include <linux/usb/pd.h> +#include <linux/usb/role.h> +#include <linux/usb/typec.h> + +/* firmware regs */ + +#define ANX7688_REG_VBUS_OFF_DELAY_TIME 0x22 +#define ANX7688_REG_FEATURE_CTRL 0x27 +#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_CHANGE BIT(3) +#define ANX7688_IRQS_CC_STATUS_CHANGE BIT(4) +#define ANX7688_IRQS_DATA_ROLE_CHANGE BIT(5) + +#define ANX7688_REG_STATUS 0x29 +#define ANX7688_VCONN_STATUS BIT(2) /* 0 = off 1 = on */ +#define ANX7688_VBUS_STATUS BIT(3) /* 0 = off 1 = on */ +#define ANX7688_DATA_ROLE_STATUS BIT(5) /* 0 = device 1 = host */ + +#define ANX7688_REG_CC_STATUS 0x2a +#define ANX7688_REG_TRY_UFP_TIMER 0x23 +#define ANX7688_REG_TIME_CTRL 0x24 + +#define ANX7688_REG_MAX_VOLTAGE 0x1b +#define ANX7688_REG_MAX_POWER 0x1c +#define ANX7688_REG_MIN_POWER 0x1d +#define ANX7688_REG_MAX_VOLTAGE_STATUS 0x1e +#define ANX7688_REG_MAX_POWER_STATUS 0x1f + +#define ANX7688_SOFT_INT_MASK 0x7f + +/* tcpc regs */ + +#define ANX7688_TCPC_REG_VENDOR_ID0 0x00 +#define ANX7688_TCPC_REG_VENDOR_ID1 0x01 +#define ANX7688_TCPC_REG_ALERT0 0x10 +#define ANX7688_TCPC_REG_ALERT1 0x11 +#define ANX7688_TCPC_REG_ALERT_MASK0 0x12 +#define ANX7688_TCPC_REG_ALERT_MASK1 0x13 +#define ANX7688_TCPC_REG_INTERFACE_SEND 0x30 +#define ANX7688_TCPC_REG_INTERFACE_RECV 0x51 + +/* hw regs */ + +#define ANX7688_REG_IRQ_EXT_SOURCE0 0x3e +#define ANX7688_REG_IRQ_EXT_SOURCE1 0x4e +#define ANX7688_REG_IRQ_EXT_SOURCE2 0x4f +#define ANX7688_REG_IRQ_EXT_MASK0 0x3b +#define ANX7688_REG_IRQ_EXT_MASK1 0x3c +#define ANX7688_REG_IRQ_EXT_MASK2 0x3d +#define ANX7688_REG_IRQ_SOURCE0 0x54 +#define ANX7688_REG_IRQ_SOURCE1 0x55 +#define ANX7688_REG_IRQ_SOURCE2 0x56 +#define ANX7688_REG_IRQ_MASK0 0x57 +#define ANX7688_REG_IRQ_MASK1 0x58 +#define ANX7688_REG_IRQ_MASK2 0x59 + +#define ANX7688_IRQ2_SOFT_INT BIT(2) + +#define ANX7688_REG_USBC_RESET_CTRL 0x05 +#define ANX7688_USBC_RESET_CTRL_OCM_RESET BIT(4) + +/* ocm messages */ + +#define ANX7688_OCM_MSG_PWR_SRC_CAP 0x00 +#define ANX7688_OCM_MSG_PWR_SNK_CAP 0x01 +#define ANX7688_OCM_MSG_DP_SNK_IDENTITY 0x02 +#define ANX7688_OCM_MSG_SVID 0x03 +#define ANX7688_OCM_MSG_GET_DP_SNK_CAP 0x04 +#define ANX7688_OCM_MSG_ACCEPT 0x05 +#define ANX7688_OCM_MSG_REJECT 0x06 +#define ANX7688_OCM_MSG_PSWAP_REQ 0x10 +#define ANX7688_OCM_MSG_DSWAP_REQ 0x11 +#define ANX7688_OCM_MSG_GOTO_MIN_REQ 0x12 +#define ANX7688_OCM_MSG_VCONN_SWAP_REQ 0x13 +#define ANX7688_OCM_MSG_VDM 0x14 +#define ANX7688_OCM_MSG_DP_SNK_CFG 0x15 +#define ANX7688_OCM_MSG_PWR_OBJ_REQ 0x16 +#define ANX7688_OCM_MSG_PD_STATUS_REQ 0x17 +#define ANX7688_OCM_MSG_DP_ALT_ENTER 0x19 +#define ANX7688_OCM_MSG_DP_ALT_EXIT 0x1a +#define ANX7688_OCM_MSG_GET_SNK_CAP 0x1b +#define ANX7688_OCM_MSG_RESPONSE_TO_REQ 0xf0 +#define ANX7688_OCM_MSG_SOFT_RST 0xf1 +#define ANX7688_OCM_MSG_HARD_RST 0xf2 +#define ANX7688_OCM_MSG_RESTART 0xf3 + +static const char * const anx7688_supply_names[] = { + "avdd33", + "avdd18", + "dvdd18", + "avdd10", + "dvdd10", + "i2c", + "hdmi-vt", + + "vconn", // power for VCONN1/VCONN2 switches + "vbus", // vbus power +}; + +#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; + + u32 src_caps[8]; + unsigned int n_src_caps; + + u32 snk_caps[8]; + unsigned int n_snk_caps; + + unsigned long flags[1]; + + struct delayed_work work; + struct timer_list work_timer; + + struct mutex lock; + bool vbus_on, vconn_on; + bool pd_capable; + int pd_current_limit; // mA + ktime_t current_update_deadline; + + struct typec_port *port; + struct typec_partner *partner; + struct usb_pd_identity partner_identity; + struct usb_role_switch *role_sw; + int pwr_role, data_role; + + struct dentry *debug_root; + + /* for debug */ + int last_status; + int last_cc_status; + int last_dp_state; + int last_bc_result; + + /* for HDMI HPD */ + struct extcon_dev *extcon; + int last_extcon_state; +}; + +static const unsigned int anx7688_extcon_cable[] = { + EXTCON_DISP_HDMI, + EXTCON_NONE, +}; + +static int anx7688_reg_read(struct anx7688 *anx7688, u8 reg_addr) +{ + int ret; + + ret = i2c_smbus_read_byte_data(anx7688->client, reg_addr); + if (ret < 0) + dev_err(anx7688->dev, "i2c read failed at 0x%x (%d)\n", + reg_addr, ret); + + return ret; +} + +static int anx7688_reg_write(struct anx7688 *anx7688, u8 reg_addr, u8 value) +{ + int ret; + + ret = i2c_smbus_write_byte_data(anx7688->client, reg_addr, value); + if (ret < 0) + dev_err(anx7688->dev, "i2c write failed at 0x%x (%d)\n", + reg_addr, ret); + + return ret; +} + +static int anx7688_reg_update_bits(struct anx7688 *anx7688, u8 reg_addr, + u8 mask, u8 value) +{ + int ret; + + ret = anx7688_reg_read(anx7688, reg_addr); + if (ret < 0) + return ret; + + ret &= ~mask; + ret |= value; + + return anx7688_reg_write(anx7688, reg_addr, ret); +} + +static int anx7688_tcpc_reg_read(struct anx7688 *anx7688, u8 reg_addr) +{ + int ret; + + ret = i2c_smbus_read_byte_data(anx7688->client_tcpc, reg_addr); + if (ret < 0) + dev_err(anx7688->dev, "tcpc i2c read failed at 0x%x (%d)\n", + reg_addr, ret); + + return ret; +} + +static int anx7688_tcpc_reg_write(struct anx7688 *anx7688, u8 reg_addr, u8 value) +{ + int ret; + + ret = i2c_smbus_write_byte_data(anx7688->client_tcpc, reg_addr, value); + if (ret < 0) + dev_err(anx7688->dev, "tcpc i2c write failed at 0x%x (%d)\n", + reg_addr, ret); + + return ret; +} + +static void anx7688_power_enable(struct anx7688 *anx7688) +{ + gpiod_set_value(anx7688->gpio_reset, 1); + gpiod_set_value(anx7688->gpio_enable, 1); + + /* wait for power to stabilize and release reset */ + msleep(10); + gpiod_set_value(anx7688->gpio_reset, 0); + usleep_range(2, 4); + + dev_dbg(anx7688->dev, "power enabled\n"); + + set_bit(ANX7688_F_POWERED, anx7688->flags); +} + +static void anx7688_power_disable(struct anx7688 *anx7688) +{ + gpiod_set_value(anx7688->gpio_reset, 1); + msleep(5); + gpiod_set_value(anx7688->gpio_enable, 0); + + dev_dbg(anx7688->dev, "power disabled\n"); + + clear_bit(ANX7688_F_POWERED, anx7688->flags); +} + +static int anx7688_send_ocm_message(struct anx7688 *anx7688, int cmd, + const u8 *data, int data_len) +{ + int ret = 0, i; + u8 pkt[32]; + + if (data_len > sizeof(pkt) - 3) { + dev_dbg(anx7688->dev, + "invalid ocm message length cmd=0x%02x len=%d\n", + cmd, data_len); + return -EINVAL; + } + + // prepare pd packet + pkt[0] = data_len + 1; + pkt[1] = cmd; + if (data_len > 0) + memcpy(pkt + 2, data, data_len); + pkt[2 + data_len] = 0; + for (i = 0; i < data_len + 2; i++) + pkt[data_len + 2] -= pkt[i]; + + dev_dbg(anx7688->dev, "send pd packet cmd=0x%02x %*ph\n", + cmd, data_len + 3, pkt); + + ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND); + if (ret) { + dev_err(anx7688->dev, "failed to send pd packet (tx buffer full)\n"); + return -EBUSY; + } + + ret = i2c_smbus_write_i2c_block_data(anx7688->client_tcpc, + ANX7688_TCPC_REG_INTERFACE_SEND, + data_len + 3, pkt); + if (ret < 0) + dev_err(anx7688->dev, "failed to send pd packet (err=%d)\n", ret); + + // wait until the message is processed (30ms max) + for (i = 0; i < 300; i++) { + ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND); + if (ret <= 0) + return ret; + + udelay(100); + } + + dev_err(anx7688->dev, "timeout waiting for the message queue flush\n"); + return -ETIMEDOUT; +} + +static int anx7688_connect(struct anx7688 *anx7688) +{ + struct typec_partner_desc desc = {}; + int ret, i; + u8 fw[2]; + const u8 dp_snk_identity[16] = { + 0x00, 0x00, 0x00, 0xec, /* id header */ + 0x00, 0x00, 0x00, 0x00, /* cert stat */ + 0x00, 0x00, 0x00, 0x00, /* product type */ + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ + }; + const u8 svid[4] = { + 0x00, 0x00, 0x01, 0xff, + }; + u32 caps[8]; + + dev_dbg(anx7688->dev, "cable inserted\n"); + + anx7688->last_status = -1; + anx7688->last_cc_status = -1; + anx7688->last_dp_state = -1; + + msleep(10); + anx7688_power_enable(anx7688); + + ret = regulator_enable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); + if (ret) { + dev_err(anx7688->dev, "failed to enable vconn\n"); + goto err_poweroff; + } + anx7688->vconn_on = true; + + /* wait till the firmware is loaded (typically ~30ms) */ + i = 0; + while (1) { + ret = anx7688_reg_read(anx7688, ANX7688_REG_EEPROM_LOAD_STATUS0); + + if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) == ANX7688_EEPROM_FW_LOADED) { + dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret); + dev_dbg(anx7688->dev, "fw loaded after %d ms\n", i * 10); + break; + } + + if (i > 99) { + set_bit(ANX7688_F_FW_FAILED, anx7688->flags); + dev_err(anx7688->dev, + "boot firmware load failed (you may need to flash FW to anx7688 first)\n"); + ret = -ETIMEDOUT; + goto err_vconoff; + } + msleep(5); + i++; + } + + ret = i2c_smbus_read_i2c_block_data(anx7688->client, + ANX7688_REG_FW_VERSION1, 2, fw); + if (ret < 0) { + dev_err(anx7688->dev, "failed to read firmware version\n"); + goto err_vconoff; + } + + dev_dbg(anx7688->dev, "OCM firmware loaded (version 0x%04x)\n", + fw[1] | fw[0] << 8); + + /* Unmask interrupts */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); + if (ret) + goto err_vconoff; + + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT_MASK, ~ANX7688_SOFT_INT_MASK); + if (ret) + goto err_vconoff; + + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, 0xff); + if (ret) + goto err_vconoff; + + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_MASK2, (u8)~ANX7688_IRQ2_SOFT_INT); + if (ret) + goto err_vconoff; + + /* time to turn off vbus after cc disconnect (unit is 4 ms) */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_VBUS_OFF_DELAY_TIME, 100 / 4); + if (ret) + goto err_vconoff; + + /* 300ms (unit is 2 ms) */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_TRY_UFP_TIMER, 300 / 2); + if (ret) + goto err_vconoff; + + /* maximum voltage in 100 mV units */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_VOLTAGE, 50); /* 5 V */ + if (ret) + goto err_vconoff; + + /* min/max power in 500 mW units */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_POWER, 15 * 2); /* 15 W */ + if (ret) + goto err_vconoff; + + ret = anx7688_reg_write(anx7688, ANX7688_REG_MIN_POWER, 1); /* 0.5 W */ + if (ret) + goto err_vconoff; + + /* auto_pd, try.src, try.sink, goto safe 5V */ + ret = anx7688_reg_write(anx7688, ANX7688_REG_FEATURE_CTRL, 0x1e & ~BIT(2)); // disable try_src + if (ret) + goto err_vconoff; + + for (i = 0; i < anx7688->n_src_caps; i++) + caps[i] = cpu_to_le32(anx7688->src_caps[i]); + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PWR_SRC_CAP, + (u8 *)&caps, 4 * anx7688->n_src_caps); + if (ret) + goto err_vconoff; + + for (i = 0; i < anx7688->n_snk_caps; i++) + caps[i] = cpu_to_le32(anx7688->snk_caps[i]); + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PWR_SNK_CAP, + (u8 *)&caps, 4 * anx7688->n_snk_caps); + if (ret) + goto err_vconoff; + + /* Send DP SNK identity */ + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_DP_SNK_IDENTITY, + dp_snk_identity, sizeof(dp_snk_identity)); + if (ret) + goto err_vconoff; + + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_SVID, + svid, sizeof(svid)); + if (ret) + goto err_vconoff; + + dev_dbg(anx7688->dev, "OCM configuration completed\n"); + + desc.accessory = TYPEC_ACCESSORY_NONE; + + typec_unregister_partner(anx7688->partner); + + anx7688->partner = typec_register_partner(anx7688->port, &desc); + if (IS_ERR(anx7688->partner)) { + ret = PTR_ERR(anx7688->partner); + goto err_vconoff; + } + + // after this deadline passes we'll check if device is pd_capable and + // set up the current limit accordingly + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 3000); + + set_bit(ANX7688_F_CONNECTED, anx7688->flags); + return 0; + +err_vconoff: + regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); + anx7688->vconn_on = false; +err_poweroff: + anx7688_power_disable(anx7688); + dev_err(anx7688->dev, "OCM configuration failed\n"); + return ret; +} + +static void anx7688_set_hdmi_hpd(struct anx7688 *anx7688, int state) +{ + if (!anx7688->extcon) + return; + + if (anx7688->last_extcon_state != state) { + extcon_set_state_sync(anx7688->extcon, EXTCON_DISP_HDMI, state); + anx7688->last_extcon_state = state; + } +} + +static void anx7688_disconnect(struct anx7688 *anx7688) +{ + union power_supply_propval val = {0,}; + struct device *dev = anx7688->dev; + int ret; + + dev_dbg(dev, "cable removed\n"); + + anx7688->current_update_deadline = 0; + + anx7688_set_hdmi_hpd(anx7688, 0); + + if (anx7688->vconn_on) { + regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); + anx7688->vconn_on = false; + } + + if (anx7688->vbus_on) { + regulator_disable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); + anx7688->vbus_on = false; + } + + anx7688_power_disable(anx7688); + + anx7688->pd_capable = false; + + typec_unregister_partner(anx7688->partner); + anx7688->partner = NULL; + + anx7688->pwr_role = TYPEC_SINK; + anx7688->data_role = TYPEC_DEVICE; + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); + typec_set_data_role(anx7688->port, anx7688->data_role); + typec_set_pwr_opmode(anx7688->port, TYPEC_PWR_MODE_USB); + typec_set_vconn_role(anx7688->port, TYPEC_SINK); + + usb_role_switch_set_role(anx7688->role_sw, USB_ROLE_NONE); + + val.intval = 500 * 1000; + dev_dbg(dev, "setting vbus_in current limit to %d mA\n", val.intval); + ret = power_supply_set_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, + &val); + if (ret) + dev_err(dev, "failed to set vbus_in current to %d mA\n", + val.intval / 1000); + + val.intval = 0; + dev_dbg(dev, "disabling vbus_in power path\n"); + ret = power_supply_set_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_ONLINE, + &val); + if (ret) + dev_err(dev, "failed to offline vbus_in\n"); + + dev_dbg(dev, "enabling USB BC 1.2 detection\n"); + + clear_bit(ANX7688_F_CONNECTED, anx7688->flags); +} + +static void anx7688_handle_cable_change(struct anx7688 *anx7688) +{ + int cabledet; + bool connected; + + connected = test_bit(ANX7688_F_CONNECTED, anx7688->flags); + cabledet = gpiod_get_value(anx7688->gpio_cabledet); + + if (cabledet && !connected) + anx7688_connect(anx7688); + else if (!cabledet && connected) + anx7688_disconnect(anx7688); +} + +static irqreturn_t anx7688_irq_plug_handler(int irq, void *data) +{ + struct anx7688 *anx7688 = data; + + dev_dbg(anx7688->dev, "plug irq (cd=%d)\n", + gpiod_get_value(anx7688->gpio_cabledet)); + + /* + * After each cabledet change the scheduled work timer is reset + * to fire in ~10ms. So the work is done only after the cabledet + * is stable for ~10ms. + */ + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(10)); + + return IRQ_HANDLED; +} + +enum { + CMD_SUCCESS, + CMD_REJECT, + CMD_FAIL, + CMD_BUSY, +}; + +static const char * const anx7688_cmd_statuses[] = { + "SUCCESS", + "REJECT", + "FAIL", + "BUSY", +}; + + +static int anx7688_handle_pd_message(struct anx7688 *anx7688, + u8 cmd, u8 *msg, unsigned int len) +{ + union power_supply_propval psy_val = {0,}; + u32 *pdos = (u32 *)msg; + int ret, i, rdo_max_v, rdo_max_p; + u32 pdo, rdo; + + switch (cmd) { + case ANX7688_OCM_MSG_PWR_SRC_CAP: + dev_dbg(anx7688->dev, "received SRC_CAP\n"); + + if (len % 4 != 0) { + dev_warn(anx7688->dev, "received invalid sized PDO array\n"); + break; + } + + /* the partner is PD capable */ + anx7688->pd_capable = true; + + for (i = 0; i < len / 4; i++) { + pdo = le32_to_cpu(pdos[i]); + + if (pdo_type(pdo) == PDO_TYPE_FIXED) { + unsigned int voltage = pdo_fixed_voltage(pdo); + unsigned int max_curr = pdo_max_current(pdo); + + dev_dbg(anx7688->dev, "SRC_CAP PDO_FIXED (%umV %umA)\n", + voltage, max_curr); + } else if (pdo_type(pdo) == PDO_TYPE_BATT) { + unsigned int min_volt = pdo_min_voltage(pdo); + unsigned int max_volt = pdo_max_voltage(pdo); + unsigned int max_pow = pdo_max_power(pdo); + + dev_dbg(anx7688->dev, "SRC_CAP PDO_BATT (%umV-%umV %umW)\n", + min_volt, max_volt, max_pow); + } else if (pdo_type(pdo) == PDO_TYPE_VAR) { + unsigned int min_volt = pdo_min_voltage(pdo); + unsigned int max_volt = pdo_max_voltage(pdo); + unsigned int max_curr = pdo_max_current(pdo); + + dev_dbg(anx7688->dev, "SRC_CAP PDO_VAR (%umV-%umV %umA)\n", + min_volt, max_volt, max_curr); + } else { + dev_dbg(anx7688->dev, "SRC_CAP PDO_APDO (0x%08X)\n", pdo); + } + } + + /* when auto_pd mode is enabled, the FW has already set + * RDO_MAX_VOLTAGE and RDO_MAX_POWER for the RDO it sent to the + * partner based on the received SOURCE_CAPs. This does not + * mean, the request was acked, but we can't do better here than + * calculate the current_limit to set later and hope for the best. + */ + rdo_max_v = anx7688_reg_read(anx7688, ANX7688_REG_MAX_VOLTAGE_STATUS); + if (rdo_max_v < 0) + return rdo_max_v; + if (rdo_max_v == 0) + return -EINVAL; + + rdo_max_p = anx7688_reg_read(anx7688, ANX7688_REG_MAX_POWER_STATUS); + if (rdo_max_p < 0) + return rdo_max_p; + + anx7688->pd_current_limit = rdo_max_p * 5000 / rdo_max_v; + + dev_dbg(anx7688->dev, "RDO max voltage = %dmV, max power = %dmW, PD current limit = %dmA\n", + rdo_max_v * 100, rdo_max_p * 500, anx7688->pd_current_limit); + + // update current limit sooner, now that we have PD negotiation result + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 500); + break; + + case ANX7688_OCM_MSG_PWR_SNK_CAP: + dev_dbg(anx7688->dev, "received SNK_CAP\n"); + + if (len % 4 != 0) { + dev_warn(anx7688->dev, "received invalid sized PDO array\n"); + break; + } + + for (i = 0; i < len / 4; i++) { + pdo = le32_to_cpu(pdos[i]); + + if (pdo_type(pdo) == PDO_TYPE_FIXED) { + unsigned int voltage = pdo_fixed_voltage(pdo); + unsigned int max_curr = pdo_max_current(pdo); + + dev_dbg(anx7688->dev, "SNK_CAP PDO_FIXED (%umV %umA)\n", + voltage, max_curr); + } else if (pdo_type(pdo) == PDO_TYPE_BATT) { + unsigned int min_volt = pdo_min_voltage(pdo); + unsigned int max_volt = pdo_max_voltage(pdo); + unsigned int max_pow = pdo_max_power(pdo); + + dev_dbg(anx7688->dev, "SNK_CAP PDO_BATT (%umV-%umV %umW)\n", + min_volt, max_volt, max_pow); + } else if (pdo_type(pdo) == PDO_TYPE_VAR) { + unsigned int min_volt = pdo_min_voltage(pdo); + unsigned int max_volt = pdo_max_voltage(pdo); + unsigned int max_curr = pdo_max_current(pdo); + + dev_dbg(anx7688->dev, "SNK_CAP PDO_VAR (%umV-%umV %umA)\n", + min_volt, max_volt, max_curr); + } else { + dev_dbg(anx7688->dev, "SNK_CAP PDO_APDO (0x%08X)\n", pdo); + } + } + + break; + + case ANX7688_OCM_MSG_PWR_OBJ_REQ: + dev_dbg(anx7688->dev, "received PWR_OBJ_REQ\n"); + + anx7688->pd_capable = true; + + if (len != 4) { + dev_warn(anx7688->dev, "received invalid sized RDO\n"); + break; + } + + rdo = le32_to_cpu(pdos[0]); + + if (rdo_index(rdo) >= 1 && rdo_index(rdo) <= anx7688->n_src_caps) { + unsigned int rdo_op_curr = rdo_op_current(rdo); + unsigned int rdo_max_curr = rdo_max_current(rdo); + unsigned int rdo_idx = rdo_index(rdo) - 1; + unsigned int pdo_volt, pdo_max_curr; + + pdo = anx7688->src_caps[rdo_idx]; + pdo_volt = pdo_fixed_voltage(pdo); + pdo_max_curr = pdo_max_current(pdo); + + dev_dbg(anx7688->dev, "RDO (idx=%d op=%umA max=%umA)\n", + rdo_idx, rdo_op_curr, rdo_max_curr); + + dev_dbg(anx7688->dev, "PDO_FIXED (%umV %umA)\n", + pdo_volt, pdo_max_curr); + + // We could check the req and respond with accept/reject + // but we're using auto_pd feature, so the FW will do + // this for us + } else { + dev_dbg(anx7688->dev, + "PWR_OBJ RDO index out of range (RDO = 0x%08X)\n", rdo); + } + + break; + + case ANX7688_OCM_MSG_RESPONSE_TO_REQ: + if (len < 2) { + dev_warn(anx7688->dev, "received short RESPONSE_TO_REQ\n"); + break; + } + break; + + case ANX7688_OCM_MSG_HARD_RST: + if (anx7688->pd_capable) { + dev_dbg(anx7688->dev, "received HARD_RST\n"); + + // stop drawing power from VBUS + psy_val.intval = 0; + ret = power_supply_set_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_ONLINE, + &psy_val); + if (ret) + dev_err(anx7688->dev, "failed to offline vbus_in\n"); + + // wait till the dust settles + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 3000); + } else { + dev_dbg(anx7688->dev, "received HARD_RST, idiot firmware is bored\n"); + } + + break; + + case ANX7688_OCM_MSG_ACCEPT: + case ANX7688_OCM_MSG_REJECT: + case ANX7688_OCM_MSG_SOFT_RST: + case ANX7688_OCM_MSG_RESTART: + case ANX7688_OCM_MSG_PSWAP_REQ: + case ANX7688_OCM_MSG_DSWAP_REQ: + case ANX7688_OCM_MSG_VCONN_SWAP_REQ: + case ANX7688_OCM_MSG_DP_ALT_ENTER: + case ANX7688_OCM_MSG_DP_ALT_EXIT: + case ANX7688_OCM_MSG_DP_SNK_IDENTITY: + case ANX7688_OCM_MSG_SVID: + case ANX7688_OCM_MSG_VDM: + case ANX7688_OCM_MSG_GOTO_MIN_REQ: + case ANX7688_OCM_MSG_PD_STATUS_REQ: + case ANX7688_OCM_MSG_GET_DP_SNK_CAP: + case ANX7688_OCM_MSG_DP_SNK_CFG: + dev_dbg(anx7688->dev, "received message 0x%02x\n", cmd); + break; + + default: + dev_dbg(anx7688->dev, "received unknown message 0x%02x\n", cmd); + break; + } + + return 0; +} + +static int anx7688_receive_msg(struct anx7688 *anx7688) +{ + u8 pkt[32], checksum = 0; + int i, ret; + + ret = i2c_smbus_read_i2c_block_data(anx7688->client_tcpc, + ANX7688_TCPC_REG_INTERFACE_RECV, + 32, pkt); + if (ret < 0) { + dev_err(anx7688->dev, "failed to read pd msg\n"); + return ret; + } + + ret = anx7688_tcpc_reg_write(anx7688, ANX7688_TCPC_REG_INTERFACE_RECV, 0); + if (ret) + dev_warn(anx7688->dev, "failed to clear recv FIFO\n"); + + if (pkt[0] == 0 || pkt[0] > sizeof(pkt) - 2) { + dev_err(anx7688->dev, "received invalid pd message: %*ph\n", + (int)sizeof(pkt), pkt); + return -EINVAL; + } + + dev_dbg(anx7688->dev, "recv ocm message cmd=0x%02x %*ph\n", + pkt[1], pkt[0] + 2, pkt); + + for (i = 0; i < pkt[0] + 2; i++) + checksum += pkt[i]; + + if (checksum != 0) { + dev_err(anx7688->dev, "bad checksum on received message\n"); + return -EINVAL; + } + + anx7688_handle_pd_message(anx7688, pkt[1], pkt + 2, pkt[0] - 1); + return 0; +} + +static const char *anx7688_cc_status_string(unsigned int v) +{ + switch (v) { + case 0: return "SRC.Open"; + case 1: return "SRC.Rd"; + case 2: return "SRC.Ra"; + case 4: return "SNK.Default"; + case 8: return "SNK.Power1.5"; + case 12: return "SNK.Power3.0"; + default: return "UNK"; + } +} + +static int anx7688_update_status(struct anx7688 *anx7688) +{ + struct device *dev = anx7688->dev; + bool vbus_on, vconn_on, dr_dfp; + int status, cc_status, dp_state, dp_substate, ret; + + status = anx7688_reg_read(anx7688, ANX7688_REG_STATUS); + if (status < 0) + return status; + + cc_status = anx7688_reg_read(anx7688, ANX7688_REG_CC_STATUS); + if (cc_status < 0) + return cc_status; + + dp_state = anx7688_tcpc_reg_read(anx7688, 0x87); + if (dp_state < 0) + return dp_state; + + dp_substate = anx7688_tcpc_reg_read(anx7688, 0x88); + if (dp_substate < 0) + return dp_substate; + + anx7688_set_hdmi_hpd(anx7688, dp_state >= 3); + + dp_state = (dp_state << 8) | dp_substate; + + if (anx7688->last_status == -1 || anx7688->last_status != status) { + anx7688->last_status = status; + dev_dbg(dev, "status changed to 0x%02x\n", status); + } + + if (anx7688->last_cc_status == -1 || anx7688->last_cc_status != cc_status) { + anx7688->last_cc_status = cc_status; + dev_dbg(dev, "cc_status changed to CC1 = %s CC2 = %s\n", + anx7688_cc_status_string(cc_status & 0xf), + anx7688_cc_status_string((cc_status >> 4) & 0xf)); + } + + if (anx7688->last_dp_state == -1 || anx7688->last_dp_state != dp_state) { + anx7688->last_dp_state = dp_state; + dev_dbg(dev, "DP state changed to 0x%04x\n", dp_state); + } + + vbus_on = !!(status & ANX7688_VBUS_STATUS); + vconn_on = !!(status & ANX7688_VCONN_STATUS); + dr_dfp = !!(status & ANX7688_DATA_ROLE_STATUS); + + if (anx7688->vbus_on != vbus_on) { + dev_dbg(anx7688->dev, "POWER role change to %s\n", + vbus_on ? "SOURCE" : "SINK"); + + if (vbus_on) { + ret = regulator_enable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); + if (ret) { + dev_err(anx7688->dev, "failed to enable vbus\n"); + return ret; + } + } else { + ret = regulator_disable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); + if (ret) { + dev_err(anx7688->dev, "failed to disable vbus\n"); + return ret; + } + } + + anx7688->pwr_role = vbus_on ? TYPEC_SOURCE : TYPEC_SINK; + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); + anx7688->vbus_on = vbus_on; + } + + if (anx7688->vconn_on != vconn_on) { + dev_dbg(anx7688->dev, "VCONN role change to %s\n", + vconn_on ? "SOURCE" : "SINK"); + + if (vconn_on) { + ret = regulator_enable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); + if (ret) { + dev_err(anx7688->dev, "failed to enable vconn\n"); + return ret; + } + } else { + ret = regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); + if (ret) { + dev_err(anx7688->dev, "failed to disable vconn\n"); + return ret; + } + } + + typec_set_vconn_role(anx7688->port, vconn_on ? TYPEC_SOURCE : TYPEC_SINK); + anx7688->vconn_on = vconn_on; + } + + anx7688->data_role = dr_dfp ? TYPEC_HOST : TYPEC_DEVICE; + typec_set_data_role(anx7688->port, anx7688->data_role); + + if (usb_role_switch_get_role(anx7688->role_sw) != + (dr_dfp ? USB_ROLE_HOST : USB_ROLE_DEVICE)) { + dev_dbg(anx7688->dev, "DATA role change requested to %s\n", + dr_dfp ? "DFP" : "UFP"); + + ret = usb_role_switch_set_role(anx7688->role_sw, + dr_dfp ? USB_ROLE_HOST : USB_ROLE_DEVICE); + if (ret) + return ret; + } + + return 0; +} + +static irqreturn_t anx7688_irq_status_handler(int irq, void *data) +{ + struct anx7688 *anx7688 = data; + struct device *dev = anx7688->dev; + int tcpc_status, ext2_status, soft_status; + + mutex_lock(&anx7688->lock); + + if (!test_bit(ANX7688_F_CONNECTED, anx7688->flags)) { + dev_dbg(dev, "spurious status irq\n"); + /* + * anx chip should be disabled and power off, nothing + * more to do + */ + goto out_unlock; + } + + // clear tcpc interrupt + tcpc_status = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_ALERT0); + if (tcpc_status > 0) + anx7688_tcpc_reg_write(anx7688, ANX7688_TCPC_REG_ALERT0, tcpc_status); + + ext2_status = anx7688_reg_read(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2); + if (ext2_status & ANX7688_IRQ2_SOFT_INT) { + soft_status = anx7688_reg_read(anx7688, ANX7688_REG_STATUS_INT); + anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); + + if (soft_status > 0) { + soft_status &= ANX7688_SOFT_INT_MASK; + + if (soft_status & ANX7688_IRQS_RECEIVED_MSG) + anx7688_receive_msg(anx7688); + + if (soft_status & (ANX7688_IRQS_CC_STATUS_CHANGE | + ANX7688_IRQS_VBUS_CHANGE | + ANX7688_IRQS_VCONN_CHANGE | + ANX7688_IRQS_DATA_ROLE_CHANGE)) { + anx7688_update_status(anx7688); + } + } + + anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, ANX7688_IRQ2_SOFT_INT); + } + +out_unlock: + mutex_unlock(&anx7688->lock); + + return IRQ_HANDLED; +} + +static int anx7688_dr_set(struct typec_port *port, enum typec_data_role role) +{ + struct anx7688 *anx7688 = typec_get_drvdata(port); + int ret = 0; + + dev_dbg(anx7688->dev, "data role set %d\n", role); + + if (anx7688->data_role != role) { + mutex_lock(&anx7688->lock); + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_DSWAP_REQ, 0, 0); + mutex_unlock(&anx7688->lock); + } + + return ret; +} + +static int anx7688_pr_set(struct typec_port *port, enum typec_role role) +{ + struct anx7688 *anx7688 = typec_get_drvdata(port); + int ret = 0; + + dev_dbg(anx7688->dev, "power role set %d\n", role); + + if (anx7688->pwr_role != role) { + mutex_lock(&anx7688->lock); + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PSWAP_REQ, 0, 0); + mutex_unlock(&anx7688->lock); + } + + return ret; +} + +/* + * Calls to the EEPROM functions need to be taken under the anx7688 lock. + */ +static int anx7688_eeprom_set_address(struct anx7688 *anx7688, u16 addr) +{ + int ret; + + ret = anx7688_reg_write(anx7688, 0xe0, (addr >> 8) & 0xff); + if (ret < 0) + return ret; + + return anx7688_reg_write(anx7688, 0xe1, addr & 0xff); +} + +static int anx7688_eeprom_wait_done(struct anx7688 *anx7688) +{ + ktime_t timeout; + int ret; + + // wait for read to be done + timeout = ktime_add_us(ktime_get(), 50000); + while (true) { + ret = anx7688_reg_read(anx7688, 0xe2); + if (ret < 0) + return ret; + + if (ret & BIT(3)) + return 0; + + if (ktime_after(ktime_get(), timeout)) { + dev_err(anx7688->dev, "timeout waiting for eeprom\n"); + return -ETIMEDOUT; + } + } +} + +/* + * Wait for internal FSM of EEPROM to be in a state ready for + * programming/reading + */ +static int anx7688_eeprom_wait_ready(struct anx7688 *anx7688) +{ + ktime_t timeout; + int ret; + + // wait until eeprom is ready + timeout = ktime_add_us(ktime_get(), 1000000); + while (true) { + ret = anx7688_reg_read(anx7688, 0x7f); + if (ret < 0) + return ret; + + if ((ret & 0x0f) == 7) + return 0; + + if (ktime_after(ktime_get(), timeout)) { + dev_err(anx7688->dev, "timeout waiting for eeprom to initialize\n"); + return -ETIMEDOUT; + } + + msleep(5); + } +} + +static int anx7688_eeprom_read(struct anx7688 *anx7688, unsigned int addr, u8 buf[16]) +{ + int ret; + + ret = anx7688_eeprom_set_address(anx7688, addr); + if (ret) + return ret; + + // initiate read + ret = anx7688_reg_write(anx7688, 0xe2, 0x06); + if (ret < 0) + return ret; + + ret = anx7688_eeprom_wait_done(anx7688); + if (ret) + return ret; + + ret = i2c_smbus_read_i2c_block_data(anx7688->client, 0xd0, 16, buf); + if (ret < 0) { + dev_err(anx7688->dev, + "failed to read eeprom data (err=%d)\n", ret); + return ret; + } + + return 0; +} + +static const struct typec_operations anx7688_typec_ops = { + .dr_set = anx7688_dr_set, + .pr_set = anx7688_pr_set, +}; + +/* + * This function has to work when the ANX7688 is active, and when + * it is powered down. It power cycles the chip and asserts the OCM + * reset, to prevent OCM FW interfering with EEPROM reading. + * + * After reading EEPROM, the reconnection is scheduled. + */ +static int anx7688_firmware_show(struct seq_file *s, void *data) +{ + struct anx7688 *anx7688 = s->private; + unsigned int addr; + u8 buf[16]; + int ret; + + mutex_lock(&anx7688->lock); + + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) + anx7688_disconnect(anx7688); + + msleep(20); + + anx7688_power_enable(anx7688); + + ret = anx7688_reg_update_bits(anx7688, ANX7688_REG_USBC_RESET_CTRL, + ANX7688_USBC_RESET_CTRL_OCM_RESET, + ANX7688_USBC_RESET_CTRL_OCM_RESET); + if (ret < 0) + goto out_powerdown; + + ret = anx7688_eeprom_wait_ready(anx7688); + if (ret) + goto out_powerdown; + + msleep(10); + + for (addr = 0x10; addr < 0x10000; addr += 16) { + // set address + ret = anx7688_eeprom_read(anx7688, addr, buf); + if (ret < 0) + goto out_powerdown; + + seq_write(s, buf, sizeof(buf)); + } + +out_powerdown: + anx7688_power_disable(anx7688); + schedule_delayed_work(&anx7688->work, 0); + mutex_unlock(&anx7688->lock); + + return ret; +} +DEFINE_SHOW_ATTRIBUTE(anx7688_firmware); + +static int anx7688_regs_show(struct seq_file *s, void *data) +{ + struct anx7688 *anx7688 = s->private; + u8 buf[16]; + unsigned int i, addr; + int ret = -ENODEV; + + mutex_lock(&anx7688->lock); + + if (!test_bit(ANX7688_F_POWERED, anx7688->flags)) + goto out_unlock; + + for (addr = 0; addr < 256; addr += 16) { + ret = i2c_smbus_read_i2c_block_data(anx7688->client, addr, + sizeof(buf), buf); + if (ret < 0) { + dev_dbg(anx7688->dev, + "failed to read registers (err=%d)\n", ret); + goto out_unlock; + } + + for (i = 0; i < 16; i++) + seq_printf(s, "50%02x: %02x\n", addr + i, buf[i]); + } + + for (addr = 0; addr < 256; addr += 16) { + ret = i2c_smbus_read_i2c_block_data(anx7688->client_tcpc, addr, + sizeof(buf), buf); + if (ret < 0) { + dev_dbg(anx7688->dev, + "failed to read registers (err=%d)\n", ret); + goto out_unlock; + } + + for (i = 0; i < 16; i++) + seq_printf(s, "58%02x: %02x\n", addr + i, buf[i]); + } + +out_unlock: + mutex_unlock(&anx7688->lock); + + return ret; +} +DEFINE_SHOW_ATTRIBUTE(anx7688_regs); + +/* + * This is just a 1s watchdog checking the state if cabledet pin. + */ +static void anx7688_cabledet_timer_fn(struct timer_list *t) +{ + struct anx7688 *anx7688 = from_timer(anx7688, t, work_timer); + + schedule_delayed_work(&anx7688->work, 0); + mod_timer(t, jiffies + msecs_to_jiffies(1000)); +} + +static void anx7688_handle_vbus_in_notify(struct anx7688 *anx7688) +{ + union power_supply_propval psy_val = {0,}; + struct device *dev = anx7688->dev; + int ret; + + ret = power_supply_get_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_USB_TYPE, + &psy_val); + if (ret) { + dev_err(dev, "failed to get USB BC1.2 result\n"); + return; + } + + if (anx7688->last_bc_result == psy_val.intval) + return; + + anx7688->last_bc_result = psy_val.intval; + + switch (psy_val.intval) { + case POWER_SUPPLY_USB_TYPE_DCP: + case POWER_SUPPLY_USB_TYPE_CDP: + dev_dbg(dev, "BC 1.2 result: DCP or CDP\n"); + break; + case POWER_SUPPLY_USB_TYPE_SDP: + default: + dev_dbg(dev, "BC 1.2 result: SDP\n"); + break; + } +} + +static int anx7688_cc_status(unsigned int v) +{ + switch (v) { + case 0: return -1; + case 1: return -1; + case 2: return -1; + case 4: return TYPEC_PWR_MODE_USB; + case 8: return TYPEC_PWR_MODE_1_5A; + case 12: return TYPEC_PWR_MODE_3_0A; + default: return -1; + } +} + +static const char *anx7688_get_power_mode_name(enum typec_pwr_opmode mode) +{ + switch (mode) { + case TYPEC_PWR_MODE_USB: return "USB"; + case TYPEC_PWR_MODE_1_5A: return "1.5A"; + case TYPEC_PWR_MODE_3_0A: return "3.0A"; + case TYPEC_PWR_MODE_PD: return "PD"; + default: return "Unknown"; + } +} + +/* + * This is called after 500ms after connection when the PD contract should have + * been negotiated. We should inspect CC pins or PD status here and decide what + * input current limit to set. + */ +static void anx7688_handle_current_update(struct anx7688 *anx7688) +{ + unsigned int cc_status = anx7688->last_cc_status; + union power_supply_propval val = {0,}; + struct device *dev = anx7688->dev; + int pwr_mode, ret, current_limit = 0; + + if (anx7688->pd_capable) { + pwr_mode = TYPEC_PWR_MODE_PD; + } else if (cc_status < 0) { + pwr_mode = TYPEC_PWR_MODE_USB; + } else { + pwr_mode = anx7688_cc_status(cc_status & 0xf); + if (pwr_mode < 0) + pwr_mode = anx7688_cc_status((cc_status >> 4) & 0xf); + if (pwr_mode < 0) + pwr_mode = TYPEC_PWR_MODE_USB; + } + + if (pwr_mode == TYPEC_PWR_MODE_1_5A) + current_limit = 1500; + else if (pwr_mode == TYPEC_PWR_MODE_3_0A) + current_limit = 3000; + else if (pwr_mode == TYPEC_PWR_MODE_PD) + current_limit = anx7688->pd_current_limit; + + anx7688->input_current_limit = current_limit; + + dev_dbg(anx7688->dev, "updating power mode to %s, current limit %dmA (0 => BC1.2)\n", + anx7688_get_power_mode_name(pwr_mode), current_limit); + + if (current_limit) { + /* + * Disable BC1.2 detection, because we'll be setting + * a current limit determined by USB-PD + */ + dev_dbg(dev, "disabling USB BC 1.2 detection\n"); + + val.intval = current_limit * 1000; + dev_dbg(dev, "setting vbus_in current limit to %d mA\n", current_limit); + ret = power_supply_set_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, + &val); + if (ret) + dev_err(dev, "failed to set vbus_in current to %d mA\n", + current_limit); + } + /* + * else use the result of BC1.2 detection performed by PMIC. + */ + + /* Turn on VBUS power path inside PMIC. */ + val.intval = 1; + dev_dbg(dev, "enabling vbus_in power path\n"); + ret = power_supply_set_property(anx7688->vbus_in_supply, + POWER_SUPPLY_PROP_ONLINE, + &val); + if (ret) + dev_err(anx7688->dev, "failed to enable vbus_in\n"); + + typec_set_pwr_opmode(anx7688->port, pwr_mode); +} + +static int anx7688_vbus_in_notify(struct notifier_block *nb, + unsigned long val, void *v) +{ + struct anx7688 *anx7688 = container_of(nb, struct anx7688, vbus_in_nb); + struct power_supply *psy = v; + + /* atomic context */ + if (val == PSY_EVENT_PROP_CHANGED && psy == anx7688->vbus_in_supply) { + set_bit(ANX7688_F_PWRSUPPLY_CHANGE, anx7688->flags); + schedule_delayed_work(&anx7688->work, 0); + } + + return NOTIFY_OK; +} + +static void anx7688_work(struct work_struct *work) +{ + struct anx7688 *anx7688 = container_of(work, struct anx7688, work.work); + + if (test_bit(ANX7688_F_FW_FAILED, anx7688->flags)) + return; + + mutex_lock(&anx7688->lock); + + if (test_and_clear_bit(ANX7688_F_PWRSUPPLY_CHANGE, anx7688->flags)) + anx7688_handle_vbus_in_notify(anx7688); + + anx7688_handle_cable_change(anx7688); + + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) { + /* + * We check status periodically outside of interrupt, just to + * be sure we didn't miss any status interrupts + */ + anx7688_update_status(anx7688); + + if (anx7688->current_update_deadline && + ktime_after(ktime_get(), anx7688->current_update_deadline)) { + anx7688->current_update_deadline = 0; + anx7688_handle_current_update(anx7688); + } + } + + mutex_unlock(&anx7688->lock); +} + +static int anx7688_parse_connector(struct device *dev, struct anx7688 *anx7688, struct typec_capability *cap) +{ + struct fwnode_handle *fwnode; + const char *buf; + int ret; + + fwnode = device_get_named_child_node(dev, "connector"); + if (!fwnode) + return -EINVAL; + + ret = fwnode_property_read_string(fwnode, "power-role", &buf); + if (ret) { + dev_err(dev, "power-role not found: %d\n", ret); + return ret; + } + + ret = typec_find_port_power_role(buf); + if (ret < 0) + return ret; + cap->type = ret; + + ret = fwnode_property_read_string(fwnode, "data-role", &buf); + if (ret) { + dev_err(dev, "data-role not found: %d\n", ret); + return ret; + } + + ret = typec_find_port_data_role(buf); + if (ret < 0) + return ret; + cap->data = ret; + + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf); + if (ret) { + dev_err(dev, "try-power-role not found: %d\n", ret); + return ret; + } + + ret = typec_find_power_role(buf); + if (ret < 0) + return ret; + cap->prefer_role = ret; + + /* Get source pdos */ + ret = fwnode_property_count_u32(fwnode, "source-pdos"); + if (ret <= 0) + return -EINVAL; + + anx7688->n_src_caps = min_t(u8, ret, ARRAY_SIZE(anx7688->src_caps)); + ret = fwnode_property_read_u32_array(fwnode, "source-pdos", + anx7688->src_caps, + anx7688->n_src_caps); + if (ret < 0) { + dev_err(dev, "source cap validate failed: %d\n", ret); + return ret; + } + + ret = fwnode_property_count_u32(fwnode, "sink-pdos"); + if (ret <= 0) + return -EINVAL; + + anx7688->n_snk_caps = min_t(u8, ret, ARRAY_SIZE(anx7688->snk_caps)); + ret = fwnode_property_read_u32_array(fwnode, "sink-pdos", + anx7688->snk_caps, + anx7688->n_snk_caps); + if (ret < 0) { + dev_err(dev, "sink cap validate failed: %d\n", ret); + return ret; + } + + return 0; +} + +static int anx7688_i2c_probe(struct i2c_client *client) +{ + struct anx7688 *anx7688; + struct device *dev = &client->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 = &client->dev; + mutex_init(&anx7688->lock); + INIT_DELAYED_WORK(&anx7688->work, anx7688_work); + anx7688->last_extcon_state = -1; + + ret = anx7688_parse_connector(dev, anx7688, &typec_cap); + if (ret < 0) { + dev_err(dev, "failed to parse connector from DT\n"); + return ret; + } + + for (i = 0; i < ANX7688_NUM_SUPPLIES; i++) + anx7688->supplies[i].supply = anx7688_supply_names[i]; + ret = devm_regulator_bulk_get(dev, ANX7688_NUM_SUPPLIES, + anx7688->supplies); + if (ret) + return ret; + + anx7688->vbus_in_supply = + devm_power_supply_get_by_phandle(dev, "vbus-in-supply"); + if (IS_ERR(anx7688->vbus_in_supply)) { + dev_err(dev, "Couldn't get the VBUS power supply\n"); + return PTR_ERR(anx7688->vbus_in_supply); + } + + if (!anx7688->vbus_in_supply) + return -EPROBE_DEFER; + + anx7688->gpio_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(anx7688->gpio_enable)) { + dev_err(dev, "Could not get enable gpio\n"); + return PTR_ERR(anx7688->gpio_enable); + } + + anx7688->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(anx7688->gpio_reset)) { + dev_err(dev, "Could not get reset gpio\n"); + return PTR_ERR(anx7688->gpio_reset); + } + + anx7688->gpio_cabledet = devm_gpiod_get(dev, "cabledet", GPIOD_IN); + if (IS_ERR(anx7688->gpio_cabledet)) { + dev_err(dev, "Could not get cabledet gpio\n"); + return PTR_ERR(anx7688->gpio_cabledet); + } + + irq_cabledet = gpiod_to_irq(anx7688->gpio_cabledet); + if (irq_cabledet < 0) { + dev_err(dev, "Could not get cabledet irq\n"); + return irq_cabledet; + } + + // Initialize extcon device + anx7688->extcon = devm_extcon_dev_allocate(dev, anx7688_extcon_cable); + if (IS_ERR(anx7688->extcon)) + return -ENOMEM; + + ret = devm_extcon_dev_register(dev, anx7688->extcon); + if (ret) { + dev_err(dev, "failed to register extcon device\n"); + return ret; + } + + // Register the TCPC i2c interface as second interface (0x58) + anx7688->client_tcpc = i2c_new_dummy_device(client->adapter, 0x2c); + if (IS_ERR(anx7688->client_tcpc)) { + dev_err(dev, "Could not register tcpc i2c client\n"); + return PTR_ERR(anx7688->client_tcpc); + } + i2c_set_clientdata(anx7688->client_tcpc, anx7688); + + // powerup and probe the ANX chip + + ret = regulator_bulk_enable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, + anx7688->supplies); + if (ret) { + dev_err(dev, "Could not enable regulators\n"); + goto err_dummy_dev; + } + + msleep(10); + + anx7688_power_enable(anx7688); + + vid_l = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_VENDOR_ID0); + vid_h = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_VENDOR_ID1); + if (vid_l < 0 || vid_h < 0) { + ret = vid_l < 0 ? vid_l : vid_h; + anx7688_power_disable(anx7688); + goto err_disable_reg; + } + + dev_dbg(dev, "Vendor id 0x%04x\n", vid_l | vid_h << 8); + + anx7688_power_disable(anx7688); + + anx7688->role_sw = usb_role_switch_get(dev); + if (IS_ERR(anx7688->role_sw)) { + dev_err(dev, "Could not get role switch\n"); + ret = PTR_ERR(anx7688->role_sw); + goto err_disable_reg; + } + + // setup a typec port device + typec_cap.revision = USB_TYPEC_REV_1_2; + typec_cap.pd_revision = 0x200; + ret = -EINVAL; + if (typec_cap.type != TYPEC_PORT_DRP) + goto err_disable_reg; + if (typec_cap.data != TYPEC_PORT_DRD) + goto err_disable_reg; + typec_cap.driver_data = anx7688; + typec_cap.ops = &anx7688_typec_ops; + + anx7688->port = typec_register_port(dev, &typec_cap); + if (IS_ERR(anx7688->port)) { + dev_err(dev, "Could not register type-c port\n"); + ret = PTR_ERR(anx7688->port); + goto err_role_sw; + } + + anx7688->pwr_role = TYPEC_SINK; + anx7688->data_role = TYPEC_DEVICE; + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); + typec_set_data_role(anx7688->port, anx7688->data_role); + typec_set_pwr_opmode(anx7688->port, TYPEC_PWR_MODE_USB); + typec_set_vconn_role(anx7688->port, TYPEC_SINK); + + // make sure BC1.2 detection in PMIC is enabled + anx7688->last_bc_result = -1; + + dev_dbg(dev, "enabling USB BC 1.2 detection\n"); + + ret = devm_request_irq(dev, irq_cabledet, anx7688_irq_plug_handler, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, + "anx7688-cabledet", anx7688); + if (ret < 0) { + dev_err(dev, "Could not request cabledet irq (%d)\n", ret); + goto err_cport; + } + + ret = devm_request_threaded_irq(dev, client->irq, + NULL, anx7688_irq_status_handler, + IRQF_ONESHOT, NULL, anx7688); + if (ret < 0) { + dev_err(dev, "Could not request irq (%d)\n", ret); + goto err_cport; + } + + anx7688->vbus_in_nb.notifier_call = anx7688_vbus_in_notify; + anx7688->vbus_in_nb.priority = 0; + ret = power_supply_reg_notifier(&anx7688->vbus_in_nb); + if (ret) + goto err_cport; + + anx7688->debug_root = debugfs_create_dir("anx7688", NULL); + debugfs_create_file("firmware", 0444, anx7688->debug_root, anx7688, + &anx7688_firmware_fops); + debugfs_create_file("regs", 0444, anx7688->debug_root, anx7688, + &anx7688_regs_fops); + + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(10)); + + timer_setup(&anx7688->work_timer, anx7688_cabledet_timer_fn, 0); + mod_timer(&anx7688->work_timer, jiffies + msecs_to_jiffies(1000)); + return 0; + +err_cport: + typec_unregister_port(anx7688->port); +err_role_sw: + usb_role_switch_put(anx7688->role_sw); +err_disable_reg: + regulator_bulk_disable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, anx7688->supplies); +err_dummy_dev: + i2c_unregister_device(anx7688->client_tcpc); + return ret; +} + +static void anx7688_i2c_remove(struct i2c_client *client) +{ + struct anx7688 *anx7688 = i2c_get_clientdata(client); + + mutex_lock(&anx7688->lock); + + power_supply_unreg_notifier(&anx7688->vbus_in_nb); + + del_timer_sync(&anx7688->work_timer); + + cancel_delayed_work_sync(&anx7688->work); + + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) + anx7688_disconnect(anx7688); + + typec_unregister_partner(anx7688->partner); + typec_unregister_port(anx7688->port); + usb_role_switch_put(anx7688->role_sw); + + regulator_bulk_disable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, anx7688->supplies); + i2c_unregister_device(anx7688->client_tcpc); + + debugfs_remove(anx7688->debug_root); + + mutex_unlock(&anx7688->lock); +} + +static int __maybe_unused anx7688_suspend(struct device *dev) +{ + struct anx7688 *anx7688 = i2c_get_clientdata(to_i2c_client(dev)); + + del_timer_sync(&anx7688->work_timer); + cancel_delayed_work_sync(&anx7688->work); + + regulator_disable(anx7688->supplies[ANX7688_I2C_INDEX].consumer); + + return 0; +} + +static int __maybe_unused anx7688_resume(struct device *dev) +{ + struct anx7688 *anx7688 = i2c_get_clientdata(to_i2c_client(dev)); + int ret; + + ret = regulator_enable(anx7688->supplies[ANX7688_I2C_INDEX].consumer); + if (ret) + dev_warn(anx7688->dev, + "failed to enable I2C regulator (%d)\n", ret); + + // check status right after resume, since it could have changed during + // sleep + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(50)); + mod_timer(&anx7688->work_timer, jiffies + msecs_to_jiffies(1000)); + + return 0; +} + +static const struct dev_pm_ops anx7688_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(anx7688_suspend, anx7688_resume) +}; + +static const struct i2c_device_id anx7688_ids[] = { + { "anx7688", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, anx7688_ids); + +static const struct of_device_id anx7688_of_match_table[] = { + { .compatible = "analogix,anx7688" }, + { }, +}; +MODULE_DEVICE_TABLE(of, anx7688_of_match_table); + +static struct i2c_driver anx7688_driver = { + .driver = { + .name = "anx7688", + .of_match_table = anx7688_of_match_table, + .pm = &anx7688_pm_ops, + }, + .probe = anx7688_i2c_probe, + .remove = anx7688_i2c_remove, + .id_table = anx7688_ids, +}; + +module_i2c_driver(anx7688_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Martijn Braam <martijn@brixit.nl>"); +MODULE_AUTHOR("Ondrej Jirman <megi@xff.cz>"); +MODULE_DESCRIPTION("Analogix ANX7688 USB-C DisplayPort bridge"); -- People of Russia, stop Putin before his war on Ukraine escalates. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-08 10:54 ` [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge Pavel Machek @ 2024-04-09 9:56 ` Heikki Krogerus 2024-04-09 11:04 ` Pavel Machek 0 siblings, 1 reply; 22+ messages in thread From: Heikki Krogerus @ 2024-04-09 9:56 UTC (permalink / raw) To: Pavel Machek Cc: phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb, megi Hi, On Mon, Apr 08, 2024 at 12:54:25PM +0200, Pavel Machek wrote: > From: Ondrej Jirman <megi@xff.cz> > > 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 <megi@xff.cz> > Co-developed-by: Martijn Braam <martijn@brixit.nl> > Co-developed-by: Samuel Holland <samuel@sholland.org> > Signed-off-by: Pavel Machek <pavel@ucw.cz> Just couple of quick comments below - I did not have time to go over this very thoroughly, but I think you need to make a new version in any case because of comments in 1/2. Btw, Co-developed-by comes before Signed-off-by I think. > --- > > v2: Fix checkpatch stuff. Some cleanups, adapt to dts format in 1/2. > v3: Turn down debugging, as requested during review. > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index 2f80c2792dbd..c9043ae61546 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -64,6 +64,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 000000000000..407cbd5fedba > --- /dev/null > +++ b/drivers/usb/typec/anx7688.c > @@ -0,0 +1,1830 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ANX7688 USB-C HDMI bridge/PD driver > + * > + * Warning, this driver is somewhat PinePhone specific. So why not split this into core part and PinePhone specific glue part? > + * 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 > + */ > + > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/extcon-provider.h> > +#include <linux/firmware.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/irqreturn.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/power_supply.h> > +#include <linux/regulator/consumer.h> > +#include <linux/usb/pd.h> > +#include <linux/usb/role.h> > +#include <linux/usb/typec.h> > + > +/* firmware regs */ > + > +#define ANX7688_REG_VBUS_OFF_DELAY_TIME 0x22 > +#define ANX7688_REG_FEATURE_CTRL 0x27 > +#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_CHANGE BIT(3) > +#define ANX7688_IRQS_CC_STATUS_CHANGE BIT(4) > +#define ANX7688_IRQS_DATA_ROLE_CHANGE BIT(5) > + > +#define ANX7688_REG_STATUS 0x29 > +#define ANX7688_VCONN_STATUS BIT(2) /* 0 = off 1 = on */ > +#define ANX7688_VBUS_STATUS BIT(3) /* 0 = off 1 = on */ > +#define ANX7688_DATA_ROLE_STATUS BIT(5) /* 0 = device 1 = host */ > + > +#define ANX7688_REG_CC_STATUS 0x2a > +#define ANX7688_REG_TRY_UFP_TIMER 0x23 > +#define ANX7688_REG_TIME_CTRL 0x24 > + > +#define ANX7688_REG_MAX_VOLTAGE 0x1b > +#define ANX7688_REG_MAX_POWER 0x1c > +#define ANX7688_REG_MIN_POWER 0x1d > +#define ANX7688_REG_MAX_VOLTAGE_STATUS 0x1e > +#define ANX7688_REG_MAX_POWER_STATUS 0x1f > + > +#define ANX7688_SOFT_INT_MASK 0x7f > + > +/* tcpc regs */ > + > +#define ANX7688_TCPC_REG_VENDOR_ID0 0x00 > +#define ANX7688_TCPC_REG_VENDOR_ID1 0x01 > +#define ANX7688_TCPC_REG_ALERT0 0x10 > +#define ANX7688_TCPC_REG_ALERT1 0x11 > +#define ANX7688_TCPC_REG_ALERT_MASK0 0x12 > +#define ANX7688_TCPC_REG_ALERT_MASK1 0x13 > +#define ANX7688_TCPC_REG_INTERFACE_SEND 0x30 > +#define ANX7688_TCPC_REG_INTERFACE_RECV 0x51 > + > +/* hw regs */ > + > +#define ANX7688_REG_IRQ_EXT_SOURCE0 0x3e > +#define ANX7688_REG_IRQ_EXT_SOURCE1 0x4e > +#define ANX7688_REG_IRQ_EXT_SOURCE2 0x4f > +#define ANX7688_REG_IRQ_EXT_MASK0 0x3b > +#define ANX7688_REG_IRQ_EXT_MASK1 0x3c > +#define ANX7688_REG_IRQ_EXT_MASK2 0x3d > +#define ANX7688_REG_IRQ_SOURCE0 0x54 > +#define ANX7688_REG_IRQ_SOURCE1 0x55 > +#define ANX7688_REG_IRQ_SOURCE2 0x56 > +#define ANX7688_REG_IRQ_MASK0 0x57 > +#define ANX7688_REG_IRQ_MASK1 0x58 > +#define ANX7688_REG_IRQ_MASK2 0x59 > + > +#define ANX7688_IRQ2_SOFT_INT BIT(2) > + > +#define ANX7688_REG_USBC_RESET_CTRL 0x05 > +#define ANX7688_USBC_RESET_CTRL_OCM_RESET BIT(4) > + > +/* ocm messages */ > + > +#define ANX7688_OCM_MSG_PWR_SRC_CAP 0x00 > +#define ANX7688_OCM_MSG_PWR_SNK_CAP 0x01 > +#define ANX7688_OCM_MSG_DP_SNK_IDENTITY 0x02 > +#define ANX7688_OCM_MSG_SVID 0x03 > +#define ANX7688_OCM_MSG_GET_DP_SNK_CAP 0x04 > +#define ANX7688_OCM_MSG_ACCEPT 0x05 > +#define ANX7688_OCM_MSG_REJECT 0x06 > +#define ANX7688_OCM_MSG_PSWAP_REQ 0x10 > +#define ANX7688_OCM_MSG_DSWAP_REQ 0x11 > +#define ANX7688_OCM_MSG_GOTO_MIN_REQ 0x12 > +#define ANX7688_OCM_MSG_VCONN_SWAP_REQ 0x13 > +#define ANX7688_OCM_MSG_VDM 0x14 > +#define ANX7688_OCM_MSG_DP_SNK_CFG 0x15 > +#define ANX7688_OCM_MSG_PWR_OBJ_REQ 0x16 > +#define ANX7688_OCM_MSG_PD_STATUS_REQ 0x17 > +#define ANX7688_OCM_MSG_DP_ALT_ENTER 0x19 > +#define ANX7688_OCM_MSG_DP_ALT_EXIT 0x1a > +#define ANX7688_OCM_MSG_GET_SNK_CAP 0x1b > +#define ANX7688_OCM_MSG_RESPONSE_TO_REQ 0xf0 > +#define ANX7688_OCM_MSG_SOFT_RST 0xf1 > +#define ANX7688_OCM_MSG_HARD_RST 0xf2 > +#define ANX7688_OCM_MSG_RESTART 0xf3 > + > +static const char * const anx7688_supply_names[] = { > + "avdd33", > + "avdd18", > + "dvdd18", > + "avdd10", > + "dvdd10", > + "i2c", > + "hdmi-vt", > + > + "vconn", // power for VCONN1/VCONN2 switches > + "vbus", // vbus power > +}; > + > +#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; > + > + u32 src_caps[8]; > + unsigned int n_src_caps; > + > + u32 snk_caps[8]; > + unsigned int n_snk_caps; > + > + unsigned long flags[1]; > + > + struct delayed_work work; > + struct timer_list work_timer; > + > + struct mutex lock; Undocumented lock. > + bool vbus_on, vconn_on; > + bool pd_capable; > + int pd_current_limit; // mA > + ktime_t current_update_deadline; > + > + struct typec_port *port; > + struct typec_partner *partner; > + struct usb_pd_identity partner_identity; > + struct usb_role_switch *role_sw; > + int pwr_role, data_role; > + > + struct dentry *debug_root; > + > + /* for debug */ > + int last_status; > + int last_cc_status; > + int last_dp_state; > + int last_bc_result; > + > + /* for HDMI HPD */ > + struct extcon_dev *extcon; > + int last_extcon_state; > +}; > + > +static const unsigned int anx7688_extcon_cable[] = { > + EXTCON_DISP_HDMI, > + EXTCON_NONE, > +}; > + > +static int anx7688_reg_read(struct anx7688 *anx7688, u8 reg_addr) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(anx7688->client, reg_addr); > + if (ret < 0) > + dev_err(anx7688->dev, "i2c read failed at 0x%x (%d)\n", > + reg_addr, ret); > + > + return ret; > +} > + > +static int anx7688_reg_write(struct anx7688 *anx7688, u8 reg_addr, u8 value) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(anx7688->client, reg_addr, value); > + if (ret < 0) > + dev_err(anx7688->dev, "i2c write failed at 0x%x (%d)\n", > + reg_addr, ret); > + > + return ret; > +} > + > +static int anx7688_reg_update_bits(struct anx7688 *anx7688, u8 reg_addr, > + u8 mask, u8 value) > +{ > + int ret; > + > + ret = anx7688_reg_read(anx7688, reg_addr); > + if (ret < 0) > + return ret; > + > + ret &= ~mask; > + ret |= value; > + > + return anx7688_reg_write(anx7688, reg_addr, ret); > +} > + > +static int anx7688_tcpc_reg_read(struct anx7688 *anx7688, u8 reg_addr) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(anx7688->client_tcpc, reg_addr); > + if (ret < 0) > + dev_err(anx7688->dev, "tcpc i2c read failed at 0x%x (%d)\n", > + reg_addr, ret); > + > + return ret; > +} > + > +static int anx7688_tcpc_reg_write(struct anx7688 *anx7688, u8 reg_addr, u8 value) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(anx7688->client_tcpc, reg_addr, value); > + if (ret < 0) > + dev_err(anx7688->dev, "tcpc i2c write failed at 0x%x (%d)\n", > + reg_addr, ret); > + > + return ret; > +} > + > +static void anx7688_power_enable(struct anx7688 *anx7688) > +{ > + gpiod_set_value(anx7688->gpio_reset, 1); > + gpiod_set_value(anx7688->gpio_enable, 1); > + > + /* wait for power to stabilize and release reset */ > + msleep(10); > + gpiod_set_value(anx7688->gpio_reset, 0); > + usleep_range(2, 4); Why not just use usleep_range() in both cases. > + dev_dbg(anx7688->dev, "power enabled\n"); > + > + set_bit(ANX7688_F_POWERED, anx7688->flags); > +} > + > +static void anx7688_power_disable(struct anx7688 *anx7688) > +{ > + gpiod_set_value(anx7688->gpio_reset, 1); > + msleep(5); usleep_range() > + gpiod_set_value(anx7688->gpio_enable, 0); > + > + dev_dbg(anx7688->dev, "power disabled\n"); > + > + clear_bit(ANX7688_F_POWERED, anx7688->flags); > +} > + > +static int anx7688_send_ocm_message(struct anx7688 *anx7688, int cmd, > + const u8 *data, int data_len) > +{ > + int ret = 0, i; > + u8 pkt[32]; > + > + if (data_len > sizeof(pkt) - 3) { > + dev_dbg(anx7688->dev, > + "invalid ocm message length cmd=0x%02x len=%d\n", > + cmd, data_len); > + return -EINVAL; > + } > + > + // prepare pd packet > + pkt[0] = data_len + 1; > + pkt[1] = cmd; > + if (data_len > 0) > + memcpy(pkt + 2, data, data_len); > + pkt[2 + data_len] = 0; > + for (i = 0; i < data_len + 2; i++) > + pkt[data_len + 2] -= pkt[i]; > + > + dev_dbg(anx7688->dev, "send pd packet cmd=0x%02x %*ph\n", > + cmd, data_len + 3, pkt); > + > + ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND); > + if (ret) { > + dev_err(anx7688->dev, "failed to send pd packet (tx buffer full)\n"); > + return -EBUSY; > + } > + > + ret = i2c_smbus_write_i2c_block_data(anx7688->client_tcpc, > + ANX7688_TCPC_REG_INTERFACE_SEND, > + data_len + 3, pkt); > + if (ret < 0) > + dev_err(anx7688->dev, "failed to send pd packet (err=%d)\n", ret); > + > + // wait until the message is processed (30ms max) > + for (i = 0; i < 300; i++) { > + ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND); > + if (ret <= 0) > + return ret; > + > + udelay(100); > + } > + > + dev_err(anx7688->dev, "timeout waiting for the message queue flush\n"); > + return -ETIMEDOUT; > +} > + > +static int anx7688_connect(struct anx7688 *anx7688) > +{ > + struct typec_partner_desc desc = {}; > + int ret, i; > + u8 fw[2]; > + const u8 dp_snk_identity[16] = { > + 0x00, 0x00, 0x00, 0xec, /* id header */ > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > + 0x00, 0x00, 0x00, 0x00, /* product type */ > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > + }; > + const u8 svid[4] = { > + 0x00, 0x00, 0x01, 0xff, > + }; Why not get those from DT? > + u32 caps[8]; > + > + dev_dbg(anx7688->dev, "cable inserted\n"); > + > + anx7688->last_status = -1; > + anx7688->last_cc_status = -1; > + anx7688->last_dp_state = -1; > + > + msleep(10); Please make a comment here why you have to wait, and use usleep_range(). > + anx7688_power_enable(anx7688); > + > + ret = regulator_enable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); > + if (ret) { > + dev_err(anx7688->dev, "failed to enable vconn\n"); > + goto err_poweroff; > + } > + anx7688->vconn_on = true; > + > + /* wait till the firmware is loaded (typically ~30ms) */ > + i = 0; > + while (1) { > + ret = anx7688_reg_read(anx7688, ANX7688_REG_EEPROM_LOAD_STATUS0); > + > + if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) == ANX7688_EEPROM_FW_LOADED) { > + dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret); > + dev_dbg(anx7688->dev, "fw loaded after %d ms\n", i * 10); > + break; > + } > + > + if (i > 99) { > + set_bit(ANX7688_F_FW_FAILED, anx7688->flags); > + dev_err(anx7688->dev, > + "boot firmware load failed (you may need to flash FW to anx7688 first)\n"); > + ret = -ETIMEDOUT; > + goto err_vconoff; > + } > + msleep(5); > + i++; > + } You need to use something like time_is_after_jiffies() here instead of a counter. > + ret = i2c_smbus_read_i2c_block_data(anx7688->client, > + ANX7688_REG_FW_VERSION1, 2, fw); > + if (ret < 0) { > + dev_err(anx7688->dev, "failed to read firmware version\n"); > + goto err_vconoff; > + } > + > + dev_dbg(anx7688->dev, "OCM firmware loaded (version 0x%04x)\n", > + fw[1] | fw[0] << 8); > + > + /* Unmask interrupts */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); > + if (ret) > + goto err_vconoff; > + > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT_MASK, ~ANX7688_SOFT_INT_MASK); > + if (ret) > + goto err_vconoff; > + > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, 0xff); > + if (ret) > + goto err_vconoff; > + > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_MASK2, (u8)~ANX7688_IRQ2_SOFT_INT); > + if (ret) > + goto err_vconoff; > + > + /* time to turn off vbus after cc disconnect (unit is 4 ms) */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_VBUS_OFF_DELAY_TIME, 100 / 4); > + if (ret) > + goto err_vconoff; > + > + /* 300ms (unit is 2 ms) */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_TRY_UFP_TIMER, 300 / 2); > + if (ret) > + goto err_vconoff; > + > + /* maximum voltage in 100 mV units */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_VOLTAGE, 50); /* 5 V */ > + if (ret) > + goto err_vconoff; > + > + /* min/max power in 500 mW units */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_POWER, 15 * 2); /* 15 W */ > + if (ret) > + goto err_vconoff; > + > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MIN_POWER, 1); /* 0.5 W */ > + if (ret) > + goto err_vconoff; > + > + /* auto_pd, try.src, try.sink, goto safe 5V */ > + ret = anx7688_reg_write(anx7688, ANX7688_REG_FEATURE_CTRL, 0x1e & ~BIT(2)); // disable try_src Those two comments are obscure. This function seems to have lot of hard coded information above. Shouldn't much of that come from DT? Please note that if you have that PinePhone specific glue driver, you can get much of the hard coded information from there, if getting the information from DT is not an option for what ever reason. > + if (ret) > + goto err_vconoff; > + > + for (i = 0; i < anx7688->n_src_caps; i++) > + caps[i] = cpu_to_le32(anx7688->src_caps[i]); > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PWR_SRC_CAP, > + (u8 *)&caps, 4 * anx7688->n_src_caps); > + if (ret) > + goto err_vconoff; > + > + for (i = 0; i < anx7688->n_snk_caps; i++) > + caps[i] = cpu_to_le32(anx7688->snk_caps[i]); > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PWR_SNK_CAP, > + (u8 *)&caps, 4 * anx7688->n_snk_caps); > + if (ret) > + goto err_vconoff; > + > + /* Send DP SNK identity */ > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_DP_SNK_IDENTITY, > + dp_snk_identity, sizeof(dp_snk_identity)); > + if (ret) > + goto err_vconoff; > + > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_SVID, > + svid, sizeof(svid)); > + if (ret) > + goto err_vconoff; > + > + dev_dbg(anx7688->dev, "OCM configuration completed\n"); > + > + desc.accessory = TYPEC_ACCESSORY_NONE; > + > + typec_unregister_partner(anx7688->partner); > + > + anx7688->partner = typec_register_partner(anx7688->port, &desc); > + if (IS_ERR(anx7688->partner)) { > + ret = PTR_ERR(anx7688->partner); > + goto err_vconoff; > + } > + > + // after this deadline passes we'll check if device is pd_capable and > + // set up the current limit accordingly > + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 3000); > + > + set_bit(ANX7688_F_CONNECTED, anx7688->flags); > + return 0; > + > +err_vconoff: > + regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); > + anx7688->vconn_on = false; > +err_poweroff: > + anx7688_power_disable(anx7688); > + dev_err(anx7688->dev, "OCM configuration failed\n"); > + return ret; > +} > + > +static void anx7688_set_hdmi_hpd(struct anx7688 *anx7688, int state) > +{ > + if (!anx7688->extcon) > + return; > + > + if (anx7688->last_extcon_state != state) { > + extcon_set_state_sync(anx7688->extcon, EXTCON_DISP_HDMI, state); > + anx7688->last_extcon_state = state; > + } > +} > + > +static void anx7688_disconnect(struct anx7688 *anx7688) > +{ > + union power_supply_propval val = {0,}; > + struct device *dev = anx7688->dev; > + int ret; > + > + dev_dbg(dev, "cable removed\n"); > + > + anx7688->current_update_deadline = 0; > + > + anx7688_set_hdmi_hpd(anx7688, 0); > + > + if (anx7688->vconn_on) { > + regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); > + anx7688->vconn_on = false; > + } > + > + if (anx7688->vbus_on) { > + regulator_disable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); > + anx7688->vbus_on = false; > + } > + > + anx7688_power_disable(anx7688); > + > + anx7688->pd_capable = false; > + > + typec_unregister_partner(anx7688->partner); > + anx7688->partner = NULL; > + > + anx7688->pwr_role = TYPEC_SINK; > + anx7688->data_role = TYPEC_DEVICE; > + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); > + typec_set_data_role(anx7688->port, anx7688->data_role); > + typec_set_pwr_opmode(anx7688->port, TYPEC_PWR_MODE_USB); > + typec_set_vconn_role(anx7688->port, TYPEC_SINK); > + > + usb_role_switch_set_role(anx7688->role_sw, USB_ROLE_NONE); > + > + val.intval = 500 * 1000; > + dev_dbg(dev, "setting vbus_in current limit to %d mA\n", val.intval); > + ret = power_supply_set_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, > + &val); > + if (ret) > + dev_err(dev, "failed to set vbus_in current to %d mA\n", > + val.intval / 1000); > + > + val.intval = 0; > + dev_dbg(dev, "disabling vbus_in power path\n"); > + ret = power_supply_set_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_ONLINE, > + &val); > + if (ret) > + dev_err(dev, "failed to offline vbus_in\n"); > + > + dev_dbg(dev, "enabling USB BC 1.2 detection\n"); > + > + clear_bit(ANX7688_F_CONNECTED, anx7688->flags); > +} > + > +static void anx7688_handle_cable_change(struct anx7688 *anx7688) > +{ > + int cabledet; > + bool connected; > + > + connected = test_bit(ANX7688_F_CONNECTED, anx7688->flags); > + cabledet = gpiod_get_value(anx7688->gpio_cabledet); > + > + if (cabledet && !connected) > + anx7688_connect(anx7688); > + else if (!cabledet && connected) > + anx7688_disconnect(anx7688); > +} > + > +static irqreturn_t anx7688_irq_plug_handler(int irq, void *data) > +{ > + struct anx7688 *anx7688 = data; > + > + dev_dbg(anx7688->dev, "plug irq (cd=%d)\n", > + gpiod_get_value(anx7688->gpio_cabledet)); > + > + /* > + * After each cabledet change the scheduled work timer is reset > + * to fire in ~10ms. So the work is done only after the cabledet > + * is stable for ~10ms. > + */ > + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(10)); > + > + return IRQ_HANDLED; > +} > + > +enum { > + CMD_SUCCESS, > + CMD_REJECT, > + CMD_FAIL, > + CMD_BUSY, > +}; > + > +static const char * const anx7688_cmd_statuses[] = { > + "SUCCESS", > + "REJECT", > + "FAIL", > + "BUSY", > +}; > + > + Extra newline. > +static int anx7688_handle_pd_message(struct anx7688 *anx7688, > + u8 cmd, u8 *msg, unsigned int len) > +{ > + union power_supply_propval psy_val = {0,}; > + u32 *pdos = (u32 *)msg; > + int ret, i, rdo_max_v, rdo_max_p; > + u32 pdo, rdo; > + > + switch (cmd) { > + case ANX7688_OCM_MSG_PWR_SRC_CAP: > + dev_dbg(anx7688->dev, "received SRC_CAP\n"); > + > + if (len % 4 != 0) { > + dev_warn(anx7688->dev, "received invalid sized PDO array\n"); > + break; > + } > + > + /* the partner is PD capable */ > + anx7688->pd_capable = true; > + > + for (i = 0; i < len / 4; i++) { > + pdo = le32_to_cpu(pdos[i]); > + > + if (pdo_type(pdo) == PDO_TYPE_FIXED) { > + unsigned int voltage = pdo_fixed_voltage(pdo); > + unsigned int max_curr = pdo_max_current(pdo); > + > + dev_dbg(anx7688->dev, "SRC_CAP PDO_FIXED (%umV %umA)\n", > + voltage, max_curr); > + } else if (pdo_type(pdo) == PDO_TYPE_BATT) { > + unsigned int min_volt = pdo_min_voltage(pdo); > + unsigned int max_volt = pdo_max_voltage(pdo); > + unsigned int max_pow = pdo_max_power(pdo); > + > + dev_dbg(anx7688->dev, "SRC_CAP PDO_BATT (%umV-%umV %umW)\n", > + min_volt, max_volt, max_pow); > + } else if (pdo_type(pdo) == PDO_TYPE_VAR) { > + unsigned int min_volt = pdo_min_voltage(pdo); > + unsigned int max_volt = pdo_max_voltage(pdo); > + unsigned int max_curr = pdo_max_current(pdo); > + > + dev_dbg(anx7688->dev, "SRC_CAP PDO_VAR (%umV-%umV %umA)\n", > + min_volt, max_volt, max_curr); > + } else { > + dev_dbg(anx7688->dev, "SRC_CAP PDO_APDO (0x%08X)\n", pdo); > + } > + } > + > + /* when auto_pd mode is enabled, the FW has already set > + * RDO_MAX_VOLTAGE and RDO_MAX_POWER for the RDO it sent to the > + * partner based on the received SOURCE_CAPs. This does not > + * mean, the request was acked, but we can't do better here than > + * calculate the current_limit to set later and hope for the best. > + */ > + rdo_max_v = anx7688_reg_read(anx7688, ANX7688_REG_MAX_VOLTAGE_STATUS); > + if (rdo_max_v < 0) > + return rdo_max_v; > + if (rdo_max_v == 0) > + return -EINVAL; > + > + rdo_max_p = anx7688_reg_read(anx7688, ANX7688_REG_MAX_POWER_STATUS); > + if (rdo_max_p < 0) > + return rdo_max_p; > + > + anx7688->pd_current_limit = rdo_max_p * 5000 / rdo_max_v; > + > + dev_dbg(anx7688->dev, "RDO max voltage = %dmV, max power = %dmW, PD current limit = %dmA\n", > + rdo_max_v * 100, rdo_max_p * 500, anx7688->pd_current_limit); > + > + // update current limit sooner, now that we have PD negotiation result > + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 500); > + break; > + > + case ANX7688_OCM_MSG_PWR_SNK_CAP: > + dev_dbg(anx7688->dev, "received SNK_CAP\n"); > + > + if (len % 4 != 0) { > + dev_warn(anx7688->dev, "received invalid sized PDO array\n"); > + break; > + } > + > + for (i = 0; i < len / 4; i++) { > + pdo = le32_to_cpu(pdos[i]); > + > + if (pdo_type(pdo) == PDO_TYPE_FIXED) { > + unsigned int voltage = pdo_fixed_voltage(pdo); > + unsigned int max_curr = pdo_max_current(pdo); > + > + dev_dbg(anx7688->dev, "SNK_CAP PDO_FIXED (%umV %umA)\n", > + voltage, max_curr); > + } else if (pdo_type(pdo) == PDO_TYPE_BATT) { > + unsigned int min_volt = pdo_min_voltage(pdo); > + unsigned int max_volt = pdo_max_voltage(pdo); > + unsigned int max_pow = pdo_max_power(pdo); > + > + dev_dbg(anx7688->dev, "SNK_CAP PDO_BATT (%umV-%umV %umW)\n", > + min_volt, max_volt, max_pow); > + } else if (pdo_type(pdo) == PDO_TYPE_VAR) { > + unsigned int min_volt = pdo_min_voltage(pdo); > + unsigned int max_volt = pdo_max_voltage(pdo); > + unsigned int max_curr = pdo_max_current(pdo); > + > + dev_dbg(anx7688->dev, "SNK_CAP PDO_VAR (%umV-%umV %umA)\n", > + min_volt, max_volt, max_curr); > + } else { > + dev_dbg(anx7688->dev, "SNK_CAP PDO_APDO (0x%08X)\n", pdo); > + } > + } > + > + break; > + > + case ANX7688_OCM_MSG_PWR_OBJ_REQ: > + dev_dbg(anx7688->dev, "received PWR_OBJ_REQ\n"); > + > + anx7688->pd_capable = true; > + > + if (len != 4) { > + dev_warn(anx7688->dev, "received invalid sized RDO\n"); > + break; > + } > + > + rdo = le32_to_cpu(pdos[0]); > + > + if (rdo_index(rdo) >= 1 && rdo_index(rdo) <= anx7688->n_src_caps) { > + unsigned int rdo_op_curr = rdo_op_current(rdo); > + unsigned int rdo_max_curr = rdo_max_current(rdo); > + unsigned int rdo_idx = rdo_index(rdo) - 1; > + unsigned int pdo_volt, pdo_max_curr; > + > + pdo = anx7688->src_caps[rdo_idx]; > + pdo_volt = pdo_fixed_voltage(pdo); > + pdo_max_curr = pdo_max_current(pdo); > + > + dev_dbg(anx7688->dev, "RDO (idx=%d op=%umA max=%umA)\n", > + rdo_idx, rdo_op_curr, rdo_max_curr); > + > + dev_dbg(anx7688->dev, "PDO_FIXED (%umV %umA)\n", > + pdo_volt, pdo_max_curr); > + > + // We could check the req and respond with accept/reject > + // but we're using auto_pd feature, so the FW will do > + // this for us > + } else { > + dev_dbg(anx7688->dev, > + "PWR_OBJ RDO index out of range (RDO = 0x%08X)\n", rdo); > + } > + > + break; > + > + case ANX7688_OCM_MSG_RESPONSE_TO_REQ: > + if (len < 2) { > + dev_warn(anx7688->dev, "received short RESPONSE_TO_REQ\n"); > + break; > + } > + break; > + > + case ANX7688_OCM_MSG_HARD_RST: > + if (anx7688->pd_capable) { > + dev_dbg(anx7688->dev, "received HARD_RST\n"); > + > + // stop drawing power from VBUS > + psy_val.intval = 0; > + ret = power_supply_set_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_ONLINE, > + &psy_val); > + if (ret) > + dev_err(anx7688->dev, "failed to offline vbus_in\n"); > + > + // wait till the dust settles > + anx7688->current_update_deadline = ktime_add_ms(ktime_get(), 3000); > + } else { > + dev_dbg(anx7688->dev, "received HARD_RST, idiot firmware is bored\n"); > + } > + > + break; > + > + case ANX7688_OCM_MSG_ACCEPT: > + case ANX7688_OCM_MSG_REJECT: > + case ANX7688_OCM_MSG_SOFT_RST: > + case ANX7688_OCM_MSG_RESTART: > + case ANX7688_OCM_MSG_PSWAP_REQ: > + case ANX7688_OCM_MSG_DSWAP_REQ: > + case ANX7688_OCM_MSG_VCONN_SWAP_REQ: > + case ANX7688_OCM_MSG_DP_ALT_ENTER: > + case ANX7688_OCM_MSG_DP_ALT_EXIT: > + case ANX7688_OCM_MSG_DP_SNK_IDENTITY: > + case ANX7688_OCM_MSG_SVID: > + case ANX7688_OCM_MSG_VDM: > + case ANX7688_OCM_MSG_GOTO_MIN_REQ: > + case ANX7688_OCM_MSG_PD_STATUS_REQ: > + case ANX7688_OCM_MSG_GET_DP_SNK_CAP: > + case ANX7688_OCM_MSG_DP_SNK_CFG: > + dev_dbg(anx7688->dev, "received message 0x%02x\n", cmd); > + break; > + > + default: > + dev_dbg(anx7688->dev, "received unknown message 0x%02x\n", cmd); > + break; > + } > + > + return 0; > +} > + > +static int anx7688_receive_msg(struct anx7688 *anx7688) > +{ > + u8 pkt[32], checksum = 0; > + int i, ret; > + > + ret = i2c_smbus_read_i2c_block_data(anx7688->client_tcpc, > + ANX7688_TCPC_REG_INTERFACE_RECV, > + 32, pkt); > + if (ret < 0) { > + dev_err(anx7688->dev, "failed to read pd msg\n"); > + return ret; > + } > + > + ret = anx7688_tcpc_reg_write(anx7688, ANX7688_TCPC_REG_INTERFACE_RECV, 0); > + if (ret) > + dev_warn(anx7688->dev, "failed to clear recv FIFO\n"); > + > + if (pkt[0] == 0 || pkt[0] > sizeof(pkt) - 2) { > + dev_err(anx7688->dev, "received invalid pd message: %*ph\n", > + (int)sizeof(pkt), pkt); > + return -EINVAL; > + } > + > + dev_dbg(anx7688->dev, "recv ocm message cmd=0x%02x %*ph\n", > + pkt[1], pkt[0] + 2, pkt); > + > + for (i = 0; i < pkt[0] + 2; i++) > + checksum += pkt[i]; > + > + if (checksum != 0) { > + dev_err(anx7688->dev, "bad checksum on received message\n"); > + return -EINVAL; > + } > + > + anx7688_handle_pd_message(anx7688, pkt[1], pkt + 2, pkt[0] - 1); > + return 0; > +} > + > +static const char *anx7688_cc_status_string(unsigned int v) > +{ > + switch (v) { > + case 0: return "SRC.Open"; > + case 1: return "SRC.Rd"; > + case 2: return "SRC.Ra"; > + case 4: return "SNK.Default"; > + case 8: return "SNK.Power1.5"; > + case 12: return "SNK.Power3.0"; > + default: return "UNK"; > + } > +} > + > +static int anx7688_update_status(struct anx7688 *anx7688) > +{ > + struct device *dev = anx7688->dev; > + bool vbus_on, vconn_on, dr_dfp; > + int status, cc_status, dp_state, dp_substate, ret; > + > + status = anx7688_reg_read(anx7688, ANX7688_REG_STATUS); > + if (status < 0) > + return status; > + > + cc_status = anx7688_reg_read(anx7688, ANX7688_REG_CC_STATUS); > + if (cc_status < 0) > + return cc_status; > + > + dp_state = anx7688_tcpc_reg_read(anx7688, 0x87); > + if (dp_state < 0) > + return dp_state; > + > + dp_substate = anx7688_tcpc_reg_read(anx7688, 0x88); > + if (dp_substate < 0) > + return dp_substate; > + > + anx7688_set_hdmi_hpd(anx7688, dp_state >= 3); > + > + dp_state = (dp_state << 8) | dp_substate; > + > + if (anx7688->last_status == -1 || anx7688->last_status != status) { > + anx7688->last_status = status; > + dev_dbg(dev, "status changed to 0x%02x\n", status); > + } > + > + if (anx7688->last_cc_status == -1 || anx7688->last_cc_status != cc_status) { > + anx7688->last_cc_status = cc_status; > + dev_dbg(dev, "cc_status changed to CC1 = %s CC2 = %s\n", > + anx7688_cc_status_string(cc_status & 0xf), > + anx7688_cc_status_string((cc_status >> 4) & 0xf)); > + } > + > + if (anx7688->last_dp_state == -1 || anx7688->last_dp_state != dp_state) { > + anx7688->last_dp_state = dp_state; > + dev_dbg(dev, "DP state changed to 0x%04x\n", dp_state); > + } > + > + vbus_on = !!(status & ANX7688_VBUS_STATUS); > + vconn_on = !!(status & ANX7688_VCONN_STATUS); > + dr_dfp = !!(status & ANX7688_DATA_ROLE_STATUS); > + > + if (anx7688->vbus_on != vbus_on) { > + dev_dbg(anx7688->dev, "POWER role change to %s\n", > + vbus_on ? "SOURCE" : "SINK"); > + > + if (vbus_on) { > + ret = regulator_enable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); > + if (ret) { > + dev_err(anx7688->dev, "failed to enable vbus\n"); > + return ret; > + } > + } else { > + ret = regulator_disable(anx7688->supplies[ANX7688_VBUS_INDEX].consumer); > + if (ret) { > + dev_err(anx7688->dev, "failed to disable vbus\n"); > + return ret; > + } > + } > + > + anx7688->pwr_role = vbus_on ? TYPEC_SOURCE : TYPEC_SINK; > + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); > + anx7688->vbus_on = vbus_on; > + } > + > + if (anx7688->vconn_on != vconn_on) { > + dev_dbg(anx7688->dev, "VCONN role change to %s\n", > + vconn_on ? "SOURCE" : "SINK"); > + > + if (vconn_on) { > + ret = regulator_enable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); > + if (ret) { > + dev_err(anx7688->dev, "failed to enable vconn\n"); > + return ret; > + } > + } else { > + ret = regulator_disable(anx7688->supplies[ANX7688_VCONN_INDEX].consumer); > + if (ret) { > + dev_err(anx7688->dev, "failed to disable vconn\n"); > + return ret; > + } > + } > + > + typec_set_vconn_role(anx7688->port, vconn_on ? TYPEC_SOURCE : TYPEC_SINK); > + anx7688->vconn_on = vconn_on; > + } > + > + anx7688->data_role = dr_dfp ? TYPEC_HOST : TYPEC_DEVICE; > + typec_set_data_role(anx7688->port, anx7688->data_role); > + > + if (usb_role_switch_get_role(anx7688->role_sw) != > + (dr_dfp ? USB_ROLE_HOST : USB_ROLE_DEVICE)) { > + dev_dbg(anx7688->dev, "DATA role change requested to %s\n", > + dr_dfp ? "DFP" : "UFP"); > + > + ret = usb_role_switch_set_role(anx7688->role_sw, > + dr_dfp ? USB_ROLE_HOST : USB_ROLE_DEVICE); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static irqreturn_t anx7688_irq_status_handler(int irq, void *data) > +{ > + struct anx7688 *anx7688 = data; > + struct device *dev = anx7688->dev; > + int tcpc_status, ext2_status, soft_status; > + > + mutex_lock(&anx7688->lock); > + > + if (!test_bit(ANX7688_F_CONNECTED, anx7688->flags)) { > + dev_dbg(dev, "spurious status irq\n"); > + /* > + * anx chip should be disabled and power off, nothing > + * more to do > + */ > + goto out_unlock; > + } > + > + // clear tcpc interrupt > + tcpc_status = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_ALERT0); > + if (tcpc_status > 0) > + anx7688_tcpc_reg_write(anx7688, ANX7688_TCPC_REG_ALERT0, tcpc_status); > + > + ext2_status = anx7688_reg_read(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2); > + if (ext2_status & ANX7688_IRQ2_SOFT_INT) { > + soft_status = anx7688_reg_read(anx7688, ANX7688_REG_STATUS_INT); > + anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); > + > + if (soft_status > 0) { > + soft_status &= ANX7688_SOFT_INT_MASK; > + > + if (soft_status & ANX7688_IRQS_RECEIVED_MSG) > + anx7688_receive_msg(anx7688); > + > + if (soft_status & (ANX7688_IRQS_CC_STATUS_CHANGE | > + ANX7688_IRQS_VBUS_CHANGE | > + ANX7688_IRQS_VCONN_CHANGE | > + ANX7688_IRQS_DATA_ROLE_CHANGE)) { > + anx7688_update_status(anx7688); > + } > + } > + > + anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, ANX7688_IRQ2_SOFT_INT); > + } > + > +out_unlock: > + mutex_unlock(&anx7688->lock); > + > + return IRQ_HANDLED; > +} > + > +static int anx7688_dr_set(struct typec_port *port, enum typec_data_role role) > +{ > + struct anx7688 *anx7688 = typec_get_drvdata(port); > + int ret = 0; > + > + dev_dbg(anx7688->dev, "data role set %d\n", role); > + > + if (anx7688->data_role != role) { > + mutex_lock(&anx7688->lock); > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_DSWAP_REQ, 0, 0); > + mutex_unlock(&anx7688->lock); > + } > + > + return ret; > +} > + > +static int anx7688_pr_set(struct typec_port *port, enum typec_role role) > +{ > + struct anx7688 *anx7688 = typec_get_drvdata(port); > + int ret = 0; > + > + dev_dbg(anx7688->dev, "power role set %d\n", role); > + > + if (anx7688->pwr_role != role) { > + mutex_lock(&anx7688->lock); > + ret = anx7688_send_ocm_message(anx7688, ANX7688_OCM_MSG_PSWAP_REQ, 0, 0); > + mutex_unlock(&anx7688->lock); > + } > + > + return ret; > +} > + > +/* > + * Calls to the EEPROM functions need to be taken under the anx7688 lock. > + */ > +static int anx7688_eeprom_set_address(struct anx7688 *anx7688, u16 addr) > +{ > + int ret; > + > + ret = anx7688_reg_write(anx7688, 0xe0, (addr >> 8) & 0xff); > + if (ret < 0) > + return ret; > + > + return anx7688_reg_write(anx7688, 0xe1, addr & 0xff); > +} > + > +static int anx7688_eeprom_wait_done(struct anx7688 *anx7688) > +{ > + ktime_t timeout; > + int ret; > + > + // wait for read to be done > + timeout = ktime_add_us(ktime_get(), 50000); > + while (true) { > + ret = anx7688_reg_read(anx7688, 0xe2); > + if (ret < 0) > + return ret; > + > + if (ret & BIT(3)) > + return 0; > + > + if (ktime_after(ktime_get(), timeout)) { > + dev_err(anx7688->dev, "timeout waiting for eeprom\n"); > + return -ETIMEDOUT; > + } > + } > +} > + > +/* > + * Wait for internal FSM of EEPROM to be in a state ready for > + * programming/reading > + */ > +static int anx7688_eeprom_wait_ready(struct anx7688 *anx7688) > +{ > + ktime_t timeout; > + int ret; > + > + // wait until eeprom is ready > + timeout = ktime_add_us(ktime_get(), 1000000); > + while (true) { > + ret = anx7688_reg_read(anx7688, 0x7f); > + if (ret < 0) > + return ret; > + > + if ((ret & 0x0f) == 7) > + return 0; > + > + if (ktime_after(ktime_get(), timeout)) { > + dev_err(anx7688->dev, "timeout waiting for eeprom to initialize\n"); > + return -ETIMEDOUT; > + } > + > + msleep(5); > + } > +} > + > +static int anx7688_eeprom_read(struct anx7688 *anx7688, unsigned int addr, u8 buf[16]) > +{ > + int ret; > + > + ret = anx7688_eeprom_set_address(anx7688, addr); > + if (ret) > + return ret; > + > + // initiate read > + ret = anx7688_reg_write(anx7688, 0xe2, 0x06); > + if (ret < 0) > + return ret; > + > + ret = anx7688_eeprom_wait_done(anx7688); > + if (ret) > + return ret; > + > + ret = i2c_smbus_read_i2c_block_data(anx7688->client, 0xd0, 16, buf); > + if (ret < 0) { > + dev_err(anx7688->dev, > + "failed to read eeprom data (err=%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static const struct typec_operations anx7688_typec_ops = { > + .dr_set = anx7688_dr_set, > + .pr_set = anx7688_pr_set, > +}; > + > +/* > + * This function has to work when the ANX7688 is active, and when > + * it is powered down. It power cycles the chip and asserts the OCM > + * reset, to prevent OCM FW interfering with EEPROM reading. > + * > + * After reading EEPROM, the reconnection is scheduled. > + */ > +static int anx7688_firmware_show(struct seq_file *s, void *data) > +{ > + struct anx7688 *anx7688 = s->private; > + unsigned int addr; > + u8 buf[16]; > + int ret; > + > + mutex_lock(&anx7688->lock); > + > + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) > + anx7688_disconnect(anx7688); > + > + msleep(20); > + > + anx7688_power_enable(anx7688); > + > + ret = anx7688_reg_update_bits(anx7688, ANX7688_REG_USBC_RESET_CTRL, > + ANX7688_USBC_RESET_CTRL_OCM_RESET, > + ANX7688_USBC_RESET_CTRL_OCM_RESET); > + if (ret < 0) > + goto out_powerdown; > + > + ret = anx7688_eeprom_wait_ready(anx7688); > + if (ret) > + goto out_powerdown; > + > + msleep(10); > + > + for (addr = 0x10; addr < 0x10000; addr += 16) { > + // set address > + ret = anx7688_eeprom_read(anx7688, addr, buf); > + if (ret < 0) > + goto out_powerdown; > + > + seq_write(s, buf, sizeof(buf)); > + } > + > +out_powerdown: > + anx7688_power_disable(anx7688); > + schedule_delayed_work(&anx7688->work, 0); > + mutex_unlock(&anx7688->lock); > + > + return ret; > +} > +DEFINE_SHOW_ATTRIBUTE(anx7688_firmware); > + > +static int anx7688_regs_show(struct seq_file *s, void *data) > +{ > + struct anx7688 *anx7688 = s->private; > + u8 buf[16]; > + unsigned int i, addr; > + int ret = -ENODEV; > + > + mutex_lock(&anx7688->lock); > + > + if (!test_bit(ANX7688_F_POWERED, anx7688->flags)) > + goto out_unlock; > + > + for (addr = 0; addr < 256; addr += 16) { > + ret = i2c_smbus_read_i2c_block_data(anx7688->client, addr, > + sizeof(buf), buf); > + if (ret < 0) { > + dev_dbg(anx7688->dev, > + "failed to read registers (err=%d)\n", ret); > + goto out_unlock; > + } > + > + for (i = 0; i < 16; i++) > + seq_printf(s, "50%02x: %02x\n", addr + i, buf[i]); > + } > + > + for (addr = 0; addr < 256; addr += 16) { > + ret = i2c_smbus_read_i2c_block_data(anx7688->client_tcpc, addr, > + sizeof(buf), buf); > + if (ret < 0) { > + dev_dbg(anx7688->dev, > + "failed to read registers (err=%d)\n", ret); > + goto out_unlock; > + } > + > + for (i = 0; i < 16; i++) > + seq_printf(s, "58%02x: %02x\n", addr + i, buf[i]); > + } > + > +out_unlock: > + mutex_unlock(&anx7688->lock); > + > + return ret; > +} > +DEFINE_SHOW_ATTRIBUTE(anx7688_regs); > + > +/* > + * This is just a 1s watchdog checking the state if cabledet pin. > + */ > +static void anx7688_cabledet_timer_fn(struct timer_list *t) > +{ > + struct anx7688 *anx7688 = from_timer(anx7688, t, work_timer); > + > + schedule_delayed_work(&anx7688->work, 0); > + mod_timer(t, jiffies + msecs_to_jiffies(1000)); > +} > + > +static void anx7688_handle_vbus_in_notify(struct anx7688 *anx7688) > +{ > + union power_supply_propval psy_val = {0,}; > + struct device *dev = anx7688->dev; > + int ret; > + > + ret = power_supply_get_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_USB_TYPE, > + &psy_val); > + if (ret) { > + dev_err(dev, "failed to get USB BC1.2 result\n"); > + return; > + } > + > + if (anx7688->last_bc_result == psy_val.intval) > + return; > + > + anx7688->last_bc_result = psy_val.intval; > + > + switch (psy_val.intval) { > + case POWER_SUPPLY_USB_TYPE_DCP: > + case POWER_SUPPLY_USB_TYPE_CDP: > + dev_dbg(dev, "BC 1.2 result: DCP or CDP\n"); > + break; > + case POWER_SUPPLY_USB_TYPE_SDP: > + default: > + dev_dbg(dev, "BC 1.2 result: SDP\n"); > + break; > + } > +} > + > +static int anx7688_cc_status(unsigned int v) > +{ > + switch (v) { > + case 0: return -1; > + case 1: return -1; > + case 2: return -1; > + case 4: return TYPEC_PWR_MODE_USB; > + case 8: return TYPEC_PWR_MODE_1_5A; > + case 12: return TYPEC_PWR_MODE_3_0A; > + default: return -1; > + } > +} > + > +static const char *anx7688_get_power_mode_name(enum typec_pwr_opmode mode) > +{ > + switch (mode) { > + case TYPEC_PWR_MODE_USB: return "USB"; > + case TYPEC_PWR_MODE_1_5A: return "1.5A"; > + case TYPEC_PWR_MODE_3_0A: return "3.0A"; > + case TYPEC_PWR_MODE_PD: return "PD"; > + default: return "Unknown"; > + } > +} > + > +/* > + * This is called after 500ms after connection when the PD contract should have > + * been negotiated. We should inspect CC pins or PD status here and decide what > + * input current limit to set. > + */ > +static void anx7688_handle_current_update(struct anx7688 *anx7688) > +{ > + unsigned int cc_status = anx7688->last_cc_status; > + union power_supply_propval val = {0,}; > + struct device *dev = anx7688->dev; > + int pwr_mode, ret, current_limit = 0; > + > + if (anx7688->pd_capable) { > + pwr_mode = TYPEC_PWR_MODE_PD; > + } else if (cc_status < 0) { > + pwr_mode = TYPEC_PWR_MODE_USB; > + } else { > + pwr_mode = anx7688_cc_status(cc_status & 0xf); > + if (pwr_mode < 0) > + pwr_mode = anx7688_cc_status((cc_status >> 4) & 0xf); > + if (pwr_mode < 0) > + pwr_mode = TYPEC_PWR_MODE_USB; > + } > + > + if (pwr_mode == TYPEC_PWR_MODE_1_5A) > + current_limit = 1500; > + else if (pwr_mode == TYPEC_PWR_MODE_3_0A) > + current_limit = 3000; > + else if (pwr_mode == TYPEC_PWR_MODE_PD) > + current_limit = anx7688->pd_current_limit; > + > + anx7688->input_current_limit = current_limit; > + > + dev_dbg(anx7688->dev, "updating power mode to %s, current limit %dmA (0 => BC1.2)\n", > + anx7688_get_power_mode_name(pwr_mode), current_limit); > + > + if (current_limit) { > + /* > + * Disable BC1.2 detection, because we'll be setting > + * a current limit determined by USB-PD > + */ > + dev_dbg(dev, "disabling USB BC 1.2 detection\n"); > + > + val.intval = current_limit * 1000; > + dev_dbg(dev, "setting vbus_in current limit to %d mA\n", current_limit); > + ret = power_supply_set_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, > + &val); > + if (ret) > + dev_err(dev, "failed to set vbus_in current to %d mA\n", > + current_limit); > + } > + /* > + * else use the result of BC1.2 detection performed by PMIC. > + */ > + > + /* Turn on VBUS power path inside PMIC. */ > + val.intval = 1; > + dev_dbg(dev, "enabling vbus_in power path\n"); > + ret = power_supply_set_property(anx7688->vbus_in_supply, > + POWER_SUPPLY_PROP_ONLINE, > + &val); > + if (ret) > + dev_err(anx7688->dev, "failed to enable vbus_in\n"); > + > + typec_set_pwr_opmode(anx7688->port, pwr_mode); > +} > + > +static int anx7688_vbus_in_notify(struct notifier_block *nb, > + unsigned long val, void *v) > +{ > + struct anx7688 *anx7688 = container_of(nb, struct anx7688, vbus_in_nb); > + struct power_supply *psy = v; > + > + /* atomic context */ > + if (val == PSY_EVENT_PROP_CHANGED && psy == anx7688->vbus_in_supply) { > + set_bit(ANX7688_F_PWRSUPPLY_CHANGE, anx7688->flags); > + schedule_delayed_work(&anx7688->work, 0); > + } > + > + return NOTIFY_OK; > +} > + > +static void anx7688_work(struct work_struct *work) > +{ > + struct anx7688 *anx7688 = container_of(work, struct anx7688, work.work); > + > + if (test_bit(ANX7688_F_FW_FAILED, anx7688->flags)) > + return; > + > + mutex_lock(&anx7688->lock); > + > + if (test_and_clear_bit(ANX7688_F_PWRSUPPLY_CHANGE, anx7688->flags)) > + anx7688_handle_vbus_in_notify(anx7688); > + > + anx7688_handle_cable_change(anx7688); > + > + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) { > + /* > + * We check status periodically outside of interrupt, just to > + * be sure we didn't miss any status interrupts > + */ > + anx7688_update_status(anx7688); > + > + if (anx7688->current_update_deadline && > + ktime_after(ktime_get(), anx7688->current_update_deadline)) { > + anx7688->current_update_deadline = 0; > + anx7688_handle_current_update(anx7688); > + } > + } > + > + mutex_unlock(&anx7688->lock); > +} > + > +static int anx7688_parse_connector(struct device *dev, struct anx7688 *anx7688, struct typec_capability *cap) > +{ > + struct fwnode_handle *fwnode; > + const char *buf; > + int ret; > + > + fwnode = device_get_named_child_node(dev, "connector"); > + if (!fwnode) > + return -EINVAL; > + > + ret = fwnode_property_read_string(fwnode, "power-role", &buf); > + if (ret) { > + dev_err(dev, "power-role not found: %d\n", ret); > + return ret; > + } > + > + ret = typec_find_port_power_role(buf); > + if (ret < 0) > + return ret; > + cap->type = ret; > + > + ret = fwnode_property_read_string(fwnode, "data-role", &buf); > + if (ret) { > + dev_err(dev, "data-role not found: %d\n", ret); > + return ret; > + } > + > + ret = typec_find_port_data_role(buf); > + if (ret < 0) > + return ret; > + cap->data = ret; > + > + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf); > + if (ret) { > + dev_err(dev, "try-power-role not found: %d\n", ret); > + return ret; > + } > + > + ret = typec_find_power_role(buf); > + if (ret < 0) > + return ret; > + cap->prefer_role = ret; > + > + /* Get source pdos */ > + ret = fwnode_property_count_u32(fwnode, "source-pdos"); > + if (ret <= 0) > + return -EINVAL; > + > + anx7688->n_src_caps = min_t(u8, ret, ARRAY_SIZE(anx7688->src_caps)); > + ret = fwnode_property_read_u32_array(fwnode, "source-pdos", > + anx7688->src_caps, > + anx7688->n_src_caps); > + if (ret < 0) { > + dev_err(dev, "source cap validate failed: %d\n", ret); > + return ret; > + } > + > + ret = fwnode_property_count_u32(fwnode, "sink-pdos"); > + if (ret <= 0) > + return -EINVAL; > + > + anx7688->n_snk_caps = min_t(u8, ret, ARRAY_SIZE(anx7688->snk_caps)); > + ret = fwnode_property_read_u32_array(fwnode, "sink-pdos", > + anx7688->snk_caps, > + anx7688->n_snk_caps); > + if (ret < 0) { > + dev_err(dev, "sink cap validate failed: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int anx7688_i2c_probe(struct i2c_client *client) > +{ > + struct anx7688 *anx7688; > + struct device *dev = &client->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 = &client->dev; > + mutex_init(&anx7688->lock); > + INIT_DELAYED_WORK(&anx7688->work, anx7688_work); > + anx7688->last_extcon_state = -1; > + > + ret = anx7688_parse_connector(dev, anx7688, &typec_cap); > + if (ret < 0) { > + dev_err(dev, "failed to parse connector from DT\n"); > + return ret; > + } > + > + for (i = 0; i < ANX7688_NUM_SUPPLIES; i++) > + anx7688->supplies[i].supply = anx7688_supply_names[i]; > + ret = devm_regulator_bulk_get(dev, ANX7688_NUM_SUPPLIES, > + anx7688->supplies); > + if (ret) > + return ret; > + > + anx7688->vbus_in_supply = > + devm_power_supply_get_by_phandle(dev, "vbus-in-supply"); > + if (IS_ERR(anx7688->vbus_in_supply)) { > + dev_err(dev, "Couldn't get the VBUS power supply\n"); > + return PTR_ERR(anx7688->vbus_in_supply); > + } > + > + if (!anx7688->vbus_in_supply) > + return -EPROBE_DEFER; > + > + anx7688->gpio_enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(anx7688->gpio_enable)) { > + dev_err(dev, "Could not get enable gpio\n"); > + return PTR_ERR(anx7688->gpio_enable); > + } > + > + anx7688->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(anx7688->gpio_reset)) { > + dev_err(dev, "Could not get reset gpio\n"); > + return PTR_ERR(anx7688->gpio_reset); > + } > + > + anx7688->gpio_cabledet = devm_gpiod_get(dev, "cabledet", GPIOD_IN); > + if (IS_ERR(anx7688->gpio_cabledet)) { > + dev_err(dev, "Could not get cabledet gpio\n"); > + return PTR_ERR(anx7688->gpio_cabledet); > + } > + > + irq_cabledet = gpiod_to_irq(anx7688->gpio_cabledet); > + if (irq_cabledet < 0) { > + dev_err(dev, "Could not get cabledet irq\n"); > + return irq_cabledet; > + } > + > + // Initialize extcon device > + anx7688->extcon = devm_extcon_dev_allocate(dev, anx7688_extcon_cable); > + if (IS_ERR(anx7688->extcon)) > + return -ENOMEM; > + > + ret = devm_extcon_dev_register(dev, anx7688->extcon); > + if (ret) { > + dev_err(dev, "failed to register extcon device\n"); > + return ret; > + } > + > + // Register the TCPC i2c interface as second interface (0x58) > + anx7688->client_tcpc = i2c_new_dummy_device(client->adapter, 0x2c); > + if (IS_ERR(anx7688->client_tcpc)) { > + dev_err(dev, "Could not register tcpc i2c client\n"); > + return PTR_ERR(anx7688->client_tcpc); > + } > + i2c_set_clientdata(anx7688->client_tcpc, anx7688); > + > + // powerup and probe the ANX chip > + > + ret = regulator_bulk_enable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, > + anx7688->supplies); > + if (ret) { > + dev_err(dev, "Could not enable regulators\n"); > + goto err_dummy_dev; > + } > + > + msleep(10); > + > + anx7688_power_enable(anx7688); > + > + vid_l = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_VENDOR_ID0); > + vid_h = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_VENDOR_ID1); > + if (vid_l < 0 || vid_h < 0) { > + ret = vid_l < 0 ? vid_l : vid_h; > + anx7688_power_disable(anx7688); > + goto err_disable_reg; > + } > + > + dev_dbg(dev, "Vendor id 0x%04x\n", vid_l | vid_h << 8); > + > + anx7688_power_disable(anx7688); > + > + anx7688->role_sw = usb_role_switch_get(dev); > + if (IS_ERR(anx7688->role_sw)) { > + dev_err(dev, "Could not get role switch\n"); > + ret = PTR_ERR(anx7688->role_sw); > + goto err_disable_reg; > + } > + > + // setup a typec port device > + typec_cap.revision = USB_TYPEC_REV_1_2; > + typec_cap.pd_revision = 0x200; > + ret = -EINVAL; > + if (typec_cap.type != TYPEC_PORT_DRP) > + goto err_disable_reg; > + if (typec_cap.data != TYPEC_PORT_DRD) > + goto err_disable_reg; > + typec_cap.driver_data = anx7688; > + typec_cap.ops = &anx7688_typec_ops; > + > + anx7688->port = typec_register_port(dev, &typec_cap); > + if (IS_ERR(anx7688->port)) { > + dev_err(dev, "Could not register type-c port\n"); > + ret = PTR_ERR(anx7688->port); > + goto err_role_sw; > + } > + > + anx7688->pwr_role = TYPEC_SINK; > + anx7688->data_role = TYPEC_DEVICE; > + typec_set_pwr_role(anx7688->port, anx7688->pwr_role); > + typec_set_data_role(anx7688->port, anx7688->data_role); > + typec_set_pwr_opmode(anx7688->port, TYPEC_PWR_MODE_USB); > + typec_set_vconn_role(anx7688->port, TYPEC_SINK); > + > + // make sure BC1.2 detection in PMIC is enabled > + anx7688->last_bc_result = -1; > + > + dev_dbg(dev, "enabling USB BC 1.2 detection\n"); > + > + ret = devm_request_irq(dev, irq_cabledet, anx7688_irq_plug_handler, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > + "anx7688-cabledet", anx7688); > + if (ret < 0) { > + dev_err(dev, "Could not request cabledet irq (%d)\n", ret); > + goto err_cport; > + } > + > + ret = devm_request_threaded_irq(dev, client->irq, > + NULL, anx7688_irq_status_handler, > + IRQF_ONESHOT, NULL, anx7688); > + if (ret < 0) { > + dev_err(dev, "Could not request irq (%d)\n", ret); > + goto err_cport; > + } > + > + anx7688->vbus_in_nb.notifier_call = anx7688_vbus_in_notify; > + anx7688->vbus_in_nb.priority = 0; > + ret = power_supply_reg_notifier(&anx7688->vbus_in_nb); > + if (ret) > + goto err_cport; > + > + anx7688->debug_root = debugfs_create_dir("anx7688", NULL); > + debugfs_create_file("firmware", 0444, anx7688->debug_root, anx7688, > + &anx7688_firmware_fops); > + debugfs_create_file("regs", 0444, anx7688->debug_root, anx7688, > + &anx7688_regs_fops); > + > + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(10)); > + > + timer_setup(&anx7688->work_timer, anx7688_cabledet_timer_fn, 0); > + mod_timer(&anx7688->work_timer, jiffies + msecs_to_jiffies(1000)); > + return 0; > + > +err_cport: > + typec_unregister_port(anx7688->port); > +err_role_sw: > + usb_role_switch_put(anx7688->role_sw); > +err_disable_reg: > + regulator_bulk_disable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, anx7688->supplies); > +err_dummy_dev: > + i2c_unregister_device(anx7688->client_tcpc); > + return ret; > +} > + > +static void anx7688_i2c_remove(struct i2c_client *client) > +{ > + struct anx7688 *anx7688 = i2c_get_clientdata(client); > + > + mutex_lock(&anx7688->lock); > + > + power_supply_unreg_notifier(&anx7688->vbus_in_nb); > + > + del_timer_sync(&anx7688->work_timer); > + > + cancel_delayed_work_sync(&anx7688->work); > + > + if (test_bit(ANX7688_F_CONNECTED, anx7688->flags)) > + anx7688_disconnect(anx7688); > + > + typec_unregister_partner(anx7688->partner); > + typec_unregister_port(anx7688->port); > + usb_role_switch_put(anx7688->role_sw); > + > + regulator_bulk_disable(ANX7688_NUM_ALWAYS_ON_SUPPLIES, anx7688->supplies); > + i2c_unregister_device(anx7688->client_tcpc); > + > + debugfs_remove(anx7688->debug_root); > + > + mutex_unlock(&anx7688->lock); > +} > + > +static int __maybe_unused anx7688_suspend(struct device *dev) > +{ > + struct anx7688 *anx7688 = i2c_get_clientdata(to_i2c_client(dev)); > + > + del_timer_sync(&anx7688->work_timer); > + cancel_delayed_work_sync(&anx7688->work); > + > + regulator_disable(anx7688->supplies[ANX7688_I2C_INDEX].consumer); > + > + return 0; > +} > + > +static int __maybe_unused anx7688_resume(struct device *dev) > +{ > + struct anx7688 *anx7688 = i2c_get_clientdata(to_i2c_client(dev)); > + int ret; > + > + ret = regulator_enable(anx7688->supplies[ANX7688_I2C_INDEX].consumer); > + if (ret) > + dev_warn(anx7688->dev, > + "failed to enable I2C regulator (%d)\n", ret); > + > + // check status right after resume, since it could have changed during > + // sleep > + schedule_delayed_work(&anx7688->work, msecs_to_jiffies(50)); > + mod_timer(&anx7688->work_timer, jiffies + msecs_to_jiffies(1000)); > + > + return 0; > +} > + > +static const struct dev_pm_ops anx7688_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(anx7688_suspend, anx7688_resume) > +}; > + > +static const struct i2c_device_id anx7688_ids[] = { > + { "anx7688", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, anx7688_ids); > + > +static const struct of_device_id anx7688_of_match_table[] = { > + { .compatible = "analogix,anx7688" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, anx7688_of_match_table); > + > +static struct i2c_driver anx7688_driver = { > + .driver = { > + .name = "anx7688", > + .of_match_table = anx7688_of_match_table, > + .pm = &anx7688_pm_ops, > + }, > + .probe = anx7688_i2c_probe, > + .remove = anx7688_i2c_remove, > + .id_table = anx7688_ids, > +}; > + > +module_i2c_driver(anx7688_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Martijn Braam <martijn@brixit.nl>"); > +MODULE_AUTHOR("Ondrej Jirman <megi@xff.cz>"); > +MODULE_DESCRIPTION("Analogix ANX7688 USB-C DisplayPort bridge"); I really think you should split this driver into a separate core and glue parts. thanks, -- heikki ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-09 9:56 ` Heikki Krogerus @ 2024-04-09 11:04 ` Pavel Machek 2024-04-09 15:35 ` Dmitry Baryshkov 2024-04-16 7:29 ` Heikki Krogerus 0 siblings, 2 replies; 22+ messages in thread From: Pavel Machek @ 2024-04-09 11:04 UTC (permalink / raw) To: Heikki Krogerus Cc: phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb, megi [-- Attachment #1: Type: text/plain, Size: 7214 bytes --] Hi! > > 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 <megi@xff.cz> > > Co-developed-by: Martijn Braam <martijn@brixit.nl> > > Co-developed-by: Samuel Holland <samuel@sholland.org> > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > Just couple of quick comments below - I did not have time to go over > this very thoroughly, but I think you need to make a new version in > any case because of comments in 1/2. Yes, there will be new version. There is ton of sleep primitives, and existing ones are okay. I can change everything to fdelay or whatever primitive-of-the-day is, but I'd rather not do pointless changes. You can ask for major changes, or complain about extra newlines, but doing both in the same email does not make sense. > Btw, Co-developed-by comes before Signed-off-by I think. I believe this order is fine, too. > > +++ b/drivers/usb/typec/anx7688.c > > @@ -0,0 +1,1830 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ANX7688 USB-C HDMI bridge/PD driver > > + * > > + * Warning, this driver is somewhat PinePhone specific. > > So why not split this into core part and PinePhone specific glue > part? Potentially a lot of work I can't really test and would rather not do. > > + struct delayed_work work; > > + struct timer_list work_timer; > > + > > + struct mutex lock; > > Undocumented lock. This is simple driver. How do you expect me to document it? Protects this data structure, not exactly uncommon. > > + > > + /* wait for power to stabilize and release reset */ > > + msleep(10); > > + gpiod_set_value(anx7688->gpio_reset, 0); > > + usleep_range(2, 4); > > Why not just use usleep_range() in both cases. It does not really make code any better. Can do if you insist. > > +static int anx7688_connect(struct anx7688 *anx7688) > > +{ > > + struct typec_partner_desc desc = {}; > > + int ret, i; > > + u8 fw[2]; > > + const u8 dp_snk_identity[16] = { > > + 0x00, 0x00, 0x00, 0xec, /* id header */ > > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > > + 0x00, 0x00, 0x00, 0x00, /* product type */ > > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > > + }; > > + const u8 svid[4] = { > > + 0x00, 0x00, 0x01, 0xff, > > + }; > > Why not get those from DT? Are you sure it belongs to the DT (and that DT people will agree)? > > + u32 caps[8]; > > + > > + dev_dbg(anx7688->dev, "cable inserted\n"); > > + > > + anx7688->last_status = -1; > > + anx7688->last_cc_status = -1; > > + anx7688->last_dp_state = -1; > > + > > + msleep(10); > > Please make a comment here why you have to wait, and use > usleep_range(). /* Dunno because working code does that and waiting for hardware to settle down after cable insertion kind of looks like a good thing */ I did not write the driver, and there's no good documentation for this chip. I can try to invent something, but... > > + i = 0; > > + while (1) { > > + ret = anx7688_reg_read(anx7688, ANX7688_REG_EEPROM_LOAD_STATUS0); > > + > > + if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) == ANX7688_EEPROM_FW_LOADED) { > > + dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret); > > + dev_dbg(anx7688->dev, "fw loaded after %d ms\n", i * 10); > > + break; > > + } > > + > > + if (i > 99) { > > + set_bit(ANX7688_F_FW_FAILED, anx7688->flags); > > + dev_err(anx7688->dev, > > + "boot firmware load failed (you may need to flash FW to anx7688 first)\n"); > > + ret = -ETIMEDOUT; > > + goto err_vconoff; > > + } > > + msleep(5); > > + i++; > > + } > > You need to use something like time_is_after_jiffies() here instead of > a counter. Well, this works as well, but yes, I agree here. > > + ret = i2c_smbus_read_i2c_block_data(anx7688->client, > > + ANX7688_REG_FW_VERSION1, 2, fw); > > + if (ret < 0) { > > + dev_err(anx7688->dev, "failed to read firmware version\n"); > > + goto err_vconoff; > > + } > > + > > + dev_dbg(anx7688->dev, "OCM firmware loaded (version 0x%04x)\n", > > + fw[1] | fw[0] << 8); > > + > > + /* Unmask interrupts */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); > > + if (ret) > > + goto err_vconoff; > > + > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT_MASK, ~ANX7688_SOFT_INT_MASK); > > + if (ret) > > + goto err_vconoff; > > + > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, 0xff); > > + if (ret) > > + goto err_vconoff; > > + > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_MASK2, (u8)~ANX7688_IRQ2_SOFT_INT); > > + if (ret) > > + goto err_vconoff; > > + > > + /* time to turn off vbus after cc disconnect (unit is 4 ms) */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_VBUS_OFF_DELAY_TIME, 100 / 4); > > + if (ret) > > + goto err_vconoff; > > + > > + /* 300ms (unit is 2 ms) */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_TRY_UFP_TIMER, 300 / 2); > > + if (ret) > > + goto err_vconoff; > > + > > + /* maximum voltage in 100 mV units */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_VOLTAGE, 50); /* 5 V */ > > + if (ret) > > + goto err_vconoff; > > + > > + /* min/max power in 500 mW units */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_POWER, 15 * 2); /* 15 W */ > > + if (ret) > > + goto err_vconoff; > > + > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MIN_POWER, 1); /* 0.5 W */ > > + if (ret) > > + goto err_vconoff; > > + > > + /* auto_pd, try.src, try.sink, goto safe 5V */ > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_FEATURE_CTRL, 0x1e & ~BIT(2)); // disable try_src > > Those two comments are obscure. I hoped they make sense to someone familiar with the area. Can't do much better than remove them. > This function seems to have lot of hard coded information above. > Shouldn't much of that come from DT? You tell me, I suppose you seen some similar drivers. > Please note that if you have that PinePhone specific glue driver, you > can get much of the hard coded information from there, if getting the > information from DT is not an option for what ever reason. Thanks for review. Could you trim the parts of email you are not replying to? Do you see any other major problems? Do you have any idea if this chip is used elsewhere? I do not have any other hardware with anx7688, so I won't be able to test it elsewhere, and if there are no other users, having specific driver should not be a problem for anyone. If there's other user, well, there's chance they have docs and can help. How would you envision the split? Do you have any other driver that could be used as an example? Is someone else putting them in the device tree? Best regards, Pavel -- People of Russia, stop Putin before his war on Ukraine escalates. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-09 11:04 ` Pavel Machek @ 2024-04-09 15:35 ` Dmitry Baryshkov 2024-04-10 11:32 ` Ondřej Jirman 2024-04-16 7:29 ` Heikki Krogerus 1 sibling, 1 reply; 22+ messages in thread From: Dmitry Baryshkov @ 2024-04-09 15:35 UTC (permalink / raw) To: Pavel Machek Cc: Heikki Krogerus, phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb, megi On Tue, Apr 09, 2024 at 01:04:12PM +0200, Pavel Machek wrote: > Hi! > > > > 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 <megi@xff.cz> > > > Co-developed-by: Martijn Braam <martijn@brixit.nl> > > > Co-developed-by: Samuel Holland <samuel@sholland.org> > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > Just couple of quick comments below - I did not have time to go over > > this very thoroughly, but I think you need to make a new version in > > any case because of comments in 1/2. > [skipped] > > > > +static int anx7688_connect(struct anx7688 *anx7688) > > > +{ > > > + struct typec_partner_desc desc = {}; > > > + int ret, i; > > > + u8 fw[2]; > > > + const u8 dp_snk_identity[16] = { > > > + 0x00, 0x00, 0x00, 0xec, /* id header */ > > > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > > > + 0x00, 0x00, 0x00, 0x00, /* product type */ > > > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > > > + }; > > > + const u8 svid[4] = { > > > + 0x00, 0x00, 0x01, 0xff, > > > + }; > > > > Why not get those from DT? > > Are you sure it belongs to the DT (and that DT people will agree)? From Documentation/devicetree/bindings/connector/usb-connector.yaml: altmodes { displayport { svid = /bits/ 16 <0xff01>; vdo = <0x00001c46>; }; }; BTW, I don't see the VDO for the DP altmode in your code. Maybe I missed it at a quick glance. > > > > + u32 caps[8]; > > > + -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-09 15:35 ` Dmitry Baryshkov @ 2024-04-10 11:32 ` Ondřej Jirman 2024-04-10 11:35 ` Ondřej Jirman 0 siblings, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-10 11:32 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Pavel Machek, Heikki Krogerus, phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb On Tue, Apr 09, 2024 at 06:35:50PM GMT, Dmitry Baryshkov wrote: > On Tue, Apr 09, 2024 at 01:04:12PM +0200, Pavel Machek wrote: > > Hi! > > > > > > 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 <megi@xff.cz> > > > > Co-developed-by: Martijn Braam <martijn@brixit.nl> > > > > Co-developed-by: Samuel Holland <samuel@sholland.org> > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > > > Just couple of quick comments below - I did not have time to go over > > > this very thoroughly, but I think you need to make a new version in > > > any case because of comments in 1/2. > > > > [skipped] > > > > > > > +static int anx7688_connect(struct anx7688 *anx7688) > > > > +{ > > > > + struct typec_partner_desc desc = {}; > > > > + int ret, i; > > > > + u8 fw[2]; > > > > + const u8 dp_snk_identity[16] = { > > > > + 0x00, 0x00, 0x00, 0xec, /* id header */ > > > > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > > > > + 0x00, 0x00, 0x00, 0x00, /* product type */ > > > > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > > > > + }; > > > > + const u8 svid[4] = { > > > > + 0x00, 0x00, 0x01, 0xff, > > > > + }; > > > > > > Why not get those from DT? > > > > Are you sure it belongs to the DT (and that DT people will agree)? > > From Documentation/devicetree/bindings/connector/usb-connector.yaml: > > altmodes { > displayport { > svid = /bits/ 16 <0xff01>; > vdo = <0x00001c46>; > }; > }; > > BTW, I don't see the VDO for the DP altmode in your code. Maybe I missed > it at a quick glance. VDO is set via TYPE_DP_SNK_CFG message to the firmware. There may be some default in the firmware which matches Pinephone receptacle configuration. I guess the driver can send the VDO value from DT after firmware is initialized. Other values can be set in DT too, but extreme care needs to ne take, because firmware has some bugs, which cause it to request high voltage from PD PSU when it's in fact not part of PDO, potentially destroying the device. So I'd rather not expose at least PDOs in DT to random unsuspecting DT users who just copy paste and edit DT without reading the driver code or some obscure notes somewhere. kind regards, o. > > > > > > + u32 caps[8]; > > > > + > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-10 11:32 ` Ondřej Jirman @ 2024-04-10 11:35 ` Ondřej Jirman 0 siblings, 0 replies; 22+ messages in thread From: Ondřej Jirman @ 2024-04-10 11:35 UTC (permalink / raw) To: Dmitry Baryshkov, Pavel Machek, Heikki Krogerus, phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb On Wed, Apr 10, 2024 at 01:32:27PM GMT, megi xff wrote: > On Tue, Apr 09, 2024 at 06:35:50PM GMT, Dmitry Baryshkov wrote: > > On Tue, Apr 09, 2024 at 01:04:12PM +0200, Pavel Machek wrote: > > > Hi! > > > > > > > > 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 <megi@xff.cz> > > > > > Co-developed-by: Martijn Braam <martijn@brixit.nl> > > > > > Co-developed-by: Samuel Holland <samuel@sholland.org> > > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > > > > > Just couple of quick comments below - I did not have time to go over > > > > this very thoroughly, but I think you need to make a new version in > > > > any case because of comments in 1/2. > > > > > > > [skipped] > > > > > > > > > > +static int anx7688_connect(struct anx7688 *anx7688) > > > > > +{ > > > > > + struct typec_partner_desc desc = {}; > > > > > + int ret, i; > > > > > + u8 fw[2]; > > > > > + const u8 dp_snk_identity[16] = { > > > > > + 0x00, 0x00, 0x00, 0xec, /* id header */ > > > > > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > > > > > + 0x00, 0x00, 0x00, 0x00, /* product type */ > > > > > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > > > > > + }; > > > > > + const u8 svid[4] = { > > > > > + 0x00, 0x00, 0x01, 0xff, > > > > > + }; > > > > > > > > Why not get those from DT? > > > > > > Are you sure it belongs to the DT (and that DT people will agree)? > > > > From Documentation/devicetree/bindings/connector/usb-connector.yaml: > > > > altmodes { > > displayport { > > svid = /bits/ 16 <0xff01>; > > vdo = <0x00001c46>; > > }; > > }; > > > > BTW, I don't see the VDO for the DP altmode in your code. Maybe I missed > > it at a quick glance. > > VDO is set via TYPE_DP_SNK_CFG message to the firmware. There may be some > default in the firmware which matches Pinephone receptacle configuration. > > I guess the driver can send the VDO value from DT after firmware is > initialized. Other values can be set in DT too, but extreme care needs to > ne take, because firmware has some bugs, which cause it to request > high voltage from PD PSU when it's in fact not part of PDO, potentially > destroying the device. So I'd rather not expose at least PDOs in DT to random > unsuspecting DT users who just copy paste and edit DT without reading the driver > code or some obscure notes somewhere. Or at least have strong warnings everywhere this is used in DT, not just in DT bindings documentation, because requesting 21V when PDO in DT says 5V is pretty unpleasant. kind regards, o. > kind regards, > o. > > > > > > > > > + u32 caps[8]; > > > > > + > > > > > > -- > > With best wishes > > Dmitry ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge 2024-04-09 11:04 ` Pavel Machek 2024-04-09 15:35 ` Dmitry Baryshkov @ 2024-04-16 7:29 ` Heikki Krogerus 1 sibling, 0 replies; 22+ messages in thread From: Heikki Krogerus @ 2024-04-16 7:29 UTC (permalink / raw) To: Pavel Machek Cc: phone-devel, kernel list, fiona.klute, martijn, samuel, gregkh, linux-usb, megi On Tue, Apr 09, 2024 at 01:04:12PM +0200, Pavel Machek wrote: > Hi! > > > > 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 <megi@xff.cz> > > > Co-developed-by: Martijn Braam <martijn@brixit.nl> > > > Co-developed-by: Samuel Holland <samuel@sholland.org> > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > Just couple of quick comments below - I did not have time to go over > > this very thoroughly, but I think you need to make a new version in > > any case because of comments in 1/2. > > Yes, there will be new version. > > There is ton of sleep primitives, and existing ones are okay. I can > change everything to fdelay or whatever primitive-of-the-day is, but > I'd rather not do pointless changes. > > You can ask for major changes, or complain about extra newlines, but > doing both in the same email does not make sense. I'm not telling you to do any major changes, yet. Right now I'm trying to understand why you are doing thing the way you are doing them. > > Btw, Co-developed-by comes before Signed-off-by I think. > > I believe this order is fine, too. > > > > +++ b/drivers/usb/typec/anx7688.c > > > @@ -0,0 +1,1830 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * ANX7688 USB-C HDMI bridge/PD driver > > > + * > > > + * Warning, this driver is somewhat PinePhone specific. > > > > So why not split this into core part and PinePhone specific glue > > part? > > Potentially a lot of work I can't really test and would rather not do. Glue layer is usually an easy way to organise your driver into smaller more manageable chunks, and most importantly, keep the core parts generic. But just to be clear here, it is not always necessary - maybe that's the case here. > > > + struct delayed_work work; > > > + struct timer_list work_timer; > > > + > > > + struct mutex lock; > > > > Undocumented lock. > > This is simple driver. How do you expect me to document it? Protects > this data structure, not exactly uncommon. You use a comment to describe what the lock protects. You can use "scripts/checkpatch.pl --strict" to spot this kind of stuff for you. > > > + > > > + /* wait for power to stabilize and release reset */ > > > + msleep(10); > > > + gpiod_set_value(anx7688->gpio_reset, 0); > > > + usleep_range(2, 4); > > > > Why not just use usleep_range() in both cases. > > It does not really make code any better. Can do if you insist. Consistency makes any code better. > > > +static int anx7688_connect(struct anx7688 *anx7688) > > > +{ > > > + struct typec_partner_desc desc = {}; > > > + int ret, i; > > > + u8 fw[2]; > > > + const u8 dp_snk_identity[16] = { > > > + 0x00, 0x00, 0x00, 0xec, /* id header */ > > > + 0x00, 0x00, 0x00, 0x00, /* cert stat */ > > > + 0x00, 0x00, 0x00, 0x00, /* product type */ > > > + 0x39, 0x00, 0x00, 0x51 /* alt mode adapter */ > > > + }; > > > + const u8 svid[4] = { > > > + 0x00, 0x00, 0x01, 0xff, > > > + }; > > > > Why not get those from DT? > > Are you sure it belongs to the DT (and that DT people will agree)? Yes, they belong to the DT, and there are already bindings. Dmitry gave you the link to the bindings I think (thanks for that Dmitry). > > > + u32 caps[8]; > > > + > > > + dev_dbg(anx7688->dev, "cable inserted\n"); > > > + > > > + anx7688->last_status = -1; > > > + anx7688->last_cc_status = -1; > > > + anx7688->last_dp_state = -1; > > > + > > > + msleep(10); > > > > Please make a comment here why you have to wait, and use > > usleep_range(). > > /* Dunno because working code does that and waiting for hardware to > settle down after cable insertion kind of looks like a good thing */ > > I did not write the driver, and there's no good documentation for this > chip. I can try to invent something, but... > > > > + i = 0; > > > + while (1) { > > > + ret = anx7688_reg_read(anx7688, ANX7688_REG_EEPROM_LOAD_STATUS0); > > > + > > > + if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) == ANX7688_EEPROM_FW_LOADED) { > > > + dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret); > > > + dev_dbg(anx7688->dev, "fw loaded after %d ms\n", i * 10); > > > + break; > > > + } > > > + > > > + if (i > 99) { > > > + set_bit(ANX7688_F_FW_FAILED, anx7688->flags); > > > + dev_err(anx7688->dev, > > > + "boot firmware load failed (you may need to flash FW to anx7688 first)\n"); > > > + ret = -ETIMEDOUT; > > > + goto err_vconoff; > > > + } > > > + msleep(5); > > > + i++; > > > + } > > > > You need to use something like time_is_after_jiffies() here instead of > > a counter. > > Well, this works as well, but yes, I agree here. > > > > + ret = i2c_smbus_read_i2c_block_data(anx7688->client, > > > + ANX7688_REG_FW_VERSION1, 2, fw); > > > + if (ret < 0) { > > > + dev_err(anx7688->dev, "failed to read firmware version\n"); > > > + goto err_vconoff; > > > + } > > > + > > > + dev_dbg(anx7688->dev, "OCM firmware loaded (version 0x%04x)\n", > > > + fw[1] | fw[0] << 8); > > > + > > > + /* Unmask interrupts */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT, 0); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_STATUS_INT_MASK, ~ANX7688_SOFT_INT_MASK); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_SOURCE2, 0xff); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_IRQ_EXT_MASK2, (u8)~ANX7688_IRQ2_SOFT_INT); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + /* time to turn off vbus after cc disconnect (unit is 4 ms) */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_VBUS_OFF_DELAY_TIME, 100 / 4); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + /* 300ms (unit is 2 ms) */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_TRY_UFP_TIMER, 300 / 2); > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + /* maximum voltage in 100 mV units */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_VOLTAGE, 50); /* 5 V */ > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + /* min/max power in 500 mW units */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MAX_POWER, 15 * 2); /* 15 W */ > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_MIN_POWER, 1); /* 0.5 W */ > > > + if (ret) > > > + goto err_vconoff; > > > + > > > + /* auto_pd, try.src, try.sink, goto safe 5V */ > > > + ret = anx7688_reg_write(anx7688, ANX7688_REG_FEATURE_CTRL, 0x1e & ~BIT(2)); // disable try_src > > > > Those two comments are obscure. I'm making a note about the comments only because right now I have to rely on things like them to get an idea about what's going on. I don't know anything about this chip. > I hoped they make sense to someone familiar with the area. Can't do > much better than remove them. > > > This function seems to have lot of hard coded information above. > > Shouldn't much of that come from DT? > > You tell me, I suppose you seen some similar drivers. > > > Please note that if you have that PinePhone specific glue driver, you > > can get much of the hard coded information from there, if getting the > > information from DT is not an option for what ever reason. > > Thanks for review. > > Could you trim the parts of email you are not replying to? > > Do you see any other major problems? > > Do you have any idea if this chip is used elsewhere? I do not have any > other hardware with anx7688, so I won't be able to test it elsewhere, > and if there are no other users, having specific driver should not be > a problem for anyone. If there's other user, well, there's chance they > have docs and can help. I don't know anything about this chip. > How would you envision the split? Do you have any other driver that > could be used as an example? Check drivers/usb/dwc3/. It is a nice example how to decouple the glue layer from the core completely - there is no API, not even driver data is passed beteen the two. For library like solutions, check: drivers/usb/typec/tcpm/ drivers/usb/typec/ucsi/ drivers/usb/host/xhci* drivers/tty/serial/8250/ But again, glue layer is just a suggestion. It would be one way to rid of most of the hardcoded stuff by moving it there. But if much of the hard coded parts can be moved to DT instead, then there is most likely no need for the glue layers in this case. thanks, -- heikki ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 10:51 [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Pavel Machek 2024-04-08 10:54 ` [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge Pavel Machek @ 2024-04-08 11:17 ` Krzysztof Kozlowski 2024-04-08 11:21 ` Pavel Machek 2024-04-08 11:51 ` Ondřej Jirman 1 sibling, 2 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 11:17 UTC (permalink / raw) To: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, megi On 08/04/2024 12:51, Pavel Machek wrote: > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > but I did best I could. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> ... > + cabledet-gpios: > + maxItems: 1 > + description: GPIO controlling CABLE_DET (C3) pin. > + > + avdd10-supply: > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > + > + dvdd10-supply: > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > + > + avdd18-supply: > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > + > + dvdd18-supply: > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > + > + avdd33-supply: > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > + > + i2c-supply: true > + vconn-supply: true There are no such supplies like i2c and vconn on the schematics. I think this represents some other part of component which was added here only for convenience. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:17 ` [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Krzysztof Kozlowski @ 2024-04-08 11:21 ` Pavel Machek 2024-04-08 11:24 ` Krzysztof Kozlowski 2024-04-08 11:51 ` Ondřej Jirman 1 sibling, 1 reply; 22+ messages in thread From: Pavel Machek @ 2024-04-08 11:21 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, megi [-- Attachment #1: Type: text/plain, Size: 1199 bytes --] Hi! > > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > > but I did best I could. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > ... > > > + cabledet-gpios: > > + maxItems: 1 > > + description: GPIO controlling CABLE_DET (C3) pin. > > + > > + avdd10-supply: > > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > > + > > + dvdd10-supply: > > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > > + > > + avdd18-supply: > > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > > + > > + dvdd18-supply: > > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > > + > > + avdd33-supply: > > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > > + > > + i2c-supply: true > > + vconn-supply: true > > There are no such supplies like i2c and vconn on the schematics. > > I think this represents some other part of component which was added > here only for convenience. Can you give me pointer to documentation you are looking at? Best regards, Pavel -- People of Russia, stop Putin before his war on Ukraine escalates. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:21 ` Pavel Machek @ 2024-04-08 11:24 ` Krzysztof Kozlowski 2024-04-08 11:52 ` Ondřej Jirman 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 11:24 UTC (permalink / raw) To: Pavel Machek Cc: phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, megi On 08/04/2024 13:21, Pavel Machek wrote: > Hi! > >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>> but I did best I could. >>> >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >> >> ... >> >>> + cabledet-gpios: >>> + maxItems: 1 >>> + description: GPIO controlling CABLE_DET (C3) pin. >>> + >>> + avdd10-supply: >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>> + >>> + dvdd10-supply: >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>> + >>> + avdd18-supply: >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>> + >>> + dvdd18-supply: >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>> + >>> + avdd33-supply: >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>> + >>> + i2c-supply: true >>> + vconn-supply: true >> >> There are no such supplies like i2c and vconn on the schematics. >> >> I think this represents some other part of component which was added >> here only for convenience. > > Can you give me pointer to documentation you are looking at? The schematics you linked in the document at the beginning. Page 13. Do you see these pins there? I saw only VCONN1_EN, but that's not a supply. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:24 ` Krzysztof Kozlowski @ 2024-04-08 11:52 ` Ondřej Jirman 2024-04-08 11:59 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-08 11:52 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 13:21, Pavel Machek wrote: > > Hi! > > > >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > >>> but I did best I could. > >>> > >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> > >> > >> ... > >> > >>> + cabledet-gpios: > >>> + maxItems: 1 > >>> + description: GPIO controlling CABLE_DET (C3) pin. > >>> + > >>> + avdd10-supply: > >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins > >>> + > >>> + dvdd10-supply: > >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins > >>> + > >>> + avdd18-supply: > >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins > >>> + > >>> + dvdd18-supply: > >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins > >>> + > >>> + avdd33-supply: > >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins > >>> + > >>> + i2c-supply: true > >>> + vconn-supply: true > >> > >> There are no such supplies like i2c and vconn on the schematics. > >> > >> I think this represents some other part of component which was added > >> here only for convenience. > > > > Can you give me pointer to documentation you are looking at? > > The schematics you linked in the document at the beginning. Page 13. Do > you see these pins there? I saw only VCONN1_EN, but that's not a supply. The supply is U1308. regards, o. > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:52 ` Ondřej Jirman @ 2024-04-08 11:59 ` Krzysztof Kozlowski 2024-04-08 12:48 ` Ondřej Jirman 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 11:59 UTC (permalink / raw) To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On 08/04/2024 13:52, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 13:21, Pavel Machek wrote: >>> Hi! >>> >>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>>>> but I did best I could. >>>>> >>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >>>> >>>> ... >>>> >>>>> + cabledet-gpios: >>>>> + maxItems: 1 >>>>> + description: GPIO controlling CABLE_DET (C3) pin. >>>>> + >>>>> + avdd10-supply: >>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>>>> + >>>>> + dvdd10-supply: >>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>>>> + >>>>> + avdd18-supply: >>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>>>> + >>>>> + dvdd18-supply: >>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>>>> + >>>>> + avdd33-supply: >>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>>>> + >>>>> + i2c-supply: true >>>>> + vconn-supply: true >>>> >>>> There are no such supplies like i2c and vconn on the schematics. >>>> >>>> I think this represents some other part of component which was added >>>> here only for convenience. >>> >>> Can you give me pointer to documentation you are looking at? >> >> The schematics you linked in the document at the beginning. Page 13. Do >> you see these pins there? I saw only VCONN1_EN, but that's not a supply. > > The supply is U1308. That's not a supply to anx7688. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:59 ` Krzysztof Kozlowski @ 2024-04-08 12:48 ` Ondřej Jirman 2024-04-08 13:27 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-08 12:48 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On Mon, Apr 08, 2024 at 01:59:12PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 13:52, Ondřej Jirman wrote: > > On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: > >> On 08/04/2024 13:21, Pavel Machek wrote: > >>> Hi! > >>> > >>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > >>>>> but I did best I could. > >>>>> > >>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> > >>>> > >>>> ... > >>>> > >>>>> + cabledet-gpios: > >>>>> + maxItems: 1 > >>>>> + description: GPIO controlling CABLE_DET (C3) pin. > >>>>> + > >>>>> + avdd10-supply: > >>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins > >>>>> + > >>>>> + dvdd10-supply: > >>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins > >>>>> + > >>>>> + avdd18-supply: > >>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins > >>>>> + > >>>>> + dvdd18-supply: > >>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins > >>>>> + > >>>>> + avdd33-supply: > >>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins > >>>>> + > >>>>> + i2c-supply: true > >>>>> + vconn-supply: true > >>>> > >>>> There are no such supplies like i2c and vconn on the schematics. > >>>> > >>>> I think this represents some other part of component which was added > >>>> here only for convenience. > >>> > >>> Can you give me pointer to documentation you are looking at? > >> > >> The schematics you linked in the document at the beginning. Page 13. Do > >> you see these pins there? I saw only VCONN1_EN, but that's not a supply. > > > > The supply is U1308. > > That's not a supply to anx7688. Yeah, I understand where the confusion is. The driver is not for anx7688 chip really. The driver is named anx7688, but that's mostly a historical accident at this point. I guess there can be a driver for anx7688 chip that can directly use the chip's resources from the host by directly manipulating its registers and implementing type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like fusb302 driver, or various tcpci subdrivers). But in this case the chip is driven by an optional on-chip microcontroller's firmware and *this driver* is specifically for *the Type-C port on Pinephone* and serves as an integration driver for quite a bunch of things that need to work together on Pinephone for all of the Type-C port's features to operate reasonably well (and one of those is some communication with anx7688 firmware that we use, and enabling power to this chip and other things as appropriate, based on the communication from the firmware). It handles the specific needs of the Pinephone's Type-C implementation, all of its quirks (of which there are many over several HW revisions) that can't be handled by the particular implementation of on-chip microcontroller firmware directly and need host side interaction. In an ideal world, many of the things this driver handles would be handled by embedded microcontroller on the board (like it is with some RK3399 based Google devices), but Pinephone has no such thing and this glue needs to be implemented somewhere in the kernel. Kind regards, o. > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 12:48 ` Ondřej Jirman @ 2024-04-08 13:27 ` Krzysztof Kozlowski 2024-04-08 15:17 ` Ondřej Jirman 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 13:27 UTC (permalink / raw) To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On 08/04/2024 14:48, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 01:59:12PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 13:52, Ondřej Jirman wrote: >>> On Mon, Apr 08, 2024 at 01:24:03PM GMT, Krzysztof Kozlowski wrote: >>>> On 08/04/2024 13:21, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>>>>>> but I did best I could. >>>>>>> >>>>>>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >>>>>> >>>>>> ... >>>>>> >>>>>>> + cabledet-gpios: >>>>>>> + maxItems: 1 >>>>>>> + description: GPIO controlling CABLE_DET (C3) pin. >>>>>>> + >>>>>>> + avdd10-supply: >>>>>>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>>>>>> + >>>>>>> + dvdd10-supply: >>>>>>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>>>>>> + >>>>>>> + avdd18-supply: >>>>>>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>>>>>> + >>>>>>> + dvdd18-supply: >>>>>>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>>>>>> + >>>>>>> + avdd33-supply: >>>>>>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>>>>>> + >>>>>>> + i2c-supply: true >>>>>>> + vconn-supply: true >>>>>> >>>>>> There are no such supplies like i2c and vconn on the schematics. >>>>>> >>>>>> I think this represents some other part of component which was added >>>>>> here only for convenience. >>>>> >>>>> Can you give me pointer to documentation you are looking at? >>>> >>>> The schematics you linked in the document at the beginning. Page 13. Do >>>> you see these pins there? I saw only VCONN1_EN, but that's not a supply. >>> >>> The supply is U1308. >> >> That's not a supply to anx7688. > > Yeah, I understand where the confusion is. The driver is not for anx7688 chip > really. The driver is named anx7688, but that's mostly a historical accident at > this point. > > I guess there can be a driver for anx7688 chip that can directly use the chip's > resources from the host by directly manipulating its registers and implementing > type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like > fusb302 driver, or various tcpci subdrivers). > > But in this case the chip is driven by an optional on-chip microcontroller's > firmware and *this driver* is specifically for *the Type-C port on Pinephone* We do not talk here about the driver, but bindings, so hardware. > and serves as an integration driver for quite a bunch of things that need to > work together on Pinephone for all of the Type-C port's features to operate > reasonably well (and one of those is some communication with anx7688 firmware > that we use, and enabling power to this chip and other things as appropriate, > based on the communication from the firmware). That's still looking like putting board design into particular device binding. > > It handles the specific needs of the Pinephone's Type-C implementation, all of > its quirks (of which there are many over several HW revisions) that can't be > handled by the particular implementation of on-chip microcontroller firmware > directly and need host side interaction. > > In an ideal world, many of the things this driver handles would be handled by > embedded microcontroller on the board (like it is with some RK3399 based Google > devices), but Pinephone has no such thing and this glue needs to be implemented > somewhere in the kernel. You might need multiple schemas, because this is for anx7688, not for Pinephone type-c implementation. However I still do not see yet a limitation of DTS requiring stuffing some other properties into anx7688 or creating some other, virtual entity. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 13:27 ` Krzysztof Kozlowski @ 2024-04-08 15:17 ` Ondřej Jirman 2024-04-08 20:12 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-08 15:17 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On Mon, Apr 08, 2024 at 03:27:00PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 14:48, Ondřej Jirman wrote: > > Yeah, I understand where the confusion is. The driver is not for anx7688 chip > > really. The driver is named anx7688, but that's mostly a historical accident at > > this point. > > > > I guess there can be a driver for anx7688 chip that can directly use the chip's > > resources from the host by directly manipulating its registers and implementing > > type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like > > fusb302 driver, or various tcpci subdrivers). > > > > But in this case the chip is driven by an optional on-chip microcontroller's > > firmware and *this driver* is specifically for *the Type-C port on Pinephone* > > We do not talk here about the driver, but bindings, so hardware. Got it. Bindings should be the same regardless of what driver would be used, whether this OCM based one, or some future one based on the above mentioned TCPCI in-kernel implementation. Hardware is the same in both cases. Just trying to imagine how to actually solve the issues... Basic thing with the I2C regulator thing is that needs to be enabled as long as anx7688 needs to communicate over I2C. Other user of this power rail is touchscreen controller for its normal power supply, and it needs to be able to disable it during system suspend. 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. 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. 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. kind regards, o. > > and serves as an integration driver for quite a bunch of things that need to > > work together on Pinephone for all of the Type-C port's features to operate > > reasonably well (and one of those is some communication with anx7688 firmware > > that we use, and enabling power to this chip and other things as appropriate, > > based on the communication from the firmware). > > That's still looking like putting board design into particular device > binding. > > > > > It handles the specific needs of the Pinephone's Type-C implementation, all of > > its quirks (of which there are many over several HW revisions) that can't be > > handled by the particular implementation of on-chip microcontroller firmware > > directly and need host side interaction. > > > > In an ideal world, many of the things this driver handles would be handled by > > embedded microcontroller on the board (like it is with some RK3399 based Google > > devices), but Pinephone has no such thing and this glue needs to be implemented > > somewhere in the kernel. > > You might need multiple schemas, because this is for anx7688, not for > Pinephone type-c implementation. > > However I still do not see yet a limitation of DTS requiring stuffing > some other properties into anx7688 or creating some other, virtual entity. > > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 15:17 ` Ondřej Jirman @ 2024-04-08 20:12 ` Krzysztof Kozlowski 2024-04-10 2:20 ` Ondřej Jirman 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 20:12 UTC (permalink / raw) To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On 08/04/2024 17:17, Ondřej Jirman wrote: > On Mon, Apr 08, 2024 at 03:27:00PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 14:48, Ondřej Jirman wrote: >>> Yeah, I understand where the confusion is. The driver is not for anx7688 chip >>> really. The driver is named anx7688, but that's mostly a historical accident at >>> this point. >>> >>> I guess there can be a driver for anx7688 chip that can directly use the chip's >>> resources from the host by directly manipulating its registers and implementing >>> type-c functionality via eg. Linux's TCPM or TCPCI stack, etc. (eg. like >>> fusb302 driver, or various tcpci subdrivers). >>> >>> But in this case the chip is driven by an optional on-chip microcontroller's >>> firmware and *this driver* is specifically for *the Type-C port on Pinephone* >> >> We do not talk here about the driver, but bindings, so hardware. > > Got it. Bindings should be the same regardless of what driver would be used, > whether this OCM based one, or some future one based on the above mentioned > TCPCI in-kernel implementation. Hardware is the same in both cases. > > Just trying to imagine how to actually solve the issues... > > Basic thing with the I2C regulator thing is that needs to be enabled as long > as anx7688 needs to communicate over I2C. Other user of this power rail is > touchscreen controller for its normal power supply, and it needs to be able > to disable it during system suspend. This does not look like anything specific to this particular device... but even without this, please think how you want it to be solved. Same supply which has to be on always, because your anx7688 can talk over I2C, and in the same time sometimes off, so touchscreen can be shutdown. If this is regulator for the I2C bus, then I think we already had this discussion some time ago. I think it is not a property of the I2C device, but the controller. > > 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? > > 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? You need to properly represent the hardware, not bend it to your one use-case for one hardware. > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 20:12 ` Krzysztof Kozlowski @ 2024-04-10 2:20 ` Ondřej Jirman 2024-04-11 19:59 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-10 2:20 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-10 2:20 ` Ondřej Jirman @ 2024-04-11 19:59 ` Krzysztof Kozlowski 2024-04-11 20:17 ` Dmitry Baryshkov 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-11 19:59 UTC (permalink / raw) To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On 10/04/2024 04:20, Ondřej Jirman wrote: > 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. Either I don't understand about which I2C regulator you speak or this is not I2C core regulator. This is a regulator to be managed by the I2C controller, not by I2C core. > >>> >>> 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 Yeah, the vconn looks reasonable. Just provide description of the supply, so it will be obvious. > Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-11 19:59 ` Krzysztof Kozlowski @ 2024-04-11 20:17 ` Dmitry Baryshkov 0 siblings, 0 replies; 22+ messages in thread From: Dmitry Baryshkov @ 2024-04-11 20:17 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On Thu, Apr 11, 2024 at 09:59:35PM +0200, Krzysztof Kozlowski wrote: > On 10/04/2024 04:20, Ondřej Jirman wrote: > > 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. > > Either I don't understand about which I2C regulator you speak or this is > not I2C core regulator. This is a regulator to be managed by the I2C > controller, not by I2C core. If it is a supply that pulls up the SDA/SCL lines, then it is generic enough to be handled by the core. For example, on Qualcomm platforms CCI lines also usually have external supply as a pull-up. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:17 ` [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Krzysztof Kozlowski 2024-04-08 11:21 ` Pavel Machek @ 2024-04-08 11:51 ` Ondřej Jirman 2024-04-08 12:01 ` Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Ondřej Jirman @ 2024-04-08 11:51 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree Hi Krzysztof, On Mon, Apr 08, 2024 at 01:17:32PM GMT, Krzysztof Kozlowski wrote: > On 08/04/2024 12:51, Pavel Machek wrote: > > Add binding for anx7688 usb type-c bridge. I don't have a datasheet, > > but I did best I could. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > ... > > > + cabledet-gpios: > > + maxItems: 1 > > + description: GPIO controlling CABLE_DET (C3) pin. > > + > > + avdd10-supply: > > + description: 1.0V power supply going to AVDD10 (A4, ...) pins > > + > > + dvdd10-supply: > > + description: 1.0V power supply going to DVDD10 (D6, ...) pins > > + > > + avdd18-supply: > > + description: 1.8V power supply going to AVDD18 (E3, ...) pins > > + > > + dvdd18-supply: > > + description: 1.8V power supply going to DVDD18 (G4, ...) pins > > + > > + avdd33-supply: > > + description: 3.3V power supply going to AVDD33 (C4, ...) pins > > + > > + i2c-supply: true > > + vconn-supply: true > > There are no such supplies like i2c and vconn on the schematics. Which schematics? ANX7688 has VCONN1/2_EN GPIOs that control a switching of VCONN power supply to resective CCx pins. That's just a switch signal. Power for VCONN needs to come from somewhere and the driver needs to enable the regulator at the appropriate time only. On Pinephone it can't be an always on power supply and needs to be enabled only when used due to HW design of the circuit. (default without ANX driver initialized would be to shove 5V to both CC pins, which breaks Type-C spec) I2C supply is needed for I2C bus to work, apparently. There's nothing that says that I2C pull-ups supply has to come from supplies powering the chip. I2C I/O is open drain and the device needs to enable a bus supply in order to communicate. You can say that bus master should be managing the bus supply, but you'd still have a problem that each device may be behind a voltage translator, and logically, bus master driver should care only about its side of the bus then. Both side of level shifter need the pull-up power enabled. You can also make an argument that bus supply can be always on, but that causes several other issues on Pinephone due to shared nature of most resources like these. There are other devices on shared power rails, that need to be turned off during sleep, etc. Kind regards, o. > I think this represents some other part of component which was added > here only for convenience. > > > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document 2024-04-08 11:51 ` Ondřej Jirman @ 2024-04-08 12:01 ` Krzysztof Kozlowski 0 siblings, 0 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2024-04-08 12:01 UTC (permalink / raw) To: Ondřej Jirman, Pavel Machek, phone-devel, kernel list, fiona.klute, martijn, samuel, heikki.krogerus, gregkh, linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree On 08/04/2024 13:51, Ondřej Jirman wrote: > Hi Krzysztof, > > On Mon, Apr 08, 2024 at 01:17:32PM GMT, Krzysztof Kozlowski wrote: >> On 08/04/2024 12:51, Pavel Machek wrote: >>> Add binding for anx7688 usb type-c bridge. I don't have a datasheet, >>> but I did best I could. >>> >>> Signed-off-by: Pavel Machek <pavel@ucw.cz> >> >> ... >> >>> + cabledet-gpios: >>> + maxItems: 1 >>> + description: GPIO controlling CABLE_DET (C3) pin. >>> + >>> + avdd10-supply: >>> + description: 1.0V power supply going to AVDD10 (A4, ...) pins >>> + >>> + dvdd10-supply: >>> + description: 1.0V power supply going to DVDD10 (D6, ...) pins >>> + >>> + avdd18-supply: >>> + description: 1.8V power supply going to AVDD18 (E3, ...) pins >>> + >>> + dvdd18-supply: >>> + description: 1.8V power supply going to DVDD18 (G4, ...) pins >>> + >>> + avdd33-supply: >>> + description: 3.3V power supply going to AVDD33 (C4, ...) pins >>> + >>> + i2c-supply: true >>> + vconn-supply: true >> >> There are no such supplies like i2c and vconn on the schematics. > > Which schematics? > > ANX7688 has VCONN1/2_EN GPIOs that control a switching of VCONN power supply > to resective CCx pins. That's just a switch signal. Power for VCONN needs > to come from somewhere and the driver needs to enable the regulator at > the appropriate time only. > > On Pinephone it can't be an always on power supply and needs to be enabled > only when used due to HW design of the circuit. (default without ANX driver > initialized would be to shove 5V to both CC pins, which breaks Type-C spec) > > I2C supply is needed for I2C bus to work, apparently. There's nothing > that says that I2C pull-ups supply has to come from supplies powering the > chip. I2C I/O is open drain and the device needs to enable a bus supply > in order to communicate. No, that's misunderstanding of DT. These are not supplies to anx7688. Bus supply is not related to anx7688. > > You can say that bus master should be managing the bus supply, but you'd still > have a problem that each device may be behind a voltage translator, and > logically, bus master driver should care only about its side of the bus then. > Both side of level shifter need the pull-up power enabled. Again, that's nothing related to anx7688. If you want to add it here, why not to every device everywhere? > > You can also make an argument that bus supply can be always on, but that > causes several other issues on Pinephone due to shared nature of most > resources like these. There are other devices on shared power rails, that > need to be turned off during sleep, etc. > No, do not add non-existing properties on this device as workaround for other issues. Please drop these two supplies *and all other which do not exist* on anx7688. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-04-16 7:29 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-08 10:51 [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Pavel Machek 2024-04-08 10:54 ` [PATCHv3 2/2] usb: typec: anx7688: Add driver for ANX7688 USB-C HDMI bridge Pavel Machek 2024-04-09 9:56 ` Heikki Krogerus 2024-04-09 11:04 ` Pavel Machek 2024-04-09 15:35 ` Dmitry Baryshkov 2024-04-10 11:32 ` Ondřej Jirman 2024-04-10 11:35 ` Ondřej Jirman 2024-04-16 7:29 ` Heikki Krogerus 2024-04-08 11:17 ` [PATCHv3 1/2] dt-bindings: usb: typec: anx7688: start a binding document Krzysztof Kozlowski 2024-04-08 11:21 ` Pavel Machek 2024-04-08 11:24 ` Krzysztof Kozlowski 2024-04-08 11:52 ` Ondřej Jirman 2024-04-08 11:59 ` Krzysztof Kozlowski 2024-04-08 12:48 ` Ondřej Jirman 2024-04-08 13:27 ` Krzysztof Kozlowski 2024-04-08 15:17 ` Ondřej Jirman 2024-04-08 20:12 ` Krzysztof Kozlowski 2024-04-10 2:20 ` Ondřej Jirman 2024-04-11 19:59 ` Krzysztof Kozlowski 2024-04-11 20:17 ` Dmitry Baryshkov 2024-04-08 11:51 ` Ondřej Jirman 2024-04-08 12:01 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox