[PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Marc Dietrich]

Add device tree bindings information for rfkill gpio switches.

Cc: linux-wireless@vger.kernel.org
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Rhyland Klein <rklein@nvidia.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt

diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
new file mode 100644
index 0000000..22bf22a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
@@ -0,0 +1,38 @@
+RFKILL switches connected to GPIO lines
+
+Required properties:
+- compatible : should be "rfkill-gpio".
+
+Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
+Each node has a label property which represents the name of the corresponding
+rfkill device.
+
+RFKILL sub-node properties:
+- label :  (optional) The label for this rfkill switch.  If omitted, the label is
+  taken from the node name (excluding the unit address).
+- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
+  shutdown (see "Specifying GPIO information for devices" in
+  Documentation/devicetree/booting-without-of.txt).
+- type : enumerated type of the gpio (see include/linux/rfkill.h).
+- clock : (optional) name of the clock name associated with the rfkill switch
+  (see include/linux/rfkill-gpio.h)
+
+Examples:
+
+rfkill-switches {
+	compatible = "rfkill-gpio";
+
+	wifi {
+		label = "wifi";
+		reset-gpio = <&gpio 25 0>; /* Active high */
+		shutdown-gpio = <&gpio 85 0>; /* Active high */
+		type = <1>;
+	};
+
+	bt {
+		label = "bluetooth";
+		reset-gpio = <&gpio 17 0>; /* Active high */
+		shutdown-gpio = <&gpio 35 0>; /* Active high */
+		type = <1>;
+	};
+};
-- 
1.7.5.4

Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Olof Johansson]

Hi,

On Sun, Feb 5, 2012 at 9:18 AM, Marc Dietrich <marvin24@gmx.de> wrote:
> Add device tree bindings information for rfkill gpio switches.
>
> Cc: linux-wireless@vger.kernel.org
> Cc: "John W. Linville" <linville@tuxdriver.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Rhyland Klein <rklein@nvidia.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>

Please cc devicetree-discuss@lists.ozlabs.org on device tree bindings;
not everyone monitors the linux mailing lists.



> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
> new file mode 100644
> index 0000000..22bf22a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
> @@ -0,0 +1,38 @@
> +RFKILL switches connected to GPIO lines
> +
> +Required properties:
> +- compatible : should be "rfkill-gpio".
> +
> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
> +Each node has a label property which represents the name of the corresponding
> +rfkill device.
> +
> +RFKILL sub-node properties:
> +- label :  (optional) The label for this rfkill switch.  If omitted, the label is
> +  taken from the node name (excluding the unit address).
> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
> +  shutdown (see "Specifying GPIO information for devices" in
> +  Documentation/devicetree/booting-without-of.txt).
> +- type : enumerated type of the gpio (see include/linux/rfkill.h).
> +- clock : (optional) name of the clock name associated with the rfkill switch
> +  (see include/linux/rfkill-gpio.h)

Sorry, but this is going about things the wrong way.

A device tree binding is meant to describe hardware. For example, the
type and clock properties are clearly linux specifics that shouldn't
be expressed that way in the device tree.

instead, think about what you actually need to provide for a driver to
do its work. Does the wifi device have a gpio to control power (and
one for providing reset outside of the standard interface)? Well, then
that should probably be defined in the node that describes the device
-- i.e. under the sdhci node in question here.

That doesn't fit the rfkill device/driver model perfectly, since it
doesn't have something to bind against, so the rfkill platform device
needs to be instantiated from somewhere else. It's possible that best
match is to have the wifi driver handle it, which should be OK, I
think?


-Olof

[PATCH 1/3] net: rfkill-gpio: add device tree support

[Marc Dietrich]

This adds device tree support for rfkill-gpio. The optional platform
paramters gpio_runtime_close and gpio_runtime_setup are not implemented.

Cc: linux-wireless@vger.kernel.org
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Rhyland Klein <rklein@nvidia.com>
Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 include/linux/rfkill-gpio.h |    2 +-
 net/rfkill/rfkill-gpio.c    |   57 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/include/linux/rfkill-gpio.h b/include/linux/rfkill-gpio.h
