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

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:

quoted

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 <redacted>
---
  .../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.

[...]

quoted

+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

[...]

quoted

+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

quoted

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