* [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name @ 2017-04-21 13:01 ` Hans de Goede 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede ` (4 more replies) 0 siblings, 5 replies; 26+ messages in thread From: Hans de Goede @ 2017-04-21 13:01 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel The parent device name is not necessarily always useful, e.g. with i2c devices it may simply be e.g.: "0-0022" and it also depends on the i2c-bus number which depends on probe ordering. This commit allows drivers to set their own, more useful name, avoiding the problems with some i2c-device names. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index f422a78..f8d3c1b 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -1117,7 +1117,8 @@ int extcon_dev_register(struct extcon_dev *edev) edev->dev.class = extcon_class; edev->dev.release = extcon_dev_release; - edev->name = dev_name(edev->dev.parent); + if (!edev->name) + edev->name = dev_name(edev->dev.parent); if (IS_ERR_OR_NULL(edev->name)) { dev_err(&edev->dev, "extcon device name is null\n"); -- 2.9.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede @ 2017-04-21 13:01 ` Hans de Goede 2017-04-21 18:51 ` Andy Shevchenko ` (2 more replies) 2017-04-21 13:01 ` [PATCH 3/5] extcon: intel-cht-wc: Default to SDP on regmap io errors Hans de Goede ` (3 subsequent siblings) 4 siblings, 3 replies; 26+ messages in thread From: Hans de Goede @ 2017-04-21 13:01 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel Add support for USB TYPE-C cable detection on systems using a FUSB302 USB TYPE-C controller. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/Kconfig | 8 + drivers/extcon/Makefile | 1 + drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 791 insertions(+) create mode 100644 drivers/extcon/extcon-fusb302.c diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index 32f2dc8..562db5b 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -35,6 +35,14 @@ config EXTCON_AXP288 Say Y here to enable support for USB peripheral detection and USB MUX switching by X-Power AXP288 PMIC. +config EXTCON_FUSB302 + tristate "FUSB302 USB TYPE-C controller support" + depends on I2C + select REGMAP_I2C + help + Say Y here to enable support for USB TYPE-C cable detection using + a FUSB302 USB TYPE-C controller. + config EXTCON_GPIO tristate "GPIO extcon support" depends on GPIOLIB || COMPILE_TEST diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index ecfa958..e5eb493 100644 --- a/drivers/extcon/Makefile +++ b/drivers/extcon/Makefile @@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o +obj-$(CONFIG_EXTCON_FUSB302) += extcon-fusb302.o obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o diff --git a/drivers/extcon/extcon-fusb302.c b/drivers/extcon/extcon-fusb302.c new file mode 100644 index 0000000..577adb9 --- /dev/null +++ b/drivers/extcon/extcon-fusb302.c @@ -0,0 +1,782 @@ +/* + * Extcon USB-C cable detection driver for FUSB302 USB TYPE-C controller + * + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/extcon.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/workqueue.h> +#include "extcon.h" + +#define REG_DEVICE_ID 0x01 +#define DEVICE_ID_VER_MASK GENMASK(7, 4) +#define DEVICE_ID_FUSB302_VER 0x80 + +#define REG_SWITCHES0 0x02 +#define SWITCHES0_PDWN1 BIT(0) +#define SWITCHES0_PDWN2 BIT(1) +#define SWITCHES0_PDWN GENMASK(1, 0) +#define SWITCHES0_MEAS_CC1 BIT(2) +#define SWITCHES0_MEAS_CC2 BIT(3) +#define SWITCHES0_VCONN_CC1 BIT(4) +#define SWITCHES0_VCONN_CC2 BIT(5) +#define SWITCHES0_PU_EN1 BIT(6) +#define SWITCHES0_PU_EN2 BIT(7) + +#define REG_MEASURE 0x04 +#define MEASURE_MDAC_MASK GENMASK(5, 0) +/* Datasheet says MDAC must be set to 0x34 / 2226mV for vRd-3.0 detection */ +#define MEASURE_MDAC_SNK_VRD30 0x34 +/* MDAC must be set to 0x25 / 1600mV for disconnect det. with 80uA host-cur */ +#define MEASURE_MDAC_SRC_80UA_HOST_CUR 0x25 + +#define REG_CONTROL0 0x06 +#define CONTROL0_HOST_CUR_MASK GENMASK(3, 2) +#define CONTROL0_HOST_CUR_DISABLED (0 << 2) +#define CONTROL0_HOST_CUR_80UA (1 << 2) +#define CONTROL0_HOST_CUR_180UA (2 << 2) +#define CONTROL0_HOST_CUR_330UA (3 << 2) +#define CONTROL0_INT_MASK BIT(5) + +#define REG_CONTROL2 0x08 +#define CONTROL2_TOGGLE BIT(0) +#define CONTROL2_MODE_MASK GENMASK(2, 1) +#define CONTROL2_MODE_DRP (1 << 1) +#define CONTROL2_MODE_SNK (2 << 1) +#define CONTROL2_MODE_SRC (3 << 1) +#define CONTROL2_SAVE_PWR_MASK GENMASK(7, 6) +#define CONTROL2_SAVE_PWR_DISABLED (0 << 6) +#define CONTROL2_SAVE_PWR_40MS (1 << 6) +#define CONTROL2_SAVE_PWR_80MS (2 << 6) +#define CONTROL2_SAVE_PWR_160MS (3 << 6) + +#define REG_MASK1 0x0a +/* REG_MASK1 value for low-power / disabled state from datasheet */ +#define MASK1_DISABLED 0xfe +#define MASK1_COMP_CHNG BIT(5) +#define MASK1_VBUSOK BIT(7) + +#define REG_POWER 0x0b +/* REG_POWER values for disabled and normal state from datasheet */ +#define POWER_DISABLED BIT(0) +#define POWER_NORMAL GENMASK(2, 0) + +#define REG_RESET 0x0c +#define RESET_SW_RESET BIT(0) + +#define REG_OCP 0x0d + +#define REG_MASKA 0x0e +/* REG_MASKA value for low-power / disabled state from datasheet */ +#define MASKA_DISABLED 0xbf + +#define REG_MASKB 0x0f +/* REG_MASKB value for low-power / disabled state from datasheet */ +#define MASKB_DISABLED 0x01 + +#define REG_STATUS1A 0x3d +#define STATUS1A_TOGSS_MASK GENMASK(5, 3) +#define STATUS1A_TOGSS_SRC_CC1 (1 << 3) +#define STATUS1A_TOGSS_SRC_CC2 (2 << 3) +#define STATUS1A_TOGSS_SNK_CC1 (5 << 3) +#define STATUS1A_TOGSS_SNK_CC2 (6 << 3) + +#define REG_INTERRUPTA 0x3e +#define INTERRUPTA_TOGDONE BIT(6) + +#define REG_STATUS0 0x40 +#define STATUS0_BC_LEVEL_MASK GENMASK(1, 0) +#define STATUS0_BC_LEVEL_VRA 0 +#define STATUS0_BC_LEVEL_VRDUSB 1 +#define STATUS0_BC_LEVEL_VRD15 2 +#define STATUS0_BC_LEVEL_VRD30 3 +#define STATUS0_COMP BIT(5) +#define STATUS0_VBUSOK BIT(7) + +#define REG_INTERRUPT 0x42 +#define INTERRUPT_BC_LVL BIT(0) +#define INTERRUPT_COMP_CHNG BIT(5) +#define INTERRUPT_VBUSOK BIT(7) + +/* Timeouts from the FUSB302 datasheet */ +#define TTOGCYCLE msecs_to_jiffies(40 + 60 + 40) + +/* Timeouts from the USB-C specification */ +#define TPDDEBOUNCE msecs_to_jiffies(20) +#define TDRPTRY_MS 75 +#define TDRPTRY msecs_to_jiffies(TDRPTRY_MS) +#define TDRPTRYWAIT msecs_to_jiffies(400) +#define TVBUSON msecs_to_jiffies(275) + +enum typec_port_type { + TYPEC_PORT_DFP, + TYPEC_PORT_UFP, + TYPEC_PORT_DRP, +}; + +enum typec_role { + TYPEC_SINK, + TYPEC_SOURCE, +}; + +enum fusb302_state { + DISABLED_SNK, /* UFP */ + DISABLED_SRC, /* DFP */ + DISABLED_DRP, + UNATTACHED_SNK, + ATTACHED_SNK, + UNATTACHED_SRC, + ATTACHED_SRC, +}; +/* For debugging */ +static __maybe_unused const char *fusb302_state_str[] = { + "DISABLED_SNK", + "DISABLED_SRC", + "DISABLED_DRP", + "UNATTACHED_SNK", + "ATTACHED_SNK", + "UNATTACHED_SRC", + "ATTACHED_SRC", +}; + +enum fusb302_event { + TOGGLE_DONE, + BC_LVL_CHANGE, + VBUS_VALID, + VBUS_INVALID, + COMP_LOW, + COMP_HIGH, + TIMEOUT, + FALLBACK_TIMEOUT, +}; +/* For debugging */ +static __maybe_unused const char *fusb302_event_str[] = { + "TOGGLE_DONE", + "BC_LVL_CHANGE", + "VBUS_VALID", + "VBUS_INVALID", + "COMP_LOW", + "COMP_HIGH", + "TIMEOUT", + "FALLBACK_TIMEOUT", +}; + +static const unsigned int fusb302_extcon_cables[] = { + EXTCON_USB, + EXTCON_USB_HOST, + EXTCON_CHG_USB_SDP, + EXTCON_CHG_USB_CDP, + EXTCON_CHG_USB_FAST, + EXTCON_NONE, +}; + +struct fusb302_data { + struct device *dev; + struct regmap *regmap; + struct extcon_dev *edev; + struct mutex lock; + struct delayed_work timeout; + struct delayed_work fallback_timeout; + struct delayed_work bc_work; + enum fusb302_state state; + enum typec_port_type port_type; + enum typec_role preferred_role; + int status0; + int status1a; + unsigned int snk_cable_id; +}; + +static void fusb302_write_reg(struct fusb302_data *data, int reg, int val) +{ + int ret; + + ret = regmap_write(data->regmap, reg, val); + if (ret) + dev_err(data->dev, "Error writing reg %02x: %d\n", reg, ret); +} + +static int fusb302_read_reg(struct fusb302_data *data, int reg) +{ + int ret, val; + + ret = regmap_read(data->regmap, reg, &val); + if (ret) { + dev_err(data->dev, "Error reading reg %02x: %d\n", reg, ret); + return 0; + } + + return val; +} + +static void fusb302_update_bits(struct fusb302_data *data, int reg, + int mask, int val) +{ + int ret; + + ret = regmap_update_bits(data->regmap, reg, mask, val); + if (ret) + dev_err(data->dev, "Error modifying reg %02x: %d\n", reg, ret); +} + +/* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ +static void fusb302_set_extcon_state(struct fusb302_data *data, + unsigned int cable, bool state) +{ + extcon_set_state_sync(data->edev, cable, state); + if (cable == EXTCON_CHG_USB_SDP) + extcon_set_state_sync(data->edev, EXTCON_USB, state); +} + +/* Helper for the disabled states */ +static void fusb302_disable(struct fusb302_data *data) +{ + fusb302_write_reg(data, REG_POWER, POWER_DISABLED); + fusb302_write_reg(data, REG_MASK1, MASK1_DISABLED); + fusb302_write_reg(data, REG_MASKA, MASKA_DISABLED); + fusb302_write_reg(data, REG_MASKB, MASKB_DISABLED); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); +} + +/* + * fusb302_set_state() and fusb302_handle_event() implement the 3 state + * machines from the datasheet folded into 1 state-machine for code re-use. + */ +static void fusb302_set_state(struct fusb302_data *data, + enum fusb302_state state) +{ + int status, switches0 = 0; + enum fusb302_state old_state = data->state; + union extcon_property_value prop; + + /* Kill pending timeouts from previous state */ + cancel_delayed_work(&data->timeout); + cancel_delayed_work(&data->fallback_timeout); + cancel_delayed_work(&data->bc_work); + + dev_dbg(data->dev, "New state %s\n", fusb302_state_str[state]); + + switch (old_state) { + case ATTACHED_SNK: + fusb302_set_extcon_state(data, data->snk_cable_id, false); + break; + case ATTACHED_SRC: + fusb302_set_extcon_state(data, EXTCON_USB_HOST, false); + break; + default: + break; /* Do nothing */ + } + + switch (state) { + case DISABLED_SNK: + fusb302_disable(data); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, + CONTROL2_MODE_SNK); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, + CONTROL2_TOGGLE); + break; + + case DISABLED_SRC: + fusb302_disable(data); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, + CONTROL2_MODE_SRC); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, + CONTROL2_TOGGLE); + break; + + case DISABLED_DRP: + fusb302_disable(data); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, + CONTROL2_MODE_DRP); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, + CONTROL2_TOGGLE); + break; + + case UNATTACHED_SNK: + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); + + /* Enable pull-down on CC1 / CC2 based on orientation */ + switch (data->status1a & STATUS1A_TOGSS_MASK) { + case STATUS1A_TOGSS_SNK_CC1: + switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; + prop.intval = USB_TYPEC_POLARITY_NORMAL; + break; + case STATUS1A_TOGSS_SNK_CC2: + switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; + prop.intval = USB_TYPEC_POLARITY_FLIP; + break; + } + fusb302_write_reg(data, REG_SWITCHES0, switches0); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); + extcon_set_property(data->edev, EXTCON_USB, + EXTCON_PROP_USB_TYPEC_POLARITY, prop); + + /* Enable VBUSOK and COMP irq at 2.24V for BC detection */ + fusb302_update_bits(data, REG_MASK1, + MASK1_VBUSOK | MASK1_COMP_CHNG, 0); + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, + MEASURE_MDAC_SNK_VRD30); + + status = fusb302_read_reg(data, REG_STATUS0); + if (status & STATUS0_VBUSOK) { + /* Go straight to ATTACHED_SNK */ + fusb302_set_state(data, ATTACHED_SNK); + return; + } + + mod_delayed_work(system_wq, &data->timeout, TVBUSON); + break; + + case ATTACHED_SNK: + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); + break; + + case UNATTACHED_SRC: + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); + + /* Enable pull-up / Vconn on CC1 / CC2 based on orientation */ + switch (data->status1a & STATUS1A_TOGSS_MASK) { + case STATUS1A_TOGSS_SRC_CC1: + switches0 = SWITCHES0_PU_EN1 | SWITCHES0_VCONN_CC2 | + SWITCHES0_MEAS_CC1; + prop.intval = USB_TYPEC_POLARITY_NORMAL; + break; + case STATUS1A_TOGSS_SRC_CC2: + switches0 = SWITCHES0_PU_EN2 | SWITCHES0_VCONN_CC1 | + SWITCHES0_MEAS_CC2; + prop.intval = USB_TYPEC_POLARITY_FLIP; + break; + } + fusb302_write_reg(data, REG_SWITCHES0, switches0); + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); + extcon_set_property(data->edev, EXTCON_USB, + EXTCON_PROP_USB_TYPEC_POLARITY, prop); + + /* Enable COMP irq at 1.6V for detach detection */ + fusb302_update_bits(data, REG_MASK1, MASK1_COMP_CHNG, 0); + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, + MEASURE_MDAC_SRC_80UA_HOST_CUR); + + status = fusb302_read_reg(data, REG_STATUS0); + if (!(status & STATUS0_COMP)) { + /* Go straight to ATTACHED_SRC */ + fusb302_set_state(data, ATTACHED_SRC); + return; + } + + mod_delayed_work(system_wq, &data->timeout, TPDDEBOUNCE); + break; + + case ATTACHED_SRC: + fusb302_set_extcon_state(data, EXTCON_USB_HOST, true); + break; + } + + data->state = state; +} + +static void fusb302_set_state_disabled(struct fusb302_data *data) +{ + + switch (data->port_type) { + case TYPEC_PORT_UFP: + fusb302_set_state(data, DISABLED_SNK); + break; + case TYPEC_PORT_DFP: + fusb302_set_state(data, DISABLED_SRC); + break; + case TYPEC_PORT_DRP: + fusb302_set_state(data, DISABLED_DRP); + break; + } +} + +static void fusb302_handle_disabled_snk_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case TOGGLE_DONE: + switch (data->status1a & STATUS1A_TOGSS_MASK) { + case STATUS1A_TOGSS_SNK_CC1: + case STATUS1A_TOGSS_SNK_CC2: + fusb302_set_state(data, UNATTACHED_SNK); + break; + } + break; + + case TIMEOUT: + /* Cannot become snk fallback to src */ + fusb302_set_state(data, DISABLED_SRC); + mod_delayed_work(system_wq, &data->fallback_timeout, + TDRPTRYWAIT); + break; + + case FALLBACK_TIMEOUT: + /* Both states failed return to disabled drp state */ + fusb302_set_state(data, DISABLED_DRP); + break; + } +} + +static void fusb302_handle_disabled_src_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case TOGGLE_DONE: + switch (data->status1a & STATUS1A_TOGSS_MASK) { + case STATUS1A_TOGSS_SRC_CC1: + case STATUS1A_TOGSS_SRC_CC2: + fusb302_set_state(data, UNATTACHED_SRC); + break; + } + break; + + case TIMEOUT: + /* Cannot become src fallback to snk */ + fusb302_set_state(data, DISABLED_SNK); + mod_delayed_work(system_wq, &data->fallback_timeout, + TDRPTRYWAIT); + break; + + case FALLBACK_TIMEOUT: + /* Both states failed return to disabled drp state */ + fusb302_set_state(data, DISABLED_DRP); + break; + } +} + +static void fusb302_handle_disabled_drp_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case TOGGLE_DONE: + switch (data->status1a & STATUS1A_TOGSS_MASK) { + case STATUS1A_TOGSS_SNK_CC1: + case STATUS1A_TOGSS_SNK_CC2: + if (data->preferred_role == TYPEC_SINK) { + /* Jump directly to UNATTACHED_SNK */ + fusb302_set_state(data, UNATTACHED_SNK); + } else { + /* Try to become src */ + fusb302_set_state(data, DISABLED_SNK); + mod_delayed_work(system_wq, &data->timeout, + TDRPTRY); + } + break; + case STATUS1A_TOGSS_SRC_CC1: + case STATUS1A_TOGSS_SRC_CC2: + if (data->preferred_role == TYPEC_SOURCE) { + /* Jump directly to UNATTACHED_SRC */ + fusb302_set_state(data, UNATTACHED_SRC); + } else { + /* + * The USB-C spec says we must enable pull-downs + * and then wait tDRPTry before checking to + * avoid endless role-bouncing if both ends + * prefer the snk role. + */ + fusb302_write_reg(data, REG_SWITCHES0, + SWITCHES0_PDWN); + fusb302_update_bits(data, REG_CONTROL2, + CONTROL2_TOGGLE, 0); + msleep(TDRPTRY_MS); + /* + * Use the toggle engine to do the src + * detection to keep things the same as for + * directly entering the src role. + */ + fusb302_set_state(data, DISABLED_SNK); + mod_delayed_work(system_wq, &data->timeout, + TTOGCYCLE); + } + break; + } + break; + } +} + +static void fusb302_handle_unattached_snk_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case VBUS_VALID: /* Cable attached */ + fusb302_set_state(data, ATTACHED_SNK); + break; + case TIMEOUT: + fusb302_set_state_disabled(data); + break; + } +} + +static void fusb302_handle_attached_snk_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case BC_LVL_CHANGE: + case COMP_LOW: + case COMP_HIGH: + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); + break; + case VBUS_INVALID: /* Cable detached */ + fusb302_set_state_disabled(data); + break; + } +} + +static void fusb302_handle_unattached_src_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case COMP_LOW: /* Cable attached */ + fusb302_set_state(data, ATTACHED_SRC); + break; + case TIMEOUT: + fusb302_set_state_disabled(data); + break; + } +} + +static void fusb302_handle_attached_src_event(struct fusb302_data *data, + int event) +{ + switch (event) { + case COMP_HIGH: /* Cable detached */ + fusb302_set_state_disabled(data); + break; + } +} + +static void fusb302_handle_event(struct fusb302_data *data, int event) +{ + + mutex_lock(&data->lock); + + dev_dbg(data->dev, "Handling state %s event %s status %02x %02x\n", + fusb302_state_str[data->state], fusb302_event_str[event], + fusb302_read_reg(data, REG_STATUS0), + fusb302_read_reg(data, REG_STATUS1A)); + + switch (data->state) { + case DISABLED_SNK: + fusb302_handle_disabled_snk_event(data, event); + break; + case DISABLED_SRC: + fusb302_handle_disabled_src_event(data, event); + break; + case DISABLED_DRP: + fusb302_handle_disabled_drp_event(data, event); + break; + case UNATTACHED_SNK: + fusb302_handle_unattached_snk_event(data, event); + break; + case ATTACHED_SNK: + fusb302_handle_attached_snk_event(data, event); + break; + case UNATTACHED_SRC: + fusb302_handle_unattached_src_event(data, event); + break; + case ATTACHED_SRC: + fusb302_handle_attached_src_event(data, event); + break; + } + mutex_unlock(&data->lock); +} + +static void fusb302_timeout(struct work_struct *work) +{ + struct fusb302_data *data = + container_of(work, struct fusb302_data, timeout.work); + + fusb302_handle_event(data, TIMEOUT); +} + +static void fusb302_fallback_timeout(struct work_struct *work) +{ + struct fusb302_data *data = + container_of(work, struct fusb302_data, fallback_timeout.work); + + fusb302_handle_event(data, FALLBACK_TIMEOUT); +} + +static void fusb302_report_bc_level(struct work_struct *work) +{ + struct fusb302_data *data = + container_of(work, struct fusb302_data, bc_work.work); + + /* Clear old charger cable id */ + fusb302_set_extcon_state(data, data->snk_cable_id, false); + + if (data->status0 & STATUS0_COMP) { + dev_warn(data->dev, "vRd over maximum, assuming 500mA source\n"); + data->snk_cable_id = EXTCON_CHG_USB_SDP; + } else { + switch (data->status0 & STATUS0_BC_LEVEL_MASK) { + case STATUS0_BC_LEVEL_VRA: + case STATUS0_BC_LEVEL_VRDUSB: + data->snk_cable_id = EXTCON_CHG_USB_SDP; + break; + case STATUS0_BC_LEVEL_VRD15: + data->snk_cable_id = EXTCON_CHG_USB_CDP; + break; + case STATUS0_BC_LEVEL_VRD30: + /* Use CHG_USB_FAST to indicate 3A capability */ + data->snk_cable_id = EXTCON_CHG_USB_FAST; + break; + } + } + fusb302_set_extcon_state(data, data->snk_cable_id, true); +} + +static irqreturn_t fusb302_irq_handler_thread(int irq, void *handler_data) +{ + struct fusb302_data *data = handler_data; + int interrupt, interrupta; + + interrupt = fusb302_read_reg(data, REG_INTERRUPT); + interrupta = fusb302_read_reg(data, REG_INTERRUPTA); + + if (interrupt) + data->status0 = fusb302_read_reg(data, REG_STATUS0); + + if (interrupta) + data->status1a = fusb302_read_reg(data, REG_STATUS1A); + + dev_dbg(data->dev, "Handling interrupt %02x %02x status %02x %02x\n", + interrupt, interrupta, data->status0, data->status1a); + + if (interrupt & INTERRUPT_BC_LVL) + fusb302_handle_event(data, BC_LVL_CHANGE); + + if (interrupt & INTERRUPT_COMP_CHNG) { + if (data->status0 & STATUS0_COMP) + fusb302_handle_event(data, COMP_HIGH); + else + fusb302_handle_event(data, COMP_LOW); + } + + if (interrupt & INTERRUPT_VBUSOK) { + if (data->status0 & STATUS0_VBUSOK) + fusb302_handle_event(data, VBUS_VALID); + else + fusb302_handle_event(data, VBUS_INVALID); + } + + if (interrupta & INTERRUPTA_TOGDONE) + fusb302_handle_event(data, TOGGLE_DONE); + + return IRQ_HANDLED; +} + +static const struct regmap_config fusb302_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .val_format_endian = REGMAP_ENDIAN_NATIVE, +}; + +static int fusb302_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct fusb302_data *data; + int ret; + + if (!client->irq) { + dev_err(dev, "Error irq not specified\n"); + return -EINVAL; + } + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->dev = dev; + /* TODO make these 2 configurable using device-properties */ + data->port_type = TYPEC_PORT_DRP; + data->preferred_role = TYPEC_SINK; + + data->regmap = devm_regmap_init_i2c(client, &fusb302_regmap_config); + if (IS_ERR(data->regmap)) { + ret = PTR_ERR(data->regmap); + dev_err(dev, "Error to initializing regmap: %d\n", ret); + return ret; + } + + mutex_init(&data->lock); + INIT_DELAYED_WORK(&data->timeout, fusb302_timeout); + INIT_DELAYED_WORK(&data->fallback_timeout, fusb302_fallback_timeout); + INIT_DELAYED_WORK(&data->bc_work, fusb302_report_bc_level); + + data->edev = devm_extcon_dev_allocate(dev, fusb302_extcon_cables); + if (IS_ERR(data->edev)) + return PTR_ERR(data->edev); + + data->edev->name = "fusb302"; + + ret = devm_extcon_dev_register(dev, data->edev); + if (ret) + return ret; + + extcon_set_property_capability(data->edev, EXTCON_USB, + EXTCON_PROP_USB_TYPEC_POLARITY); + + fusb302_write_reg(data, REG_RESET, RESET_SW_RESET); + usleep_range(10000, 20000); + + /* Enable power-saving */ + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_SAVE_PWR_MASK, + CONTROL2_SAVE_PWR_40MS); + + fusb302_set_state_disabled(data); + + ret = devm_request_threaded_irq(dev, client->irq, NULL, + fusb302_irq_handler_thread, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, "fusb302", data); + if (ret) { + dev_err(dev, "Error requesting irq: %d\n", ret); + return ret; + } + + fusb302_update_bits(data, REG_CONTROL0, CONTROL0_INT_MASK, 0); + + i2c_set_clientdata(client, data); + return 0; +} + +static int fusb302_remove(struct i2c_client *client) +{ + struct fusb302_data *data = i2c_get_clientdata(client); + + devm_free_irq(data->dev, client->irq, data); + cancel_delayed_work_sync(&data->timeout); + cancel_delayed_work_sync(&data->fallback_timeout); + + return 0; +} + +static const struct i2c_device_id fusb302_i2c_ids[] = { + { "fusb302" }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, fusb302_i2c_ids); + +static struct i2c_driver fusb302_driver = { + .probe_new = fusb302_probe, + .remove = fusb302_remove, + .id_table = fusb302_i2c_ids, + .driver = { + .name = "fusb302", + }, +}; +module_i2c_driver(fusb302_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); +MODULE_DESCRIPTION("FUSB302 USB TYPE-C controller Driver"); -- 2.9.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede @ 2017-04-21 18:51 ` Andy Shevchenko 2017-04-24 11:02 ` Heikki Krogerus 2017-04-21 23:23 ` kbuild test robot 2017-05-23 9:27 ` Chanwoo Choi 2 siblings, 1 reply; 26+ messages in thread From: Andy Shevchenko @ 2017-04-21 18:51 UTC (permalink / raw) To: Hans de Goede, Krogerus, Heikki Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org +Cc: Heikki. He might comment on this. On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Add support for USB TYPE-C cable detection on systems using a > FUSB302 USB TYPE-C controller. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/Kconfig | 8 + > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 791 insertions(+) > create mode 100644 drivers/extcon/extcon-fusb302.c > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index 32f2dc8..562db5b 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -35,6 +35,14 @@ config EXTCON_AXP288 > Say Y here to enable support for USB peripheral detection > and USB MUX switching by X-Power AXP288 PMIC. > > +config EXTCON_FUSB302 > + tristate "FUSB302 USB TYPE-C controller support" > + depends on I2C > + select REGMAP_I2C > + help > + Say Y here to enable support for USB TYPE-C cable detection using > + a FUSB302 USB TYPE-C controller. > + > config EXTCON_GPIO > tristate "GPIO extcon support" > depends on GPIOLIB || COMPILE_TEST > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index ecfa958..e5eb493 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o > +obj-$(CONFIG_EXTCON_FUSB302) += extcon-fusb302.o > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o > obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o > obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o > diff --git a/drivers/extcon/extcon-fusb302.c b/drivers/extcon/extcon-fusb302.c > new file mode 100644 > index 0000000..577adb9 > --- /dev/null > +++ b/drivers/extcon/extcon-fusb302.c > @@ -0,0 +1,782 @@ > +/* > + * Extcon USB-C cable detection driver for FUSB302 USB TYPE-C controller > + * > + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/extcon.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > +#include "extcon.h" > + > +#define REG_DEVICE_ID 0x01 > +#define DEVICE_ID_VER_MASK GENMASK(7, 4) > +#define DEVICE_ID_FUSB302_VER 0x80 > + > +#define REG_SWITCHES0 0x02 > +#define SWITCHES0_PDWN1 BIT(0) > +#define SWITCHES0_PDWN2 BIT(1) > +#define SWITCHES0_PDWN GENMASK(1, 0) > +#define SWITCHES0_MEAS_CC1 BIT(2) > +#define SWITCHES0_MEAS_CC2 BIT(3) > +#define SWITCHES0_VCONN_CC1 BIT(4) > +#define SWITCHES0_VCONN_CC2 BIT(5) > +#define SWITCHES0_PU_EN1 BIT(6) > +#define SWITCHES0_PU_EN2 BIT(7) > + > +#define REG_MEASURE 0x04 > +#define MEASURE_MDAC_MASK GENMASK(5, 0) > +/* Datasheet says MDAC must be set to 0x34 / 2226mV for vRd-3.0 detection */ > +#define MEASURE_MDAC_SNK_VRD30 0x34 > +/* MDAC must be set to 0x25 / 1600mV for disconnect det. with 80uA host-cur */ > +#define MEASURE_MDAC_SRC_80UA_HOST_CUR 0x25 > + > +#define REG_CONTROL0 0x06 > +#define CONTROL0_HOST_CUR_MASK GENMASK(3, 2) > +#define CONTROL0_HOST_CUR_DISABLED (0 << 2) > +#define CONTROL0_HOST_CUR_80UA (1 << 2) > +#define CONTROL0_HOST_CUR_180UA (2 << 2) > +#define CONTROL0_HOST_CUR_330UA (3 << 2) > +#define CONTROL0_INT_MASK BIT(5) > + > +#define REG_CONTROL2 0x08 > +#define CONTROL2_TOGGLE BIT(0) > +#define CONTROL2_MODE_MASK GENMASK(2, 1) > +#define CONTROL2_MODE_DRP (1 << 1) > +#define CONTROL2_MODE_SNK (2 << 1) > +#define CONTROL2_MODE_SRC (3 << 1) > +#define CONTROL2_SAVE_PWR_MASK GENMASK(7, 6) > +#define CONTROL2_SAVE_PWR_DISABLED (0 << 6) > +#define CONTROL2_SAVE_PWR_40MS (1 << 6) > +#define CONTROL2_SAVE_PWR_80MS (2 << 6) > +#define CONTROL2_SAVE_PWR_160MS (3 << 6) > + > +#define REG_MASK1 0x0a > +/* REG_MASK1 value for low-power / disabled state from datasheet */ > +#define MASK1_DISABLED 0xfe > +#define MASK1_COMP_CHNG BIT(5) > +#define MASK1_VBUSOK BIT(7) > + > +#define REG_POWER 0x0b > +/* REG_POWER values for disabled and normal state from datasheet */ > +#define POWER_DISABLED BIT(0) > +#define POWER_NORMAL GENMASK(2, 0) > + > +#define REG_RESET 0x0c > +#define RESET_SW_RESET BIT(0) > + > +#define REG_OCP 0x0d > + > +#define REG_MASKA 0x0e > +/* REG_MASKA value for low-power / disabled state from datasheet */ > +#define MASKA_DISABLED 0xbf > + > +#define REG_MASKB 0x0f > +/* REG_MASKB value for low-power / disabled state from datasheet */ > +#define MASKB_DISABLED 0x01 > + > +#define REG_STATUS1A 0x3d > +#define STATUS1A_TOGSS_MASK GENMASK(5, 3) > +#define STATUS1A_TOGSS_SRC_CC1 (1 << 3) > +#define STATUS1A_TOGSS_SRC_CC2 (2 << 3) > +#define STATUS1A_TOGSS_SNK_CC1 (5 << 3) > +#define STATUS1A_TOGSS_SNK_CC2 (6 << 3) > + > +#define REG_INTERRUPTA 0x3e > +#define INTERRUPTA_TOGDONE BIT(6) > + > +#define REG_STATUS0 0x40 > +#define STATUS0_BC_LEVEL_MASK GENMASK(1, 0) > +#define STATUS0_BC_LEVEL_VRA 0 > +#define STATUS0_BC_LEVEL_VRDUSB 1 > +#define STATUS0_BC_LEVEL_VRD15 2 > +#define STATUS0_BC_LEVEL_VRD30 3 > +#define STATUS0_COMP BIT(5) > +#define STATUS0_VBUSOK BIT(7) > + > +#define REG_INTERRUPT 0x42 > +#define INTERRUPT_BC_LVL BIT(0) > +#define INTERRUPT_COMP_CHNG BIT(5) > +#define INTERRUPT_VBUSOK BIT(7) > + > +/* Timeouts from the FUSB302 datasheet */ > +#define TTOGCYCLE msecs_to_jiffies(40 + 60 + 40) > + > +/* Timeouts from the USB-C specification */ > +#define TPDDEBOUNCE msecs_to_jiffies(20) > +#define TDRPTRY_MS 75 > +#define TDRPTRY msecs_to_jiffies(TDRPTRY_MS) > +#define TDRPTRYWAIT msecs_to_jiffies(400) > +#define TVBUSON msecs_to_jiffies(275) > + > +enum typec_port_type { > + TYPEC_PORT_DFP, > + TYPEC_PORT_UFP, > + TYPEC_PORT_DRP, > +}; > + > +enum typec_role { > + TYPEC_SINK, > + TYPEC_SOURCE, > +}; > + > +enum fusb302_state { > + DISABLED_SNK, /* UFP */ > + DISABLED_SRC, /* DFP */ > + DISABLED_DRP, > + UNATTACHED_SNK, > + ATTACHED_SNK, > + UNATTACHED_SRC, > + ATTACHED_SRC, > +}; > +/* For debugging */ > +static __maybe_unused const char *fusb302_state_str[] = { > + "DISABLED_SNK", > + "DISABLED_SRC", > + "DISABLED_DRP", > + "UNATTACHED_SNK", > + "ATTACHED_SNK", > + "UNATTACHED_SRC", > + "ATTACHED_SRC", > +}; > + > +enum fusb302_event { > + TOGGLE_DONE, > + BC_LVL_CHANGE, > + VBUS_VALID, > + VBUS_INVALID, > + COMP_LOW, > + COMP_HIGH, > + TIMEOUT, > + FALLBACK_TIMEOUT, > +}; > +/* For debugging */ > +static __maybe_unused const char *fusb302_event_str[] = { > + "TOGGLE_DONE", > + "BC_LVL_CHANGE", > + "VBUS_VALID", > + "VBUS_INVALID", > + "COMP_LOW", > + "COMP_HIGH", > + "TIMEOUT", > + "FALLBACK_TIMEOUT", > +}; > + > +static const unsigned int fusb302_extcon_cables[] = { > + EXTCON_USB, > + EXTCON_USB_HOST, > + EXTCON_CHG_USB_SDP, > + EXTCON_CHG_USB_CDP, > + EXTCON_CHG_USB_FAST, > + EXTCON_NONE, > +}; > + > +struct fusb302_data { > + struct device *dev; > + struct regmap *regmap; > + struct extcon_dev *edev; > + struct mutex lock; > + struct delayed_work timeout; > + struct delayed_work fallback_timeout; > + struct delayed_work bc_work; > + enum fusb302_state state; > + enum typec_port_type port_type; > + enum typec_role preferred_role; > + int status0; > + int status1a; > + unsigned int snk_cable_id; > +}; > + > +static void fusb302_write_reg(struct fusb302_data *data, int reg, int val) > +{ > + int ret; > + > + ret = regmap_write(data->regmap, reg, val); > + if (ret) > + dev_err(data->dev, "Error writing reg %02x: %d\n", reg, ret); > +} > + > +static int fusb302_read_reg(struct fusb302_data *data, int reg) > +{ > + int ret, val; > + > + ret = regmap_read(data->regmap, reg, &val); > + if (ret) { > + dev_err(data->dev, "Error reading reg %02x: %d\n", reg, ret); > + return 0; > + } > + > + return val; > +} > + > +static void fusb302_update_bits(struct fusb302_data *data, int reg, > + int mask, int val) > +{ > + int ret; > + > + ret = regmap_update_bits(data->regmap, reg, mask, val); > + if (ret) > + dev_err(data->dev, "Error modifying reg %02x: %d\n", reg, ret); > +} > + > +/* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ > +static void fusb302_set_extcon_state(struct fusb302_data *data, > + unsigned int cable, bool state) > +{ > + extcon_set_state_sync(data->edev, cable, state); > + if (cable == EXTCON_CHG_USB_SDP) > + extcon_set_state_sync(data->edev, EXTCON_USB, state); > +} > + > +/* Helper for the disabled states */ > +static void fusb302_disable(struct fusb302_data *data) > +{ > + fusb302_write_reg(data, REG_POWER, POWER_DISABLED); > + fusb302_write_reg(data, REG_MASK1, MASK1_DISABLED); > + fusb302_write_reg(data, REG_MASKA, MASKA_DISABLED); > + fusb302_write_reg(data, REG_MASKB, MASKB_DISABLED); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > +} > + > +/* > + * fusb302_set_state() and fusb302_handle_event() implement the 3 state > + * machines from the datasheet folded into 1 state-machine for code re-use. > + */ > +static void fusb302_set_state(struct fusb302_data *data, > + enum fusb302_state state) > +{ > + int status, switches0 = 0; > + enum fusb302_state old_state = data->state; > + union extcon_property_value prop; > + > + /* Kill pending timeouts from previous state */ > + cancel_delayed_work(&data->timeout); > + cancel_delayed_work(&data->fallback_timeout); > + cancel_delayed_work(&data->bc_work); > + > + dev_dbg(data->dev, "New state %s\n", fusb302_state_str[state]); > + > + switch (old_state) { > + case ATTACHED_SNK: > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > + break; > + case ATTACHED_SRC: > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, false); > + break; > + default: > + break; /* Do nothing */ > + } > + > + switch (state) { > + case DISABLED_SNK: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_SNK); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case DISABLED_SRC: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_SRC); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case DISABLED_DRP: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_DRP); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case UNATTACHED_SNK: > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > + > + /* Enable pull-down on CC1 / CC2 based on orientation */ > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > + break; > + case STATUS1A_TOGSS_SNK_CC2: > + switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; > + prop.intval = USB_TYPEC_POLARITY_FLIP; > + break; > + } > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > + extcon_set_property(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > + > + /* Enable VBUSOK and COMP irq at 2.24V for BC detection */ > + fusb302_update_bits(data, REG_MASK1, > + MASK1_VBUSOK | MASK1_COMP_CHNG, 0); > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > + MEASURE_MDAC_SNK_VRD30); > + > + status = fusb302_read_reg(data, REG_STATUS0); > + if (status & STATUS0_VBUSOK) { > + /* Go straight to ATTACHED_SNK */ > + fusb302_set_state(data, ATTACHED_SNK); > + return; > + } > + > + mod_delayed_work(system_wq, &data->timeout, TVBUSON); > + break; > + > + case ATTACHED_SNK: > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > + break; > + > + case UNATTACHED_SRC: > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > + > + /* Enable pull-up / Vconn on CC1 / CC2 based on orientation */ > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SRC_CC1: > + switches0 = SWITCHES0_PU_EN1 | SWITCHES0_VCONN_CC2 | > + SWITCHES0_MEAS_CC1; > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > + break; > + case STATUS1A_TOGSS_SRC_CC2: > + switches0 = SWITCHES0_PU_EN2 | SWITCHES0_VCONN_CC1 | > + SWITCHES0_MEAS_CC2; > + prop.intval = USB_TYPEC_POLARITY_FLIP; > + break; > + } > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > + extcon_set_property(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > + > + /* Enable COMP irq at 1.6V for detach detection */ > + fusb302_update_bits(data, REG_MASK1, MASK1_COMP_CHNG, 0); > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > + MEASURE_MDAC_SRC_80UA_HOST_CUR); > + > + status = fusb302_read_reg(data, REG_STATUS0); > + if (!(status & STATUS0_COMP)) { > + /* Go straight to ATTACHED_SRC */ > + fusb302_set_state(data, ATTACHED_SRC); > + return; > + } > + > + mod_delayed_work(system_wq, &data->timeout, TPDDEBOUNCE); > + break; > + > + case ATTACHED_SRC: > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, true); > + break; > + } > + > + data->state = state; > +} > + > +static void fusb302_set_state_disabled(struct fusb302_data *data) > +{ > + > + switch (data->port_type) { > + case TYPEC_PORT_UFP: > + fusb302_set_state(data, DISABLED_SNK); > + break; > + case TYPEC_PORT_DFP: > + fusb302_set_state(data, DISABLED_SRC); > + break; > + case TYPEC_PORT_DRP: > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + case STATUS1A_TOGSS_SNK_CC2: > + fusb302_set_state(data, UNATTACHED_SNK); > + break; > + } > + break; > + > + case TIMEOUT: > + /* Cannot become snk fallback to src */ > + fusb302_set_state(data, DISABLED_SRC); > + mod_delayed_work(system_wq, &data->fallback_timeout, > + TDRPTRYWAIT); > + break; > + > + case FALLBACK_TIMEOUT: > + /* Both states failed return to disabled drp state */ > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SRC_CC1: > + case STATUS1A_TOGSS_SRC_CC2: > + fusb302_set_state(data, UNATTACHED_SRC); > + break; > + } > + break; > + > + case TIMEOUT: > + /* Cannot become src fallback to snk */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->fallback_timeout, > + TDRPTRYWAIT); > + break; > + > + case FALLBACK_TIMEOUT: > + /* Both states failed return to disabled drp state */ > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_drp_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + case STATUS1A_TOGSS_SNK_CC2: > + if (data->preferred_role == TYPEC_SINK) { > + /* Jump directly to UNATTACHED_SNK */ > + fusb302_set_state(data, UNATTACHED_SNK); > + } else { > + /* Try to become src */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->timeout, > + TDRPTRY); > + } > + break; > + case STATUS1A_TOGSS_SRC_CC1: > + case STATUS1A_TOGSS_SRC_CC2: > + if (data->preferred_role == TYPEC_SOURCE) { > + /* Jump directly to UNATTACHED_SRC */ > + fusb302_set_state(data, UNATTACHED_SRC); > + } else { > + /* > + * The USB-C spec says we must enable pull-downs > + * and then wait tDRPTry before checking to > + * avoid endless role-bouncing if both ends > + * prefer the snk role. > + */ > + fusb302_write_reg(data, REG_SWITCHES0, > + SWITCHES0_PDWN); > + fusb302_update_bits(data, REG_CONTROL2, > + CONTROL2_TOGGLE, 0); > + msleep(TDRPTRY_MS); > + /* > + * Use the toggle engine to do the src > + * detection to keep things the same as for > + * directly entering the src role. > + */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->timeout, > + TTOGCYCLE); > + } > + break; > + } > + break; > + } > +} > + > +static void fusb302_handle_unattached_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case VBUS_VALID: /* Cable attached */ > + fusb302_set_state(data, ATTACHED_SNK); > + break; > + case TIMEOUT: > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_attached_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case BC_LVL_CHANGE: > + case COMP_LOW: > + case COMP_HIGH: > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > + break; > + case VBUS_INVALID: /* Cable detached */ > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_unattached_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case COMP_LOW: /* Cable attached */ > + fusb302_set_state(data, ATTACHED_SRC); > + break; > + case TIMEOUT: > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_attached_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case COMP_HIGH: /* Cable detached */ > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_event(struct fusb302_data *data, int event) > +{ > + > + mutex_lock(&data->lock); > + > + dev_dbg(data->dev, "Handling state %s event %s status %02x %02x\n", > + fusb302_state_str[data->state], fusb302_event_str[event], > + fusb302_read_reg(data, REG_STATUS0), > + fusb302_read_reg(data, REG_STATUS1A)); > + > + switch (data->state) { > + case DISABLED_SNK: > + fusb302_handle_disabled_snk_event(data, event); > + break; > + case DISABLED_SRC: > + fusb302_handle_disabled_src_event(data, event); > + break; > + case DISABLED_DRP: > + fusb302_handle_disabled_drp_event(data, event); > + break; > + case UNATTACHED_SNK: > + fusb302_handle_unattached_snk_event(data, event); > + break; > + case ATTACHED_SNK: > + fusb302_handle_attached_snk_event(data, event); > + break; > + case UNATTACHED_SRC: > + fusb302_handle_unattached_src_event(data, event); > + break; > + case ATTACHED_SRC: > + fusb302_handle_attached_src_event(data, event); > + break; > + } > + mutex_unlock(&data->lock); > +} > + > +static void fusb302_timeout(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, timeout.work); > + > + fusb302_handle_event(data, TIMEOUT); > +} > + > +static void fusb302_fallback_timeout(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, fallback_timeout.work); > + > + fusb302_handle_event(data, FALLBACK_TIMEOUT); > +} > + > +static void fusb302_report_bc_level(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, bc_work.work); > + > + /* Clear old charger cable id */ > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > + > + if (data->status0 & STATUS0_COMP) { > + dev_warn(data->dev, "vRd over maximum, assuming 500mA source\n"); > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > + } else { > + switch (data->status0 & STATUS0_BC_LEVEL_MASK) { > + case STATUS0_BC_LEVEL_VRA: > + case STATUS0_BC_LEVEL_VRDUSB: > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > + break; > + case STATUS0_BC_LEVEL_VRD15: > + data->snk_cable_id = EXTCON_CHG_USB_CDP; > + break; > + case STATUS0_BC_LEVEL_VRD30: > + /* Use CHG_USB_FAST to indicate 3A capability */ > + data->snk_cable_id = EXTCON_CHG_USB_FAST; > + break; > + } > + } > + fusb302_set_extcon_state(data, data->snk_cable_id, true); > +} > + > +static irqreturn_t fusb302_irq_handler_thread(int irq, void *handler_data) > +{ > + struct fusb302_data *data = handler_data; > + int interrupt, interrupta; > + > + interrupt = fusb302_read_reg(data, REG_INTERRUPT); > + interrupta = fusb302_read_reg(data, REG_INTERRUPTA); > + > + if (interrupt) > + data->status0 = fusb302_read_reg(data, REG_STATUS0); > + > + if (interrupta) > + data->status1a = fusb302_read_reg(data, REG_STATUS1A); > + > + dev_dbg(data->dev, "Handling interrupt %02x %02x status %02x %02x\n", > + interrupt, interrupta, data->status0, data->status1a); > + > + if (interrupt & INTERRUPT_BC_LVL) > + fusb302_handle_event(data, BC_LVL_CHANGE); > + > + if (interrupt & INTERRUPT_COMP_CHNG) { > + if (data->status0 & STATUS0_COMP) > + fusb302_handle_event(data, COMP_HIGH); > + else > + fusb302_handle_event(data, COMP_LOW); > + } > + > + if (interrupt & INTERRUPT_VBUSOK) { > + if (data->status0 & STATUS0_VBUSOK) > + fusb302_handle_event(data, VBUS_VALID); > + else > + fusb302_handle_event(data, VBUS_INVALID); > + } > + > + if (interrupta & INTERRUPTA_TOGDONE) > + fusb302_handle_event(data, TOGGLE_DONE); > + > + return IRQ_HANDLED; > +} > + > +static const struct regmap_config fusb302_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .val_format_endian = REGMAP_ENDIAN_NATIVE, > +}; > + > +static int fusb302_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct fusb302_data *data; > + int ret; > + > + if (!client->irq) { > + dev_err(dev, "Error irq not specified\n"); > + return -EINVAL; > + } > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->dev = dev; > + /* TODO make these 2 configurable using device-properties */ > + data->port_type = TYPEC_PORT_DRP; > + data->preferred_role = TYPEC_SINK; > + > + data->regmap = devm_regmap_init_i2c(client, &fusb302_regmap_config); > + if (IS_ERR(data->regmap)) { > + ret = PTR_ERR(data->regmap); > + dev_err(dev, "Error to initializing regmap: %d\n", ret); > + return ret; > + } > + > + mutex_init(&data->lock); > + INIT_DELAYED_WORK(&data->timeout, fusb302_timeout); > + INIT_DELAYED_WORK(&data->fallback_timeout, fusb302_fallback_timeout); > + INIT_DELAYED_WORK(&data->bc_work, fusb302_report_bc_level); > + > + data->edev = devm_extcon_dev_allocate(dev, fusb302_extcon_cables); > + if (IS_ERR(data->edev)) > + return PTR_ERR(data->edev); > + > + data->edev->name = "fusb302"; > + > + ret = devm_extcon_dev_register(dev, data->edev); > + if (ret) > + return ret; > + > + extcon_set_property_capability(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY); > + > + fusb302_write_reg(data, REG_RESET, RESET_SW_RESET); > + usleep_range(10000, 20000); > + > + /* Enable power-saving */ > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_SAVE_PWR_MASK, > + CONTROL2_SAVE_PWR_40MS); > + > + fusb302_set_state_disabled(data); > + > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > + fusb302_irq_handler_thread, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, "fusb302", data); > + if (ret) { > + dev_err(dev, "Error requesting irq: %d\n", ret); > + return ret; > + } > + > + fusb302_update_bits(data, REG_CONTROL0, CONTROL0_INT_MASK, 0); > + > + i2c_set_clientdata(client, data); > + return 0; > +} > + > +static int fusb302_remove(struct i2c_client *client) > +{ > + struct fusb302_data *data = i2c_get_clientdata(client); > + > + devm_free_irq(data->dev, client->irq, data); > + cancel_delayed_work_sync(&data->timeout); > + cancel_delayed_work_sync(&data->fallback_timeout); > + > + return 0; > +} > + > +static const struct i2c_device_id fusb302_i2c_ids[] = { > + { "fusb302" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, fusb302_i2c_ids); > + > +static struct i2c_driver fusb302_driver = { > + .probe_new = fusb302_probe, > + .remove = fusb302_remove, > + .id_table = fusb302_i2c_ids, > + .driver = { > + .name = "fusb302", > + }, > +}; > +module_i2c_driver(fusb302_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > +MODULE_DESCRIPTION("FUSB302 USB TYPE-C controller Driver"); > -- > 2.9.3 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-21 18:51 ` Andy Shevchenko @ 2017-04-24 11:02 ` Heikki Krogerus 2017-04-24 13:12 ` Guenter Roeck 0 siblings, 1 reply; 26+ messages in thread From: Heikki Krogerus @ 2017-04-24 11:02 UTC (permalink / raw) To: Hans de Goede, Guenter Roeck Cc: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org +Guenter On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote: > +Cc: Heikki. > > He might comment on this. Thanks Andy. > On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: > > Add support for USB TYPE-C cable detection on systems using a > > FUSB302 USB TYPE-C controller. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/extcon/Kconfig | 8 + > > drivers/extcon/Makefile | 1 + > > drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 791 insertions(+) > > create mode 100644 drivers/extcon/extcon-fusb302.c There is now the typec class in linux-next that really needs to be used with all USB Type-C PHYs like fusb302. Unless I'm mistaken, Guenter has also written a driver for fusb302. I have not seen it, but if I understood correctly, that driver will register itself with the upcoming tcpm (USB Type-C Port Manager) [1], which in practice would mean we can take properly advantage of the USB PD transceiver on fusb302. Guenter! Can you publish the fusb302 driver? [1] https://chromium-review.googlesource.com/c/389916/ > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > > index 32f2dc8..562db5b 100644 > > --- a/drivers/extcon/Kconfig > > +++ b/drivers/extcon/Kconfig > > @@ -35,6 +35,14 @@ config EXTCON_AXP288 > > Say Y here to enable support for USB peripheral detection > > and USB MUX switching by X-Power AXP288 PMIC. > > > > +config EXTCON_FUSB302 > > + tristate "FUSB302 USB TYPE-C controller support" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + Say Y here to enable support for USB TYPE-C cable detection using > > + a FUSB302 USB TYPE-C controller. > > + > > config EXTCON_GPIO > > tristate "GPIO extcon support" > > depends on GPIOLIB || COMPILE_TEST > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > > index ecfa958..e5eb493 100644 > > --- a/drivers/extcon/Makefile > > +++ b/drivers/extcon/Makefile > > @@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o > > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o > > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > > obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o > > +obj-$(CONFIG_EXTCON_FUSB302) += extcon-fusb302.o > > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o > > obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o > > obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o > > diff --git a/drivers/extcon/extcon-fusb302.c b/drivers/extcon/extcon-fusb302.c > > new file mode 100644 > > index 0000000..577adb9 > > --- /dev/null > > +++ b/drivers/extcon/extcon-fusb302.c > > @@ -0,0 +1,782 @@ > > +/* > > + * Extcon USB-C cable detection driver for FUSB302 USB TYPE-C controller > > + * > > + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/extcon.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > +#include <linux/workqueue.h> > > +#include "extcon.h" > > + > > +#define REG_DEVICE_ID 0x01 > > +#define DEVICE_ID_VER_MASK GENMASK(7, 4) > > +#define DEVICE_ID_FUSB302_VER 0x80 > > + > > +#define REG_SWITCHES0 0x02 > > +#define SWITCHES0_PDWN1 BIT(0) > > +#define SWITCHES0_PDWN2 BIT(1) > > +#define SWITCHES0_PDWN GENMASK(1, 0) > > +#define SWITCHES0_MEAS_CC1 BIT(2) > > +#define SWITCHES0_MEAS_CC2 BIT(3) > > +#define SWITCHES0_VCONN_CC1 BIT(4) > > +#define SWITCHES0_VCONN_CC2 BIT(5) > > +#define SWITCHES0_PU_EN1 BIT(6) > > +#define SWITCHES0_PU_EN2 BIT(7) > > + > > +#define REG_MEASURE 0x04 > > +#define MEASURE_MDAC_MASK GENMASK(5, 0) > > +/* Datasheet says MDAC must be set to 0x34 / 2226mV for vRd-3.0 detection */ > > +#define MEASURE_MDAC_SNK_VRD30 0x34 > > +/* MDAC must be set to 0x25 / 1600mV for disconnect det. with 80uA host-cur */ > > +#define MEASURE_MDAC_SRC_80UA_HOST_CUR 0x25 > > + > > +#define REG_CONTROL0 0x06 > > +#define CONTROL0_HOST_CUR_MASK GENMASK(3, 2) > > +#define CONTROL0_HOST_CUR_DISABLED (0 << 2) > > +#define CONTROL0_HOST_CUR_80UA (1 << 2) > > +#define CONTROL0_HOST_CUR_180UA (2 << 2) > > +#define CONTROL0_HOST_CUR_330UA (3 << 2) > > +#define CONTROL0_INT_MASK BIT(5) > > + > > +#define REG_CONTROL2 0x08 > > +#define CONTROL2_TOGGLE BIT(0) > > +#define CONTROL2_MODE_MASK GENMASK(2, 1) > > +#define CONTROL2_MODE_DRP (1 << 1) > > +#define CONTROL2_MODE_SNK (2 << 1) > > +#define CONTROL2_MODE_SRC (3 << 1) > > +#define CONTROL2_SAVE_PWR_MASK GENMASK(7, 6) > > +#define CONTROL2_SAVE_PWR_DISABLED (0 << 6) > > +#define CONTROL2_SAVE_PWR_40MS (1 << 6) > > +#define CONTROL2_SAVE_PWR_80MS (2 << 6) > > +#define CONTROL2_SAVE_PWR_160MS (3 << 6) > > + > > +#define REG_MASK1 0x0a > > +/* REG_MASK1 value for low-power / disabled state from datasheet */ > > +#define MASK1_DISABLED 0xfe > > +#define MASK1_COMP_CHNG BIT(5) > > +#define MASK1_VBUSOK BIT(7) > > + > > +#define REG_POWER 0x0b > > +/* REG_POWER values for disabled and normal state from datasheet */ > > +#define POWER_DISABLED BIT(0) > > +#define POWER_NORMAL GENMASK(2, 0) > > + > > +#define REG_RESET 0x0c > > +#define RESET_SW_RESET BIT(0) > > + > > +#define REG_OCP 0x0d > > + > > +#define REG_MASKA 0x0e > > +/* REG_MASKA value for low-power / disabled state from datasheet */ > > +#define MASKA_DISABLED 0xbf > > + > > +#define REG_MASKB 0x0f > > +/* REG_MASKB value for low-power / disabled state from datasheet */ > > +#define MASKB_DISABLED 0x01 > > + > > +#define REG_STATUS1A 0x3d > > +#define STATUS1A_TOGSS_MASK GENMASK(5, 3) > > +#define STATUS1A_TOGSS_SRC_CC1 (1 << 3) > > +#define STATUS1A_TOGSS_SRC_CC2 (2 << 3) > > +#define STATUS1A_TOGSS_SNK_CC1 (5 << 3) > > +#define STATUS1A_TOGSS_SNK_CC2 (6 << 3) > > + > > +#define REG_INTERRUPTA 0x3e > > +#define INTERRUPTA_TOGDONE BIT(6) > > + > > +#define REG_STATUS0 0x40 > > +#define STATUS0_BC_LEVEL_MASK GENMASK(1, 0) > > +#define STATUS0_BC_LEVEL_VRA 0 > > +#define STATUS0_BC_LEVEL_VRDUSB 1 > > +#define STATUS0_BC_LEVEL_VRD15 2 > > +#define STATUS0_BC_LEVEL_VRD30 3 > > +#define STATUS0_COMP BIT(5) > > +#define STATUS0_VBUSOK BIT(7) > > + > > +#define REG_INTERRUPT 0x42 > > +#define INTERRUPT_BC_LVL BIT(0) > > +#define INTERRUPT_COMP_CHNG BIT(5) > > +#define INTERRUPT_VBUSOK BIT(7) > > + > > +/* Timeouts from the FUSB302 datasheet */ > > +#define TTOGCYCLE msecs_to_jiffies(40 + 60 + 40) > > + > > +/* Timeouts from the USB-C specification */ > > +#define TPDDEBOUNCE msecs_to_jiffies(20) > > +#define TDRPTRY_MS 75 > > +#define TDRPTRY msecs_to_jiffies(TDRPTRY_MS) > > +#define TDRPTRYWAIT msecs_to_jiffies(400) > > +#define TVBUSON msecs_to_jiffies(275) > > + > > +enum typec_port_type { > > + TYPEC_PORT_DFP, > > + TYPEC_PORT_UFP, > > + TYPEC_PORT_DRP, > > +}; > > + > > +enum typec_role { > > + TYPEC_SINK, > > + TYPEC_SOURCE, > > +}; > > + > > +enum fusb302_state { > > + DISABLED_SNK, /* UFP */ > > + DISABLED_SRC, /* DFP */ > > + DISABLED_DRP, > > + UNATTACHED_SNK, > > + ATTACHED_SNK, > > + UNATTACHED_SRC, > > + ATTACHED_SRC, > > +}; > > +/* For debugging */ > > +static __maybe_unused const char *fusb302_state_str[] = { > > + "DISABLED_SNK", > > + "DISABLED_SRC", > > + "DISABLED_DRP", > > + "UNATTACHED_SNK", > > + "ATTACHED_SNK", > > + "UNATTACHED_SRC", > > + "ATTACHED_SRC", > > +}; > > + > > +enum fusb302_event { > > + TOGGLE_DONE, > > + BC_LVL_CHANGE, > > + VBUS_VALID, > > + VBUS_INVALID, > > + COMP_LOW, > > + COMP_HIGH, > > + TIMEOUT, > > + FALLBACK_TIMEOUT, > > +}; > > +/* For debugging */ > > +static __maybe_unused const char *fusb302_event_str[] = { > > + "TOGGLE_DONE", > > + "BC_LVL_CHANGE", > > + "VBUS_VALID", > > + "VBUS_INVALID", > > + "COMP_LOW", > > + "COMP_HIGH", > > + "TIMEOUT", > > + "FALLBACK_TIMEOUT", > > +}; > > + > > +static const unsigned int fusb302_extcon_cables[] = { > > + EXTCON_USB, > > + EXTCON_USB_HOST, > > + EXTCON_CHG_USB_SDP, > > + EXTCON_CHG_USB_CDP, > > + EXTCON_CHG_USB_FAST, > > + EXTCON_NONE, > > +}; > > + > > +struct fusb302_data { > > + struct device *dev; > > + struct regmap *regmap; > > + struct extcon_dev *edev; > > + struct mutex lock; > > + struct delayed_work timeout; > > + struct delayed_work fallback_timeout; > > + struct delayed_work bc_work; > > + enum fusb302_state state; > > + enum typec_port_type port_type; > > + enum typec_role preferred_role; > > + int status0; > > + int status1a; > > + unsigned int snk_cable_id; > > +}; > > + > > +static void fusb302_write_reg(struct fusb302_data *data, int reg, int val) > > +{ > > + int ret; > > + > > + ret = regmap_write(data->regmap, reg, val); > > + if (ret) > > + dev_err(data->dev, "Error writing reg %02x: %d\n", reg, ret); > > +} > > + > > +static int fusb302_read_reg(struct fusb302_data *data, int reg) > > +{ > > + int ret, val; > > + > > + ret = regmap_read(data->regmap, reg, &val); > > + if (ret) { > > + dev_err(data->dev, "Error reading reg %02x: %d\n", reg, ret); > > + return 0; > > + } > > + > > + return val; > > +} > > + > > +static void fusb302_update_bits(struct fusb302_data *data, int reg, > > + int mask, int val) > > +{ > > + int ret; > > + > > + ret = regmap_update_bits(data->regmap, reg, mask, val); > > + if (ret) > > + dev_err(data->dev, "Error modifying reg %02x: %d\n", reg, ret); > > +} > > + > > +/* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ > > +static void fusb302_set_extcon_state(struct fusb302_data *data, > > + unsigned int cable, bool state) > > +{ > > + extcon_set_state_sync(data->edev, cable, state); > > + if (cable == EXTCON_CHG_USB_SDP) > > + extcon_set_state_sync(data->edev, EXTCON_USB, state); > > +} > > + > > +/* Helper for the disabled states */ > > +static void fusb302_disable(struct fusb302_data *data) > > +{ > > + fusb302_write_reg(data, REG_POWER, POWER_DISABLED); > > + fusb302_write_reg(data, REG_MASK1, MASK1_DISABLED); > > + fusb302_write_reg(data, REG_MASKA, MASKA_DISABLED); > > + fusb302_write_reg(data, REG_MASKB, MASKB_DISABLED); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > > +} > > + > > +/* > > + * fusb302_set_state() and fusb302_handle_event() implement the 3 state > > + * machines from the datasheet folded into 1 state-machine for code re-use. > > + */ > > +static void fusb302_set_state(struct fusb302_data *data, > > + enum fusb302_state state) > > +{ > > + int status, switches0 = 0; > > + enum fusb302_state old_state = data->state; > > + union extcon_property_value prop; > > + > > + /* Kill pending timeouts from previous state */ > > + cancel_delayed_work(&data->timeout); > > + cancel_delayed_work(&data->fallback_timeout); > > + cancel_delayed_work(&data->bc_work); > > + > > + dev_dbg(data->dev, "New state %s\n", fusb302_state_str[state]); > > + > > + switch (old_state) { > > + case ATTACHED_SNK: > > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > > + break; > > + case ATTACHED_SRC: > > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, false); > > + break; > > + default: > > + break; /* Do nothing */ > > + } > > + > > + switch (state) { > > + case DISABLED_SNK: > > + fusb302_disable(data); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > > + CONTROL2_MODE_SNK); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > > + CONTROL2_TOGGLE); > > + break; > > + > > + case DISABLED_SRC: > > + fusb302_disable(data); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > > + CONTROL2_MODE_SRC); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > > + CONTROL2_TOGGLE); > > + break; > > + > > + case DISABLED_DRP: > > + fusb302_disable(data); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > > + CONTROL2_MODE_DRP); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > > + CONTROL2_TOGGLE); > > + break; > > + > > + case UNATTACHED_SNK: > > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > > + > > + /* Enable pull-down on CC1 / CC2 based on orientation */ > > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > > + case STATUS1A_TOGSS_SNK_CC1: > > + switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; > > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > > + break; > > + case STATUS1A_TOGSS_SNK_CC2: > > + switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; > > + prop.intval = USB_TYPEC_POLARITY_FLIP; > > + break; > > + } > > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > > + extcon_set_property(data->edev, EXTCON_USB, > > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > > + > > + /* Enable VBUSOK and COMP irq at 2.24V for BC detection */ > > + fusb302_update_bits(data, REG_MASK1, > > + MASK1_VBUSOK | MASK1_COMP_CHNG, 0); > > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > > + MEASURE_MDAC_SNK_VRD30); > > + > > + status = fusb302_read_reg(data, REG_STATUS0); > > + if (status & STATUS0_VBUSOK) { > > + /* Go straight to ATTACHED_SNK */ > > + fusb302_set_state(data, ATTACHED_SNK); > > + return; > > + } > > + > > + mod_delayed_work(system_wq, &data->timeout, TVBUSON); > > + break; > > + > > + case ATTACHED_SNK: > > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > > + break; > > + > > + case UNATTACHED_SRC: > > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > > + > > + /* Enable pull-up / Vconn on CC1 / CC2 based on orientation */ > > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > > + case STATUS1A_TOGSS_SRC_CC1: > > + switches0 = SWITCHES0_PU_EN1 | SWITCHES0_VCONN_CC2 | > > + SWITCHES0_MEAS_CC1; > > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > > + break; > > + case STATUS1A_TOGSS_SRC_CC2: > > + switches0 = SWITCHES0_PU_EN2 | SWITCHES0_VCONN_CC1 | > > + SWITCHES0_MEAS_CC2; > > + prop.intval = USB_TYPEC_POLARITY_FLIP; > > + break; > > + } > > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > > + extcon_set_property(data->edev, EXTCON_USB, > > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > > + > > + /* Enable COMP irq at 1.6V for detach detection */ > > + fusb302_update_bits(data, REG_MASK1, MASK1_COMP_CHNG, 0); > > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > > + MEASURE_MDAC_SRC_80UA_HOST_CUR); > > + > > + status = fusb302_read_reg(data, REG_STATUS0); > > + if (!(status & STATUS0_COMP)) { > > + /* Go straight to ATTACHED_SRC */ > > + fusb302_set_state(data, ATTACHED_SRC); > > + return; > > + } > > + > > + mod_delayed_work(system_wq, &data->timeout, TPDDEBOUNCE); > > + break; > > + > > + case ATTACHED_SRC: > > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, true); > > + break; > > + } > > + > > + data->state = state; > > +} > > + > > +static void fusb302_set_state_disabled(struct fusb302_data *data) > > +{ > > + > > + switch (data->port_type) { > > + case TYPEC_PORT_UFP: > > + fusb302_set_state(data, DISABLED_SNK); > > + break; > > + case TYPEC_PORT_DFP: > > + fusb302_set_state(data, DISABLED_SRC); > > + break; > > + case TYPEC_PORT_DRP: > > + fusb302_set_state(data, DISABLED_DRP); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_disabled_snk_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case TOGGLE_DONE: > > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > > + case STATUS1A_TOGSS_SNK_CC1: > > + case STATUS1A_TOGSS_SNK_CC2: > > + fusb302_set_state(data, UNATTACHED_SNK); > > + break; > > + } > > + break; > > + > > + case TIMEOUT: > > + /* Cannot become snk fallback to src */ > > + fusb302_set_state(data, DISABLED_SRC); > > + mod_delayed_work(system_wq, &data->fallback_timeout, > > + TDRPTRYWAIT); > > + break; > > + > > + case FALLBACK_TIMEOUT: > > + /* Both states failed return to disabled drp state */ > > + fusb302_set_state(data, DISABLED_DRP); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_disabled_src_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case TOGGLE_DONE: > > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > > + case STATUS1A_TOGSS_SRC_CC1: > > + case STATUS1A_TOGSS_SRC_CC2: > > + fusb302_set_state(data, UNATTACHED_SRC); > > + break; > > + } > > + break; > > + > > + case TIMEOUT: > > + /* Cannot become src fallback to snk */ > > + fusb302_set_state(data, DISABLED_SNK); > > + mod_delayed_work(system_wq, &data->fallback_timeout, > > + TDRPTRYWAIT); > > + break; > > + > > + case FALLBACK_TIMEOUT: > > + /* Both states failed return to disabled drp state */ > > + fusb302_set_state(data, DISABLED_DRP); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_disabled_drp_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case TOGGLE_DONE: > > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > > + case STATUS1A_TOGSS_SNK_CC1: > > + case STATUS1A_TOGSS_SNK_CC2: > > + if (data->preferred_role == TYPEC_SINK) { > > + /* Jump directly to UNATTACHED_SNK */ > > + fusb302_set_state(data, UNATTACHED_SNK); > > + } else { > > + /* Try to become src */ > > + fusb302_set_state(data, DISABLED_SNK); > > + mod_delayed_work(system_wq, &data->timeout, > > + TDRPTRY); > > + } > > + break; > > + case STATUS1A_TOGSS_SRC_CC1: > > + case STATUS1A_TOGSS_SRC_CC2: > > + if (data->preferred_role == TYPEC_SOURCE) { > > + /* Jump directly to UNATTACHED_SRC */ > > + fusb302_set_state(data, UNATTACHED_SRC); > > + } else { > > + /* > > + * The USB-C spec says we must enable pull-downs > > + * and then wait tDRPTry before checking to > > + * avoid endless role-bouncing if both ends > > + * prefer the snk role. > > + */ > > + fusb302_write_reg(data, REG_SWITCHES0, > > + SWITCHES0_PDWN); > > + fusb302_update_bits(data, REG_CONTROL2, > > + CONTROL2_TOGGLE, 0); > > + msleep(TDRPTRY_MS); > > + /* > > + * Use the toggle engine to do the src > > + * detection to keep things the same as for > > + * directly entering the src role. > > + */ > > + fusb302_set_state(data, DISABLED_SNK); > > + mod_delayed_work(system_wq, &data->timeout, > > + TTOGCYCLE); > > + } > > + break; > > + } > > + break; > > + } > > +} > > + > > +static void fusb302_handle_unattached_snk_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case VBUS_VALID: /* Cable attached */ > > + fusb302_set_state(data, ATTACHED_SNK); > > + break; > > + case TIMEOUT: > > + fusb302_set_state_disabled(data); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_attached_snk_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case BC_LVL_CHANGE: > > + case COMP_LOW: > > + case COMP_HIGH: > > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > > + break; > > + case VBUS_INVALID: /* Cable detached */ > > + fusb302_set_state_disabled(data); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_unattached_src_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case COMP_LOW: /* Cable attached */ > > + fusb302_set_state(data, ATTACHED_SRC); > > + break; > > + case TIMEOUT: > > + fusb302_set_state_disabled(data); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_attached_src_event(struct fusb302_data *data, > > + int event) > > +{ > > + switch (event) { > > + case COMP_HIGH: /* Cable detached */ > > + fusb302_set_state_disabled(data); > > + break; > > + } > > +} > > + > > +static void fusb302_handle_event(struct fusb302_data *data, int event) > > +{ > > + > > + mutex_lock(&data->lock); > > + > > + dev_dbg(data->dev, "Handling state %s event %s status %02x %02x\n", > > + fusb302_state_str[data->state], fusb302_event_str[event], > > + fusb302_read_reg(data, REG_STATUS0), > > + fusb302_read_reg(data, REG_STATUS1A)); > > + > > + switch (data->state) { > > + case DISABLED_SNK: > > + fusb302_handle_disabled_snk_event(data, event); > > + break; > > + case DISABLED_SRC: > > + fusb302_handle_disabled_src_event(data, event); > > + break; > > + case DISABLED_DRP: > > + fusb302_handle_disabled_drp_event(data, event); > > + break; > > + case UNATTACHED_SNK: > > + fusb302_handle_unattached_snk_event(data, event); > > + break; > > + case ATTACHED_SNK: > > + fusb302_handle_attached_snk_event(data, event); > > + break; > > + case UNATTACHED_SRC: > > + fusb302_handle_unattached_src_event(data, event); > > + break; > > + case ATTACHED_SRC: > > + fusb302_handle_attached_src_event(data, event); > > + break; > > + } > > + mutex_unlock(&data->lock); > > +} > > + > > +static void fusb302_timeout(struct work_struct *work) > > +{ > > + struct fusb302_data *data = > > + container_of(work, struct fusb302_data, timeout.work); > > + > > + fusb302_handle_event(data, TIMEOUT); > > +} > > + > > +static void fusb302_fallback_timeout(struct work_struct *work) > > +{ > > + struct fusb302_data *data = > > + container_of(work, struct fusb302_data, fallback_timeout.work); > > + > > + fusb302_handle_event(data, FALLBACK_TIMEOUT); > > +} > > + > > +static void fusb302_report_bc_level(struct work_struct *work) > > +{ > > + struct fusb302_data *data = > > + container_of(work, struct fusb302_data, bc_work.work); > > + > > + /* Clear old charger cable id */ > > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > > + > > + if (data->status0 & STATUS0_COMP) { > > + dev_warn(data->dev, "vRd over maximum, assuming 500mA source\n"); > > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > > + } else { > > + switch (data->status0 & STATUS0_BC_LEVEL_MASK) { > > + case STATUS0_BC_LEVEL_VRA: > > + case STATUS0_BC_LEVEL_VRDUSB: > > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > > + break; > > + case STATUS0_BC_LEVEL_VRD15: > > + data->snk_cable_id = EXTCON_CHG_USB_CDP; > > + break; > > + case STATUS0_BC_LEVEL_VRD30: > > + /* Use CHG_USB_FAST to indicate 3A capability */ > > + data->snk_cable_id = EXTCON_CHG_USB_FAST; > > + break; > > + } > > + } > > + fusb302_set_extcon_state(data, data->snk_cable_id, true); > > +} > > + > > +static irqreturn_t fusb302_irq_handler_thread(int irq, void *handler_data) > > +{ > > + struct fusb302_data *data = handler_data; > > + int interrupt, interrupta; > > + > > + interrupt = fusb302_read_reg(data, REG_INTERRUPT); > > + interrupta = fusb302_read_reg(data, REG_INTERRUPTA); > > + > > + if (interrupt) > > + data->status0 = fusb302_read_reg(data, REG_STATUS0); > > + > > + if (interrupta) > > + data->status1a = fusb302_read_reg(data, REG_STATUS1A); > > + > > + dev_dbg(data->dev, "Handling interrupt %02x %02x status %02x %02x\n", > > + interrupt, interrupta, data->status0, data->status1a); > > + > > + if (interrupt & INTERRUPT_BC_LVL) > > + fusb302_handle_event(data, BC_LVL_CHANGE); > > + > > + if (interrupt & INTERRUPT_COMP_CHNG) { > > + if (data->status0 & STATUS0_COMP) > > + fusb302_handle_event(data, COMP_HIGH); > > + else > > + fusb302_handle_event(data, COMP_LOW); > > + } > > + > > + if (interrupt & INTERRUPT_VBUSOK) { > > + if (data->status0 & STATUS0_VBUSOK) > > + fusb302_handle_event(data, VBUS_VALID); > > + else > > + fusb302_handle_event(data, VBUS_INVALID); > > + } > > + > > + if (interrupta & INTERRUPTA_TOGDONE) > > + fusb302_handle_event(data, TOGGLE_DONE); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static const struct regmap_config fusb302_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .val_format_endian = REGMAP_ENDIAN_NATIVE, > > +}; > > + > > +static int fusb302_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct fusb302_data *data; > > + int ret; > > + > > + if (!client->irq) { > > + dev_err(dev, "Error irq not specified\n"); > > + return -EINVAL; > > + } > > + > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->dev = dev; > > + /* TODO make these 2 configurable using device-properties */ > > + data->port_type = TYPEC_PORT_DRP; > > + data->preferred_role = TYPEC_SINK; > > + > > + data->regmap = devm_regmap_init_i2c(client, &fusb302_regmap_config); > > + if (IS_ERR(data->regmap)) { > > + ret = PTR_ERR(data->regmap); > > + dev_err(dev, "Error to initializing regmap: %d\n", ret); > > + return ret; > > + } > > + > > + mutex_init(&data->lock); > > + INIT_DELAYED_WORK(&data->timeout, fusb302_timeout); > > + INIT_DELAYED_WORK(&data->fallback_timeout, fusb302_fallback_timeout); > > + INIT_DELAYED_WORK(&data->bc_work, fusb302_report_bc_level); > > + > > + data->edev = devm_extcon_dev_allocate(dev, fusb302_extcon_cables); > > + if (IS_ERR(data->edev)) > > + return PTR_ERR(data->edev); > > + > > + data->edev->name = "fusb302"; > > + > > + ret = devm_extcon_dev_register(dev, data->edev); > > + if (ret) > > + return ret; > > + > > + extcon_set_property_capability(data->edev, EXTCON_USB, > > + EXTCON_PROP_USB_TYPEC_POLARITY); > > + > > + fusb302_write_reg(data, REG_RESET, RESET_SW_RESET); > > + usleep_range(10000, 20000); > > + > > + /* Enable power-saving */ > > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_SAVE_PWR_MASK, > > + CONTROL2_SAVE_PWR_40MS); > > + > > + fusb302_set_state_disabled(data); > > + > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > > + fusb302_irq_handler_thread, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, "fusb302", data); > > + if (ret) { > > + dev_err(dev, "Error requesting irq: %d\n", ret); > > + return ret; > > + } > > + > > + fusb302_update_bits(data, REG_CONTROL0, CONTROL0_INT_MASK, 0); > > + > > + i2c_set_clientdata(client, data); > > + return 0; > > +} > > + > > +static int fusb302_remove(struct i2c_client *client) > > +{ > > + struct fusb302_data *data = i2c_get_clientdata(client); > > + > > + devm_free_irq(data->dev, client->irq, data); > > + cancel_delayed_work_sync(&data->timeout); > > + cancel_delayed_work_sync(&data->fallback_timeout); > > + > > + return 0; > > +} > > + > > +static const struct i2c_device_id fusb302_i2c_ids[] = { > > + { "fusb302" }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(i2c, fusb302_i2c_ids); > > + > > +static struct i2c_driver fusb302_driver = { > > + .probe_new = fusb302_probe, > > + .remove = fusb302_remove, > > + .id_table = fusb302_i2c_ids, > > + .driver = { > > + .name = "fusb302", > > + }, > > +}; > > +module_i2c_driver(fusb302_driver); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > > +MODULE_DESCRIPTION("FUSB302 USB TYPE-C controller Driver"); > > -- > > 2.9.3 > > Br, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-24 11:02 ` Heikki Krogerus @ 2017-04-24 13:12 ` Guenter Roeck 2017-05-12 21:22 ` Hans de Goede 0 siblings, 1 reply; 26+ messages in thread From: Guenter Roeck @ 2017-04-24 13:12 UTC (permalink / raw) To: Heikki Krogerus, Hans de Goede Cc: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, "Yueyao (Nathan) Zhu (yueyao" On 04/24/2017 04:02 AM, Heikki Krogerus wrote: > +Guenter > > On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote: >> +Cc: Heikki. >> >> He might comment on this. > > Thanks Andy. > >> On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Add support for USB TYPE-C cable detection on systems using a >>> FUSB302 USB TYPE-C controller. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/extcon/Kconfig | 8 + >>> drivers/extcon/Makefile | 1 + >>> drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 791 insertions(+) >>> create mode 100644 drivers/extcon/extcon-fusb302.c > > There is now the typec class in linux-next that really needs to be > used with all USB Type-C PHYs like fusb302. > > Unless I'm mistaken, Guenter has also written a driver for fusb302. I Not me; it was Nathan. > have not seen it, but if I understood correctly, that driver will > register itself with the upcoming tcpm (USB Type-C Port Manager) [1], > which in practice would mean we can take properly advantage of the USB > PD transceiver on fusb302. > > Guenter! Can you publish the fusb302 driver? > Working on it. Guenter > > [1] https://chromium-review.googlesource.com/c/389916/ > > >>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig >>> index 32f2dc8..562db5b 100644 >>> --- a/drivers/extcon/Kconfig >>> +++ b/drivers/extcon/Kconfig >>> @@ -35,6 +35,14 @@ config EXTCON_AXP288 >>> Say Y here to enable support for USB peripheral detection >>> and USB MUX switching by X-Power AXP288 PMIC. >>> >>> +config EXTCON_FUSB302 >>> + tristate "FUSB302 USB TYPE-C controller support" >>> + depends on I2C >>> + select REGMAP_I2C >>> + help >>> + Say Y here to enable support for USB TYPE-C cable detection using >>> + a FUSB302 USB TYPE-C controller. >>> + >>> config EXTCON_GPIO >>> tristate "GPIO extcon support" >>> depends on GPIOLIB || COMPILE_TEST >>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile >>> index ecfa958..e5eb493 100644 >>> --- a/drivers/extcon/Makefile >>> +++ b/drivers/extcon/Makefile >>> @@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o >>> obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o >>> obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o >>> obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o >>> +obj-$(CONFIG_EXTCON_FUSB302) += extcon-fusb302.o >>> obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o >>> obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o >>> obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o >>> diff --git a/drivers/extcon/extcon-fusb302.c b/drivers/extcon/extcon-fusb302.c >>> new file mode 100644 >>> index 0000000..577adb9 >>> --- /dev/null >>> +++ b/drivers/extcon/extcon-fusb302.c >>> @@ -0,0 +1,782 @@ >>> +/* >>> + * Extcon USB-C cable detection driver for FUSB302 USB TYPE-C controller >>> + * >>> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include <linux/extcon.h> >>> +#include <linux/i2c.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/regmap.h> >>> +#include <linux/slab.h> >>> +#include <linux/workqueue.h> >>> +#include "extcon.h" >>> + >>> +#define REG_DEVICE_ID 0x01 >>> +#define DEVICE_ID_VER_MASK GENMASK(7, 4) >>> +#define DEVICE_ID_FUSB302_VER 0x80 >>> + >>> +#define REG_SWITCHES0 0x02 >>> +#define SWITCHES0_PDWN1 BIT(0) >>> +#define SWITCHES0_PDWN2 BIT(1) >>> +#define SWITCHES0_PDWN GENMASK(1, 0) >>> +#define SWITCHES0_MEAS_CC1 BIT(2) >>> +#define SWITCHES0_MEAS_CC2 BIT(3) >>> +#define SWITCHES0_VCONN_CC1 BIT(4) >>> +#define SWITCHES0_VCONN_CC2 BIT(5) >>> +#define SWITCHES0_PU_EN1 BIT(6) >>> +#define SWITCHES0_PU_EN2 BIT(7) >>> + >>> +#define REG_MEASURE 0x04 >>> +#define MEASURE_MDAC_MASK GENMASK(5, 0) >>> +/* Datasheet says MDAC must be set to 0x34 / 2226mV for vRd-3.0 detection */ >>> +#define MEASURE_MDAC_SNK_VRD30 0x34 >>> +/* MDAC must be set to 0x25 / 1600mV for disconnect det. with 80uA host-cur */ >>> +#define MEASURE_MDAC_SRC_80UA_HOST_CUR 0x25 >>> + >>> +#define REG_CONTROL0 0x06 >>> +#define CONTROL0_HOST_CUR_MASK GENMASK(3, 2) >>> +#define CONTROL0_HOST_CUR_DISABLED (0 << 2) >>> +#define CONTROL0_HOST_CUR_80UA (1 << 2) >>> +#define CONTROL0_HOST_CUR_180UA (2 << 2) >>> +#define CONTROL0_HOST_CUR_330UA (3 << 2) >>> +#define CONTROL0_INT_MASK BIT(5) >>> + >>> +#define REG_CONTROL2 0x08 >>> +#define CONTROL2_TOGGLE BIT(0) >>> +#define CONTROL2_MODE_MASK GENMASK(2, 1) >>> +#define CONTROL2_MODE_DRP (1 << 1) >>> +#define CONTROL2_MODE_SNK (2 << 1) >>> +#define CONTROL2_MODE_SRC (3 << 1) >>> +#define CONTROL2_SAVE_PWR_MASK GENMASK(7, 6) >>> +#define CONTROL2_SAVE_PWR_DISABLED (0 << 6) >>> +#define CONTROL2_SAVE_PWR_40MS (1 << 6) >>> +#define CONTROL2_SAVE_PWR_80MS (2 << 6) >>> +#define CONTROL2_SAVE_PWR_160MS (3 << 6) >>> + >>> +#define REG_MASK1 0x0a >>> +/* REG_MASK1 value for low-power / disabled state from datasheet */ >>> +#define MASK1_DISABLED 0xfe >>> +#define MASK1_COMP_CHNG BIT(5) >>> +#define MASK1_VBUSOK BIT(7) >>> + >>> +#define REG_POWER 0x0b >>> +/* REG_POWER values for disabled and normal state from datasheet */ >>> +#define POWER_DISABLED BIT(0) >>> +#define POWER_NORMAL GENMASK(2, 0) >>> + >>> +#define REG_RESET 0x0c >>> +#define RESET_SW_RESET BIT(0) >>> + >>> +#define REG_OCP 0x0d >>> + >>> +#define REG_MASKA 0x0e >>> +/* REG_MASKA value for low-power / disabled state from datasheet */ >>> +#define MASKA_DISABLED 0xbf >>> + >>> +#define REG_MASKB 0x0f >>> +/* REG_MASKB value for low-power / disabled state from datasheet */ >>> +#define MASKB_DISABLED 0x01 >>> + >>> +#define REG_STATUS1A 0x3d >>> +#define STATUS1A_TOGSS_MASK GENMASK(5, 3) >>> +#define STATUS1A_TOGSS_SRC_CC1 (1 << 3) >>> +#define STATUS1A_TOGSS_SRC_CC2 (2 << 3) >>> +#define STATUS1A_TOGSS_SNK_CC1 (5 << 3) >>> +#define STATUS1A_TOGSS_SNK_CC2 (6 << 3) >>> + >>> +#define REG_INTERRUPTA 0x3e >>> +#define INTERRUPTA_TOGDONE BIT(6) >>> + >>> +#define REG_STATUS0 0x40 >>> +#define STATUS0_BC_LEVEL_MASK GENMASK(1, 0) >>> +#define STATUS0_BC_LEVEL_VRA 0 >>> +#define STATUS0_BC_LEVEL_VRDUSB 1 >>> +#define STATUS0_BC_LEVEL_VRD15 2 >>> +#define STATUS0_BC_LEVEL_VRD30 3 >>> +#define STATUS0_COMP BIT(5) >>> +#define STATUS0_VBUSOK BIT(7) >>> + >>> +#define REG_INTERRUPT 0x42 >>> +#define INTERRUPT_BC_LVL BIT(0) >>> +#define INTERRUPT_COMP_CHNG BIT(5) >>> +#define INTERRUPT_VBUSOK BIT(7) >>> + >>> +/* Timeouts from the FUSB302 datasheet */ >>> +#define TTOGCYCLE msecs_to_jiffies(40 + 60 + 40) >>> + >>> +/* Timeouts from the USB-C specification */ >>> +#define TPDDEBOUNCE msecs_to_jiffies(20) >>> +#define TDRPTRY_MS 75 >>> +#define TDRPTRY msecs_to_jiffies(TDRPTRY_MS) >>> +#define TDRPTRYWAIT msecs_to_jiffies(400) >>> +#define TVBUSON msecs_to_jiffies(275) >>> + >>> +enum typec_port_type { >>> + TYPEC_PORT_DFP, >>> + TYPEC_PORT_UFP, >>> + TYPEC_PORT_DRP, >>> +}; >>> + >>> +enum typec_role { >>> + TYPEC_SINK, >>> + TYPEC_SOURCE, >>> +}; >>> + >>> +enum fusb302_state { >>> + DISABLED_SNK, /* UFP */ >>> + DISABLED_SRC, /* DFP */ >>> + DISABLED_DRP, >>> + UNATTACHED_SNK, >>> + ATTACHED_SNK, >>> + UNATTACHED_SRC, >>> + ATTACHED_SRC, >>> +}; >>> +/* For debugging */ >>> +static __maybe_unused const char *fusb302_state_str[] = { >>> + "DISABLED_SNK", >>> + "DISABLED_SRC", >>> + "DISABLED_DRP", >>> + "UNATTACHED_SNK", >>> + "ATTACHED_SNK", >>> + "UNATTACHED_SRC", >>> + "ATTACHED_SRC", >>> +}; >>> + >>> +enum fusb302_event { >>> + TOGGLE_DONE, >>> + BC_LVL_CHANGE, >>> + VBUS_VALID, >>> + VBUS_INVALID, >>> + COMP_LOW, >>> + COMP_HIGH, >>> + TIMEOUT, >>> + FALLBACK_TIMEOUT, >>> +}; >>> +/* For debugging */ >>> +static __maybe_unused const char *fusb302_event_str[] = { >>> + "TOGGLE_DONE", >>> + "BC_LVL_CHANGE", >>> + "VBUS_VALID", >>> + "VBUS_INVALID", >>> + "COMP_LOW", >>> + "COMP_HIGH", >>> + "TIMEOUT", >>> + "FALLBACK_TIMEOUT", >>> +}; >>> + >>> +static const unsigned int fusb302_extcon_cables[] = { >>> + EXTCON_USB, >>> + EXTCON_USB_HOST, >>> + EXTCON_CHG_USB_SDP, >>> + EXTCON_CHG_USB_CDP, >>> + EXTCON_CHG_USB_FAST, >>> + EXTCON_NONE, >>> +}; >>> + >>> +struct fusb302_data { >>> + struct device *dev; >>> + struct regmap *regmap; >>> + struct extcon_dev *edev; >>> + struct mutex lock; >>> + struct delayed_work timeout; >>> + struct delayed_work fallback_timeout; >>> + struct delayed_work bc_work; >>> + enum fusb302_state state; >>> + enum typec_port_type port_type; >>> + enum typec_role preferred_role; >>> + int status0; >>> + int status1a; >>> + unsigned int snk_cable_id; >>> +}; >>> + >>> +static void fusb302_write_reg(struct fusb302_data *data, int reg, int val) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_write(data->regmap, reg, val); >>> + if (ret) >>> + dev_err(data->dev, "Error writing reg %02x: %d\n", reg, ret); >>> +} >>> + >>> +static int fusb302_read_reg(struct fusb302_data *data, int reg) >>> +{ >>> + int ret, val; >>> + >>> + ret = regmap_read(data->regmap, reg, &val); >>> + if (ret) { >>> + dev_err(data->dev, "Error reading reg %02x: %d\n", reg, ret); >>> + return 0; >>> + } >>> + >>> + return val; >>> +} >>> + >>> +static void fusb302_update_bits(struct fusb302_data *data, int reg, >>> + int mask, int val) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_update_bits(data->regmap, reg, mask, val); >>> + if (ret) >>> + dev_err(data->dev, "Error modifying reg %02x: %d\n", reg, ret); >>> +} >>> + >>> +/* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ >>> +static void fusb302_set_extcon_state(struct fusb302_data *data, >>> + unsigned int cable, bool state) >>> +{ >>> + extcon_set_state_sync(data->edev, cable, state); >>> + if (cable == EXTCON_CHG_USB_SDP) >>> + extcon_set_state_sync(data->edev, EXTCON_USB, state); >>> +} >>> + >>> +/* Helper for the disabled states */ >>> +static void fusb302_disable(struct fusb302_data *data) >>> +{ >>> + fusb302_write_reg(data, REG_POWER, POWER_DISABLED); >>> + fusb302_write_reg(data, REG_MASK1, MASK1_DISABLED); >>> + fusb302_write_reg(data, REG_MASKA, MASKA_DISABLED); >>> + fusb302_write_reg(data, REG_MASKB, MASKB_DISABLED); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); >>> +} >>> + >>> +/* >>> + * fusb302_set_state() and fusb302_handle_event() implement the 3 state >>> + * machines from the datasheet folded into 1 state-machine for code re-use. >>> + */ >>> +static void fusb302_set_state(struct fusb302_data *data, >>> + enum fusb302_state state) >>> +{ >>> + int status, switches0 = 0; >>> + enum fusb302_state old_state = data->state; >>> + union extcon_property_value prop; >>> + >>> + /* Kill pending timeouts from previous state */ >>> + cancel_delayed_work(&data->timeout); >>> + cancel_delayed_work(&data->fallback_timeout); >>> + cancel_delayed_work(&data->bc_work); >>> + >>> + dev_dbg(data->dev, "New state %s\n", fusb302_state_str[state]); >>> + >>> + switch (old_state) { >>> + case ATTACHED_SNK: >>> + fusb302_set_extcon_state(data, data->snk_cable_id, false); >>> + break; >>> + case ATTACHED_SRC: >>> + fusb302_set_extcon_state(data, EXTCON_USB_HOST, false); >>> + break; >>> + default: >>> + break; /* Do nothing */ >>> + } >>> + >>> + switch (state) { >>> + case DISABLED_SNK: >>> + fusb302_disable(data); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, >>> + CONTROL2_MODE_SNK); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, >>> + CONTROL2_TOGGLE); >>> + break; >>> + >>> + case DISABLED_SRC: >>> + fusb302_disable(data); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, >>> + CONTROL2_MODE_SRC); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, >>> + CONTROL2_TOGGLE); >>> + break; >>> + >>> + case DISABLED_DRP: >>> + fusb302_disable(data); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, >>> + CONTROL2_MODE_DRP); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, >>> + CONTROL2_TOGGLE); >>> + break; >>> + >>> + case UNATTACHED_SNK: >>> + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); >>> + >>> + /* Enable pull-down on CC1 / CC2 based on orientation */ >>> + switch (data->status1a & STATUS1A_TOGSS_MASK) { >>> + case STATUS1A_TOGSS_SNK_CC1: >>> + switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; >>> + prop.intval = USB_TYPEC_POLARITY_NORMAL; >>> + break; >>> + case STATUS1A_TOGSS_SNK_CC2: >>> + switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; >>> + prop.intval = USB_TYPEC_POLARITY_FLIP; >>> + break; >>> + } >>> + fusb302_write_reg(data, REG_SWITCHES0, switches0); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); >>> + extcon_set_property(data->edev, EXTCON_USB, >>> + EXTCON_PROP_USB_TYPEC_POLARITY, prop); >>> + >>> + /* Enable VBUSOK and COMP irq at 2.24V for BC detection */ >>> + fusb302_update_bits(data, REG_MASK1, >>> + MASK1_VBUSOK | MASK1_COMP_CHNG, 0); >>> + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, >>> + MEASURE_MDAC_SNK_VRD30); >>> + >>> + status = fusb302_read_reg(data, REG_STATUS0); >>> + if (status & STATUS0_VBUSOK) { >>> + /* Go straight to ATTACHED_SNK */ >>> + fusb302_set_state(data, ATTACHED_SNK); >>> + return; >>> + } >>> + >>> + mod_delayed_work(system_wq, &data->timeout, TVBUSON); >>> + break; >>> + >>> + case ATTACHED_SNK: >>> + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); >>> + break; >>> + >>> + case UNATTACHED_SRC: >>> + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); >>> + >>> + /* Enable pull-up / Vconn on CC1 / CC2 based on orientation */ >>> + switch (data->status1a & STATUS1A_TOGSS_MASK) { >>> + case STATUS1A_TOGSS_SRC_CC1: >>> + switches0 = SWITCHES0_PU_EN1 | SWITCHES0_VCONN_CC2 | >>> + SWITCHES0_MEAS_CC1; >>> + prop.intval = USB_TYPEC_POLARITY_NORMAL; >>> + break; >>> + case STATUS1A_TOGSS_SRC_CC2: >>> + switches0 = SWITCHES0_PU_EN2 | SWITCHES0_VCONN_CC1 | >>> + SWITCHES0_MEAS_CC2; >>> + prop.intval = USB_TYPEC_POLARITY_FLIP; >>> + break; >>> + } >>> + fusb302_write_reg(data, REG_SWITCHES0, switches0); >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); >>> + extcon_set_property(data->edev, EXTCON_USB, >>> + EXTCON_PROP_USB_TYPEC_POLARITY, prop); >>> + >>> + /* Enable COMP irq at 1.6V for detach detection */ >>> + fusb302_update_bits(data, REG_MASK1, MASK1_COMP_CHNG, 0); >>> + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, >>> + MEASURE_MDAC_SRC_80UA_HOST_CUR); >>> + >>> + status = fusb302_read_reg(data, REG_STATUS0); >>> + if (!(status & STATUS0_COMP)) { >>> + /* Go straight to ATTACHED_SRC */ >>> + fusb302_set_state(data, ATTACHED_SRC); >>> + return; >>> + } >>> + >>> + mod_delayed_work(system_wq, &data->timeout, TPDDEBOUNCE); >>> + break; >>> + >>> + case ATTACHED_SRC: >>> + fusb302_set_extcon_state(data, EXTCON_USB_HOST, true); >>> + break; >>> + } >>> + >>> + data->state = state; >>> +} >>> + >>> +static void fusb302_set_state_disabled(struct fusb302_data *data) >>> +{ >>> + >>> + switch (data->port_type) { >>> + case TYPEC_PORT_UFP: >>> + fusb302_set_state(data, DISABLED_SNK); >>> + break; >>> + case TYPEC_PORT_DFP: >>> + fusb302_set_state(data, DISABLED_SRC); >>> + break; >>> + case TYPEC_PORT_DRP: >>> + fusb302_set_state(data, DISABLED_DRP); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_disabled_snk_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case TOGGLE_DONE: >>> + switch (data->status1a & STATUS1A_TOGSS_MASK) { >>> + case STATUS1A_TOGSS_SNK_CC1: >>> + case STATUS1A_TOGSS_SNK_CC2: >>> + fusb302_set_state(data, UNATTACHED_SNK); >>> + break; >>> + } >>> + break; >>> + >>> + case TIMEOUT: >>> + /* Cannot become snk fallback to src */ >>> + fusb302_set_state(data, DISABLED_SRC); >>> + mod_delayed_work(system_wq, &data->fallback_timeout, >>> + TDRPTRYWAIT); >>> + break; >>> + >>> + case FALLBACK_TIMEOUT: >>> + /* Both states failed return to disabled drp state */ >>> + fusb302_set_state(data, DISABLED_DRP); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_disabled_src_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case TOGGLE_DONE: >>> + switch (data->status1a & STATUS1A_TOGSS_MASK) { >>> + case STATUS1A_TOGSS_SRC_CC1: >>> + case STATUS1A_TOGSS_SRC_CC2: >>> + fusb302_set_state(data, UNATTACHED_SRC); >>> + break; >>> + } >>> + break; >>> + >>> + case TIMEOUT: >>> + /* Cannot become src fallback to snk */ >>> + fusb302_set_state(data, DISABLED_SNK); >>> + mod_delayed_work(system_wq, &data->fallback_timeout, >>> + TDRPTRYWAIT); >>> + break; >>> + >>> + case FALLBACK_TIMEOUT: >>> + /* Both states failed return to disabled drp state */ >>> + fusb302_set_state(data, DISABLED_DRP); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_disabled_drp_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case TOGGLE_DONE: >>> + switch (data->status1a & STATUS1A_TOGSS_MASK) { >>> + case STATUS1A_TOGSS_SNK_CC1: >>> + case STATUS1A_TOGSS_SNK_CC2: >>> + if (data->preferred_role == TYPEC_SINK) { >>> + /* Jump directly to UNATTACHED_SNK */ >>> + fusb302_set_state(data, UNATTACHED_SNK); >>> + } else { >>> + /* Try to become src */ >>> + fusb302_set_state(data, DISABLED_SNK); >>> + mod_delayed_work(system_wq, &data->timeout, >>> + TDRPTRY); >>> + } >>> + break; >>> + case STATUS1A_TOGSS_SRC_CC1: >>> + case STATUS1A_TOGSS_SRC_CC2: >>> + if (data->preferred_role == TYPEC_SOURCE) { >>> + /* Jump directly to UNATTACHED_SRC */ >>> + fusb302_set_state(data, UNATTACHED_SRC); >>> + } else { >>> + /* >>> + * The USB-C spec says we must enable pull-downs >>> + * and then wait tDRPTry before checking to >>> + * avoid endless role-bouncing if both ends >>> + * prefer the snk role. >>> + */ >>> + fusb302_write_reg(data, REG_SWITCHES0, >>> + SWITCHES0_PDWN); >>> + fusb302_update_bits(data, REG_CONTROL2, >>> + CONTROL2_TOGGLE, 0); >>> + msleep(TDRPTRY_MS); >>> + /* >>> + * Use the toggle engine to do the src >>> + * detection to keep things the same as for >>> + * directly entering the src role. >>> + */ >>> + fusb302_set_state(data, DISABLED_SNK); >>> + mod_delayed_work(system_wq, &data->timeout, >>> + TTOGCYCLE); >>> + } >>> + break; >>> + } >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_unattached_snk_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case VBUS_VALID: /* Cable attached */ >>> + fusb302_set_state(data, ATTACHED_SNK); >>> + break; >>> + case TIMEOUT: >>> + fusb302_set_state_disabled(data); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_attached_snk_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case BC_LVL_CHANGE: >>> + case COMP_LOW: >>> + case COMP_HIGH: >>> + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); >>> + break; >>> + case VBUS_INVALID: /* Cable detached */ >>> + fusb302_set_state_disabled(data); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_unattached_src_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case COMP_LOW: /* Cable attached */ >>> + fusb302_set_state(data, ATTACHED_SRC); >>> + break; >>> + case TIMEOUT: >>> + fusb302_set_state_disabled(data); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_attached_src_event(struct fusb302_data *data, >>> + int event) >>> +{ >>> + switch (event) { >>> + case COMP_HIGH: /* Cable detached */ >>> + fusb302_set_state_disabled(data); >>> + break; >>> + } >>> +} >>> + >>> +static void fusb302_handle_event(struct fusb302_data *data, int event) >>> +{ >>> + >>> + mutex_lock(&data->lock); >>> + >>> + dev_dbg(data->dev, "Handling state %s event %s status %02x %02x\n", >>> + fusb302_state_str[data->state], fusb302_event_str[event], >>> + fusb302_read_reg(data, REG_STATUS0), >>> + fusb302_read_reg(data, REG_STATUS1A)); >>> + >>> + switch (data->state) { >>> + case DISABLED_SNK: >>> + fusb302_handle_disabled_snk_event(data, event); >>> + break; >>> + case DISABLED_SRC: >>> + fusb302_handle_disabled_src_event(data, event); >>> + break; >>> + case DISABLED_DRP: >>> + fusb302_handle_disabled_drp_event(data, event); >>> + break; >>> + case UNATTACHED_SNK: >>> + fusb302_handle_unattached_snk_event(data, event); >>> + break; >>> + case ATTACHED_SNK: >>> + fusb302_handle_attached_snk_event(data, event); >>> + break; >>> + case UNATTACHED_SRC: >>> + fusb302_handle_unattached_src_event(data, event); >>> + break; >>> + case ATTACHED_SRC: >>> + fusb302_handle_attached_src_event(data, event); >>> + break; >>> + } >>> + mutex_unlock(&data->lock); >>> +} >>> + >>> +static void fusb302_timeout(struct work_struct *work) >>> +{ >>> + struct fusb302_data *data = >>> + container_of(work, struct fusb302_data, timeout.work); >>> + >>> + fusb302_handle_event(data, TIMEOUT); >>> +} >>> + >>> +static void fusb302_fallback_timeout(struct work_struct *work) >>> +{ >>> + struct fusb302_data *data = >>> + container_of(work, struct fusb302_data, fallback_timeout.work); >>> + >>> + fusb302_handle_event(data, FALLBACK_TIMEOUT); >>> +} >>> + >>> +static void fusb302_report_bc_level(struct work_struct *work) >>> +{ >>> + struct fusb302_data *data = >>> + container_of(work, struct fusb302_data, bc_work.work); >>> + >>> + /* Clear old charger cable id */ >>> + fusb302_set_extcon_state(data, data->snk_cable_id, false); >>> + >>> + if (data->status0 & STATUS0_COMP) { >>> + dev_warn(data->dev, "vRd over maximum, assuming 500mA source\n"); >>> + data->snk_cable_id = EXTCON_CHG_USB_SDP; >>> + } else { >>> + switch (data->status0 & STATUS0_BC_LEVEL_MASK) { >>> + case STATUS0_BC_LEVEL_VRA: >>> + case STATUS0_BC_LEVEL_VRDUSB: >>> + data->snk_cable_id = EXTCON_CHG_USB_SDP; >>> + break; >>> + case STATUS0_BC_LEVEL_VRD15: >>> + data->snk_cable_id = EXTCON_CHG_USB_CDP; >>> + break; >>> + case STATUS0_BC_LEVEL_VRD30: >>> + /* Use CHG_USB_FAST to indicate 3A capability */ >>> + data->snk_cable_id = EXTCON_CHG_USB_FAST; >>> + break; >>> + } >>> + } >>> + fusb302_set_extcon_state(data, data->snk_cable_id, true); >>> +} >>> + >>> +static irqreturn_t fusb302_irq_handler_thread(int irq, void *handler_data) >>> +{ >>> + struct fusb302_data *data = handler_data; >>> + int interrupt, interrupta; >>> + >>> + interrupt = fusb302_read_reg(data, REG_INTERRUPT); >>> + interrupta = fusb302_read_reg(data, REG_INTERRUPTA); >>> + >>> + if (interrupt) >>> + data->status0 = fusb302_read_reg(data, REG_STATUS0); >>> + >>> + if (interrupta) >>> + data->status1a = fusb302_read_reg(data, REG_STATUS1A); >>> + >>> + dev_dbg(data->dev, "Handling interrupt %02x %02x status %02x %02x\n", >>> + interrupt, interrupta, data->status0, data->status1a); >>> + >>> + if (interrupt & INTERRUPT_BC_LVL) >>> + fusb302_handle_event(data, BC_LVL_CHANGE); >>> + >>> + if (interrupt & INTERRUPT_COMP_CHNG) { >>> + if (data->status0 & STATUS0_COMP) >>> + fusb302_handle_event(data, COMP_HIGH); >>> + else >>> + fusb302_handle_event(data, COMP_LOW); >>> + } >>> + >>> + if (interrupt & INTERRUPT_VBUSOK) { >>> + if (data->status0 & STATUS0_VBUSOK) >>> + fusb302_handle_event(data, VBUS_VALID); >>> + else >>> + fusb302_handle_event(data, VBUS_INVALID); >>> + } >>> + >>> + if (interrupta & INTERRUPTA_TOGDONE) >>> + fusb302_handle_event(data, TOGGLE_DONE); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static const struct regmap_config fusb302_regmap_config = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + .val_format_endian = REGMAP_ENDIAN_NATIVE, >>> +}; >>> + >>> +static int fusb302_probe(struct i2c_client *client) >>> +{ >>> + struct device *dev = &client->dev; >>> + struct fusb302_data *data; >>> + int ret; >>> + >>> + if (!client->irq) { >>> + dev_err(dev, "Error irq not specified\n"); >>> + return -EINVAL; >>> + } >>> + >>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >>> + if (!data) >>> + return -ENOMEM; >>> + >>> + data->dev = dev; >>> + /* TODO make these 2 configurable using device-properties */ >>> + data->port_type = TYPEC_PORT_DRP; >>> + data->preferred_role = TYPEC_SINK; >>> + >>> + data->regmap = devm_regmap_init_i2c(client, &fusb302_regmap_config); >>> + if (IS_ERR(data->regmap)) { >>> + ret = PTR_ERR(data->regmap); >>> + dev_err(dev, "Error to initializing regmap: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + mutex_init(&data->lock); >>> + INIT_DELAYED_WORK(&data->timeout, fusb302_timeout); >>> + INIT_DELAYED_WORK(&data->fallback_timeout, fusb302_fallback_timeout); >>> + INIT_DELAYED_WORK(&data->bc_work, fusb302_report_bc_level); >>> + >>> + data->edev = devm_extcon_dev_allocate(dev, fusb302_extcon_cables); >>> + if (IS_ERR(data->edev)) >>> + return PTR_ERR(data->edev); >>> + >>> + data->edev->name = "fusb302"; >>> + >>> + ret = devm_extcon_dev_register(dev, data->edev); >>> + if (ret) >>> + return ret; >>> + >>> + extcon_set_property_capability(data->edev, EXTCON_USB, >>> + EXTCON_PROP_USB_TYPEC_POLARITY); >>> + >>> + fusb302_write_reg(data, REG_RESET, RESET_SW_RESET); >>> + usleep_range(10000, 20000); >>> + >>> + /* Enable power-saving */ >>> + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_SAVE_PWR_MASK, >>> + CONTROL2_SAVE_PWR_40MS); >>> + >>> + fusb302_set_state_disabled(data); >>> + >>> + ret = devm_request_threaded_irq(dev, client->irq, NULL, >>> + fusb302_irq_handler_thread, >>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT, "fusb302", data); >>> + if (ret) { >>> + dev_err(dev, "Error requesting irq: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + fusb302_update_bits(data, REG_CONTROL0, CONTROL0_INT_MASK, 0); >>> + >>> + i2c_set_clientdata(client, data); >>> + return 0; >>> +} >>> + >>> +static int fusb302_remove(struct i2c_client *client) >>> +{ >>> + struct fusb302_data *data = i2c_get_clientdata(client); >>> + >>> + devm_free_irq(data->dev, client->irq, data); >>> + cancel_delayed_work_sync(&data->timeout); >>> + cancel_delayed_work_sync(&data->fallback_timeout); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct i2c_device_id fusb302_i2c_ids[] = { >>> + { "fusb302" }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(i2c, fusb302_i2c_ids); >>> + >>> +static struct i2c_driver fusb302_driver = { >>> + .probe_new = fusb302_probe, >>> + .remove = fusb302_remove, >>> + .id_table = fusb302_i2c_ids, >>> + .driver = { >>> + .name = "fusb302", >>> + }, >>> +}; >>> +module_i2c_driver(fusb302_driver); >>> + >>> +MODULE_LICENSE("GPL"); >>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); >>> +MODULE_DESCRIPTION("FUSB302 USB TYPE-C controller Driver"); >>> -- >>> 2.9.3 >>> > > Br, > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-24 13:12 ` Guenter Roeck @ 2017-05-12 21:22 ` Hans de Goede 2017-05-16 12:07 ` Heikki Krogerus 2017-05-16 22:52 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Guenter Roeck 0 siblings, 2 replies; 26+ messages in thread From: Hans de Goede @ 2017-05-12 21:22 UTC (permalink / raw) To: Guenter Roeck, Heikki Krogerus Cc: Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, "Yueyao (Nathan) Zhu (yueyao" Hi, On 24-04-17 15:12, Guenter Roeck wrote: > On 04/24/2017 04:02 AM, Heikki Krogerus wrote: >> +Guenter >> >> On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote: >>> +Cc: Heikki. >>> >>> He might comment on this. >> >> Thanks Andy. >> >>> On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Add support for USB TYPE-C cable detection on systems using a >>>> FUSB302 USB TYPE-C controller. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/extcon/Kconfig | 8 + >>>> drivers/extcon/Makefile | 1 + >>>> drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 791 insertions(+) >>>> create mode 100644 drivers/extcon/extcon-fusb302.c >> >> There is now the typec class in linux-next that really needs to be >> used with all USB Type-C PHYs like fusb302. >> >> Unless I'm mistaken, Guenter has also written a driver for fusb302. I > > Not me; it was Nathan. > >> have not seen it, but if I understood correctly, that driver will >> register itself with the upcoming tcpm (USB Type-C Port Manager) [1], >> which in practice would mean we can take properly advantage of the USB >> PD transceiver on fusb302. >> >> Guenter! Can you publish the fusb302 driver? >> > > Working on it. Ok, I see this has landed in staging. So lets go with this driver then and not mine (I did not get around to the PD stuff yet, so that is fine). Question, how is this supposed to interface with the rest of the kernel ? Specifically the commit message for the tcpm frameworks says: "This driver only implements the state machine. Lower level drivers are responsible for - Reporting VBUS status and activating VBUS - Setting CC lines and providing CC line status - Setting line polarity - Activating and deactivating VCONN - Setting the current limit - Activating and deactivating PD message transfers - Sending and receiving PD messages" But the FUSB302 cannot set the current limit... The device I'm working on: http://www.gpd.hk/gpdwin.asp Has the following more or less relevant components: -Intel Cherry Trail Z8700 SoC -Intel Cherry Trail Whiskey Cove PMIC (which is different from the Broxton Whiskey Cove PMIC), this PMIC uses an external charger and fuel-gauge: -TI bq24292i charger -Maxim MAX17047 fuel-gauge -PI3USB30532 USB TYPE-C mux Currently I only have the USB-C connector on the device working in USB-2 mode (I did not realize the USB-3 bits were wired up at first). The device ships with a charger with an USB-A receptacle and an USB-2 only USB-A to USB-C cable. The way I've hooked up USB-2 charging is like this: The Whiskey Cove PMIC has built in USB-2 BC detection, and I've written an extcon driver for this which reports one of the following cables depended on the BC detection: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP, And I've modified the bq24290_charger driver to (optionally) receive an extcon name through device-properties and if it does, then set the input current based on the detected charger type (0.5 A for SDP, 1.5A for CDP, 2A for DCP). Most of the patches for this are upstream. But once we hookup the FUSB302 driver we need to propagate the current limit it has negotiated to the bq24290_charger driver. Which is part of the reason why I wrote the 5th patch of my original series, let me reply to Heikki's question about that one here as it is related: On 24-04-17 16:25, Heikki Krogerus wrote: > Doesn't this mean you have several extcon_dev registered for a > single port? Yes this is unfortunate, but the primary consumer of extcon info is the kernel itself and we can teach it to look at the right one. > I'm not completely sure how extcon framework is meant to > be used, but shouldn't a single extcon_dev represent all the types of > connectors a port is meant to support? Ideally, yes. But here we've 2 chips / drivers connected to the same connector (more even, but 2 which provide extcon related info). As explained above the device ships with an USB-2 charger + USB-A to USB-C cable, that cable correctly gets seen by the FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input current to 0.5A, but as the FUSB302 datasheet says: "The host software is expected to follow the USB Type-C specification for charging current priority based on feedback from the FUSB302 detection, external BC1.2 detection and any USB Power Delivery communication. The FUSB302 does not integrate BC1.2 charger detection which is assumed available in the USB transceiver or USB charger in the system." So when we see TYPEC_CC_RP_DEF we need to ask the Cherry Trail PMIC (in my case) to do BC detection and use the result of that, otherwise we end up setting input current limit way too low. >> Since the Type-C controller info is incomplete and needs to be >> supplemented with the BC1.2 detection result in some cases, this >> commit makes the intel-cht-wc extcon driver monitor the extcon device >> registered by the Type-C controller and report that as our extcon state >> except when BC-1.2 detection should be used. This allows extcon >> consumers on these boards to keep monitoring only the intel-cht-wc extcon >> and then get complete extcon info from that, which combines the Type-C >> and BC-1.2 info. > > I don't really understand why does this need to be done in such a > complex manner? Both components should just report the result and be > done with it, and there is no need for any coupling between the two. > If there is a consumer for the power, the driver for it can decide on > its own the limits and maximum power from the two sources. To me making the combination of the 2 sources the problem of the consumer seems just wrong, as you mentioned above there should really be only one extcon for the connector, likewise their should be only one definitive source on what the input current limit is. TL;DR: We need some way to combine the current limit info from TYPE-C and USB-2 BC detection into a single source and to propagate that current to drivers which can actually set the current-limit. Note I'm happy to use something else then extcon for this, but we do need some way to combine + propagate the current-limit. Likewise we need a way to tell another driver to drive VBus on the USB-C connector. In my case this is again done by the bq24290_charger driver, which currently does this based on the combination of EXTCON_CHG_USB_* not being set on the extcon + EXTCON_USB_HOST being set. One possible solution would be for the drivers/extcon/extcon-intel-cht-wc.c to somehow hook into the tpmc device and to get notifications of state changes, then it could reflect the info from the FUSB302 in its extcon info, not sure if that is the best design though. What also seems to missing is for a way to hookup a mux driver to deal with upside-down plug-ins and alt-mode switching. Regards, Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-05-12 21:22 ` Hans de Goede @ 2017-05-16 12:07 ` Heikki Krogerus 2017-05-16 22:24 ` Hans de Goede 2017-05-16 22:52 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Guenter Roeck 1 sibling, 1 reply; 26+ messages in thread From: Heikki Krogerus @ 2017-05-16 12:07 UTC (permalink / raw) To: Hans de Goede Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org Hi Hans, On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote: > On 24-04-17 16:25, Heikki Krogerus wrote: > > > Doesn't this mean you have several extcon_dev registered for a > > single port? > > Yes this is unfortunate, but the primary consumer of extcon > info is the kernel itself and we can teach it to look at the > right one. > > > I'm not completely sure how extcon framework is meant to > > be used, but shouldn't a single extcon_dev represent all the types of > > connectors a port is meant to support? > > Ideally, yes. But here we've 2 chips / drivers connected > to the same connector (more even, but 2 which provide extcon > related info). > > As explained above the device ships with an USB-2 charger + > USB-A to USB-C cable, that cable correctly gets seen by the > FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input > current to 0.5A, but as the FUSB302 datasheet says: > > "The host software is expected to follow the USB Type-C > specification for charging current priority based on > feedback from the FUSB302 detection, external BC1.2 detection > and any USB Power Delivery communication. > > The FUSB302 does not integrate BC1.2 charger > detection which is assumed available in the USB > transceiver or USB charger in the system." > > So when we see TYPEC_CC_RP_DEF we need to ask the > Cherry Trail PMIC (in my case) to do BC detection > and use the result of that, otherwise we end up > setting input current limit way too low. I understand that. > >> Since the Type-C controller info is incomplete and needs to be > >> supplemented with the BC1.2 detection result in some cases, this > >> commit makes the intel-cht-wc extcon driver monitor the extcon device > >> registered by the Type-C controller and report that as our extcon state > >> except when BC-1.2 detection should be used. This allows extcon > >> consumers on these boards to keep monitoring only the intel-cht-wc extcon > >> and then get complete extcon info from that, which combines the Type-C > >> and BC-1.2 info. > > > > I don't really understand why does this need to be done in such a > > complex manner? Both components should just report the result and be > > done with it, and there is no need for any coupling between the two. > > If there is a consumer for the power, the driver for it can decide on > > its own the limits and maximum power from the two sources. > > To me making the combination of the 2 sources the problem > of the consumer seems just wrong, as you mentioned > above there should really be only one extcon for the > connector, likewise their should be only one definitive > source on what the input current limit is. Why? The consumer driver, bq24290 in this case, does not need to understand it has multiple sources. It will just react to events from *a* source, and the psy framework takes care of everything else. The psy framework will need some tuning, but that should not be a problem. I'm preparing support for _PCL (power consumer list) ACPI method handling for the psy framework, so I have some patches already. The idea is that in the future USB ports could have virtual power source object with _PCL. This is in any case separate issue compared to the problem of telling the BC1.2 detection to start in case of TYPEC_CC_RP_DEF. > TL;DR: We need some way to combine the current limit info > from TYPE-C and USB-2 BC detection into a single source and > to propagate that current to drivers which can actually set > the current-limit. > > Note I'm happy to use something else then extcon for this, > but we do need some way to combine + propagate the > current-limit. That must be handled using psy framework. Otherwise we'll just be trying to re-invent the wheel. There is no problem for a consumer to have multiple sources. There are several issues here.. > Likewise we need a way to tell another driver to drive VBus > on the USB-C connector. In my case this is again done by the > bq24290_charger driver, which currently does this based on the > combination of EXTCON_CHG_USB_* not being set on the extcon + > EXTCON_USB_HOST being set. This is an other.. > One possible solution would be for the > drivers/extcon/extcon-intel-cht-wc.c to somehow hook into > the tpmc device and to get notifications of state changes, > then it could reflect the info from the FUSB302 in its > extcon info, not sure if that is the best design though. > > What also seems to missing is for a way to hookup a > mux driver to deal with upside-down plug-ins and > alt-mode switching. This is fourth issue, but let's not go into that now. Just in case you guys are interested, right now I'm looking if we can use device graph to link all the components related to an alt-mode: Documentation/devicetree/bindings/graph.txt The reason for that is because we should be able to use it also with ACPI: Documentation/acpi/dsd/graph.txt Back to this topic. I can see at least the following problems: 1. Tell the BC1.2 detection to start from fusb302 driver 2. Deliver the power limits to the discrete charger ic or battery driver 3. Tell what ever regulates VBUS to start driving VBUS. You are trying to solve everything using extcon, and that is wrong. The second problem definitely needs to be handled using psy framework. The psy framework provides already everything needed for that. I don't have suggestions for the other issues at this point. If extcon can be used with those without any problems, OK. But let's try to solve one problem at a time. Thanks, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-05-16 12:07 ` Heikki Krogerus @ 2017-05-16 22:24 ` Hans de Goede 2017-05-17 11:45 ` Heikki Krogerus 0 siblings, 1 reply; 26+ messages in thread From: Hans de Goede @ 2017-05-16 22:24 UTC (permalink / raw) To: Heikki Krogerus Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org Hi, On 05/16/2017 02:07 PM, Heikki Krogerus wrote: > Hi Hans, > > On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote: <snip> >> To me making the combination of the 2 sources the problem >> of the consumer seems just wrong, as you mentioned >> above there should really be only one extcon for the >> connector, likewise their should be only one definitive >> source on what the input current limit is. > > Why? The consumer driver, bq24290 in this case, does not need to > understand it has multiple sources. It will just react to events from > *a* source, and the psy framework takes care of everything else. The > psy framework will need some tuning, but that should not be a problem. > I'm preparing support for _PCL (power consumer list) ACPI method > handling for the psy framework, so I have some patches already. The > idea is that in the future USB ports could have virtual power source > object with _PCL. > > This is in any case separate issue compared to the problem of telling > the BC1.2 detection to start in case of TYPEC_CC_RP_DEF. > >> TL;DR: We need some way to combine the current limit info >> from TYPE-C and USB-2 BC detection into a single source and >> to propagate that current to drivers which can actually set >> the current-limit. >> >> Note I'm happy to use something else then extcon for this, >> but we do need some way to combine + propagate the >> current-limit. > > That must be handled using psy framework. Otherwise we'll just be > trying to re-invent the wheel. There is no problem for a consumer to > have multiple sources. But we don't really have multiple sources here, we have only one source, the USB-C cable hooked to an external power-supply. Just because 2 different pieces of hardware may be involved in determining the current limit does not mean that we should model this as 2 different sources. Just as you rightfully pointed out that me using 2 extcon devices for the single Type-C connector is wrong, modelling it as 2 sources is wrong too. <snip> > Back to this topic. I can see at least the following problems: > > 1. Tell the BC1.2 detection to start from fusb302 driver > 2. Deliver the power limits to the discrete charger ic or battery driver > 3. Tell what ever regulates VBUS to start driving VBUS. > > You are trying to solve everything using extcon, and that is wrong. As indicated I'm not stuck on using extcon, I started using it in my paches because it is used to set the current limit in some other places already, but I'm fine with using something else. > The second problem definitely needs to be handled using psy framework. > The psy framework provides already everything needed for that. So above you're talking about sources to the bq24190, which if I understand you correctly suggest that you want the tcpm code to register a power-supply device per Type-C port ? I'm not sure that is a good idea, any registered power-supply devices will show up under /sys/class/power_supply, currently on a typical system there will be 2 entries under /sys/class/power_supply one for the "Mains" or USB power input and one for the battery, adding more entries there is likely to confuse userspace. More in general having 2 power-supply devices for what is in essence one power-input feels wrong. We can still use the power-supply framework, but I would envision this working like this: The platform code which enumerates the type-c controller sets a "input-current-power-supply-name" string device-property on the tcpm's (parent)device. When this is set the tpcm code will do a power_supply_get_by_name and set the input current on the returned power_supply by setting the POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. For 3. (vbus) we could do something similar using a "vbus-regulator-id" device-property and the regulator framework, making the driver which controls Vbus register itself as a regulator. I can take a shot at implementing both suggestions, but first lets try to get some general idea of how we want to solve this. Regards, Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-05-16 22:24 ` Hans de Goede @ 2017-05-17 11:45 ` Heikki Krogerus 2017-05-17 14:47 ` USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede 0 siblings, 1 reply; 26+ messages in thread From: Heikki Krogerus @ 2017-05-17 11:45 UTC (permalink / raw) To: Hans de Goede Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org Hi, On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote: > Hi, > > On 05/16/2017 02:07 PM, Heikki Krogerus wrote: > > Hi Hans, > > > > On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote: > > <snip> > > > > To me making the combination of the 2 sources the problem > > > of the consumer seems just wrong, as you mentioned > > > above there should really be only one extcon for the > > > connector, likewise their should be only one definitive > > > source on what the input current limit is. > > > > Why? The consumer driver, bq24290 in this case, does not need to > > understand it has multiple sources. It will just react to events from > > *a* source, and the psy framework takes care of everything else. The > > psy framework will need some tuning, but that should not be a problem. > > I'm preparing support for _PCL (power consumer list) ACPI method > > handling for the psy framework, so I have some patches already. The > > idea is that in the future USB ports could have virtual power source > > object with _PCL. > > > > This is in any case separate issue compared to the problem of telling > > the BC1.2 detection to start in case of TYPEC_CC_RP_DEF. > > > > > TL;DR: We need some way to combine the current limit info > > > from TYPE-C and USB-2 BC detection into a single source and > > > to propagate that current to drivers which can actually set > > > the current-limit. > > > > > > Note I'm happy to use something else then extcon for this, > > > but we do need some way to combine + propagate the > > > current-limit. > > > > That must be handled using psy framework. Otherwise we'll just be > > trying to re-invent the wheel. There is no problem for a consumer to > > have multiple sources. > > But we don't really have multiple sources here, we have only > one source, the USB-C cable hooked to an external power-supply. > Just because 2 different pieces of hardware may be involved in > determining the current limit does not mean that we should > model this as 2 different sources. Just as you rightfully > pointed out that me using 2 extcon devices for the single > Type-C connector is wrong, modelling it as 2 sources is > wrong too. The power supply devices don't represent the port like the extcon device would. The power supply devices represent the two types of external chargers we support. So BC1.2 and Type-C/PD source. > > Back to this topic. I can see at least the following problems: > > > > 1. Tell the BC1.2 detection to start from fusb302 driver > > 2. Deliver the power limits to the discrete charger ic or battery driver > > 3. Tell what ever regulates VBUS to start driving VBUS. > > > > You are trying to solve everything using extcon, and that is wrong. > > As indicated I'm not stuck on using extcon, I started using it > in my paches because it is used to set the current limit in some > other places already, but I'm fine with using something else. > > > The second problem definitely needs to be handled using psy framework. > > The psy framework provides already everything needed for that. > > So above you're talking about sources to the bq24190, which if > I understand you correctly suggest that you want the tcpm code to > register a power-supply device per Type-C port ? No, the underlying device driver (so fusb302) needs to register the power supply at this point. We just notify the psy framework if the limits we get from tcpm to the set_current_limit hook change. > I'm not sure that > is a good idea, any registered power-supply devices will show up > under /sys/class/power_supply, currently on a typical system > there will be 2 entries under /sys/class/power_supply one for > the "Mains" or USB power input and one for the battery, adding > more entries there is likely to confuse userspace. The userspace uses the type attribute to differentiate the supplies. Otherwise it would not be able to tell even which is the Battery and which is Mains. Having more power supplies is not a problem. > More in general having 2 power-supply devices for what is > in essence one power-input feels wrong. > > We can still use the power-supply framework, but I would > envision this working like this: > > The platform code which enumerates the type-c controller > sets a "input-current-power-supply-name" string device-property > on the tcpm's (parent)device. When this is set the tpcm code > will do a power_supply_get_by_name and set the input > current on the returned power_supply by setting the > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. The psy framework is probable a bit messy at them moment. It definitely does not do much protecting and in many cases one driver registers a power supply and an other just takes it over, but that should be avoided as it makes things difficult (painful) to maintain. It also poses risks IMO. There certainly almost never seems to be a real need for that. In this case the driver for fusb302 registers a power supply that provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and simply calls power_supply_changed() when ever needed (when we know the limits and when a source is attached/de-attached -> changes PRESENT & ONLINE properties). The fusb302 driver does not need to know if there are any consumers for it or not. The platform driver that registers the device for it will simply assign the consumer for it in this case, but in the future we want to get that kind of detail from the platform of course, so ACPI or DT. The PMIC charger driver will similarly register a power supply device and function pretty much exactly the same way. The consumer, bq24190, will receive notification from psy framework to its external_power_changed hook when ever either of the supplies calls power_supply_changed(). It then needs to check the properties of the supply (the external power) that are interesting to it in order to set the limits accordingly (this btw. is where the psy API and the class driver can be improved without much effort to make things easier for the consumers). The consumer driver in any case does not need to know what the supplies actually are, how many of them it has, or are there any at all, just like the drivers for the supplies don't need to know the consumers. But in any case, we should never just pick a power supply and set its values from outside its driver, even if the current API allows it. Instead maybe we should try to prevent that by improving the API. > For 3. (vbus) we could do something similar using a > "vbus-regulator-id" device-property and the regulator > framework, making the driver which controls Vbus register > itself as a regulator. The regulator framework does sound reasonable. > I can take a shot at implementing both suggestions, but > first lets try to get some general idea of how we want > to solve this. I hope I was able to explain myself, and make my case. One more thing. I think we could actually build a little bit of hierarchy and make the fusb302 the supply of, not bq24190, but the PMIC instead. The PMIC would then be the only supply for the bq24190. Then in case of TYPEC_CC_RP_DEF, the PMIC charger driver would always know that it needs to execute the detection, and otherwise simply passthough the limits. The driver for fusb302 would not need to do anything extra in case of TYPEC_CC_RP_DEF. Thanks, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-17 11:45 ` Heikki Krogerus @ 2017-05-17 14:47 ` Hans de Goede 2017-05-18 8:37 ` Heikki Krogerus 0 siblings, 1 reply; 26+ messages in thread From: Hans de Goede @ 2017-05-17 14:47 UTC (permalink / raw) To: Heikki Krogerus Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel Hi, On 17-05-17 13:45, Heikki Krogerus wrote: > Hi, > > On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote: >> Hi, >> >> On 05/16/2017 02:07 PM, Heikki Krogerus wrote: >> But we don't really have multiple sources here, we have only >> one source, the USB-C cable hooked to an external power-supply. >> Just because 2 different pieces of hardware may be involved in >> determining the current limit does not mean that we should >> model this as 2 different sources. Just as you rightfully >> pointed out that me using 2 extcon devices for the single >> Type-C connector is wrong, modelling it as 2 sources is >> wrong too. > > The power supply devices don't represent the port like the extcon > device would. The power supply devices represent the two types > of external chargers we support. So BC1.2 and Type-C/PD source. Which are both USB chargers, and the TI bq24290 driver itself also registers itself as a USB charger, continued below ... >>> 1. Tell the BC1.2 detection to start from fusb302 driver >>> 2. Deliver the power limits to the discrete charger ic or battery driver >>> 3. Tell what ever regulates VBUS to start driving VBUS. <snip> >>> The second problem definitely needs to be handled using psy framework. >>> The psy framework provides already everything needed for that. >> >> So above you're talking about sources to the bq24190, which if >> I understand you correctly suggest that you want the tcpm code to >> register a power-supply device per Type-C port ? > > No, the underlying device driver (so fusb302) needs to register the > power supply at this point. We just notify the psy framework if the > limits we get from tcpm to the set_current_limit hook change. > >> I'm not sure that >> is a good idea, any registered power-supply devices will show up >> under /sys/class/power_supply, currently on a typical system >> there will be 2 entries under /sys/class/power_supply one for >> the "Mains" or USB power input and one for the battery, adding >> more entries there is likely to confuse userspace. > > The userspace uses the type attribute to differentiate the supplies. > Otherwise it would not be able to tell even which is the Battery and > which is Mains. Having more power supplies is not a problem. I disagree yes power-supply devices have a type, so if we do as you suggest we end up with 3 power-supplies with type USB under /sys/class/power_supply suggesting to userspace that the device may be supplied with power through 3 different USB connectors. This is simply just wrong. I understand that you want to decouple the different drivers from each-other and I agree with that as a goal. But using the power-supply subsys in the way you suggests means that the way we end up solving a problem which is purely about different pieces of code in the kernel talking to each other gets leaked to userspace and once we've done this userspace may start depending on this and we cannot change things later. TL;DR: I'm against the FUSB302 and the BC1.2 drivers each registering a power-supply because: 1) There should be only one power-supply device registered not 3, since there is only one power-input to the board, not 3. 2) Ideally the in kernel interface should not be visible to userspace at all, we are still figuring all of this out and we may need to change things later leaking internal kernel details to userspace in something which will become an ABI is not a good idea. I've added Sebastian Reichel, the power-supply subsys maintainer to the Cc so that he can weigh in on this. >> More in general having 2 power-supply devices for what is >> in essence one power-input feels wrong. >> >> We can still use the power-supply framework, but I would >> envision this working like this: >> >> The platform code which enumerates the type-c controller >> sets a "input-current-power-supply-name" string device-property >> on the tcpm's (parent)device. When this is set the tpcm code >> will do a power_supply_get_by_name and set the input >> current on the returned power_supply by setting the >> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. > > The psy framework is probable a bit messy at them moment. It > definitely does not do much protecting and in many cases one driver > registers a power supply and an other just takes it over, but that > should be avoided as it makes things difficult (painful) to maintain. > It also poses risks IMO. There certainly almost never seems to be a > real need for that. > > In this case the driver for fusb302 registers a power supply that > provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and > simply calls power_supply_changed() when ever needed (when we know the > limits and when a source is attached/de-attached -> changes PRESENT > & ONLINE properties). The fusb302 driver does not need to know if > there are any consumers for it or not. The platform driver that > registers the device for it will simply assign the consumer for it in > this case, but in the future we want to get that kind of detail from > the platform of course, so ACPI or DT. > > The PMIC charger driver will similarly register a power supply device > and function pretty much exactly the same way. > > The consumer, bq24190, will receive notification from psy framework to > its external_power_changed hook when ever either of the supplies > calls power_supply_changed(). It then needs to check the properties of > the supply (the external power) that are interesting to it in order to > set the limits accordingly (this btw. is where the psy API and the > class driver can be improved without much effort to make things easier > for the consumers). The consumer driver in any case does not need to > know what the supplies actually are, how many of them it has, or are > there any at all, just like the drivers for the supplies don't need to > know the consumers. I like parts of this idea, but as said I've trouble with exporting 3 power-supply devices to userspace for this. I must also say that I'm not a fan of making the BC-1.2 charger a separate power-supply and letting the consumer figure out which one to use, but lets forget about the BC-1.2 charger for now and focus on solving this for just setting the TYPE-C determined input current limit for now. <snip> > I hope I was able to explain myself, and make my case. Explain yes, but I'm really worried about the consequences of exporting extra API/ABI to userspace for this, while we are purely trying to solve in an kernel communication problem. > One more thing. I think we could actually build a little bit of > hierarchy and make the fusb302 the supply of, not bq24190, but the > PMIC instead. The PMIC would then be the only supply for the bq24190. Actually if you look at the schematic (I don't have a schematic for my device, but I can deduce one) then the bq24190 is supplying power to the pmic not the other-way around and the fusb302 is not supplying power in anyway, it purely is negotiating things, but from an electrical pov it is not supplying anything. Anyways as said lets forgot about the PMIC doing BC1.2 detection for now and first focus on how we can get the current-limit info from the fusb302 driver to the power-supply device registered by the bq24190 driver. Regards, Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-17 14:47 ` USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede @ 2017-05-18 8:37 ` Heikki Krogerus 2017-05-18 11:50 ` Heikki Krogerus 2017-05-19 20:12 ` Hans de Goede 0 siblings, 2 replies; 26+ messages in thread From: Heikki Krogerus @ 2017-05-18 8:37 UTC (permalink / raw) To: Hans de Goede Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: > Hi, > > On 17-05-17 13:45, Heikki Krogerus wrote: > > Hi, > > > > On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 05/16/2017 02:07 PM, Heikki Krogerus wrote: > > > But we don't really have multiple sources here, we have only > > > one source, the USB-C cable hooked to an external power-supply. > > > Just because 2 different pieces of hardware may be involved in > > > determining the current limit does not mean that we should > > > model this as 2 different sources. Just as you rightfully > > > pointed out that me using 2 extcon devices for the single > > > Type-C connector is wrong, modelling it as 2 sources is > > > wrong too. > > > > The power supply devices don't represent the port like the extcon > > device would. The power supply devices represent the two types > > of external chargers we support. So BC1.2 and Type-C/PD source. > > Which are both USB chargers, and the TI bq24290 driver itself > also registers itself as a USB charger, continued below ... > > > > > 1. Tell the BC1.2 detection to start from fusb302 driver > > > > 2. Deliver the power limits to the discrete charger ic or battery driver > > > > 3. Tell what ever regulates VBUS to start driving VBUS. > > <snip> > > > > > The second problem definitely needs to be handled using psy framework. > > > > The psy framework provides already everything needed for that. > > > > > > So above you're talking about sources to the bq24190, which if > > > I understand you correctly suggest that you want the tcpm code to > > > register a power-supply device per Type-C port ? > > > > No, the underlying device driver (so fusb302) needs to register the > > power supply at this point. We just notify the psy framework if the > > limits we get from tcpm to the set_current_limit hook change. > > > > > I'm not sure that > > > is a good idea, any registered power-supply devices will show up > > > under /sys/class/power_supply, currently on a typical system > > > there will be 2 entries under /sys/class/power_supply one for > > > the "Mains" or USB power input and one for the battery, adding > > > more entries there is likely to confuse userspace. > > > > The userspace uses the type attribute to differentiate the supplies. > > Otherwise it would not be able to tell even which is the Battery and > > which is Mains. Having more power supplies is not a problem. > > I disagree yes power-supply devices have a type, so if we do > as you suggest we end up with 3 power-supplies with type USB under > /sys/class/power_supply suggesting to userspace that the device > may be supplied with power through 3 different USB connectors. > > This is simply just wrong. I understand that you want to decouple > the different drivers from each-other and I agree with that as > a goal. > > But using the power-supply subsys in the way you suggests means > that the way we end up solving a problem which is purely > about different pieces of code in the kernel talking to each > other gets leaked to userspace and once we've done this userspace > may start depending on this and we cannot change things later. > > TL;DR: I'm against the FUSB302 and the BC1.2 drivers each > registering a power-supply because: > > 1) There should be only one power-supply device registered > not 3, since there is only one power-input to the board, not 3. > > 2) Ideally the in kernel interface should not be visible to > userspace at all, we are still figuring all of this out and > we may need to change things later leaking internal kernel > details to userspace in something which will become an ABI > is not a good idea. > > I've added Sebastian Reichel, the power-supply subsys > maintainer to the Cc so that he can weigh in on this. > > > > More in general having 2 power-supply devices for what is > > > in essence one power-input feels wrong. > > > > > > We can still use the power-supply framework, but I would > > > envision this working like this: > > > > > > The platform code which enumerates the type-c controller > > > sets a "input-current-power-supply-name" string device-property > > > on the tcpm's (parent)device. When this is set the tpcm code > > > will do a power_supply_get_by_name and set the input > > > current on the returned power_supply by setting the > > > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property. > > > > The psy framework is probable a bit messy at them moment. It > > definitely does not do much protecting and in many cases one driver > > registers a power supply and an other just takes it over, but that > > should be avoided as it makes things difficult (painful) to maintain. > > It also poses risks IMO. There certainly almost never seems to be a > > real need for that. > > > > In this case the driver for fusb302 registers a power supply that > > provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and > > simply calls power_supply_changed() when ever needed (when we know the > > limits and when a source is attached/de-attached -> changes PRESENT > > & ONLINE properties). The fusb302 driver does not need to know if > > there are any consumers for it or not. The platform driver that > > registers the device for it will simply assign the consumer for it in > > this case, but in the future we want to get that kind of detail from > > the platform of course, so ACPI or DT. > > > > The PMIC charger driver will similarly register a power supply device > > and function pretty much exactly the same way. > > > > The consumer, bq24190, will receive notification from psy framework to > > its external_power_changed hook when ever either of the supplies > > calls power_supply_changed(). It then needs to check the properties of > > the supply (the external power) that are interesting to it in order to > > set the limits accordingly (this btw. is where the psy API and the > > class driver can be improved without much effort to make things easier > > for the consumers). The consumer driver in any case does not need to > > know what the supplies actually are, how many of them it has, or are > > there any at all, just like the drivers for the supplies don't need to > > know the consumers. > > I like parts of this idea, but as said I've trouble with exporting 3 > power-supply devices to userspace for this. > > I must also say that I'm not a fan of making the BC-1.2 charger > a separate power-supply and letting the consumer figure out which > one to use, but lets forget about the BC-1.2 charger for now and > focus on solving this for just setting the TYPE-C determined > input current limit for now. OK, You have a point. I happy to agree that adding an other psy for the BC1.2 charger alone is not the correct thing to do. I'm mainly interested in just considering USB as a power supply with a board like this, because really, that is what it is, but also by using the power supply class properly (and possibly improving it a little), we avoid unnecessary software couplings. > > I hope I was able to explain myself, and make my case. > > Explain yes, but I'm really worried about the consequences of exporting > extra API/ABI to userspace for this, while we are purely trying to > solve in an kernel communication problem. > > > One more thing. I think we could actually build a little bit of > > hierarchy and make the fusb302 the supply of, not bq24190, but the > > PMIC instead. The PMIC would then be the only supply for the bq24190. > > Actually if you look at the schematic (I don't have a schematic for > my device, but I can deduce one) then the bq24190 is supplying power > to the pmic not the other-way around and the fusb302 is not supplying > power in anyway, it purely is negotiating things, but from an electrical > pov it is not supplying anything. Anyways as said lets forgot about > the PMIC doing BC1.2 detection for now and first focus on how we > can get the current-limit info from the fusb302 driver to the > power-supply device registered by the bq24190 driver. I second that. Thanks Hans, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-18 8:37 ` Heikki Krogerus @ 2017-05-18 11:50 ` Heikki Krogerus 2017-05-19 20:12 ` Hans de Goede 1 sibling, 0 replies; 26+ messages in thread From: Heikki Krogerus @ 2017-05-18 11:50 UTC (permalink / raw) To: Hans de Goede Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel On Thu, May 18, 2017 at 11:37:56AM +0300, Heikki Krogerus wrote: > I'm mainly interested in just considering USB as a power supply with a > board like this, because really, that is what it is, but also by using > the power supply class properly (and possibly improving it a little), > we avoid unnecessary software couplings. "USB" -> "USB Type-C/PD". Sorry about that. -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-18 8:37 ` Heikki Krogerus 2017-05-18 11:50 ` Heikki Krogerus @ 2017-05-19 20:12 ` Hans de Goede 2017-05-19 20:15 ` Hans de Goede 2017-05-22 14:56 ` Heikki Krogerus 1 sibling, 2 replies; 26+ messages in thread From: Hans de Goede @ 2017-05-19 20:12 UTC (permalink / raw) To: Heikki Krogerus Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel Hi, On 18-05-17 10:37, Heikki Krogerus wrote: > On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: >> Hi, >> >> On 17-05-17 13:45, Heikki Krogerus wrote: <snip> >>> In this case the driver for fusb302 registers a power supply that >>> provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and >>> simply calls power_supply_changed() when ever needed (when we know the >>> limits and when a source is attached/de-attached -> changes PRESENT >>> & ONLINE properties). The fusb302 driver does not need to know if >>> there are any consumers for it or not. The platform driver that >>> registers the device for it will simply assign the consumer for it in >>> this case, but in the future we want to get that kind of detail from >>> the platform of course, so ACPI or DT. >>> >>> The PMIC charger driver will similarly register a power supply device >>> and function pretty much exactly the same way. >>> >>> The consumer, bq24190, will receive notification from psy framework to >>> its external_power_changed hook when ever either of the supplies >>> calls power_supply_changed(). It then needs to check the properties of >>> the supply (the external power) that are interesting to it in order to >>> set the limits accordingly (this btw. is where the psy API and the >>> class driver can be improved without much effort to make things easier >>> for the consumers). The consumer driver in any case does not need to >>> know what the supplies actually are, how many of them it has, or are >>> there any at all, just like the drivers for the supplies don't need to >>> know the consumers. >> >> I like parts of this idea, but as said I've trouble with exporting 3 >> power-supply devices to userspace for this. >> >> I must also say that I'm not a fan of making the BC-1.2 charger >> a separate power-supply and letting the consumer figure out which >> one to use, but lets forget about the BC-1.2 charger for now and >> focus on solving this for just setting the TYPE-C determined >> input current limit for now. > > OK, You have a point. I happy to agree that adding an other psy for > the BC1.2 charger alone is not the correct thing to do. > > I'm mainly interested in just considering USB as a power supply with a > board like this, because really, that is what it is, but also by using > the power supply class properly (and possibly improving it a little), > we avoid unnecessary software couplings. Ok, so although I'm still not 100% sold on having the fusb302 driver registering a power-supply is a good idea from an userspace API pov, I from a design pov otherwise. And in a way it does make sense the fusb302 driver is a way to query (and in some case modify) settings of the external power-supply connected to the USB-C port. So maybe we need a new "External" power-supply type for this ? Either way this should not be a real issue since userspace (upower) does not seem to do much if anything with power-supply class devices with a type of USB. So lets go with the plan to make the fusb302 driver register a power-supply for now, which will be a supplier for (for example) the bq24190 charger. 2 questions about implementing this: 1) You said you want the power-supply code (get_property, etc.) to live in the fusb302 driver, rather then in the tcpm code. I agree that the fusb302 device should be the parent-device of the power-supply, but I can see registering a power-supply and querying things like POWER_SUPPLY_PROP_VOLTAGE_MAX being something which we will want in other USB TYPE-C drivers too, so wouldn't it be better to have this code in the generic tcpm bits ? 2) I could modify the bq24190 driver to check and update its input-current-limit itself from its external_power_changed callback, but this seems like something which many charger drivers will need to implement so instead I think that drivers like bq24190 should just be able to set a "sync_input_current_with_suppliers" flag and then when a supplier calls power_supply_changed() have the core call set_property PROP_INPUT_CURRENT_LIMIT. That way all we need to do to get charger drivers to support this is set that flag rather then copy and paste code. Or maybe a power_supply_sync_input_current_with_suppliers() helper which drivers can call from their external_power_changed callback. Thinking more about this I think I like the helper function idea better. Regards, Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-19 20:12 ` Hans de Goede @ 2017-05-19 20:15 ` Hans de Goede 2017-05-22 14:56 ` Heikki Krogerus 1 sibling, 0 replies; 26+ messages in thread From: Hans de Goede @ 2017-05-19 20:15 UTC (permalink / raw) To: Heikki Krogerus Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel Hi, On 19-05-17 22:12, Hans de Goede wrote: > Hi, > > On 18-05-17 10:37, Heikki Krogerus wrote: >> On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 17-05-17 13:45, Heikki Krogerus wrote: > > <snip> > >>>> In this case the driver for fusb302 registers a power supply that >>>> provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and >>>> simply calls power_supply_changed() when ever needed (when we know the >>>> limits and when a source is attached/de-attached -> changes PRESENT >>>> & ONLINE properties). The fusb302 driver does not need to know if >>>> there are any consumers for it or not. The platform driver that >>>> registers the device for it will simply assign the consumer for it in >>>> this case, but in the future we want to get that kind of detail from >>>> the platform of course, so ACPI or DT. >>>> >>>> The PMIC charger driver will similarly register a power supply device >>>> and function pretty much exactly the same way. >>>> >>>> The consumer, bq24190, will receive notification from psy framework to >>>> its external_power_changed hook when ever either of the supplies >>>> calls power_supply_changed(). It then needs to check the properties of >>>> the supply (the external power) that are interesting to it in order to >>>> set the limits accordingly (this btw. is where the psy API and the >>>> class driver can be improved without much effort to make things easier >>>> for the consumers). The consumer driver in any case does not need to >>>> know what the supplies actually are, how many of them it has, or are >>>> there any at all, just like the drivers for the supplies don't need to >>>> know the consumers. >>> >>> I like parts of this idea, but as said I've trouble with exporting 3 >>> power-supply devices to userspace for this. >>> >>> I must also say that I'm not a fan of making the BC-1.2 charger >>> a separate power-supply and letting the consumer figure out which >>> one to use, but lets forget about the BC-1.2 charger for now and >>> focus on solving this for just setting the TYPE-C determined >>> input current limit for now. >> >> OK, You have a point. I happy to agree that adding an other psy for >> the BC1.2 charger alone is not the correct thing to do. >> >> I'm mainly interested in just considering USB as a power supply with a >> board like this, because really, that is what it is, but also by using >> the power supply class properly (and possibly improving it a little), >> we avoid unnecessary software couplings. > > Ok, so although I'm still not 100% sold on having the fusb302 driver > registering a power-supply is a good idea from an userspace API pov, I > from a design pov otherwise. And in a way it does make sense the fusb302 > driver is a way to query (and in some case modify) settings of the external > power-supply connected to the USB-C port. Ugh I somewhat mangled the first sentence of this paragraph, here is a correct version: Ok, so although I'm still not 100% sold on having the fusb302 driver registering a power-supply being a good idea from an userspace API pov, I do actually like it from a design pov. > So maybe we need a new "External" power-supply type for this ? Either way > this should not be a real issue since userspace (upower) does not seem to > do much if anything with power-supply class devices with a type of USB. > > So lets go with the plan to make the fusb302 driver register a power-supply > for now, which will be a supplier for (for example) the bq24190 charger. > > 2 questions about implementing this: > > 1) You said you want the power-supply code (get_property, etc.) to live > in the fusb302 driver, rather then in the tcpm code. I agree that the > fusb302 device should be the parent-device of the power-supply, but I can > see registering a power-supply and querying things like > POWER_SUPPLY_PROP_VOLTAGE_MAX being something which we will want in other > USB TYPE-C drivers too, so wouldn't it be better to have this code in > the generic tcpm bits ? > > 2) I could modify the bq24190 driver to check and update its > input-current-limit itself from its external_power_changed callback, > but this seems like something which many charger drivers will need > to implement so instead I think that drivers like bq24190 should just > be able to set a "sync_input_current_with_suppliers" flag and then > when a supplier calls power_supply_changed() have the core call > set_property PROP_INPUT_CURRENT_LIMIT? That way all we need to do > to get charger drivers to support this is set that flag rather then > copy and paste code. Or maybe a > power_supply_sync_input_current_with_suppliers() helper which drivers > can call from their external_power_changed callback ? >> Thinking more about this I think I like the helper function idea better Regards, Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) 2017-05-19 20:12 ` Hans de Goede 2017-05-19 20:15 ` Hans de Goede @ 2017-05-22 14:56 ` Heikki Krogerus 1 sibling, 0 replies; 26+ messages in thread From: Heikki Krogerus @ 2017-05-22 14:56 UTC (permalink / raw) To: Hans de Goede Cc: Guenter Roeck, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Sebastian Reichel On Fri, May 19, 2017 at 10:12:14PM +0200, Hans de Goede wrote: > Hi, > > On 18-05-17 10:37, Heikki Krogerus wrote: > > On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 17-05-17 13:45, Heikki Krogerus wrote: > > <snip> > > > > > In this case the driver for fusb302 registers a power supply that > > > > provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and > > > > simply calls power_supply_changed() when ever needed (when we know the > > > > limits and when a source is attached/de-attached -> changes PRESENT > > > > & ONLINE properties). The fusb302 driver does not need to know if > > > > there are any consumers for it or not. The platform driver that > > > > registers the device for it will simply assign the consumer for it in > > > > this case, but in the future we want to get that kind of detail from > > > > the platform of course, so ACPI or DT. > > > > > > > > The PMIC charger driver will similarly register a power supply device > > > > and function pretty much exactly the same way. > > > > > > > > The consumer, bq24190, will receive notification from psy framework to > > > > its external_power_changed hook when ever either of the supplies > > > > calls power_supply_changed(). It then needs to check the properties of > > > > the supply (the external power) that are interesting to it in order to > > > > set the limits accordingly (this btw. is where the psy API and the > > > > class driver can be improved without much effort to make things easier > > > > for the consumers). The consumer driver in any case does not need to > > > > know what the supplies actually are, how many of them it has, or are > > > > there any at all, just like the drivers for the supplies don't need to > > > > know the consumers. > > > > > > I like parts of this idea, but as said I've trouble with exporting 3 > > > power-supply devices to userspace for this. > > > > > > I must also say that I'm not a fan of making the BC-1.2 charger > > > a separate power-supply and letting the consumer figure out which > > > one to use, but lets forget about the BC-1.2 charger for now and > > > focus on solving this for just setting the TYPE-C determined > > > input current limit for now. > > > > OK, You have a point. I happy to agree that adding an other psy for > > the BC1.2 charger alone is not the correct thing to do. > > > > I'm mainly interested in just considering USB as a power supply with a > > board like this, because really, that is what it is, but also by using > > the power supply class properly (and possibly improving it a little), > > we avoid unnecessary software couplings. > > Ok, so although I'm still not 100% sold on having the fusb302 driver > registering a power-supply is a good idea from an userspace API pov, I > from a design pov otherwise. And in a way it does make sense the fusb302 > driver is a way to query (and in some case modify) settings of the external > power-supply connected to the USB-C port. > > So maybe we need a new "External" power-supply type for this ? Either way > this should not be a real issue since userspace (upower) does not seem to > do much if anything with power-supply class devices with a type of USB. > > So lets go with the plan to make the fusb302 driver register a power-supply > for now, which will be a supplier for (for example) the bq24190 charger. Couldn't we extend the psy framework a little so it would be able to support classes of devices that are not being presented to the user space as power supplies as such, but are possibly just part of a chain that makes up a single power supply? I'm playing with ideas at this point, I'm not sure if that is feasible, but it does not sound impossible. I've been playing with that idea for some time now. Maybe it's time to really check out what could be done :-). > 2 questions about implementing this: > > 1) You said you want the power-supply code (get_property, etc.) to live > in the fusb302 driver, rather then in the tcpm code. I agree that the > fusb302 device should be the parent-device of the power-supply, but I can > see registering a power-supply and querying things like > POWER_SUPPLY_PROP_VOLTAGE_MAX being something which we will want in other > USB TYPE-C drivers too, so wouldn't it be better to have this code in > the generic tcpm bits ? I was thinking that at first stage it would be better to mix the support into the device drivers, and later move it to tcpm, but I'm not against it. Actually, I was hoping that we could at one point consider every USB port as a potential "power supply", not only Type-C ports, and therefore handle the support in USB core, but that at least is something that we probable should not think about at this point. > 2) I could modify the bq24190 driver to check and update its > input-current-limit itself from its external_power_changed callback, > but this seems like something which many charger drivers will need > to implement so instead I think that drivers like bq24190 should just > be able to set a "sync_input_current_with_suppliers" flag and then > when a supplier calls power_supply_changed() have the core call > set_property PROP_INPUT_CURRENT_LIMIT. That way all we need to do > to get charger drivers to support this is set that flag rather then > copy and paste code. Or maybe a > power_supply_sync_input_current_with_suppliers() helper which drivers > can call from their external_power_changed callback. Thinking more > about this I think I like the helper function idea better. That sounds reasonable to me. Br, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-05-12 21:22 ` Hans de Goede 2017-05-16 12:07 ` Heikki Krogerus @ 2017-05-16 22:52 ` Guenter Roeck 1 sibling, 0 replies; 26+ messages in thread From: Guenter Roeck @ 2017-05-16 22:52 UTC (permalink / raw) To: Hans de Goede Cc: Heikki Krogerus, Andy Shevchenko, MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org, Yueyao Zhu, Badhri Jagan Sridharan On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote: > Hi, > > On 24-04-17 15:12, Guenter Roeck wrote: > >On 04/24/2017 04:02 AM, Heikki Krogerus wrote: > >>+Guenter > >> > >>On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote: > >>>+Cc: Heikki. > >>> > >>>He might comment on this. > >> > >>Thanks Andy. > >> > >>>On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: > >>>>Add support for USB TYPE-C cable detection on systems using a > >>>>FUSB302 USB TYPE-C controller. > >>>> > >>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>--- > >>>> drivers/extcon/Kconfig | 8 + > >>>> drivers/extcon/Makefile | 1 + > >>>> drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ > >>>> 3 files changed, 791 insertions(+) > >>>> create mode 100644 drivers/extcon/extcon-fusb302.c > >> > >>There is now the typec class in linux-next that really needs to be > >>used with all USB Type-C PHYs like fusb302. > >> > >>Unless I'm mistaken, Guenter has also written a driver for fusb302. I > > > >Not me; it was Nathan. > > > >>have not seen it, but if I understood correctly, that driver will > >>register itself with the upcoming tcpm (USB Type-C Port Manager) [1], > >>which in practice would mean we can take properly advantage of the USB > >>PD transceiver on fusb302. > >> > >>Guenter! Can you publish the fusb302 driver? > >> > > > >Working on it. > > Ok, I see this has landed in staging. So lets go with this driver > then and not mine (I did not get around to the PD stuff yet, so > that is fine). > > Question, how is this supposed to interface with the rest of the > kernel ? Specifically the commit message for the tcpm frameworks > says: > > "This driver only implements the state machine. Lower level drivers are > responsible for > - Reporting VBUS status and activating VBUS > - Setting CC lines and providing CC line status > - Setting line polarity > - Activating and deactivating VCONN > - Setting the current limit > - Activating and deactivating PD message transfers > - Sending and receiving PD messages" > > But the FUSB302 cannot set the current limit... > There is an underlying driver which I didn't pay enough attention to. Badhri, Yueyao, can we publish the pd engine code as well ? If that doesn't solve the problem, it might at least give a starting point. Thanks, Guenter > The device I'm working on: > http://www.gpd.hk/gpdwin.asp > > Has the following more or less relevant components: > > -Intel Cherry Trail Z8700 SoC > -Intel Cherry Trail Whiskey Cove PMIC (which is different from the Broxton Whiskey Cove PMIC), > this PMIC uses an external charger and fuel-gauge: > -TI bq24292i charger > -Maxim MAX17047 fuel-gauge > -PI3USB30532 USB TYPE-C mux > > Currently I only have the USB-C connector on the > device working in USB-2 mode (I did not realize > the USB-3 bits were wired up at first). > > The device ships with a charger with an USB-A > receptacle and an USB-2 only USB-A to USB-C > cable. > > The way I've hooked up USB-2 charging is like this: > The Whiskey Cove PMIC has built in USB-2 BC detection, and > I've written an extcon driver for this which reports one > of the following cables depended on the BC detection: > > EXTCON_CHG_USB_SDP, > EXTCON_CHG_USB_CDP, > EXTCON_CHG_USB_DCP, > > And I've modified the bq24290_charger driver to (optionally) > receive an extcon name through device-properties and if > it does, then set the input current based on the detected > charger type (0.5 A for SDP, 1.5A for CDP, 2A for DCP). > > Most of the patches for this are upstream. But once we > hookup the FUSB302 driver we need to propagate the current > limit it has negotiated to the bq24290_charger driver. > > Which is part of the reason why I wrote the 5th patch of > my original series, let me reply to Heikki's question about > that one here as it is related: > > On 24-04-17 16:25, Heikki Krogerus wrote: > > > Doesn't this mean you have several extcon_dev registered for a > > single port? > > Yes this is unfortunate, but the primary consumer of extcon > info is the kernel itself and we can teach it to look at the > right one. > > > I'm not completely sure how extcon framework is meant to > > be used, but shouldn't a single extcon_dev represent all the types of > > connectors a port is meant to support? > > Ideally, yes. But here we've 2 chips / drivers connected > to the same connector (more even, but 2 which provide extcon > related info). > > As explained above the device ships with an USB-2 charger + > USB-A to USB-C cable, that cable correctly gets seen by the > FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input > current to 0.5A, but as the FUSB302 datasheet says: > > "The host software is expected to follow the USB Type-C > specification for charging current priority based on > feedback from the FUSB302 detection, external BC1.2 detection > and any USB Power Delivery communication. > > The FUSB302 does not integrate BC1.2 charger > detection which is assumed available in the USB > transceiver or USB charger in the system." > > So when we see TYPEC_CC_RP_DEF we need to ask the > Cherry Trail PMIC (in my case) to do BC detection > and use the result of that, otherwise we end up > setting input current limit way too low. > > >> Since the Type-C controller info is incomplete and needs to be > >> supplemented with the BC1.2 detection result in some cases, this > >> commit makes the intel-cht-wc extcon driver monitor the extcon device > >> registered by the Type-C controller and report that as our extcon state > >> except when BC-1.2 detection should be used. This allows extcon > >> consumers on these boards to keep monitoring only the intel-cht-wc extcon > >> and then get complete extcon info from that, which combines the Type-C > >> and BC-1.2 info. > > > > I don't really understand why does this need to be done in such a > > complex manner? Both components should just report the result and be > > done with it, and there is no need for any coupling between the two. > > If there is a consumer for the power, the driver for it can decide on > > its own the limits and maximum power from the two sources. > > To me making the combination of the 2 sources the problem > of the consumer seems just wrong, as you mentioned > above there should really be only one extcon for the > connector, likewise their should be only one definitive > source on what the input current limit is. > > TL;DR: We need some way to combine the current limit info > from TYPE-C and USB-2 BC detection into a single source and > to propagate that current to drivers which can actually set > the current-limit. > > Note I'm happy to use something else then extcon for this, > but we do need some way to combine + propagate the > current-limit. > > Likewise we need a way to tell another driver to drive VBus > on the USB-C connector. In my case this is again done by the > bq24290_charger driver, which currently does this based on the > combination of EXTCON_CHG_USB_* not being set on the extcon + > EXTCON_USB_HOST being set. > > One possible solution would be for the > drivers/extcon/extcon-intel-cht-wc.c to somehow hook into > the tpmc device and to get notifications of state changes, > then it could reflect the info from the FUSB302 in its > extcon info, not sure if that is the best design though. > > What also seems to missing is for a way to hookup a > mux driver to deal with upside-down plug-ins and > alt-mode switching. > > Regards, > > Hans ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede 2017-04-21 18:51 ` Andy Shevchenko @ 2017-04-21 23:23 ` kbuild test robot 2017-05-23 9:27 ` Chanwoo Choi 2 siblings, 0 replies; 26+ messages in thread From: kbuild test robot @ 2017-04-21 23:23 UTC (permalink / raw) To: Hans de Goede Cc: kbuild-all, MyungJoo Ham, Chanwoo Choi, Hans de Goede, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2105 bytes --] Hi Hans, [auto build test ERROR on chanwoo-extcon/extcon-next] [also build test ERROR on v4.11-rc7 next-20170421] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/extcon-Allow-extcon-drivers-to-specify-the-extcon-name/20170422-025657 base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next config: i386-allmodconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/extcon/extcon-fusb302.c: In function 'fusb302_set_state': >> drivers/extcon/extcon-fusb302.c:311:18: error: 'USB_TYPEC_POLARITY_NORMAL' undeclared (first use in this function) prop.intval = USB_TYPEC_POLARITY_NORMAL; ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-fusb302.c:311:18: note: each undeclared identifier is reported only once for each function it appears in >> drivers/extcon/extcon-fusb302.c:315:18: error: 'USB_TYPEC_POLARITY_FLIP' undeclared (first use in this function) prop.intval = USB_TYPEC_POLARITY_FLIP; ^~~~~~~~~~~~~~~~~~~~~~~ vim +/USB_TYPEC_POLARITY_NORMAL +311 drivers/extcon/extcon-fusb302.c 305 fusb302_write_reg(data, REG_POWER, POWER_NORMAL); 306 307 /* Enable pull-down on CC1 / CC2 based on orientation */ 308 switch (data->status1a & STATUS1A_TOGSS_MASK) { 309 case STATUS1A_TOGSS_SNK_CC1: 310 switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; > 311 prop.intval = USB_TYPEC_POLARITY_NORMAL; 312 break; 313 case STATUS1A_TOGSS_SNK_CC2: 314 switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; > 315 prop.intval = USB_TYPEC_POLARITY_FLIP; 316 break; 317 } 318 fusb302_write_reg(data, REG_SWITCHES0, switches0); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 59021 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede 2017-04-21 18:51 ` Andy Shevchenko 2017-04-21 23:23 ` kbuild test robot @ 2017-05-23 9:27 ` Chanwoo Choi 2017-05-24 12:23 ` Andy Shevchenko 2 siblings, 1 reply; 26+ messages in thread From: Chanwoo Choi @ 2017-05-23 9:27 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, I'm sorry for late reply. I'll check this thread and then give you my opinion. Regards, Chanwoo Choi On 2017년 04월 21일 22:01, Hans de Goede wrote: > Add support for USB TYPE-C cable detection on systems using a > FUSB302 USB TYPE-C controller. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/Kconfig | 8 + > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 791 insertions(+) > create mode 100644 drivers/extcon/extcon-fusb302.c > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index 32f2dc8..562db5b 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -35,6 +35,14 @@ config EXTCON_AXP288 > Say Y here to enable support for USB peripheral detection > and USB MUX switching by X-Power AXP288 PMIC. > > +config EXTCON_FUSB302 > + tristate "FUSB302 USB TYPE-C controller support" > + depends on I2C > + select REGMAP_I2C > + help > + Say Y here to enable support for USB TYPE-C cable detection using > + a FUSB302 USB TYPE-C controller. > + > config EXTCON_GPIO > tristate "GPIO extcon support" > depends on GPIOLIB || COMPILE_TEST > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index ecfa958..e5eb493 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -7,6 +7,7 @@ extcon-core-objs += extcon.o devres.o > obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o > +obj-$(CONFIG_EXTCON_FUSB302) += extcon-fusb302.o > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o > obj-$(CONFIG_EXTCON_INTEL_INT3496) += extcon-intel-int3496.o > obj-$(CONFIG_EXTCON_INTEL_CHT_WC) += extcon-intel-cht-wc.o > diff --git a/drivers/extcon/extcon-fusb302.c b/drivers/extcon/extcon-fusb302.c > new file mode 100644 > index 0000000..577adb9 > --- /dev/null > +++ b/drivers/extcon/extcon-fusb302.c > @@ -0,0 +1,782 @@ > +/* > + * Extcon USB-C cable detection driver for FUSB302 USB TYPE-C controller > + * > + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/extcon.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > +#include "extcon.h" > + > +#define REG_DEVICE_ID 0x01 > +#define DEVICE_ID_VER_MASK GENMASK(7, 4) > +#define DEVICE_ID_FUSB302_VER 0x80 > + > +#define REG_SWITCHES0 0x02 > +#define SWITCHES0_PDWN1 BIT(0) > +#define SWITCHES0_PDWN2 BIT(1) > +#define SWITCHES0_PDWN GENMASK(1, 0) > +#define SWITCHES0_MEAS_CC1 BIT(2) > +#define SWITCHES0_MEAS_CC2 BIT(3) > +#define SWITCHES0_VCONN_CC1 BIT(4) > +#define SWITCHES0_VCONN_CC2 BIT(5) > +#define SWITCHES0_PU_EN1 BIT(6) > +#define SWITCHES0_PU_EN2 BIT(7) > + > +#define REG_MEASURE 0x04 > +#define MEASURE_MDAC_MASK GENMASK(5, 0) > +/* Datasheet says MDAC must be set to 0x34 / 2226mV for vRd-3.0 detection */ > +#define MEASURE_MDAC_SNK_VRD30 0x34 > +/* MDAC must be set to 0x25 / 1600mV for disconnect det. with 80uA host-cur */ > +#define MEASURE_MDAC_SRC_80UA_HOST_CUR 0x25 > + > +#define REG_CONTROL0 0x06 > +#define CONTROL0_HOST_CUR_MASK GENMASK(3, 2) > +#define CONTROL0_HOST_CUR_DISABLED (0 << 2) > +#define CONTROL0_HOST_CUR_80UA (1 << 2) > +#define CONTROL0_HOST_CUR_180UA (2 << 2) > +#define CONTROL0_HOST_CUR_330UA (3 << 2) > +#define CONTROL0_INT_MASK BIT(5) > + > +#define REG_CONTROL2 0x08 > +#define CONTROL2_TOGGLE BIT(0) > +#define CONTROL2_MODE_MASK GENMASK(2, 1) > +#define CONTROL2_MODE_DRP (1 << 1) > +#define CONTROL2_MODE_SNK (2 << 1) > +#define CONTROL2_MODE_SRC (3 << 1) > +#define CONTROL2_SAVE_PWR_MASK GENMASK(7, 6) > +#define CONTROL2_SAVE_PWR_DISABLED (0 << 6) > +#define CONTROL2_SAVE_PWR_40MS (1 << 6) > +#define CONTROL2_SAVE_PWR_80MS (2 << 6) > +#define CONTROL2_SAVE_PWR_160MS (3 << 6) > + > +#define REG_MASK1 0x0a > +/* REG_MASK1 value for low-power / disabled state from datasheet */ > +#define MASK1_DISABLED 0xfe > +#define MASK1_COMP_CHNG BIT(5) > +#define MASK1_VBUSOK BIT(7) > + > +#define REG_POWER 0x0b > +/* REG_POWER values for disabled and normal state from datasheet */ > +#define POWER_DISABLED BIT(0) > +#define POWER_NORMAL GENMASK(2, 0) > + > +#define REG_RESET 0x0c > +#define RESET_SW_RESET BIT(0) > + > +#define REG_OCP 0x0d > + > +#define REG_MASKA 0x0e > +/* REG_MASKA value for low-power / disabled state from datasheet */ > +#define MASKA_DISABLED 0xbf > + > +#define REG_MASKB 0x0f > +/* REG_MASKB value for low-power / disabled state from datasheet */ > +#define MASKB_DISABLED 0x01 > + > +#define REG_STATUS1A 0x3d > +#define STATUS1A_TOGSS_MASK GENMASK(5, 3) > +#define STATUS1A_TOGSS_SRC_CC1 (1 << 3) > +#define STATUS1A_TOGSS_SRC_CC2 (2 << 3) > +#define STATUS1A_TOGSS_SNK_CC1 (5 << 3) > +#define STATUS1A_TOGSS_SNK_CC2 (6 << 3) > + > +#define REG_INTERRUPTA 0x3e > +#define INTERRUPTA_TOGDONE BIT(6) > + > +#define REG_STATUS0 0x40 > +#define STATUS0_BC_LEVEL_MASK GENMASK(1, 0) > +#define STATUS0_BC_LEVEL_VRA 0 > +#define STATUS0_BC_LEVEL_VRDUSB 1 > +#define STATUS0_BC_LEVEL_VRD15 2 > +#define STATUS0_BC_LEVEL_VRD30 3 > +#define STATUS0_COMP BIT(5) > +#define STATUS0_VBUSOK BIT(7) > + > +#define REG_INTERRUPT 0x42 > +#define INTERRUPT_BC_LVL BIT(0) > +#define INTERRUPT_COMP_CHNG BIT(5) > +#define INTERRUPT_VBUSOK BIT(7) > + > +/* Timeouts from the FUSB302 datasheet */ > +#define TTOGCYCLE msecs_to_jiffies(40 + 60 + 40) > + > +/* Timeouts from the USB-C specification */ > +#define TPDDEBOUNCE msecs_to_jiffies(20) > +#define TDRPTRY_MS 75 > +#define TDRPTRY msecs_to_jiffies(TDRPTRY_MS) > +#define TDRPTRYWAIT msecs_to_jiffies(400) > +#define TVBUSON msecs_to_jiffies(275) > + > +enum typec_port_type { > + TYPEC_PORT_DFP, > + TYPEC_PORT_UFP, > + TYPEC_PORT_DRP, > +}; > + > +enum typec_role { > + TYPEC_SINK, > + TYPEC_SOURCE, > +}; > + > +enum fusb302_state { > + DISABLED_SNK, /* UFP */ > + DISABLED_SRC, /* DFP */ > + DISABLED_DRP, > + UNATTACHED_SNK, > + ATTACHED_SNK, > + UNATTACHED_SRC, > + ATTACHED_SRC, > +}; > +/* For debugging */ > +static __maybe_unused const char *fusb302_state_str[] = { > + "DISABLED_SNK", > + "DISABLED_SRC", > + "DISABLED_DRP", > + "UNATTACHED_SNK", > + "ATTACHED_SNK", > + "UNATTACHED_SRC", > + "ATTACHED_SRC", > +}; > + > +enum fusb302_event { > + TOGGLE_DONE, > + BC_LVL_CHANGE, > + VBUS_VALID, > + VBUS_INVALID, > + COMP_LOW, > + COMP_HIGH, > + TIMEOUT, > + FALLBACK_TIMEOUT, > +}; > +/* For debugging */ > +static __maybe_unused const char *fusb302_event_str[] = { > + "TOGGLE_DONE", > + "BC_LVL_CHANGE", > + "VBUS_VALID", > + "VBUS_INVALID", > + "COMP_LOW", > + "COMP_HIGH", > + "TIMEOUT", > + "FALLBACK_TIMEOUT", > +}; > + > +static const unsigned int fusb302_extcon_cables[] = { > + EXTCON_USB, > + EXTCON_USB_HOST, > + EXTCON_CHG_USB_SDP, > + EXTCON_CHG_USB_CDP, > + EXTCON_CHG_USB_FAST, > + EXTCON_NONE, > +}; > + > +struct fusb302_data { > + struct device *dev; > + struct regmap *regmap; > + struct extcon_dev *edev; > + struct mutex lock; > + struct delayed_work timeout; > + struct delayed_work fallback_timeout; > + struct delayed_work bc_work; > + enum fusb302_state state; > + enum typec_port_type port_type; > + enum typec_role preferred_role; > + int status0; > + int status1a; > + unsigned int snk_cable_id; > +}; > + > +static void fusb302_write_reg(struct fusb302_data *data, int reg, int val) > +{ > + int ret; > + > + ret = regmap_write(data->regmap, reg, val); > + if (ret) > + dev_err(data->dev, "Error writing reg %02x: %d\n", reg, ret); > +} > + > +static int fusb302_read_reg(struct fusb302_data *data, int reg) > +{ > + int ret, val; > + > + ret = regmap_read(data->regmap, reg, &val); > + if (ret) { > + dev_err(data->dev, "Error reading reg %02x: %d\n", reg, ret); > + return 0; > + } > + > + return val; > +} > + > +static void fusb302_update_bits(struct fusb302_data *data, int reg, > + int mask, int val) > +{ > + int ret; > + > + ret = regmap_update_bits(data->regmap, reg, mask, val); > + if (ret) > + dev_err(data->dev, "Error modifying reg %02x: %d\n", reg, ret); > +} > + > +/* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ > +static void fusb302_set_extcon_state(struct fusb302_data *data, > + unsigned int cable, bool state) > +{ > + extcon_set_state_sync(data->edev, cable, state); > + if (cable == EXTCON_CHG_USB_SDP) > + extcon_set_state_sync(data->edev, EXTCON_USB, state); > +} > + > +/* Helper for the disabled states */ > +static void fusb302_disable(struct fusb302_data *data) > +{ > + fusb302_write_reg(data, REG_POWER, POWER_DISABLED); > + fusb302_write_reg(data, REG_MASK1, MASK1_DISABLED); > + fusb302_write_reg(data, REG_MASKA, MASKA_DISABLED); > + fusb302_write_reg(data, REG_MASKB, MASKB_DISABLED); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > +} > + > +/* > + * fusb302_set_state() and fusb302_handle_event() implement the 3 state > + * machines from the datasheet folded into 1 state-machine for code re-use. > + */ > +static void fusb302_set_state(struct fusb302_data *data, > + enum fusb302_state state) > +{ > + int status, switches0 = 0; > + enum fusb302_state old_state = data->state; > + union extcon_property_value prop; > + > + /* Kill pending timeouts from previous state */ > + cancel_delayed_work(&data->timeout); > + cancel_delayed_work(&data->fallback_timeout); > + cancel_delayed_work(&data->bc_work); > + > + dev_dbg(data->dev, "New state %s\n", fusb302_state_str[state]); > + > + switch (old_state) { > + case ATTACHED_SNK: > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > + break; > + case ATTACHED_SRC: > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, false); > + break; > + default: > + break; /* Do nothing */ > + } > + > + switch (state) { > + case DISABLED_SNK: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_SNK); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case DISABLED_SRC: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_SRC); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case DISABLED_DRP: > + fusb302_disable(data); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_MODE_MASK, > + CONTROL2_MODE_DRP); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, > + CONTROL2_TOGGLE); > + break; > + > + case UNATTACHED_SNK: > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > + > + /* Enable pull-down on CC1 / CC2 based on orientation */ > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + switches0 = SWITCHES0_PDWN1 | SWITCHES0_MEAS_CC1; > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > + break; > + case STATUS1A_TOGSS_SNK_CC2: > + switches0 = SWITCHES0_PDWN2 | SWITCHES0_MEAS_CC2; > + prop.intval = USB_TYPEC_POLARITY_FLIP; > + break; > + } > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > + extcon_set_property(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > + > + /* Enable VBUSOK and COMP irq at 2.24V for BC detection */ > + fusb302_update_bits(data, REG_MASK1, > + MASK1_VBUSOK | MASK1_COMP_CHNG, 0); > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > + MEASURE_MDAC_SNK_VRD30); > + > + status = fusb302_read_reg(data, REG_STATUS0); > + if (status & STATUS0_VBUSOK) { > + /* Go straight to ATTACHED_SNK */ > + fusb302_set_state(data, ATTACHED_SNK); > + return; > + } > + > + mod_delayed_work(system_wq, &data->timeout, TVBUSON); > + break; > + > + case ATTACHED_SNK: > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > + break; > + > + case UNATTACHED_SRC: > + fusb302_write_reg(data, REG_POWER, POWER_NORMAL); > + > + /* Enable pull-up / Vconn on CC1 / CC2 based on orientation */ > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SRC_CC1: > + switches0 = SWITCHES0_PU_EN1 | SWITCHES0_VCONN_CC2 | > + SWITCHES0_MEAS_CC1; > + prop.intval = USB_TYPEC_POLARITY_NORMAL; > + break; > + case STATUS1A_TOGSS_SRC_CC2: > + switches0 = SWITCHES0_PU_EN2 | SWITCHES0_VCONN_CC1 | > + SWITCHES0_MEAS_CC2; > + prop.intval = USB_TYPEC_POLARITY_FLIP; > + break; > + } > + fusb302_write_reg(data, REG_SWITCHES0, switches0); > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_TOGGLE, 0); > + extcon_set_property(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY, prop); > + > + /* Enable COMP irq at 1.6V for detach detection */ > + fusb302_update_bits(data, REG_MASK1, MASK1_COMP_CHNG, 0); > + fusb302_update_bits(data, REG_MEASURE, MEASURE_MDAC_MASK, > + MEASURE_MDAC_SRC_80UA_HOST_CUR); > + > + status = fusb302_read_reg(data, REG_STATUS0); > + if (!(status & STATUS0_COMP)) { > + /* Go straight to ATTACHED_SRC */ > + fusb302_set_state(data, ATTACHED_SRC); > + return; > + } > + > + mod_delayed_work(system_wq, &data->timeout, TPDDEBOUNCE); > + break; > + > + case ATTACHED_SRC: > + fusb302_set_extcon_state(data, EXTCON_USB_HOST, true); > + break; > + } > + > + data->state = state; > +} > + > +static void fusb302_set_state_disabled(struct fusb302_data *data) > +{ > + > + switch (data->port_type) { > + case TYPEC_PORT_UFP: > + fusb302_set_state(data, DISABLED_SNK); > + break; > + case TYPEC_PORT_DFP: > + fusb302_set_state(data, DISABLED_SRC); > + break; > + case TYPEC_PORT_DRP: > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + case STATUS1A_TOGSS_SNK_CC2: > + fusb302_set_state(data, UNATTACHED_SNK); > + break; > + } > + break; > + > + case TIMEOUT: > + /* Cannot become snk fallback to src */ > + fusb302_set_state(data, DISABLED_SRC); > + mod_delayed_work(system_wq, &data->fallback_timeout, > + TDRPTRYWAIT); > + break; > + > + case FALLBACK_TIMEOUT: > + /* Both states failed return to disabled drp state */ > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SRC_CC1: > + case STATUS1A_TOGSS_SRC_CC2: > + fusb302_set_state(data, UNATTACHED_SRC); > + break; > + } > + break; > + > + case TIMEOUT: > + /* Cannot become src fallback to snk */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->fallback_timeout, > + TDRPTRYWAIT); > + break; > + > + case FALLBACK_TIMEOUT: > + /* Both states failed return to disabled drp state */ > + fusb302_set_state(data, DISABLED_DRP); > + break; > + } > +} > + > +static void fusb302_handle_disabled_drp_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case TOGGLE_DONE: > + switch (data->status1a & STATUS1A_TOGSS_MASK) { > + case STATUS1A_TOGSS_SNK_CC1: > + case STATUS1A_TOGSS_SNK_CC2: > + if (data->preferred_role == TYPEC_SINK) { > + /* Jump directly to UNATTACHED_SNK */ > + fusb302_set_state(data, UNATTACHED_SNK); > + } else { > + /* Try to become src */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->timeout, > + TDRPTRY); > + } > + break; > + case STATUS1A_TOGSS_SRC_CC1: > + case STATUS1A_TOGSS_SRC_CC2: > + if (data->preferred_role == TYPEC_SOURCE) { > + /* Jump directly to UNATTACHED_SRC */ > + fusb302_set_state(data, UNATTACHED_SRC); > + } else { > + /* > + * The USB-C spec says we must enable pull-downs > + * and then wait tDRPTry before checking to > + * avoid endless role-bouncing if both ends > + * prefer the snk role. > + */ > + fusb302_write_reg(data, REG_SWITCHES0, > + SWITCHES0_PDWN); > + fusb302_update_bits(data, REG_CONTROL2, > + CONTROL2_TOGGLE, 0); > + msleep(TDRPTRY_MS); > + /* > + * Use the toggle engine to do the src > + * detection to keep things the same as for > + * directly entering the src role. > + */ > + fusb302_set_state(data, DISABLED_SNK); > + mod_delayed_work(system_wq, &data->timeout, > + TTOGCYCLE); > + } > + break; > + } > + break; > + } > +} > + > +static void fusb302_handle_unattached_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case VBUS_VALID: /* Cable attached */ > + fusb302_set_state(data, ATTACHED_SNK); > + break; > + case TIMEOUT: > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_attached_snk_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case BC_LVL_CHANGE: > + case COMP_LOW: > + case COMP_HIGH: > + mod_delayed_work(system_wq, &data->bc_work, TPDDEBOUNCE); > + break; > + case VBUS_INVALID: /* Cable detached */ > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_unattached_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case COMP_LOW: /* Cable attached */ > + fusb302_set_state(data, ATTACHED_SRC); > + break; > + case TIMEOUT: > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_attached_src_event(struct fusb302_data *data, > + int event) > +{ > + switch (event) { > + case COMP_HIGH: /* Cable detached */ > + fusb302_set_state_disabled(data); > + break; > + } > +} > + > +static void fusb302_handle_event(struct fusb302_data *data, int event) > +{ > + > + mutex_lock(&data->lock); > + > + dev_dbg(data->dev, "Handling state %s event %s status %02x %02x\n", > + fusb302_state_str[data->state], fusb302_event_str[event], > + fusb302_read_reg(data, REG_STATUS0), > + fusb302_read_reg(data, REG_STATUS1A)); > + > + switch (data->state) { > + case DISABLED_SNK: > + fusb302_handle_disabled_snk_event(data, event); > + break; > + case DISABLED_SRC: > + fusb302_handle_disabled_src_event(data, event); > + break; > + case DISABLED_DRP: > + fusb302_handle_disabled_drp_event(data, event); > + break; > + case UNATTACHED_SNK: > + fusb302_handle_unattached_snk_event(data, event); > + break; > + case ATTACHED_SNK: > + fusb302_handle_attached_snk_event(data, event); > + break; > + case UNATTACHED_SRC: > + fusb302_handle_unattached_src_event(data, event); > + break; > + case ATTACHED_SRC: > + fusb302_handle_attached_src_event(data, event); > + break; > + } > + mutex_unlock(&data->lock); > +} > + > +static void fusb302_timeout(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, timeout.work); > + > + fusb302_handle_event(data, TIMEOUT); > +} > + > +static void fusb302_fallback_timeout(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, fallback_timeout.work); > + > + fusb302_handle_event(data, FALLBACK_TIMEOUT); > +} > + > +static void fusb302_report_bc_level(struct work_struct *work) > +{ > + struct fusb302_data *data = > + container_of(work, struct fusb302_data, bc_work.work); > + > + /* Clear old charger cable id */ > + fusb302_set_extcon_state(data, data->snk_cable_id, false); > + > + if (data->status0 & STATUS0_COMP) { > + dev_warn(data->dev, "vRd over maximum, assuming 500mA source\n"); > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > + } else { > + switch (data->status0 & STATUS0_BC_LEVEL_MASK) { > + case STATUS0_BC_LEVEL_VRA: > + case STATUS0_BC_LEVEL_VRDUSB: > + data->snk_cable_id = EXTCON_CHG_USB_SDP; > + break; > + case STATUS0_BC_LEVEL_VRD15: > + data->snk_cable_id = EXTCON_CHG_USB_CDP; > + break; > + case STATUS0_BC_LEVEL_VRD30: > + /* Use CHG_USB_FAST to indicate 3A capability */ > + data->snk_cable_id = EXTCON_CHG_USB_FAST; > + break; > + } > + } > + fusb302_set_extcon_state(data, data->snk_cable_id, true); > +} > + > +static irqreturn_t fusb302_irq_handler_thread(int irq, void *handler_data) > +{ > + struct fusb302_data *data = handler_data; > + int interrupt, interrupta; > + > + interrupt = fusb302_read_reg(data, REG_INTERRUPT); > + interrupta = fusb302_read_reg(data, REG_INTERRUPTA); > + > + if (interrupt) > + data->status0 = fusb302_read_reg(data, REG_STATUS0); > + > + if (interrupta) > + data->status1a = fusb302_read_reg(data, REG_STATUS1A); > + > + dev_dbg(data->dev, "Handling interrupt %02x %02x status %02x %02x\n", > + interrupt, interrupta, data->status0, data->status1a); > + > + if (interrupt & INTERRUPT_BC_LVL) > + fusb302_handle_event(data, BC_LVL_CHANGE); > + > + if (interrupt & INTERRUPT_COMP_CHNG) { > + if (data->status0 & STATUS0_COMP) > + fusb302_handle_event(data, COMP_HIGH); > + else > + fusb302_handle_event(data, COMP_LOW); > + } > + > + if (interrupt & INTERRUPT_VBUSOK) { > + if (data->status0 & STATUS0_VBUSOK) > + fusb302_handle_event(data, VBUS_VALID); > + else > + fusb302_handle_event(data, VBUS_INVALID); > + } > + > + if (interrupta & INTERRUPTA_TOGDONE) > + fusb302_handle_event(data, TOGGLE_DONE); > + > + return IRQ_HANDLED; > +} > + > +static const struct regmap_config fusb302_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .val_format_endian = REGMAP_ENDIAN_NATIVE, > +}; > + > +static int fusb302_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct fusb302_data *data; > + int ret; > + > + if (!client->irq) { > + dev_err(dev, "Error irq not specified\n"); > + return -EINVAL; > + } > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->dev = dev; > + /* TODO make these 2 configurable using device-properties */ > + data->port_type = TYPEC_PORT_DRP; > + data->preferred_role = TYPEC_SINK; > + > + data->regmap = devm_regmap_init_i2c(client, &fusb302_regmap_config); > + if (IS_ERR(data->regmap)) { > + ret = PTR_ERR(data->regmap); > + dev_err(dev, "Error to initializing regmap: %d\n", ret); > + return ret; > + } > + > + mutex_init(&data->lock); > + INIT_DELAYED_WORK(&data->timeout, fusb302_timeout); > + INIT_DELAYED_WORK(&data->fallback_timeout, fusb302_fallback_timeout); > + INIT_DELAYED_WORK(&data->bc_work, fusb302_report_bc_level); > + > + data->edev = devm_extcon_dev_allocate(dev, fusb302_extcon_cables); > + if (IS_ERR(data->edev)) > + return PTR_ERR(data->edev); > + > + data->edev->name = "fusb302"; > + > + ret = devm_extcon_dev_register(dev, data->edev); > + if (ret) > + return ret; > + > + extcon_set_property_capability(data->edev, EXTCON_USB, > + EXTCON_PROP_USB_TYPEC_POLARITY); > + > + fusb302_write_reg(data, REG_RESET, RESET_SW_RESET); > + usleep_range(10000, 20000); > + > + /* Enable power-saving */ > + fusb302_update_bits(data, REG_CONTROL2, CONTROL2_SAVE_PWR_MASK, > + CONTROL2_SAVE_PWR_40MS); > + > + fusb302_set_state_disabled(data); > + > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > + fusb302_irq_handler_thread, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, "fusb302", data); > + if (ret) { > + dev_err(dev, "Error requesting irq: %d\n", ret); > + return ret; > + } > + > + fusb302_update_bits(data, REG_CONTROL0, CONTROL0_INT_MASK, 0); > + > + i2c_set_clientdata(client, data); > + return 0; > +} > + > +static int fusb302_remove(struct i2c_client *client) > +{ > + struct fusb302_data *data = i2c_get_clientdata(client); > + > + devm_free_irq(data->dev, client->irq, data); > + cancel_delayed_work_sync(&data->timeout); > + cancel_delayed_work_sync(&data->fallback_timeout); > + > + return 0; > +} > + > +static const struct i2c_device_id fusb302_i2c_ids[] = { > + { "fusb302" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, fusb302_i2c_ids); > + > +static struct i2c_driver fusb302_driver = { > + .probe_new = fusb302_probe, > + .remove = fusb302_remove, > + .id_table = fusb302_i2c_ids, > + .driver = { > + .name = "fusb302", > + }, > +}; > +module_i2c_driver(fusb302_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > +MODULE_DESCRIPTION("FUSB302 USB TYPE-C controller Driver"); > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support 2017-05-23 9:27 ` Chanwoo Choi @ 2017-05-24 12:23 ` Andy Shevchenko 0 siblings, 0 replies; 26+ messages in thread From: Andy Shevchenko @ 2017-05-24 12:23 UTC (permalink / raw) To: Chanwoo Choi; +Cc: Hans de Goede, MyungJoo Ham, linux-kernel@vger.kernel.org On Tue, May 23, 2017 at 12:27 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote: > Hi Hans, > > I'm sorry for late reply. > I'll check this thread and then give you my opinion. Just don't forget to Cc your mails to USB Type-C people (Heikki et al). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/5] extcon: intel-cht-wc: Default to SDP on regmap io errors 2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede @ 2017-04-21 13:01 ` Hans de Goede 2017-04-21 13:01 ` [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper Hans de Goede ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Hans de Goede @ 2017-04-21 13:01 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel In the theoretical case of regmap_read failing get_charger would return an error and we would continue with EXTCON_NONE. Since we've already seen Vbus at this point, we should really set some other cable value to avoid other drivers enabling the 5V boost converter on the OTG port, even though there is an external Vbus. This commit modifies cht_wc_extcon_get_charger() to always return a valid cable, falling back to EXTCON_CHG_USB_SDP in case of regmap_read errors. This also removes the need for error-checking from callers of cht_wc_extcon_get_charger(). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-intel-cht-wc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c index 91a0023..8806427e 100644 --- a/drivers/extcon/extcon-intel-cht-wc.c +++ b/drivers/extcon/extcon-intel-cht-wc.c @@ -125,7 +125,7 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext, ret = regmap_read(ext->regmap, CHT_WC_USBSRC, &usbsrc); if (ret) { dev_err(ext->dev, "Error reading usbsrc: %d\n", ret); - return ret; + return EXTCON_CHG_USB_SDP; /* Save fallback */ } status = usbsrc & CHT_WC_USBSRC_STS_MASK; @@ -230,9 +230,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) goto set_state; } - ret = cht_wc_extcon_get_charger(ext, ignore_get_charger_errors); - if (ret >= 0) - cable = ret; + cable = cht_wc_extcon_get_charger(ext, ignore_get_charger_errors); charger_det_done: /* Route D+ and D- to SoC for the host or gadget controller */ -- 2.9.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper 2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede 2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede 2017-04-21 13:01 ` [PATCH 3/5] extcon: intel-cht-wc: Default to SDP on regmap io errors Hans de Goede @ 2017-04-21 13:01 ` Hans de Goede 2017-04-21 18:54 ` Andy Shevchenko 2017-04-21 13:01 ` [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller Hans de Goede 2017-05-23 9:26 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Chanwoo Choi 4 siblings, 1 reply; 26+ messages in thread From: Hans de Goede @ 2017-04-21 13:01 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel This allows a nice cleanup of cht_wc_extcon_pwrsrc_event, getting rid of all the gotos in there. This also is a preparation patch for adding USB Type-C controller monitoring. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-intel-cht-wc.c | 54 ++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c index 8806427e..684f6b2 100644 --- a/drivers/extcon/extcon-intel-cht-wc.c +++ b/drivers/extcon/extcon-intel-cht-wc.c @@ -196,20 +196,40 @@ static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext, } /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ -static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, - unsigned int cable, bool state) +static void cht_wc_extcon_set_cable_state(struct cht_wc_extcon_data *ext, + unsigned int cable, bool state) { extcon_set_state_sync(ext->edev, cable, state); if (cable == EXTCON_CHG_USB_SDP) extcon_set_state_sync(ext->edev, EXTCON_USB, state); } +static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, + unsigned int new_cable) +{ + if (new_cable == EXTCON_NONE && !ext->usb_host) { + /* Route D+ and D- to PMIC for future charger detection */ + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); + } else { + /* Route D+ and D- to SoC for the host or gadget controller */ + cht_wc_extcon_set_phymux(ext, MUX_SEL_SOC); + } + + if (new_cable != ext->previous_cable) { + cht_wc_extcon_set_cable_state(ext, new_cable, true); + cht_wc_extcon_set_cable_state(ext, ext->previous_cable, false); + ext->previous_cable = new_cable; + } + + extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); +} + static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) { int ret, pwrsrc_sts, id; unsigned int cable = EXTCON_NONE; /* Ignore errors in host mode, as the 5v boost converter is on then */ - bool ignore_get_charger_errors = ext->usb_host; + bool ignore_get_charger_err = ext->usb_host; ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts); if (ret) { @@ -218,33 +238,13 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) } id = cht_wc_extcon_get_id(ext, pwrsrc_sts); - if (id == USB_ID_GND) { - /* The 5v boost causes a false VBUS / SDP detect, skip */ - goto charger_det_done; - } - - /* Plugged into a host/charger or not connected? */ - if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) { - /* Route D+ and D- to PMIC for future charger detection */ - cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); - goto set_state; - } - - cable = cht_wc_extcon_get_charger(ext, ignore_get_charger_errors); -charger_det_done: - /* Route D+ and D- to SoC for the host or gadget controller */ - cht_wc_extcon_set_phymux(ext, MUX_SEL_SOC); - -set_state: - if (cable != ext->previous_cable) { - cht_wc_extcon_set_state(ext, cable, true); - cht_wc_extcon_set_state(ext, ext->previous_cable, false); - ext->previous_cable = cable; - } + /* When id == gnd the 5v boost causes a false VBUS detect */ + if (id != USB_ID_GND && (pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) + cable = cht_wc_extcon_get_charger(ext, ignore_get_charger_err); ext->usb_host = ((id == USB_ID_GND) || (id == USB_RID_A)); - extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); + cht_wc_extcon_set_state(ext, cable); } static irqreturn_t cht_wc_extcon_isr(int irq, void *data) -- 2.9.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper 2017-04-21 13:01 ` [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper Hans de Goede @ 2017-04-21 18:54 ` Andy Shevchenko 0 siblings, 0 replies; 26+ messages in thread From: Andy Shevchenko @ 2017-04-21 18:54 UTC (permalink / raw) To: Hans de Goede, Krogerus, Heikki Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org +Cc: Heikki. It looks like you better Cc him with entire series. On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: > This allows a nice cleanup of cht_wc_extcon_pwrsrc_event, getting rid > of all the gotos in there. > > This also is a preparation patch for adding USB Type-C controller > monitoring. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-intel-cht-wc.c | 54 ++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c > index 8806427e..684f6b2 100644 > --- a/drivers/extcon/extcon-intel-cht-wc.c > +++ b/drivers/extcon/extcon-intel-cht-wc.c > @@ -196,20 +196,40 @@ static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext, > } > > /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ > -static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, > - unsigned int cable, bool state) > +static void cht_wc_extcon_set_cable_state(struct cht_wc_extcon_data *ext, > + unsigned int cable, bool state) > { > extcon_set_state_sync(ext->edev, cable, state); > if (cable == EXTCON_CHG_USB_SDP) > extcon_set_state_sync(ext->edev, EXTCON_USB, state); > } > > +static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, > + unsigned int new_cable) > +{ > + if (new_cable == EXTCON_NONE && !ext->usb_host) { > + /* Route D+ and D- to PMIC for future charger detection */ > + cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); > + } else { > + /* Route D+ and D- to SoC for the host or gadget controller */ > + cht_wc_extcon_set_phymux(ext, MUX_SEL_SOC); > + } > + > + if (new_cable != ext->previous_cable) { > + cht_wc_extcon_set_cable_state(ext, new_cable, true); > + cht_wc_extcon_set_cable_state(ext, ext->previous_cable, false); > + ext->previous_cable = new_cable; > + } > + > + extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); > +} > + > static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) > { > int ret, pwrsrc_sts, id; > unsigned int cable = EXTCON_NONE; > /* Ignore errors in host mode, as the 5v boost converter is on then */ > - bool ignore_get_charger_errors = ext->usb_host; > + bool ignore_get_charger_err = ext->usb_host; > > ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts); > if (ret) { > @@ -218,33 +238,13 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) > } > > id = cht_wc_extcon_get_id(ext, pwrsrc_sts); > - if (id == USB_ID_GND) { > - /* The 5v boost causes a false VBUS / SDP detect, skip */ > - goto charger_det_done; > - } > - > - /* Plugged into a host/charger or not connected? */ > - if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) { > - /* Route D+ and D- to PMIC for future charger detection */ > - cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); > - goto set_state; > - } > - > - cable = cht_wc_extcon_get_charger(ext, ignore_get_charger_errors); > > -charger_det_done: > - /* Route D+ and D- to SoC for the host or gadget controller */ > - cht_wc_extcon_set_phymux(ext, MUX_SEL_SOC); > - > -set_state: > - if (cable != ext->previous_cable) { > - cht_wc_extcon_set_state(ext, cable, true); > - cht_wc_extcon_set_state(ext, ext->previous_cable, false); > - ext->previous_cable = cable; > - } > + /* When id == gnd the 5v boost causes a false VBUS detect */ > + if (id != USB_ID_GND && (pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) > + cable = cht_wc_extcon_get_charger(ext, ignore_get_charger_err); > > ext->usb_host = ((id == USB_ID_GND) || (id == USB_RID_A)); > - extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); > + cht_wc_extcon_set_state(ext, cable); > } > > static irqreturn_t cht_wc_extcon_isr(int irq, void *data) > -- > 2.9.3 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller 2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede ` (2 preceding siblings ...) 2017-04-21 13:01 ` [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper Hans de Goede @ 2017-04-21 13:01 ` Hans de Goede 2017-04-21 18:55 ` Andy Shevchenko 2017-04-24 14:25 ` Heikki Krogerus 2017-05-23 9:26 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Chanwoo Choi 4 siblings, 2 replies; 26+ messages in thread From: Hans de Goede @ 2017-04-21 13:01 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: Hans de Goede, linux-kernel On some boards the Whiskey Cove PMIC is combined with an external USB Type-C controller, in this case extcon consumers should use the Type-C extcon state, except when the USB Type-C controller detects a current limit of 500 mA which may indicate USB-C to USB-A cable at which point the extcon consumer should use the Whiskey Cove's BC-1.2 detection's state. Since the Type-C controller info is incomplete and needs to be supplemented with the BC1.2 detection result in some cases, this commit makes the intel-cht-wc extcon driver monitor the extcon device registered by the Type-C controller and report that as our extcon state except when BC-1.2 detection should be used. This allows extcon consumers on these boards to keep monitoring only the intel-cht-wc extcon and then get complete extcon info from that, which combines the Type-C and BC-1.2 info. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/extcon-intel-cht-wc.c | 96 ++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 16 deletions(-) diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c index 684f6b2..e510188 100644 --- a/drivers/extcon/extcon-intel-cht-wc.c +++ b/drivers/extcon/extcon-intel-cht-wc.c @@ -15,6 +15,7 @@ * more details. */ +#include <linux/acpi.h> #include <linux/extcon.h> #include <linux/interrupt.h> #include <linux/kernel.h> @@ -88,6 +89,7 @@ static const unsigned int cht_wc_extcon_cables[] = { EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_ACA, + EXTCON_CHG_USB_FAST, EXTCON_NONE, }; @@ -95,6 +97,9 @@ struct cht_wc_extcon_data { struct device *dev; struct regmap *regmap; struct extcon_dev *edev; + struct extcon_dev *usbc; + struct notifier_block usbc_nb; + struct work_struct work; unsigned int previous_cable; bool usb_host; }; @@ -224,8 +229,32 @@ static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); } -static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) +static void cht_wc_extcon_usb_c_event(struct work_struct *work) { + struct cht_wc_extcon_data *ext = + container_of(work, struct cht_wc_extcon_data, work); + unsigned int cable = EXTCON_NONE; + + if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_SDP) > 0) { + /* + * USB-C to USB-A cable ? Try BC1.2 charger detection from + * WC PMIC, ignore charger detection errors. + */ + cable = cht_wc_extcon_get_charger(ext, true); + } else if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_CDP) > 0) { + cable = EXTCON_CHG_USB_CDP; + } else if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_FAST) > 0) { + cable = EXTCON_CHG_USB_FAST; + } + + ext->usb_host = (extcon_get_state(ext->usbc, EXTCON_USB_HOST) > 0); + cht_wc_extcon_set_state(ext, cable); +} + +static void cht_wc_extcon_pwrsrc_event(struct work_struct *work) +{ + struct cht_wc_extcon_data *ext = + container_of(work, struct cht_wc_extcon_data, work); int ret, pwrsrc_sts, id; unsigned int cable = EXTCON_NONE; /* Ignore errors in host mode, as the 5v boost converter is on then */ @@ -258,7 +287,7 @@ static irqreturn_t cht_wc_extcon_isr(int irq, void *data) return IRQ_NONE; } - cht_wc_extcon_pwrsrc_event(ext); + schedule_work(&ext->work); ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ, irqs); if (ret) { @@ -269,6 +298,17 @@ static irqreturn_t cht_wc_extcon_isr(int irq, void *data) return IRQ_HANDLED; } +static int cht_wc_extcon_usbc_evt(struct notifier_block *nb, + unsigned long event, void *param) +{ + struct cht_wc_extcon_data *ext = + container_of(nb, struct cht_wc_extcon_data, usbc_nb); + + schedule_work(&ext->work); + + return NOTIFY_OK; +} + static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable) { int ret, mask, val; @@ -300,6 +340,15 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) ext->regmap = pmic->regmap; ext->previous_cable = EXTCON_NONE; + if (acpi_dev_present("INT33FE", NULL, -1)) { + ext->usbc = extcon_get_extcon_dev("fusb302"); + if (!ext->usbc) + return -EPROBE_DEFER; + + dev_info(&pdev->dev, + "Using FUSB302 extcon for USB Type-C cable info\n"); + } + /* Initialize extcon device */ ext->edev = devm_extcon_dev_allocate(ext->dev, cht_wc_extcon_cables); if (IS_ERR(ext->edev)) @@ -335,25 +384,40 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) /* Route D+ and D- to PMIC for initial charger detection */ cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); - /* Get initial state */ - cht_wc_extcon_pwrsrc_event(ext); - - ret = devm_request_threaded_irq(ext->dev, irq, NULL, cht_wc_extcon_isr, - IRQF_ONESHOT, pdev->name, ext); - if (ret) { - dev_err(ext->dev, "Error requesting interrupt: %d\n", ret); - goto disable_sw_control; - } + if (ext->usbc) { + INIT_WORK(&ext->work, cht_wc_extcon_usb_c_event); + ext->usbc_nb.notifier_call = cht_wc_extcon_usbc_evt; + ret = devm_extcon_register_notifier_all(&pdev->dev, ext->usbc, + &ext->usbc_nb); + if (ret) { + dev_err(&pdev->dev, + "Error registering usbc extcon notifier: %d\n", + ret); + goto disable_sw_control; + } + } else { + INIT_WORK(&ext->work, cht_wc_extcon_pwrsrc_event); + ret = devm_request_threaded_irq(ext->dev, irq, NULL, + cht_wc_extcon_isr, IRQF_ONESHOT, + pdev->name, ext); + if (ret) { + dev_err(ext->dev, "Error requesting irq: %d\n", ret); + goto disable_sw_control; + } - /* Unmask irqs */ - ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK, + /* Unmask irqs */ + ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK, (int)~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_ID_GND | CHT_WC_PWRSRC_ID_FLOAT)); - if (ret) { - dev_err(ext->dev, "Error writing irq-mask: %d\n", ret); - goto disable_sw_control; + if (ret) { + dev_err(ext->dev, "Error writing irq-mask: %d\n", ret); + goto disable_sw_control; + } } + /* Get initial state */ + schedule_work(&ext->work); + platform_set_drvdata(pdev, ext); return 0; -- 2.9.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller 2017-04-21 13:01 ` [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller Hans de Goede @ 2017-04-21 18:55 ` Andy Shevchenko 2017-04-24 14:25 ` Heikki Krogerus 1 sibling, 0 replies; 26+ messages in thread From: Andy Shevchenko @ 2017-04-21 18:55 UTC (permalink / raw) To: Hans de Goede, Krogerus, Heikki Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel@vger.kernel.org +Cc: Heikki, On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On some boards the Whiskey Cove PMIC is combined with an external USB > Type-C controller, in this case extcon consumers should use the Type-C > extcon state, except when the USB Type-C controller detects a current > limit of 500 mA which may indicate USB-C to USB-A cable at which point > the extcon consumer should use the Whiskey Cove's BC-1.2 detection's > state. > > Since the Type-C controller info is incomplete and needs to be > supplemented with the BC1.2 detection result in some cases, this > commit makes the intel-cht-wc extcon driver monitor the extcon device > registered by the Type-C controller and report that as our extcon state > except when BC-1.2 detection should be used. This allows extcon > consumers on these boards to keep monitoring only the intel-cht-wc extcon > and then get complete extcon info from that, which combines the Type-C > and BC-1.2 info. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-intel-cht-wc.c | 96 ++++++++++++++++++++++++++++++------ > 1 file changed, 80 insertions(+), 16 deletions(-) > > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c > index 684f6b2..e510188 100644 > --- a/drivers/extcon/extcon-intel-cht-wc.c > +++ b/drivers/extcon/extcon-intel-cht-wc.c > @@ -15,6 +15,7 @@ > * more details. > */ > > +#include <linux/acpi.h> > #include <linux/extcon.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > @@ -88,6 +89,7 @@ static const unsigned int cht_wc_extcon_cables[] = { > EXTCON_CHG_USB_CDP, > EXTCON_CHG_USB_DCP, > EXTCON_CHG_USB_ACA, > + EXTCON_CHG_USB_FAST, > EXTCON_NONE, > }; > > @@ -95,6 +97,9 @@ struct cht_wc_extcon_data { > struct device *dev; > struct regmap *regmap; > struct extcon_dev *edev; > + struct extcon_dev *usbc; > + struct notifier_block usbc_nb; > + struct work_struct work; > unsigned int previous_cable; > bool usb_host; > }; > @@ -224,8 +229,32 @@ static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, > extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); > } > > -static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) > +static void cht_wc_extcon_usb_c_event(struct work_struct *work) > { > + struct cht_wc_extcon_data *ext = > + container_of(work, struct cht_wc_extcon_data, work); > + unsigned int cable = EXTCON_NONE; > + > + if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_SDP) > 0) { > + /* > + * USB-C to USB-A cable ? Try BC1.2 charger detection from > + * WC PMIC, ignore charger detection errors. > + */ > + cable = cht_wc_extcon_get_charger(ext, true); > + } else if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_CDP) > 0) { > + cable = EXTCON_CHG_USB_CDP; > + } else if (extcon_get_state(ext->usbc, EXTCON_CHG_USB_FAST) > 0) { > + cable = EXTCON_CHG_USB_FAST; > + } > + > + ext->usb_host = (extcon_get_state(ext->usbc, EXTCON_USB_HOST) > 0); > + cht_wc_extcon_set_state(ext, cable); > +} > + > +static void cht_wc_extcon_pwrsrc_event(struct work_struct *work) > +{ > + struct cht_wc_extcon_data *ext = > + container_of(work, struct cht_wc_extcon_data, work); > int ret, pwrsrc_sts, id; > unsigned int cable = EXTCON_NONE; > /* Ignore errors in host mode, as the 5v boost converter is on then */ > @@ -258,7 +287,7 @@ static irqreturn_t cht_wc_extcon_isr(int irq, void *data) > return IRQ_NONE; > } > > - cht_wc_extcon_pwrsrc_event(ext); > + schedule_work(&ext->work); > > ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ, irqs); > if (ret) { > @@ -269,6 +298,17 @@ static irqreturn_t cht_wc_extcon_isr(int irq, void *data) > return IRQ_HANDLED; > } > > +static int cht_wc_extcon_usbc_evt(struct notifier_block *nb, > + unsigned long event, void *param) > +{ > + struct cht_wc_extcon_data *ext = > + container_of(nb, struct cht_wc_extcon_data, usbc_nb); > + > + schedule_work(&ext->work); > + > + return NOTIFY_OK; > +} > + > static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable) > { > int ret, mask, val; > @@ -300,6 +340,15 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) > ext->regmap = pmic->regmap; > ext->previous_cable = EXTCON_NONE; > > + if (acpi_dev_present("INT33FE", NULL, -1)) { > + ext->usbc = extcon_get_extcon_dev("fusb302"); > + if (!ext->usbc) > + return -EPROBE_DEFER; > + > + dev_info(&pdev->dev, > + "Using FUSB302 extcon for USB Type-C cable info\n"); > + } > + > /* Initialize extcon device */ > ext->edev = devm_extcon_dev_allocate(ext->dev, cht_wc_extcon_cables); > if (IS_ERR(ext->edev)) > @@ -335,25 +384,40 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) > /* Route D+ and D- to PMIC for initial charger detection */ > cht_wc_extcon_set_phymux(ext, MUX_SEL_PMIC); > > - /* Get initial state */ > - cht_wc_extcon_pwrsrc_event(ext); > - > - ret = devm_request_threaded_irq(ext->dev, irq, NULL, cht_wc_extcon_isr, > - IRQF_ONESHOT, pdev->name, ext); > - if (ret) { > - dev_err(ext->dev, "Error requesting interrupt: %d\n", ret); > - goto disable_sw_control; > - } > + if (ext->usbc) { > + INIT_WORK(&ext->work, cht_wc_extcon_usb_c_event); > + ext->usbc_nb.notifier_call = cht_wc_extcon_usbc_evt; > + ret = devm_extcon_register_notifier_all(&pdev->dev, ext->usbc, > + &ext->usbc_nb); > + if (ret) { > + dev_err(&pdev->dev, > + "Error registering usbc extcon notifier: %d\n", > + ret); > + goto disable_sw_control; > + } > + } else { > + INIT_WORK(&ext->work, cht_wc_extcon_pwrsrc_event); > + ret = devm_request_threaded_irq(ext->dev, irq, NULL, > + cht_wc_extcon_isr, IRQF_ONESHOT, > + pdev->name, ext); > + if (ret) { > + dev_err(ext->dev, "Error requesting irq: %d\n", ret); > + goto disable_sw_control; > + } > > - /* Unmask irqs */ > - ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK, > + /* Unmask irqs */ > + ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK, > (int)~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_ID_GND | > CHT_WC_PWRSRC_ID_FLOAT)); > - if (ret) { > - dev_err(ext->dev, "Error writing irq-mask: %d\n", ret); > - goto disable_sw_control; > + if (ret) { > + dev_err(ext->dev, "Error writing irq-mask: %d\n", ret); > + goto disable_sw_control; > + } > } > > + /* Get initial state */ > + schedule_work(&ext->work); > + > platform_set_drvdata(pdev, ext); > > return 0; > -- > 2.9.3 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller 2017-04-21 13:01 ` [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller Hans de Goede 2017-04-21 18:55 ` Andy Shevchenko @ 2017-04-24 14:25 ` Heikki Krogerus 1 sibling, 0 replies; 26+ messages in thread From: Heikki Krogerus @ 2017-04-24 14:25 UTC (permalink / raw) To: Hans de Goede; +Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel Hi Hans, On Fri, Apr 21, 2017 at 03:01:19PM +0200, Hans de Goede wrote: > On some boards the Whiskey Cove PMIC is combined with an external USB > Type-C controller, in this case extcon consumers should use the Type-C > extcon state, except when the USB Type-C controller detects a current > limit of 500 mA which may indicate USB-C to USB-A cable at which point > the extcon consumer should use the Whiskey Cove's BC-1.2 detection's > state. Doesn't this mean you have several extcon_dev registered for a single port? I'm not completely sure how extcon framework is meant to be used, but shouldn't a single extcon_dev represent all the types of connectors a port is meant to support? > Since the Type-C controller info is incomplete and needs to be > supplemented with the BC1.2 detection result in some cases, this > commit makes the intel-cht-wc extcon driver monitor the extcon device > registered by the Type-C controller and report that as our extcon state > except when BC-1.2 detection should be used. This allows extcon > consumers on these boards to keep monitoring only the intel-cht-wc extcon > and then get complete extcon info from that, which combines the Type-C > and BC-1.2 info. I don't really understand why does this need to be done in such a complex manner? Both components should just report the result and be done with it, and there is no need for any coupling between the two. If there is a consumer for the power, the driver for it can decide on its own the limits and maximum power from the two sources. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon-intel-cht-wc.c | 96 ++++++++++++++++++++++++++++++------ I did not realize we had this driver. It really should register a psy. We need to be able to advertise the results of the detection to the consumers in order to support for example boards that have a discrete charger ic or battery attached to the SoC instead of PMIC, and without being forced to use board/platform specific quirks like this in the driver. This driver should just report the result of the detection forward and let the psy framework take care of notifying the other components, consumers and whatnot, if there are any. Thanks, -- heikki ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name 2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede ` (3 preceding siblings ...) 2017-04-21 13:01 ` [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller Hans de Goede @ 2017-05-23 9:26 ` Chanwoo Choi 4 siblings, 0 replies; 26+ messages in thread From: Chanwoo Choi @ 2017-05-23 9:26 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel Hi Hans, I'm sorry for late reply. On 2017년 04월 21일 22:01, Hans de Goede wrote: > The parent device name is not necessarily always useful, e.g. > with i2c devices it may simply be e.g.: "0-0022" and it also depends > on the i2c-bus number which depends on probe ordering. > > This commit allows drivers to set their own, more useful name, > avoiding the problems with some i2c-device names. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/extcon/extcon.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index f422a78..f8d3c1b 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -1117,7 +1117,8 @@ int extcon_dev_register(struct extcon_dev *edev) > edev->dev.class = extcon_class; > edev->dev.release = extcon_dev_release; > > - edev->name = dev_name(edev->dev.parent); > + if (!edev->name) > + edev->name = dev_name(edev->dev.parent); I think that the variables of 'struct extcon_dev' should be modified by extcon core. I don't want to touch the variables of 'struct extcon_dev' by extcon provider driver. > if (IS_ERR_OR_NULL(edev->name)) { > dev_err(&edev->dev, > "extcon device name is null\n"); > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-05-24 12:24 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170421130123epcas4p12e891d305aa2b4126089e691d15c6659@epcas4p1.samsung.com>
2017-04-21 13:01 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Hans de Goede
2017-04-21 13:01 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede
2017-04-21 18:51 ` Andy Shevchenko
2017-04-24 11:02 ` Heikki Krogerus
2017-04-24 13:12 ` Guenter Roeck
2017-05-12 21:22 ` Hans de Goede
2017-05-16 12:07 ` Heikki Krogerus
2017-05-16 22:24 ` Hans de Goede
2017-05-17 11:45 ` Heikki Krogerus
2017-05-17 14:47 ` USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede
2017-05-18 8:37 ` Heikki Krogerus
2017-05-18 11:50 ` Heikki Krogerus
2017-05-19 20:12 ` Hans de Goede
2017-05-19 20:15 ` Hans de Goede
2017-05-22 14:56 ` Heikki Krogerus
2017-05-16 22:52 ` [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Guenter Roeck
2017-04-21 23:23 ` kbuild test robot
2017-05-23 9:27 ` Chanwoo Choi
2017-05-24 12:23 ` Andy Shevchenko
2017-04-21 13:01 ` [PATCH 3/5] extcon: intel-cht-wc: Default to SDP on regmap io errors Hans de Goede
2017-04-21 13:01 ` [PATCH 4/5] extcon: intel-cht-wc: Add new cht_wc_extcon_set_state helper Hans de Goede
2017-04-21 18:54 ` Andy Shevchenko
2017-04-21 13:01 ` [PATCH 5/5] extcon: intel-cht-wc: Add support for monitoring external USB Type-C controller Hans de Goede
2017-04-21 18:55 ` Andy Shevchenko
2017-04-24 14:25 ` Heikki Krogerus
2017-05-23 9:26 ` [PATCH 1/5] extcon: Allow extcon drivers to specify the extcon name Chanwoo Choi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox