Add ltc3562 voltage regulator driver

Add ltc3562 voltage regulator driver

[Mike Looijmans]

This patch adds the LTC3562 I2C controlled voltage regulator to
the kernel. The driver was tested and developed on a Topic Miami
board where this chip supplies the IO voltages for the FPGA pins.

[PATCH] Add ltc3562 voltage regulator driver

[Mike Looijmans]

The ltc3562 is an I2C controlled regulator supporting 4 independent
outputs.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/regulator/Kconfig   |    7 +
 drivers/regulator/Makefile  |    1 +
 drivers/regulator/ltc3562.c |  387 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 395 insertions(+)
 create mode 100644 drivers/regulator/ltc3562.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 55d7b7b..4012601 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -300,6 +300,13 @@ config REGULATOR_LP8788
 	help
 	  This driver supports LP8788 voltage regulator chip.
 
+config REGULATOR_LTC3562
+	tristate "LCT3562 quad output voltage regulator"
+	depends on I2C
+	help
+	  This enables support for the LCT3562 quad output voltage regulator
+	  controlled via I2C.
+
 config REGULATOR_LTC3589
 	tristate "LTC3589 8-output voltage regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 1029ed3..9a15146 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-buck.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-ldo.o
 obj-$(CONFIG_REGULATOR_LP8755) += lp8755.o
+obj-$(CONFIG_REGULATOR_LTC3562) += ltc3562.o
 obj-$(CONFIG_REGULATOR_LTC3589) += ltc3589.o
 obj-$(CONFIG_REGULATOR_MAX14577) += max14577.o
 obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
