[PATCH RFC 0/4] Adding DT functionality to w1 busses

[PATCH RFC 0/4] Adding DT functionality to w1 busses

[Daniel Mack]

In order to fully move battery-supplied devices over to devicetree, the
onewire subsystem must get some updates.

Currently, the w1 bus system works like this. Device families, such as
battery controllers etc, are registered to the w1 core. Once a master
device is probed, it starts scanning the bus. Slave devices that are
revealed through this scan and that match previously registered
families are then registered, and their .add_slave() callback is
invoked.

Some devices, such as the ds2760, use that callback to call
platform_device_alloc() at runtime with a fixed device name to
instanciate their only user. That user does the actual work, while the
slave device merely functions as an I/O proxy. In the user
implementation, the w1 slave device is accessible through
dev->parent.

Now, for devicetree environments, this has to change a bit. First, slave
devices need to have a matching table so they can be listed as sub-nodes
of their master bus controller. Second, the core needs to match the
entries in these tables against the sub-nodes of the master node.
These two are trivial to do.

The next question is how the w1 slave device and its user(s) are linked
together. I'm proposing a DT layout with the following example:

	onewire {
		compatible = "w1-gpio";

		w1_slave: slave@0 {
			compatible = "maxim,w1-ds2760";
		};
	};

	battery-supply {
		compatible = "maxim,ds2760-supply";
		w1-slave = <&w1_slave>;
	};

Another option would be to place the supply user as another nested node
under the slave device, but I'm not sure if that's beneficial. I'm open
to discussions though.

This patch set implements the changes for the above in the w1 core.
Changing the supply users is left for later when the basics have
settlerd, but a helper to reference slave devices through phandles is
already provided.


Thanks,
Daniel


Daniel Mack (4):
  dt-bindings: w1: document sub-node bindings for DS2760
  w1: core: match sub-nodes of bus masters in devicetree
  w1: core: provide helper to look up w1 slaves through devicetree nodes
  w1: ds2760: add devicetree matching glue

 .../devicetree/bindings/w1/w1-ds2760.txt      |  9 +++++
 .../devicetree/bindings/w1/w1-gpio.txt        |  6 +++
 Documentation/devicetree/bindings/w1/w1.txt   | 17 ++++++++
 drivers/w1/slaves/w1_ds2760.c                 | 15 ++++++-
 drivers/w1/w1.c                               | 40 +++++++++++++++++++
 include/linux/w1.h                            | 13 ++++++
 6 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/w1/w1-ds2760.txt
 create mode 100644 Documentation/devicetree/bindings/w1/w1.txt

-- 
2.17.1

[PATCH RFC 1/4] dt-bindings: w1: document sub-node bindings for DS2760

[Daniel Mack]

This patch add a generic w1 bindings document that merely describes how
slave deviceses are grouped under master nodes. It also adds a specific
binding for the ds2760 battery monitor.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 .../devicetree/bindings/w1/w1-ds2760.txt        |  9 +++++++++
 .../devicetree/bindings/w1/w1-gpio.txt          |  6 ++++++
 Documentation/devicetree/bindings/w1/w1.txt     | 17 +++++++++++++++++
 3 files changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/w1/w1-ds2760.txt
 create mode 100644 Documentation/devicetree/bindings/w1/w1.txt

diff --git a/Documentation/devicetree/bindings/w1/w1-ds2760.txt b/Documentation/devicetree/bindings/w1/w1-ds2760.txt
new file mode 100644
index 000000000000..86a0f4c573eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/w1/w1-ds2760.txt
@@ -0,0 +1,9 @@
+Devicetree bindings for Maxim DS2760
+====================================
+
+The ds2760 is a w1 slave device and must hence have its sub-node in DT
+under a w1 bus master node.
+
+Required properties:
+- compatible: must be "maxim,ds2760-w1"
+
diff --git a/Documentation/devicetree/bindings/w1/w1-gpio.txt b/Documentation/devicetree/bindings/w1/w1-gpio.txt
index 6e09c35d9f1a..a3e0e964b260 100644
--- a/Documentation/devicetree/bindings/w1/w1-gpio.txt
+++ b/Documentation/devicetree/bindings/w1/w1-gpio.txt
@@ -13,10 +13,16 @@ Optional properties:
  - linux,open-drain: if specified, the data pin is considered in
 		     open-drain mode.
 
+Also refer to the generic w1.txt document.
+
 Examples:
 
 	onewire@0 {
 		compatible = "w1-gpio";
 		gpios = <&gpio 126 0>, <&gpio 105 0>;
+
+		slave@0 {
+			compatible = "maxim,w1-ds2760";
+		};
 	};
 
diff --git a/Documentation/devicetree/bindings/w1/w1.txt b/Documentation/devicetree/bindings/w1/w1.txt
new file mode 100644
index 000000000000..504adf7c7352
--- /dev/null
+++ b/Documentation/devicetree/bindings/w1/w1.txt
@@ -0,0 +1,17 @@
+Generic devicetree bindings for onewire (w1) busses
+===================================================
+
+Onewire busses are described through nodes of their master bus controller.
+Slave devices are listed as sub-nodes of such master devices.
+
+Example:
+
+	onewire {
+		compatible = "w1-gpio";
+		gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+
+		w1_slave: slave@0 {
+			compatible = "maxim,w1-ds2760";
+		};
+	};
+
-- 
2.17.1

Re: [PATCH RFC 1/4] dt-bindings: w1: document sub-node bindings for DS2760

[Rob Herring]

On Tue, Jun 19, 2018 at 11:27:41PM +0200, Daniel Mack wrote:
> This patch add a generic w1 bindings document that merely describes how
> slave deviceses are grouped under master nodes. It also adds a specific
> binding for the ds2760 battery monitor.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  .../devicetree/bindings/w1/w1-ds2760.txt        |  9 +++++++++
>  .../devicetree/bindings/w1/w1-gpio.txt          |  6 ++++++
>  Documentation/devicetree/bindings/w1/w1.txt     | 17 +++++++++++++++++
>  3 files changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/w1/w1-ds2760.txt
>  create mode 100644 Documentation/devicetree/bindings/w1/w1.txt
> 
> diff --git a/Documentation/devicetree/bindings/w1/w1-ds2760.txt b/Documentation/devicetree/bindings/w1/w1-ds2760.txt
> new file mode 100644
> index 000000000000..86a0f4c573eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/w1/w1-ds2760.txt
> @@ -0,0 +1,9 @@
> +Devicetree bindings for Maxim DS2760
> +====================================
> +
> +The ds2760 is a w1 slave device and must hence have its sub-node in DT
> +under a w1 bus master node.
> +
> +Required properties:
> +- compatible: must be "maxim,ds2760-w1"
> +
> diff --git a/Documentation/devicetree/bindings/w1/w1-gpio.txt b/Documentation/devicetree/bindings/w1/w1-gpio.txt
> index 6e09c35d9f1a..a3e0e964b260 100644
> --- a/Documentation/devicetree/bindings/w1/w1-gpio.txt
> +++ b/Documentation/devicetree/bindings/w1/w1-gpio.txt
> @@ -13,10 +13,16 @@ Optional properties:
>   - linux,open-drain: if specified, the data pin is considered in
>  		     open-drain mode.
>  
> +Also refer to the generic w1.txt document.
> +
>  Examples:
>  
>  	onewire@0 {
>  		compatible = "w1-gpio";
>  		gpios = <&gpio 126 0>, <&gpio 105 0>;
> +
> +		slave@0 {

A unit-address should have a corresponding reg property. Also, the node 
name should reflect the class of device.

You don't really need the unit address here, but for multiple slaves you 
would. I'm not sure what you'd use here since the only id/address is 
the serial number which is unknown except for the family code. If you 
have 2 identical devices with some different DT properties, how would 
you match? I don't see a way other than putting the serial number (or 
some portion of it) in DT.

I'm not sure how common multiple slaves are. Perhaps just support a 
single slave for now.

Rob

[PATCH RFC 2/4] w1: core: match sub-nodes of bus masters in devicetree

[Daniel Mack]

Once a new slave device is detected, match it against all sub-nodes of the
master bus controller. If a match is found, set the slave device's of_node
pointer.

This alone can already be used by slave device implementations to obtain
more properties from devicetree.

Another use-case is introduced in the next patch.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/w1/w1.c    | 3 +++
 include/linux/w1.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 80a778b02f28..dc73d8c08438 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -26,6 +26,7 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 #include <linux/hwmon.h>
+#include <linux/of.h>
 
 #include <linux/atomic.h>
 
@@ -686,6 +687,8 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 	sl->dev.bus = &w1_bus_type;
 	sl->dev.release = &w1_slave_release;
 	sl->dev.groups = w1_slave_groups;
+	sl->dev.of_node = of_find_matching_node(sl->master->dev.of_node,
+						sl->family->of_match_table);
 
 	dev_set_name(&sl->dev, "%02x-%012llx",
 		 (unsigned int) sl->reg_num.family,
diff --git a/include/linux/w1.h b/include/linux/w1.h
index 694101f744c7..3111585c371f 100644
--- a/include/linux/w1.h
+++ b/include/linux/w1.h
@@ -274,6 +274,8 @@ struct w1_family {
 
 	struct w1_family_ops	*fops;
 
+	const struct of_device_id *of_match_table;
+
 	atomic_t		refcnt;
 };
 
-- 
2.17.1

[PATCH RFC 3/4] w1: core: provide helper to look up w1 slaves through devicetree nodes

[Daniel Mack]

This patch adds a helper called w1_of_get_slave() that allows users to refer
to w1 slave devices through phandles in devicetree nodes.

The implementation walks all master devices and all their slaves in order to
find the right slave device.

The API is stubbed out for !CONFIG_OF.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/w1/w1.c    | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/w1.h | 11 +++++++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index dc73d8c08438..693aa9be2cd9 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -1185,6 +1185,43 @@ int w1_process(void *data)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+struct w1_slave *w1_of_get_slave(struct device_node *np,
+				 const char *name, int index)
+{
+	struct device_node *slave_node;
+	struct w1_master *master;
+	struct w1_slave *sl;
+	bool found = false;
+
+	slave_node = of_parse_phandle(np, name, index);
+	if (!slave_node)
+		return NULL;
+
+	mutex_lock(&w1_mlock);
+	list_for_each_entry(master, &w1_masters, w1_master_entry) {
+		mutex_lock(&master->list_mutex);
+		list_for_each_entry(sl, &master->slist, w1_slave_entry) {
+			if (sl->dev.of_node == slave_node) {
+				found = true;
+				break;
+			}
+		}
+		mutex_unlock(&master->list_mutex);
+
+		if (found)
+			break;
+	}
+	mutex_unlock(&w1_mlock);
+
+	if (!found)
+		return NULL;
+
+	return sl;
+}
+EXPORT_SYMBOL_GPL(w1_of_get_slave);
+#endif /* CONFIG_OF */
+
 static int __init w1_init(void)
 {
 	int retval;
diff --git a/include/linux/w1.h b/include/linux/w1.h
index 3111585c371f..c44dffe782f0 100644
--- a/include/linux/w1.h
+++ b/include/linux/w1.h
@@ -322,6 +322,17 @@ static inline struct w1_master* dev_to_w1_master(struct device *dev)
 	return container_of(dev, struct w1_master, dev);
 }
 
+#ifdef CONFIG_OF
+struct w1_slave *w1_of_get_slave(struct device_node *np,
+				 const char *name, int index);
+#else
+static inline struct w1_slave *w1_of_get_slave(struct device_node *np,
+					       const char *name, int index)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 #endif /* __KERNEL__ */
 
 #endif /* __LINUX_W1_H */
-- 
2.17.1

[PATCH RFC 4/4] w1: ds2760: add devicetree matching glue

[Daniel Mack]

Add an id table for ds2760 so it can be matched by the core.

Also, refrain from allocating the platform device for the supply driver at
runtime when the device was matched via DT, because in this case, supply
devices are also probed with device tree nodes, so they will always be
present.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/w1/slaves/w1_ds2760.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/w1/slaves/w1_ds2760.c b/drivers/w1/slaves/w1_ds2760.c
index 26168abfb8b8..a737a96204ed 100644
--- a/drivers/w1/slaves/w1_ds2760.c
+++ b/drivers/w1/slaves/w1_ds2760.c
@@ -17,6 +17,7 @@
 #include <linux/mutex.h>
 #include <linux/idr.h>
 #include <linux/gfp.h>
+#include <linux/of.h>
 
 #include <linux/w1.h>
 
@@ -131,6 +132,9 @@ static int w1_ds2760_add_slave(struct w1_slave *sl)
 	int ret;
 	struct platform_device *pdev;
 
+	if (sl->dev.of_node)
+		return 0;
+
 	pdev = platform_device_alloc("ds2760-battery", PLATFORM_DEVID_AUTO);
 	if (!pdev)
 		return -ENOMEM;
@@ -154,9 +158,17 @@ static void w1_ds2760_remove_slave(struct w1_slave *sl)
 {
 	struct platform_device *pdev = dev_get_drvdata(&sl->dev);
 
-	platform_device_unregister(pdev);
+	if (!sl->dev.of_node)
+		platform_device_unregister(pdev);
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id w1_ds2760_of_ids[] = {
+	{ .compatible = "maxim,w1-ds2760" },
+	{}
+};
+#endif
+
 static struct w1_family_ops w1_ds2760_fops = {
 	.add_slave    = w1_ds2760_add_slave,
 	.remove_slave = w1_ds2760_remove_slave,
@@ -166,6 +178,7 @@ static struct w1_family_ops w1_ds2760_fops = {
 static struct w1_family w1_ds2760_family = {
 	.fid = W1_FAMILY_DS2760,
 	.fops = &w1_ds2760_fops,
+	.of_match_table = of_match_ptr(w1_ds2760_of_ids),
 };
 module_w1_family(w1_ds2760_family);
 
-- 
2.17.1

Re: [PATCH RFC 0/4] Adding DT functionality to w1 busses

[Rob Herring]

On Tue, Jun 19, 2018 at 3:27 PM, Daniel Mack <daniel@zonque.org> wrote:
> In order to fully move battery-supplied devices over to devicetree, the
> onewire subsystem must get some updates.
>
> Currently, the w1 bus system works like this. Device families, such as
> battery controllers etc, are registered to the w1 core. Once a master
> device is probed, it starts scanning the bus. Slave devices that are
> revealed through this scan and that match previously registered
> families are then registered, and their .add_slave() callback is
> invoked.
>
> Some devices, such as the ds2760, use that callback to call
> platform_device_alloc() at runtime with a fixed device name to
> instanciate their only user. That user does the actual work, while the
> slave device merely functions as an I/O proxy. In the user
> implementation, the w1 slave device is accessible through
> dev->parent.

Looks to me like you are letting the driver structure define the DT
structure. This detail is all irrelevant to DT.

>
> Now, for devicetree environments, this has to change a bit. First, slave
> devices need to have a matching table so they can be listed as sub-nodes
> of their master bus controller. Second, the core needs to match the
> entries in these tables against the sub-nodes of the master node.
> These two are trivial to do.
>
> The next question is how the w1 slave device and its user(s) are linked
> together. I'm proposing a DT layout with the following example:
>
>         onewire {
>                 compatible = "w1-gpio";
>
>                 w1_slave: slave@0 {
>                         compatible = "maxim,w1-ds2760";
>                 };
>         };
>
>         battery-supply {
>                 compatible = "maxim,ds2760-supply";
>                 w1-slave = <&w1_slave>;
>         };

This should just be one node and a child of the 1-wire master.

Rob

Re: [PATCH RFC 0/4] Adding DT functionality to w1 busses

[Daniel Mack]

On Tuesday, June 26, 2018 11:39 PM, Rob Herring wrote:
> On Tue, Jun 19, 2018 at 3:27 PM, Daniel Mack <daniel@zonque.org> wrote:
>> In order to fully move battery-supplied devices over to devicetree, the
>> onewire subsystem must get some updates.
>>
>> Currently, the w1 bus system works like this. Device families, such as
>> battery controllers etc, are registered to the w1 core. Once a master
>> device is probed, it starts scanning the bus. Slave devices that are
>> revealed through this scan and that match previously registered
>> families are then registered, and their .add_slave() callback is
>> invoked.
>>
>> Some devices, such as the ds2760, use that callback to call
>> platform_device_alloc() at runtime with a fixed device name to
>> instanciate their only user. That user does the actual work, while the
>> slave device merely functions as an I/O proxy. In the user
>> implementation, the w1 slave device is accessible through
>> dev->parent.
> 
> Looks to me like you are letting the driver structure define the DT
> structure. This detail is all irrelevant to DT.
> 
>>
>> Now, for devicetree environments, this has to change a bit. First, slave
>> devices need to have a matching table so they can be listed as sub-nodes
>> of their master bus controller. Second, the core needs to match the
>> entries in these tables against the sub-nodes of the master node.
>> These two are trivial to do.
>>
>> The next question is how the w1 slave device and its user(s) are linked
>> together. I'm proposing a DT layout with the following example:
>>
>>          onewire {
>>                  compatible = "w1-gpio";
>>
>>                  w1_slave: slave@0 {
>>                          compatible = "maxim,w1-ds2760";
>>                  };
>>          };
>>
>>          battery-supply {
>>                  compatible = "maxim,ds2760-supply";
>>                  w1-slave = <&w1_slave>;
>>          };
> 
> This should just be one node and a child of the 1-wire master.

Yeah, you're right. This also makes the patch set much simpler.

Thanks for the feedback - I'll send another round of patches.



Daniel