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

[Sreedhar Sambangi]

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