Re: [PATCH] usb: phy: add dedicated notifier for charger events

From: Ivaylo Dimitrov
Date: Wed Nov 16 2022 - 02:12:14 EST




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