| 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 |
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 >>
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(-)