lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABBYNZ+JRbvOKnup0dH8V7YBpYcGuSn00i7zPhKP=3M3rEfF7Q@mail.gmail.com>
Date: Thu, 22 Jun 2023 12:54:29 -0700
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Konrad Dybcio <konrad.dybcio@...aro.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Balakrishna Godavarthi <bgodavar@...eaurora.org>, Rocky Liao <rjliao@...eaurora.org>, 
	Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg <johan.hedberg@...il.com>, 
	Andy Gross <agross@...nel.org>, Bjorn Andersson <andersson@...nel.org>, netdev@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-bluetooth@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/4] bluetooth: qca: add support for WCN7850

Hi Neil

On Tue, Jun 20, 2023 at 8:43 AM Konrad Dybcio <konrad.dybcio@...aro.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@...aro.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.

> 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);
> >



-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