| 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
| ||
|
Message-ID: <CAGhaMFNENkcwvz1yxTLVZAXuWes5OKT6HqfWAHh+=SCLc=foeQ@mail.gmail.com>
Date: Fri, 18 Apr 2025 14:46:34 +0200
From: Artur Rojek <artur@...clusive.pl>
To: Sascha Hauer <s.hauer@...gutronix.de>
Cc: Johannes Berg <johannes@...solutions.net>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-wireless@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Jakub Klama <jakub@...clusive.pl>, Wojciech Kloska <wojciech@...clusive.pl>,
Ulf Axelsson <ulf.axelsson@...dicsemi.no>
Subject: Re: [RFC PATCH 1/2] net: wireless: Add Nordic nRF70 series Wi-Fi driver
Hi Sascha,
thanks for the review, reply inline.
On Tue, Apr 1, 2025 at 4:11 PM Sascha Hauer <s.hauer@...gutronix.de> wrote:
>
> Hi Artur,
>
> Some review feedback inside.
>
> As a general remark there are several functions returning 1 or -1 in
> case of errors. Although they seem to be used only internally in the
> driver it's far too easy to forward these errors to other kernel code
> with future changes, so better return error codes instead.
>
>
> On Mon, Mar 24, 2025 at 10:10:44PM +0100, Artur Rojek wrote:
> > +
> > +static struct nrf70_mem_op {
> > + int op;
> > + int width;
> > + int dummy;
> > + enum spi_mem_data_dir dir;
> > +} nrf70_read_ops[] = {
> > + { NRF70_OP_RD4, 4, 3, SPI_MEM_DATA_OUT },
> > + { NRF70_OP_FASTRD, 1, 1, SPI_MEM_DATA_OUT },
> > +}, nrf70_write_ops[] = {
> > + { NRF70_OP_PP4, 4, 0, SPI_MEM_DATA_IN },
> > + { NRF70_OP_PP, 1, 0, SPI_MEM_DATA_IN },
> > +};
>
> Should be const.
>
> > +static int nrf70_load_firmware(struct spi_mem *mem)
> > +{
> > + struct device *dev = &mem->spi->dev;
> > + struct nrf70_priv *priv = spi_mem_get_drvdata(mem);
> > + const struct nrf70_fw_header *header;
> > + const struct nrf70_fw_img *image;
> > + const struct firmware *firmware;
> > + int val, i, ret;
> > + u32 type, len;
> > +
> > + /* Perform RPU MCU reset. */
> > + nrf70_writel(mem, NRF70_SBUS_MIPS_MCU_CONTROL, 0x1);
> > +
> > + if (read_poll_timeout(nrf70_readl, val, !(val & 0x1),
> > + 10 * USEC_PER_MSEC, 50 * USEC_PER_MSEC, false,
> > + mem, NRF70_SBUS_MIPS_MCU_CONTROL)) {
> > + dev_err(dev, "Unable to reset LMAC\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + if (read_poll_timeout(nrf70_readl, val, val & 0x1,
> > + 10 * USEC_PER_MSEC, 50 * USEC_PER_MSEC, false,
> > + mem, NRF70_SBUS_CP0_SLEEP_STATUS)) {
> > + dev_err(dev, "Unable to reset LMAC2\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + nrf70_writel(mem, NRF70_SBUS_MIPS_MCU2_CONTROL, 0x1);
> > +
> > + if (read_poll_timeout(nrf70_readl, val, !(val & 0x1),
> > + 10 * USEC_PER_MSEC, 50 * USEC_PER_MSEC, false,
> > + mem, NRF70_SBUS_MIPS_MCU2_CONTROL)) {
> > + dev_err(dev, "Unable to reset UMAC\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + if (read_poll_timeout(nrf70_readl, val, val & 0x1,
> > + 10 * USEC_PER_MSEC, 50 * USEC_PER_MSEC, false,
> > + mem, NRF70_SBUS_CP1_SLEEP_STATUS)) {
> > + dev_err(dev, "Unable to reset UMAC2\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + ret = request_firmware(&firmware, "nrf70.bin", dev);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to request firmware: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + header = (const struct nrf70_fw_header *)firmware->data;
> > + if (header->signature != NRF70_FW_SIGNATURE) {
> > + dev_err(dev, "Invalid firmware signature\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + ret = nrf70_verify_firmware(dev, header);
> > + if (ret)
> > + goto out;
> > +
> > + priv->has_raw_mode = header->feature_flags & NRF70_FW_FEATURE_RAW_MODE;
> > +
> > + image = (const struct nrf70_fw_img *)header->data;
> > +
> > + for (i = 0; i < header->num_images; i++) {
> > + type = image->type;
> > + if (type > 3) {
>
> type >= ARRAY_SIZE(nrf_rpu_addr_lut)
>
> > +static int nrf70_init_rx_command(struct spi_mem *mem)
> > +{
> > + int i, ret = 0;
> > +
> > + for (i = 0; ret || i < NRF70_NUM_RX_BUFS; i++)
> > + ret = nrf70_init_rx(mem, i);
>
> I think the loop break condition should be
>
> !ret && i < NRF70_NUM_RX_BUFS
>
> This is rather hard to read though, so I suggest a plain
>
> for () {
> ret = nrf70_init_rx();
> if (ret)
> return ret;
> }
>
> > +
> > + return ret;
> > +}
> > +
> > +static struct nrf70_msg *nrf70_create_msg(u32 type, u32 id, size_t data_len,
> > + int iface)
> > +{
> > + struct nrf70_msg *msg;
> > + union cmd_header {
> > + struct __packed nrf70_header sys;
> > + struct __packed nrf70_umac_header umac;
> > + } *hdr;
> > + size_t len = sizeof(*msg) + data_len;
> > +
> > + msg = kzalloc(len, GFP_KERNEL);
> > + if (!msg)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + msg->type = type;
> > + msg->len = len;
> > +
> > + hdr = (union cmd_header *)msg->data;
> > + switch (type) {
> > + case NRF70_MSG_SYSTEM:
> > + fallthrough;
> > + case NRF70_MSG_DATA:
> > + hdr->sys.id = id;
> > + hdr->sys.len = data_len;
> > + break;
>
> This break is misaligned.
>
> > + case NRF70_MSG_UMAC:
> > + hdr->umac.id = id;
> > + if (iface >= 0) {
> > + hdr->umac.idx.wdev_id = iface;
> > + hdr->umac.idx.valid_fields = NRF70_UMAC_ID_WDEV;
> > + }
> > + break;
>
> ditto
>
> > + default:
> > + kfree(msg);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + return msg;
> > +}
> > +
> > +static const u8 nrf7002_qfn_rf_params[NRF70_RF_PARAMS_SZ] = {
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + /* XO */
> > + NRF70_QFN_XO_VAL,
>
> You could create defines for the array offsets instead of the values and use
> C99 initializers here. For example
>
> #define NRF70_QFN_XO_VAL_OFFSET 6
>
> and then:
>
> [NRF70_QFN_XO_VAL_OFFSET] = 0x2a,
>
> That way the offsets are immediately clear without counting the array
> members.
>
> With this having defines for the values is IMO rather unnecessary then
> as these are magic values anyway and usage of C99 initializers already
> makes their meaning clear.
>
> > + /* PD adjust values for MCS7. Currently unused. */
> > + NRF70_PD_ADJUST_VAL,
> > + NRF70_PD_ADJUST_VAL,
> > + NRF70_PD_ADJUST_VAL,
> > + NRF70_PD_ADJUST_VAL,
> > + /* TX power systematic offset. */
> > + NRF70_SYSTEM_OFFSET_LB,
> > + NRF70_SYSTEM_OFFSET_HB_CHAN_LOW,
> > + NRF70_SYSTEM_OFFSET_HB_CHAN_MID,
> > + NRF70_SYSTEM_OFFSET_HB_CHAN_HIGH,
> > + /* Max TX power value for which both EVM and SEM pass. */
> > + NRF70_QFN_MAX_TX_PWR_DSSS,
> > + NRF70_QFN_MAX_TX_PWR_LB_MCS7,
> > + NRF70_QFN_MAX_TX_PWR_LB_MCS0,
> > + NRF70_QFN_MAX_TX_PWR_HB_LOW_CHAN_MCS7,
> > + NRF70_QFN_MAX_TX_PWR_HB_MID_CHAN_MCS7,
> > + NRF70_QFN_MAX_TX_PWR_HB_HIGH_CHAN_MCS7,
> > + NRF70_QFN_MAX_TX_PWR_HB_LOW_CHAN_MCS0,
> > + NRF70_QFN_MAX_TX_PWR_HB_MID_CHAN_MCS0,
> > + NRF70_QFN_MAX_TX_PWR_HB_HIGH_CHAN_MCS0,
> > + /* RX gain adjustment offsets. */
> > + NRF70_RX_GAIN_OFFSET_LB_CHAN,
> > + NRF70_RX_GAIN_OFFSET_HB_LOW_CHAN,
> > + NRF70_RX_GAIN_OFFSET_HB_MID_CHAN,
> > + NRF70_RX_GAIN_OFFSET_HB_HIGH_CHAN,
> > + /* Voltage and temperature dependent backoffs. */
> > + NRF70_QFN_MAX_CHIP_TEMP,
> > + NRF70_QFN_MIN_CHIP_TEMP,
> > + NRF70_QFN_LB_MAX_PWR_BKF_HI_TEMP,
> > + NRF70_QFN_LB_MAX_PWR_BKF_LOW_TEMP,
> > + NRF70_QFN_HB_MAX_PWR_BKF_HI_TEMP,
> > + NRF70_QFN_HB_MAX_PWR_BKF_LOW_TEMP,
> > + NRF70_QFN_LB_VBT_LT_VLOW,
> > + NRF70_QFN_HB_VBT_LT_VLOW,
> > + NRF70_QFN_LB_VBT_LT_LOW,
> > + NRF70_QFN_HB_VBT_LT_LOW,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + NRF70_RESERVED,
> > + /* PHY parameters blob. */
> > + NRF70_PHY_PARAMS,
> > +};
> > +
>
>
> > +static struct ieee80211_rate nrf70_dsss_rates[] = {
>
> Should be const.
>
> > + { .bitrate = 10, .hw_value = 2 },
> > + { .bitrate = 20, .hw_value = 4,
> > + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> > + { .bitrate = 55,
> > + .hw_value = 11,
> > + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> > + { .bitrate = 110,
> > + .hw_value = 22,
> > + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> > + { .bitrate = 60, .hw_value = 12 },
> > + { .bitrate = 90, .hw_value = 18 },
> > + { .bitrate = 120, .hw_value = 24 },
> > + { .bitrate = 180, .hw_value = 36 },
> > + { .bitrate = 240, .hw_value = 48 },
> > + { .bitrate = 360, .hw_value = 72 },
> > + { .bitrate = 480, .hw_value = 96 },
> > + { .bitrate = 540, .hw_value = 108 },
> > +};
> > +
> > +static struct ieee80211_supported_band nrf70_band_2ghz = {
>
> Should be const.
>
> > + .channels = nrf70_dsss_chans,
> > + .n_channels = ARRAY_SIZE(nrf70_dsss_chans),
> > + .band = NL80211_BAND_2GHZ,
> > + .bitrates = nrf70_dsss_rates,
> > + .n_bitrates = ARRAY_SIZE(nrf70_dsss_rates),
> > + .ht_cap = {
> > + .ht_supported = 1,
> > + .cap = IEEE80211_HT_CAP_MAX_AMSDU |
> > + IEEE80211_HT_CAP_SGI_20 |
> > + IEEE80211_HT_CAP_SGI_40 |
> > + IEEE80211_HT_CAP_SUP_WIDTH_20_40 |
> > + BIT(IEEE80211_HT_CAP_RX_STBC_SHIFT) |
> > + IEEE80211_HT_CAP_LSIG_TXOP_PROT,
> > + .ampdu_factor = IEEE80211_HT_MAX_AMPDU_32K,
> > + .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
> > + .mcs = {
> > + .tx_params = IEEE80211_HT_MCS_TX_DEFINED,
> > + .rx_mask[0] = 0xff,
> > + .rx_mask[4] = 0x1,
> > + },
> > + },
> > + .iftype_data = &(const struct ieee80211_sband_iftype_data){
> > + .types_mask = BIT(NL80211_IFTYPE_STATION) |
> > + BIT(NL80211_IFTYPE_AP),
> > + .he_cap = {
> > + .has_he = true,
> > + .he_cap_elem = {
> > + .mac_cap_info[0] = IEEE80211_HE_MAC_CAP0_HTC_HE,
> > + },
> > + .he_mcs_nss_supp = {
> > + .rx_mcs_80 = 0xfffc,
> > + .tx_mcs_80 = 0xfffc,
> > + .rx_mcs_160 = 0xffff,
> > + .tx_mcs_160 = 0xffff,
> > + .rx_mcs_80p80 = 0xffff,
> > + .tx_mcs_80p80 = 0xffff,
> > + },
> > + },
> > + },
> > + .n_iftype_data = 1,
> > +};
> > +
> > +#define NRF70_CHAN5G(freq, idx, flgs) \
> > +{ \
> > + .band = NL80211_BAND_5GHZ, \
> > + .center_freq = (freq), \
> > + .hw_value = (idx), \
> > + .max_power = 20, \
> > + .flags = (flgs) \
> > +}
> > +
> > +static struct ieee80211_channel nrf70_ofdm_chans[] = {
>
> const
>
> > + NRF70_CHAN5G(5180, 14, 0),
> > + NRF70_CHAN5G(5200, 15, 0),
> > + NRF70_CHAN5G(5220, 16, 0),
> > + NRF70_CHAN5G(5240, 17, 0),
> > + NRF70_CHAN5G(5260, 18, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5280, 19, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5300, 20, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5320, 21, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5500, 22, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5520, 23, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5540, 24, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5560, 25, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5580, 26, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5600, 27, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5620, 28, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5640, 29, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5660, 30, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5680, 31, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5700, 32, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5720, 33, IEEE80211_CHAN_RADAR),
> > + NRF70_CHAN5G(5745, 34, 0),
> > + NRF70_CHAN5G(5765, 35, 0),
> > + NRF70_CHAN5G(5785, 36, 0),
> > + NRF70_CHAN5G(5805, 37, 0),
> > + NRF70_CHAN5G(5825, 38, 0),
> > + NRF70_CHAN5G(5845, 39, 0),
> > + NRF70_CHAN5G(5865, 40, 0),
> > + NRF70_CHAN5G(5885, 41, 0),
> > +};
> > +
> > +static struct ieee80211_rate nrf70_ofdm_rates[] = {
>
> const
>
> > + { .bitrate = 60, .hw_value = 12 },
> > + { .bitrate = 90, .hw_value = 18 },
> > + { .bitrate = 120, .hw_value = 24 },
> > + { .bitrate = 180, .hw_value = 36 },
> > + { .bitrate = 240, .hw_value = 48 },
> > + { .bitrate = 360, .hw_value = 72 },
> > + { .bitrate = 480, .hw_value = 96 },
> > + { .bitrate = 540, .hw_value = 108 },
> > +};
> > +
> > +#define NRF70_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_SHIFT \
> > + (3 << IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_SHIFT)
> > +
> > +#define NRF70_VHT_MCS_MAP \
> > + ((IEEE80211_VHT_MCS_SUPPORT_0_7 << 2 * 0) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 1) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 2) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 3) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 4) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 5) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 6) | \
> > + (IEEE80211_VHT_MCS_NOT_SUPPORTED << 2 * 7))
> > +
> > +static struct ieee80211_supported_band nrf70_band_5ghz = {
>
> const
>
> > + .channels = nrf70_ofdm_chans,
> > + .n_channels = ARRAY_SIZE(nrf70_ofdm_chans),
> > + .band = NL80211_BAND_5GHZ,
> > + .bitrates = nrf70_ofdm_rates,
> > + .n_bitrates = ARRAY_SIZE(nrf70_ofdm_rates),
> > + .ht_cap = {
> > + .ht_supported = 1,
> > + .cap = IEEE80211_HT_CAP_MAX_AMSDU |
> > + IEEE80211_HT_CAP_SGI_20 |
> > + IEEE80211_HT_CAP_SGI_40 |
> > + IEEE80211_HT_CAP_SUP_WIDTH_20_40 |
> > + BIT(IEEE80211_HT_CAP_RX_STBC_SHIFT) |
> > + IEEE80211_HT_CAP_LSIG_TXOP_PROT,
> > + .ampdu_factor = IEEE80211_HT_MAX_AMPDU_32K,
> > + .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
> > + .mcs = {
> > + .tx_params = IEEE80211_HT_MCS_TX_DEFINED,
> > + .rx_mask[0] = 0xff,
> > + .rx_mask[4] = 0x1,
> > + },
> > + },
> > + .vht_cap = {
> > + .vht_supported = true,
> > + .cap = IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454 |
> > + IEEE80211_VHT_CAP_SHORT_GI_80 |
> > + IEEE80211_VHT_CAP_RXLDPC |
> > + IEEE80211_VHT_CAP_TXSTBC |
> > + IEEE80211_VHT_CAP_RXSTBC_1 |
> > + IEEE80211_VHT_CAP_HTC_VHT |
> > + NRF70_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_SHIFT,
> > + .vht_mcs = {
> > + .rx_mcs_map = NRF70_VHT_MCS_MAP,
> > + .tx_mcs_map = NRF70_VHT_MCS_MAP,
> > + },
> > + },
> > + .iftype_data = &(const struct ieee80211_sband_iftype_data){
> > + .types_mask = BIT(NL80211_IFTYPE_STATION) |
> > + BIT(NL80211_IFTYPE_AP),
> > + .he_cap = {
> > + .has_he = true,
> > + .he_cap_elem = {
> > + .mac_cap_info[0] = IEEE80211_HE_MAC_CAP0_HTC_HE,
> > + },
> > + .he_mcs_nss_supp = {
> > + .rx_mcs_80 = 0xfffc,
> > + .tx_mcs_80 = 0xfffc,
> > + .rx_mcs_160 = 0xffff,
> > + .tx_mcs_160 = 0xffff,
> > + .rx_mcs_80p80 = 0xffff,
> > + .tx_mcs_80p80 = 0xffff,
> > + },
> > + },
> > + },
> > + .n_iftype_data = 1,
> > +};
> > +
>
>
>
> > +static int nrf70_probe(struct spi_mem *mem)
> > +{
> > + struct nrf70_priv *priv;
> > + struct nrf70_wiphy_priv *wpriv;
> > + struct device *dev = &mem->spi->dev;
> > + struct nrf70_vif *vif;
> > + int irq_num, ret, val;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > + spi_mem_set_drvdata(mem, priv);
> > + priv->mem = mem;
> > +
> > + mutex_init(&priv->write_lock);
> > + mutex_init(&priv->read_lock);
> > + mutex_init(&priv->enqueue_lock);
> > + mutex_init(&priv->desc_lock);
> > +
> > + priv->buck_en = devm_gpiod_get(dev, "bucken", 0);
> > + if (!priv->buck_en) {
>
> devm_gpiod_get() returns an error pointer in case of failure. You have
> to check the return value with IS_ERR().
>
> > + dev_err(dev, "Unable to find bucken-gpios property\n");
> > + return -EINVAL;
> > + }
>
> You could use dev_err_probe() for returning an error from the probe
> function.
>
> > +
> > + ret = gpiod_direction_output(priv->buck_en, 0);
> > + if (ret) {
> > + dev_err(dev, "Unable to set buck_en direction\n");
> > + return -EIO;
> > + }
>
> Should this "bucken" GPIO rather be a regulator?
>
> Is this really mandatory? It sounds like it could be hardwired to some
> fixed voltage.
Take this with a grain of salt, as I am not a hardware designer.
Nordic's Product Specification document [1] stipulates that BUCKEN line
controls the PWR IP core. In order to start the IC, a power up sequence
is required: first the various power supply lines, then BUCKEN, then
IOVDD signal. Similar with a power down sequence.
To me, this reads that BUCKEN cannot be simply wired to some fixed
voltage and needs some sort of state control. Additionally, it is
the (only?) way for the software to reset the IC and put it into a known
state during probe, or after a hang.
At least for the second case, the driver needs some sort of power
control, whether it's directly the BUCKEN line, or some other circuit
that in turn flicks the BUCKEN. I could rename it to 'vpwr-supply' if
that makes things more transparent. I would risk saying that it makes
it mandatory.
PS. The annoying part about the regulator API is that it reference
counts its usage, and as such regulator_disable() cannot be called
without a prior call to regulator_enable(). So to power-cycle the IC via
the BUCKEN line, I will need to do the following sequence:
regulator_enable() -> regulator_disable() -> regulator_enable()
But I can live with that ;)
[1] https://docs-be.nordicsemi.com/bundle/ps_nrf7002/attach/nRF7002_PS_v1.2.pdf?_LANG=enus
(p. 51)
Cheers,
Artur
>
> > +
> > + priv->iovdd_en = devm_gpiod_get_optional(dev, "iovdd", 0);
>
> Same here. Should this rather be a regulator?
>
> > + if (IS_ERR(priv->iovdd_en)) {
> > + dev_err(dev, "Invalid iovdd-gpios property\n");
> > + return PTR_ERR(priv->iovdd_en);
> > + }
> > +
> > + if (priv->iovdd_en) {
> > + ret = gpiod_direction_output(priv->iovdd_en, 0);
> > + if (ret) {
> > + dev_err(dev, "Unable to set iovdd_en direction\n");
> > + return -EIO;
> > + }
> > + }
> > +
> > + priv->irq = devm_gpiod_get(dev, "irq", 0);
> > + if (!priv->irq) {
> > + dev_err(dev, "Unable to find irq-gpios property\n");
> > + return -EINVAL;
> > + }
>
> Any particular reason you describe the interrupt as GPIO?
>
> I would just use the interrupts / interrupt-parent binding.
>
> > +
> > + ret = gpiod_direction_input(priv->irq);
> > + if (ret) {
> > + dev_err(dev, "Unable to set irq direction\n");
> > + return -EIO;
> > + }
> > +
> > + irq_num = gpiod_to_irq(priv->irq);
> > + if (irq_num < 0) {
> > + dev_err(dev, "Unable to get gpio irq number: %d\n", ret);
> > + return irq_num;
> > + }
> > +
> > + /* Test support of opcodes. */
> > + priv->read_op = nrf70_select_op_variant(mem, nrf70_read_ops,
> > + ARRAY_SIZE(nrf70_read_ops));
> > + if (!priv->read_op)
> > + return -EOPNOTSUPP;
> > + priv->write_op = nrf70_select_op_variant(mem, nrf70_write_ops,
> > + ARRAY_SIZE(nrf70_write_ops));
> > + if (!priv->write_op)
> > + return -EOPNOTSUPP;
> > +
> > + /* Wake up RPU. */
> > + gpiod_set_value(priv->buck_en, 0);
> > + if (priv->iovdd_en)
> > + gpiod_set_value(priv->iovdd_en, 0);
> > + usleep_range(1000, 2000);
> > + gpiod_set_value(priv->buck_en, 1);
> > + usleep_range(1000, 2000);
> > + if (priv->iovdd_en) {
> > + gpiod_set_value(priv->iovdd_en, 1);
> > + usleep_range(1000, 2000);
> > + }
> > +
> > + nrf70_wrsr2(mem, NRF70_SR2_WAKEUP_REQ);
> > +
> > + if (read_poll_timeout(nrf70_rdsr2, val, val & NRF70_SR2_WAKEUP_REQ,
> > + 5 * USEC_PER_MSEC, 2 * USEC_PER_SEC, false,
> > + mem)) {
> > + dev_err(dev, "Unable to wake up RPU: request failed\n");
> > + ret = -ETIMEDOUT;
> > + goto err_disable_rpu;
> > + }
> > +
> > + if (read_poll_timeout(nrf70_rdsr1, val, val & NRF70_SR1_AWAKE,
> > + 5 * USEC_PER_MSEC, 2 * USEC_PER_SEC, false,
> > + mem)) {
> > + dev_err(dev, "Unable to wake up RPU: bus not active\n");
> > + ret = -ETIMEDOUT;
> > + goto err_disable_rpu;
> > + }
> > +
> > + /* Ungate RPU clocks. */
> > + nrf70_writel(mem, NRF70_PBUS_CLK, NRF70_PBUS_CLK_UNGATE);
> > +
> > + ret = nrf70_tune_read_op(mem);
> > + if (ret) {
> > + dev_err(dev, "Unable to tune-in read op timing\n");
> > + goto err_disable_rpu;
> > + }
> > +
> > + ret = nrf70_load_firmware(mem);
> > + if (ret)
> > + goto err_disable_rpu;
> > +
> > + init_completion(&priv->init_done);
> > + init_completion(&priv->station_info_available);
> > + init_completion(&priv->regdom_updated);
> > + INIT_WORK(&priv->event_work, nrf70_event_worker);
> > +
> > + ret = devm_request_threaded_irq(dev, irq_num, NULL, nrf70_irq,
> > + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
> > + dev_name(dev), mem);
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to request threaded irq: %d\n", ret);
> > + goto err_disable_rpu;
> > + }
> > +
> > + priv->tx_desc_bitmap[0] = NRF70_DESC_MASK;
> > + priv->tx_desc_bitmap[1] = NRF70_DESC_MASK;
> > + INIT_LIST_HEAD(&priv->cookies);
> > + INIT_LIST_HEAD(&priv->vifs);
> > +
> > + ret = nrf70_mac_init(mem);
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to initialize UMAC: %d\n", ret);
> > + goto err_disable_rpu;
> > + }
> > +
> > + priv->wiphy = wiphy_new(&nrf70_cfg80211_ops, sizeof(*wpriv));
> > + if (!priv->wiphy) {
> > + dev_err(dev, "Unable to allocate wiphy\n");
> > + ret = -ENOMEM;
> > + goto err_deinit_rpu;
> > + }
> > +
> > + set_wiphy_dev(priv->wiphy, dev);
> > + wpriv = wiphy_priv(priv->wiphy);
> > + wpriv->priv = priv;
> > +
> > + priv->wiphy->mgmt_stypes = nrf70_default_mgmt_stypes;
> > + priv->wiphy->iface_combinations = nrf70_if_comb;
> > + priv->wiphy->flags |= WIPHY_FLAG_NETNS_OK | WIPHY_FLAG_4ADDR_AP |
> > + WIPHY_FLAG_4ADDR_STATION |
> > + WIPHY_FLAG_REPORTS_OBSS | WIPHY_FLAG_OFFCHAN_TX |
> > + WIPHY_FLAG_CONTROL_PORT_PROTOCOL |
> > + WIPHY_FLAG_AP_UAPSD;
> > +
> > + priv->wiphy->features |= NL80211_FEATURE_SK_TX_STATUS |
> > + NL80211_FEATURE_SAE |
> > + NL80211_FEATURE_HT_IBSS |
> > + NL80211_FEATURE_MAC_ON_CREATE;
> > +
> > + wiphy_ext_feature_set(priv->wiphy, NL80211_EXT_FEATURE_SET_SCAN_DWELL);
> > +
> > + priv->wiphy->bands[NL80211_BAND_2GHZ] = &nrf70_band_2ghz;
> > + priv->wiphy->bands[NL80211_BAND_5GHZ] = &nrf70_band_5ghz;
> > + priv->wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
> > + priv->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
> > + BIT(NL80211_IFTYPE_AP);
> > + if (priv->has_raw_mode)
> > + priv->wiphy->interface_modes |= BIT(NL80211_IFTYPE_MONITOR);
> > + priv->wiphy->max_scan_ssids = NRF70_SCAN_SSIDS_MAX;
> > + priv->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
> > + priv->wiphy->cipher_suites = nrf70_cipher_suites;
> > + priv->wiphy->n_cipher_suites = ARRAY_SIZE(nrf70_cipher_suites);
> > +
> > + priv->wiphy->reg_notifier = nrf70_reg_notifier;
> > +
> > + ret = wiphy_register(priv->wiphy);
> > + if (ret < 0) {
> > + dev_err(dev, "Unable to register wiphy: %d\n", ret);
> > + goto err_wiphy;
> > + }
> > +
> > + priv->vif_bitmap = NRF70_VIFS_MASK;
> > +
> > + /* Add primary net interface. */
> > + vif = nrf70_add_if(priv, "nrf%d", NET_NAME_UNKNOWN,
> > + NL80211_IFTYPE_STATION, NULL, false);
> > + if (!IS_ERR(vif))
> > + return 0;
> > +
> > + ret = PTR_ERR(vif);
> > + wiphy_unregister(priv->wiphy);
> > +err_wiphy:
> > + wiphy_free(priv->wiphy);
> > +err_deinit_rpu:
> > + nrf70_deinit_command(mem);
> > +err_disable_rpu:
> > + nrf70_writel(mem, NRF70_PBUS_CLK, 0x0);
> > + if (priv->iovdd_en)
> > + gpiod_set_value(priv->iovdd_en, 0);
> > + gpiod_set_value(priv->buck_en, 0);
> > +
> > + return ret;
> > +}
> > +
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists