Re: [PATCH 2/2] qcom: ipq4019: Add LDO regulator driver for SDHC controller

[Sreedhar Sambangi <ssambang@codeaurora.org>]

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@codeaurora.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@codeaurora.org>
>> Signed-off-by: Rajith Cherian <rajith@codeaurora.org>
>> Signed-off-by: Sreedhar Sambangi <ssambang@codeaurora.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