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@xxxxxxxxx>
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
