Messages in this thread | | | Date | Mon, 27 Jul 2020 17:02:59 +0200 | From | Paul Cercueil <> | Subject | Re: [RESEND PATCH] usb: common: usb-conn-gpio: Register optional charger |
| |
Le lun. 27 juil. 2020 à 13:42, Chunfeng Yun <chunfeng.yun@mediatek.com> a écrit : > On Sun, 2020-07-26 at 12:27 +0200, Paul Cercueil wrote: >> >> Le dim. 26 juil. 2020 à 13:14, Andy Shevchenko >> <andy.shevchenko@gmail.com> a écrit : >> > On Mon, Jun 22, 2020 at 1:51 AM Paul Cercueil >> <paul@crapouillou.net> >> > wrote: >> >> >> >> Register a power supply charger, if the Kconfig option >> >> USB_CONN_GPIO_CHARGER is set, whose online state depends on >> whether >> >> the USB role is set to device or not. >> >> >> >> This is useful when the USB role is the only way to know if the >> >> device >> >> is charging from USB. The API is the standard power supply >> charger >> >> API, >> >> you get a /sys/class/power_supply/xxx/online node which tells >> you >> >> the >> >> state of the charger. >> >> >> >> The sole purpose of this is to give userspace applications a >> way to >> >> know whether or not the charger is plugged. >> > >> > I'm not sure I understand the purpose of this (third?) way to >> detect >> > USB charger and notify user space about. >> > Why is extcon not good enough? >> >> We can't have extcon and USB role detection at the same time. >> >> -Paul >> >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> >> --- >> >> drivers/usb/common/Kconfig | 11 +++++++ >> >> drivers/usb/common/usb-conn-gpio.c | 47 >> >> ++++++++++++++++++++++++++++++ >> >> 2 files changed, 58 insertions(+) >> >> >> >> diff --git a/drivers/usb/common/Kconfig >> b/drivers/usb/common/Kconfig >> >> index d611477aae41..5405ae96c68f 100644 >> >> --- a/drivers/usb/common/Kconfig >> >> +++ b/drivers/usb/common/Kconfig >> >> @@ -49,3 +49,14 @@ config USB_CONN_GPIO >> >> >> >> To compile the driver as a module, choose M here: the >> >> module will >> >> be called usb-conn-gpio.ko >> >> + >> >> +if USB_CONN_GPIO >> >> + >> >> +config USB_CONN_GPIO_CHARGER >> >> + bool "USB charger support" >> >> + select POWER_SUPPLY >> >> + help >> >> + Register a charger with the power supply subsystem. >> This >> >> will allow >> >> + userspace to know whether or not the device is >> charging >> >> from USB. >> >> + >> >> +endif >> >> diff --git a/drivers/usb/common/usb-conn-gpio.c >> >> b/drivers/usb/common/usb-conn-gpio.c >> >> index ed204cbb63ea..129d48db280b 100644 >> >> --- a/drivers/usb/common/usb-conn-gpio.c >> >> +++ b/drivers/usb/common/usb-conn-gpio.c >> >> @@ -17,6 +17,7 @@ >> >> #include <linux/of.h> >> >> #include <linux/pinctrl/consumer.h> >> >> #include <linux/platform_device.h> >> >> +#include <linux/power_supply.h> >> >> #include <linux/regulator/consumer.h> >> >> #include <linux/usb/role.h> >> >> >> >> @@ -38,6 +39,9 @@ struct usb_conn_info { >> >> struct gpio_desc *vbus_gpiod; >> >> int id_irq; >> >> int vbus_irq; >> >> + >> >> + struct power_supply_desc desc; >> >> + struct power_supply *charger; >> >> }; >> >> >> >> /** >> >> @@ -98,6 +102,8 @@ static void usb_conn_detect_cable(struct >> >> work_struct *work) >> >> ret = regulator_enable(info->vbus); >> >> if (ret) >> >> dev_err(info->dev, "enable vbus >> regulator >> >> failed\n"); >> >> + } else if (IS_ENABLED(CONFIG_USB_CONN_GPIO_CHARGER)) { >> >> + power_supply_changed(info->charger); >> >> } >> >> >> >> info->last_role = role; >> >> @@ -121,10 +127,35 @@ static irqreturn_t usb_conn_isr(int irq, >> void >> >> *dev_id) >> >> return IRQ_HANDLED; >> >> } >> >> >> >> +static enum power_supply_property usb_charger_properties[] = { >> >> + POWER_SUPPLY_PROP_ONLINE, >> >> +}; >> >> + >> >> +static int usb_charger_get_property(struct power_supply *psy, >> >> + enum power_supply_property >> psp, >> >> + union power_supply_propval >> *val) >> >> +{ >> >> + struct usb_conn_info *info = >> power_supply_get_drvdata(psy); >> >> + >> >> + switch (psp) { >> >> + case POWER_SUPPLY_PROP_ONLINE: >> >> + val->intval = info->last_role == >> USB_ROLE_DEVICE; > What will happen if you not change info->last_role here? > I prefer it's only changed by usb_conn_isr(), if it's changed by other > drivers, for example, through power_supply_get_property(), may skip > role > switch.
If you read carefully, info->last_role is not modified here :)
-Paul
> >> >> + break; >> >> + default: >> >> + return -EINVAL; >> >> + } >> >> + >> >> + return 0; >> >> +} >> >> + >> >> static int usb_conn_probe(struct platform_device *pdev) >> >> { >> >> struct device *dev = &pdev->dev; >> >> + struct power_supply_desc *desc; >> >> struct usb_conn_info *info; >> >> + struct power_supply_config cfg = { >> >> + .of_node = dev->of_node, >> >> + }; >> >> int ret = 0; >> >> >> >> info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); >> >> @@ -203,6 +234,22 @@ static int usb_conn_probe(struct >> >> platform_device *pdev) >> >> } >> >> } >> >> >> >> + if (IS_ENABLED(CONFIG_USB_CONN_GPIO_CHARGER)) { >> >> + desc = &info->desc; >> >> + desc->name = "usb-charger"; >> >> + desc->properties = usb_charger_properties; >> >> + desc->num_properties = >> >> ARRAY_SIZE(usb_charger_properties); >> >> + desc->get_property = usb_charger_get_property; >> >> + desc->type = POWER_SUPPLY_TYPE_USB; >> >> + cfg.drv_data = info; >> >> + >> >> + info->charger = devm_power_supply_register(dev, >> >> desc, &cfg); >> >> + if (IS_ERR(info->charger)) { >> >> + dev_err(dev, "Unable to register >> >> charger\n"); >> >> + return PTR_ERR(info->charger); >> >> + } >> >> + } >> >> + >> >> platform_set_drvdata(pdev, info); >> >> >> >> /* Perform initial detection */ >> >> -- >> >> 2.27.0 >> >> >> > >> > >> > -- >> > With Best Regards, >> > Andy Shevchenko >> >> >
| |