This patch series we add support for wcn3998 BT chip set. This new chipset
is based from the wcn3990 with minimal power numbers. So here in this patch
The major difference between wcn3990 and wcn3998 is only power numbers. where
as init process and fw download is same with wcn3990.So we add new compatible
for wcn3998.
Removed the Reading of regulator values from dts.
Added new compatible for wcn3998.
Changed in V3:
updated to latest code base.
Harish Bandi (2):
Bluetooth: hci_qca: Added support for wcn3998
dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
wcn3998
.../devicetree/bindings/net/qualcomm-bluetooth.txt | 15 ++++++++
drivers/bluetooth/btqca.c | 4 +--
drivers/bluetooth/btqca.h | 3 +-
drivers/bluetooth/hci_qca.c | 40 ++++++++++++++--------
4 files changed, 45 insertions(+), 17 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Added new compatible for wcn3998 and corresponding voltage
and current values to wcn3998 compatible.
Changed driver code to support wcn3998
Signed-off-by: Harish Bandi <[email protected]>
---
changes in v3:
- updated to latest code base.
---
drivers/bluetooth/btqca.c | 4 ++--
drivers/bluetooth/btqca.h | 3 ++-
drivers/bluetooth/hci_qca.c | 40 ++++++++++++++++++++++++++--------------
3 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 6122685..70cab13 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -344,7 +344,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
/* Download rampatch file */
config.type = TLV_TYPE_PATCH;
- if (soc_type == QCA_WCN3990) {
+ if (soc_type >= QCA_WCN3990) {
/* Firmware files to download are based on ROM version.
* ROM version is derived from last two bytes of soc_ver.
*/
@@ -365,7 +365,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
/* Download NVM configuration */
config.type = TLV_TYPE_NVM;
- if (soc_type == QCA_WCN3990)
+ if (soc_type >= QCA_WCN3990)
snprintf(config.fwname, sizeof(config.fwname),
"qca/crnv%02x.bin", rom_ver);
else
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index c72c56e..f03d96e 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -132,7 +132,8 @@ enum qca_btsoc_type {
QCA_INVALID = -1,
QCA_AR3002,
QCA_ROME,
- QCA_WCN3990
+ QCA_WCN3990,
+ QCA_WCN3998
};
#if IS_ENABLED(CONFIG_BT_QCA)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 4ea995d..1221f5a 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -521,7 +521,7 @@ static int qca_open(struct hci_uart *hu)
if (hu->serdev) {
qcadev = serdev_device_get_drvdata(hu->serdev);
- if (qcadev->btsoc_type != QCA_WCN3990) {
+ if (qcadev->btsoc_type == QCA_ROME) {
gpiod_set_value_cansleep(qcadev->bt_en, 1);
} else {
hu->init_speed = qcadev->init_speed;
@@ -627,7 +627,7 @@ static int qca_close(struct hci_uart *hu)
if (hu->serdev) {
qcadev = serdev_device_get_drvdata(hu->serdev);
- if (qcadev->btsoc_type == QCA_WCN3990)
+ if (qcadev->btsoc_type >= QCA_WCN3990)
qca_power_shutdown(hu);
else
gpiod_set_value_cansleep(qcadev->bt_en, 0);
@@ -1008,7 +1008,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
/* Give the controller time to process the request */
- if (qca_soc_type(hu) == QCA_WCN3990)
+ if (qca_soc_type(hu) >= QCA_WCN3990)
msleep(10);
else
msleep(300);
@@ -1084,7 +1084,7 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
static int qca_check_speeds(struct hci_uart *hu)
{
- if (qca_soc_type(hu) == QCA_WCN3990) {
+ if (qca_soc_type(hu) >= QCA_WCN3990) {
if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
!qca_get_speed(hu, QCA_OPER_SPEED))
return -EINVAL;
@@ -1116,7 +1116,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
/* Disable flow control for wcn3990 to deassert RTS while
* changing the baudrate of chip and host.
*/
- if (soc_type == QCA_WCN3990)
+ if (soc_type >= QCA_WCN3990)
hci_uart_set_flow_control(hu, true);
qca_baudrate = qca_get_baudrate_value(speed);
@@ -1128,7 +1128,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
host_set_baudrate(hu, speed);
error:
- if (soc_type == QCA_WCN3990)
+ if (soc_type >= QCA_WCN3990)
hci_uart_set_flow_control(hu, false);
}
@@ -1201,7 +1201,7 @@ static int qca_setup(struct hci_uart *hu)
/* Patch downloading has to be done without IBS mode */
clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
- if (soc_type == QCA_WCN3990) {
+ if (soc_type >= QCA_WCN3990) {
bt_dev_info(hdev, "setting up wcn3990");
/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
@@ -1232,7 +1232,7 @@ static int qca_setup(struct hci_uart *hu)
qca_baudrate = qca_get_baudrate_value(speed);
}
- if (soc_type != QCA_WCN3990) {
+ if (soc_type == QCA_ROME) {
/* Get QCA version information */
ret = qca_read_soc_version(hdev, &soc_ver);
if (ret)
@@ -1257,7 +1257,7 @@ static int qca_setup(struct hci_uart *hu)
}
/* Setup bdaddr */
- if (soc_type == QCA_WCN3990)
+ if (soc_type >= QCA_WCN3990)
hu->hdev->set_bdaddr = qca_set_bdaddr;
else
hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
@@ -1280,7 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
.dequeue = qca_dequeue,
};
-static const struct qca_vreg_data qca_soc_data = {
+static const struct qca_vreg_data qca_soc_data_wcn3990 = {
.soc_type = QCA_WCN3990,
.vregs = (struct qca_vreg []) {
{ "vddio", 1800000, 1900000, 15000 },
@@ -1291,6 +1291,17 @@ static int qca_setup(struct hci_uart *hu)
.num_vregs = 4,
};
+static const struct qca_vreg_data qca_soc_data_wcn3998 = {
+ .soc_type = QCA_WCN3998,
+ .vregs = (struct qca_vreg []) {
+ { "vddio", 1800000, 1900000, 10000 },
+ { "vddxo", 1800000, 1900000, 80000 },
+ { "vddrf", 1300000, 1352000, 300000 },
+ { "vddch0", 3300000, 3300000, 450000 },
+ },
+ .num_vregs = 4,
+};
+
static void qca_power_shutdown(struct hci_uart *hu)
{
struct qca_data *qca = hu->priv;
@@ -1424,8 +1435,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
qcadev->serdev_hu.serdev = serdev;
data = of_device_get_match_data(&serdev->dev);
serdev_device_set_drvdata(serdev, qcadev);
- if (data && data->soc_type == QCA_WCN3990) {
- qcadev->btsoc_type = QCA_WCN3990;
+ if (data && data->soc_type >= QCA_WCN3990) {
+ qcadev->btsoc_type = data->soc_type;
qcadev->bt_power = devm_kzalloc(&serdev->dev,
sizeof(struct qca_power),
GFP_KERNEL);
@@ -1489,7 +1500,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
{
struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
- if (qcadev->btsoc_type == QCA_WCN3990)
+ if (qcadev->btsoc_type >= QCA_WCN3990)
qca_power_shutdown(&qcadev->serdev_hu);
else
clk_disable_unprepare(qcadev->susclk);
@@ -1499,7 +1510,8 @@ static void qca_serdev_remove(struct serdev_device *serdev)
static const struct of_device_id qca_bluetooth_of_match[] = {
{ .compatible = "qcom,qca6174-bt" },
- { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data},
+ { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
+ { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
This patch enables regulators for the Qualcomm Bluetooth wcn3998
controller.
Signed-off-by: Harish Bandi <[email protected]>
---
changes in v3:
- updated to latest code base.
---
.../devicetree/bindings/net/qualcomm-bluetooth.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
index 824c0e2..1221535 100644
--- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -53,3 +53,18 @@ serial@898000 {
max-speed = <3200000>;
};
};
+
+&blsp1_uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&blsp1_uart3_default>;
+ status = "okay";
+
+ bluetooth: wcn3998-bt {
+ compatible = "qcom,wcn3998-bt";
+ vddio-supply = <&vreg_l6_1p8>;
+ vddxo-supply = <&vreg_l5_1p8>;
+ vddrf-supply = <&vreg_s5_1p35>;
+ vddch0-supply = <&vdd_ch0_3p3>;
+ max-speed = <3200000>;
+ };
+};
\ No newline at end of file
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Harish,
On Tue, Mar 12, 2019 at 05:52:58PM +0530, Harish Bandi wrote:
> Added new compatible for wcn3998 and corresponding voltage
> and current values to wcn3998 compatible.
> Changed driver code to support wcn3998
>
> Signed-off-by: Harish Bandi <[email protected]>
> ---
> changes in v3:
> - updated to latest code base.
This is not useful, for future versions please describe what changed
(e.g. 'specify regulator constraints in the driver instead of the DT')
> ---
> drivers/bluetooth/btqca.c | 4 ++--
> drivers/bluetooth/btqca.h | 3 ++-
> drivers/bluetooth/hci_qca.c | 40 ++++++++++++++++++++++++++--------------
> 3 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 6122685..70cab13 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -344,7 +344,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>
> /* Download rampatch file */
> config.type = TLV_TYPE_PATCH;
> - if (soc_type == QCA_WCN3990) {
> + if (soc_type >= QCA_WCN3990) {
That works, but isn't super-clear and might need to be adapted when
future non-WCN399x controllers are added.
Some possible alternatives:
- is_wcn399x(soc_type)
- have a family (Rome, Cherokee (IIRC this name was used for WCN3990))
and a chip id (QCA6174, WCN3990, WCN3998, ...)
> /* Firmware files to download are based on ROM version.
> * ROM version is derived from last two bytes of soc_ver.
> */
> @@ -365,7 +365,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>
> /* Download NVM configuration */
> config.type = TLV_TYPE_NVM;
> - if (soc_type == QCA_WCN3990)
> + if (soc_type >= QCA_WCN3990)
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/crnv%02x.bin", rom_ver);
> else
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index c72c56e..f03d96e 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -132,7 +132,8 @@ enum qca_btsoc_type {
> QCA_INVALID = -1,
> QCA_AR3002,
> QCA_ROME,
> - QCA_WCN3990
> + QCA_WCN3990,
> + QCA_WCN3998
nit: if you add a comma after the last value the line doesn't need to
be changed when a new type is added in the future.
Is 'WCN3998' specific enough? You mentioned earlier that there are
multiple WCN3998 variants with different requirements for regulator
voltages/max currents. Which names does Qualcomm use to distinguish
between them (e.g. WCN3998-A, WCN3998-B, ...)?
Thanks
Matthias
+DT folks
Please add them in future versions (script/scripts/get_maintainer.pl
<patches> should have listed them)
On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
> This patch enables regulators for the Qualcomm Bluetooth wcn3998
> controller.
No, it doesn't.
The next version should probably say something like "Add compatible
string for the Qualcomm WCN3998 Bluetooth controller.
Is there any particular reason why QCA drivers folks use 'wcn' instead
of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
consistently use the uppercase name in comments and documentation (and
log messages?).
> Signed-off-by: Harish Bandi <[email protected]>
> ---
> changes in v3:
> - updated to latest code base.
This comment is useless, please describe what changed wrt the previous
version.
> ---
> .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> index 824c0e2..1221535 100644
> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -53,3 +53,18 @@ serial@898000 {
> max-speed = <3200000>;
> };
> };
> +
> +&blsp1_uart3 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&blsp1_uart3_default>;
> + status = "okay";
> +
> + bluetooth: wcn3998-bt {
> + compatible = "qcom,wcn3998-bt";
> + vddio-supply = <&vreg_l6_1p8>;
> + vddxo-supply = <&vreg_l5_1p8>;
> + vddrf-supply = <&vreg_s5_1p35>;
> + vddch0-supply = <&vdd_ch0_3p3>;
> + max-speed = <3200000>;
> + };
> +};
> \ No newline at end of file
I think the example isn't really needed since it's essentially the
same as the one for 'qcom,wcn3990-bt'.
But the important part is missing: add the new compatible string under
´Required properties´. You also want to update the documentation that
mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
that other possible WCN399x chips would be similar).
You mentioned in an earlier version of the series that there are
multiple WCN3998 variants with different requirements for
voltage/current. This seems to suggests that multiple compatible
strings are needed to distinguish between them.
Thanks
Matthias
Hi Matthias,
On 2019-03-12 21:59, Matthias Kaehlcke wrote:
> Hi Harish,
>
> On Tue, Mar 12, 2019 at 05:52:58PM +0530, Harish Bandi wrote:
>> Added new compatible for wcn3998 and corresponding voltage
>> and current values to wcn3998 compatible.
>> Changed driver code to support wcn3998
>>
>> Signed-off-by: Harish Bandi <[email protected]>
>> ---
>> changes in v3:
>> - updated to latest code base.
>
> This is not useful, for future versions please describe what changed
> (e.g. 'specify regulator constraints in the driver instead of the DT')
>
[Harish] -- added details in v2, and v3 uploaded just to rebase on tip
of bluetooth-next
for better understanding of code in review. From new patch onwards will
add all patch
version changes and add proper description.
>> ---
>> drivers/bluetooth/btqca.c | 4 ++--
>> drivers/bluetooth/btqca.h | 3 ++-
>> drivers/bluetooth/hci_qca.c | 40
>> ++++++++++++++++++++++++++--------------
>> 3 files changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index 6122685..70cab13 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -344,7 +344,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
>> baudrate,
>>
>> /* Download rampatch file */
>> config.type = TLV_TYPE_PATCH;
>> - if (soc_type == QCA_WCN3990) {
>> + if (soc_type >= QCA_WCN3990) {
>
> That works, but isn't super-clear and might need to be adapted when
> future non-WCN399x controllers are added.
>
> Some possible alternatives:
>
> - is_wcn399x(soc_type)
> - have a family (Rome, Cherokee (IIRC this name was used for WCN3990))
> and a chip id (QCA6174, WCN3990, WCN3998, ...)
>
[Harish] -- Will change like is_wcn399x(soc_type) and come up with new
patch
>> /* Firmware files to download are based on ROM version.
>> * ROM version is derived from last two bytes of soc_ver.
>> */
>> @@ -365,7 +365,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
>> baudrate,
>>
>> /* Download NVM configuration */
>> config.type = TLV_TYPE_NVM;
>> - if (soc_type == QCA_WCN3990)
>> + if (soc_type >= QCA_WCN3990)
>> snprintf(config.fwname, sizeof(config.fwname),
>> "qca/crnv%02x.bin", rom_ver);
>> else
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index c72c56e..f03d96e 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -132,7 +132,8 @@ enum qca_btsoc_type {
>> QCA_INVALID = -1,
>> QCA_AR3002,
>> QCA_ROME,
>> - QCA_WCN3990
>> + QCA_WCN3990,
>> + QCA_WCN3998
>
> nit: if you add a comma after the last value the line doesn't need to
> be changed when a new type is added in the future.
>
[Harish] -- will take care in new patch
> Is 'WCN3998' specific enough? You mentioned earlier that there are
> multiple WCN3998 variants with different requirements for regulator
> voltages/max currents. Which names does Qualcomm use to distinguish
> between them (e.g. WCN3998-A, WCN3998-B, ...)?
[Harish] -- for now we want to add WCN3998 support only, What i mean to
say in my earlier
explanation that. WCN3990 is base variant and on top of that we have
variants like WCN3990,
WCN3998 and WCN3998-0,WCN3998-1 like that..
>
> Thanks
>
> Matthias
Thanks,
Harish
Hi Matthias,
On 2019-03-12 22:29, Matthias Kaehlcke wrote:
> +DT folks
>
> Please add them in future versions (script/scripts/get_maintainer.pl
> <patches> should have listed them)
[Harish] -- Will add them in new version of patches.
>
> On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
>> This patch enables regulators for the Qualcomm Bluetooth wcn3998
>> controller.
>
> No, it doesn't.
>
> The next version should probably say something like "Add compatible
> string for the Qualcomm WCN3998 Bluetooth controller.
>
[Harish] -- From new patch onwards will add all patch
version changes and add proper description.
> Is there any particular reason why QCA drivers folks use 'wcn' instead
> of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
> consistently use the uppercase name in comments and documentation (and
> log messages?).
>
[Harish] -- I think in DT we need to have small case like wcn, i think
that is the reason it started using in code, comments and dt
documentation.
>> Signed-off-by: Harish Bandi <[email protected]>
>> ---
>> changes in v3:
>> - updated to latest code base.
>
> This comment is useless, please describe what changed wrt the previous
> version.
[Harish] -- added details in v2, and v3 uploaded just to rebase on tip
of bluetooth-next
for better understanding of code in review. From new patch onwards will
add all patch
version changes and add proper description.
>
>> ---
>> .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15
>> +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> index 824c0e2..1221535 100644
>> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> @@ -53,3 +53,18 @@ serial@898000 {
>> max-speed = <3200000>;
>> };
>> };
>> +
>> +&blsp1_uart3 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&blsp1_uart3_default>;
>> + status = "okay";
>> +
>> + bluetooth: wcn3998-bt {
>> + compatible = "qcom,wcn3998-bt";
>> + vddio-supply = <&vreg_l6_1p8>;
>> + vddxo-supply = <&vreg_l5_1p8>;
>> + vddrf-supply = <&vreg_s5_1p35>;
>> + vddch0-supply = <&vdd_ch0_3p3>;
>> + max-speed = <3200000>;
>> + };
>> +};
>> \ No newline at end of file
>
> I think the example isn't really needed since it's essentially the
> same as the one for 'qcom,wcn3990-bt'.
>
> But the important part is missing: add the new compatible string under
> ´Required properties´. You also want to update the documentation that
> mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
> that other possible WCN399x chips would be similar).
>
[Harish] -- Will check the DT properties, documentation and update
accordingly in new patch.
> You mentioned in an earlier version of the series that there are
> multiple WCN3998 variants with different requirements for
> voltage/current. This seems to suggests that multiple compatible
> strings are needed to distinguish between them.
>
[Harish] -- for now we want to add WCN3998 support only, What i mean to
say in my earlier
explanation that. WCN3990 is base variant and on top of that we have
variants like WCN3990,
WCN3998 and WCN3998-0,WCN3998-1 like that..
So I think wcn399x would make sense for this series.
> Thanks
>
> Matthias
Thanks,
Harish
Hi Harish,
On Wed, Mar 13, 2019 at 12:00:06PM +0530, [email protected] wrote:
> Hi Matthias,
>
>
> On 2019-03-12 22:29, Matthias Kaehlcke wrote:
> > +DT folks
> >
> > Please add them in future versions (script/scripts/get_maintainer.pl
> > <patches> should have listed them)
>
> [Harish] -- Will add them in new version of patches.
> >
> > On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
> > > This patch enables regulators for the Qualcomm Bluetooth wcn3998
> > > controller.
> >
> > No, it doesn't.
> >
> > The next version should probably say something like "Add compatible
> > string for the Qualcomm WCN3998 Bluetooth controller.
> >
> [Harish] -- From new patch onwards will add all patch
> version changes and add proper description.
>
> > Is there any particular reason why QCA drivers folks use 'wcn' instead
> > of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
> > consistently use the uppercase name in comments and documentation (and
> > log messages?).
> >
> [Harish] -- I think in DT we need to have small case like wcn,
agreed
> i think that is the reason it started using in code, comments and dt
> documentation.
AFAIK there are no hard rules for everything, my suggestion would be:
- use WCN399x
- for general comments/documentation
- commit messages
- in DT context wcn3998-n seems ok
- use wcn399x
- for function and variable names
- for compatible strings
For logging: whatever, just be consistent.
> > > Signed-off-by: Harish Bandi <[email protected]>
> > > ---
> > > changes in v3:
> > > - updated to latest code base.
> >
> > This comment is useless, please describe what changed wrt the previous
> > version.
> [Harish] -- added details in v2, and v3 uploaded just to rebase on tip of
> bluetooth-next
> for better understanding of code in review. From new patch onwards will add
> all patch
> version changes and add proper description.
> >
> > > ---
> > > .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15
> > > +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > index 824c0e2..1221535 100644
> > > --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > @@ -53,3 +53,18 @@ serial@898000 {
> > > max-speed = <3200000>;
> > > };
> > > };
> > > +
> > > +&blsp1_uart3 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&blsp1_uart3_default>;
> > > + status = "okay";
> > > +
> > > + bluetooth: wcn3998-bt {
> > > + compatible = "qcom,wcn3998-bt";
> > > + vddio-supply = <&vreg_l6_1p8>;
> > > + vddxo-supply = <&vreg_l5_1p8>;
> > > + vddrf-supply = <&vreg_s5_1p35>;
> > > + vddch0-supply = <&vdd_ch0_3p3>;
> > > + max-speed = <3200000>;
> > > + };
> > > +};
> > > \ No newline at end of file
> >
> > I think the example isn't really needed since it's essentially the
> > same as the one for 'qcom,wcn3990-bt'.
> >
> > But the important part is missing: add the new compatible string under
> > ´Required properties´. You also want to update the documentation that
> > mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
> > that other possible WCN399x chips would be similar).
> >
> [Harish] -- Will check the DT properties, documentation and update
> accordingly in new patch.
>
> > You mentioned in an earlier version of the series that there are
> > multiple WCN3998 variants with different requirements for
> > voltage/current. This seems to suggests that multiple compatible
> > strings are needed to distinguish between them.
> >
> [Harish] -- for now we want to add WCN3998 support only, What i mean to say
> in my earlier
> explanation that. WCN3990 is base variant and on top of that we have
> variants like WCN3990,
> WCN3998 and WCN3998-0,WCN3998-1 like that..
> So I think wcn399x would make sense for this series.
If the variants have relevant differences between them (like different
regulator requirements) I think you want unique names, rather than
'wcn399x' (I was referring to comments/documentation with this
string).
If there are variants wouldn't your first 'wcn3998' already be a
'wcn3998-n'? If 'wcn3998' without suffix is used I think it needs to
be valid for all 'wcn3998-n' variants (it might be less
power-efficient though than using the variant specific compatible
string), otherwise things get confusing (a 'wcn3998-2' isn't a
'wcn3998'?)
Thanks
Matthias
Hi Matthias,
On 2019-03-15 00:26, Matthias Kaehlcke wrote:
> Hi Harish,
>
> On Wed, Mar 13, 2019 at 12:00:06PM +0530, [email protected]
> wrote:
>> Hi Matthias,
>>
>>
>> On 2019-03-12 22:29, Matthias Kaehlcke wrote:
>> > +DT folks
>> >
>> > Please add them in future versions (script/scripts/get_maintainer.pl
>> > <patches> should have listed them)
>>
>> [Harish] -- Will add them in new version of patches.
>> >
>> > On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
>> > > This patch enables regulators for the Qualcomm Bluetooth wcn3998
>> > > controller.
>> >
>> > No, it doesn't.
>> >
>> > The next version should probably say something like "Add compatible
>> > string for the Qualcomm WCN3998 Bluetooth controller.
>> >
>> [Harish] -- From new patch onwards will add all patch
>> version changes and add proper description.
>>
>> > Is there any particular reason why QCA drivers folks use 'wcn' instead
>> > of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
>> > consistently use the uppercase name in comments and documentation (and
>> > log messages?).
>> >
>> [Harish] -- I think in DT we need to have small case like wcn,
>
> agreed
>
>> i think that is the reason it started using in code, comments and dt
>> documentation.
>
> AFAIK there are no hard rules for everything, my suggestion would be:
>
> - use WCN399x
> - for general comments/documentation
> - commit messages
> - in DT context wcn3998-n seems ok
> - use wcn399x
> - for function and variable names
> - for compatible strings
>
> For logging: whatever, just be consistent.
[Harish] --For Commit messages and all will try to follow change it new
patch
>
>> > > Signed-off-by: Harish Bandi <[email protected]>
>> > > ---
>> > > changes in v3:
>> > > - updated to latest code base.
>> >
>> > This comment is useless, please describe what changed wrt the previous
>> > version.
>> [Harish] -- added details in v2, and v3 uploaded just to rebase on
>> tip of
>> bluetooth-next
>> for better understanding of code in review. From new patch onwards
>> will add
>> all patch
>> version changes and add proper description.
>> >
>> > > ---
>> > > .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15
>> > > +++++++++++++++
>> > > 1 file changed, 15 insertions(+)
>> > >
>> > > diff --git
>> > > a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > index 824c0e2..1221535 100644
>> > > --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > @@ -53,3 +53,18 @@ serial@898000 {
>> > > max-speed = <3200000>;
>> > > };
>> > > };
>> > > +
>> > > +&blsp1_uart3 {
>> > > + pinctrl-names = "default";
>> > > + pinctrl-0 = <&blsp1_uart3_default>;
>> > > + status = "okay";
>> > > +
>> > > + bluetooth: wcn3998-bt {
>> > > + compatible = "qcom,wcn3998-bt";
>> > > + vddio-supply = <&vreg_l6_1p8>;
>> > > + vddxo-supply = <&vreg_l5_1p8>;
>> > > + vddrf-supply = <&vreg_s5_1p35>;
>> > > + vddch0-supply = <&vdd_ch0_3p3>;
>> > > + max-speed = <3200000>;
>> > > + };
>> > > +};
>> > > \ No newline at end of file
>> >
>> > I think the example isn't really needed since it's essentially the
>> > same as the one for 'qcom,wcn3990-bt'.
>> >
>> > But the important part is missing: add the new compatible string under
>> > ´Required properties´. You also want to update the documentation that
>> > mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
>> > that other possible WCN399x chips would be similar).
>> >
>> [Harish] -- Will check the DT properties, documentation and update
>> accordingly in new patch.
>>
>> > You mentioned in an earlier version of the series that there are
>> > multiple WCN3998 variants with different requirements for
>> > voltage/current. This seems to suggests that multiple compatible
>> > strings are needed to distinguish between them.
>> >
>> [Harish] -- for now we want to add WCN3998 support only, What i mean
>> to say
>> in my earlier
>> explanation that. WCN3990 is base variant and on top of that we have
>> variants like WCN3990,
>> WCN3998 and WCN3998-0,WCN3998-1 like that..
>> So I think wcn399x would make sense for this series.
>
> If the variants have relevant differences between them (like different
> regulator requirements) I think you want unique names, rather than
> 'wcn399x' (I was referring to comments/documentation with this
> string).
>
> If there are variants wouldn't your first 'wcn3998' already be a
> 'wcn3998-n'? If 'wcn3998' without suffix is used I think it needs to
> be valid for all 'wcn3998-n' variants (it might be less
> power-efficient though than using the variant specific compatible
> string), otherwise things get confusing (a 'wcn3998-2' isn't a
> 'wcn3998'?)
>
[Harish] -- for now WCN3990 and WCN3998 only, also wcn3998-2 and
wcn3998 are same.
So for now we are going to have only WCN3990 and WCN3998 for DT.
> Thanks
>
> Matthias
On Mon, Mar 25, 2019 at 05:00:31PM +0530, [email protected] wrote:
> Hi Matthias,
>
> On 2019-03-15 00:26, Matthias Kaehlcke wrote:
> > Hi Harish,
> >
> > On Wed, Mar 13, 2019 at 12:00:06PM +0530, [email protected] wrote:
> > > Hi Matthias,
> > >
> > >
> > > On 2019-03-12 22:29, Matthias Kaehlcke wrote:
> > > > +DT folks
> > > >
> > > > Please add them in future versions (script/scripts/get_maintainer.pl
> > > > <patches> should have listed them)
> > >
> > > [Harish] -- Will add them in new version of patches.
> > > >
> > > > On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
> > > > > This patch enables regulators for the Qualcomm Bluetooth wcn3998
> > > > > controller.
> > > >
> > > > No, it doesn't.
> > > >
> > > > The next version should probably say something like "Add compatible
> > > > string for the Qualcomm WCN3998 Bluetooth controller.
> > > >
> > > [Harish] -- From new patch onwards will add all patch
> > > version changes and add proper description.
> > >
> > > > Is there any particular reason why QCA drivers folks use 'wcn' instead
> > > > of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
> > > > consistently use the uppercase name in comments and documentation (and
> > > > log messages?).
> > > >
> > > [Harish] -- I think in DT we need to have small case like wcn,
> >
> > agreed
> >
> > > i think that is the reason it started using in code, comments and dt
> > > documentation.
> >
> > AFAIK there are no hard rules for everything, my suggestion would be:
> >
> > - use WCN399x
> > - for general comments/documentation
> > - commit messages
> > - in DT context wcn3998-n seems ok
> > - use wcn399x
> > - for function and variable names
> > - for compatible strings
> >
> > For logging: whatever, just be consistent.
>
> [Harish] --For Commit messages and all will try to follow change it new
> patch
>
> >
> > > > > Signed-off-by: Harish Bandi <[email protected]>
> > > > > ---
> > > > > changes in v3:
> > > > > - updated to latest code base.
> > > >
> > > > This comment is useless, please describe what changed wrt the previous
> > > > version.
> > > [Harish] -- added details in v2, and v3 uploaded just to rebase on
> > > tip of
> > > bluetooth-next
> > > for better understanding of code in review. From new patch onwards
> > > will add
> > > all patch
> > > version changes and add proper description.
> > > >
> > > > > ---
> > > > > .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15
> > > > > +++++++++++++++
> > > > > 1 file changed, 15 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > > > b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > > > index 824c0e2..1221535 100644
> > > > > --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > > > +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> > > > > @@ -53,3 +53,18 @@ serial@898000 {
> > > > > max-speed = <3200000>;
> > > > > };
> > > > > };
> > > > > +
> > > > > +&blsp1_uart3 {
> > > > > + pinctrl-names = "default";
> > > > > + pinctrl-0 = <&blsp1_uart3_default>;
> > > > > + status = "okay";
> > > > > +
> > > > > + bluetooth: wcn3998-bt {
> > > > > + compatible = "qcom,wcn3998-bt";
> > > > > + vddio-supply = <&vreg_l6_1p8>;
> > > > > + vddxo-supply = <&vreg_l5_1p8>;
> > > > > + vddrf-supply = <&vreg_s5_1p35>;
> > > > > + vddch0-supply = <&vdd_ch0_3p3>;
> > > > > + max-speed = <3200000>;
> > > > > + };
> > > > > +};
> > > > > \ No newline at end of file
> > > >
> > > > I think the example isn't really needed since it's essentially the
> > > > same as the one for 'qcom,wcn3990-bt'.
> > > >
> > > > But the important part is missing: add the new compatible string under
> > > > ´Required properties´. You also want to update the documentation that
> > > > mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
> > > > that other possible WCN399x chips would be similar).
> > > >
> > > [Harish] -- Will check the DT properties, documentation and update
> > > accordingly in new patch.
> > >
> > > > You mentioned in an earlier version of the series that there are
> > > > multiple WCN3998 variants with different requirements for
> > > > voltage/current. This seems to suggests that multiple compatible
> > > > strings are needed to distinguish between them.
> > > >
> > > [Harish] -- for now we want to add WCN3998 support only, What i mean
> > > to say
> > > in my earlier
> > > explanation that. WCN3990 is base variant and on top of that we have
> > > variants like WCN3990,
> > > WCN3998 and WCN3998-0,WCN3998-1 like that..
> > > So I think wcn399x would make sense for this series.
> >
> > If the variants have relevant differences between them (like different
> > regulator requirements) I think you want unique names, rather than
> > 'wcn399x' (I was referring to comments/documentation with this
> > string).
> >
> > If there are variants wouldn't your first 'wcn3998' already be a
> > 'wcn3998-n'? If 'wcn3998' without suffix is used I think it needs to
> > be valid for all 'wcn3998-n' variants (it might be less
> > power-efficient though than using the variant specific compatible
> > string), otherwise things get confusing (a 'wcn3998-2' isn't a
> > 'wcn3998'?)
> >
>
>
> [Harish] -- for now WCN3990 and WCN3998 only, also wcn3998-2 and wcn3998
> are same.
> So for now we are going to have only WCN3990 and WCN3998 for DT.
Since you brought up the different power requirements initially:
are your reasonably sure that future wcn3998-3, wcn3998-4, ... can
operate with the generic wcn3998 power settings?
Hi Matthias,
On 2019-03-27 05:24, Matthias Kaehlcke wrote:
> On Mon, Mar 25, 2019 at 05:00:31PM +0530, [email protected]
> wrote:
>> Hi Matthias,
>>
>> On 2019-03-15 00:26, Matthias Kaehlcke wrote:
>> > Hi Harish,
>> >
>> > On Wed, Mar 13, 2019 at 12:00:06PM +0530, [email protected] wrote:
>> > > Hi Matthias,
>> > >
>> > >
>> > > On 2019-03-12 22:29, Matthias Kaehlcke wrote:
>> > > > +DT folks
>> > > >
>> > > > Please add them in future versions (script/scripts/get_maintainer.pl
>> > > > <patches> should have listed them)
>> > >
>> > > [Harish] -- Will add them in new version of patches.
>> > > >
>> > > > On Tue, Mar 12, 2019 at 05:52:59PM +0530, Harish Bandi wrote:
>> > > > > This patch enables regulators for the Qualcomm Bluetooth wcn3998
>> > > > > controller.
>> > > >
>> > > > No, it doesn't.
>> > > >
>> > > > The next version should probably say something like "Add compatible
>> > > > string for the Qualcomm WCN3998 Bluetooth controller.
>> > > >
>> > > [Harish] -- From new patch onwards will add all patch
>> > > version changes and add proper description.
>> > >
>> > > > Is there any particular reason why QCA drivers folks use 'wcn' instead
>> > > > of 'WCN'? The QCA documentations calls it WCN399x, so I'd suggest to
>> > > > consistently use the uppercase name in comments and documentation (and
>> > > > log messages?).
>> > > >
>> > > [Harish] -- I think in DT we need to have small case like wcn,
>> >
>> > agreed
>> >
>> > > i think that is the reason it started using in code, comments and dt
>> > > documentation.
>> >
>> > AFAIK there are no hard rules for everything, my suggestion would be:
>> >
>> > - use WCN399x
>> > - for general comments/documentation
>> > - commit messages
>> > - in DT context wcn3998-n seems ok
>> > - use wcn399x
>> > - for function and variable names
>> > - for compatible strings
>> >
>> > For logging: whatever, just be consistent.
>>
>> [Harish] --For Commit messages and all will try to follow change it
>> new
>> patch
>>
>> >
>> > > > > Signed-off-by: Harish Bandi <[email protected]>
>> > > > > ---
>> > > > > changes in v3:
>> > > > > - updated to latest code base.
>> > > >
>> > > > This comment is useless, please describe what changed wrt the previous
>> > > > version.
>> > > [Harish] -- added details in v2, and v3 uploaded just to rebase on
>> > > tip of
>> > > bluetooth-next
>> > > for better understanding of code in review. From new patch onwards
>> > > will add
>> > > all patch
>> > > version changes and add proper description.
>> > > >
>> > > > > ---
>> > > > > .../devicetree/bindings/net/qualcomm-bluetooth.txt | 15
>> > > > > +++++++++++++++
>> > > > > 1 file changed, 15 insertions(+)
>> > > > >
>> > > > > diff --git
>> > > > > a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > > > b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > > > index 824c0e2..1221535 100644
>> > > > > --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > > > +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> > > > > @@ -53,3 +53,18 @@ serial@898000 {
>> > > > > max-speed = <3200000>;
>> > > > > };
>> > > > > };
>> > > > > +
>> > > > > +&blsp1_uart3 {
>> > > > > + pinctrl-names = "default";
>> > > > > + pinctrl-0 = <&blsp1_uart3_default>;
>> > > > > + status = "okay";
>> > > > > +
>> > > > > + bluetooth: wcn3998-bt {
>> > > > > + compatible = "qcom,wcn3998-bt";
>> > > > > + vddio-supply = <&vreg_l6_1p8>;
>> > > > > + vddxo-supply = <&vreg_l5_1p8>;
>> > > > > + vddrf-supply = <&vreg_s5_1p35>;
>> > > > > + vddch0-supply = <&vdd_ch0_3p3>;
>> > > > > + max-speed = <3200000>;
>> > > > > + };
>> > > > > +};
>> > > > > \ No newline at end of file
>> > > >
>> > > > I think the example isn't really needed since it's essentially the
>> > > > same as the one for 'qcom,wcn3990-bt'.
>> > > >
>> > > > But the important part is missing: add the new compatible string under
>> > > > ´Required properties´. You also want to update the documentation that
>> > > > mentiones 'qcom,wcn3990-bt' to 'qcom,wcn399x-bt' (assuming for now
>> > > > that other possible WCN399x chips would be similar).
>> > > >
>> > > [Harish] -- Will check the DT properties, documentation and update
>> > > accordingly in new patch.
>> > >
>> > > > You mentioned in an earlier version of the series that there are
>> > > > multiple WCN3998 variants with different requirements for
>> > > > voltage/current. This seems to suggests that multiple compatible
>> > > > strings are needed to distinguish between them.
>> > > >
>> > > [Harish] -- for now we want to add WCN3998 support only, What i mean
>> > > to say
>> > > in my earlier
>> > > explanation that. WCN3990 is base variant and on top of that we have
>> > > variants like WCN3990,
>> > > WCN3998 and WCN3998-0,WCN3998-1 like that..
>> > > So I think wcn399x would make sense for this series.
>> >
>> > If the variants have relevant differences between them (like different
>> > regulator requirements) I think you want unique names, rather than
>> > 'wcn399x' (I was referring to comments/documentation with this
>> > string).
>> >
>> > If there are variants wouldn't your first 'wcn3998' already be a
>> > 'wcn3998-n'? If 'wcn3998' without suffix is used I think it needs to
>> > be valid for all 'wcn3998-n' variants (it might be less
>> > power-efficient though than using the variant specific compatible
>> > string), otherwise things get confusing (a 'wcn3998-2' isn't a
>> > 'wcn3998'?)
>> >
>>
>>
>> [Harish] -- for now WCN3990 and WCN3998 only, also wcn3998-2 and
>> wcn3998
>> are same.
>> So for now we are going to have only WCN3990 and WCN3998 for DT.
>
> Since you brought up the different power requirements initially:
>
> are your reasonably sure that future wcn3998-3, wcn3998-4, ... can
> operate with the generic wcn3998 power settings?
[Harish] - yes with power settings of wcn3998, wcn3998-3 and wcn3998-4
... also will work.
On Wed, Mar 27, 2019 at 03:53:58PM +0530, Harish Bandi wrote:
> Hi Matthias,
>
> > > > > > You mentioned in an earlier version of the series that there are
> > > > > > multiple WCN3998 variants with different requirements for
> > > > > > voltage/current. This seems to suggests that multiple compatible
> > > > > > strings are needed to distinguish between them.
> > > > > >
> > > > > [Harish] -- for now we want to add WCN3998 support only, What i mean
> > > > > to say
> > > > > in my earlier
> > > > > explanation that. WCN3990 is base variant and on top of that we have
> > > > > variants like WCN3990,
> > > > > WCN3998 and WCN3998-0,WCN3998-1 like that..
> > > > > So I think wcn399x would make sense for this series.
> > > >
> > > > If the variants have relevant differences between them (like different
> > > > regulator requirements) I think you want unique names, rather than
> > > > 'wcn399x' (I was referring to comments/documentation with this
> > > > string).
> > > >
> > > > If there are variants wouldn't your first 'wcn3998' already be a
> > > > 'wcn3998-n'? If 'wcn3998' without suffix is used I think it needs to
> > > > be valid for all 'wcn3998-n' variants (it might be less
> > > > power-efficient though than using the variant specific compatible
> > > > string), otherwise things get confusing (a 'wcn3998-2' isn't a
> > > > 'wcn3998'?)
> > > >
> > >
> > >
> > > [Harish] -- for now WCN3990 and WCN3998 only, also wcn3998-2 and
> > > wcn3998
> > > are same.
> > > So for now we are going to have only WCN3990 and WCN3998 for DT.
> >
> > Since you brought up the different power requirements initially:
> >
> > are your reasonably sure that future wcn3998-3, wcn3998-4, ... can
> > operate with the generic wcn3998 power settings?
>
> [Harish] - yes with power settings of wcn3998, wcn3998-3 and wcn3998-4 ...
> also will work.
Thanks for the confirmation, in that case just using 'wcn3998' for now
sounds good to me.