index 4d09f6e..76a9674 100644
--- a/include/linux/rfkill-gpio.h
+++ b/include/linux/rfkill-gpio.h
@@ -35,7 +35,7 @@
  */
 
 struct rfkill_gpio_platform_data {
-	char			*name;
+	const char		*name;
 	int			reset_gpio;
 	int			shutdown_gpio;
 	const char		*power_clk_name;
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 865adb6..4db343d 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -21,6 +21,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/rfkill.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
@@ -77,13 +78,68 @@ static const struct rfkill_ops rfkill_gpio_ops = {
 	.set_block = rfkill_gpio_set_power,
 };
 
+#ifdef CONFIG_OF
+static struct rfkill_gpio_platform_data * __devinit
+			rfkill_gpio_parse_pdata(struct platform_device *pdev)
+{
+	struct rfkill_gpio_platform_data *pdata, *rfkill;
+	struct device_node *np = pdev->dev.of_node, *child;
+	int count = 0;
+
+	for_each_child_of_node(np, child)
+		count++;
+	if (!count)
+		return NULL;
+
+	pdata = devm_kzalloc(&pdev->dev,
+				sizeof(struct rfkill_gpio_platform_data) *
+				count, GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	rfkill = pdata;
+
+	for_each_child_of_node(np, child) {
+		of_property_read_string(child, "label", &rfkill->name);
+		if (!rfkill->name)
+			rfkill->name = child->name;
+		rfkill->reset_gpio = of_get_named_gpio(child, "reset-gpio", 0);
+		rfkill->shutdown_gpio = of_get_named_gpio(child,
+							  "shutdown-gpio", 0);
+		of_property_read_u32(child, "type", &rfkill->type);
+		of_property_read_string(child, "clock",
+					&rfkill->power_clk_name);
+
+		rfkill += sizeof(struct rfkill_gpio_platform_data);
+	}
+
+	return pdata;
+}
+
+static const struct of_device_id of_rfkill_gpio_match[] = {
+	{ .compatible = "rfkill-gpio", },
+	{},
+};
+
+#else
+static inline struct rfkill_gpio_platform_data
+			*rfkill_gpio_parse_pdata(struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int rfkill_gpio_probe(struct platform_device *pdev)
 {
 	struct rfkill_gpio_data *rfkill;
 	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
 	int ret = 0;
 	int len = 0;
 
+	if (np)
+		pdata = rfkill_gpio_parse_pdata(pdev);
+
 	if (!pdata) {
 		pr_warn("%s: No platform data specified\n", __func__);
 		return -EINVAL;
@@ -217,6 +273,7 @@ static struct platform_driver rfkill_gpio_driver = {
 	.driver = {
 		   .name = "rfkill_gpio",
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(of_rfkill_gpio_match),
 	},
 };
 
-- 
1.7.5.4

[PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Marc Dietrich]

Add device tree bindings information for rfkill gpio switches.

Cc: linux-wireless@vger.kernel.org
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Rhyland Klein <rklein@nvidia.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: devicetree-discuss@lists.ozlabs.org
Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt

diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
new file mode 100644
index 0000000..22bf22a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
@@ -0,0 +1,38 @@
+RFKILL switches connected to GPIO lines
+
+Required properties:
+- compatible : should be "rfkill-gpio".
+
+Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
+Each node has a label property which represents the name of the corresponding
+rfkill device.
+
+RFKILL sub-node properties:
+- label :  (optional) The label for this rfkill switch.  If omitted, the label is
+  taken from the node name (excluding the unit address).
+- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
+  shutdown (see "Specifying GPIO information for devices" in
+  Documentation/devicetree/booting-without-of.txt).
+- type : enumerated type of the gpio (see include/linux/rfkill.h).
+- clock : (optional) name of the clock name associated with the rfkill switch
+  (see include/linux/rfkill-gpio.h)
+
+Examples:
+
+rfkill-switches {
+	compatible = "rfkill-gpio";
+
+	wifi {
+		label = "wifi";
+		reset-gpio = <&gpio 25 0>; /* Active high */
+		shutdown-gpio = <&gpio 85 0>; /* Active high */
+		type = <1>;
+	};
+
+	bt {
+		label = "bluetooth";
+		reset-gpio = <&gpio 17 0>; /* Active high */
+		shutdown-gpio = <&gpio 35 0>; /* Active high */
+		type = <1>;
+	};
+};
-- 
1.7.5.4

Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Simon Glass]

Hi Marc,

On Sun, Feb 12, 2012 at 11:13 AM, Marc Dietrich <marvin24@gmx.de> wrote:
> Add device tree bindings information for rfkill gpio switches.
>
> Cc: linux-wireless@vger.kernel.org
> Cc: "John W. Linville" <linville@tuxdriver.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Rhyland Klein <rklein@nvidia.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: devicetree-discuss@lists.ozlabs.org
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
>  1 files changed, 38 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
> new file mode 100644
> index 0000000..22bf22a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
> @@ -0,0 +1,38 @@
> +RFKILL switches connected to GPIO lines
> +
> +Required properties:
> +- compatible : should be "rfkill-gpio".
> +
> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
> +Each node has a label property which represents the name of the corresponding
> +rfkill device.
> +
> +RFKILL sub-node properties:
> +- label :  (optional) The label for this rfkill switch.  If omitted, the label is
> +  taken from the node name (excluding the unit address).
> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
> +  shutdown (see "Specifying GPIO information for devices" in

Should that be reset-gpios, shutdown-gpios? Even though you have only
one it seems that people put an 's' on the end.

> +  Documentation/devicetree/booting-without-of.txt).
> +- type : enumerated type of the gpio (see include/linux/rfkill.h).

It would be better I think if this were explicit here. If you have a
number, then what values does it take and what do they mean?

> +- clock : (optional) name of the clock name associated with the rfkill switch

Can this be a phandle instead of a string?

> +  (see include/linux/rfkill-gpio.h)

IMO device tree bindings should be fully documented in this file,
rather than needing to look at a separate header. This is particularly
true if the binding is used in another project.

> +
> +Examples:
> +
> +rfkill-switches {
> +       compatible = "rfkill-gpio";
> +
> +       wifi {
> +               label = "wifi";
> +               reset-gpio = <&gpio 25 0>; /* Active high */
> +               shutdown-gpio = <&gpio 85 0>; /* Active high */
> +               type = <1>;
> +       };
> +
> +       bt {
> +               label = "bluetooth";
> +               reset-gpio = <&gpio 17 0>; /* Active high */
> +               shutdown-gpio = <&gpio 35 0>; /* Active high */
> +               type = <1>;
> +       };
> +};
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Simon

Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Rob Herring]

On 02/12/2012 02:21 PM, Simon Glass wrote:
> Hi Marc,
> 
> On Sun, Feb 12, 2012 at 11:13 AM, Marc Dietrich <marvin24@gmx.de> wrote:
>> Add device tree bindings information for rfkill gpio switches.
>>
>> Cc: linux-wireless@vger.kernel.org
>> Cc: "John W. Linville" <linville@tuxdriver.com>
>> Cc: Johannes Berg <johannes@sipsolutions.net>
>> Cc: Rhyland Klein <rklein@nvidia.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
>> ---
>>  Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
>>  1 files changed, 38 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
>> new file mode 100644
>> index 0000000..22bf22a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
>> @@ -0,0 +1,38 @@
>> +RFKILL switches connected to GPIO lines
>> +
>> +Required properties:
>> +- compatible : should be "rfkill-gpio".
>> +
>> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
>> +Each node has a label property which represents the name of the corresponding
>> +rfkill device.
>> +
>> +RFKILL sub-node properties:
>> +- label :  (optional) The label for this rfkill switch.  If omitted, the label is
>> +  taken from the node name (excluding the unit address).
>> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
>> +  shutdown (see "Specifying GPIO information for devices" in
> 
> Should that be reset-gpios, shutdown-gpios? Even though you have only
> one it seems that people put an 's' on the end.
> 
>> +  Documentation/devicetree/booting-without-of.txt).
>> +- type : enumerated type of the gpio (see include/linux/rfkill.h).
> 
> It would be better I think if this were explicit here. If you have a
> number, then what values does it take and what do they mean?
> 
>> +- clock : (optional) name of the clock name associated with the rfkill switch
> 
> Can this be a phandle instead of a string?
> 

This seems to be in the wrong place altogether. The gpio controller
would have a clock, not particular gpio line.

>> +  (see include/linux/rfkill-gpio.h)
> 
> IMO device tree bindings should be fully documented in this file,
> rather than needing to look at a separate header. This is particularly
> true if the binding is used in another project.
> 

Correct. A binding should not be Linux specific. It should describe the h/w.

>> +
>> +Examples:
>> +
>> +rfkill-switches {
>> +       compatible = "rfkill-gpio";
>> +
>> +       wifi {
>> +               label = "wifi";
>> +               reset-gpio = <&gpio 25 0>; /* Active high */
>> +               shutdown-gpio = <&gpio 85 0>; /* Active high */
>> +               type = <1>;
>> +       };
>> +
>> +       bt {
>> +               label = "bluetooth";
>> +               reset-gpio = <&gpio 17 0>; /* Active high */
>> +               shutdown-gpio = <&gpio 35 0>; /* Active high */
>> +               type = <1>;
>> +       };

Why wouldn't the gpio lines just be part of the bt and wifi device nodes
themselves? The DT is supposed to describe h/w connections.

Rob

>> +};
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Regards,
> Simon
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/3] net: rfkill-gpio: add device tree support

[Rhyland Klein]

On Sun, 2012-02-12 at 11:13 -0800, Marc Dietrich wrote:
> This adds device tree support for rfkill-gpio. The optional platform
> paramters gpio_runtime_close and gpio_runtime_setup are not implemented.
> 
> Cc: linux-wireless@vger.kernel.org
> Cc: "John W. Linville" <linville@tuxdriver.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Rhyland Klein <rklein@nvidia.com>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> +
>  static int rfkill_gpio_probe(struct platform_device *pdev)
>  {
>  	struct rfkill_gpio_data *rfkill;
>  	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret = 0;
>  	int len = 0;
>  
> +	if (np)
> +		pdata = rfkill_gpio_parse_pdata(pdev);
> +

The only concern I have is the precedence of devicetree settings vs
platform data settings? If there is pdata passed in from board file
initialization, and there is a device tree (a corner case but I think a
valid one) then I believe the order would be that defined pdata would
override the devicetree settings. That way if someone wanted to make a
quick update, they wouldn't need to change the boot loader as well.

>  	if (!pdata) {
>  		pr_warn("%s: No platform data specified\n", __func__);
>  		return -EINVAL;
> @@ -217,6 +273,7 @@ static struct platform_driver rfkill_gpio_driver = {
>  	.driver = {
>  		   .name = "rfkill_gpio",
>  		   .owner = THIS_MODULE,
> +		   .of_match_table = of_match_ptr(of_rfkill_gpio_match),
>  	},
>  };
>  

-rhyland

Re: [PATCH 1/3] net: rfkill-gpio: add device tree support

[Olof Johansson]

On Mon, Feb 13, 2012 at 11:25 AM, Rhyland Klein <rklein@nvidia.com> wrote:
> On Sun, 2012-02-12 at 11:13 -0800, Marc Dietrich wrote:
>> This adds device tree support for rfkill-gpio. The optional platform
>> paramters gpio_runtime_close and gpio_runtime_setup are not implemented.
>>
>> Cc: linux-wireless@vger.kernel.org
>> Cc: "John W. Linville" <linville@tuxdriver.com>
>> Cc: Johannes Berg <johannes@sipsolutions.net>
>> Cc: Rhyland Klein <rklein@nvidia.com>
>> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
>> +
>>  static int rfkill_gpio_probe(struct platform_device *pdev)
>>  {
>>       struct rfkill_gpio_data *rfkill;
>>       struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
>> +     struct device_node *np = pdev->dev.of_node;
>>       int ret = 0;
>>       int len = 0;
>>
>> +     if (np)
>> +             pdata = rfkill_gpio_parse_pdata(pdev);
>> +
>
> The only concern I have is the precedence of devicetree settings vs
> platform data settings? If there is pdata passed in from board file
> initialization, and there is a device tree (a corner case but I think a
> valid one) then I believe the order would be that defined pdata would
> override the devicetree settings. That way if someone wanted to make a
> quick update, they wouldn't need to change the boot loader as well.

Yes, that is how other drivers tend to be coded -- only of pdata is
null will the driver try to fill in from the DT.


-Olof

Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Olof Johansson]

Hi,

On Mon, Feb 13, 2012 at 5:47 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 02/12/2012 02:21 PM, Simon Glass wrote:
>> Hi Marc,
>>
>> On Sun, Feb 12, 2012 at 11:13 AM, Marc Dietrich <marvin24@gmx.de> wrote:
>>> Add device tree bindings information for rfkill gpio switches.
>>>
>>> Cc: linux-wireless@vger.kernel.org
>>> Cc: "John W. Linville" <linville@tuxdriver.com>
>>> Cc: Johannes Berg <johannes@sipsolutions.net>
>>> Cc: Rhyland Klein <rklein@nvidia.com>
>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>> Cc: devicetree-discuss@lists.ozlabs.org
>>> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
>>> ---
>>>  Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
>>>  1 files changed, 38 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
>>> new file mode 100644
>>> index 0000000..22bf22a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
>>> @@ -0,0 +1,38 @@
>>> +RFKILL switches connected to GPIO lines
>>> +
>>> +Required properties:
>>> +- compatible : should be "rfkill-gpio".
>>> +
>>> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
>>> +Each node has a label property which represents the name of the corresponding
>>> +rfkill device.
>>> +
>>> +RFKILL sub-node properties:
>>> +- label :  (optional) The label for this rfkill switch.  If omitted, the label is
>>> +  taken from the node name (excluding the unit address).
>>> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
>>> +  shutdown (see "Specifying GPIO information for devices" in
>>
>> Should that be reset-gpios, shutdown-gpios? Even though you have only
>> one it seems that people put an 's' on the end.

Agreed.

>>
>>> +  Documentation/devicetree/booting-without-of.txt).
>>> +- type : enumerated type of the gpio (see include/linux/rfkill.h).
>>
>> It would be better I think if this were explicit here. If you have a
>> number, then what values does it take and what do they mean?

This should most likely be moved to a set of properties instad of an
enumerated type, I agree. And/or use a string to encode the type
simiar to how powerpc does some of the USB interfaces.

>>> +- clock : (optional) name of the clock name associated with the rfkill switch
>>
>> Can this be a phandle instead of a string?
>
> This seems to be in the wrong place altogether. The gpio controller
> would have a clock, not particular gpio line.

And either way, this should conform to the standard clock binding, not
use something locally hacked up.

>>> +  (see include/linux/rfkill-gpio.h)
>>
>> IMO device tree bindings should be fully documented in this file,
>> rather than needing to look at a separate header. This is particularly
>> true if the binding is used in another project.
>>
>
> Correct. A binding should not be Linux specific. It should describe the h/w.
>
>>> +
>>> +Examples:
>>> +
>>> +rfkill-switches {
>>> +       compatible = "rfkill-gpio";
>>> +
>>> +       wifi {
>>> +               label = "wifi";
>>> +               reset-gpio = <&gpio 25 0>; /* Active high */
>>> +               shutdown-gpio = <&gpio 85 0>; /* Active high */
>>> +               type = <1>;
>>> +       };
>>> +
>>> +       bt {
>>> +               label = "bluetooth";
>>> +               reset-gpio = <&gpio 17 0>; /* Active high */
>>> +               shutdown-gpio = <&gpio 35 0>; /* Active high */
>>> +               type = <1>;
>>> +       };
>
> Why wouldn't the gpio lines just be part of the bt and wifi device nodes
> themselves? The DT is supposed to describe h/w connections.

The thing is, that "rfkill" isn't a _device_, and Marc is trying to
describe it as one. It's really just a software abstraction of a
collection of power supplies and/or GPIO lines that are used to power
up/down specific peripherals.

I know that the USB modem, for example, is probed through autoprobing
the USB bus. So there's no device to associate it with, per se. But
the USB slot that the modem is connected to, which is also the
connector that the GPIO controls the power supplies and reset line to,
are connected directly to one of the USB host controllers, right? So
maybe describing it there is a better option.

That still leaves the issue of actually having something to bind it
against. As I already said, rfkill isn't a device, so crafting one
just because linux wants one is the wrong way to go about. Maybe using
/chosen to refer to the device nodes for the GPIO lines under USB
instead, and have rfkill look for those and create a device if they're
found is a better way to go about it.


-Olof

Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Marc Dietrich]

Am 13.02.2012 20:43, schrieb Olof Johansson:
> On Mon, Feb 13, 2012 at 5:47 AM, Rob Herring<robherring2@gmail.com>  wrote:
>> On 02/12/2012 02:21 PM, Simon Glass wrote:
>>
>>
>> On Sun, Feb 12, 2012 at 11:13 AM, Marc Dietrich<marvin24@gmx.de>  wrote:
>>>> Add device tree bindings information for rfkill gpio switches.
>>>>
>>>> Cc: linux-wireless@vger.kernel.org
>>>> Cc: "John W. Linville"<linville@tuxdriver.com>
>>>> Cc: Johannes Berg<johannes@sipsolutions.net>
>>>> Cc: Rhyland Klein<rklein@nvidia.com>
>>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>>> Cc: devicetree-discuss@lists.ozlabs.org
>>>> Signed-off-by: Marc Dietrich<marvin24@gmx.de>
>>>> ---
>>>>   Documentation/devicetree/bindings/gpio/rfkill.txt |   38 +++++++++++++++++++++
>>>>   1 files changed, 38 insertions(+), 0 deletions(-)
>>>>   create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt b/Documentation/devicetree/bindings/gpio/rfkill.txt
>>>> new file mode 100644
>>>> index 0000000..22bf22a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
>>>> @@ -0,0 +1,38 @@
>>>> +RFKILL switches connected to GPIO lines
>>>> +
>>>> +Required properties:
>>>> +- compatible : should be "rfkill-gpio".
>>>> +
>>>> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
>>>> +Each node has a label property which represents the name of the corresponding
>>>> +rfkill device.
>>>> +
>>>> +RFKILL sub-node properties:
>>>> +- label :  (optional) The label for this rfkill switch.  If omitted, the label is
>>>> +  taken from the node name (excluding the unit address).
>>>> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset and
>>>> +  shutdown (see "Specifying GPIO information for devices" in
>>> Should that be reset-gpios, shutdown-gpios? Even though you have only
>>> one it seems that people put an 's' on the end.
> Agreed.
>
>>>> +  Documentation/devicetree/booting-without-of.txt).
>>>> +- type : enumerated type of the gpio (see include/linux/rfkill.h).
>>> It would be better I think if this were explicit here. If you have a
>>> number, then what values does it take and what do they mean?
> This should most likely be moved to a set of properties instad of an
> enumerated type, I agree. And/or use a string to encode the type
> simiar to how powerpc does some of the USB interfaces.
>
>>>> +- clock : (optional) name of the clock name associated with the rfkill switch
>>> Can this be a phandle instead of a string?
>> This seems to be in the wrong place altogether. The gpio controller
>> would have a clock, not particular gpio line.
> And either way, this should conform to the standard clock binding, not
> use something locally hacked up.
>
>>>> +  (see include/linux/rfkill-gpio.h)
>>> IMO device tree bindings should be fully documented in this file,
>>> rather than needing to look at a separate header. This is particularly
>>> true if the binding is used in another project.
>>>
>> Correct. A binding should not be Linux specific. It should describe the h/w.
>>
>>>> +
>>>> +Examples:
>>>> +
>>>> +rfkill-switches {
>>>> +       compatible = "rfkill-gpio";
>>>> +
>>>> +       wifi {
>>>> +               label = "wifi";
>>>> +               reset-gpio =<&gpio 25 0>; /* Active high */
>>>> +               shutdown-gpio =<&gpio 85 0>; /* Active high */
>>>> +               type =<1>;
>>>> +       };
>>>> +
>>>> +       bt {
>>>> +               label = "bluetooth";
>>>> +               reset-gpio =<&gpio 17 0>; /* Active high */
>>>> +               shutdown-gpio =<&gpio 35 0>; /* Active high */
>>>> +               type =<1>;
>>>> +       };
>> Why wouldn't the gpio lines just be part of the bt and wifi device nodes
>> themselves? The DT is supposed to describe h/w connections.
> The thing is, that "rfkill" isn't a _device_, and Marc is trying to
> describe it as one. It's really just a software abstraction of a
> collection of power supplies and/or GPIO lines that are used to power
> up/down specific peripherals.

Oh, this gets more complicated than I wanted. First I only translated 
the *existing* platform_data to devicetree format. I understand that 
devicetree needs some more careful thoughts. Let me shortly describe my 
hw setup and lets see how it can me mapped.

> I know that the USB modem, for example, is probed through autoprobing
> the USB bus. So there's no device to associate it with, per se. But
> the USB slot that the modem is connected to, which is also the
> connector that the GPIO controls the power supplies and reset line to,
> are connected directly to one of the USB host controllers, right? So
> maybe describing it there is a better option.

The wifi module (some models also include an additional bluetooth part) 
is plugged into a mini-pci-e like slot. This has pins for usb (the 
module uses usb), dap, sdio, uart, and some other pins which also 
include the gpio lines for rfkill. The other gpio end is directly 
connected to the tegra soc.

So there is no usb slot and a standard usb slot also does not contain 
any gpio lines (just 2 power and 2 data lines). The usb controller also 
sits in the tegra soc (well, more or less because it is the ulpi port 
connected to SMC3315 and later a SMC2512 4-port usb hub). So gpio and 
usb take a totally different path. IMHO, it makes no sense to add a gpio 
to the usb controller, but see below.

> That still leaves the issue of actually having something to bind it
> against. As I already said, rfkill isn't a device, so crafting one
> just because linux wants one is the wrong way to go about. Maybe using
> /chosen to refer to the device nodes for the GPIO lines under USB
> instead, and have rfkill look for those and create a device if they're
> found is a better way to go about it.
To me it looks more like how the LEDs are implemented but with a 
"linux,rfkill_type" property. To me this looks saner than adding a gpio 
to an usb controller, even if there if no physical rfkill device. This 
still leaves the problem of what to do with the clock (it is not used by 
any machine AFAIK).

The main problem here is that usb devices have no device tree entries 
(only the host controllers). Perhaps it is possible to a fake usb device 
as a workaround? This has the advantage that it exists physically, but 
the resources are not taken from the device tree. Instead all other non 
standard properties (gpios, clocks) are.

Marc

Re: [PATCH 1/3] net: rfkill-gpio: add device tree support

[Marc Dietrich]

Am 13.02.2012 20:36, schrieb Olof Johansson:
> On Mon, Feb 13, 2012 at 11:25 AM, Rhyland Klein<rklein@nvidia.com>  wrote:
>> On Sun, 2012-02-12 at 11:13 -0800, Marc Dietrich wrote:
>>> This adds device tree support for rfkill-gpio. The optional platform
>>> paramters gpio_runtime_close and gpio_runtime_setup are not implemented.
>>>
>>> Cc: linux-wireless@vger.kernel.org
>>> Cc: "John W. Linville"<linville@tuxdriver.com>
>>> Cc: Johannes Berg<johannes@sipsolutions.net>
>>> Cc: Rhyland Klein<rklein@nvidia.com>
>>> Signed-off-by: Marc Dietrich<marvin24@gmx.de>
>>> +
>>>   static int rfkill_gpio_probe(struct platform_device *pdev)
>>>   {
>>>        struct rfkill_gpio_data *rfkill;
>>>        struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
>>> +     struct device_node *np = pdev->dev.of_node;
>>>        int ret = 0;
>>>        int len = 0;
>>>
>>> +     if (np)
>>> +             pdata = rfkill_gpio_parse_pdata(pdev);
>>> +
>> The only concern I have is the precedence of devicetree settings vs
>> platform data settings? If there is pdata passed in from board file
>> initialization, and there is a device tree (a corner case but I think a
>> valid one) then I believe the order would be that defined pdata would
>> override the devicetree settings. That way if someone wanted to make a
>> quick update, they wouldn't need to change the boot loader as well.
> Yes, that is how other drivers tend to be coded -- only of pdata is
> null will the driver try to fill in from the DT.

If the device tree from the bootloader does not contain the rfkill 
information the resources from platform_data will be used (so no need to 
update the bootloader). If the bootloader contains rfkill information, 
why shouldn't it be used?

Marc

Re: Re: [PATCH 2/3] dt: rfkill-gpio: add bindings documentation

[Marc Dietrich]

Am Montag, 13. Februar 2012, 11:43:17 schrieb Olof Johansson:
> Hi,
> 
> On Mon, Feb 13, 2012 at 5:47 AM, Rob Herring <robherring2@gmail.com> wrote:
> > On 02/12/2012 02:21 PM, Simon Glass wrote:
> >> Hi Marc,
> >> 
> >> On Sun, Feb 12, 2012 at 11:13 AM, Marc Dietrich <marvin24@gmx.de> wrote:
> >>> Add device tree bindings information for rfkill gpio switches.
> >>> 
> >>> Cc: linux-wireless@vger.kernel.org
> >>> Cc: "John W. Linville" <linville@tuxdriver.com>
> >>> Cc: Johannes Berg <johannes@sipsolutions.net>
> >>> Cc: Rhyland Klein <rklein@nvidia.com>
> >>> Cc: Grant Likely <grant.likely@secretlab.ca>
> >>> Cc: devicetree-discuss@lists.ozlabs.org
> >>> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/gpio/rfkill.txt |   38
> >>> +++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/rfkill.txt
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/gpio/rfkill.txt
> >>> b/Documentation/devicetree/bindings/gpio/rfkill.txt new file mode 100644
> >>> index 0000000..22bf22a
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/rfkill.txt
> >>> @@ -0,0 +1,38 @@
> >>> +RFKILL switches connected to GPIO lines
> >>> +
> >>> +Required properties:
> >>> +- compatible : should be "rfkill-gpio".
> >>> +
> >>> +Each rfkill switch is represented as a sub-node of the rfkill-gpio device.
> >>> +Each node has a label property which represents the name of the
> >>> corresponding
> >>> +rfkill device.
> >>> +
> >>> +RFKILL sub-node properties:
> >>> +- label :  (optional) The label for this rfkill switch.  If omitted, the
> >>> label is +  taken from the node name (excluding the unit address).
> >>> +- reset-gpio, shutdown-gpio :  Should specify the rfkill gpios for reset
> >>> and
> >>> +  shutdown (see "Specifying GPIO information for devices" in
> >> 
> >> Should that be reset-gpios, shutdown-gpios? Even though you have only
> >> one it seems that people put an 's' on the end.
> 
> Agreed.
> 
> >>> +  Documentation/devicetree/booting-without-of.txt).
> >>> +- type : enumerated type of the gpio (see include/linux/rfkill.h).
> >> 
> >> It would be better I think if this were explicit here. If you have a
> >> number, then what values does it take and what do they mean?
> 
> This should most likely be moved to a set of properties instad of an
> enumerated type, I agree. And/or use a string to encode the type
> simiar to how powerpc does some of the USB interfaces.
> 
> >>> +- clock : (optional) name of the clock name associated with the rfkill
> >>> switch
> >> 
> >> Can this be a phandle instead of a string?
> > 
> > This seems to be in the wrong place altogether. The gpio controller
> > would have a clock, not particular gpio line.
> 
> And either way, this should conform to the standard clock binding, not
> use something locally hacked up.
> 
> >>> +  (see include/linux/rfkill-gpio.h)
> >> 
> >> IMO device tree bindings should be fully documented in this file,
> >> rather than needing to look at a separate header. This is particularly
> >> true if the binding is used in another project.
> > 
> > Correct. A binding should not be Linux specific. It should describe the h/w.
> > 
> >>> +
> >>> +Examples:
> >>> +
> >>> +rfkill-switches {
> >>> +       compatible = "rfkill-gpio";
> >>> +
> >>> +       wifi {
> >>> +               label = "wifi";
> >>> +               reset-gpio = <&gpio 25 0>; /* Active high */
> >>> +               shutdown-gpio = <&gpio 85 0>; /* Active high */
> >>> +               type = <1>;
> >>> +       };
> >>> +
> >>> +       bt {
> >>> +               label = "bluetooth";
> >>> +               reset-gpio = <&gpio 17 0>; /* Active high */
> >>> +               shutdown-gpio = <&gpio 35 0>; /* Active high */
> >>> +               type = <1>;
> >>> +       };
> > 
> > Why wouldn't the gpio lines just be part of the bt and wifi device nodes
> > themselves? The DT is supposed to describe h/w connections.
> 
> The thing is, that "rfkill" isn't a _device_, and Marc is trying to
> describe it as one. It's really just a software abstraction of a
> collection of power supplies and/or GPIO lines that are used to power
> up/down specific peripherals.
> 
> I know that the USB modem, for example, is probed through autoprobing
> the USB bus. So there's no device to associate it with, per se. But
> the USB slot that the modem is connected to, which is also the
> connector that the GPIO controls the power supplies and reset line to,
> are connected directly to one of the USB host controllers, right? So
> maybe describing it there is a better option.
> 
> That still leaves the issue of actually having something to bind it
> against. As I already said, rfkill isn't a device, so crafting one
> just because linux wants one is the wrong way to go about. Maybe using
> /chosen to refer to the device nodes for the GPIO lines under USB
> instead, and have rfkill look for those and create a device if they're
> found is a better way to go about it.

So to move forward, what about a "fake" device like this?

usb@c5000000 {
		wifi-card@1 { /* 1nd port on usb bus 1 */
				compatible = "rfkill-gpio";

				wifi {
						label = "internal wifi";
						reset-gpios = <&gpio 25 0>; /* Active high */
						shutdown-gpios = <&gpio 85 0>; /* Active high */
						type = "wlan";
						clocks = <&tegra-car 17>;
				};
		};

		bt-card@2 { /* 2rd port on usb bus 1 */
				compatible = "rfkill-gpio";

				bt {
						label = "internal bluetooth";
						reset-gpios = <&gpio 17 0>; /* Active high */
						shutdown-gpios = <&gpio 35 0>; /* Active high */
						type = "bluetooth";
				};
		};
};

I hope this won't confuse the usb controller.

Marc