[PATCH 0/7] net: rfkill: gpio: Add device tree support

[PATCH 0/7] net: rfkill: gpio: Add device tree support

[Chen-Yu Tsai]

Hi everyone,

This patch series adds device tree support to rfkill-gpio, and
fixes some issues I ran into. This is so we can define and control
RF devices through the device tree, such as the Broadcom BCM20710
UART-based Bluetooth device found on the CubieTruck.

The series is based on Heikki's rfkill-gpio cleanup patches [1],
applied on 3.15-rc1. Kudos to Heikki for paving the way.

 [1] https://lkml.org/lkml/2014/4/1/451

The device tree bindings are much better than was defined in the
RFC series I sent a few months earlier [2], due to cleanups by
Heikki, and named gpios (via gpio-names) implemented in the first
two patches. Hopefully this will satisfy everyone.

 [2] https://lkml.org/lkml/2014/1/17/31

The CubieTruck uses a non-default clock rate oscillator for the
BCM20710 device. As the datasheet states, a precise 32.768 KHz
low power clock must be provided at power on for the device to
detect the correct clock rate of the main oscillator. Hence the
need for the "clock-frequency" property.

The first 2 patches should go through the gpio tree. The 4 rfkill-gpio
patches should go through the same tree that Heikki's patches are
in. Maxime, can you take the last one?

A big thanks to everyone who gave reviews and suggestions.


Changes since RFC:

  - Dropped gpio name buffer fix patch (not needed after cleanup patches)
  - New gpios/gpio-names support for device trees
  - Simplify device tree bindings due to name cleanup and gpio-names
    support


Cheers

ChenYu

Chen-Yu Tsai (7):
  gpiolib: gpiolib-of: Implement device tree gpio-names based lookup
  gpiolib: Support purely name based gpiod lookup in device trees
  net: rfkill: gpio: use clk_prepare_enable/clk_disable_unprepare
  net: rfkill: gpio: fix reversed clock enable state
  net: rfkill: gpio: add device tree support
  net: rfkill: gpio: add clock-frequency device tree property
  ARM: sun7i: cubietruck: enable bluetooth module

 .../devicetree/bindings/rfkill/rfkill-gpio.txt     | 26 ++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts         | 25 +++++++++++
 drivers/gpio/gpiolib-of.c                          | 48 ++++++++++++++++++++++
 drivers/gpio/gpiolib.c                             |  9 +++-
 include/linux/of_gpio.h                            |  3 ++
 net/rfkill/rfkill-gpio.c                           | 34 +++++++++++++--
 6 files changed, 140 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt

-- 
1.9.1

[PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Chen-Yu Tsai]

This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
phandles by name only, through gpios/gpio-names, and not by index.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpio/gpiolib-of.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_gpio.h   |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2024d45..5c586fa 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -97,6 +97,54 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 EXPORT_SYMBOL(of_get_named_gpiod_flags);
 
 /**
+ * of_get_gpiod_flags_by_name() - Get a GPIO descriptor and flags by name
+ * @np:		device node to get GPIO from
+ * @name:	matching name of gpio phandle
+ * @flags:	a flags pointer to fill in
+ *
+ * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
+ * value on the error condition. If @flags is not NULL the function also fills
+ * in flags for the GPIO.
+ */
+struct gpio_desc *of_get_gpiod_flags_by_name(struct device_node *np,
+		const char *name, enum of_gpio_flags *flags)
+{
+	/* Return -EPROBE_DEFER to support probe() functions to be called
+	 * later when the GPIO actually becomes available
+	 */
+	struct gg_data gg_data = {
+		.flags = flags,
+		.out_gpio = ERR_PTR(-EPROBE_DEFER)
+	};
+	int index = 0;
+	int ret;
+
+	/* exit if no name given */
+	if (!name)
+		return ERR_PTR(-EINVAL);
+
+	/* .of_xlate might decide to not fill in the flags, so clear it. */
+	if (flags)
+		*flags = 0;
+
+	if (name)
+		index = of_property_match_string(np, "gpio-names", name);
+
+	ret = of_parse_phandle_with_args(np, "gpios", "#gpio-cells", index,
+					 &gg_data.gpiospec);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
+
+	of_node_put(gg_data.gpiospec.np);
+
+	return gg_data.out_gpio;
+}
+EXPORT_SYMBOL(of_get_gpiod_flags_by_names);
+
+/**
  * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags
  * @gc:		pointer to the gpio_chip structure
  * @np:		device node of the GPIO chip
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index f14123a..134331f 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -51,6 +51,9 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
 extern struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		const char *list_name, int index, enum of_gpio_flags *flags);
 
+extern struct gpio_desc *of_get_gpiod_flags_by_name(struct device_node *np,
+		const char *name, enum of_gpio_flags *flags);
+
 extern int of_mm_gpiochip_add(struct device_node *np,
 			      struct of_mm_gpio_chip *mm_gc);
 
-- 
1.9.1

[PATCH 2/7] gpiolib: Support purely name based gpiod lookup in device trees

[Chen-Yu Tsai]

This patch enables gpio-names based gpiod lookup in device tree usage,
which ignores the index passed to gpiod_get_index. If this fails, fall
back to the original function-index ("con_id"-gpios) based lookup scheme,
for backward compatibility and any drivers needing more than one GPIO
for any function.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpio/gpiolib.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 761013f..956f01e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2585,8 +2585,13 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	else
 		snprintf(prop_name, 32, "gpios");
 
-	desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
-					&of_flags);
+	/* try gpio-names based lookup first */
+	desc = of_get_gpiod_flags_by_name(dev->of_node, con_id, &of_flags);
+
+	/* fallback to function based lookup */
+	if (IS_ERR(desc))
+		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
+				&of_flags);
 
 	if (IS_ERR(desc))
 		return desc;
-- 
1.9.1

[PATCH 3/7] net: rfkill: gpio: use clk_prepare_enable/clk_disable_unprepare

[Chen-Yu Tsai]

rfkill-gpio calls clk_enable() without first calling clk_prepare(),
resulting in a warning and no effect. Switch to clk_prepare_enable()
and clk_disable_unprepare.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 net/rfkill/rfkill-gpio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 9c4a5eb..29ff07c 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -49,10 +49,10 @@ static int rfkill_gpio_set_power(void *data, bool blocked)
 		gpiod_set_value(rfkill->shutdown_gpio, 0);
 		gpiod_set_value(rfkill->reset_gpio, 0);
 		if (!IS_ERR(rfkill->clk) && rfkill->clk_enabled)
