From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752390AbcF2MHb (ORCPT ); Wed, 29 Jun 2016 08:07:31 -0400 Received: from mga09.intel.com ([134.134.136.24]:40944 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751962AbcF2MH3 (ORCPT ); Wed, 29 Jun 2016 08:07:29 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,546,1459839600"; d="asc'?scan'208";a="130544350" From: Felipe Balbi To: Baolin Wang Cc: Greg KH , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , robh@kernel.org, Jun Li , Marek Szyprowski , Ruslan Bilovol , Peter Chen , Alan Stern , r.baldyga@samsung.com, grygorii.strashko@ti.com, Yoshihiro Shimoda , Lee Jones , Mark Brown , Charles Keepax , patches@opensource.wolfsonmicro.com, Linux PM list , USB , device-mainlining@lists.linuxfoundation.org, LKML Subject: Re: [PATCH v12 2/4] gadget: Support for the usb charger framework In-Reply-To: References: <87bn2uomzd.fsf@linux.intel.com> <87eg7giexn.fsf@linux.intel.com> <87bn2kieb6.fsf@linux.intel.com> User-Agent: Notmuch/0.22+51~gcc1a6d2 (http://notmuchmail.org) Emacs/25.0.93.2 (x86_64-pc-linux-gnu) Date: Wed, 29 Jun 2016 15:06:46 +0300 Message-ID: <87twgcgpwp.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: >>>>>>> For supporting the usb charger, it adds the usb_charger_init() and >>>>>>> usb_charger_exit() functions for usb charger initialization and exi= t. >>>>>>> >>>>>>> It will report to the usb charger when the gadget state is changed, >>>>>>> then the usb charger can do the power things. >>>>>>> >>>>>>> Signed-off-by: Baolin Wang >>>>>>> Reviewed-by: Li Jun >>>>>>> Tested-by: Li Jun >>>>>> >>>>>> Before anything, I must say that I really liked this patch. It's >>>>>> minimaly invasive to udc core and does all the necessary changes. If= it >>>>>> wasn't for the extra charger class, this would've been perfect. >>>>>> >>>>>> Can't you just tie a charger to a UDC and avoid the charger class >>>>>> completely? >>>>>> >>>>>>> static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, = unsigned mA) >>>>>>> { >>>>>>> + if (gadget->charger) >>>>>> >>>>>> I guess you could do this check inside >>>>>> usb_gadget_set_cur_limit_by_type() itself. >>>>> >>>>> We will access the 'gadget->charger->type' member when issuing >>>>> usb_gadget_set_cur_limit_by_type(), so I think I should leave the >>>>> check here in next new version. >>>> >>>> Here's what I mean: >>>> >>>> int usb_charger_set_cur_limit(struct usb_gadget *gadget, unsigned int = mA) >>>> { >>>> struct usb_charger *charger; >>>> enum usb_charger_type type; >>>> >>>> if (!gadget->charger) >>>> return 0; >>>> >>>> charger =3D gadget->charger; >>>> type =3D charger->type; >>>> >>>> return __usb_charger_set_cur_limit(charger, type, mA); >>>> } >>> >>> But that means we need to export both 'usb_charger_set_cur_limit()' >>> function and '__usb_charger_set_cur_limit()' function in charger.c >>> file. Cause some user may want to set the current limitation by one >>> charger type parameter (may be not from charger->type), like by >>> issuing '__usb_charger_set_cur_limit(charger, SDP_TYPE, mA)'. How do >>> you think about this situation? Thanks. >> >> if we have that requirement, that's totally fine. Just rename >> __usb_charger_set_cur_limit() back to >> _usb_charger_set_cur_limit_by_type() and expose both. But >> set_cur_limit_by_type can assume its arguments are valid at all times. > > Make sense. I'll fix this issue in v14 version. Thanks. there's one thing bothering me though: gadget->charger is supposed to be "current detected charger" right? If we have a single port tied to a single charger, in which case would we *ever* need to change current limit of any charger type other than "current charger" ? IOW, why do we need _set_cur_limit_by_type() exported at all? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXc7nWAAoJEIaOsuA1yqREackP/3GhRlN1gavtNaMtBrJ0fItG OHtk9tckBJXw/I2Gfx/lNpNbC6y7zkqUSX+dkBLSUNkR5HRK61uOam8sGlo4XYyK 87/3W8fL0rbQQddM1lLyXoJ9zwv0BHKIKu52fWgBd08Gp/vZ/8otv1xQX8nxisls BpdyVMTcXiv6nV9UZtNlaj//8+XD2dF6Lx2neyCpYtYdsIDPrYB8wlR4ClTWuJGJ abfhKqS/0zUnUi8xccPQpkn3XOzPGa6TtstbUklYAnPszWDcc0QIwrul2QokqcdQ C6dcd46tY5uZ3yh/UibfSfpVoKLo3k9CU5zeaDsElojmhXdtlmGOZdHV5Oql2Pxh QyqCjlXYfHRCpwwz8Ff8Xs8GRTuTN+BAxIkKcBgt+s/Kvn9Juj+1MuPcyQN8WdM/ TdySzUobK3B1YTLTaFdBCnPx4e7ZY18j6GnYDAijHUpTqlC8ZlJlYcmxG8XLvocA VqjJw7tL2whUH/aHdfku1spD7nAvOxVkkWpDKs2ANyIXSPom4Led45rPEReiN00k ZS1wgtN0Kvr12lY5r6EiHSixdGT3Uz7T5gaklAGivZKrENB1B4x1OYWU/mTtiRt/ RMlt71juzEZi83oUSM0W90Hn8DQGh4X0lXbmtFQaNgd7wNUdINcku4wenls1FYP1 hrhO+iUjrREeAg0ERKYt =K8Xg -----END PGP SIGNATURE----- --=-=-=--