[PATCH 1/2] regulator: max1586 add device-tree support

[PATCH 1/2] regulator: max1586 add device-tree support

[Robert Jarzmik]

Add device-tree support to max1586.
The driver can still be used with the legacy platform data, or the new
device-tree way.

This work is heavily inspired by the device-tree support of its cousin
max8660 driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/regulator/max1586.c       | 74 +++++++++++++++++++++++++++++++++++++--
 include/linux/regulator/max1586.h |  2 +-
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index d23d057..0b04602 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -24,6 +24,8 @@
 #include <linux/regulator/driver.h>
 #include <linux/slab.h>
 #include <linux/regulator/max1586.h>
+#include <linux/of_device.h>
+#include <linux/regulator/of_regulator.h>
 
 #define MAX1586_V3_MAX_VSEL 31
 #define MAX1586_V6_MAX_VSEL 3
@@ -157,13 +159,80 @@ static struct regulator_desc max1586_reg[] = {
 	},
 };
 
+int of_get_max1586_platform_data(struct device *dev,
+				 struct max1586_platform_data *pdata)
+{
+	struct max1586_subdev_data *sub;
+	struct of_regulator_match rmatch[ARRAY_SIZE(max1586_reg)];
+	struct device_node *np = dev->of_node;
+	int i, matched;
+
+	if (of_property_read_u32(np, "v3-gain",
+				 &pdata->v3_gain) < 0) {
+		dev_err(dev, "%s has no 'v3-gain' property\n", np->full_name);
+		return -EINVAL;
+	}
+
+	np = of_get_child_by_name(np, "regulators");
+	if (!np) {
+		dev_err(dev, "missing 'regulators' subnode in DT\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(rmatch); i++)
+		rmatch[i].name = max1586_reg[i].name;
+
+	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
+	of_node_put(np);
+	if (matched <= 0)
+		return matched;
+
+	pdata->subdevs = devm_kzalloc(dev, sizeof(struct max1586_subdev_data) *
+						matched, GFP_KERNEL);
+	if (!pdata->subdevs)
+		return -ENOMEM;
+
+	pdata->num_subdevs = matched;
+	sub = pdata->subdevs;
+
+	for (i = 0; i < matched; i++) {
+		sub->id = i;
+		sub->name = rmatch[i].of_node->name;
+		sub->platform_data = rmatch[i].init_data;
+		sub++;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id max1586_of_match[] = {
+	{ .compatible = "maxim,max1586", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max1586_of_match);
+
 static int max1586_pmic_probe(struct i2c_client *client,
 					const struct i2c_device_id *i2c_id)
 {
-	struct max1586_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct max1586_platform_data *pdata, pdata_of;
 	struct regulator_config config = { };
 	struct max1586_data *max1586;
-	int i, id;
+	int i, id, ret;
+	const struct of_device_id *match;
+
+	pdata = dev_get_platdata(&client->dev);
+	if (client->dev.of_node && !pdata) {
+		match = of_match_device(of_match_ptr(max1586_of_match),
+					&client->dev);
+		if (!match) {
+			dev_err(&client->dev, "Error: No device match found\n");
+			return -ENODEV;
+		}
+		ret = of_get_max1586_platform_data(&client->dev, &pdata_of);
+		if (ret < 0)
+			return ret;
+		pdata = &pdata_of;
+	}
 
 	max1586 = devm_kzalloc(&client->dev, sizeof(struct max1586_data),
 			GFP_KERNEL);
@@ -229,6 +298,7 @@ static struct i2c_driver max1586_pmic_driver = {
 	.driver		= {
 		.name	= "max1586",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(max1586_of_match),
 	},
 	.id_table	= max1586_id,
 };
diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
index de9a7fa..cedd0fe 100644
--- a/include/linux/regulator/max1586.h
+++ b/include/linux/regulator/max1586.h
@@ -40,7 +40,7 @@
  */
 struct max1586_subdev_data {
 	int				id;
-	char				*name;
+	const char			*name;
 	struct regulator_init_data	*platform_data;
 };
 
-- 
2.0.0.rc2

[PATCH 2/2] regulator: max1586 add device-tree support

[Robert Jarzmik]

Add max1586 regulator device-tree bindings documentation.

Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
---
 .../bindings/regulator/max1586-regulator.txt       | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/max1586-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/max1586-regulator.txt b/Documentation/devicetree/bindings/regulator/max1586-regulator.txt
new file mode 100644
index 0000000..c050c17
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/max1586-regulator.txt
@@ -0,0 +1,28 @@
+Maxim MAX1586 voltage regulator
+
+Required properties:
+- compatible: must be "maxim,max1586"
+- reg: I2C slave address, usually 0x14
+- v3-gain: integer specifying the V3 gain as per datasheet
+           (1 + R24/R25 + R24/185.5kOhm)
+- any required generic properties defined in regulator.txt
+
+Example:
+
+	i2c_master {
+		max1586@14 {
+			compatible = "maxim,max1586";
+			reg = <0x14>;
+			v3-gain = <1000000>;
+
+			regulators {
+				vcc_core: v3 {
+					regulator-name = "vcc_core";
+					regulator-compatible = "Output_V3";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <1705000>;
+					regulator-always-on;
+				};
+			};
+		};
+	};
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Sat, Jun 14, 2014 at 04:54:24PM +0200, Robert Jarzmik wrote:

> +	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
> +	of_node_put(np);
> +	if (matched <= 0)
> +		return matched;

Why is this treating zero as an error?  We should be able to at least
report the current state of regulators even if none are configured in
the device tree.

Otherwise this looks good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Sat, Jun 14, 2014 at 04:54:24PM +0200, Robert Jarzmik wrote:
>
>> +	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
>> +	of_node_put(np);
>> +	if (matched <= 0)
>> +		return matched;
>
> Why is this treating zero as an error?  We should be able to at least
> report the current state of regulators even if none are configured in
> the device tree.

Euh how so an error ?

If 0 is returned, this means no regulators are found in device-tree. It's not an
error, it's a lack of regulators (ie. no Output_V3 and no Output_V6), and no
more handling is necessary in this function, while returning "ok", ie 0 ...

As for the "state report", this max1586 doesn't report anything, it cannot even
be queried about the current voltage, sic ...

If you want me to modify this bit I need a bit more of an explanation to
understand.

Cheers.

-- 
Robert

Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On Tue, Jun 17, 2014 at 09:16:52PM +0200, Robert Jarzmik wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:
> > On Sat, Jun 14, 2014 at 04:54:24PM +0200, Robert Jarzmik wrote:

> >> +	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
> >> +	of_node_put(np);
> >> +	if (matched <= 0)
> >> +		return matched;

> > Why is this treating zero as an error?  We should be able to at least
> > report the current state of regulators even if none are configured in
> > the device tree.

> Euh how so an error ?

> If 0 is returned, this means no regulators are found in device-tree. It's not an
> error, it's a lack of regulators (ie. no Output_V3 and no Output_V6), and no
> more handling is necessary in this function, while returning "ok", ie 0 ...

OK, so there's just nothing to do in that case.  That's fine, but it's
just not at all clear from the code.  A comment would help.

> As for the "state report", this max1586 doesn't report anything, it cannot even
> be queried about the current voltage, sic ...

It can't?  That's unfortunate, though I was able to turn up a datasheet
which appears to support that.

> If you want me to modify this bit I need a bit more of an explanation to
> understand.

Where the driver is doing unusual things if they are actually sensible
then the change needs to be clearer about why.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Tue, Jun 17, 2014 at 09:16:52PM +0200, Robert Jarzmik wrote:
>> Mark Brown <broonie@kernel.org> writes:
>> > On Sat, Jun 14, 2014 at 04:54:24PM +0200, Robert Jarzmik wrote:
>
>> >> +	matched = of_regulator_match(dev, np, rmatch, ARRAY_SIZE(rmatch));
>> >> +	of_node_put(np);
>> >> +	if (matched <= 0)
>> >> +		return matched;
>
>> > Why is this treating zero as an error?  We should be able to at least
>> > report the current state of regulators even if none are configured in
>> > the device tree.
>
>> Euh how so an error ?
>
>> If 0 is returned, this means no regulators are found in device-tree. It's not an
>> error, it's a lack of regulators (ie. no Output_V3 and no Output_V6), and no
>> more handling is necessary in this function, while returning "ok", ie 0 ...
>
> OK, so there's just nothing to do in that case.  That's fine, but it's
> just not at all clear from the code.  A comment would help.
OK, no problem.

>
>> As for the "state report", this max1586 doesn't report anything, it cannot even
>> be queried about the current voltage, sic ...
>
> It can't?  That's unfortunate, though I was able to turn up a datasheet
> which appears to support that.
Oh really ? Well, tell me where you read it.

My personal reading from the Max1586 specs is (page 21, chapter "Serial Interface") :
    The LSB of the address word is the read/write (R/W) bit.
    R/W indicates whether the master is writing or reading
    (RD/W 0 = write, RD/W 1 = read). The MAX1586/
    MAX1587 only support the SEND BYTE format; there-
    fore, RD/W is required to be 0.

I'm wondering if you have this sentence in your datasheet too.

>> If you want me to modify this bit I need a bit more of an explanation to
>> understand.
>
> Where the driver is doing unusual things if they are actually sensible
> then the change needs to be clearer about why.
So would a comment like this address your comment ?

	/* Either matched < 0 and return the error. Or matched is 0 which means
	 * no init data was found, ie. no regulator is configured, and return 0
	 * to caller, stating neither error nor any matched regulator.
	 */

	if (matched <= 0)
		return matched;

Cheers.

-- 
Robert

Re: [PATCH 1/2] regulator: max1586 add device-tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Tue, Jun 24, 2014 at 08:05:30PM +0200, Robert Jarzmik wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> >> As for the "state report", this max1586 doesn't report anything, it cannot even
> >> be queried about the current voltage, sic ...

> > It can't?  That's unfortunate, though I was able to turn up a datasheet
> > which appears to support that.

> Oh really ? Well, tell me where you read it.

I said I *was* able to turn up a datasheet which appeared to support
that.

> >> If you want me to modify this bit I need a bit more of an explanation to
> >> understand.

> > Where the driver is doing unusual things if they are actually sensible
> > then the change needs to be clearer about why.

> So would a comment like this address your comment ?

> 	/* Either matched < 0 and return the error. Or matched is 0 which means
> 	 * no init data was found, ie. no regulator is configured, and return 0
> 	 * to caller, stating neither error nor any matched regulator.
> 	 */
> 	if (matched <= 0)
> 		return matched;

Can you include something like "...and we have no readback support in
the device so can't report status" or something please?  That's the key
bit - the point is that even unconfigured regulators should normally be
registered.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]