[PATCH 0/3] iio: potentiometer: mcp4531: New parts and device tree

[PATCH 0/3] iio: potentiometer: mcp4531: New parts and device tree

[Florian Vaussard]

Hello,

This series first adds support for parts missing from mcp4531 driver
(MCP454x, MCP456x, MCP464x and MCP466x). It then introduces the necessary
device tree binding to perform DT boot.

Tested with MCP4561-103 and MCP4561-503 (DT boot).

Best regards,
Florian

Florian Vaussard (3):
  iio: potentiometer: mcp4531: Add support for MCP454x, MCP456x, MCP464x
    and MCP466x
  iio: potentiometer: mcp4531: Add device tree binding documentation
  iio: potentiometer: mcp4531: Add device tree binding

 .../bindings/iio/potentiometer/mcp4531.txt         |  84 +++++++++++
 drivers/iio/potentiometer/Kconfig                  |   6 +-
 drivers/iio/potentiometer/mcp4531.c                | 155 ++++++++++++++++++++-
 3 files changed, 242 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt

-- 
2.5.0

[PATCH 1/3] iio: potentiometer: mcp4531: Add support for MCP454x, MCP456x, MCP464x and MCP466x

[Florian Vaussard]

This patch adds support for MCP454x, MCP456x, MCP464x and MCP466x parts.
The main difference with currently supported parts (MCP453x and alike) is
the addition of a non-volatile memory in order to recall the wiper setting
at power-on. This feature is currently not supported and only the
volatile memory is used to set the wiper.

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/iio/potentiometer/Kconfig   |  6 ++--
 drivers/iio/potentiometer/mcp4531.c | 72 +++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 0941c8d4..55c2414 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -49,8 +49,10 @@ config MCP4531
 	depends on I2C
 	help
 	  Say yes here to build support for the Microchip
-	  MCP4531, MCP4532, MCP4551, MCP4552,
-	  MCP4631, MCP4632, MCP4651, MCP4652
+	  MCP4531, MCP4532, MCP4541, MCP4542,
+	  MCP4551, MCP4552, MCP4561, MCP4562,
+	  MCP4631, MCP4632, MCP4641, MCP4642,
+	  MCP4651, MCP4652, MCP4661, MCP4662
 	  digital potentiomenter chips.
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/potentiometer/mcp4531.c b/drivers/iio/potentiometer/mcp4531.c
index 3b72e1a..2251173 100644
--- a/drivers/iio/potentiometer/mcp4531.c
+++ b/drivers/iio/potentiometer/mcp4531.c
@@ -8,12 +8,20 @@
  * DEVID	#Wipers	#Positions	Resistor Opts (kOhm)	i2c address
  * mcp4531	1	129		5, 10, 50, 100          010111x
  * mcp4532	1	129		5, 10, 50, 100          01011xx
+ * mcp4541	1	129		5, 10, 50, 100          010111x
+ * mcp4542	1	129		5, 10, 50, 100          01011xx
  * mcp4551	1	257		5, 10, 50, 100          010111x
  * mcp4552	1	257		5, 10, 50, 100          01011xx
+ * mcp4561	1	257		5, 10, 50, 100          010111x
+ * mcp4562	1	257		5, 10, 50, 100          01011xx
  * mcp4631	2	129		5, 10, 50, 100          0101xxx
  * mcp4632	2	129		5, 10, 50, 100          01011xx
+ * mcp4641	2	129		5, 10, 50, 100          0101xxx
+ * mcp4642	2	129		5, 10, 50, 100          01011xx
  * mcp4651	2	257		5, 10, 50, 100          0101xxx
  * mcp4652	2	257		5, 10, 50, 100          01011xx
+ * mcp4661	2	257		5, 10, 50, 100          0101xxx
+ * mcp4662	2	257		5, 10, 50, 100          01011xx
  *
  * 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
@@ -37,18 +45,34 @@ enum mcp4531_type {
 	MCP453x_103,
 	MCP453x_503,
 	MCP453x_104,
+	MCP454x_502,
+	MCP454x_103,
+	MCP454x_503,
+	MCP454x_104,
 	MCP455x_502,
 	MCP455x_103,
 	MCP455x_503,
 	MCP455x_104,
+	MCP456x_502,
+	MCP456x_103,
+	MCP456x_503,
+	MCP456x_104,
 	MCP463x_502,
 	MCP463x_103,
 	MCP463x_503,
 	MCP463x_104,
+	MCP464x_502,
+	MCP464x_103,
+	MCP464x_503,
+	MCP464x_104,
 	MCP465x_502,
 	MCP465x_103,
 	MCP465x_503,
 	MCP465x_104,
+	MCP466x_502,
+	MCP466x_103,
+	MCP466x_503,
+	MCP466x_104,
 };
 
 static const struct mcp4531_cfg mcp4531_cfg[] = {
@@ -56,18 +80,34 @@ static const struct mcp4531_cfg mcp4531_cfg[] = {
 	[MCP453x_103] = { .wipers = 1, .max_pos = 128, .kohms =  10, },
 	[MCP453x_503] = { .wipers = 1, .max_pos = 128, .kohms =  50, },
 	[MCP453x_104] = { .wipers = 1, .max_pos = 128, .kohms = 100, },
+	[MCP454x_502] = { .wipers = 1, .max_pos = 128, .kohms =   5, },
+	[MCP454x_103] = { .wipers = 1, .max_pos = 128, .kohms =  10, },
+	[MCP454x_503] = { .wipers = 1, .max_pos = 128, .kohms =  50, },
+	[MCP454x_104] = { .wipers = 1, .max_pos = 128, .kohms = 100, },
 	[MCP455x_502] = { .wipers = 1, .max_pos = 256, .kohms =   5, },
 	[MCP455x_103] = { .wipers = 1, .max_pos = 256, .kohms =  10, },
 	[MCP455x_503] = { .wipers = 1, .max_pos = 256, .kohms =  50, },
 	[MCP455x_104] = { .wipers = 1, .max_pos = 256, .kohms = 100, },
+	[MCP456x_502] = { .wipers = 1, .max_pos = 256, .kohms =   5, },
+	[MCP456x_103] = { .wipers = 1, .max_pos = 256, .kohms =  10, },
+	[MCP456x_503] = { .wipers = 1, .max_pos = 256, .kohms =  50, },
+	[MCP456x_104] = { .wipers = 1, .max_pos = 256, .kohms = 100, },
 	[MCP463x_502] = { .wipers = 2, .max_pos = 128, .kohms =   5, },
 	[MCP463x_103] = { .wipers = 2, .max_pos = 128, .kohms =  10, },
 	[MCP463x_503] = { .wipers = 2, .max_pos = 128, .kohms =  50, },
 	[MCP463x_104] = { .wipers = 2, .max_pos = 128, .kohms = 100, },
+	[MCP464x_502] = { .wipers = 2, .max_pos = 128, .kohms =   5, },
+	[MCP464x_103] = { .wipers = 2, .max_pos = 128, .kohms =  10, },
+	[MCP464x_503] = { .wipers = 2, .max_pos = 128, .kohms =  50, },
+	[MCP464x_104] = { .wipers = 2, .max_pos = 128, .kohms = 100, },
 	[MCP465x_502] = { .wipers = 2, .max_pos = 256, .kohms =   5, },
 	[MCP465x_103] = { .wipers = 2, .max_pos = 256, .kohms =  10, },
 	[MCP465x_503] = { .wipers = 2, .max_pos = 256, .kohms =  50, },
 	[MCP465x_104] = { .wipers = 2, .max_pos = 256, .kohms = 100, },
+	[MCP466x_502] = { .wipers = 2, .max_pos = 256, .kohms =   5, },
+	[MCP466x_103] = { .wipers = 2, .max_pos = 256, .kohms =  10, },
+	[MCP466x_503] = { .wipers = 2, .max_pos = 256, .kohms =  50, },
+	[MCP466x_104] = { .wipers = 2, .max_pos = 256, .kohms = 100, },
 };
 
 #define MCP4531_WRITE (0 << 2)
@@ -187,6 +227,14 @@ static const struct i2c_device_id mcp4531_id[] = {
 	{ "mcp4532-103", MCP453x_103 },
 	{ "mcp4532-503", MCP453x_503 },
 	{ "mcp4532-104", MCP453x_104 },
+	{ "mcp4541-502", MCP454x_502 },
+	{ "mcp4541-103", MCP454x_103 },
+	{ "mcp4541-503", MCP454x_503 },
+	{ "mcp4541-104", MCP454x_104 },
+	{ "mcp4542-502", MCP454x_502 },
+	{ "mcp4542-103", MCP454x_103 },
+	{ "mcp4542-503", MCP454x_503 },
+	{ "mcp4542-104", MCP454x_104 },
 	{ "mcp4551-502", MCP455x_502 },
 	{ "mcp4551-103", MCP455x_103 },
 	{ "mcp4551-503", MCP455x_503 },
@@ -195,6 +243,14 @@ static const struct i2c_device_id mcp4531_id[] = {
 	{ "mcp4552-103", MCP455x_103 },
 	{ "mcp4552-503", MCP455x_503 },
 	{ "mcp4552-104", MCP455x_104 },
+	{ "mcp4561-502", MCP456x_502 },
+	{ "mcp4561-103", MCP456x_103 },
+	{ "mcp4561-503", MCP456x_503 },
+	{ "mcp4561-104", MCP456x_104 },
+	{ "mcp4562-502", MCP456x_502 },
+	{ "mcp4562-103", MCP456x_103 },
+	{ "mcp4562-503", MCP456x_503 },
+	{ "mcp4562-104", MCP456x_104 },
 	{ "mcp4631-502", MCP463x_502 },
 	{ "mcp4631-103", MCP463x_103 },
 	{ "mcp4631-503", MCP463x_503 },
@@ -203,6 +259,14 @@ static const struct i2c_device_id mcp4531_id[] = {
 	{ "mcp4632-103", MCP463x_103 },
 	{ "mcp4632-503", MCP463x_503 },
 	{ "mcp4632-104", MCP463x_104 },
+	{ "mcp4641-502", MCP464x_502 },
+	{ "mcp4641-103", MCP464x_103 },
+	{ "mcp4641-503", MCP464x_503 },
+	{ "mcp4641-104", MCP464x_104 },
+	{ "mcp4642-502", MCP464x_502 },
+	{ "mcp4642-103", MCP464x_103 },
+	{ "mcp4642-503", MCP464x_503 },
+	{ "mcp4642-104", MCP464x_104 },
 	{ "mcp4651-502", MCP465x_502 },
 	{ "mcp4651-103", MCP465x_103 },
 	{ "mcp4651-503", MCP465x_503 },
@@ -211,6 +275,14 @@ static const struct i2c_device_id mcp4531_id[] = {
 	{ "mcp4652-103", MCP465x_103 },
 	{ "mcp4652-503", MCP465x_503 },
 	{ "mcp4652-104", MCP465x_104 },
+	{ "mcp4661-502", MCP466x_502 },
+	{ "mcp4661-103", MCP466x_103 },
+	{ "mcp4661-503", MCP466x_503 },
+	{ "mcp4661-104", MCP466x_104 },
+	{ "mcp4662-502", MCP466x_502 },
+	{ "mcp4662-103", MCP466x_103 },
+	{ "mcp4662-503", MCP466x_503 },
+	{ "mcp4662-104", MCP466x_104 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, mcp4531_id);
-- 
2.5.0

[PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Florian Vaussard]

Add the device tree documentation for all the supported parts. Apart the
compatible string and standard I2C binding, no other binding is currently
needed.

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
new file mode 100644
index 0000000..b052299
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
@@ -0,0 +1,84 @@
+* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
+  driver
+
+The node for this driver must be a child node of a I2C controller, hence
+all mandatory properties for your controller must be specified. See directory:
+
+        Documentation/devicetree/bindings/i2c
+
+for more details.
+
+Required properties:
+	- compatible:  	Must be one of the following, depending on the
+			model:
+			"microchip,mcp4531-502"
+			"microchip,mcp4531-103"
+			"microchip,mcp4531-503"
+			"microchip,mcp4531-104"
+			"microchip,mcp4532-502"
+			"microchip,mcp4532-103"
+			"microchip,mcp4532-503"
+			"microchip,mcp4532-104"
+			"microchip,mcp4541-502"
+			"microchip,mcp4541-103"
+			"microchip,mcp4541-503"
+			"microchip,mcp4541-104"
+			"microchip,mcp4542-502"
+			"microchip,mcp4542-103"
+			"microchip,mcp4542-503"
+			"microchip,mcp4542-104"
+			"microchip,mcp4551-502"
+			"microchip,mcp4551-103"
+			"microchip,mcp4551-503"
+			"microchip,mcp4551-104"
+			"microchip,mcp4552-502"
+			"microchip,mcp4552-103"
+			"microchip,mcp4552-503"
+			"microchip,mcp4552-104"
+			"microchip,mcp4561-502"
+			"microchip,mcp4561-103"
+			"microchip,mcp4561-503"
+			"microchip,mcp4561-104"
+			"microchip,mcp4562-502"
+			"microchip,mcp4562-103"
+			"microchip,mcp4562-503"
+			"microchip,mcp4562-104"
+			"microchip,mcp4631-502"
+			"microchip,mcp4631-103"
+			"microchip,mcp4631-503"
+			"microchip,mcp4631-104"
+			"microchip,mcp4632-502"
+			"microchip,mcp4632-103"
+			"microchip,mcp4632-503"
+			"microchip,mcp4632-104"
+			"microchip,mcp4641-502"
+			"microchip,mcp4641-103"
+			"microchip,mcp4641-503"
+			"microchip,mcp4641-104"
+			"microchip,mcp4642-502"
+			"microchip,mcp4642-103"
+			"microchip,mcp4642-503"
+			"microchip,mcp4642-104"
+			"microchip,mcp4651-502"
+			"microchip,mcp4651-103"
+			"microchip,mcp4651-503"
+			"microchip,mcp4651-104"
+			"microchip,mcp4652-502"
+			"microchip,mcp4652-103"
+			"microchip,mcp4652-503"
+			"microchip,mcp4652-104"
+			"microchip,mcp4661-502"
+			"microchip,mcp4661-103"
+			"microchip,mcp4661-503"
+			"microchip,mcp4661-104"
+			"microchip,mcp4662-502"
+			"microchip,mcp4662-103"
+			"microchip,mcp4662-503"
+			"microchip,mcp4662-104"
+	-reg: Slave I2C address of the potentiometer
+
+Example:
+mcp4561: mcp4561@2e {
+	compatible = "microchip,mcp4561-103";
+	reg = <0x2e>;
+};
-- 
2.5.0

[PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Florian Vaussard]

This patch adds the necessary device tree binding to allow DT probing of
currently supported parts.

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/iio/potentiometer/mcp4531.c | 83 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/potentiometer/mcp4531.c b/drivers/iio/potentiometer/mcp4531.c
index 2251173..41a1e46 100644
--- a/drivers/iio/potentiometer/mcp4531.c
+++ b/drivers/iio/potentiometer/mcp4531.c
@@ -31,6 +31,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/iio/iio.h>
 
@@ -188,12 +190,84 @@ static const struct iio_info mcp4531_info = {
 	.driver_module = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id mcp45xx_of_match[] = {
+	{ .compatible = "microchip,mcp4531-502", .data = (void *)MCP453x_502 },
+	{ .compatible = "microchip,mcp4531-103", .data = (void *)MCP453x_103 },
+	{ .compatible = "microchip,mcp4531-503", .data = (void *)MCP453x_503 },
+	{ .compatible = "microchip,mcp4531-104", .data = (void *)MCP453x_104 },
+	{ .compatible = "microchip,mcp4532-502", .data = (void *)MCP453x_502 },
+	{ .compatible = "microchip,mcp4532-103", .data = (void *)MCP453x_103 },
+	{ .compatible = "microchip,mcp4532-503", .data = (void *)MCP453x_503 },
+	{ .compatible = "microchip,mcp4532-104", .data = (void *)MCP453x_104 },
+	{ .compatible = "microchip,mcp4541-502", .data = (void *)MCP454x_502 },
+	{ .compatible = "microchip,mcp4541-103", .data = (void *)MCP454x_103 },
+	{ .compatible = "microchip,mcp4541-503", .data = (void *)MCP454x_503 },
+	{ .compatible = "microchip,mcp4541-104", .data = (void *)MCP454x_104 },
+	{ .compatible = "microchip,mcp4542-502", .data = (void *)MCP454x_502 },
+	{ .compatible = "microchip,mcp4542-103", .data = (void *)MCP454x_103 },
+	{ .compatible = "microchip,mcp4542-503", .data = (void *)MCP454x_503 },
+	{ .compatible = "microchip,mcp4542-104", .data = (void *)MCP454x_104 },
+	{ .compatible = "microchip,mcp4551-502", .data = (void *)MCP455x_502 },
+	{ .compatible = "microchip,mcp4551-103", .data = (void *)MCP455x_103 },
+	{ .compatible = "microchip,mcp4551-503", .data = (void *)MCP455x_503 },
+	{ .compatible = "microchip,mcp4551-104", .data = (void *)MCP455x_104 },
+	{ .compatible = "microchip,mcp4552-502", .data = (void *)MCP455x_502 },
+	{ .compatible = "microchip,mcp4552-103", .data = (void *)MCP455x_103 },
+	{ .compatible = "microchip,mcp4552-503", .data = (void *)MCP455x_503 },
+	{ .compatible = "microchip,mcp4552-104", .data = (void *)MCP455x_104 },
+	{ .compatible = "microchip,mcp4561-502", .data = (void *)MCP456x_502 },
+	{ .compatible = "microchip,mcp4561-103", .data = (void *)MCP456x_103 },
+	{ .compatible = "microchip,mcp4561-503", .data = (void *)MCP456x_503 },
+	{ .compatible = "microchip,mcp4561-104", .data = (void *)MCP456x_104 },
+	{ .compatible = "microchip,mcp4562-502", .data = (void *)MCP456x_502 },
+	{ .compatible = "microchip,mcp4562-103", .data = (void *)MCP456x_103 },
+	{ .compatible = "microchip,mcp4562-503", .data = (void *)MCP456x_503 },
+	{ .compatible = "microchip,mcp4562-104", .data = (void *)MCP456x_104 },
+	{ .compatible = "microchip,mcp4631-502", .data = (void *)MCP463x_502 },
+	{ .compatible = "microchip,mcp4631-103", .data = (void *)MCP463x_103 },
+	{ .compatible = "microchip,mcp4631-503", .data = (void *)MCP463x_503 },
+	{ .compatible = "microchip,mcp4631-104", .data = (void *)MCP463x_104 },
+	{ .compatible = "microchip,mcp4632-502", .data = (void *)MCP463x_502 },
+	{ .compatible = "microchip,mcp4632-103", .data = (void *)MCP463x_103 },
+	{ .compatible = "microchip,mcp4632-503", .data = (void *)MCP463x_503 },
+	{ .compatible = "microchip,mcp4632-104", .data = (void *)MCP463x_104 },
+	{ .compatible = "microchip,mcp4641-502", .data = (void *)MCP464x_502 },
+	{ .compatible = "microchip,mcp4641-103", .data = (void *)MCP464x_103 },
+	{ .compatible = "microchip,mcp4641-503", .data = (void *)MCP464x_503 },
+	{ .compatible = "microchip,mcp4641-104", .data = (void *)MCP464x_104 },
+	{ .compatible = "microchip,mcp4642-502", .data = (void *)MCP464x_502 },
+	{ .compatible = "microchip,mcp4642-103", .data = (void *)MCP464x_103 },
+	{ .compatible = "microchip,mcp4642-503", .data = (void *)MCP464x_503 },
+	{ .compatible = "microchip,mcp4642-104", .data = (void *)MCP464x_104 },
+	{ .compatible = "microchip,mcp4651-502", .data = (void *)MCP465x_502 },
+	{ .compatible = "microchip,mcp4651-103", .data = (void *)MCP465x_103 },
+	{ .compatible = "microchip,mcp4651-503", .data = (void *)MCP465x_503 },
+	{ .compatible = "microchip,mcp4651-104", .data = (void *)MCP465x_104 },
+	{ .compatible = "microchip,mcp4652-502", .data = (void *)MCP465x_502 },
+	{ .compatible = "microchip,mcp4652-103", .data = (void *)MCP465x_103 },
+	{ .compatible = "microchip,mcp4652-503", .data = (void *)MCP465x_503 },
+	{ .compatible = "microchip,mcp4652-104", .data = (void *)MCP465x_104 },
+	{ .compatible = "microchip,mcp4661-502", .data = (void *)MCP466x_502 },
+	{ .compatible = "microchip,mcp4661-103", .data = (void *)MCP466x_103 },
+	{ .compatible = "microchip,mcp4661-503", .data = (void *)MCP466x_503 },
+	{ .compatible = "microchip,mcp4661-104", .data = (void *)MCP466x_104 },
+	{ .compatible = "microchip,mcp4662-502", .data = (void *)MCP466x_502 },
+	{ .compatible = "microchip,mcp4662-103", .data = (void *)MCP466x_103 },
+	{ .compatible = "microchip,mcp4662-503", .data = (void *)MCP466x_503 },
+	{ .compatible = "microchip,mcp4662-104", .data = (void *)MCP466x_104 },
+	{ /*sentinel*/ }
+};
+#endif
+
 static int mcp4531_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
+	unsigned long devid;
 	struct mcp4531_data *data;
 	struct iio_dev *indio_dev;
+	const struct of_device_id *match;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -201,13 +275,19 @@ static int mcp4531_probe(struct i2c_client *client,
 		return -EOPNOTSUPP;
 	}
 
+	match = of_match_device(of_match_ptr(mcp45xx_of_match), dev);
+	if (match)
+		devid = (int)of_device_get_match_data(dev);
+	else
+		devid = id->driver_data;
+
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
-	data->cfg = &mcp4531_cfg[id->driver_data];
+	data->cfg = &mcp4531_cfg[devid];
 
 	indio_dev->dev.parent = dev;
 	indio_dev->info = &mcp4531_info;
@@ -290,6 +370,7 @@ MODULE_DEVICE_TABLE(i2c, mcp4531_id);
 static struct i2c_driver mcp4531_driver = {
 	.driver = {
 		.name	= "mcp4531",
+		.of_match_table = of_match_ptr(mcp45xx_of_match),
 	},
 	.probe		= mcp4531_probe,
 	.id_table	= mcp4531_id,
-- 
2.5.0

Re: [PATCH 1/3] iio: potentiometer: mcp4531: Add support for MCP454x, MCP456x, MCP464x and MCP466x

[Peter Rosin]

On 2016-06-21 08:55, Florian Vaussard wrote:
> This patch adds support for MCP454x, MCP456x, MCP464x and MCP466x parts.
> The main difference with currently supported parts (MCP453x and alike) is
> the addition of a non-volatile memory in order to recall the wiper setting
> at power-on. This feature is currently not supported and only the
> volatile memory is used to set the wiper.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[kbuild test robot]

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

Hi,

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.7-rc4 next-20160620]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Florian-Vaussard/iio-potentiometer-mcp4531-New-parts-and-device-tree/20160621-150032
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-i0-201625 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/iio/potentiometer/mcp4531.c: In function 'mcp4531_probe':
>> drivers/iio/potentiometer/mcp4531.c:280:11: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
      devid = (int)of_device_get_match_data(dev);
              ^

vim +280 drivers/iio/potentiometer/mcp4531.c

   264				 const struct i2c_device_id *id)
   265	{
   266		struct device *dev = &client->dev;
   267		unsigned long devid;
   268		struct mcp4531_data *data;
   269		struct iio_dev *indio_dev;
   270		const struct of_device_id *match;
   271	
   272		if (!i2c_check_functionality(client->adapter,
   273					     I2C_FUNC_SMBUS_WORD_DATA)) {
   274			dev_err(dev, "SMBUS Word Data not supported\n");
   275			return -EOPNOTSUPP;
   276		}
   277	
   278		match = of_match_device(of_match_ptr(mcp45xx_of_match), dev);
   279		if (match)
 > 280			devid = (int)of_device_get_match_data(dev);
   281		else
   282			devid = id->driver_data;
   283	
   284		indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
   285		if (!indio_dev)
   286			return -ENOMEM;
   287		data = iio_priv(indio_dev);
   288		i2c_set_clientdata(client, indio_dev);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 28400 bytes --]

Re: [PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Peter Rosin]

On 2016-06-21 08:55, Florian Vaussard wrote:
> Add the device tree documentation for all the supported parts. Apart the
> compatible string and standard I2C binding, no other binding is currently
> needed.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>  .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> new file mode 100644
> index 0000000..b052299
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> @@ -0,0 +1,84 @@
> +* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
> +  driver
> +
> +The node for this driver must be a child node of a I2C controller, hence
> +all mandatory properties for your controller must be specified. See directory:
> +
> +        Documentation/devicetree/bindings/i2c
> +
> +for more details.
> +
> +Required properties:
> +	- compatible:  	Must be one of the following, depending on the
> +			model:
> +			"microchip,mcp4531-502"
> +			"microchip,mcp4531-103"
> +			"microchip,mcp4531-503"

*snip*

I'm not directly opposed, but I have used the following and DT booting
works like a charm here.

		mcp4651-104@28 {
			compatible = "mcp4651-104";
			reg = <0x28>;
		};

But, I suppose some DT documentation is not bad, and my understanding of
the device instantiation process and the i2c/dt interactions are not
complete, so my DT snippet might be an abomination? I'll leave the
decision if this is needed to someone with more experience on how other
drivers handle this.

Cheers,
Peter

Re: [PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Florian Vaussard]

Hello,

On 06/21/2016 09:38 AM, Peter Rosin wrote:
> On 2016-06-21 08:55, Florian Vaussard wrote:
>> Add the device tree documentation for all the supported parts. Apart the
>> compatible string and standard I2C binding, no other binding is currently
>> needed.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>  .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>> new file mode 100644
>> index 0000000..b052299
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>> @@ -0,0 +1,84 @@
>> +* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
>> +  driver
>> +
>> +The node for this driver must be a child node of a I2C controller, hence
>> +all mandatory properties for your controller must be specified. See directory:
>> +
>> +        Documentation/devicetree/bindings/i2c
>> +
>> +for more details.
>> +
>> +Required properties:
>> +	- compatible:  	Must be one of the following, depending on the
>> +			model:
>> +			"microchip,mcp4531-502"
>> +			"microchip,mcp4531-103"
>> +			"microchip,mcp4531-503"
> 
> *snip*
> 
> I'm not directly opposed, but I have used the following and DT booting
> works like a charm here.
> 
> 		mcp4651-104@28 {
> 			compatible = "mcp4651-104";
> 			reg = <0x28>;
> 		};
> 

I was not aware that the i2c subsystem had a facility to match the compatible
string against i2c_device_id. Good to know.

> But, I suppose some DT documentation is not bad, and my understanding of
> the device instantiation process and the i2c/dt interactions are not
> complete, so my DT snippet might be an abomination? I'll leave the
> decision if this is needed to someone with more experience on how other
> drivers handle this.
> 

Your compatible string is missing the vendor ID (microchip). This is the only
objection that I can see to your proposition. Let's wait to hear from a DT
maintainer.

Thanks,
Florian

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Peter Rosin]

On 2016-06-21 08:55, Florian Vaussard wrote:
> This patch adds the necessary device tree binding to allow DT probing of
> currently supported parts.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>  drivers/iio/potentiometer/mcp4531.c | 83 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/potentiometer/mcp4531.c b/drivers/iio/potentiometer/mcp4531.c
> index 2251173..41a1e46 100644
> --- a/drivers/iio/potentiometer/mcp4531.c
> +++ b/drivers/iio/potentiometer/mcp4531.c
> @@ -31,6 +31,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <linux/iio/iio.h>
>  
> @@ -188,12 +190,84 @@ static const struct iio_info mcp4531_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id mcp45xx_of_match[] = {

Should be named mcp4531_of_match, also the casting to void and
then back to an integer type is ugly. Would it work to store a
pointer directly into the mcp4531_cfg array instead? I.e.

	{ .compatible = "microchip,mcp4531-502", .data = &mcp4531_cfg[MCP453x_502] },
etc

and then adjust accordingly in mcp4531_probe()?

That is, if you need this patch at all, see my reply to 2/3...

Cheers,
Peter

> +	{ .compatible = "microchip,mcp4531-502", .data = (void *)MCP453x_502 },
> +	{ .compatible = "microchip,mcp4531-103", .data = (void *)MCP453x_103 },
> +	{ .compatible = "microchip,mcp4531-503", .data = (void *)MCP453x_503 },
> +	{ .compatible = "microchip,mcp4531-104", .data = (void *)MCP453x_104 },
> +	{ .compatible = "microchip,mcp4532-502", .data = (void *)MCP453x_502 },
> +	{ .compatible = "microchip,mcp4532-103", .data = (void *)MCP453x_103 },
> +	{ .compatible = "microchip,mcp4532-503", .data = (void *)MCP453x_503 },
> +	{ .compatible = "microchip,mcp4532-104", .data = (void *)MCP453x_104 },
> +	{ .compatible = "microchip,mcp4541-502", .data = (void *)MCP454x_502 },
> +	{ .compatible = "microchip,mcp4541-103", .data = (void *)MCP454x_103 },
> +	{ .compatible = "microchip,mcp4541-503", .data = (void *)MCP454x_503 },
> +	{ .compatible = "microchip,mcp4541-104", .data = (void *)MCP454x_104 },
> +	{ .compatible = "microchip,mcp4542-502", .data = (void *)MCP454x_502 },
> +	{ .compatible = "microchip,mcp4542-103", .data = (void *)MCP454x_103 },
> +	{ .compatible = "microchip,mcp4542-503", .data = (void *)MCP454x_503 },
> +	{ .compatible = "microchip,mcp4542-104", .data = (void *)MCP454x_104 },
> +	{ .compatible = "microchip,mcp4551-502", .data = (void *)MCP455x_502 },
> +	{ .compatible = "microchip,mcp4551-103", .data = (void *)MCP455x_103 },
> +	{ .compatible = "microchip,mcp4551-503", .data = (void *)MCP455x_503 },
> +	{ .compatible = "microchip,mcp4551-104", .data = (void *)MCP455x_104 },
> +	{ .compatible = "microchip,mcp4552-502", .data = (void *)MCP455x_502 },
> +	{ .compatible = "microchip,mcp4552-103", .data = (void *)MCP455x_103 },
> +	{ .compatible = "microchip,mcp4552-503", .data = (void *)MCP455x_503 },
> +	{ .compatible = "microchip,mcp4552-104", .data = (void *)MCP455x_104 },
> +	{ .compatible = "microchip,mcp4561-502", .data = (void *)MCP456x_502 },
> +	{ .compatible = "microchip,mcp4561-103", .data = (void *)MCP456x_103 },
> +	{ .compatible = "microchip,mcp4561-503", .data = (void *)MCP456x_503 },
> +	{ .compatible = "microchip,mcp4561-104", .data = (void *)MCP456x_104 },
> +	{ .compatible = "microchip,mcp4562-502", .data = (void *)MCP456x_502 },
> +	{ .compatible = "microchip,mcp4562-103", .data = (void *)MCP456x_103 },
> +	{ .compatible = "microchip,mcp4562-503", .data = (void *)MCP456x_503 },
> +	{ .compatible = "microchip,mcp4562-104", .data = (void *)MCP456x_104 },
> +	{ .compatible = "microchip,mcp4631-502", .data = (void *)MCP463x_502 },
> +	{ .compatible = "microchip,mcp4631-103", .data = (void *)MCP463x_103 },
> +	{ .compatible = "microchip,mcp4631-503", .data = (void *)MCP463x_503 },
> +	{ .compatible = "microchip,mcp4631-104", .data = (void *)MCP463x_104 },
> +	{ .compatible = "microchip,mcp4632-502", .data = (void *)MCP463x_502 },
> +	{ .compatible = "microchip,mcp4632-103", .data = (void *)MCP463x_103 },
> +	{ .compatible = "microchip,mcp4632-503", .data = (void *)MCP463x_503 },
> +	{ .compatible = "microchip,mcp4632-104", .data = (void *)MCP463x_104 },
> +	{ .compatible = "microchip,mcp4641-502", .data = (void *)MCP464x_502 },
> +	{ .compatible = "microchip,mcp4641-103", .data = (void *)MCP464x_103 },
> +	{ .compatible = "microchip,mcp4641-503", .data = (void *)MCP464x_503 },
> +	{ .compatible = "microchip,mcp4641-104", .data = (void *)MCP464x_104 },
> +	{ .compatible = "microchip,mcp4642-502", .data = (void *)MCP464x_502 },
> +	{ .compatible = "microchip,mcp4642-103", .data = (void *)MCP464x_103 },
> +	{ .compatible = "microchip,mcp4642-503", .data = (void *)MCP464x_503 },
> +	{ .compatible = "microchip,mcp4642-104", .data = (void *)MCP464x_104 },
> +	{ .compatible = "microchip,mcp4651-502", .data = (void *)MCP465x_502 },
> +	{ .compatible = "microchip,mcp4651-103", .data = (void *)MCP465x_103 },
> +	{ .compatible = "microchip,mcp4651-503", .data = (void *)MCP465x_503 },
> +	{ .compatible = "microchip,mcp4651-104", .data = (void *)MCP465x_104 },
> +	{ .compatible = "microchip,mcp4652-502", .data = (void *)MCP465x_502 },
> +	{ .compatible = "microchip,mcp4652-103", .data = (void *)MCP465x_103 },
> +	{ .compatible = "microchip,mcp4652-503", .data = (void *)MCP465x_503 },
> +	{ .compatible = "microchip,mcp4652-104", .data = (void *)MCP465x_104 },
> +	{ .compatible = "microchip,mcp4661-502", .data = (void *)MCP466x_502 },
> +	{ .compatible = "microchip,mcp4661-103", .data = (void *)MCP466x_103 },
> +	{ .compatible = "microchip,mcp4661-503", .data = (void *)MCP466x_503 },
> +	{ .compatible = "microchip,mcp4661-104", .data = (void *)MCP466x_104 },
> +	{ .compatible = "microchip,mcp4662-502", .data = (void *)MCP466x_502 },
> +	{ .compatible = "microchip,mcp4662-103", .data = (void *)MCP466x_103 },
> +	{ .compatible = "microchip,mcp4662-503", .data = (void *)MCP466x_503 },
> +	{ .compatible = "microchip,mcp4662-104", .data = (void *)MCP466x_104 },
> +	{ /*sentinel*/ }
> +};
> +#endif
> +
>  static int mcp4531_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> +	unsigned long devid;
>  	struct mcp4531_data *data;
>  	struct iio_dev *indio_dev;
> +	const struct of_device_id *match;
>  
>  	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_WORD_DATA)) {
> @@ -201,13 +275,19 @@ static int mcp4531_probe(struct i2c_client *client,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	match = of_match_device(of_match_ptr(mcp45xx_of_match), dev);
> +	if (match)
> +		devid = (int)of_device_get_match_data(dev);
> +	else
> +		devid = id->driver_data;
> +
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> -	data->cfg = &mcp4531_cfg[id->driver_data];
> +	data->cfg = &mcp4531_cfg[devid];
>  
>  	indio_dev->dev.parent = dev;
>  	indio_dev->info = &mcp4531_info;
> @@ -290,6 +370,7 @@ MODULE_DEVICE_TABLE(i2c, mcp4531_id);
>  static struct i2c_driver mcp4531_driver = {
>  	.driver = {
>  		.name	= "mcp4531",
> +		.of_match_table = of_match_ptr(mcp45xx_of_match),
>  	},
>  	.probe		= mcp4531_probe,
>  	.id_table	= mcp4531_id,
> 

Re: [PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Rob Herring]

On Tue, Jun 21, 2016 at 09:49:20AM +0200, Florian Vaussard wrote:
> Hello,
> 
> On 06/21/2016 09:38 AM, Peter Rosin wrote:
> > On 2016-06-21 08:55, Florian Vaussard wrote:
> >> Add the device tree documentation for all the supported parts. Apart the
> >> compatible string and standard I2C binding, no other binding is currently
> >> needed.
> >>
> >> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> >> ---
> >>  .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
> >>  1 file changed, 84 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> >> new file mode 100644
> >> index 0000000..b052299
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> >> @@ -0,0 +1,84 @@
> >> +* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
> >> +  driver
> >> +
> >> +The node for this driver must be a child node of a I2C controller, hence
> >> +all mandatory properties for your controller must be specified. See directory:
> >> +
> >> +        Documentation/devicetree/bindings/i2c
> >> +
> >> +for more details.
> >> +
> >> +Required properties:
> >> +	- compatible:  	Must be one of the following, depending on the
> >> +			model:
> >> +			"microchip,mcp4531-502"
> >> +			"microchip,mcp4531-103"
> >> +			"microchip,mcp4531-503"
> > 
> > *snip*
> > 
> > I'm not directly opposed, but I have used the following and DT booting
> > works like a charm here.
> > 
> > 		mcp4651-104@28 {
> > 			compatible = "mcp4651-104";
> > 			reg = <0x28>;
> > 		};
> > 
> 
> I was not aware that the i2c subsystem had a facility to match the compatible
> string against i2c_device_id. Good to know.
> 
> > But, I suppose some DT documentation is not bad, and my understanding of
> > the device instantiation process and the i2c/dt interactions are not
> > complete, so my DT snippet might be an abomination? I'll leave the
> > decision if this is needed to someone with more experience on how other
> > drivers handle this.
> > 
> 
> Your compatible string is missing the vendor ID (microchip). This is the only
> objection that I can see to your proposition. Let's wait to hear from a DT
> maintainer.

I2C subsystem happens to ignore the vendor prefix in matching. You 
should not rely on that and provide proper compatible strings.

Rob

Re: [PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Rob Herring]

On Tue, Jun 21, 2016 at 08:55:36AM +0200, Florian Vaussard wrote:
> Add the device tree documentation for all the supported parts. Apart the
> compatible string and standard I2C binding, no other binding is currently
> needed.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>  .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> new file mode 100644
> index 0000000..b052299
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
> @@ -0,0 +1,84 @@
> +* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
> +  driver
> +
> +The node for this driver must be a child node of a I2C controller, hence
> +all mandatory properties for your controller must be specified. See directory:
> +
> +        Documentation/devicetree/bindings/i2c
> +
> +for more details.
> +
> +Required properties:
> +	- compatible:  	Must be one of the following, depending on the
> +			model:
> +			"microchip,mcp4531-502"

These can go in trivial-devices.txt

> +			"microchip,mcp4531-103"
> +			"microchip,mcp4531-503"
> +			"microchip,mcp4531-104"
> +			"microchip,mcp4532-502"
> +			"microchip,mcp4532-103"
> +			"microchip,mcp4532-503"
> +			"microchip,mcp4532-104"
> +			"microchip,mcp4541-502"
> +			"microchip,mcp4541-103"
> +			"microchip,mcp4541-503"
> +			"microchip,mcp4541-104"
> +			"microchip,mcp4542-502"
> +			"microchip,mcp4542-103"
> +			"microchip,mcp4542-503"
> +			"microchip,mcp4542-104"
> +			"microchip,mcp4551-502"
> +			"microchip,mcp4551-103"
> +			"microchip,mcp4551-503"
> +			"microchip,mcp4551-104"
> +			"microchip,mcp4552-502"
> +			"microchip,mcp4552-103"
> +			"microchip,mcp4552-503"
> +			"microchip,mcp4552-104"
> +			"microchip,mcp4561-502"
> +			"microchip,mcp4561-103"
> +			"microchip,mcp4561-503"
> +			"microchip,mcp4561-104"
> +			"microchip,mcp4562-502"
> +			"microchip,mcp4562-103"
> +			"microchip,mcp4562-503"
> +			"microchip,mcp4562-104"
> +			"microchip,mcp4631-502"
> +			"microchip,mcp4631-103"
> +			"microchip,mcp4631-503"
> +			"microchip,mcp4631-104"
> +			"microchip,mcp4632-502"
> +			"microchip,mcp4632-103"
> +			"microchip,mcp4632-503"
> +			"microchip,mcp4632-104"
> +			"microchip,mcp4641-502"
> +			"microchip,mcp4641-103"
> +			"microchip,mcp4641-503"
> +			"microchip,mcp4641-104"
> +			"microchip,mcp4642-502"
> +			"microchip,mcp4642-103"
> +			"microchip,mcp4642-503"
> +			"microchip,mcp4642-104"
> +			"microchip,mcp4651-502"
> +			"microchip,mcp4651-103"
> +			"microchip,mcp4651-503"
> +			"microchip,mcp4651-104"
> +			"microchip,mcp4652-502"
> +			"microchip,mcp4652-103"
> +			"microchip,mcp4652-503"
> +			"microchip,mcp4652-104"
> +			"microchip,mcp4661-502"
> +			"microchip,mcp4661-103"
> +			"microchip,mcp4661-503"
> +			"microchip,mcp4661-104"
> +			"microchip,mcp4662-502"
> +			"microchip,mcp4662-103"
> +			"microchip,mcp4662-503"
> +			"microchip,mcp4662-104"
> +	-reg: Slave I2C address of the potentiometer
> +
> +Example:
> +mcp4561: mcp4561@2e {
> +	compatible = "microchip,mcp4561-103";
> +	reg = <0x2e>;
> +};
> -- 
> 2.5.0
> 

Re: [PATCH 2/3] iio: potentiometer: mcp4531: Add device tree binding documentation

[Florian Vaussard]

Hi Rob,

Le 21. 06. 16 à 23:43, Rob Herring a écrit :
> On Tue, Jun 21, 2016 at 08:55:36AM +0200, Florian Vaussard wrote:
>> Add the device tree documentation for all the supported parts. Apart the
>> compatible string and standard I2C binding, no other binding is currently
>> needed.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>  .../bindings/iio/potentiometer/mcp4531.txt         | 84 ++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>> new file mode 100644
>> index 0000000..b052299
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>> @@ -0,0 +1,84 @@
>> +* Microchip MCP453X/454X/455X/456X/463X/464X/465X/466X Digital Potentiometer
>> +  driver
>> +
>> +The node for this driver must be a child node of a I2C controller, hence
>> +all mandatory properties for your controller must be specified. See directory:
>> +
>> +        Documentation/devicetree/bindings/i2c
>> +
>> +for more details.
>> +
>> +Required properties:
>> +	- compatible:  	Must be one of the following, depending on the
>> +			model:
>> +			"microchip,mcp4531-502"
> 
> These can go in trivial-devices.txt
> 

Yes indeed. I will fix.

Thanks for the review.

Florian

>> +			"microchip,mcp4531-103"
>> +			"microchip,mcp4531-503"
>> +			"microchip,mcp4531-104"
>> +			"microchip,mcp4532-502"
>> +			"microchip,mcp4532-103"
>> +			"microchip,mcp4532-503"
>> +			"microchip,mcp4532-104"
>> +			"microchip,mcp4541-502"
>> +			"microchip,mcp4541-103"
>> +			"microchip,mcp4541-503"
>> +			"microchip,mcp4541-104"
>> +			"microchip,mcp4542-502"
>> +			"microchip,mcp4542-103"
>> +			"microchip,mcp4542-503"
>> +			"microchip,mcp4542-104"
>> +			"microchip,mcp4551-502"
>> +			"microchip,mcp4551-103"
>> +			"microchip,mcp4551-503"
>> +			"microchip,mcp4551-104"
>> +			"microchip,mcp4552-502"
>> +			"microchip,mcp4552-103"
>> +			"microchip,mcp4552-503"
>> +			"microchip,mcp4552-104"
>> +			"microchip,mcp4561-502"
>> +			"microchip,mcp4561-103"
>> +			"microchip,mcp4561-503"
>> +			"microchip,mcp4561-104"
>> +			"microchip,mcp4562-502"
>> +			"microchip,mcp4562-103"
>> +			"microchip,mcp4562-503"
>> +			"microchip,mcp4562-104"
>> +			"microchip,mcp4631-502"
>> +			"microchip,mcp4631-103"
>> +			"microchip,mcp4631-503"
>> +			"microchip,mcp4631-104"
>> +			"microchip,mcp4632-502"
>> +			"microchip,mcp4632-103"
>> +			"microchip,mcp4632-503"
>> +			"microchip,mcp4632-104"
>> +			"microchip,mcp4641-502"
>> +			"microchip,mcp4641-103"
>> +			"microchip,mcp4641-503"
>> +			"microchip,mcp4641-104"
>> +			"microchip,mcp4642-502"
>> +			"microchip,mcp4642-103"
>> +			"microchip,mcp4642-503"
>> +			"microchip,mcp4642-104"
>> +			"microchip,mcp4651-502"
>> +			"microchip,mcp4651-103"
>> +			"microchip,mcp4651-503"
>> +			"microchip,mcp4651-104"
>> +			"microchip,mcp4652-502"
>> +			"microchip,mcp4652-103"
>> +			"microchip,mcp4652-503"
>> +			"microchip,mcp4652-104"
>> +			"microchip,mcp4661-502"
>> +			"microchip,mcp4661-103"
>> +			"microchip,mcp4661-503"
>> +			"microchip,mcp4661-104"
>> +			"microchip,mcp4662-502"
>> +			"microchip,mcp4662-103"
>> +			"microchip,mcp4662-503"
>> +			"microchip,mcp4662-104"
>> +	-reg: Slave I2C address of the potentiometer
>> +
>> +Example:
>> +mcp4561: mcp4561@2e {
>> +	compatible = "microchip,mcp4561-103";
>> +	reg = <0x2e>;
>> +};
>> -- 
>> 2.5.0
>>

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Florian Vaussard]

Hello Peter,

Le 21. 06. 16 à 09:51, Peter Rosin a écrit :
> On 2016-06-21 08:55, Florian Vaussard wrote:
>> This patch adds the necessary device tree binding to allow DT probing of
>> currently supported parts.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>  drivers/iio/potentiometer/mcp4531.c | 83 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/potentiometer/mcp4531.c b/drivers/iio/potentiometer/mcp4531.c
>> index 2251173..41a1e46 100644
>> --- a/drivers/iio/potentiometer/mcp4531.c
>> +++ b/drivers/iio/potentiometer/mcp4531.c
>> @@ -31,6 +31,8 @@
>>  #include <linux/module.h>
>>  #include <linux/i2c.h>
>>  #include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>>  
>>  #include <linux/iio/iio.h>
>>  
>> @@ -188,12 +190,84 @@ static const struct iio_info mcp4531_info = {
>>  	.driver_module = THIS_MODULE,
>>  };
>>  
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id mcp45xx_of_match[] = {
> 
> Should be named mcp4531_of_match, also the casting to void and
> then back to an integer type is ugly. Would it work to store a
> pointer directly into the mcp4531_cfg array instead? I.e.
> 
> 	{ .compatible = "microchip,mcp4531-502", .data = &mcp4531_cfg[MCP453x_502] },
> etc
> 
> and then adjust accordingly in mcp4531_probe()?
> 

Yes the casting is pretty ugly. And kbuild agrees with you :) I took this from
another driver (lame excuse). Your suggestion is sensible, I will give it a try.

> That is, if you need this patch at all, see my reply to 2/3...
> 

This seems necessary in order to have the vendor ID in the compatible string.

Thanks for your review.

Florian

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Peter Rosin]

On 2016-06-22 08:22, Florian Vaussard wrote:
> Hello Peter,
> 
> Le 21. 06. 16 à 09:51, Peter Rosin a écrit :
>> That is, if you need this patch at all, see my reply to 2/3...
>>
> 
> This seems necessary in order to have the vendor ID in the compatible string.

Hmm, I don't think so. The way I read the response from Rob was that
*my* device tree snippet should not assume that the i2c subsystem
ignores the vendor. So, I think that even w/o this patch a DT entry
like

		mcp4651-104@28 {
			compatible = "microchip,mcp4651-104";
			reg = <0x28>;
		};

will work, precisely since i2c ignores the microchip, part (and thus
allows you to omit/misspell it). I.e. I think that Rob is concerned
with how the DT is documented/defined, and not so much about how it
is then implemented in Linux.

I haven't actually tested that though, so I may be completely wrong
in all of the above statements...

Cheers,
Peter

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Florian Vaussard]

Hello Peter,

On 06/22/2016 09:06 AM, Peter Rosin wrote:
> On 2016-06-22 08:22, Florian Vaussard wrote:
>> Hello Peter,
>>
>> Le 21. 06. 16 à 09:51, Peter Rosin a écrit :
>>> That is, if you need this patch at all, see my reply to 2/3...
>>>
>>
>> This seems necessary in order to have the vendor ID in the compatible string.
> 
> Hmm, I don't think so. The way I read the response from Rob was that
> *my* device tree snippet should not assume that the i2c subsystem
> ignores the vendor. So, I think that even w/o this patch a DT entry
> like
> 
> 		mcp4651-104@28 {
> 			compatible = "microchip,mcp4651-104";
> 			reg = <0x28>;
> 		};
> 
> will work, precisely since i2c ignores the microchip, part (and thus
> allows you to omit/misspell it). I.e. I think that Rob is concerned
> with how the DT is documented/defined, and not so much about how it
> is then implemented in Linux.
> 

The way that I read Rob's reply is reverse: you should not rely on the matching
done by i2c subsystem and provide a proper of_device_id table instead.

I think that ignoring the vendor when matching the compatible, like what is
currently done by the i2c subsystem, is bad. What happens if you have two chips
from two companies with the same name (yes it happens)? You may need different
data to address the small specificities of each chip, or even worse a different
driver at all. This implies that we should explicitly match the vendor as well,
otherwise we would be unable to cope with such cases.

Moreover, if we look at the current i2c drivers, a whole bunch of them already
have a of_device_id table:

(on v4.6)

$ git grep -l i2c_device_id | wc -l
636
$ git grep -l i2c_device_id | xargs grep -l 'of_device_id' | wc -l
197

Currently 30% of all i2c drivers explicitly declare DT compatible strings. And I
am sure that this number will keep increasing as drivers are converted to DT.

Regards,
Florian

Re: [PATCH 3/3] iio: potentiometer: mcp4531: Add device tree binding

[Jonathan Cameron]

On 23/06/16 09:50, Florian Vaussard wrote:
> Hello Peter,
> 
> On 06/22/2016 09:06 AM, Peter Rosin wrote:
>> On 2016-06-22 08:22, Florian Vaussard wrote:
>>> Hello Peter,
>>>
>>> Le 21. 06. 16 à 09:51, Peter Rosin a écrit :
>>>> That is, if you need this patch at all, see my reply to 2/3...
>>>>
>>>
>>> This seems necessary in order to have the vendor ID in the compatible string.
>>
>> Hmm, I don't think so. The way I read the response from Rob was that
>> *my* device tree snippet should not assume that the i2c subsystem
>> ignores the vendor. So, I think that even w/o this patch a DT entry
>> like
>>
>> 		mcp4651-104@28 {
>> 			compatible = "microchip,mcp4651-104";
>> 			reg = <0x28>;
>> 		};
>>
>> will work, precisely since i2c ignores the microchip, part (and thus
>> allows you to omit/misspell it). I.e. I think that Rob is concerned
>> with how the DT is documented/defined, and not so much about how it
>> is then implemented in Linux.
>>
> 
> The way that I read Rob's reply is reverse: you should not rely on the matching
> done by i2c subsystem and provide a proper of_device_id table instead.
The original 'quirk' of matching in i2c and spi based on the part number
alone has been the source of much grief.  Unfortunately it is there
and there are numerous device trees out there using it.

The preferred option is a nice conventional table as you added in patch 2.

We just haven't gotten round to updating all the drivers yet and as it
'works' right now there is little motivation to do it unless someone is
working on the driver as you are.
> 
> I think that ignoring the vendor when matching the compatible, like what is
> currently done by the i2c subsystem, is bad. What happens if you have two chips
> from two companies with the same name (yes it happens)? You may need different
> data to address the small specificities of each chip, or even worse a different
> driver at all. This implies that we should explicitly match the vendor as well,
> otherwise we would be unable to cope with such cases.
Exactly and it's already happened I believe... With two totally different parts
having the same part number rather than the similar ones case.
> 
> Moreover, if we look at the current i2c drivers, a whole bunch of them already
> have a of_device_id table:
> 
> (on v4.6)
> 
> $ git grep -l i2c_device_id | wc -l
> 636
> $ git grep -l i2c_device_id | xargs grep -l 'of_device_id' | wc -l
> 197
> 
> Currently 30% of all i2c drivers explicitly declare DT compatible strings. And I
> am sure that this number will keep increasing as drivers are converted to DT.
> 
> Regards,
> Florian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] iio: potentiometer: mcp4531: New parts and device tree

[Jonathan Cameron]

On 21/06/16 07:55, Florian Vaussard wrote:
> Hello,
> 
> This series first adds support for parts missing from mcp4531 driver
> (MCP454x, MCP456x, MCP464x and MCP466x). It then introduces the necessary
> device tree binding to perform DT boot.
> 
> Tested with MCP4561-103 and MCP4561-503 (DT boot).
> 
Series looks good to me other than the various bits Peter pointed out.

Thanks,

Jonathan
> Best regards,
> Florian
> 
> Florian Vaussard (3):
>   iio: potentiometer: mcp4531: Add support for MCP454x, MCP456x, MCP464x
>     and MCP466x
>   iio: potentiometer: mcp4531: Add device tree binding documentation
>   iio: potentiometer: mcp4531: Add device tree binding
> 
>  .../bindings/iio/potentiometer/mcp4531.txt         |  84 +++++++++++
>  drivers/iio/potentiometer/Kconfig                  |   6 +-
>  drivers/iio/potentiometer/mcp4531.c                | 155 ++++++++++++++++++++-
>  3 files changed, 242 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4531.txt
>