From patchwork Tue Jul 22 19:31:39 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Javier Martinez Canillas X-Patchwork-Id: 4605121 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id D1BD3C0514 for ; Tue, 22 Jul 2014 19:34:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B8AD220155 for ; Tue, 22 Jul 2014 19:34:20 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A62A020123 for ; Tue, 22 Jul 2014 19:34:19 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1X9fnf-0006V5-Fv; Tue, 22 Jul 2014 19:32:35 +0000 Received: from mail-wg0-f49.google.com ([74.125.82.49]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1X9fn9-0005Xd-2P for linux-arm-kernel@lists.infradead.org; Tue, 22 Jul 2014 19:32:04 +0000 Received: by mail-wg0-f49.google.com with SMTP id k14so115642wgh.32 for ; Tue, 22 Jul 2014 12:31:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=hZOxd8MFL52TO8DiBCGkXjlYz9w7fo8uPOwSwqAVA8o=; b=Fjkxu4J4s1nZ6odxZdF266sCPsKjXNaHCRqnBm8oTFNJWDVEnNivdN7+CHzE9wyOib 7Mk230eqmdlyOL1gxr7P0D/ABwutSJdQelTk+1YlSGeN2P8pUohx7UcoFQEB150ayxkq B9/7j4LpF1UobCcGz9ntOq8yUMy1xSoPRZImuW2QH7Ssk6EeucnvAaBn4IozmTUz74iq pe/nTFSF/vE8k01E75PV3lH2ZRnWWaRrjkQpovW/h9rGLMplWFZ08nxbz5CkrRB7Or+V I7wt3252IUw9cv4GE+vZkybpvW326SXr1u7A3nk8U/xt7ohVTFwgYtTgGYr/LC8fyeVm wBig== X-Gm-Message-State: ALoCoQmpl9YJqCNeaDHbtwI22qOspz3o264JMP6ajWtjhKyD5aV0wzT+vm9m4LnzqovT59eQlHQg MIME-Version: 1.0 X-Received: by 10.194.61.47 with SMTP id m15mr37756852wjr.63.1406057499297; Tue, 22 Jul 2014 12:31:39 -0700 (PDT) Received: by 10.180.211.200 with HTTP; Tue, 22 Jul 2014 12:31:39 -0700 (PDT) X-Originating-IP: [95.21.62.104] In-Reply-To: References: <1403520321-2431-1-git-send-email-yuvaraj.cd@samsung.com> <1403520321-2431-2-git-send-email-yuvaraj.cd@samsung.com> Date: Tue, 22 Jul 2014 21:31:39 +0200 Message-ID: Subject: Re: [PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators From: Javier Martinez Canillas To: Yuvaraj Kumar X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140722_123203_417552_574A2ED9 X-CRM114-Status: GOOD ( 29.10 ) X-Spam-Score: -0.7 (/) Cc: Yuvaraj Kumar C D , Ulf Hansson , linux-samsung-soc , Prashanth G , Seungwon Jeon , ks.giri@samsung.com, Tomasz Figa , "linux-mmc@vger.kernel.org" , Doug Anderson , sunil joshi , Jaehoon Chung , Kukjin Kim , Alim Akhtar , Chris Ball , Sonny Rao , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hello Yuvaraj, On Sat, Jun 28, 2014 at 12:47 AM, Doug Anderson wrote: > On Fri, Jun 27, 2014 at 3:59 AM, Yuvaraj Kumar wrote: >>>> mmc_regulator_set_ocr() is failing as its a fixed-regulator. >>> >>> Can you explain more about what's failing? It sure looks like mmc >>> core is supposed to handle this given comments below >>> >>> /* >>> * If we're using a fixed/static regulator, don't call >>> * regulator_set_voltage; it would fail. >>> */ >> tps65090 driver does not register through fixed regulator framework.It >> uses normal regulator framework and supports only >> enable/disable/is_enabled callbacks.Also it lacks certain callbacks >> for list_voltage, get_voltage ,set_voltage etc. >> [ 2.306476] dwmmc_exynos 12220000.mmc: Failed getting OCR mask: -22 >> [ 2.393403] dwmmc_exynos 12220000.mmc: could not set regulator OCR (-22) >> For the above reason,regulator framework treats fet4 as unused >> regulator and disables the vmmc regulator. > > Ah. Perhaps tps65090 needs to be fixed then... I'm not sure any > other drivers cared before so maybe that's why it was never caught? > I've been looking at this and as Doug and you said it seems that nobody cared and since the tps65090 fets are actually just load switches the driver only implementes the .enable and .disable functions handlers. Also since their output voltage is just equal to their input supply, that information was not present neither in the driver nor in the Device Tree. But when fet4 is used as the vmmc-supply phandle, mmc_regulator_get_supply() calls mmc_regulator_get_ocrmask() [0] which expects to fetch the regulator voltage count and current voltage as you had explained so the function was failing. Now I don't think that the driver needs to implement the {get,set,list}_voltage callbacks. If a tps65090 regulator has both regulator-min-microvolt and regulator-max-microvolt properties from the generic regulator DT binding, and the value is the same, it can be assumed that the regulator is fixed and the fixed_uV field can be set to the output voltage and n_voltages to 1. That's everything that the regulator core needs from a fixed regulator in order to make regulator_get_voltage() to succeed: static int _regulator_get_voltage(struct regulator_dev *rdev) { ... if (rdev->desc->ops->get_voltage_sel) { sel = rdev->desc->ops->get_voltage_sel(rdev); if (sel < 0) return sel; ret = rdev->desc->ops->list_voltage(rdev, sel); } else if (rdev->desc->ops->get_voltage) { ret = rdev->desc->ops->get_voltage(rdev); } else if (rdev->desc->ops->list_voltage) { ret = rdev->desc->ops->list_voltage(rdev, 0); } else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) { ret = rdev->desc->fixed_uV; } else { return -EINVAL; } ... } What do you think about patch [1]? So, with that patch mmc_regulator_get_supply() does not fail anymore and also does not break DT backward compatibility since it only has effect on the regulators that define their min and max voltage and not in the ones that don't like is the case in old DTB. Now, it is an RFC since I don't know if this is the correct solution. Even though I've seen that min == max seems to imply that the regulator is fixed, I wonder if is better to have an explicit generic DT property (regulator-fixed-microvolt?) that is used to set fixed_uV in of_get_regulation_constraints(). Since this seems to be a general problem and not only related to the tps65090 device or the Peach Pit/Pi use case. If you agree that it's a good solution then I can post it as a proper patch. I've also pushed this patch and the ones adding the needed DTS bits for Peach Pit and Pi DTS to my personal repository so you can test it. The branch is: http://cgit.collabora.com/git/user/javier/linux.git/log/?h=tps65090-fixes You also need to replace regulator_enable(mmc->supply.vmmc) and regulator_disable(mmc->supply.vmmc) by mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd) and mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0) respectively in your original patch. Best regards, Javier [0]: http://lxr.free-electrons.com/source/drivers/mmc/core/core.c#L1215 [1]: From 3d2e6079cc8c372da946d430d43d36af99e7a582 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas Date: Tue, 22 Jul 2014 19:16:47 +0200 Subject: [RFC] regulator: tps65090: Allow regulators voltage to be queried The tps65090 device has some regulators with a fixed output voltage and others with an adjustable output voltage. But the regulators with adjustable output just provide the voltage of their input supply so they really are fixed regulators within a supported voltage range that depends on their input voltage. That is why the driver only provides an .enable and .disable function handlers and not a .{get,set,list}_voltage handlers. But there is code in the kernel that expects to query the output voltage for all regulators. For example the function mmc_regulator_get_ocrmask() is executed if a regulator is used as a vmmc supply and this functions tries to fetch the number of voltages supported by the regulator and its current voltage but fails since the driver does not provide this data. The .{list,get}_voltage function handler are actually not even needed for fixed regulators since in this case the regulator core just need a fixed voltage and a single voltage selector. So, let's allow output voltage to be defined using the generic regulator-min-microvolt and regulator-max-microvolt properties and use the same semantic for fixed regulator as explained in Documentation/devicetree/bindings/regulator/fixed-regulator.txt. Where having an equal value for min and max microvolt means that the regulator is fixed and has a single voltage selector. Signed-off-by: Javier Martinez Canillas --- drivers/regulator/tps65090-regulator.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) @@ -435,6 +436,19 @@ static int tps65090_regulator_probe(struct platform_device *pdev) ri->overcurrent_wait_valid = tps_pdata->overcurrent_wait_valid; ri->overcurrent_wait = tps_pdata->overcurrent_wait; + + min_uV = tps_pdata->reg_init_data->constraints.min_uV; + max_uV = tps_pdata->reg_init_data->constraints.max_uV; + + /* + * A regulator whose regulator-min-microvolt and + * regulator-max-microvolt properties are the same + * is a fixed regulator and has a single voltage rail. + */ + if (min_uV && max_uV && min_uV == max_uV) { + ri->desc->fixed_uV = min_uV; + ri->desc->n_voltages = 1; + } } /* diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c index 2064b3f..673948d 100644 --- a/drivers/regulator/tps65090-regulator.c +++ b/drivers/regulator/tps65090-regulator.c @@ -408,6 +408,7 @@ static int tps65090_regulator_probe(struct platform_device *pdev) struct of_regulator_match *tps65090_reg_matches = NULL; int num; int ret; + int min_uV, max_uV; dev_dbg(&pdev->dev, "Probing regulator\n");