| Message ID | 1668430562-27114-1-git-send-email-ivo.g.dimitrov.75@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: phy: add dedicated notifier for charger events | expand |
ping On 16.11.22 г. 9:11 ч., Ivaylo Dimitrov wrote: > > > On 14.11.22 г. 18:46 ч., Ivaylo Dimitrov wrote: >> Hi, >> >> On 14.11.22 г. 18:14 ч., Greg KH wrote: >>> On Mon, Nov 14, 2022 at 02:56:02PM +0200, Ivaylo Dimitrov wrote: >>>> usb_phy::notifier is already used by various PHY drivers (including >>>> phy_generic) to report VBUS status changes and its usage conflicts with >>>> charger current limit changes reporting. >>> >>> How exactly does it conflict? >>> >> >> see below >> >>>> Fix that by introducing a second notifier that is dedicated to usb >>>> charger >>>> notifications. Add usb_charger_XXX_notifier functions. Fix charger >>>> drivers >>>> that currently (ab)use usb_XXX_notifier() to use the new API. >>> >>> Why not just set the notifier type to be a new one instead of adding a >>> whole new notifier list? Or use a real callback? notifier lists are >>> really horrid and should be avoided whenever possible. >>> >> >> Not sure what you mean by "notifier type', but if that is that val >> parameter of atomic_notifier_call_chain(), the way it is used by usb >> charger FW: >> >> https://elixir.bootlin.com/linux/latest/source/drivers/usb/phy/phy.c#L132 >> >> is not compatible with: >> >> https://elixir.bootlin.com/linux/latest/source/drivers/usb/phy/phy-generic.c#L185 >> >> >> for example, IIUC. >> >> The former wants to send max current as val, while latter sends event >> type as val. Sure, I may create some kind of hack, like using the MSB >> to denote charger events, but that doesn't feel right. >> >> Or, shall I do something else and fix the usage all over the place? >> Please elaborate. >> > > Digging further into that, it seems phy-ab8500-usb.c is also using > usb_phy::notifier in non-standard way, it sends events from > ux500_musb_vbus_id_status instead of usb_phy_events. I don't know the > history behind, but right now we have at least 3 incompatible usages of > usb_phy::notifier: > > 1. Most of the phy and charger drivers use usb_phy_events as notifier type > > 2. phy-ab8500-usb.c uses ux500_musb_vbus_id_status as notifier type, I > am not the only one to hit that it seems > https://elixir.bootlin.com/linux/v6.1-rc5/source/drivers/power/supply/ab8500_charger.c#L3191 > > > 3. USB charger framework uses max charging current as notifier type. > > Moreover, a charger driver in a system that has gadget drivers support > and phy that has extcon charger cable detection support and registers to > phy notifier, will inevitably receive (1) and (3) types of > notifications, without any way to distinguish I was able to find. > > I don't really see how those can be merged to use one notifier only, > without fixing most of USB phy and gadget drivers and half of charger > drivers. Not that I like adding the second notifier, I just don;t see > other way. > > Regards, > Ivo > >> In regards to callback - I didn't want to come-up with a whole new >> API, but just fix the current one. Also, a single callback will not be >> enough - imagine a case with 2 batteries that have to be charged by a >> single USB port, so 2 separate charger devices, most-probably. We will >> have to keep a list of callback functions somehow. I admit my lack of >> knowledge, but, do we already have such API to use? >> >>>> Fixes: a9081a008f84 ("usb: phy: Add USB charger support") >>>> >>>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> >>> >>> You can't have a blank line between there, checkpatch.pl should have >>> complained. >>> >> >> it didn't: >> >> ./scripts/checkpatch.pl >> 0001-usb-phy-add-dedicated-notifier-for-charger-events.patch >> total: 0 errors, 0 warnings, 90 lines checked >> >> 0001-usb-phy-add-dedicated-notifier-for-charger-events.patch has no >> obvious style problems and is ready for submission. >> >> Will fix, if I am to send v2 >> >> Thanks, >> Ivo >> >>> thanks, >>> >>> greg k-h >>>
On Wed, Nov 16, 2022 at 09:11:58AM +0200, Ivaylo Dimitrov wrote: > > > On 14.11.22 г. 18:46 ч., Ivaylo Dimitrov wrote: > > Hi, > > > > On 14.11.22 г. 18:14 ч., Greg KH wrote: > > > On Mon, Nov 14, 2022 at 02:56:02PM +0200, Ivaylo Dimitrov wrote: > > > > usb_phy::notifier is already used by various PHY drivers (including > > > > phy_generic) to report VBUS status changes and its usage conflicts with > > > > charger current limit changes reporting. > > > > > > How exactly does it conflict? > > > > > > > see below > > > > > > Fix that by introducing a second notifier that is dedicated to > > > > usb charger > > > > notifications. Add usb_charger_XXX_notifier functions. Fix > > > > charger drivers > > > > that currently (ab)use usb_XXX_notifier() to use the new API. > > > > > > Why not just set the notifier type to be a new one instead of adding a > > > whole new notifier list? Or use a real callback? notifier lists are > > > really horrid and should be avoided whenever possible. > > > > > > > Not sure what you mean by "notifier type', but if that is that val > > parameter of atomic_notifier_call_chain(), the way it is used by usb > > charger FW: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/usb/phy/phy.c#L132 > > > > is not compatible with: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/usb/phy/phy-generic.c#L185 > > > > > > for example, IIUC. > > > > The former wants to send max current as val, while latter sends event > > type as val. Sure, I may create some kind of hack, like using the MSB to > > denote charger events, but that doesn't feel right. > > > > Or, shall I do something else and fix the usage all over the place? > > Please elaborate. > > > > Digging further into that, it seems phy-ab8500-usb.c is also using > usb_phy::notifier in non-standard way, it sends events from > ux500_musb_vbus_id_status instead of usb_phy_events. I don't know the > history behind, but right now we have at least 3 incompatible usages of > usb_phy::notifier: > > 1. Most of the phy and charger drivers use usb_phy_events as notifier type > > 2. phy-ab8500-usb.c uses ux500_musb_vbus_id_status as notifier type, I am > not the only one to hit that it seems https://elixir.bootlin.com/linux/v6.1-rc5/source/drivers/power/supply/ab8500_charger.c#L3191 > > 3. USB charger framework uses max charging current as notifier type. > > Moreover, a charger driver in a system that has gadget drivers support and > phy that has extcon charger cable detection support and registers to phy > notifier, will inevitably receive (1) and (3) types of notifications, > without any way to distinguish I was able to find. Can't they properly detect this based on the type of the notification sent to them? Why not just set that correctly? > I don't really see how those can be merged to use one notifier only, without > fixing most of USB phy and gadget drivers and half of charger drivers. Not > that I like adding the second notifier, I just don;t see other way. Fixing them all so that we don't have this mess and require yet-another-notifier would be very good. I know it's not your mess, but I think it's the best long-term solution to it, don't you? thanks, greg k-h
diff --git a/drivers/power/supply/sc2731_charger.c b/drivers/power/supply/sc2731_charger.c index 9ac17cf..e3fa0e2 100644 --- a/drivers/power/supply/sc2731_charger.c +++ b/drivers/power/supply/sc2731_charger.c @@ -500,7 +500,7 @@ static int sc2731_charger_probe(struct platform_device *pdev) } info->usb_notify.notifier_call = sc2731_charger_usb_change; - ret = usb_register_notifier(info->usb_phy, &info->usb_notify); + ret = usb_charger_register_notifier(info->usb_phy, &info->usb_notify); if (ret) { dev_err(&pdev->dev, "failed to register notifier: %d\n", ret); return ret; @@ -515,7 +515,7 @@ static int sc2731_charger_remove(struct platform_device *pdev) { struct sc2731_charger_info *info = platform_get_drvdata(pdev); - usb_unregister_notifier(info->usb_phy, &info->usb_notify); + usb_charger_unregister_notifier(info->usb_phy, &info->usb_notify); return 0; } diff --git a/drivers/power/supply/wm831x_power.c b/drivers/power/supply/wm831x_power.c index 82e3106..0744167 100644 --- a/drivers/power/supply/wm831x_power.c +++ b/drivers/power/supply/wm831x_power.c @@ -650,7 +650,8 @@ static int wm831x_power_probe(struct platform_device *pdev) switch (ret) { case 0: power->usb_notify.notifier_call = wm831x_usb_limit_change; - ret = usb_register_notifier(power->usb_phy, &power->usb_notify); + ret = usb_charger_register_notifier(power->usb_phy, + &power->usb_notify); if (ret) { dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret); @@ -701,8 +702,8 @@ static int wm831x_power_remove(struct platform_device *pdev) int irq, i; if (wm831x_power->usb_phy) { - usb_unregister_notifier(wm831x_power->usb_phy, - &wm831x_power->usb_notify); + usb_charger_unregister_notifier(wm831x_power->usb_phy, + &wm831x_power->usb_notify); } for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) { diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c index 1b24492..6b8bf05 100644 --- a/drivers/usb/phy/phy.c +++ b/drivers/usb/phy/phy.c @@ -129,12 +129,13 @@ static void usb_phy_notify_charger_work(struct work_struct *work) case USB_CHARGER_PRESENT: usb_phy_get_charger_current(usb_phy, &min, &max); - atomic_notifier_call_chain(&usb_phy->notifier, max, usb_phy); + atomic_notifier_call_chain(&usb_phy->chg_notifier, max, + usb_phy); break; case USB_CHARGER_ABSENT: usb_phy_set_default_current(usb_phy); - atomic_notifier_call_chain(&usb_phy->notifier, 0, usb_phy); + atomic_notifier_call_chain(&usb_phy->chg_notifier, 0, usb_phy); break; default: dev_warn(usb_phy->dev, "Unknown USB charger state: %d\n", @@ -678,6 +679,7 @@ int usb_add_phy(struct usb_phy *x, enum usb_phy_type type) return ret; ATOMIC_INIT_NOTIFIER_HEAD(&x->notifier); + ATOMIC_INIT_NOTIFIER_HEAD(&x->chg_notifier); spin_lock_irqsave(&phy_lock, flags); @@ -730,6 +732,7 @@ int usb_add_phy_dev(struct usb_phy *x) x->dev->type = &usb_phy_dev_type; ATOMIC_INIT_NOTIFIER_HEAD(&x->notifier); + ATOMIC_INIT_NOTIFIER_HEAD(&x->chg_notifier); spin_lock_irqsave(&phy_lock, flags); list_add_tail(&x->head, &phy_list); diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h index e4de6bc..23db554 100644 --- a/include/linux/usb/phy.h +++ b/include/linux/usb/phy.h @@ -111,6 +111,7 @@ struct usb_phy { enum usb_charger_state chg_state; struct usb_charger_current chg_cur; struct work_struct chg_work; + struct atomic_notifier_head chg_notifier; /* for notification of usb_phy_events */ struct atomic_notifier_head notifier; @@ -347,6 +348,19 @@ static inline void usb_phy_set_charger_state(struct usb_phy *usb_phy, atomic_notifier_chain_unregister(&x->notifier, nb); } +/* notifiers */ +static inline int +usb_charger_register_notifier(struct usb_phy *x, struct notifier_block *nb) +{ + return atomic_notifier_chain_register(&x->chg_notifier, nb); +} + +static inline void +usb_charger_unregister_notifier(struct usb_phy *x, struct notifier_block *nb) +{ + atomic_notifier_chain_unregister(&x->chg_notifier, nb); +} + static inline const char *usb_phy_type_string(enum usb_phy_type type) { switch (type) {
usb_phy::notifier is already used by various PHY drivers (including phy_generic) to report VBUS status changes and its usage conflicts with charger current limit changes reporting. Fix that by introducing a second notifier that is dedicated to usb charger notifications. Add usb_charger_XXX_notifier functions. Fix charger drivers that currently (ab)use usb_XXX_notifier() to use the new API. Fixes: a9081a008f84 ("usb: phy: Add USB charger support") Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> --- drivers/power/supply/sc2731_charger.c | 4 ++-- drivers/power/supply/wm831x_power.c | 7 ++++--- drivers/usb/phy/phy.c | 7 +++++-- include/linux/usb/phy.h | 14 ++++++++++++++ 4 files changed, 25 insertions(+), 7 deletions(-)