Hi, On Wed, Apr 16, 2014 at 5:44 PM, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html