-			clk_disable(rfkill->clk);
+			clk_disable_unprepare(rfkill->clk);
 	} else {
 		if (!IS_ERR(rfkill->clk) && !rfkill->clk_enabled)
-			clk_enable(rfkill->clk);
+			clk_prepare_enable(rfkill->clk);
 		gpiod_set_value(rfkill->reset_gpio, 1);
 		gpiod_set_value(rfkill->shutdown_gpio, 1);
 	}
-- 
1.9.1

[PATCH 4/7] net: rfkill: gpio: fix reversed clock enable state

[Chen-Yu Tsai]

rfkill-gpio has clk_enabled = blocked, which is true when rfkill
blocks the device. This results in calling clock enable/disable at
the wrong time. Reversing the value fixes this.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 net/rfkill/rfkill-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index 29ff07c..f46ddf7 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -57,7 +57,7 @@ static int rfkill_gpio_set_power(void *data, bool blocked)
 		gpiod_set_value(rfkill->shutdown_gpio, 1);
 	}
 
-	rfkill->clk_enabled = blocked;
+	rfkill->clk_enabled = !blocked;
 
 	return 0;
 }
-- 
1.9.1

[PATCH 5/7] net: rfkill: gpio: add device tree support

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../devicetree/bindings/rfkill/rfkill-gpio.txt     | 24 ++++++++++++++++++++++
 net/rfkill/rfkill-gpio.c                           | 23 +++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt

diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
new file mode 100644
index 0000000..a23da65
--- /dev/null
+++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
@@ -0,0 +1,24 @@
+GPIO controlled RFKILL devices
+
+Required properties:
+- compatible	: Must be "rfkill-gpio".
+- rfkill-name	: Name of RFKILL device
+- rfkill-type	: Type of RFKILL device: 1 for WiFi, 2 for BlueTooth, etc.
+		  See include/uapi/linux/rfkill.h for all valid values
+- gpios		: At most two GPIO phandles
+- gpio-names	: Shall be "reset" or "shutdown", matching gpios.
+		  If both are provided, the "reset" GPIO is toggled first.
+
+Optional properties:
+- clocks		: phandle to clock to enable/disable
+
+Example:
+
+	rfkill_bt {
+		compatible = "rfkill-gpio";
+		rfkill-name = "bluetooth";
+		rfkill-type = <2>;
+		gpios = <&pio 7 18 0>;
+		gpio-names = "reset";
+		clocks = <&clk_out_a>;
+	};
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index f46ddf7..a174359 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
 
 #include <linux/rfkill-gpio.h>
 
@@ -81,6 +82,18 @@ static int rfkill_gpio_acpi_probe(struct device *dev,
 	return 0;
 }
 
+static int rfkill_gpio_dt_probe(struct device *dev,
+				struct rfkill_gpio_data *rfkill)
+{
+	struct device_node * np = dev->of_node;
+
+	rfkill->name = np->name;
+	of_property_read_string(np, "rfkill-name", &rfkill->name);
+	of_property_read_u32(np, "rfkill-type", &rfkill->type);
+
+	return 0;
+}
+
 static int rfkill_gpio_probe(struct platform_device *pdev)
 {
 	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
@@ -96,6 +109,10 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 		ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill);
 		if (ret)
 			return ret;
+	} else if (&pdev->dev.of_node) {
+		ret = rfkill_gpio_dt_probe(&pdev->dev, rfkill);
+		if (ret)
+			return ret;
 	} else if (pdata) {
 		rfkill->name = pdata->name;
 		rfkill->type = pdata->type;
@@ -167,6 +184,11 @@ static const struct acpi_device_id rfkill_acpi_match[] = {
 };
 #endif
 
+static const struct of_device_id rfkill_of_match[] = {
+	{ .compatible = "rfkill-gpio", },
+	{},
+};
+
 static struct platform_driver rfkill_gpio_driver = {
 	.probe = rfkill_gpio_probe,
 	.remove = rfkill_gpio_remove,
@@ -174,6 +196,7 @@ static struct platform_driver rfkill_gpio_driver = {
 		.name = "rfkill_gpio",
 		.owner = THIS_MODULE,
 		.acpi_match_table = ACPI_PTR(rfkill_acpi_match),
+		.of_match_table = of_match_ptr(rfkill_of_match),
 	},
 };
 
-- 
1.9.1

[PATCH 6/7] net: rfkill: gpio: add clock-frequency device tree property

[Chen-Yu Tsai]

Some devices, such as Broadcom Bluetooth devices, require a specific
clock rate for the clock tied to the rfkill device. Add a clock-frequency
property so we can specify this from the device tree.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt | 2 ++
 net/rfkill/rfkill-gpio.c                                 | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
index a23da65..67b5edb 100644
--- a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
+++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
@@ -11,6 +11,7 @@ Required properties:
 
 Optional properties:
 - clocks		: phandle to clock to enable/disable
+- clock-frequency	: desired clock rate for the given clock
 
 Example:
 
@@ -21,4 +22,5 @@ Example:
 		gpios = <&pio 7 18 0>;
 		gpio-names = "reset";
 		clocks = <&clk_out_a>;
+		clock-frequency = <32678>;
 	};
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index a174359..14ac8c1 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -38,6 +38,7 @@ struct rfkill_gpio_data {
 
 	struct rfkill		*rfkill_dev;
 	struct clk		*clk;
+	uint32_t		clk_frequency;
 
 	bool			clk_enabled;
 };
@@ -90,6 +91,7 @@ static int rfkill_gpio_dt_probe(struct device *dev,
 	rfkill->name = np->name;
 	of_property_read_string(np, "rfkill-name", &rfkill->name);
 	of_property_read_u32(np, "rfkill-type", &rfkill->type);
+	of_property_read_u32(np, "clock-frequency", &rfkill->clk_frequency);
 
 	return 0;
 }
@@ -122,6 +124,9 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);
 
+	if (!IS_ERR(rfkill->clk) && rfkill->clk_frequency > 0)
+		clk_set_rate(rfkill->clk, rfkill->clk_frequency);
+
 	gpio = devm_gpiod_get_index(&pdev->dev, "reset", 0);
 	if (!IS_ERR(gpio)) {
 		ret = gpiod_direction_output(gpio, 0);
-- 
1.9.1

[PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Chen-Yu Tsai]

The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
part is a BCM20710 device connected to UART2 in the A20 SoC.

The IC requires a 32.768 KHz low power clock input for proper
auto-detection of the main clock, and an enable signal via GPIO.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index cb25d3c..767c8e1 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -61,6 +61,13 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
+				allwinner,pins = "PH18";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		uart0: serial@01c28000 {
@@ -69,6 +76,12 @@
 			status = "okay";
 		};
 
+		uart2: serial@01c28800 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&uart2_pins_a>;
+			status = "okay";
+		};
+
 		i2c0: i2c@01c2ac00 {
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
@@ -139,4 +152,16 @@
 	reg_usb2_vbus: usb2-vbus {
 		status = "okay";
 	};
+
+	rfkill_bt {
+		compatible = "rfkill-gpio";
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
+		clocks = <&clk_out_a>;
+		clock-frequency = <32768>;
+		gpios = <&pio 7 18 0>; /* PH18 */
+		gpio-names = "reset";
+		rfkill-name = "bt";
+		rfkill-type = <2>;
+	};
 };
-- 
1.9.1

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Maxime Ripard]

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

Hi Chen-Yu,

On Tue, Apr 15, 2014 at 02:41:35PM +0800, Chen-Yu Tsai wrote:
> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
> phandles by name only, through gpios/gpio-names, and not by index.

IIRC, gpios only uses the *-gpios properties, and not gpios/gpio-names
pattern seen on various other things.

Is it some new property you introduce? If so, please add it to the
documentation.

Now, I'm not sure that having two distinct representations of GPIOs in
the DT is a good thing. Yes, it's looking odd compared to other
similar bindings, but it's what we have to deal with.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 3/7] net: rfkill: gpio: use clk_prepare_enable/clk_disable_unprepare

[Maxime Ripard]

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

On Tue, Apr 15, 2014 at 02:41:37PM +0800, Chen-Yu Tsai wrote:
> rfkill-gpio calls clk_enable() without first calling clk_prepare(),
> resulting in a warning and no effect. Switch to clk_prepare_enable()
> and clk_disable_unprepare.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 4/7] net: rfkill: gpio: fix reversed clock enable state

[Maxime Ripard]

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

On Tue, Apr 15, 2014 at 02:41:38PM +0800, Chen-Yu Tsai wrote:
> rfkill-gpio has clk_enabled = blocked, which is true when rfkill
> blocks the device. This results in calling clock enable/disable at
> the wrong time. Reversing the value fixes this.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Maxime Ripard]

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

On Tue, Apr 15, 2014 at 02:41:41PM +0800, Chen-Yu Tsai wrote:
> The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
> part is a BCM20710 device connected to UART2 in the A20 SoC.
> 
> The IC requires a 32.768 KHz low power clock input for proper
> auto-detection of the main clock, and an enable signal via GPIO.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index cb25d3c..767c8e1 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -61,6 +61,13 @@
>  				allwinner,drive = <0>;
>  				allwinner,pull = <0>;
>  			};
> +
> +			bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
> +				allwinner,pins = "PH18";
> +				allwinner,function = "gpio_out";
> +				allwinner,drive = <0>;
> +				allwinner,pull = <0>;
> +			};
>  		};
>  
>  		uart0: serial@01c28000 {
> @@ -69,6 +76,12 @@
>  			status = "okay";
>  		};
>  
> +		uart2: serial@01c28800 {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&uart2_pins_a>;
> +			status = "okay";
> +		};
> +

Please make this a separate patch.

>  		i2c0: i2c@01c2ac00 {
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&i2c0_pins_a>;
> @@ -139,4 +152,16 @@
>  	reg_usb2_vbus: usb2-vbus {
>  		status = "okay";
>  	};
> +
> +	rfkill_bt {
> +		compatible = "rfkill-gpio";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> +		clocks = <&clk_out_a>;
> +		clock-frequency = <32768>;
> +		gpios = <&pio 7 18 0>; /* PH18 */
> +		gpio-names = "reset";
> +		rfkill-name = "bt";
> +		rfkill-type = <2>;
> +	};

Hmmm, I don't think that's actually right.

If you have such a device, then I'd expect it to be represented as a
full device in the DT, probably with one part for the WiFi, one part
for the Bluetooth, and here the definition of the rfkill device that
controls it.

But tying parts of the device to the rfkill that controls it, such as
the clocks, or the frequency it runs at seems just wrong.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 6/7] net: rfkill: gpio: add clock-frequency device tree property

[Maxime Ripard]

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

On Tue, Apr 15, 2014 at 02:41:40PM +0800, Chen-Yu Tsai wrote:
> Some devices, such as Broadcom Bluetooth devices, require a specific
> clock rate for the clock tied to the rfkill device. Add a clock-frequency
> property so we can specify this from the device tree.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

I think such a property belongs to the device that requires it, and
not to the rfkill that controls it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Chen-Yu Tsai]

On Tue, Apr 15, 2014 at 10:42 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Apr 15, 2014 at 02:41:41PM +0800, Chen-Yu Tsai wrote:
>> The CubieTruck has an AMPAK AP6210 WiFi+Bluetooth module. The Bluetooth
>> part is a BCM20710 device connected to UART2 in the A20 SoC.
>>
>> The IC requires a 32.768 KHz low power clock input for proper
>> auto-detection of the main clock, and an enable signal via GPIO.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> index cb25d3c..767c8e1 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> @@ -61,6 +61,13 @@
>>                               allwinner,drive = <0>;
>>                               allwinner,pull = <0>;
>>                       };
>> +
>> +                     bt_pwr_pin_cubietruck: bt_pwr_pin@0 {
>> +                             allwinner,pins = "PH18";
>> +                             allwinner,function = "gpio_out";
>> +                             allwinner,drive = <0>;
>> +                             allwinner,pull = <0>;
>> +                     };
>>               };
>>
>>               uart0: serial@01c28000 {
>> @@ -69,6 +76,12 @@
>>                       status = "okay";
>>               };
>>
>> +             uart2: serial@01c28800 {
>> +                     pinctrl-names = "default";
>> +                     pinctrl-0 = <&uart2_pins_a>;
>> +                     status = "okay";
>> +             };
>> +
>
> Please make this a separate patch.

Will do.

>>               i2c0: i2c@01c2ac00 {
>>                       pinctrl-names = "default";
>>                       pinctrl-0 = <&i2c0_pins_a>;
>> @@ -139,4 +152,16 @@
>>       reg_usb2_vbus: usb2-vbus {
>>               status = "okay";
>>       };
>> +
>> +     rfkill_bt {
>> +             compatible = "rfkill-gpio";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>> +             clocks = <&clk_out_a>;
>> +             clock-frequency = <32768>;
>> +             gpios = <&pio 7 18 0>; /* PH18 */
>> +             gpio-names = "reset";
>> +             rfkill-name = "bt";
>> +             rfkill-type = <2>;
>> +     };
>
> Hmmm, I don't think that's actually right.
>
> If you have such a device, then I'd expect it to be represented as a
> full device in the DT, probably with one part for the WiFi, one part
> for the Bluetooth, and here the definition of the rfkill device that
> controls it.

The AP6210 is not one device, but 2 separate chips in one module. Each
chip has its own controls and interface. They just so happen to share
the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
and interfaces. The WiFi side is most likely connected via SDIO, while
the Bluetooth side is connected to a UART, and optionally I2S for sound.

> But tying parts of the device to the rfkill that controls it, such as
> the clocks, or the frequency it runs at seems just wrong.

I understand where you're coming from. For devices on buses that require
drivers (such as USB, SDIO) these properties probably should be tied to
the device node.

For our use case here, which is a bluetooth chip connected on the UART,
there is no in kernel representation or driver to tie them to. Same goes
for UART based GPS chips. They just so happen to require toggling a GPIO,
and maybe enabling a specific clock, to get it running. Afterwards,
accessing it is done solely from userspace. For our Broadcom chips, the
user has to upload its firmware first, then designate the tty as a Bluetooth
HCI using hciattach.

We are using the rfkill device as a on-off switch.

Hope this explains the situation.


Cheers
ChenYu

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[One Thousand Gnomes]

> For our use case here, which is a bluetooth chip connected on the UART,
> there is no in kernel representation or driver to tie them to. Same goes
> for UART based GPS chips. They just so happen to require toggling a GPIO,
> and maybe enabling a specific clock, to get it running. Afterwards,
> accessing it is done solely from userspace. For our Broadcom chips, the
> user has to upload its firmware first, then designate the tty as a Bluetooth
> HCI using hciattach.

Somewhat similar on some ordinary PC systems. ACPI describes the
properties there but the same things apply - its a device mostly
controlled by a blob of userspace and having a bunch of GPIO lines that
do stuff like turn it on and off.

Is there any reason for not describing it properly and letting the
userspace code fish it out of the devicetree ?

There's no fundamental reason that it's magically different because of
the location of the driver. Given in a few cases we have a choice of
kernel or user space devices for the same hardware thats even more true ?

Failing that we have a long standing proposal never implemented for
adding GPIO awareness to the tty layer. There are a few other use
cases for it including gpio widgets with serial and either some of the
modem lines hacked in via GPIO or with additional control lines for
stuff like smartcard reading interfaces.

https://lkml.org/lkml/2012/5/16/288

In hindsight I'd do it slightly differently and make each "gpio" a pair
of values (purpose,number).

Alan

Re: [PATCH 5/7] net: rfkill: gpio: add device tree support

[Stephen Warren]

On 04/15/2014 12:41 AM, Chen-Yu Tsai wrote:

Patch description?

> diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt

> +Required properties:

> +- gpios		: At most two GPIO phandles
> +- gpio-names	: Shall be "reset" or "shutdown", matching gpios.
> +		  If both are provided, the "reset" GPIO is toggled first.

As Maxime mentioned, this is an unusual way of defining GPIOs. If this
new way is acceptable, then I'd suggest more precise wording, e.g.:

- gpios: Must contain an entry for each entry in gpio-names.
  See ../gpio/gpio.txt for details.
- gpio-names: May contain any or all of the following entries:
  - reset
  - shutdown

> +- rfkill-type	: Type of RFKILL device: 1 for WiFi, 2 for BlueTooth, etc.
> +		  See include/uapi/linux/rfkill.h for all valid values

It would be nice if include/dt-bindings/rfkill-gpio.h existed and
contained e.g.:

#define RFKILL_TYPE_BLUETOOTH 2

So that:

> +Example:
> +
> +	rfkill_bt {
...
> +		rfkill-type = <2>;

Could be written as:

		rfkill-type = <RFKILL_TYPE_BLUETOOTH>;

Re: [PATCH 5/7] net: rfkill: gpio: add device tree support

[Stephen Warren]

On 04/15/2014 12:41 AM, Chen-Yu Tsai wrote:

> diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt

> +Optional properties:
> +- clocks		: phandle to clock to enable/disable

Oh, and can't we use clock-names here too, with wording like:

- clocks: Must contain an entry for each entry in clock-names.
  See ../clocks/clock-bindings.txt for details.
- clock-names: May contain any of the following entries:
  - module

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Alexandre Courbot]

On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Chen-Yu,
>
> On Tue, Apr 15, 2014 at 02:41:35PM +0800, Chen-Yu Tsai wrote:
>> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
>> phandles by name only, through gpios/gpio-names, and not by index.
>
> IIRC, gpios only uses the *-gpios properties, and not gpios/gpio-names
> pattern seen on various other things.
>
> Is it some new property you introduce? If so, please add it to the
> documentation.
>
> Now, I'm not sure that having two distinct representations of GPIOs in
> the DT is a good thing. Yes, it's looking odd compared to other
> similar bindings, but it's what we have to deal with.

Mmmm I *think* I somehow remember a discussion about this topic
recently, but I cannot find it. Maybe Chen-yu could point us to the
conclusion of this discussion and the rationale for (re)implementing
named GPIOs this way?

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Alexandre Courbot]

On Wed, Apr 16, 2014 at 3:12 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi Chen-Yu,
>>
>> On Tue, Apr 15, 2014 at 02:41:35PM +0800, Chen-Yu Tsai wrote:
>>> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
>>> phandles by name only, through gpios/gpio-names, and not by index.
>>
>> IIRC, gpios only uses the *-gpios properties, and not gpios/gpio-names
>> pattern seen on various other things.
>>
>> Is it some new property you introduce? If so, please add it to the
>> documentation.
>>
>> Now, I'm not sure that having two distinct representations of GPIOs in
>> the DT is a good thing. Yes, it's looking odd compared to other
>> similar bindings, but it's what we have to deal with.
>
> Mmmm I *think* I somehow remember a discussion about this topic
> recently, but I cannot find it. Maybe Chen-yu could point us to the
> conclusion of this discussion and the rationale for (re)implementing
> named GPIOs this way?

Aha, here maybe:

https://lkml.org/lkml/2014/1/21/164

However I don't see a clear conclusion that we should implement that
scheme. Not that I am strongly against it, but I'd like to see a
practical purpose for it.

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Maxime Ripard]

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

Hi,

Please try to keep me in CC, even though the ML doesn't make it easy..

On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >> @@ -139,4 +152,16 @@
> >>       reg_usb2_vbus: usb2-vbus {
> >>               status = "okay";
> >>       };
> >> +
> >> +     rfkill_bt {
> >> +             compatible = "rfkill-gpio";
> >> +             pinctrl-names = "default";
> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >> +             clocks = <&clk_out_a>;
> >> +             clock-frequency = <32768>;
> >> +             gpios = <&pio 7 18 0>; /* PH18 */
> >> +             gpio-names = "reset";
> >> +             rfkill-name = "bt";
> >> +             rfkill-type = <2>;
> >> +     };
> >
> > Hmmm, I don't think that's actually right.
> >
> > If you have such a device, then I'd expect it to be represented as a
> > full device in the DT, probably with one part for the WiFi, one part
> > for the Bluetooth, and here the definition of the rfkill device that
> > controls it.
> 
> The AP6210 is not one device, but 2 separate chips in one module. Each
> chip has its own controls and interface. They just so happen to share
> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> and interfaces. The WiFi side is most likely connected via SDIO, while
> the Bluetooth side is connected to a UART, and optionally I2S for sound.

It's even easier to represent then.

> > But tying parts of the device to the rfkill that controls it, such as
> > the clocks, or the frequency it runs at seems just wrong.
> 
> I understand where you're coming from. For devices on buses that require
> drivers (such as USB, SDIO) these properties probably should be tied to
> the device node.
> 
> For our use case here, which is a bluetooth chip connected on the UART,
> there is no in kernel representation or driver to tie them to. Same goes
> for UART based GPS chips. They just so happen to require toggling a GPIO,
> and maybe enabling a specific clock, to get it running. Afterwards,
> accessing it is done solely from userspace. For our Broadcom chips, the
> user has to upload its firmware first, then designate the tty as a Bluetooth
> HCI using hciattach.
> 
> We are using the rfkill device as a on-off switch.

I understand your point, but the fact that it's implemented in
user-space, or that UART is not a bus (which probably should be), is
only a Linux specific story, and how it's implemented in Linux (even
if the whole rfkill node is another one, but let's stay on topic).

This is a huge abstraction leak.

Let's say you need the I2S stream you mentionned for some
reason. Would you tie the audio stream to the rfkill node as well?
I'm sorry, but from an hardware description perspective, it makes no
sense.

What's the feeling of the DT maintainers?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Chen-Yu Tsai]

Hi,

On Wed, Apr 16, 2014 at 3:06 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Apr 16, 2014 at 3:12 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>>> Hi Chen-Yu,
>>>
>>> On Tue, Apr 15, 2014 at 02:41:35PM +0800, Chen-Yu Tsai wrote:
>>>> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
>>>> phandles by name only, through gpios/gpio-names, and not by index.
>>>
>>> IIRC, gpios only uses the *-gpios properties, and not gpios/gpio-names
>>> pattern seen on various other things.
>>>
>>> Is it some new property you introduce? If so, please add it to the
>>> documentation.
>>>
>>> Now, I'm not sure that having two distinct representations of GPIOs in
>>> the DT is a good thing. Yes, it's looking odd compared to other
>>> similar bindings, but it's what we have to deal with.
>>
>> Mmmm I *think* I somehow remember a discussion about this topic
>> recently, but I cannot find it. Maybe Chen-yu could point us to the
>> conclusion of this discussion and the rationale for (re)implementing
>> named GPIOs this way?
>
> Aha, here maybe:
>
> https://lkml.org/lkml/2014/1/21/164

They're also mentioned in:

https://lkml.org/lkml/2014/2/25/581

> However I don't see a clear conclusion that we should implement that
> scheme. Not that I am strongly against it, but I'd like to see a
> practical purpose for it.

Again no clear conclusion on this. I wrote this as it was one possible
way out of the index-based GPIO stuff.

Hopefully others will chime in and we can decide whether this is what
we want or not.


Cheers
ChenYu

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Chen-Yu Tsai]

Hi,

On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>> >> @@ -139,4 +152,16 @@
>> >>       reg_usb2_vbus: usb2-vbus {
>> >>               status = "okay";
>> >>       };
>> >> +
>> >> +     rfkill_bt {
>> >> +             compatible = "rfkill-gpio";
>> >> +             pinctrl-names = "default";
>> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>> >> +             clocks = <&clk_out_a>;
>> >> +             clock-frequency = <32768>;
>> >> +             gpios = <&pio 7 18 0>; /* PH18 */
>> >> +             gpio-names = "reset";
>> >> +             rfkill-name = "bt";
>> >> +             rfkill-type = <2>;
>> >> +     };
>> >
>> > Hmmm, I don't think that's actually right.
>> >
>> > If you have such a device, then I'd expect it to be represented as a
>> > full device in the DT, probably with one part for the WiFi, one part
>> > for the Bluetooth, and here the definition of the rfkill device that
>> > controls it.
>>
>> The AP6210 is not one device, but 2 separate chips in one module. Each
>> chip has its own controls and interface. They just so happen to share
>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>> and interfaces. The WiFi side is most likely connected via SDIO, while
>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
>
> It's even easier to represent then.
>
>> > But tying parts of the device to the rfkill that controls it, such as
>> > the clocks, or the frequency it runs at seems just wrong.
>>
>> I understand where you're coming from. For devices on buses that require
>> drivers (such as USB, SDIO) these properties probably should be tied to
>> the device node.
>>
>> For our use case here, which is a bluetooth chip connected on the UART,
>> there is no in kernel representation or driver to tie them to. Same goes
>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>> and maybe enabling a specific clock, to get it running. Afterwards,
>> accessing it is done solely from userspace. For our Broadcom chips, the
>> user has to upload its firmware first, then designate the tty as a Bluetooth
>> HCI using hciattach.
>>
>> We are using the rfkill device as a on-off switch.
>
> I understand your point, but the fact that it's implemented in
> user-space, or that UART is not a bus (which probably should be), is
> only a Linux specific story, and how it's implemented in Linux (even
> if the whole rfkill node is another one, but let's stay on topic).

I gave it some thought last night. You are right. My whole approach
is wrong. But let's try to make it right.

So considering the fact that it's primarily connected to a UART,
maybe I should make it a sub-node to the UART node it's actually
connected to? Something like:

        uart2: serial@01c28800 {
                pinctrl-names = "default";
                pinctrl-0 = <&uart2_pins_a>;
                status = "okay";

                bt: bt_hci {
                        compatible = "brcm,bcm20710";
                        /* maybe add some generic compatible */
                        pinctrl-names = "default";
                        pinctrl-0 = <&clk_out_a_pins_a>,
<&bt_pwr_pin_cubietruck>;
                        clocks = <&clk_out_a>;
                        clock-frequency = <32768>;
                        gpios = <&pio 7 18 0>; /* PH18 */
                };
        };

