Return-Path: Date: Tue, 27 Mar 2018 11:47:22 -0700 From: Bjorn Andersson To: Thierry Escande Cc: Marcel Holtmann , Rob Herring , Andy Gross , Johan Hedberg , David Brown , Mark Rutland , Andy Shevchenko , Loic Poulain , Srinivas Kandagatla , linux-bluetooth@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth Message-ID: <20180327184722.GB18510@minitux> References: <20180320032331.29865-1-thierry.escande@linaro.org> <20180320032331.29865-3-thierry.escande@linaro.org> <20180326225106.GC1403@tuxbook-pro> <2e0db4b4-80cc-9a5b-9e0a-812f1bc9fbcf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <2e0db4b4-80cc-9a5b-9e0a-812f1bc9fbcf@linaro.org> List-ID: On Tue 27 Mar 08:56 PDT 2018, Thierry Escande wrote: > Hi Bjorn, > > On 27/03/2018 00:51, Bjorn Andersson wrote: > > On Tue 20 Mar 23:58 HKT 2018, Marcel Holtmann wrote: > > > > Signed-off-by: Thierry Escande > > [..] > > > > + - clocks: clock phandle for SUSCLK_32KHZ > > > > > > if I compare this with broadcom-bluetooth.txt or ti-bluetooth.txt then > > > besides compatible, everything else is optional. The > > > nokia-bluetooth.txt has everything required, but that is also a really > > > specific platform. > > > > > > Can we be less restrictive for a QCA general purpose chip? > > > > > > > The way we deal with this in other bindings is that we tie such > > requirements to the compatible; i.e. we would say that qcom,qca6174-bt > > requires a clock and we would have something like qcom,qca-bt that makes > > it optional. > > > > The beauty of this is that the driver will tell you if you forgot to > > specify the clock when it actually is required, which saves you > > considerable amount of debugging time. > > > > > > NB. The way the bcm driver handles this is insufficient, as it treats > > any error from clk_get as "there's no clock specified". The driver > > should accept a clock not being specified, but should fail properly when > > a clock is specified but can't be acquired (e.g. due to clk_get() > > returning EPROBE_DEFER). > > > > > > + > > > > +Example: > > > > + > > > > +serial@7570000 { > > > > + pinctrl-names = "default", "sleep"; > > > > + pinctrl-0 = <&blsp1_uart1_default>; > > > > + pinctrl-1 = <&blsp1_uart1_sleep>; > > > > + > > > > + bluetooth { > > > > + compatible = "qcom,qca6174-bt"; > > > > + > > > > + enable-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>; > > > > + > > > > + pinctrl-names = "default"; > > > > + pinctrl-0 = <&bt_en_pin_a>; > > > > > > This one I do not understand and you might want to shed some light > > > into why this is done that way. > > > > > > > This is completely generic and only relates to getting the electrical > > properties of the gpio pin set up correctly. So I would recommend that > > we omit this from the binding and example (including the pinctrl > > properties for the serial above). > > If I remove the pinctrl entry in the bluetooth node, the gpio19 is then > marked as unclaimed. The drive strength also defaults to low but that > doesn't seem to be an issue and the the chip can still be enabled through > gpio19. Is it ok to have it unclaimed? If so I can remove it from the > binding and the doc then. > > Regarding the blsp1_uart1_default of the serial node, I can still enable the > chip if I remove it but the hci commands all end in timeout. It seems that > the function for these pins has to be explicitly set to blsp_uart2. So at > least, the default pinctrl seems mandatory. > Our board needs these properties to get the uart and gpio in the right state, but this is unrelated to BT - that's why I suggested that you omit these properties from the BT binding. Regards, Bjorn