Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754460AbcDFFCc (ORCPT ); Wed, 6 Apr 2016 01:02:32 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:58560 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751046AbcDFFCa (ORCPT ); Wed, 6 Apr 2016 01:02:30 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 05 Apr 2016 22:02:28 -0700 From: Sreedhar Sambangi To: Andy Gross Cc: linux-arm-msm@vger.kernel.org, qca-upstream.external@qca.qualcomm.com, lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] qcom: ipq4019: Add LDO regulator driver for SDHC controller In-Reply-To: <20160405055327.GA2639@hector> References: <1459804104-31911-1-git-send-email-ssambang@codeaurora.org> <20160405055327.GA2639@hector> Message-ID: <67657a9b306dad6f3a1b1d1cedf7f12f@codeaurora.org> User-Agent: Roundcube Webmail/1.1.4 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5505 Lines: 199 On 2016-04-04 22:53, Andy Gross wrote: > On Mon, Apr 04, 2016 at 02:08:24PM -0700, Sreedhar Sambangi wrote: >> From: Kirthik Srinivasan >> >> Add LDO regulator driver to enable SD /MMC card to >> switch between 3.0 volts and 1.8 volts >> >> Change-Id: I66f770878570b1f5b1db044ba626e0f6989acc3f >> Signed-off-by: Kirthik Srinivasan >> Signed-off-by: Rajith Cherian >> Signed-off-by: Sreedhar Sambangi >> --- >> drivers/regulator/Kconfig | 7 + >> drivers/regulator/Makefile | 1 + >> drivers/regulator/ipq4019-regulator.c | 275 >> ++++++++++++++++++++++++++++++++++ > > It'd be good to have the file name have qcom prepended. See the other > qcom > regulators as an example. Sure, but is it reasonable to say qcom_ipq4019-regulator ? Since as of now ,this regulator driver is supporting only ipq4019 SOC. > >> 3 files changed, 283 insertions(+) >> create mode 100644 drivers/regulator/ipq4019-regulator.c >> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index c77dc08..bb44873 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -843,5 +843,12 @@ config REGULATOR_WM8994 >> This driver provides support for the voltage regulators on the >> WM8994 CODEC. >> >> +config REGULATOR_IPQ4019 > > How bout REGULATOR_QCOM_IPQ4019. Sounds good, Will update in V2 >> + tristate "IPQ4019 regulator support" >> + depends on ARCH_QCOM >> + help >> + This driver provides support for the voltage regulators of the >> + IPQ40xx devices. >> + >> endif >> > > > >> +static int ipq4019_regulator_set_voltage(struct regulator_dev *dev, >> + int min_uV, int max_uV, >> + unsigned *selector) >> +{ >> + struct ipq4019_regulator_data *data = rdev_get_drvdata(dev); >> + struct ipq4019_regulator_config *cfg = data->config; >> + >> + int ptr, best_val = INT_MAX, val; >> + >> + for (ptr = 0; ptr < cfg->nr_states; ptr++) >> + if (cfg->states[ptr].range < best_val && >> + cfg->states[ptr].range >= min_uV && >> + cfg->states[ptr].range <= max_uV) { >> + best_val = cfg->states[ptr].value; >> + if (selector) >> + *selector = ptr; >> + } >> + >> + if (best_val == INT_MAX) >> + return -EINVAL; >> + >> + val = readl(cfg->base); >> + val = val & (~(cfg->mask)); > > val &= mask Thank you, Will fix this > >> + writel((val | best_val), cfg->base); >> + >> + data->range = cfg->states[ptr].range; >> + >> + return 0; >> +} >> + >> +static int ipq4019_regulator_list_voltage(struct regulator_dev *dev, >> + unsigned selector) >> +{ >> + struct ipq4019_regulator_data *data = rdev_get_drvdata(dev); >> + struct ipq4019_regulator_config *cfg = data->config; >> + >> + if (selector >= cfg->nr_states) >> + return -EINVAL; >> + >> + return cfg->states[selector].range; > > If you can define your ranges using LINEAR_RANGE, you can just use the > regulator_list_voltage_linear_range. Since we are supporting only two states , haven't gone through the approach of LINEAR_RANGE.but sure will look in to it. > >> +} >> + >> +static struct regulator_ops ipq4019_regulator_voltage_ops = { >> + .get_voltage = ipq4019_regulator_get_voltage, >> + .set_voltage = ipq4019_regulator_set_voltage, >> + .list_voltage = ipq4019_regulator_list_voltage, > > No enable and disable? There is no such states like enable and disable. > >> +}; >> + >> +static struct ipq4019_regulator_config * >> +of_get_ipq4019_regulator_data(struct device *dev, struct device_node >> *np, const struct regulator_desc *desc) >> +{ >> + struct ipq4019_regulator_config *config; >> + struct property *prop; >> + int proplen, i; >> + >> + config = devm_kzalloc(dev, >> + sizeof(struct ipq4019_regulator_config), >> + GFP_KERNEL); >> + if (!config) >> + return ERR_PTR(-ENOMEM); >> + >> + config->init_data = of_get_regulator_init_data(dev, np, desc); >> + if (!config->init_data) >> + return ERR_PTR(-EINVAL); >> + >> + config->supply_name = config->init_data->constraints.name; >> + >> + >> + /* Fetch states. */ >> + prop = of_find_property(np, "states", NULL); >> + if (!prop) { >> + dev_err(dev, "No 'states' property found\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + proplen = prop->length / sizeof(int); >> + >> + config->states = devm_kzalloc(dev, >> + sizeof(struct ipq4019_regulator_state) >> + * (proplen / 2), >> + GFP_KERNEL); >> + if (!config->states) >> + return ERR_PTR(-ENOMEM); >> + >> + for (i = 0; i < proplen / 2; i++) { >> + config->states[i].range = >> + be32_to_cpup((int *)prop->value + (i * 2)); >> + config->states[i].value = >> + be32_to_cpup((int *)prop->value + (i * 2 + 1)); >> + } >> + config->nr_states = i; > > Is it necessary to encode all of this data in the DT? Is this varied > between > boards using the IPQ4019? Or are these values fixed for this chip? If > they are > fixed, it'd be better to put the data in a static structure or see if > the > REGULATOR_LINEAR_RANGE would work for you. > >> + >> + prop = of_find_property(np, "mask", NULL); >> + if (!prop) { >> + dev_err(dev, "No 'states' property found\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + config->mask = be32_to_cpup((int *)prop->value); >> + config->type = REGULATOR_VOLTAGE; >> + >> + return config; >> +} >> + > > > > How many of these LDOs are being provided? Only one sw configurabale LDO > > Regards, > > Andy Gross -- -Sree