| Message ID | 20211114170335.66994-18-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power-suppy/i2c/extcon: Fix charger setup on Xiaomi Mi Pad 2 and Lenovo Yogabook | expand |
On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote: > > So far the extcon-intel-cht-wc code has only been tested on devices with > a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support > through a FUSB302 Type-C controller. > > Some devices with the intel-cht-wc PMIC however come with an USB-micro-B > connector, or an USB-2 only Type-C connector without USB-PD. > > Which device-model we are running on can be identified with the new > intel_cht_wc_get_model() helper and on models without a Type-C controller > the extcon code must control the Vbus 5V boost converter and the USB role > switch depending on the detected cable-type. ... > config EXTCON_INTEL_CHT_WC > tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver" > - depends on INTEL_SOC_PMIC_CHTWC > + depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT Having these two in one expression sounds a bit alogical to me, can you just add a separate "depends on"? > + select USB_ROLE_SWITCH ... > + if (ext->vbus_boost && ext->vbus_boost_enabled != enable) { > + if (enable) > + ret = regulator_enable(ext->vbus_boost); > + else > + ret = regulator_disable(ext->vbus_boost); Redundant blank line here (but it's up to you) > + if (ret == 0) > + ext->vbus_boost_enabled = enable; > + else > + dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret); Why not a traditional pattern, i.e. error handling first? > + } ... > +/* Some boards require controlling the role-sw and vbus based on the id-pin */ Vbus ? VBUS? Here and there the inconsistency of some terms... ... > + ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus"); > + if (IS_ERR(ext->vbus_boost)) { > + ret = PTR_ERR(ext->vbus_boost); > + if (ret == -ENODEV) > + ret = -EPROBE_DEFER; > + > + return dev_err_probe(ext->dev, ret, "getting vbus regulator"); Can be also written as if (PTR_ERR(ext->vbus_boost) == -ENODEV || PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER) return dev_err_probe(ext->dev, -EPROBE_DEFER, "getting vbus regulator"); return PTR_ERR(ext->vbus_boost); but up to you. > + }
On Tue, Nov 16, 2021 at 1:28 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > + ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus"); > > + if (IS_ERR(ext->vbus_boost)) { > > + ret = PTR_ERR(ext->vbus_boost); > > + if (ret == -ENODEV) > > + ret = -EPROBE_DEFER; > > + > > + return dev_err_probe(ext->dev, ret, "getting vbus regulator"); > > Can be also written as > > if (PTR_ERR(ext->vbus_boost) == -ENODEV || > PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER) > return dev_err_probe(ext->dev, -EPROBE_DEFER, "getting vbus regulator"); > > return PTR_ERR(ext->vbus_boost); Oops, other way around, of course. if (PTR_ERR(ext->vbus_boost) == -ENODEV || PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER) return -EPROBE_DEFER; return dev_err_probe(ext->dev, PTR_ERR(ext->vbus_boost), "getting vbus regulator"); > but up to you. > > > + }
On Tue, Nov 16, 2021 at 1:31 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Nov 16, 2021 at 1:28 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Can be also written as > Oops, other way around, of course. Yeah, scratch it. We have nice debugfs which will be missed...
Hi, On 11/16/21 12:28, Andy Shevchenko wrote: > On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> So far the extcon-intel-cht-wc code has only been tested on devices with >> a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support >> through a FUSB302 Type-C controller. >> >> Some devices with the intel-cht-wc PMIC however come with an USB-micro-B >> connector, or an USB-2 only Type-C connector without USB-PD. >> >> Which device-model we are running on can be identified with the new >> intel_cht_wc_get_model() helper and on models without a Type-C controller >> the extcon code must control the Vbus 5V boost converter and the USB role >> switch depending on the detected cable-type. > > ... > >> config EXTCON_INTEL_CHT_WC >> tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver" >> - depends on INTEL_SOC_PMIC_CHTWC > >> + depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT > > Having these two in one expression sounds a bit alogical to me, can > you just add a separate "depends on"? Sure. > >> + select USB_ROLE_SWITCH > > ... > >> + if (ext->vbus_boost && ext->vbus_boost_enabled != enable) { >> + if (enable) >> + ret = regulator_enable(ext->vbus_boost); >> + else >> + ret = regulator_disable(ext->vbus_boost); > > Redundant blank line here (but it's up to you) > >> + if (ret == 0) >> + ext->vbus_boost_enabled = enable; >> + else >> + dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret); > > Why not a traditional pattern, i.e. error handling first? As I've mentioned before (to a very similar remark) error handling first is not the traditional pattern, at least not for me. Traditionally (to me) the else case is the error case. This is just how humans work. E.g. if I need help for something saying something like: "If you have time can you help me with this please? Otherwise I'm afraid that I am never going to solve this." Feels natural, where as saying it like this: "If you do not have time I'm afraid I am never going to solve this, otherwise can you help me with this please ?" Feels quite unnatural, at least to me. >> + } > > ... > >> +/* Some boards require controlling the role-sw and vbus based on the id-pin */ > > Vbus ? VBUS? Here and there the inconsistency of some terms... "Vbus", I'll try to fix this up everywhere. Regards, Hans
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index c69d40ae5619..fc733689cfad 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -60,7 +60,8 @@ config EXTCON_INTEL_INT3496 config EXTCON_INTEL_CHT_WC tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver" - depends on INTEL_SOC_PMIC_CHTWC + depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT + select USB_ROLE_SWITCH help Say Y here to enable extcon support for charger detection / control on the Intel Cherrytrail Whiskey Cove PMIC. diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c index a5aeeecc44fb..119b83793123 100644 --- a/drivers/extcon/extcon-intel-cht-wc.c +++ b/drivers/extcon/extcon-intel-cht-wc.c @@ -16,7 +16,9 @@ #include <linux/platform_device.h> #include <linux/property.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> +#include <linux/usb/role.h> #include "extcon-intel.h" @@ -102,8 +104,11 @@ struct cht_wc_extcon_data { struct device *dev; struct regmap *regmap; struct extcon_dev *edev; + struct usb_role_switch *role_sw; + struct regulator *vbus_boost; unsigned int previous_cable; bool usb_host; + bool vbus_boost_enabled; }; static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts) @@ -217,6 +222,18 @@ static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext, CHT_WC_CHGRCTRL1_OTGMODE, val); if (ret) dev_err(ext->dev, "Error updating CHGRCTRL1 reg: %d\n", ret); + + if (ext->vbus_boost && ext->vbus_boost_enabled != enable) { + if (enable) + ret = regulator_enable(ext->vbus_boost); + else + ret = regulator_disable(ext->vbus_boost); + + if (ret == 0) + ext->vbus_boost_enabled = enable; + else + dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret); + } } static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext, @@ -246,6 +263,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) 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; + enum usb_role role; ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts); if (ret) { @@ -289,6 +307,18 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext) ext->usb_host = ((id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A)); extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host); + + if (ext->usb_host) + role = USB_ROLE_HOST; + else if (pwrsrc_sts & CHT_WC_PWRSRC_VBUS) + role = USB_ROLE_DEVICE; + else + role = USB_ROLE_NONE; + + /* Note: this is a no-op when ext->role_sw is NULL */ + ret = usb_role_switch_set_role(ext->role_sw, role); + if (ret) + dev_err(ext->dev, "Error setting USB-role: %d\n", ret); } static irqreturn_t cht_wc_extcon_isr(int irq, void *data) @@ -334,6 +364,61 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable) return ret; } +static int cht_wc_extcon_find_role_sw(struct cht_wc_extcon_data *ext) +{ + const struct software_node *swnode; + struct fwnode_handle *fwnode; + + swnode = software_node_find_by_name(NULL, "intel-xhci-usb-sw"); + if (!swnode) + return -EPROBE_DEFER; + + fwnode = software_node_fwnode(swnode); + ext->role_sw = usb_role_switch_find_by_fwnode(fwnode); + fwnode_handle_put(fwnode); + + return ext->role_sw ? 0 : -EPROBE_DEFER; +} + +static void cht_wc_extcon_put_role_sw(void *data) +{ + struct cht_wc_extcon_data *ext = data; + + usb_role_switch_put(ext->role_sw); +} + +/* Some boards require controlling the role-sw and vbus based on the id-pin */ +static int cht_wc_extcon_get_role_sw_and_regulator(struct cht_wc_extcon_data *ext) +{ + int ret; + + ret = cht_wc_extcon_find_role_sw(ext); + if (ret) + return ret; + + ret = devm_add_action_or_reset(ext->dev, cht_wc_extcon_put_role_sw, ext); + if (ret) + return ret; + + /* + * On x86/ACPI platforms the regulator <-> consumer link is provided + * by platform_data passed to the regulator driver. This means that + * this info is not available before the regulator driver has bound. + * Use devm_regulator_get_optional() to avoid getting a dummy + * regulator and wait for the regulator to show up if necessary. + */ + ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus"); + if (IS_ERR(ext->vbus_boost)) { + ret = PTR_ERR(ext->vbus_boost); + if (ret == -ENODEV) + ret = -EPROBE_DEFER; + + return dev_err_probe(ext->dev, ret, "getting vbus regulator"); + } + + return 0; +} + static int cht_wc_extcon_probe(struct platform_device *pdev) { struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent); @@ -376,6 +461,12 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) */ cht_wc_extcon_set_5v_boost(ext, false); break; + case INTEL_CHT_WC_LENOVO_YOGABOOK1: + case INTEL_CHT_WC_XIAOMI_MIPAD2: + ret = cht_wc_extcon_get_role_sw_and_regulator(ext); + if (ret) + return ret; + break; default: break; }
So far the extcon-intel-cht-wc code has only been tested on devices with a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support through a FUSB302 Type-C controller. Some devices with the intel-cht-wc PMIC however come with an USB-micro-B connector, or an USB-2 only Type-C connector without USB-PD. Which device-model we are running on can be identified with the new intel_cht_wc_get_model() helper and on models without a Type-C controller the extcon code must control the Vbus 5V boost converter and the USB role switch depending on the detected cable-type. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/extcon/Kconfig | 3 +- drivers/extcon/extcon-intel-cht-wc.c | 91 ++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-)