[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-***@public.gmane.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;

[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-***@public.gmane.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);
}

[Maxime Ripard]

Post by 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.

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

Maxime

[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-***@public.gmane.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;
}

[Maxime Ripard]

Post by 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.

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/***@public.gmane.org>

Thanks!
Maxime

[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-***@public.gmane.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);

[Maxime Ripard]

Post by 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.

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

Maxime

[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-***@public.gmane.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);

[Maxime Ripard]

Hi Chen-Yu,

Post by 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.

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

[Alexandre Courbot]

On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard

Post by Maxime Ripard
Hi Chen-Yu,

Post by 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.

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?

[Alexandre Courbot]

Post by Alexandre Courbot
On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard

Post by Maxime Ripard
Hi Chen-Yu,

Post by 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.

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.

[Chen-Yu Tsai]

Hi,

Post by Alexandre Courbot

Post by Alexandre Courbot
On Tue, Apr 15, 2014 at 11:20 PM, Maxime Ripard

Post by Maxime Ripard
Hi Chen-Yu,

Post by 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.

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?

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

They're also mentioned in:

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

Post by Alexandre Courbot
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

[Linus Walleij]

Post by 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.

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

[Alexandre Courbot]

On Wed, Apr 23, 2014 at 12:02 AM, Linus Walleij

Post by Linus Walleij

Post by 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.

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.

[Chen-Yu Tsai]

Post by Alexandre Courbot
On Wed, Apr 23, 2014 at 12:02 AM, Linus Walleij

Post by Linus Walleij

Post by 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.

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.

Post by Alexandre Courbot
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.

Post by Alexandre Courbot
I just need to be given one practical reason to give my ack.

Thanks
ChenYu

[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-***@public.gmane.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: ***@0 {
+ allwinner,pins = "PH18";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
};

uart0: ***@01c28000 {
@@ -69,6 +76,12 @@
status = "okay";
};

+ uart2: ***@01c28800 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins_a>;
+ status = "okay";
+ };
+
i2c0: ***@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>;
+ };
};

[Maxime Ripard]

Post by 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.
---
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>;
};
+
+ allwinner,pins = "PH18";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
};
@@ -69,6 +76,12 @@
status = "okay";
};
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins_a>;
+ status = "okay";
+ };
+

Please make this a separate patch.

Post by Chen-Yu Tsai
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

[Chen-Yu Tsai]

On Tue, Apr 15, 2014 at 10:42 PM, Maxime Ripard

Post by Maxime Ripard

Post by 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.
---
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>;
};
+
+ allwinner,pins = "PH18";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
};
@@ -69,6 +76,12 @@
status = "okay";
};
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins_a>;
+ status = "okay";
+ };
+

Please make this a separate patch.

Will do.

Post by Maxime Ripard

Post by Chen-Yu Tsai
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.

Post by Maxime Ripard
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

[One Thousand Gnomes]

Post by Chen-Yu Tsai
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

[Maxime Ripard]

Hi,

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

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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?

[Chen-Yu Tsai]

Hi,

On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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: ***@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.

Post by Maxime Ripard
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.

Post by Maxime Ripard
What's the feeling of the DT maintainers?

Cheers

ChenYu

[Hans de Goede]

Hi,

Post by Chen-Yu Tsai
Hi,
On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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
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.

Regards,

Hans

[Arend van Spriel]

Post by Hans de Goede
Hi,

Post by Chen-Yu Tsai
Hi,
On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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
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.

Post by Hans de Goede
Regards,
Hans
_______________________________________________
linux-arm-kernel mailing list
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[maxime.ripard]

Post by Arend van Spriel

Post by Hans de Goede
Hi,

Post by Chen-Yu Tsai
Hi,
On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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
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]

Post by Chen-Yu Tsai
Hi,
On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Sorry about that.

Post by Maxime Ripard

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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
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?

Post by Chen-Yu Tsai

Post by Maxime Ripard
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

[Hans de Goede]

Hi,

Post by Maxime Ripard
Hi,
Please try to keep me in CC, even though the ML doesn't make it easy..

Post by Chen-Yu Tsai

Post by Maxime Ripard

Post by Chen-Yu Tsai
@@ -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.

Post by Chen-Yu Tsai

Post by Maxime Ripard
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

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens-***@public.gmane.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),
},
};

[Stephen Warren]

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

Patch description?

Post by Chen-Yu Tsai
diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
+- 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

Post by Chen-Yu Tsai
+- 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

Post by Chen-Yu Tsai
+
+ rfkill_bt {

...

Post by Chen-Yu Tsai
+ rfkill-type = <2>;

Could be written as:

rfkill-type = <RFKILL_TYPE_BLUETOOTH>;

[Stephen Warren]

Post by Chen-Yu Tsai
diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt
+- 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

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Linus Walleij]

Post by 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.

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-QSEj5FYQhm4dnm+***@public.gmane.org>

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

Yours,
Linus Walleij

[Maxime Ripard]

Hi Linus,

Post by Linus Walleij

Post by 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.

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.
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

[Linus Walleij]

On Tue, Apr 22, 2014 at 5:18 PM, Maxime Ripard

Post by Maxime Ripard
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

[Johannes Berg]

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