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

[Ivaylo Dimitrov]

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