| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 05 Apr 2016 22:02:28 -0700
From: Sreedhar Sambangi <ssambang@...eaurora.org>
To: Andy Gross <andy.gross@...aro.org>
Cc: linux-arm-msm@...r.kernel.org,
qca-upstream.external@....qualcomm.com, lgirdwood@...il.com,
broonie@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] qcom: ipq4019: Add LDO regulator driver for SDHC
controller
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 <kirthik@...eaurora.org>
>>
>> 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 <kirthik@...eaurora.org>
>> Signed-off-by: Rajith Cherian <rajith@...eaurora.org>
>> Signed-off-by: Sreedhar Sambangi <ssambang@...eaurora.org>
>> ---
>> 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
>>
>
> <snip>
>
>> +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;
>> +}
>> +
>
> <snip>
>
> How many of these LDOs are being provided?
Only one sw configurabale LDO
>
> Regards,
>
> Andy Gross
--
-Sree
Powered by blists - more mailing lists