Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] usb: phy: add dedicated notifier for charger events
From:   Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>, felipe.balbi@linux.intel.com
Cc:     sre@kernel.org, orsonzhai@gmail.com, baolin.wang@linux.alibaba.com,
        zhang.lyra@gmail.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, patches@opensource.cirrus.com,
        linux-usb@vger.kernel.org, tony@atomide.com,
        Pavel Machek <pavel@ucw.cz>
References: <1668430562-27114-1-git-send-email-ivo.g.dimitrov.75@gmail.com>
 <Y3JpfDU7T2Ks/H4m@kroah.com> <ec4edd51-3915-a798-2310-8ceadcd8152f@gmail.com>
 <590de096-5843-eab2-a48c-ca2742cb329b@gmail.com>
Message-ID: <af63dc7c-6641-13a3-067f-c58923ee510e@gmail.com>
Date:   Mon, 5 Dec 2022 22:29:13 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Firefox/60.0 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <590de096-5843-eab2-a48c-ca2742cb329b@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Precedence: bulk

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