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

[Andy Gross <andy.gross@linaro.org>]

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.

>  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.
> +	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

> +	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.

> +}
> +
> +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?

> +};
> +
> +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?

Regards,

Andy Gross