From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753052AbcD2LzX (ORCPT ); Fri, 29 Apr 2016 07:55:23 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:35638 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752563AbcD2LzV (ORCPT ); Fri, 29 Apr 2016 07:55:21 -0400 X-AuditID: cbfec7f5-f792a6d000001302-50-57234ba470c7 Subject: Re: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization To: Mark Brown References: <1461927591-7864-1-git-send-email-k.kozlowski@samsung.com> <1461927591-7864-2-git-send-email-k.kozlowski@samsung.com> <20160429113059.GX3217@sirena.org.uk> Cc: Kukjin Kim , Chanwoo Choi , Liam Girdwood , Greg Kroah-Hartman , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-usb@vger.kernel.org, linux.amoon@gmail.com, tjakobi@math.uni-bielefeld.de, m.szyprowski@samsung.com, hverkuil@xs4all.nl, Bartlomiej Zolnierkiewicz From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <57234BA2.6020304@samsung.com> Date: Fri, 29 Apr 2016 13:55:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-version: 1.0 In-reply-to: <20160429113059.GX3217@sirena.org.uk> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrIIsWRmVeSWpSXmKPExsVy+t/xq7pLvJXDDX7vk7TYOGM9q8XUh0/Y LK5/ec5qMf/IOVaL5sXr2SxOTX7GZPH6haFF/+PXzBbfrnQwWWx6fI3V4vKuOWwWM87vY7JY tKyV2WLdxlvsFmuP3GW3aFv9gdVBwGPnrLvsHptWdbJ57J+7ht1j85J6j3/H2D36tqxi9Pi8 Sc7j1NfP7AEcUVw2Kak5mWWpRfp2CVwZTzffYiu4KVnx5fov9gbGTSJdjJwcEgImEv277rJB 2GISF+6tB7K5OIQEljJKLFl8khnCecYocW3LDGaQKmGBeIkZncsZQWwRAWWJq9/3skAUrWaU ODWvkxXEYRZ4zCxxbP58JpAqNgFjic3Ll0DtkJPo7Z7EAmLzCmhJtPzbBzaJRUBV4uz582Bx UYEIidXrrjFD1AhK/Jh8DyzOKWAksWfdZyCbA2iBnsT9i1ogYWYBeYnNa94yT2AUnIWkYxZC 1SwkVQsYmVcxiqaWJhcUJ6XnGukVJ+YWl+al6yXn525ihETc1x2MS49ZHWIU4GBU4uGdcU8p XIg1say4MvcQowQHs5IIr667crgQb0piZVVqUX58UWlOavEhRmkOFiVx3pm73ocICaQnlqRm p6YWpBbBZJk4OKUaGFc5eRv8T06VM8pMZ/m+cIO6zg4Ndz9n/m3rRaM6Y9lsnLVezF28cBL/ 5vbT098c+J2pLno0ZpWJQPOtTTvPnEnkltReZVeg6R5gvszfl+XpO4/EZ2ushH8/2qX9kzm7 SjPjq5mkQkIK39LgohCtGi/Nj5b8K/5vz28q2L3jXUXtrXzDjd/nK7EUZyQaajEXFScCAOCO 5fK0AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/29/2016 01:30 PM, Mark Brown wrote: > On Fri, Apr 29, 2016 at 12:59:49PM +0200, Krzysztof Kozlowski wrote: > >> +++ b/Documentation/devicetree/bindings/usb/usb3503.txt >> @@ -24,6 +24,7 @@ Optional properties: >> pins (optional, if not provided, driver will not set rate of the >> REFCLK signal and assume that a value from the primary reference >> clock frequencies table is used) >> +- vdd33-supply: Optional supply for VDD 3.3 V power source. > > Supplies are only optional if they may be physically absent. In this > case it's possible that on device regulators may be used instead, a > pattern more like that used for arizona-ldo1 where we represent those > regulators might be better as it's more clearly describing the > situation. I'm just wondering if the supply lookup stuff there should > be factored out as this is not an uncommon pattern.. > > It should at least be clearly stated what's going on, ignoring failure > to get supplies is generally a bug and people will tend to blindly cut > and paste things (witness all the breakage in graphics drivers with > this). The device has four power input lines (called VBAT, VDD33, VDD_CORE and VDD_12). Datasheet describes 4 valid configurations... but impression of the Odroid U3 board schematics is that they used another (custom?) configuration. I did not add rest of regulators on purpose: 1. I don't have other configurations to test. 2. It is rather old device, so I don't expect active development. The VDD33 is really optional. The device can work in different configuration, e.g. only on VBAT. How the reset logic would work then? I don't know... I would suspect that it could be exactly the same (just replace VDD33 with VBAT) but I am not sure. >> static int usb3503_reset(struct usb3503 *hub, int state) >> { >> + int err; >> + >> + err = usb3503_regulator(hub, state); >> + if (err) { >> + dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n", >> + (state ? "enable" : "disable"), err); >> + } > > Are we sure that the callers all balance enables and disables and we > don't ever end up going through reset more than once on the way down? I double checked the code and there might be in-balance if DT or platform data sets initial mode to suspend. Otherwise it should be balanced. I'll re-think the patch and fix this. > >> + hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33"); >> + if (IS_ERR(hub->vdd_reg)) { >> + if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; > > This should explicitly check for -ENODEV and return the error if it gets > anything else, that will mean that if the supply is needed but lookup > fails somehow due to a non-deferral error we'll handle it properly. I must admit I wasn't sure about handling the ENODEV and some other examples (drivers) were handling this just like that. Thanks for pointing this out. > >> + err = usb3503_regulator(hub, true); > > The naming on this function is very obscure (and there's also a couple > of other supplies). I'd suggest just folding this into the reset > function, or at least renaming so the reader can tell what these calls > do. Okay. Thanks for feedback! Best regards, Krzysztof