diff --git a/drivers/regulator/ltc3562.c b/drivers/regulator/ltc3562.c
new file mode 100644
index 0000000..be111bc
--- /dev/null
+++ b/drivers/regulator/ltc3562.c
@@ -0,0 +1,387 @@
+/*
+ * Regulator driver for Linear Technology LTC3562
+ *
+ *  Copyright (C) 2014 Topic Embedded Products
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/slab.h>
+#include <linux/regulator/of_regulator.h>
+
+#define LTC3562_NUM_REGULATORS	4
+
+#define REGULATOR_TYPE_A_NUM_VOLTAGES	16
+#define REGULATOR_TYPE_A_MIN_UV	425000
+#define REGULATOR_TYPE_A_UV_STEP	25000
+
+#define REGULATOR_TYPE_B_NUM_VOLTAGES	128
+#define REGULATOR_TYPE_B_MIN_UV	600000
+#define REGULATOR_TYPE_B_UV_STEP	25000
+
+struct ltc3562_status {
+	u8 shadow_addr;		/* Shadow register of address byte */
+	u8 shadow_data;		/* Shadow register of data byte */
+	u8 voltage_set;		/* Check if voltage has been set before */
+};
+
+struct ltc3562 {
+	struct device *dev;
+	struct i2c_client *i2c;
+	struct mutex mutex;
+	struct regulator_dev *rdev[LTC3562_NUM_REGULATORS];
+	struct ltc3562_status rstatus[LTC3562_NUM_REGULATORS];
+};
+
+static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b);
+static int ltc3562_dummy_write(struct i2c_client *i2c);
+
+#define LTC3562_R400B_ID	0x00
+#define LTC3562_R600B_ID	0x01
+#define LTC3562_R400A_ID	0x02
+#define LTC3562_R600A_ID	0x03
+
+#define OPERATING_MODE_MASK	0x03
+
+#define PROGRAM_R400B_MASK	0x10
+#define PROGRAM_R600B_MASK	0x20
+#define PROGRAM_R400A_MASK	0x40
+#define PROGRAM_R600A_MASK	0x80
+
+#define OPERATING_MODE_PULSE_SKIP	0x00
+#define OPERATING_MODE_LDO		0x01
+#define OPERATING_MODE_FOCED_BURST	0x02
+#define OPERATING_MODE_BURST		0x03
+
+#define REGULATOR_ENABLE_BIT	0x80
+
+
+static int ltc3562_regulator_enable(struct regulator_dev *dev)
+{
+	int ret, v_index;
+	unsigned int v_default;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	if (!status->voltage_set) {
+		if (of_property_read_u32(dev->dev.of_node,
+			"ltc3562-default-voltage", &v_default) == 0) {
+			v_index = dev->desc->ops->map_voltage(
+				dev, v_default, v_default);
+			if (v_index > 0)
+				status->shadow_data =
+					v_index & (~REGULATOR_ENABLE_BIT);
+			status->voltage_set = 1;
+		}
+	}
+
+	status->shadow_data |= REGULATOR_ENABLE_BIT;
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr | PROGRAM_R400B_MASK << id,
+		status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_disable(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->shadow_data &= ~(REGULATOR_ENABLE_BIT);
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr, status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_is_enabled(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = (status->shadow_data & REGULATOR_ENABLE_BIT) ? 1 : 0;
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_set_voltage_sel(struct regulator_dev *dev,
+	unsigned int selector)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->shadow_data =
+		(status->shadow_data & REGULATOR_ENABLE_BIT) | selector;
+	status->voltage_set = 1;
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr, status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_get_voltage_sel(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = status->shadow_data & ~(REGULATOR_ENABLE_BIT);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static struct regulator_ops ltc3562_regulator_ops = {
+	.is_enabled = ltc3562_regulator_is_enabled,
+	.enable = ltc3562_regulator_enable,
+	.disable = ltc3562_regulator_disable,
+	.set_voltage_sel = ltc3562_set_voltage_sel,
+	.get_voltage_sel = ltc3562_get_voltage_sel,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+};
+
+static struct regulator_desc ltc3562_regulators[] = {
+	{
+		.name = "R400B",
+		.id = LTC3562_R400B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600B",
+		.id = LTC3562_R600B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R400A",
+		.id = LTC3562_R400A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600A",
+		.id = LTC3562_R600A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+};
+
+
+static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b)
+{
+	return i2c_smbus_write_byte_data(i2c, reg_a, reg_b);
+}
+
+/* Write dummy data to detect presence of physical device */
+static int ltc3562_dummy_write(struct i2c_client *i2c)
+{
+	return ltc3562_write(i2c, 0, 0);
+}
+
+
+static int ltc3562_regulators_unregister(struct ltc3562 *ltc3562,
+	int num_regulators)
+{
+	while (--num_regulators >= 0) {
+		pr_debug("Unregistering regulator: %s\n",
+			ltc3562->rdev[num_regulators]->desc->name);
+		regulator_unregister(ltc3562->rdev[num_regulators]);
+	}
+	return 0;
+}
+
+static int ltc3562_i2c_probe(struct i2c_client *i2c,
+	const struct i2c_device_id *id)
+{
+	int i, error;
+	unsigned int r1, r2;
+	unsigned long uV;
+	struct ltc3562 *ltc3562;
+	struct regulator_config rconfig = { };
+	struct device_node *np_regulators, *np;
+	struct device_node *np_child;
+
+	ltc3562 = devm_kzalloc(&i2c->dev,
+		sizeof(struct ltc3562), GFP_KERNEL);
+	if (ltc3562 == NULL)
+		return -ENOMEM;
+
+	ltc3562->i2c = i2c;
+
+	mutex_init(&ltc3562->mutex);
+
+	if (ltc3562_dummy_write(i2c) < 0) {
+		dev_err(&i2c->dev,
+			"Could not find device LTC3562 on i2c bus\n");
+		error = -ENODEV;
+		goto lbl_exit;
+	}
+
+	np = of_node_get(i2c->dev.of_node);
+	np_regulators = of_get_child_by_name(np, "regulators");
+
+	if (np_regulators == NULL) {
+		dev_err(&i2c->dev, "Could not find regulators node\n");
+		error = -EINVAL;
+		goto lbl_exit;
+	}
+
+	for (i = 0; i < LTC3562_NUM_REGULATORS; ++i) {
+		np_child = of_get_child_by_name(np_regulators,
+			ltc3562_regulators[i].name);
+		if (np_child == NULL) {
+			dev_err(&i2c->dev,
+				"Could not find regulator data for %s in devicetree\n",
+				ltc3562_regulators[i].name);
+			error = -EINVAL;
+			goto lbl_cleanup;
+		}
+
+		if ((ltc3562_regulators[i].id == LTC3562_R400A_ID) ||
+			(ltc3562_regulators[i].id == LTC3562_R600A_ID)) {
+			/* If regulator is A type get resistor values */
+			error = of_property_read_u32(np_child,
+				"ltc3562-A-r1", &r1);
+			if (error) {
+				dev_err(&i2c->dev,
+					"Resistor value ltc3562-A-r1 for A-Type regulator %s not defined\n",
+					ltc3562_regulators[i].name);
+				goto lbl_cleanup;
+			}
+
+			error = of_property_read_u32(np_child,
+				"ltc3562-A-r2", &r2);
+			if (error) {
+				dev_err(&i2c->dev,
+					"Resistor value ltc3562-A-r2 for A-Type regulator %s not defined\n",
+					ltc3562_regulators[i].name);
+				goto lbl_cleanup;
+			}
+			uV = ltc3562_regulators[i].min_uV / 1000;
+			ltc3562_regulators[i].min_uV =
+				(uV + mult_frac(uV, r1, r2)) * 1000;
+
+			uV = ltc3562_regulators[i].uV_step / 1000;
+			ltc3562_regulators[i].uV_step =
+				(uV + mult_frac(uV, r1, r2)) * 1000;
+		}
+
+		rconfig.dev = &i2c->dev;
+		rconfig.init_data =
+			of_get_regulator_init_data(&i2c->dev, np_child);
+		rconfig.driver_data = ltc3562;
+		rconfig.of_node = np_child;
+
+		ltc3562->rstatus[i].voltage_set = 0;
+
+		ltc3562->rdev[i] =
+			regulator_register(&ltc3562_regulators[i], &rconfig);
+
+		if (IS_ERR(ltc3562->rdev[i])) {
+			error = PTR_ERR(ltc3562->rdev[i]);
+			dev_err(&i2c->dev,
+				"could not register regulator, Error %d\n",
+				error);
+			goto lbl_cleanup;
+		}
+	}
+
+	i2c_set_clientdata(i2c, ltc3562);
+	dev_dbg(&i2c->dev, "LTC3562 Driver loaded.\n");
+	return 0;
+
+lbl_cleanup:
+	ltc3562_regulators_unregister(ltc3562, i);
+lbl_exit:
+	return error;
+}
+
+static int ltc3562_i2c_remove(struct i2c_client *i2c)
+{
+	struct ltc3562 *ltc3562 = i2c_get_clientdata(i2c);
+
+	ltc3562_regulators_unregister(ltc3562, LTC3562_NUM_REGULATORS);
+
+	return 0;
+}
+
+static const struct i2c_device_id ltc3562_i2c_id[] = {
+	{ "ltc3562", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ltc3562_i2c_id);
+
+static struct i2c_driver ltc3562_i2c_driver = {
+	.driver = {
+		.name = "LTC3562",
+		.owner = THIS_MODULE,
+	},
+	.probe    = ltc3562_i2c_probe,
+	.remove   = ltc3562_i2c_remove,
+	.id_table = ltc3562_i2c_id,
+};
+
+module_i2c_driver(ltc3562_i2c_driver);
+
+MODULE_DESCRIPTION("LTC3562 Regulator Driver");
+MODULE_AUTHOR("auryn.verwegen@topic.nl");
+MODULE_AUTHOR("mike.looijmans@topic.nl");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Wed, Oct 29, 2014 at 09:15:59AM +0100, Mike Looijmans wrote:
> This patch adds the LTC3562 I2C controlled voltage regulator to
> the kernel. The driver was tested and developed on a Topic Miami
> board where this chip supplies the IO voltages for the FPGA pins.

Please don't send cover letters for single patches, if there's anything
that needs to be added just put it in the changelog or after the --- if
it shouldn't go there.

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

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Wed, Oct 29, 2014 at 09:16:00AM +0100, Mike Looijmans wrote:

> +	if (!status->voltage_set) {
> +		if (of_property_read_u32(dev->dev.of_node,
> +			"ltc3562-default-voltage", &v_default) == 0) {

A couple of problems here:

 - This contains DT code but no DT bindings documentation; the binding
   documentation is mandatory for any new bindings.
 - It's not obvious why a "default voltage" property would be device
   specific - what is this for and why is it being added in an
   individual device driver?

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

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 10/29/2014 01:30 PM, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 09:16:00AM +0100, Mike Looijmans wrote:
>
>> +	if (!status->voltage_set) {
>> +		if (of_property_read_u32(dev->dev.of_node,
>> +			"ltc3562-default-voltage", &v_default) == 0) {
>
> A couple of problems here:
>
>   - This contains DT code but no DT bindings documentation; the binding
>     documentation is mandatory for any new bindings.

Should I submit a new patch to add the DT bindings and CC to the dt mailing 
list, or do you want it included in this patch? I was basically waiting for 
feedback on the driver first.

>   - It's not obvious why a "default voltage" property would be device
>     specific - what is this for and why is it being added in an
>     individual device driver?

As for the "why" part: The default is used when there isn't a consumer 
available. It'll typically be combined with "always-on". When the output is 
used for say an MMC card, the consumer will ask for a specific voltage. When 
used to regulate the IO voltage of an FPGA bank (as is the case on our board), 
there isn't a single consumer to request a particular voltage, but the min and 
max will represent the range that the IO banks support (e.g. 1v8..3v3 or 
1v2..1v8), because selecting voltages outside this range may damage the FPGA 
hardware. This allows a devicetree overlay to specify a different value for a 
different board or application, while still being able to restrict the range 
to what the hardware connected to the regulator supports.

The "default voltage" may not be device specific, but regulator.txt doesn't 
have a property that describes what we needed here, because we can't get away 
with specifying min=max as fixed-regulator does. We also cannot assume that 
default=min or default=max, because that might harm hardware connected to the 
other side of the IO bank.

I noticed after rebasing to mainline that there is now also a driver for a 
similar chip (similar in function, that is), the ltc3589, which requires 
similar properties like the resistor dividers. I think it would be good to use 
similar names in the ltc3562 driver. I'll do that first and submit a v2 patch, 
along with the DT description.

Thank you for your feedback,
Mike.



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Thu, Oct 30, 2014 at 07:47:44AM +0100, Mike Looijmans wrote:
> On 10/29/2014 01:30 PM, Mark Brown wrote:

> >A couple of problems here:

> >  - This contains DT code but no DT bindings documentation; the binding
> >    documentation is mandatory for any new bindings.

> Should I submit a new patch to add the DT bindings and CC to the dt mailing
> list, or do you want it included in this patch? I was basically waiting for
> feedback on the driver first.

I'm not going to review a driver with bindings without those bindings
being documented, it's a well documented requirement and apart from
anything else it's not really possible to review the code that manages
the bindings without seeing the bindings.

> The "default voltage" may not be device specific, but regulator.txt doesn't
> have a property that describes what we needed here, because we can't get
> away with specifying min=max as fixed-regulator does. We also cannot assume
> that default=min or default=max, because that might harm hardware connected
> to the other side of the IO bank.

No, if you need a generic property add it in the generic code rather
than just hacking a device specific property into your driver.

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

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 10/30/2014 11:15 AM, Mark Brown wrote:
> On Thu, Oct 30, 2014 at 07:47:44AM +0100, Mike Looijmans wrote:
>> On 10/29/2014 01:30 PM, Mark Brown wrote:
>
>>> A couple of problems here:
>
>>>   - This contains DT code but no DT bindings documentation; the binding
>>>     documentation is mandatory for any new bindings.
>
>> Should I submit a new patch to add the DT bindings and CC to the dt mailing
>> list, or do you want it included in this patch? I was basically waiting for
>> feedback on the driver first.
>
> I'm not going to review a driver with bindings without those bindings
> being documented, it's a well documented requirement and apart from
> anything else it's not really possible to review the code that manages
> the bindings without seeing the bindings.
>
>> The "default voltage" may not be device specific, but regulator.txt doesn't
>> have a property that describes what we needed here, because we can't get
>> away with specifying min=max as fixed-regulator does. We also cannot assume
>> that default=min or default=max, because that might harm hardware connected
>> to the other side of the IO bank.
>
> No, if you need a generic property add it in the generic code rather
> than just hacking a device specific property into your driver.

So I should add "regulator-default-voltage" to the generic code? That would 
indeed be better than trying to do it into this driver.

But would that need a separate patch for regulator core to add the property?

Mike.



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 10/30/2014 11:29 AM, Mike Looijmans wrote:
> On 10/30/2014 11:15 AM, Mark Brown wrote:
>> On Thu, Oct 30, 2014 at 07:47:44AM +0100, Mike Looijmans wrote:
>>> On 10/29/2014 01:30 PM, Mark Brown wrote:
>>
>>>> A couple of problems here:
>>
>>>>   - This contains DT code but no DT bindings documentation; the binding
>>>>     documentation is mandatory for any new bindings.
>>
>>> Should I submit a new patch to add the DT bindings and CC to the dt mailing
>>> list, or do you want it included in this patch? I was basically waiting for
>>> feedback on the driver first.
>>
>> I'm not going to review a driver with bindings without those bindings
>> being documented, it's a well documented requirement and apart from
>> anything else it's not really possible to review the code that manages
>> the bindings without seeing the bindings.
>>
>>> The "default voltage" may not be device specific, but regulator.txt doesn't
>>> have a property that describes what we needed here, because we can't get
>>> away with specifying min=max as fixed-regulator does. We also cannot assume
>>> that default=min or default=max, because that might harm hardware connected
>>> to the other side of the IO bank.
>>
>> No, if you need a generic property add it in the generic code rather
>> than just hacking a device specific property into your driver.
>
> So I should add "regulator-default-voltage" to the generic code? That would
> indeed be better than trying to do it into this driver.
>
> But would that need a separate patch for regulator core to add the property?

Hmm, I looked into doing that, but that isn't trivial, and too many devices 
will suffer.

Since default-voltage is unacceptable, I will remove it from the ltc3562 
driver and just use the min==max setting in the devicetree to reach the same 
effect. The driver that will match IO voltages between SOM and carrier is far 
from being ready for submission anyway.

I'll submit a v2 patch with all these things reworked soon.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Thu, Oct 30, 2014 at 11:53:37AM +0100, Mike Looijmans wrote:
> On 10/30/2014 11:29 AM, Mike Looijmans wrote:

> >So I should add "regulator-default-voltage" to the generic code? That would
> >indeed be better than trying to do it into this driver.

> >But would that need a separate patch for regulator core to add the property?

Yes.

> Hmm, I looked into doing that, but that isn't trivial, and too many devices
> will suffer.

Why would this have a negative effect on other devices?  Unless somehow
the other devices add the property they should not be affected by it; if
users do add the property presumably that's because it's doing something
useful.

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

[PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

The ltc3562 is an I2C controlled regulator supporting 4 independent
outputs.

v2: Prefix "lltc" to devicetree properties. Use the same property names
    as the ltc3589 driver. Remove default-voltage property. Use
    devm_register_regulator.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 .../devicetree/bindings/regulator/ltc3562.txt      |   56 ++++
 drivers/regulator/Kconfig                          |    7 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/ltc3562.c                        |  337 ++++++++++++++++++++
 4 files changed, 401 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/ltc3562.txt
 create mode 100644 drivers/regulator/ltc3562.c

diff --git a/Documentation/devicetree/bindings/regulator/ltc3562.txt b/Documentation/devicetree/bindings/regulator/ltc3562.txt
new file mode 100644
index 0000000..81ed923
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/ltc3562.txt
@@ -0,0 +1,56 @@
+Linear Technology LTC3562 I2C controlled 4-output regulator
+
+Required properties:
+- compatible: "ltc3562"
+- reg: I2C slave address
+
+Required child node:
+- regulators: Contains four regulator child nodes R400B, R600B, R400A, R600A,
+  specifying the initialization data as documented in
+  Documentation/devicetree/bindings/regulator/regulator.txt.
+
+Each regulator is defined using the standard binding for regulators. The
+nodes for R400A and R600A additionally need to specify the resistor values of
+their external feedback voltage dividers:
+
+Required properties (A-type only):
+- lltc,fb-voltage-divider: An array of two integers containing the resistor
+  values R1 and R2 of the feedback voltage divider. Both values must remain in
+  the range 1..1000, only their quotient matters.
+
+Example:
+
+	ltc3562: ltc3562@65 {
+		compatible = "ltc3562";
+		reg = <0x65>;
+		regulators {
+			R400B_reg: R400B {
+				regulator-name = "R400B";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			R600B_reg: R600B {
+				regulator-name = "R600B";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			R400A_reg: R400A {
+				regulator-name = "R400A";
+				regulator-min-microvolt = <425000>;
+				regulator-max-microvolt = <8000000>;
+				lltc,fb-voltage-divider = <1, 1>;
+			};
+			R600A_reg: R600A {
+				regulator-name = "R600A";
+				regulator-min-microvolt = <425000>;
+				regulator-max-microvolt = <1800000>;
+				lltc,fb-voltage-divider = <316, 100>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 55d7b7b..4012601 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -300,6 +300,13 @@ config REGULATOR_LP8788
 	help
 	  This driver supports LP8788 voltage regulator chip.
 
+config REGULATOR_LTC3562
+	tristate "LCT3562 quad output voltage regulator"
+	depends on I2C
+	help
+	  This enables support for the LCT3562 quad output voltage regulator
+	  controlled via I2C.
+
 config REGULATOR_LTC3589
 	tristate "LTC3589 8-output voltage regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 1029ed3..9a15146 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-buck.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-ldo.o
 obj-$(CONFIG_REGULATOR_LP8755) += lp8755.o
+obj-$(CONFIG_REGULATOR_LTC3562) += ltc3562.o
 obj-$(CONFIG_REGULATOR_LTC3589) += ltc3589.o
 obj-$(CONFIG_REGULATOR_MAX14577) += max14577.o
 obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
diff --git a/drivers/regulator/ltc3562.c b/drivers/regulator/ltc3562.c
new file mode 100644
index 0000000..b73eee7
--- /dev/null
+++ b/drivers/regulator/ltc3562.c
@@ -0,0 +1,337 @@
+/*
+ * Regulator driver for Linear Technology LTC3562
+ *
+ *  Copyright (C) 2014 Topic Embedded Products
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/slab.h>
+#include <linux/regulator/of_regulator.h>
+
+#define LTC3562_NUM_REGULATORS	4
+
+#define REGULATOR_TYPE_A_NUM_VOLTAGES	16
+#define REGULATOR_TYPE_A_MIN_UV	425000
+#define REGULATOR_TYPE_A_UV_STEP	25000
+
+#define REGULATOR_TYPE_B_NUM_VOLTAGES	128
+#define REGULATOR_TYPE_B_MIN_UV	600000
+#define REGULATOR_TYPE_B_UV_STEP	25000
+
+struct ltc3562_status {
+	u8 shadow_addr;	/* Shadow register of address byte */
+	u8 shadow_data;	/* Shadow register of data byte */
+};
+
+struct ltc3562 {
+	struct device *dev;
+	struct i2c_client *i2c;
+	struct mutex mutex;
+	struct regulator_dev *rdev[LTC3562_NUM_REGULATORS];
+	struct ltc3562_status rstatus[LTC3562_NUM_REGULATORS];
+};
+
+static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b);
+static int ltc3562_dummy_write(struct i2c_client *i2c);
+
+#define LTC3562_R400B_ID	0x00
+#define LTC3562_R600B_ID	0x01
+#define LTC3562_R400A_ID	0x02
+#define LTC3562_R600A_ID	0x03
+
+#define OPERATING_MODE_MASK	0x03
+
+#define PROGRAM_R400B_MASK	0x10
+#define PROGRAM_R600B_MASK	0x20
+#define PROGRAM_R400A_MASK	0x40
+#define PROGRAM_R600A_MASK	0x80
+
+#define OPERATING_MODE_PULSE_SKIP	0x00
+#define OPERATING_MODE_LDO		0x01
+#define OPERATING_MODE_FOCED_BURST	0x02
+#define OPERATING_MODE_BURST		0x03
+
+#define REGULATOR_ENABLE_BIT	0x80
+
+static int ltc3562_regulator_enable(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->shadow_data |= REGULATOR_ENABLE_BIT;
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr | PROGRAM_R400B_MASK << id,
+		status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_disable(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->shadow_data &= ~(REGULATOR_ENABLE_BIT);
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr, status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_is_enabled(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = (status->shadow_data & REGULATOR_ENABLE_BIT) ? 1 : 0;
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_set_voltage_sel(struct regulator_dev *dev,
+	unsigned int selector)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->shadow_data =
+		(status->shadow_data & REGULATOR_ENABLE_BIT) | selector;
+
+	ret = ltc3562_write(ltc3562->i2c,
+		status->shadow_addr, status->shadow_data);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_get_voltage_sel(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = status->shadow_data & ~(REGULATOR_ENABLE_BIT);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static struct regulator_ops ltc3562_regulator_ops = {
+	.is_enabled = ltc3562_regulator_is_enabled,
+	.enable = ltc3562_regulator_enable,
+	.disable = ltc3562_regulator_disable,
+	.set_voltage_sel = ltc3562_set_voltage_sel,
+	.get_voltage_sel = ltc3562_get_voltage_sel,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+};
+
+static struct regulator_desc ltc3562_regulators[] = {
+	{
+		.name = "R400B",
+		.id = LTC3562_R400B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600B",
+		.id = LTC3562_R600B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R400A",
+		.id = LTC3562_R400A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600A",
+		.id = LTC3562_R600A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+};
+
+
+static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b)
+{
+	dev_dbg(&i2c->dev, "%s(%#x, %#x)\n", __func__, reg_a, reg_b);
+	return i2c_smbus_write_byte_data(i2c, reg_a, reg_b);
+}
+
+/* Write dummy data to detect presence of physical device */
+static int ltc3562_dummy_write(struct i2c_client *i2c)
+{
+	return ltc3562_write(i2c, 0, 0);
+}
+
+static int ltc3562_i2c_probe(struct i2c_client *i2c,
+	const struct i2c_device_id *id)
+{
+	int i, error;
+	unsigned long uV;
+	struct ltc3562 *ltc3562;
+	struct regulator_config rconfig = { };
+	struct device_node *np_regulators, *np;
+	struct device_node *np_child;
+	u32 value;
+
+	ltc3562 = devm_kzalloc(&i2c->dev,
+		sizeof(struct ltc3562), GFP_KERNEL);
+	if (ltc3562 == NULL)
+		return -ENOMEM;
+
+	ltc3562->i2c = i2c;
+
+	mutex_init(&ltc3562->mutex);
+
+	if (ltc3562_dummy_write(i2c) < 0) {
+		dev_err(&i2c->dev,
+			"Could not find device LTC3562 on i2c bus\n");
+		error = -ENODEV;
+		goto lbl_exit;
+	}
+
+	np = of_node_get(i2c->dev.of_node);
+	np_regulators = of_get_child_by_name(np, "regulators");
+
+	if (np_regulators == NULL) {
+		dev_err(&i2c->dev, "Could not find regulators node\n");
+		error = -EINVAL;
+		goto lbl_exit;
+	}
+
+	for (i = 0; i < LTC3562_NUM_REGULATORS; ++i) {
+		np_child = of_get_child_by_name(np_regulators,
+			ltc3562_regulators[i].name);
+		if (np_child == NULL) {
+			dev_err(&i2c->dev,
+				"Could not find regulator data for %s in devicetree\n",
+				ltc3562_regulators[i].name);
+			return -EINVAL;
+		}
+
+		if ((ltc3562_regulators[i].id == LTC3562_R400A_ID) ||
+			(ltc3562_regulators[i].id == LTC3562_R600A_ID)) {
+			/* If regulator is A type get resistor values */
+			u32 vdiv[2];
+
+			error = of_property_read_u32_array(np_child,
+				"lltc,fb-voltage-divider", vdiv, 2);
+			if (error) {
+				dev_err(&i2c->dev,
+					"Failed to parse voltage divider: %d\n",
+					error);
+				return error;
+			}
+
+			uV = ltc3562_regulators[i].min_uV / 1000;
+			ltc3562_regulators[i].min_uV =
+				(uV + mult_frac(uV, vdiv[0], vdiv[1])) * 1000;
+
+			uV = ltc3562_regulators[i].uV_step / 1000;
+			ltc3562_regulators[i].uV_step =
+				(uV + mult_frac(uV, vdiv[0], vdiv[1])) * 1000;
+		}
+
+		rconfig.dev = &i2c->dev;
+		rconfig.init_data =
+			of_get_regulator_init_data(&i2c->dev, np_child);
+		rconfig.driver_data = ltc3562;
+		rconfig.of_node = np_child;
+
+		ltc3562->rdev[i] = devm_regulator_register(&i2c->dev,
+			&ltc3562_regulators[i], &rconfig);
+
+		if (IS_ERR(ltc3562->rdev[i])) {
+			error = PTR_ERR(ltc3562->rdev[i]);
+			dev_err(&i2c->dev,
+				"could not register regulator, Error %d\n",
+				error);
+		}
+	}
+
+	i2c_set_clientdata(i2c, ltc3562);
+	dev_dbg(&i2c->dev, "LTC3562 Driver loaded\n");
+	return 0;
+
+lbl_exit:
+	return error;
+}
+
+static const struct i2c_device_id ltc3562_i2c_id[] = {
+	{ "ltc3562", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ltc3562_i2c_id);
+
+static struct i2c_driver ltc3562_i2c_driver = {
+	.driver = {
+		.name = "LTC3562",
+		.owner = THIS_MODULE,
+	},
+	.probe    = ltc3562_i2c_probe,
+	.id_table = ltc3562_i2c_id,
+};
+
+module_i2c_driver(ltc3562_i2c_driver);
+
+MODULE_DESCRIPTION("LTC3562 Regulator Driver");
+MODULE_AUTHOR("auryn.verwegen@topic.nl");
+MODULE_AUTHOR("mike.looijmans@topic.nl");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 10/30/2014 11:58 AM, Mark Brown wrote:
> On Thu, Oct 30, 2014 at 11:53:37AM +0100, Mike Looijmans wrote:
>> On 10/30/2014 11:29 AM, Mike Looijmans wrote:
>
>>> So I should add "regulator-default-voltage" to the generic code? That would
>>> indeed be better than trying to do it into this driver.
>
>>> But would that need a separate patch for regulator core to add the property?
>
> Yes.
>
>> Hmm, I looked into doing that, but that isn't trivial, and too many devices
>> will suffer.
>
> Why would this have a negative effect on other devices?  Unless somehow
> the other devices add the property they should not be affected by it; if
> users do add the property presumably that's because it's doing something
> useful.

The "default" would end up in the constraints, thus adding extra fields to 
that struct.

So far, all other drivers have accomplished this by setting min=max, and some 
experimenting proved that my driver is no exception to that. It even reduced 
the amount of code.

Mike.



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Thu, Oct 30, 2014 at 12:31:42PM +0100, Mike Looijmans wrote:
> On 10/30/2014 11:58 AM, Mark Brown wrote:

> >>Hmm, I looked into doing that, but that isn't trivial, and too many devices
> >>will suffer.

> >Why would this have a negative effect on other devices?  Unless somehow
> >the other devices add the property they should not be affected by it; if
> >users do add the property presumably that's because it's doing something
> >useful.

> The "default" would end up in the constraints, thus adding extra fields to
> that struct.

Sure, but why is that a problem?

> So far, all other drivers have accomplished this by setting min=max, and
> some experimenting proved that my driver is no exception to that. It even
> reduced the amount of code.

Yes, if you don't need to both set an initial voltage and support
voltage changing that works - it sounded like you needed both.

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Thu, Oct 30, 2014 at 12:26:55PM +0100, Mike Looijmans wrote:
> The ltc3562 is an I2C controlled regulator supporting 4 independent
> outputs.
> 
> v2: Prefix "lltc" to devicetree properties. Use the same property names
>     as the ltc3589 driver. Remove default-voltage property. Use
>     devm_register_regulator.

As covered in SubmittingPatches things like this should be after ---.

> +Required properties:
> +- compatible: "ltc3562"

This needs a vendor in the compatible string.

> +Required child node:
> +- regulators: Contains four regulator child nodes R400B, R600B, R400A, R600A,
> +  specifying the initialization data as documented in
> +  Documentation/devicetree/bindings/regulator/regulator.txt.

All regulator child nodes should be optional.

> +			R600A_reg: R600A {
> +				regulator-name = "R600A";

Remove these regulator-names, this is for the name of the supplies on
the board not the regulator itself.

> +static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b);
> +static int ltc3562_dummy_write(struct i2c_client *i2c);

This appears to be reimplementing regmap (including a cache).  Please
use that instead.  Pretty much the entire driver could then be replaced
with the regmap helpers, none of the operations look like they'd be
needed, and you'd get the regmap diagnostic infrastructure.

> +	np = of_node_get(i2c->dev.of_node);
> +	np_regulators = of_get_child_by_name(np, "regulators");

> +		np_child = of_get_child_by_name(np_regulators,
> +			ltc3562_regulators[i].name);
> +		if (np_child == NULL) {

Use the core support for looking up constraints please - set
regulators_node and so on.

> +static struct i2c_driver ltc3562_i2c_driver = {
> +	.driver = {
> +		.name = "LTC3562",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe    = ltc3562_i2c_probe,
> +	.id_table = ltc3562_i2c_id,
> +};

You need to supply an of_match_table too.

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 30-10-2014 17:51, Mark Brown wrote:
> On Thu, Oct 30, 2014 at 12:26:55PM +0100, Mike Looijmans wrote:
>> The ltc3562 is an I2C controlled regulator supporting 4 independent
>> outputs.
>>
>> v2: Prefix "lltc" to devicetree properties. Use the same property names
>>      as the ltc3589 driver. Remove default-voltage property. Use
>>      devm_register_regulator.
>
> As covered in SubmittingPatches things like this should be after ---.
>
>> +Required properties:
>> +- compatible: "ltc3562"
>
> This needs a vendor in the compatible string.

Will do.

>> +Required child node:
>> +- regulators: Contains four regulator child nodes R400B, R600B, R400A, R600A,
>> +  specifying the initialization data as documented in
>> +  Documentation/devicetree/bindings/regulator/regulator.txt.
>
> All regulator child nodes should be optional.
>
>> +			R600A_reg: R600A {
>> +				regulator-name = "R600A";
>
> Remove these regulator-names, this is for the name of the supplies on
> the board not the regulator itself.
>
>> +static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b);
>> +static int ltc3562_dummy_write(struct i2c_client *i2c);
>
> This appears to be reimplementing regmap (including a cache).  Please
> use that instead.  Pretty much the entire driver could then be replaced
> with the regmap helpers, none of the operations look like they'd be
> needed, and you'd get the regmap diagnostic infrastructure.

The chip doesn't have an I2C register map, it uses "commands".
It does not support read transactions at all, it will NACK those. The 
first byte contains a bit mask that tells which outputs are to be 
configured and in what mode, the next is the enable bit and setpoint 
value. The first byte already contains data, it's not just an address.

I would have used regmap if I thought it'd help (if only for the 
diagnostics), but it simply doesn't fit here.

If you insist, I can probably get it to fit on regmap, but that would 
still mean implementing some custom write method that translates between 
some non-existing registry format and what the chip really wants.


>> +	np = of_node_get(i2c->dev.of_node);
>> +	np_regulators = of_get_child_by_name(np, "regulators");
>
>> +		np_child = of_get_child_by_name(np_regulators,
>> +			ltc3562_regulators[i].name);
>> +		if (np_child == NULL) {
>
> Use the core support for looking up constraints please - set
> regulators_node and so on.

I'll dig into it, seems that there's more infrastructure here to use.

>> +static struct i2c_driver ltc3562_i2c_driver = {
>> +	.driver = {
>> +		.name = "LTC3562",
>> +		.owner = THIS_MODULE,
>> +	},
>> +	.probe    = ltc3562_i2c_probe,
>> +	.id_table = ltc3562_i2c_id,
>> +};
>
> You need to supply an of_match_table too.

Hmm, that would explain why I couldn't get it to work using 
"lltc,ltc3562" during our first attempts at the driver.


-- 
Mike Looijmans

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Fri, Oct 31, 2014 at 03:07:57PM +0100, Mike Looijmans wrote:
> On 30-10-2014 17:51, Mark Brown wrote:

> >This appears to be reimplementing regmap (including a cache).  Please
> >use that instead.  Pretty much the entire driver could then be replaced
> >with the regmap helpers, none of the operations look like they'd be
> >needed, and you'd get the regmap diagnostic infrastructure.

> The chip doesn't have an I2C register map, it uses "commands".
> It does not support read transactions at all, it will NACK those. The first
> byte contains a bit mask that tells which outputs are to be configured and
> in what mode, the next is the enable bit and setpoint value. The first byte
> already contains data, it's not just an address.

If that's the case the code is really rather unclear about what it's
doing - the use of "reg" for what's being written doesn't help, nor does
the fact that what looks like a cache is being kept.  The code should
either use regmap or make it clear why that doesn't fit well.

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 10/30/2014 05:51 PM, Mark Brown wrote:
> On Thu, Oct 30, 2014 at 12:26:55PM +0100, Mike Looijmans wrote:
>> +	np = of_node_get(i2c->dev.of_node);
>> +	np_regulators = of_get_child_by_name(np, "regulators");
>
>> +		np_child = of_get_child_by_name(np_regulators,
>> +			ltc3562_regulators[i].name);
>> +		if (np_child == NULL) {
>
> Use the core support for looking up constraints please - set
> regulators_node and so on.

I've been reworking the driver, but this is the only feedback comment I didn't 
quite understand. What is it that I'm expected to do here? Probably just the 
name of the method I'm supposed to call here would be enough.


The rest of your feedback I've processed and tested the results.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Mon, Nov 03, 2014 at 09:10:08AM +0100, Mike Looijmans wrote:
> On 10/30/2014 05:51 PM, Mark Brown wrote:

> >>+		np_child = of_get_child_by_name(np_regulators,
> >>+			ltc3562_regulators[i].name);
> >>+		if (np_child == NULL) {

> >Use the core support for looking up constraints please - set
> >regulators_node and so on.

> I've been reworking the driver, but this is the only feedback comment I
> didn't quite understand. What is it that I'm expected to do here? Probably
> just the name of the method I'm supposed to call here would be enough.

No function calls, just use regulators_node.  What is unclear about the
functionality?

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/03/2014 01:09 PM, Mark Brown wrote:
> On Mon, Nov 03, 2014 at 09:10:08AM +0100, Mike Looijmans wrote:
>> On 10/30/2014 05:51 PM, Mark Brown wrote:
>
>>>> +		np_child = of_get_child_by_name(np_regulators,
>>>> +			ltc3562_regulators[i].name);
>>>> +		if (np_child == NULL) {
>
>>> Use the core support for looking up constraints please - set
>>> regulators_node and so on.
>
>> I've been reworking the driver, but this is the only feedback comment I
>> didn't quite understand. What is it that I'm expected to do here? Probably
>> just the name of the method I'm supposed to call here would be enough.
>
> No function calls, just use regulators_node.  What is unclear about the
> functionality?

I don't understand what you mean by "regulators_node".

"grep -R regulators_node *" in the kernel source tree returns no results, nor 
does http://lxr.free-electrons.com/ident?i=regulators_node




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Mon, Nov 03, 2014 at 03:48:56PM +0100, Mike Looijmans wrote:
> On 11/03/2014 01:09 PM, Mark Brown wrote:

> >No function calls, just use regulators_node.  What is unclear about the
> >functionality?

> I don't understand what you mean by "regulators_node".

> "grep -R regulators_node *" in the kernel source tree returns no results,
> nor does http://lxr.free-electrons.com/ident?i=regulators_node

You need to develop against current versions of the kernel, this is
something that was merged into Linus' tree during the merge window.

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 3-11-2014 16:10, Mark Brown wrote:
> On Mon, Nov 03, 2014 at 03:48:56PM +0100, Mike Looijmans wrote:
>> On 11/03/2014 01:09 PM, Mark Brown wrote:
>
>>> No function calls, just use regulators_node.  What is unclear about the
>>> functionality?
>
>> I don't understand what you mean by "regulators_node".
>
>> "grep -R regulators_node *" in the kernel source tree returns no results,
>> nor does http://lxr.free-electrons.com/ident?i=regulators_node
>
> You need to develop against current versions of the kernel, this is
> something that was merged into Linus' tree during the merge window.

Is this an absolute show-stopper for you?

With some effort I could move from our current 3.15 to 3.17, but even 
that wouldn't be recent enough then. I can justify spending a few days 
on getting the driver integrated into mainline, even if the initial 
version cost less than that; but moving everything to mainline is going 
to take weeks and the boss is definitely going to say "no" to that.

-- 
Mike Looijmans

[PATCH v3] Add ltc3562 voltage regulator driver

[Mike Looijmans]

The ltc3562 is an I2C controlled regulator supporting 4 independent
outputs.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

v3: Add .of_match_table and prefix lltc
    Clarify why regmap cannot be used
    Add lltc,operating-mode (0..3) DT property
    regulator child nodes are optional


 .../devicetree/bindings/regulator/ltc3562.txt      |   57 ++++
 drivers/regulator/Kconfig                          |    7 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/ltc3562.c                        |  337 ++++++++++++++++++++
 4 files changed, 402 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/ltc3562.txt
 create mode 100644 drivers/regulator/ltc3562.c

diff --git a/Documentation/devicetree/bindings/regulator/ltc3562.txt b/Documentation/devicetree/bindings/regulator/ltc3562.txt
new file mode 100644
index 0000000..d73c865
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/ltc3562.txt
@@ -0,0 +1,57 @@
+Linear Technology LTC3562 I2C controlled 4-output regulator
+
+Required properties:
+- compatible: "lltc,ltc3562"
+- reg: I2C slave address
+
+Required child node:
+- regulators: Contains optional regulator child nodes R400B, R600B, R400A
+  and R600A specifying the initialization data as documented in
+  Documentation/devicetree/bindings/regulator/regulator.txt.
+
+Each regulator is defined using the standard binding for regulators. The
+nodes for R400A and R600A additionally need to specify the resistor values of
+their external feedback voltage dividers:
+
+Required properties (A-type only):
+- lltc,fb-voltage-divider: An array of two integers containing the resistor
+  values R1 and R2 of the feedback voltage divider. Both values must remain in
+  the range 1..1000, only their quotient matters.
+
+Optional properties:
+- lltc,operating-mode: Operating mode as specified in table 3 of the datasheet.
+  This value is passed as the lower two bits of the first data byte, and sets
+  the operating mode: 0=Pulse-skip (default), 1=LDO, 2=Forced-burst, 3=Burst.
+
+Example:
+
+	ltc3562: ltc3562@65 {
+		compatible = "ltc3562";
+		reg = <0x65>;
+		regulators {
+			R400B_reg: R400B {
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			R600B_reg: R600B {
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+			R400A_reg: R400A {
+				regulator-min-microvolt = <425000>;
+				regulator-max-microvolt = <8000000>;
+				lltc,fb-voltage-divider = <1, 1>;
+			};
+			R600A_reg: R600A {
+				regulator-min-microvolt = <425000>;
+				regulator-max-microvolt = <1800000>;
+				lltc,fb-voltage-divider = <316, 100>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 55d7b7b..4012601 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -300,6 +300,13 @@ config REGULATOR_LP8788
 	help
 	  This driver supports LP8788 voltage regulator chip.
 
+config REGULATOR_LTC3562
+	tristate "LCT3562 quad output voltage regulator"
+	depends on I2C
+	help
+	  This enables support for the LCT3562 quad output voltage regulator
+	  controlled via I2C.
+
 config REGULATOR_LTC3589
 	tristate "LTC3589 8-output voltage regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 1029ed3..9a15146 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-buck.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-ldo.o
 obj-$(CONFIG_REGULATOR_LP8755) += lp8755.o
+obj-$(CONFIG_REGULATOR_LTC3562) += ltc3562.o
 obj-$(CONFIG_REGULATOR_LTC3589) += ltc3589.o
 obj-$(CONFIG_REGULATOR_MAX14577) += max14577.o
 obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
diff --git a/drivers/regulator/ltc3562.c b/drivers/regulator/ltc3562.c
new file mode 100644
index 0000000..69963f8
--- /dev/null
+++ b/drivers/regulator/ltc3562.c
@@ -0,0 +1,337 @@
+/*
+ * Regulator driver for Linear Technology LTC3562
+ *
+ *  Copyright (C) 2014 Topic Embedded Products
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/slab.h>
+#include <linux/regulator/of_regulator.h>
+
+#define LTC3562_NUM_REGULATORS	4
+
+#define REGULATOR_TYPE_A_NUM_VOLTAGES	16
+#define REGULATOR_TYPE_A_MIN_UV	425000
+#define REGULATOR_TYPE_A_UV_STEP	25000
+
+#define REGULATOR_TYPE_B_NUM_VOLTAGES	128
+#define REGULATOR_TYPE_B_MIN_UV	600000
+#define REGULATOR_TYPE_B_UV_STEP	25000
+
+/* the LTC3562 does not have a register map, instead it receives a two-byte
+ * command set. The first byte sets the mask for the output(s) to be programmed
+ * and the second byte hold the "enable" bit and the DAC code. */
+struct ltc3562_status {
+	u8 addr_mode;	/* sub-address byte: program mask an operating mode */
+	u8 enable_daccode;	/* data byte: Enable bit and DAC code  */
+};
+
+struct ltc3562 {
+	struct device *dev;
+	struct i2c_client *i2c;
+	struct mutex mutex;
+	struct regulator_dev *rdev[LTC3562_NUM_REGULATORS];
+	struct ltc3562_status rstatus[LTC3562_NUM_REGULATORS];
+};
+
+#define LTC3562_R400B_ID	0x00
+#define LTC3562_R600B_ID	0x01
+#define LTC3562_R400A_ID	0x02
+#define LTC3562_R600A_ID	0x03
+
+#define OPERATING_MODE_MASK	0x03
+
+#define PROGRAM_R400B_MASK	0x10
+#define PROGRAM_R600B_MASK	0x20
+#define PROGRAM_R400A_MASK	0x40
+#define PROGRAM_R600A_MASK	0x80
+
+#define OPERATING_MODE_PULSE_SKIP	0x00
+#define OPERATING_MODE_LDO		0x01
+#define OPERATING_MODE_FOCED_BURST	0x02
+#define OPERATING_MODE_BURST		0x03
+
+#define REGULATOR_ENABLE_BIT	0x80
+
+static int ltc3562_update(struct ltc3562 *ltc3562,
+	struct ltc3562_status *status)
+{
+	return i2c_smbus_write_byte_data(
+		ltc3562->i2c, status->addr_mode, status->enable_daccode);
+}
+
+static int ltc3562_regulator_enable(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->enable_daccode |= REGULATOR_ENABLE_BIT;
+	ret = ltc3562_update(ltc3562, status);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_disable(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->enable_daccode &= ~(REGULATOR_ENABLE_BIT);
+	ret = ltc3562_update(ltc3562, status);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_regulator_is_enabled(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = (status->enable_daccode & REGULATOR_ENABLE_BIT) ? 1 : 0;
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_set_voltage_sel(struct regulator_dev *dev,
+	unsigned int selector)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	status->enable_daccode =
+		(status->enable_daccode & REGULATOR_ENABLE_BIT) | selector;
+	ret = ltc3562_update(ltc3562, status);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static int ltc3562_get_voltage_sel(struct regulator_dev *dev)
+{
+	int ret;
+	struct ltc3562 *ltc3562 = rdev_get_drvdata(dev);
+	int id = rdev_get_id(dev);
+	struct ltc3562_status *status = &ltc3562->rstatus[id];
+
+	mutex_lock(&ltc3562->mutex);
+
+	ret = status->enable_daccode & ~(REGULATOR_ENABLE_BIT);
+
+	mutex_unlock(&ltc3562->mutex);
+
+	return ret;
+}
+
+static struct regulator_ops ltc3562_regulator_ops = {
+	.is_enabled = ltc3562_regulator_is_enabled,
+	.enable = ltc3562_regulator_enable,
+	.disable = ltc3562_regulator_disable,
+	.set_voltage_sel = ltc3562_set_voltage_sel,
+	.get_voltage_sel = ltc3562_get_voltage_sel,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+};
+
+static struct regulator_desc ltc3562_regulators[] = {
+	{
+		.name = "R400B",
+		.id = LTC3562_R400B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600B",
+		.id = LTC3562_R600B_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_B_MIN_UV,
+		.uV_step = REGULATOR_TYPE_B_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_B_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R400A",
+		.id = LTC3562_R400A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	{
+		.name = "R600A",
+		.id = LTC3562_R600A_ID,
+		.ops = &ltc3562_regulator_ops,
+		.min_uV = REGULATOR_TYPE_A_MIN_UV,
+		.uV_step = REGULATOR_TYPE_A_UV_STEP,
+		.n_voltages = REGULATOR_TYPE_A_NUM_VOLTAGES,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+};
+
+/* Write dummy data to detect presence of the physical device. */
+static int ltc3562_dummy_write(struct i2c_client *i2c)
+{
+	return i2c_smbus_write_byte_data(i2c, 0, 0);
+}
+
+static int ltc3562_i2c_probe(struct i2c_client *i2c,
+	const struct i2c_device_id *id)
+{
+	int i, error;
+	unsigned long uV;
+	struct ltc3562 *ltc3562;
+	struct regulator_config rconfig = { };
+	struct device_node *np_regulators, *np;
+	struct device_node *np_child;
+	u32 value;
+
+	ltc3562 = devm_kzalloc(&i2c->dev,
+		sizeof(struct ltc3562), GFP_KERNEL);
+	if (ltc3562 == NULL)
+		return -ENOMEM;
+
+	ltc3562->i2c = i2c;
+	i2c_set_clientdata(i2c, ltc3562);
+
+	mutex_init(&ltc3562->mutex);
+
+	if (ltc3562_dummy_write(i2c) < 0) {
+		dev_err(&i2c->dev,
+			"Could not find device LTC3562 on i2c bus\n");
+		return -ENODEV;
+	}
+
+	np = of_node_get(i2c->dev.of_node);
+	np_regulators = of_get_child_by_name(np, "regulators");
+
+	if (np_regulators == NULL) {
+		dev_err(&i2c->dev, "Could not find regulators node\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < LTC3562_NUM_REGULATORS; ++i) {
+		np_child = of_get_child_by_name(np_regulators,
+			ltc3562_regulators[i].name);
+		if (np_child == NULL)
+			continue;
+
+		if ((ltc3562_regulators[i].id == LTC3562_R400A_ID) ||
+			(ltc3562_regulators[i].id == LTC3562_R600A_ID)) {
+			/* If regulator is A type get resistor values */
+			u32 vdiv[2];
+
+			error = of_property_read_u32_array(np_child,
+				"lltc,fb-voltage-divider", vdiv, 2);
+			if (error) {
+				dev_err(&i2c->dev,
+					"Failed to parse voltage divider: %d\n",
+					error);
+				return error;
+			}
+
+			uV = ltc3562_regulators[i].min_uV / 1000;
+			ltc3562_regulators[i].min_uV =
+				(uV + mult_frac(uV, vdiv[0], vdiv[1])) * 1000;
+
+			uV = ltc3562_regulators[i].uV_step / 1000;
+			ltc3562_regulators[i].uV_step =
+				(uV + mult_frac(uV, vdiv[0], vdiv[1])) * 1000;
+		}
+
+		/* Set operating mode and address mask */
+		error = of_property_read_u32(np_child,
+				"lltc,operating-mode", &value);
+		if (error || (value > OPERATING_MODE_BURST))
+			value = OPERATING_MODE_PULSE_SKIP;
+		ltc3562->rstatus[i].addr_mode =
+			(PROGRAM_R400B_MASK << i) | value;
+
+		rconfig.dev = &i2c->dev;
+		rconfig.init_data =
+			of_get_regulator_init_data(&i2c->dev, np_child);
+		rconfig.driver_data = ltc3562;
+		rconfig.of_node = np_child;
+
+		ltc3562->rdev[i] = devm_regulator_register(&i2c->dev,
+			&ltc3562_regulators[i], &rconfig);
+
+		if (IS_ERR(ltc3562->rdev[i])) {
+			error = PTR_ERR(ltc3562->rdev[i]);
+			dev_err(&i2c->dev,
+				"could not register regulator, Error %d\n",
+				error);
+		}
+	}
+
+	dev_dbg(&i2c->dev, "LTC3562 Driver loaded\n");
+
+	return 0;
+}
+
+static const struct of_device_id ltc3562_match_id[] = {
+	{ .compatible = "lltc,ltc3562", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ltc3562_match_id);
+
+static const struct i2c_device_id ltc3562_i2c_id[] = {
+	{ "ltc3562", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ltc3562_i2c_id);
+
+static struct i2c_driver ltc3562_i2c_driver = {
+	.driver = {
+		.name = "LTC3562",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(ltc3562_match_id),
+	},
+	.probe    = ltc3562_i2c_probe,
+	.id_table = ltc3562_i2c_id,
+};
+
+module_i2c_driver(ltc3562_i2c_driver);
+
+MODULE_DESCRIPTION("LTC3562 Regulator Driver");
+MODULE_AUTHOR("auryn.verwegen@topic.nl");
+MODULE_AUTHOR("mike.looijmans@topic.nl");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/03/2014 06:38 PM, Mike Looijmans wrote:
> On 3-11-2014 16:10, Mark Brown wrote:
>> On Mon, Nov 03, 2014 at 03:48:56PM +0100, Mike Looijmans wrote:
>>> On 11/03/2014 01:09 PM, Mark Brown wrote:
>>
>>>> No function calls, just use regulators_node.  What is unclear about the
>>>> functionality?
>>
>>> I don't understand what you mean by "regulators_node".
>>
>>> "grep -R regulators_node *" in the kernel source tree returns no results,
>>> nor does http://lxr.free-electrons.com/ident?i=regulators_node
>>
>> You need to develop against current versions of the kernel, this is
>> something that was merged into Linus' tree during the merge window.
>
> Is this an absolute show-stopper for you?
>
> With some effort I could move from our current 3.15 to 3.17, but even that
> wouldn't be recent enough then. I can justify spending a few days on getting
> the driver integrated into mainline, even if the initial version cost less
> than that; but moving everything to mainline is going to take weeks and the
> boss is definitely going to say "no" to that.

More important than that: Since this chip powers the IO banks, we need it for 
our own products, and hence it has to work (also) on the current 3.15 kernel. 
Using APIs from 3.18 wouold render the driver useless to ourselves.



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Tue, Nov 04, 2014 at 09:55:14AM +0100, Mike Looijmans wrote:
> On 11/03/2014 06:38 PM, Mike Looijmans wrote:

> >>You need to develop against current versions of the kernel, this is
> >>something that was merged into Linus' tree during the merge window.

> >Is this an absolute show-stopper for you?

> >With some effort I could move from our current 3.15 to 3.17, but even that
> >wouldn't be recent enough then. I can justify spending a few days on getting
> >the driver integrated into mainline, even if the initial version cost less
> >than that; but moving everything to mainline is going to take weeks and the
> >boss is definitely going to say "no" to that.

It should be easy to backport the support into your current kernel, it's
just a few commits and there hasn't been much development in this area
of the code.  Can you take a look at doing that please?

> More important than that: Since this chip powers the IO banks, we need it
> for our own products, and hence it has to work (also) on the current 3.15
> kernel. Using APIs from 3.18 wouold render the driver useless to ourselves.

This is just not at all important upstream, sorry.  The kernel moves on
and internal APIs within the kernel move on so it is expected that code
used in production won't be the latest and greatest upstream code.

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/04/2014 12:34 PM, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 09:55:14AM +0100, Mike Looijmans wrote:
>> On 11/03/2014 06:38 PM, Mike Looijmans wrote:
>
>>>> You need to develop against current versions of the kernel, this is
>>>> something that was merged into Linus' tree during the merge window.
>
>>> Is this an absolute show-stopper for you?
>
>>> With some effort I could move from our current 3.15 to 3.17, but even that
>>> wouldn't be recent enough then. I can justify spending a few days on getting
>>> the driver integrated into mainline, even if the initial version cost less
>>> than that; but moving everything to mainline is going to take weeks and the
>>> boss is definitely going to say "no" to that.
>
> It should be easy to backport the support into your current kernel, it's
> just a few commits and there hasn't been much development in this area
> of the code.  Can you take a look at doing that please?

That option hadn't crossed my mind yet, I'll look into it.

>
>> More important than that: Since this chip powers the IO banks, we need it
>> for our own products, and hence it has to work (also) on the current 3.15
>> kernel. Using APIs from 3.18 wouold render the driver useless to ourselves.
>
> This is just not at all important upstream, sorry.  The kernel moves on
> and internal APIs within the kernel move on so it is expected that code
> used in production won't be the latest and greatest upstream code.

I realize that, but then the result would be that I'll stop working on getting 
the driver into mainline and force customers to use a patch or use our fork of 
linux.




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/04/2014 12:34 PM, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 09:55:14AM +0100, Mike Looijmans wrote:
>> On 11/03/2014 06:38 PM, Mike Looijmans wrote:
>
>>>> You need to develop against current versions of the kernel, this is
>>>> something that was merged into Linus' tree during the merge window.
>
>>> Is this an absolute show-stopper for you?
>
>>> With some effort I could move from our current 3.15 to 3.17, but even that
>>> wouldn't be recent enough then. I can justify spending a few days on getting
>>> the driver integrated into mainline, even if the initial version cost less
>>> than that; but moving everything to mainline is going to take weeks and the
>>> boss is definitely going to say "no" to that.
>
> It should be easy to backport the support into your current kernel, it's
> just a few commits and there hasn't been much development in this area
> of the code.  Can you take a look at doing that please?

I can cherry-pick about 4 commits to get the updated core into my 3.15 tree.

I still need help with one thing that isn't clear to me though. The DT is 
parsed when calling regulator_register. But then how do I fetch my "private" 
settings in there BEFORE the regulator gets enabled? The feedback resistors 
and regulator mode must be set to the correct board before the output is 
enabled, otherwise it may damage the chip and its periferals.

The only two drivers using the regulators_node (tps65217 and isl9305.c) don't 
have any private values in the DT.

M.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Tue, Nov 04, 2014 at 02:35:50PM +0100, Mike Looijmans wrote:

> I still need help with one thing that isn't clear to me though. The DT is
> parsed when calling regulator_register. But then how do I fetch my "private"
> settings in there BEFORE the regulator gets enabled? The feedback resistors
> and regulator mode must be set to the correct board before the output is
> enabled, otherwise it may damage the chip and its periferals.

Add a callback for this.  Javier and/or Krzysztof were looking at this
for some other stuff, though I think they were intending to add core
functionality for their specific feature.

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

Re: [PATCH v3] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Tue, Nov 04, 2014 at 07:50:45AM +0100, Mike Looijmans wrote:

> v3: Add .of_match_table and prefix lltc
>     Clarify why regmap cannot be used
>     Add lltc,operating-mode (0..3) DT property
>     regulator child nodes are optional

Leave out the mode setting in the DT for now please, Javier was looking
at adding a standard property for this with a driver defined mapping to
modes - you should implement the standard get_mode() and set_mode() (but
can always do that as a followup).

> +/* the LTC3562 does not have a register map, instead it receives a two-byte
> + * command set. The first byte sets the mask for the output(s) to be programmed
> + * and the second byte hold the "enable" bit and the DAC code. */
> +struct ltc3562_status {
> +	u8 addr_mode;	/* sub-address byte: program mask an operating mode */
> +	u8 enable_daccode;	/* data byte: Enable bit and DAC code  */
> +};

So, I managed to find a datasheet[1] and this does actually seem to be a
standard register map.  It looks like this is a 4x12 register map with
the program bytes being essentially register addresses (called sub
addresses in the datasheet), two pad bits (normally we'd include these
in the data for convenience) and the rest of the bits data.  What am I
missing here?

Otherwise this looks good.

[1] http://cds.linear.com/docs/en/datasheet/3562fa.pdf

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

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Krzysztof Kozlowski]

On wto, 2014-11-04 at 19:47 +0000, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 02:35:50PM +0100, Mike Looijmans wrote:
> 
> > I still need help with one thing that isn't clear to me though. The DT is
> > parsed when calling regulator_register. But then how do I fetch my "private"
> > settings in there BEFORE the regulator gets enabled? The feedback resistors
> > and regulator mode must be set to the correct board before the output is
> > enabled, otherwise it may damage the chip and its periferals.
> 
> Add a callback for this.  Javier and/or Krzysztof were looking at this
> for some other stuff, though I think they were intending to add core
> functionality for their specific feature.

I needed a callback and added such. The callback was called on each DT
node parsed. However I dropped that idea because it turned our to be
unsuitable to my case.

I needed to feed regulator_config.ena_gpio with data parsed from such
callback. But the callback called from inside the regulator_register()
has the regulator_config const.

Best regards,
Krzysztof

Re: [PATCH v3] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/04/2014 09:26 PM, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 07:50:45AM +0100, Mike Looijmans wrote:
>
>> v3: Add .of_match_table and prefix lltc
>>      Clarify why regmap cannot be used
>>      Add lltc,operating-mode (0..3) DT property
>>      regulator child nodes are optional
>
> Leave out the mode setting in the DT for now please, Javier was looking
> at adding a standard property for this with a driver defined mapping to
> modes - you should implement the standard get_mode() and set_mode() (but
> can always do that as a followup).
>
>> +/* the LTC3562 does not have a register map, instead it receives a two-byte
>> + * command set. The first byte sets the mask for the output(s) to be programmed
>> + * and the second byte hold the "enable" bit and the DAC code. */
>> +struct ltc3562_status {
>> +	u8 addr_mode;	/* sub-address byte: program mask an operating mode */
>> +	u8 enable_daccode;	/* data byte: Enable bit and DAC code  */
>> +};
>
> So, I managed to find a datasheet[1] and this does actually seem to be a
> standard register map.  It looks like this is a 4x12 register map with
> the program bytes being essentially register addresses (called sub
> addresses in the datasheet), two pad bits (normally we'd include these
> in the data for convenience) and the rest of the bits data.  What am I
> missing here?
>
> Otherwise this looks good.
>
> [1] http://cds.linear.com/docs/en/datasheet/3562fa.pdf
>

Things that makes me think it's NOT a register map:
- You cannot read from the device
- You always have to send a complete command set (two bytes)
- There is data (the regulator mode) in the address byte
- The 'address' is not an index but a bitmask in bits 4..7

Yeah, I think you can force-feed this to regmap. It will just make the code 
considerably bigger and harder to understand, and as far as I know, the one 
and only advantage this will bring is that you can dump the current 
"registers" (4x10 bits of data) via /sys/kernel/debug/regmap.

I fail to see the use for regmap here, but if your view on this is "regmap or 
burst" then I'll implement it.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mike Looijmans]

On 11/04/2014 08:47 PM, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 02:35:50PM +0100, Mike Looijmans wrote:
>
>> I still need help with one thing that isn't clear to me though. The DT is
>> parsed when calling regulator_register. But then how do I fetch my "private"
>> settings in there BEFORE the regulator gets enabled? The feedback resistors
>> and regulator mode must be set to the correct board before the output is
>> enabled, otherwise it may damage the chip and its periferals.
>
> Add a callback for this.  Javier and/or Krzysztof were looking at this
> for some other stuff, though I think they were intending to add core
> functionality for their specific feature.
>

I've run out of my hours-to-spend-on-driver-submission budget for this period 
, so it'll be at least a week before I can post a new version. The good news 
is that I have upgrading the kernel to 3.17 on the planning board now, so 
that'll at least reduce the version gap.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Re: [PATCH v3] Add ltc3562 voltage regulator driver

[Mark Brown]

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

On Wed, Nov 05, 2014 at 12:41:34PM +0100, Mike Looijmans wrote:
> On 11/04/2014 09:26 PM, Mark Brown wrote:

> >So, I managed to find a datasheet[1] and this does actually seem to be a
> >standard register map.  It looks like this is a 4x12 register map with
> >the program bytes being essentially register addresses (called sub
> >addresses in the datasheet), two pad bits (normally we'd include these
> >in the data for convenience) and the rest of the bits data.  What am I
> >missing here?

> Things that makes me think it's NOT a register map:
> - You cannot read from the device

That's entirely normal for older devices, on larger process nodes the
extra digital to implement read was expensive enough for people to care
and I believe there were some IP issues with implementing read support.

> - You always have to send a complete command set (two bytes)
> - There is data (the regulator mode) in the address byte

Having to send both a register and a value is the standard thing, and
with older write only devices things frequently weren't full byte
multiples, 7x9 was extremely common.

> - The 'address' is not an index but a bitmask in bits 4..7

That's just a sparse register map, again very common (you'll also often
see holes in the register map due to undocumented registers or with
larger register addresses just to keep the map neat).

> I fail to see the use for regmap here, but if your view on this is "regmap
> or burst" then I'll implement it.

The biggest advantage is that it means that a large proportion of the
code in the driver can be removed and standard implementations in the
core used instead.  In general removing code is good and this means that
if we do things like impement support for enabling and disabling
multiple regulators with a single write then the driver will support
this for free.

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