And let the uart core handle power sequencing for sub-nodes.

The rfkill node would still have the gpios and clocks, but not the
clock-frequency property. It's sole purpose would be to toggle the
controls. But I think the placement is still odd. Perhaps these
virtual devices shouldn't live in the DT at all.

> This is a huge abstraction leak.
>
> Let's say you need the I2S stream you mentionned for some
> reason. Would you tie the audio stream to the rfkill node as well?
> I'm sorry, but from an hardware description perspective, it makes no
> sense.

The above revision should be better, from a hardware perspective. I'm
not sure how to tie in the I2S stream, and there I haven't found any
examples in the DT tree.

> What's the feeling of the DT maintainers?


Cheers

ChenYu

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[Hans de Goede]

Hi,

On 04/16/2014 11:44 AM, Maxime Ripard wrote:
> Hi,
> 
> Please try to keep me in CC, even though the ML doesn't make it easy..
> 
> On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
>>>> @@ -139,4 +152,16 @@
>>>>       reg_usb2_vbus: usb2-vbus {
>>>>               status = "okay";
>>>>       };
>>>> +
>>>> +     rfkill_bt {
>>>> +             compatible = "rfkill-gpio";
>>>> +             pinctrl-names = "default";
>>>> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
>>>> +             clocks = <&clk_out_a>;
>>>> +             clock-frequency = <32768>;
>>>> +             gpios = <&pio 7 18 0>; /* PH18 */
>>>> +             gpio-names = "reset";
>>>> +             rfkill-name = "bt";
>>>> +             rfkill-type = <2>;
>>>> +     };
>>>
>>> Hmmm, I don't think that's actually right.
>>>
>>> If you have such a device, then I'd expect it to be represented as a
>>> full device in the DT, probably with one part for the WiFi, one part
>>> for the Bluetooth, and here the definition of the rfkill device that
>>> controls it.
>>
>> The AP6210 is not one device, but 2 separate chips in one module. Each
>> chip has its own controls and interface. They just so happen to share
>> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
>> and interfaces. The WiFi side is most likely connected via SDIO, while
>> the Bluetooth side is connected to a UART, and optionally I2S for sound.
> 
> It's even easier to represent then.
> 
>>> But tying parts of the device to the rfkill that controls it, such as
>>> the clocks, or the frequency it runs at seems just wrong.
>>
>> I understand where you're coming from. For devices on buses that require
>> drivers (such as USB, SDIO) these properties probably should be tied to
>> the device node.
>>
>> For our use case here, which is a bluetooth chip connected on the UART,
>> there is no in kernel representation or driver to tie them to. Same goes
>> for UART based GPS chips. They just so happen to require toggling a GPIO,
>> and maybe enabling a specific clock, to get it running. Afterwards,
>> accessing it is done solely from userspace. For our Broadcom chips, the
>> user has to upload its firmware first, then designate the tty as a Bluetooth
>> HCI using hciattach.
>>
>> We are using the rfkill device as a on-off switch.
> 
> I understand your point, but the fact that it's implemented in
> user-space, or that UART is not a bus (which probably should be), is
> only a Linux specific story, and how it's implemented in Linux (even
> if the whole rfkill node is another one, but let's stay on topic).
> 
> This is a huge abstraction leak.
> 
> Let's say you need the I2S stream you mentionned for some
> reason. Would you tie the audio stream to the rfkill node as well?
> I'm sorry, but from an hardware description perspective, it makes no
> sense.

Sorry wens, but I have to agree with Maxime here. What I really would
like to see is the bluetooth part of the AP6210 be a child node of the
uart node like how i2c slaves are child nodes of the i2c master node.

With my distro maintainer had on, I say that even under Linux we will
need this. Sure we can get things to work from userspace manually,
by running the necessary commands. But we want things to work automatically,
so we will need to know about the attached bluetooth device in userspace,
and the proper way to do that is to put the info in devicetree. I've not
yet thought about how userspace can then extract and react on this info,
but for things to work plug and play with a generic distro rootfs we
will need to eventually have some udev rules doing the firmware loading
and hciattach. Which starts with being able to identify that the uart
has a bcm hci bluetooth adapter attached.

Regards,

Hans

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[maxime.ripard]

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

On Wed, Apr 16, 2014 at 06:39:28PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > Please try to keep me in CC, even though the ML doesn't make it easy..
> 
> Sorry about that.
> 
> > On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >> >> @@ -139,4 +152,16 @@
> >> >>       reg_usb2_vbus: usb2-vbus {
> >> >>               status = "okay";
> >> >>       };
> >> >> +
> >> >> +     rfkill_bt {
> >> >> +             compatible = "rfkill-gpio";
> >> >> +             pinctrl-names = "default";
> >> >> +             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >> >> +             clocks = <&clk_out_a>;
> >> >> +             clock-frequency = <32768>;
> >> >> +             gpios = <&pio 7 18 0>; /* PH18 */
> >> >> +             gpio-names = "reset";
> >> >> +             rfkill-name = "bt";
> >> >> +             rfkill-type = <2>;
> >> >> +     };
> >> >
> >> > Hmmm, I don't think that's actually right.
> >> >
> >> > If you have such a device, then I'd expect it to be represented as a
> >> > full device in the DT, probably with one part for the WiFi, one part
> >> > for the Bluetooth, and here the definition of the rfkill device that
> >> > controls it.
> >>
> >> The AP6210 is not one device, but 2 separate chips in one module. Each
> >> chip has its own controls and interface. They just so happen to share
> >> the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> >> and interfaces. The WiFi side is most likely connected via SDIO, while
> >> the Bluetooth side is connected to a UART, and optionally I2S for sound.
> >
> > It's even easier to represent then.
> >
> >> > But tying parts of the device to the rfkill that controls it, such as
> >> > the clocks, or the frequency it runs at seems just wrong.
> >>
> >> I understand where you're coming from. For devices on buses that require
> >> drivers (such as USB, SDIO) these properties probably should be tied to
> >> the device node.
> >>
> >> For our use case here, which is a bluetooth chip connected on the UART,
> >> there is no in kernel representation or driver to tie them to. Same goes
> >> for UART based GPS chips. They just so happen to require toggling a GPIO,
> >> and maybe enabling a specific clock, to get it running. Afterwards,
> >> accessing it is done solely from userspace. For our Broadcom chips, the
> >> user has to upload its firmware first, then designate the tty as a Bluetooth
> >> HCI using hciattach.
> >>
> >> We are using the rfkill device as a on-off switch.
> >
> > I understand your point, but the fact that it's implemented in
> > user-space, or that UART is not a bus (which probably should be), is
> > only a Linux specific story, and how it's implemented in Linux (even
> > if the whole rfkill node is another one, but let's stay on topic).
> 
> I gave it some thought last night. You are right. My whole approach
> is wrong. But let's try to make it right.
> 
> So considering the fact that it's primarily connected to a UART,
> maybe I should make it a sub-node to the UART node it's actually
> connected to? Something like:
> 
>         uart2: serial@01c28800 {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&uart2_pins_a>;
>                 status = "okay";
> 
>                 bt: bt_hci {
>                         compatible = "brcm,bcm20710";
>                         /* maybe add some generic compatible */
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&clk_out_a_pins_a>,
> <&bt_pwr_pin_cubietruck>;
>                         clocks = <&clk_out_a>;
>                         clock-frequency = <32768>;
>                         gpios = <&pio 7 18 0>; /* PH18 */
>                 };
>         };
> 
> And let the uart core handle power sequencing for sub-nodes.
> 
> The rfkill node would still have the gpios and clocks, but not the
> clock-frequency property. It's sole purpose would be to toggle the
> controls. But I think the placement is still odd. Perhaps these
> virtual devices shouldn't live in the DT at all.

