Date:   Thu, 8 Dec 2022 16:39:35 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
Cc:     felipe.balbi@...ux.intel.com, sre@...nel.org, orsonzhai@...il.com,
        baolin.wang@...ux.alibaba.com, zhang.lyra@...il.com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com, linux-usb@...r.kernel.org,
        tony@...mide.com
Subject: Re: [PATCH] usb: phy: add dedicated notifier for charger events

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

Powered by blists - more mailing lists