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: 4604981 Return-Path: X-Original-To: patchwork-linux-samsung-soc@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 845FFC0514 for ; Tue, 22 Jul 2014 19:31:50 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 58DFB20123 for ; Tue, 22 Jul 2014 19:31:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EA6182015A for ; Tue, 22 Jul 2014 19:31:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756789AbaGVTbm (ORCPT ); Tue, 22 Jul 2014 15:31:42 -0400 Received: from mail-wg0-f41.google.com ([74.125.82.41]:45047 "EHLO mail-wg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756710AbaGVTbl (ORCPT ); Tue, 22 Jul 2014 15:31:41 -0400 Received: by mail-wg0-f41.google.com with SMTP id z12so117850wgg.12 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=DOFIgXqAbKOAzf4Qcv/G2vtwSM9IqVfnbDNBhfgpsqyvtu/kgcPHkexT6HCQTDctof bfFQVctqcY1WfQsWRDu2lfAT+n0gtXwrx8wCI3H6MqtMavmc+yxMzXa/crjks6v6HOcm o7V1l3QzaXDhT851hX80wRZ0RoLsNg6DDgFppp4I8BFJS44zho6W4yN0gExCPSuSRABY 60nQ0s7DXlzX0cMoBfoHZI4REfYeXNmGcPXfavRR82VIWFQDVK74N4B41aK3DoYXXI2P G4xqBVvLTrrlZxErB+F3v6XWkzl+wX8QmMt0/Y2C3WnTs205DffnxNVu0nURNwSm6YhW 1I+w== X-Gm-Message-State: ALoCoQksjfe/Ia4QmUhXx4lRLuVaV8XEYZzZqGAXoajJezY4LFypE8QJeu4qUJ72cCkq/EUzppb7 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 Cc: linux-samsung-soc , Doug Anderson , "linux-arm-kernel@lists.infradead.org" , Jaehoon Chung , Chris Ball , Seungwon Jeon , "linux-mmc@vger.kernel.org" , Ulf Hansson , Sonny Rao , Tomasz Figa , Kukjin Kim , sunil joshi , ks.giri@samsung.com, Prashanth G , Alim Akhtar , Yuvaraj Kumar C D Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham 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");