Re: [PATCH v7 3/7] regulator: add pbias regulator support

[Balaji T K <balajitk@ti.com>]

On Monday 06 January 2014 11:46 PM, Mark Rutland wrote:
> On Fri, Dec 20, 2013 at 05:35:51PM +0000, Balaji T K wrote:
>> pbias register controls internal power supply to sd card i/o pads
>> in most OMAPs (OMAP2-5, DRA7).
>> Control bits for selecting voltage level and
>> enabling/disabling are in the same PBIAS register.
>>
>> Signed-off-by: Balaji T K <balajitk@ti.com>
>> ---
>>   .../bindings/regulator/pbias-regulator.txt         |   16 ++
>>   drivers/regulator/Kconfig                          |    9 +
>>   drivers/regulator/Makefile                         |    1 +
>>   drivers/regulator/pbias-regulator.c                |  261 ++++++++++++++++++++
>>   4 files changed, 287 insertions(+), 0 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/pbias-regulator.txt
>>   create mode 100644 drivers/regulator/pbias-regulator.c
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/pbias-regulator.txt b/Documentation/devicetree/bindings/regulator/pbias-regulator.txt
>> new file mode 100644
>> index 0000000..359d3f9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/pbias-regulator.txt
>> @@ -0,0 +1,16 @@
>> +PBIAS internal regulator for SD card dual voltage i/o pads on OMAP SoCs.
>> +
>> +Required properties:
>> +- compatible:
>> +  - "ti,pbias-omap" for OMAP2, OMAP3, OMAP4, OMAP5, DRA7
>> +
>> +Optional properties:
>> +- Any optional property defined in bindings/regulator/regulator.txt
>> +
>> +Example:
>> +
>> +		pbias_regulator: pbias_regulator {
>> +			regulator-name = "pbias_mmc_omap4";
>> +			regulator-min-microvolt = <1800000>;
>> +			regulator-max-microvolt = <3000000>;
>> +	};
>
> This doesn't look like a full example. I don't see the required
> compatible string.
>

Thanks for pointing, I missed it, will add to example.

> [...]
>
>> +static struct of_regulator_match pbias_matches[] = {
>> +	{ .name = "pbias_mmc_omap2430", .driver_data = (void *)&pbias_mmc_omap2430},
>> +	{ .name = "pbias_sim_omap3", .driver_data = (void *)&pbias_sim_omap3},
>> +	{ .name = "pbias_mmc_omap4", .driver_data = (void *)&pbias_mmc_omap4},
>> +	{ .name = "pbias_mmc_omap5", .driver_data = (void *)&pbias_mmc_omap5},
>> +};
>
> These weren't documented.

Ok, will add it

>
> [...]
>
>> +static int pbias_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct device_node *dev_node;
>> +	struct pbias_regulator_data *drvdata;
>> +	struct resource *res;
>> +	struct regulator_config cfg = { };
>> +	struct regmap *syscon;
>> +	const struct pbias_reg_info *info;
>> +	int ret = 0;
>> +	int count, idx, data_idx = 0;
>> +
>> +	count = of_regulator_match(&pdev->dev, np, pbias_matches,
>> +						PBIAS_NUM_REGS);
>> +	if (count < 0)
>> +		return count;
>> +
>> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct pbias_regulator_data)
>> +			       * count, GFP_KERNEL);
>> +	if (drvdata == NULL) {
>> +		dev_err(&pdev->dev, "Failed to allocate device data\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	dev_node = of_get_parent(np);
>> +	if (!dev_node)
>> +		return -ENODEV;
>> +
>> +	syscon = syscon_node_to_regmap(dev_node);
>
> If we're relying on a particular parent node, that _must_ be described
> in the binding.

Ok
>
>> +	of_node_put(dev_node);
>> +	if (IS_ERR(syscon))
>> +		return PTR_ERR(syscon);
>> +
>> +	cfg.dev = &pdev->dev;
>> +
>> +	for (idx = 0; idx < PBIAS_NUM_REGS, data_idx < count; idx++) {
>> +		if (!pbias_matches[idx].init_data ||
>> +			!pbias_matches[idx].of_node)
>> +			continue;
>> +
>> +		info = pbias_matches[idx].driver_data;
>> +		if (!info)
>> +			return -ENODEV;
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> A reg entry was not mentioned in the binding.

Ok, will add it

>
> Why is this inside the loop? It's grabbing the reg from the parent, not
> each of the children it's walking over.
>

The reg property is same for each children and is needed for each regulator.
It is for the case where a single register can control 2
regulators as in OMAP3 (mmc and sim). In other SoCs, only one regulator
of "ti,pbias-omap" compatible exists in the pbias register.

> Thanks,
> Mark.
>