Yes, it looks much better.

I agree with you on the virtual devices things, and we have a lot of
other examples unfortunately.

However, since the rfkill nodes are in the DT, I think you'd still
have a rfkill node to handle the gpio, and a reference to the killed
device of some sort.

As far as the clock is concerned, I don't know if it makes sense to
have the BT clock in the RF kill node.

You probably want to use it to gate it whenever the device is killed,
but if the device is setting the frequency, it will more likely hold a
reference to that clock, so calling the disable in rfkill won't do
much.

Is rfkill sending any notification of some sort that the device is
being killed?

> > This is a huge abstraction leak.
> >
> > Let's say you need the I2S stream you mentionned for some
> > reason. Would you tie the audio stream to the rfkill node as well?
> > I'm sorry, but from an hardware description perspective, it makes no
> > sense.
> 
> The above revision should be better, from a hardware perspective. I'm
> not sure how to tie in the I2S stream, and there I haven't found any
> examples in the DT tree.

If it acts like an I2S "consumer", you can use ASoC's simple-card, and
you have a few examples in the other DTs.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [linux-sunxi] Re: [PATCH 7/7] ARM: sun7i: cubietruck: enable bluetooth module

[maxime.ripard]

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

On Thu, Apr 17, 2014 at 09:43:40AM +0200, Arend van Spriel wrote:
> + linux-serial@vger.kernel.org
> 
> On 16/04/14 15:09, Hans de Goede wrote:
> >Hi,
> >
> >On 04/16/2014 12:39 PM, Chen-Yu Tsai wrote:
> >>Hi,
> >>
> >>On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard
> >><maxime.ripard@free-electrons.com> wrote:
> >>>Hi,
> >>>
> >>>Please try to keep me in CC, even though the ML doesn't make it easy..
> >>
> >>Sorry about that.
> >>
> >>>On Wed, Apr 16, 2014 at 12:06:59AM +0800, Chen-Yu Tsai wrote:
> >>>>>>@@ -139,4 +152,16 @@
> >>>>>>       reg_usb2_vbus: usb2-vbus {
> >>>>>>               status = "okay";
> >>>>>>       };
> >>>>>>+
> >>>>>>+     rfkill_bt {
> >>>>>>+             compatible = "rfkill-gpio";
> >>>>>>+             pinctrl-names = "default";
> >>>>>>+             pinctrl-0 = <&bt_pwr_pin_cubietruck>, <&clk_out_a_pins_a>;
> >>>>>>+             clocks = <&clk_out_a>;
> >>>>>>+             clock-frequency = <32768>;
> >>>>>>+             gpios = <&pio 7 18 0>; /* PH18 */
> >>>>>>+             gpio-names = "reset";
> >>>>>>+             rfkill-name = "bt";
> >>>>>>+             rfkill-type = <2>;
> >>>>>>+     };
> >>>>>
> >>>>>Hmmm, I don't think that's actually right.
> >>>>>
> >>>>>If you have such a device, then I'd expect it to be represented as a
> >>>>>full device in the DT, probably with one part for the WiFi, one part
> >>>>>for the Bluetooth, and here the definition of the rfkill device that
> >>>>>controls it.
> >>>>
> >>>>The AP6210 is not one device, but 2 separate chips in one module. Each
> >>>>chip has its own controls and interface. They just so happen to share
> >>>>the same enclosure. Even 2-in-1 chips by Broadcom have separate controls
> >>>>and interfaces. The WiFi side is most likely connected via SDIO, while
> >>>>the Bluetooth side is connected to a UART, and optionally I2S for sound.
> >>>
> >>>It's even easier to represent then.
> >>>
> >>>>>But tying parts of the device to the rfkill that controls it, such as
> >>>>>the clocks, or the frequency it runs at seems just wrong.
> >>>>
> >>>>I understand where you're coming from. For devices on buses that require
> >>>>drivers (such as USB, SDIO) these properties probably should be tied to
> >>>>the device node.
> >>>>
> >>>>For our use case here, which is a bluetooth chip connected on the UART,
> >>>>there is no in kernel representation or driver to tie them to. Same goes
> >>>>for UART based GPS chips. They just so happen to require toggling a GPIO,
> >>>>and maybe enabling a specific clock, to get it running. Afterwards,
> >>>>accessing it is done solely from userspace. For our Broadcom chips, the
> >>>>user has to upload its firmware first, then designate the tty as a Bluetooth
> >>>>HCI using hciattach.
> >>>>
> >>>>We are using the rfkill device as a on-off switch.
> >>>
> >>>I understand your point, but the fact that it's implemented in
> >>>user-space, or that UART is not a bus (which probably should be), is
> >>>only a Linux specific story, and how it's implemented in Linux (even
> >>>if the whole rfkill node is another one, but let's stay on topic).
> >>
> >>I gave it some thought last night. You are right. My whole approach
> >>is wrong. But let's try to make it right.
> >>
> >>So considering the fact that it's primarily connected to a UART,
> >>maybe I should make it a sub-node to the UART node it's actually
> >>connected to? Something like:
> >>
> >>         uart2: serial@01c28800 {
> >>                 pinctrl-names = "default";
> >>                 pinctrl-0 = <&uart2_pins_a>;
> >>                 status = "okay";
> >>
> >>                 bt: bt_hci {
> >>                         compatible = "brcm,bcm20710";
> >>                         /* maybe add some generic compatible */
> >>                         pinctrl-names = "default";
> >>                         pinctrl-0 = <&clk_out_a_pins_a>,
> >><&bt_pwr_pin_cubietruck>;
> >>                         clocks = <&clk_out_a>;
> >>                         clock-frequency = <32768>;
> >>                         gpios = <&pio 7 18 0>; /* PH18 */
> >>                 };
> >>         };
> >>
> >>And let the uart core handle power sequencing for sub-nodes.
> >
> >Great, I missed this reply when I typed my mail I send a few minutes
> >ago. I agree that this approach is how thing should be.
> 
> Regarding the device tree hierarchy this seems right, but powering
> the sub-nodes seems outside the realm of uart core.

