From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966074AbcJ1REC (ORCPT ); Fri, 28 Oct 2016 13:04:02 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:44372 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755617AbcJ1REA (ORCPT ); Fri, 28 Oct 2016 13:04:00 -0400 Date: Fri, 28 Oct 2016 18:03:02 +0100 From: Mark Brown To: Baolin Wang Cc: NeilBrown , Felipe Balbi , Greg KH , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , robh@kernel.org, Jun Li , Marek Szyprowski , Ruslan Bilovol , Peter Chen , Alan Stern , grygorii.strashko@ti.com, Yoshihiro Shimoda , Lee Jones , John Stultz , Charles Keepax , patches@opensource.wolfsonmicro.com, Linux PM list , USB , device-mainlining@lists.linuxfoundation.org, LKML Message-ID: <20161028170302.au73yu2fplfig36a@sirena.org.uk> References: <87k2cttptn.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="42amaels6ebn7ksv" Content-Disposition: inline In-Reply-To: X-Cookie: Serenity through viciousness. User-Agent: NeoMutt/20161014 (1.7.1) X-SA-Exim-Connect-IP: 64.88.227.140 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: No (on mezzanine.sirena.org.uk); Unknown failure Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --42amaels6ebn7ksv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote: > On 28 October 2016 at 06:00, NeilBrown wrote: > > 1/ I think we agreed that it doesn't make sense for there to be > > two chargers registered in a system. > Yes, until now... > > However usb_charger_register() still allows that, and assigns > > and arbitrary name to each based on discovery order. > > This *cannot* make sense. > Fine, I can change that to allow only one charger to register. Yeah, it's a reasonable change. I'm not sure the prior discussion was 100% conclusive on the issue (I remember there being some debate about leaving things there to avoid any need for future refactoring to touch the interface). > > 2/ Why do you have usb_charger_set_current()?? > > No code ever calls it. > > This updates the min and max current which are defined in a > > standard. It never makes sense to change the min and max > > for a particular cable type. > Mark, do we have some scenarios which want to change the current > limitation? If not, okay, I agree with you to remove this function. I'm not aware of any, we can always add it back if the need arises. > > Related: I don't like charger_type_show(). I don't think > > the usb-charger should export that information to user-space because > > extcon already does that, and duplication is confusing and pointless. > I think we should combine all charger related information into one > place for user. Moreover if we don't get charger type from extcon, we > should also need one place to export the charger type. I had also thought there was some software negotation as well as the physical charger in cases where the device is plugged into an active host? I could be wrong. > > 5/ There is no convincing example usage of this framework. > > wm8931x_power.c just scratches the surface. > > If it is so good, it should be easy to convert a lot of other > > drivers over to it. If you did that it would be much easier > > to see how it works and what the strengths/weaknesses were. > Jun have send out one patchset[1] based on my patchset, and he tested > mypatchset. Thanks for your comments. > [1]http://www.spinics.net/lists/linux-usb/msg139809.html I think it's a good idea to pick up Jun's patches into your patch set, that way Jun doesn't need to rebase and it might help with review of your patches too. --42amaels6ebn7ksv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYE4TFAAoJECTWi3JdVIfQeiUH/AhJEjOuEBHtgFYVAJxql/VG 64mY8DdZxHBTRptPeCVz/2y7QmjpYEfX9obfSRKfeOkANwm4z4wfj15TvywQCQsu H4YH9lC7GYKa4jWwE+xnXVByB/SR7NFYvbxmsB1rcmw6bG8EKjsmfEUyO3erG/7e posGLzIFp47eEpN6LZfMXYHQSNz3DRToMfqF1DGTGGyNnGcY9aRUb59tG/Toqi4e D5IKWfCP30PxuSVv9vMmY2YfYbAe6y8Kogo4m0A2J3VTiA2DdrNg5BeHdZ569HqY IvP/IgiYRAOJavXkOuLAjrboqMYJqPtu0onSs+XXf4IgPSPK2e3HFstRPl+hQQk= =ia7K -----END PGP SIGNATURE----- --42amaels6ebn7ksv-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation Date: Fri, 28 Oct 2016 18:03:02 +0100 Message-ID: <20161028170302.au73yu2fplfig36a@sirena.org.uk> References: <87k2cttptn.fsf@notabene.neil.brown.name> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="42amaels6ebn7ksv" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Baolin Wang Cc: NeilBrown , Felipe Balbi , Greg KH , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Jun Li , Marek Szyprowski , Ruslan Bilovol , Peter Chen , Alan Stern , grygorii.strashko-l0cyMroinI0@public.gmane.org, Yoshihiro Shimoda , Lee Jones , John Stultz , Charles Keepax , patches-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org, Linux PM list , USB , dev List-Id: linux-pm@vger.kernel.org --42amaels6ebn7ksv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote: > On 28 October 2016 at 06:00, NeilBrown wrote: > > 1/ I think we agreed that it doesn't make sense for there to be > > two chargers registered in a system. > Yes, until now... > > However usb_charger_register() still allows that, and assigns > > and arbitrary name to each based on discovery order. > > This *cannot* make sense. > Fine, I can change that to allow only one charger to register. Yeah, it's a reasonable change. I'm not sure the prior discussion was 100% conclusive on the issue (I remember there being some debate about leaving things there to avoid any need for future refactoring to touch the interface). > > 2/ Why do you have usb_charger_set_current()?? > > No code ever calls it. > > This updates the min and max current which are defined in a > > standard. It never makes sense to change the min and max > > for a particular cable type. > Mark, do we have some scenarios which want to change the current > limitation? If not, okay, I agree with you to remove this function. I'm not aware of any, we can always add it back if the need arises. > > Related: I don't like charger_type_show(). I don't think > > the usb-charger should export that information to user-space because > > extcon already does that, and duplication is confusing and pointless. > I think we should combine all charger related information into one > place for user. Moreover if we don't get charger type from extcon, we > should also need one place to export the charger type. I had also thought there was some software negotation as well as the physical charger in cases where the device is plugged into an active host? I could be wrong. > > 5/ There is no convincing example usage of this framework. > > wm8931x_power.c just scratches the surface. > > If it is so good, it should be easy to convert a lot of other > > drivers over to it. If you did that it would be much easier > > to see how it works and what the strengths/weaknesses were. > Jun have send out one patchset[1] based on my patchset, and he tested > mypatchset. Thanks for your comments. > [1]http://www.spinics.net/lists/linux-usb/msg139809.html I think it's a good idea to pick up Jun's patches into your patch set, that way Jun doesn't need to rebase and it might help with review of your patches too. --42amaels6ebn7ksv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYE4TFAAoJECTWi3JdVIfQeiUH/AhJEjOuEBHtgFYVAJxql/VG 64mY8DdZxHBTRptPeCVz/2y7QmjpYEfX9obfSRKfeOkANwm4z4wfj15TvywQCQsu H4YH9lC7GYKa4jWwE+xnXVByB/SR7NFYvbxmsB1rcmw6bG8EKjsmfEUyO3erG/7e posGLzIFp47eEpN6LZfMXYHQSNz3DRToMfqF1DGTGGyNnGcY9aRUb59tG/Toqi4e D5IKWfCP30PxuSVv9vMmY2YfYbAe6y8Kogo4m0A2J3VTiA2DdrNg5BeHdZ569HqY IvP/IgiYRAOJavXkOuLAjrboqMYJqPtu0onSs+XXf4IgPSPK2e3HFstRPl+hQQk= =ia7K -----END PGP SIGNATURE----- --42amaels6ebn7ksv-- -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html