Messages in this thread | | | Date | Fri, 23 Jun 2023 14:52:31 +0200 | From | Neil Armstrong <> | Subject | Re: [PATCH 2/4] bluetooth: qca: add support for WCN7850 |
| |
Hi Luiz,
On 22/06/2023 21:54, Luiz Augusto von Dentz wrote: > Hi Neil > > On Tue, Jun 20, 2023 at 8:43 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 20.06.2023 17:19, Neil Armstrong wrote: >>> Add support for the WCN7850 Bluetooth chipset. >>> >>> Tested on the SM8550 QRD platform. >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >> I don't have comments for this patch specifically, but: >> >> >>> drivers/bluetooth/btqca.c | 7 +++++++ >>> drivers/bluetooth/btqca.h | 10 ++++++++++ >>> drivers/bluetooth/hci_qca.c | 48 ++++++++++++++++++++++++++++++++++----------- >>> 3 files changed, 54 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c >>> index e7e58a956d15..037146b476ff 100644 >>> --- a/drivers/bluetooth/btqca.c >>> +++ b/drivers/bluetooth/btqca.c >>> @@ -617,6 +617,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, >>> } else if (soc_type == QCA_WCN6855) { >>> snprintf(config.fwname, sizeof(config.fwname), >>> "qca/hpbtfw%02x.tlv", rom_ver); >>> + } else if (soc_type == QCA_WCN7850) { >>> + snprintf(config.fwname, sizeof(config.fwname), >>> + "qca/hmtbtfw%02x.tlv", rom_ver); >>> } else { >>> snprintf(config.fwname, sizeof(config.fwname), >>> "qca/rampatch_%08x.bin", soc_ver); >>> @@ -654,6 +657,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, >>> else if (soc_type == QCA_WCN6855) >>> snprintf(config.fwname, sizeof(config.fwname), >>> "qca/hpnv%02x.bin", rom_ver); >>> + else if (soc_type == QCA_WCN7850) >>> + snprintf(config.fwname, sizeof(config.fwname), >>> + "qca/hmtnv%02x.bin", rom_ver); >>> else >>> snprintf(config.fwname, sizeof(config.fwname), >>> "qca/nvm_%08x.bin", soc_ver); >> The above changes should probably be made switch statements as well >> >>> @@ -695,6 +701,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, >>> case QCA_WCN3991: >>> case QCA_WCN6750: >>> case QCA_WCN6855: >>> + case QCA_WCN7850: >>> /* get fw build info */ >>> err = qca_read_fw_build_info(hdev); >>> if (err < 0) >>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >>> index b884095bcd9d..ff1850e984fe 100644 >>> --- a/drivers/bluetooth/btqca.h >>> +++ b/drivers/bluetooth/btqca.h >>> @@ -148,6 +148,7 @@ enum qca_btsoc_type { >>> QCA_QCA6390, >>> QCA_WCN6750, >>> QCA_WCN6855, >>> + QCA_WCN7850, >>> }; >>> >>> #if IS_ENABLED(CONFIG_BT_QCA) >>> @@ -173,6 +174,10 @@ static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) >>> { >>> return soc_type == QCA_WCN6855; >>> } >>> +static inline bool qca_is_wcn7850(enum qca_btsoc_type soc_type) >>> +{ >>> + return soc_type == QCA_WCN7850; >>> +} >>> >>> #else >>> >>> @@ -216,6 +221,11 @@ static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) >>> return false; >>> } >>> >>> +static inline bool qca_is_wcn7850(enum qca_btsoc_type soc_type) >>> +{ >>> + return false; >>> +} >>> + >>> static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) >>> { >>> return -EOPNOTSUPP; >>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >>> index e30c979535b1..49b8d75d271e 100644 >>> --- a/drivers/bluetooth/hci_qca.c >>> +++ b/drivers/bluetooth/hci_qca.c >>> @@ -1322,7 +1322,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) >>> /* Give the controller time to process the request */ >>> if (qca_is_wcn399x(qca_soc_type(hu)) || >>> qca_is_wcn6750(qca_soc_type(hu)) || >>> - qca_is_wcn6855(qca_soc_type(hu))) >>> + qca_is_wcn6855(qca_soc_type(hu)) || >>> + qca_is_wcn7850(qca_soc_type(hu))) >>> usleep_range(1000, 10000); >> Separate topic, but I see usages of the helper and raw comparisons >> everywhere.. I'd vote for switch everywhere > > +1, most of the time it is preferable to go with switches which makes > the code cleaner/more readable, Im fine not requiring it in this set > but I hope someone looks into doing some cleanup in this code.
Since we have time until the end of the v6.5 merge window I'll add an intermediate change adding switch/case for v2.
Thanks, Neil
> >> Konrad >>> else >>> msleep(300); >>> @@ -1400,7 +1401,8 @@ static int qca_check_speeds(struct hci_uart *hu) >>> { >>> if (qca_is_wcn399x(qca_soc_type(hu)) || >>> qca_is_wcn6750(qca_soc_type(hu)) || >>> - qca_is_wcn6855(qca_soc_type(hu))) { >>> + qca_is_wcn6855(qca_soc_type(hu)) || >>> + qca_is_wcn7850(qca_soc_type(hu))) { >>> if (!qca_get_speed(hu, QCA_INIT_SPEED) && >>> !qca_get_speed(hu, QCA_OPER_SPEED)) >>> return -EINVAL; >>> @@ -1435,7 +1437,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) >>> */ >>> if (qca_is_wcn399x(soc_type) || >>> qca_is_wcn6750(soc_type) || >>> - qca_is_wcn6855(soc_type)) >>> + qca_is_wcn6855(soc_type) || >>> + qca_is_wcn7850(soc_type)) >>> hci_uart_set_flow_control(hu, true); >>> >>> if (soc_type == QCA_WCN3990) { >>> @@ -1454,7 +1457,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) >>> error: >>> if (qca_is_wcn399x(soc_type) || >>> qca_is_wcn6750(soc_type) || >>> - qca_is_wcn6855(soc_type)) >>> + qca_is_wcn6855(soc_type) || >>> + qca_is_wcn7850(soc_type)) >>> hci_uart_set_flow_control(hu, false); >>> >>> if (soc_type == QCA_WCN3990) { >>> @@ -1691,7 +1695,8 @@ static int qca_power_on(struct hci_dev *hdev) >>> >>> if (qca_is_wcn399x(soc_type) || >>> qca_is_wcn6750(soc_type) || >>> - qca_is_wcn6855(soc_type)) { >>> + qca_is_wcn6855(soc_type) || >>> + qca_is_wcn7850(soc_type)) { >>> ret = qca_regulator_init(hu); >>> } else { >>> qcadev = serdev_device_get_drvdata(hu->serdev); >>> @@ -1733,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu) >>> bt_dev_info(hdev, "setting up %s", >>> qca_is_wcn399x(soc_type) ? "wcn399x" : >>> (soc_type == QCA_WCN6750) ? "wcn6750" : >>> - (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); >>> + (soc_type == QCA_WCN6855) ? "wcn6855" : >>> + (soc_type == QCA_WCN7850) ? "wcn7850" : "ROME/QCA6390"); >>> >>> qca->memdump_state = QCA_MEMDUMP_IDLE; >>> >>> @@ -1746,7 +1752,8 @@ static int qca_setup(struct hci_uart *hu) >>> >>> if (qca_is_wcn399x(soc_type) || >>> qca_is_wcn6750(soc_type) || >>> - qca_is_wcn6855(soc_type)) { >>> + qca_is_wcn6855(soc_type) || >>> + qca_is_wcn7850(soc_type)) { >>> set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); >>> hci_set_aosp_capable(hdev); >>> >>> @@ -1769,7 +1776,8 @@ static int qca_setup(struct hci_uart *hu) >>> >>> if (!(qca_is_wcn399x(soc_type) || >>> qca_is_wcn6750(soc_type) || >>> - qca_is_wcn6855(soc_type))) { >>> + qca_is_wcn6855(soc_type) || >>> + qca_is_wcn7850(soc_type))) { >>> /* Get QCA version information */ >>> ret = qca_read_soc_version(hdev, &ver, soc_type); >>> if (ret) >>> @@ -1909,6 +1917,20 @@ static const struct qca_device_data qca_soc_data_wcn6855 __maybe_unused = { >>> .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, >>> }; >>> >>> +static const struct qca_device_data qca_soc_data_wcn7850 __maybe_unused = { >>> + .soc_type = QCA_WCN7850, >>> + .vregs = (struct qca_vreg []) { >>> + { "vddio", 5000 }, >>> + { "vddaon", 26000 }, >>> + { "vdddig", 126000 }, >>> + { "vddrfa0p8", 102000 }, >>> + { "vddrfa1p2", 257000 }, >>> + { "vddrfa1p9", 302000 }, >>> + }, >>> + .num_vregs = 6, >>> + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, >>> +}; >>> + >>> static void qca_power_shutdown(struct hci_uart *hu) >>> { >>> struct qca_serdev *qcadev; >>> @@ -2074,7 +2096,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) >>> if (data && >>> (qca_is_wcn399x(data->soc_type) || >>> qca_is_wcn6750(data->soc_type) || >>> - qca_is_wcn6855(data->soc_type))) { >>> + qca_is_wcn6855(data->soc_type) || >>> + qca_is_wcn7850(data->soc_type))) { >>> qcadev->btsoc_type = data->soc_type; >>> qcadev->bt_power = devm_kzalloc(&serdev->dev, >>> sizeof(struct qca_power), >>> @@ -2105,7 +2128,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) >>> GPIOD_IN); >>> if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && >>> (data->soc_type == QCA_WCN6750 || >>> - data->soc_type == QCA_WCN6855)) >>> + data->soc_type == QCA_WCN6855 || >>> + data->soc_type == QCA_WCN7850)) >>> dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n"); >>> >>> qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL); >>> @@ -2182,7 +2206,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) >>> >>> if ((qca_is_wcn399x(qcadev->btsoc_type) || >>> qca_is_wcn6750(qcadev->btsoc_type) || >>> - qca_is_wcn6855(qcadev->btsoc_type)) && >>> + qca_is_wcn6855(qcadev->btsoc_type) || >>> + qca_is_wcn7850(qcadev->btsoc_type)) && >>> power->vregs_on) >>> qca_power_shutdown(&qcadev->serdev_hu); >>> else if (qcadev->susclk) >>> @@ -2368,6 +2393,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { >>> { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998}, >>> { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750}, >>> { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855}, >>> + { .compatible = "qcom,wcn7850-bt", .data = &qca_soc_data_wcn7850}, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match); >>> > > >
| |