Yet, a lot of devices are connected to an UART: GPS, BT chips, GSM
modems, even some odd PMICs, so UART acting like a real bus might make
sense.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 2/7] gpiolib: Support purely name based gpiod lookup in device trees

[Linus Walleij]

On Tue, Apr 15, 2014 at 8:41 AM, Chen-Yu Tsai <wens@csie.org> wrote:

> This patch enables gpio-names based gpiod lookup in device tree usage,
> which ignores the index passed to gpiod_get_index. If this fails, fall
> back to the original function-index ("con_id"-gpios) based lookup scheme,
> for backward compatibility and any drivers needing more than one GPIO
> for any function.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

This looks a bit scary and I cannot claim to realize the entire semantic
effect of this patch, but if it's the way to go then OK.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Shall I just apply this to the GPIO tree or are you carrying it along with
the other stuff?

Yours,
Linus Walleij

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Linus Walleij]

On Tue, Apr 15, 2014 at 8:41 AM, Chen-Yu Tsai <wens@csie.org> wrote:

> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
> phandles by name only, through gpios/gpio-names, and not by index.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Like Alexandre I have no strong opinion on this alternative scheme.

However if I shall apply this patch I want ACKs from the DT maintainers
with them expressing that they want things to look like this going
forward.

Otherwise the set is stalled right here.

Yours,
Linus Walleij

Re: [PATCH 0/7] net: rfkill: gpio: Add device tree support

[Johannes Berg]

On Tue, 2014-04-15 at 14:41 +0800, Chen-Yu Tsai wrote:

> The first 2 patches should go through the gpio tree. The 4 rfkill-gpio
> patches should go through the same tree that Heikki's patches are
> in. Maxime, can you take the last one?

Are there no dependencies? Maybe it'd be easier if you sent it as
separate patch series then?

I can take those four patches into my topic branch on top of Heikki's
patches, but in any case, there seem to be a bunch of
comments/discussions so for now I'm not going to do anything with them.

johannes

Re: [PATCH 2/7] gpiolib: Support purely name based gpiod lookup in device trees

[Maxime Ripard]

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

Hi Linus,

On Tue, Apr 22, 2014 at 05:00:17PM +0200, Linus Walleij wrote:
> On Tue, Apr 15, 2014 at 8:41 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> 
> > This patch enables gpio-names based gpiod lookup in device tree usage,
> > which ignores the index passed to gpiod_get_index. If this fails, fall
> > back to the original function-index ("con_id"-gpios) based lookup scheme,
> > for backward compatibility and any drivers needing more than one GPIO
> > for any function.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> 
> This looks a bit scary and I cannot claim to realize the entire semantic
> effect of this patch, but if it's the way to go then OK.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Shall I just apply this to the GPIO tree or are you carrying it along with
> the other stuff?

This patch has a dependency on the first patch of this serie, since it
uses a binding that was documented there.

Maybe you'll want to wait for the ACK from the DT maintainers to merge
this one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Alexandre Courbot]

On Wed, Apr 23, 2014 at 12:02 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Apr 15, 2014 at 8:41 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>
>> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
>> phandles by name only, through gpios/gpio-names, and not by index.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> Like Alexandre I have no strong opinion on this alternative scheme.

Yeah this new lookup scheme probably does no harm, but I think it
should be a little bit more motivated as it is, after all, introducing
more potential confusion for DT users.

It does not look like this new lookup scheme is necessary to Chen-Yu's
patchset and that he could as well have used the current one. Right
now there is only one way to define GPIOs - if we introduce a second
one, then which one should new DT users choose? Which one looks
better? I can already endless fights taking place over this.

Does this new lookup help with some of the existing problems we have
like ACPI/DT lookup compatibility?

I just need to be given one practical reason to give my ack.

Alex.

Re: [PATCH 2/7] gpiolib: Support purely name based gpiod lookup in device trees

[Linus Walleij]

On Tue, Apr 22, 2014 at 5:18 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> This patch has a dependency on the first patch of this serie, since it
> uses a binding that was documented there.
>
> Maybe you'll want to wait for the ACK from the DT maintainers to merge
> this one.

Yeah I realized this a bit later, sorry for being a bit confused :-/

Yours,
Linus Walleij

Re: [linux-sunxi] Re: [PATCH 1/7] gpiolib: gpiolib-of: Implement device tree gpio-names based lookup

[Chen-Yu Tsai]

On Wed, Apr 23, 2014 at 9:49 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Apr 23, 2014 at 12:02 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Tue, Apr 15, 2014 at 8:41 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>>
>>> This patch provides of_get_gpiod_flags_by_name(), which looks up GPIO
>>> phandles by name only, through gpios/gpio-names, and not by index.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>
>> Like Alexandre I have no strong opinion on this alternative scheme.
>
> Yeah this new lookup scheme probably does no harm, but I think it
> should be a little bit more motivated as it is, after all, introducing
> more potential confusion for DT users.
>
> It does not look like this new lookup scheme is necessary to Chen-Yu's
> patchset and that he could as well have used the current one. Right
> now there is only one way to define GPIOs - if we introduce a second
> one, then which one should new DT users choose? Which one looks
> better? I can already endless fights taking place over this.

I will pull out the two patches.

> Does this new lookup help with some of the existing problems we have
> like ACPI/DT lookup compatibility?

I hope they will be compatible with ACPI named gpios, whenever support
for ACPI named properties extension lands. But that really depends on
the ACPI implementation. For now I think it's best that I just hold on
to these. We can revisit the discussion later if needed.

> I just need to be given one practical reason to give my ack.


Thanks
ChenYu