Re: [PATCH] [PATCH] [v3] wireless: Initial driver submission for pureLiFi STA devices

Joe Perches
2020-10-15T22:35:39Z

On Wed, 2020-10-14 at 11:49 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.

trivia: netdev_<level> might be better than dev_<level>.

> diff --git a/drivers/net/wireless/purelifi/chip.c b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_chip_switch_radio(struct purelifi_chip *chip, u32 value)
> +{
> + int r;
> +
> + r = usb_write_req((const u8 *)&value, sizeof(value), USB_REQ_POWER_WR);
> + if (r)
> + dev_err(purelifi_chip_dev(chip), "%s r=%d", __func__, r);

missing '\n' termination.

[]

> > +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> + static struct purelifi_chip *chip_p;
> +
> + if (chip)
> + chip_p = chip;
> + if (!chip_p)
> + return -EINVAL;
> +
> + r = usb_write_req((const u8 *)&rate, sizeof(rate), USB_REQ_RATE_WR);
> + if (r)
> + dev_err(purelifi_chip_dev(chip), "set rate failed r=%d", r);

same missing newline, etc

It might also be better to use a consistent error message like

dev_err(purelifi_chip_dev(chip), "%s: failed, r=%d\n", r);

for both.

> diff --git a/drivers/net/wireless/purelifi/mac.c b/drivers/net/wireless/purelifi/mac.c
[]
> +static const struct ieee80211_rate purelifi_rates[] = {
> + { .bitrate = 10,
> + .hw_value = PURELIFI_CCK_RATE_1M, },

[]

> + { .bitrate = 60,
> + .hw_value = PURELIFI_OFDM_RATE_6M,
> + .flags = 0 },

Seems odd to set .flags to 0 for only some of the entries.
Perhaps more readable to always leave it off if unset.

> + { .bitrate = 90,
> + .hw_value = PURELIFI_OFDM_RATE_9M,
> + .flags = 0 },
> + { .bitrate = 120,
> + .hw_value = PURELIFI_OFDM_RATE_12M,
> + .flags = 0 },
> + { .bitrate = 180,
> + .hw_value = PURELIFI_OFDM_RATE_18M,
> + .flags = 0 },
> + { .bitrate = 240,
> + .hw_value = PURELIFI_OFDM_RATE_24M,
> + .flags = 0 },
> + { .bitrate = 360,
> + .hw_value = PURELIFI_OFDM_RATE_36M,
> + .flags = 0 },
> + { .bitrate = 480,
> + .hw_value = PURELIFI_OFDM_RATE_48M,
> + .flags = 0 },
> + { .bitrate = 540,
> + .hw_value = PURELIFI_OFDM_RATE_54M,
> + .flags = 0 },
> +};

[]
> +int purelifi_mac_init_hw(struct ieee80211_hw *hw)
> +{
> + int r;
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> +
> + r = purelifi_chip_init_hw(chip);
> + if (r) {
> + dev_warn(purelifi_mac_dev(mac), "init hw failed. (%d)\n", r);
> + goto out;
> + }
> +
> + dev_dbg(purelifi_mac_dev(mac), "irq_disabled %d", irqs_disabled());

missing newline

> +static int filter_ack(struct ieee80211_hw *hw, struct ieee80211_hdr *rx_hdr,
> + struct ieee80211_rx_status *stats)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct sk_buff *skb;
> + struct sk_buff_head *q;
> + unsigned long flags;
> + int found = 0;
> + int i, position = 0;
> +
> + if (!ieee80211_is_ack(rx_hdr->frame_control))
> + return 0;
> + dev_info(purelifi_mac_dev(mac), "%s::ACK Received", __func__);

missing newline

> +static int purelifi_op_add_interface(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + static const char * const iftype80211[] = {
> + [NL80211_IFTYPE_STATION] = "Station",
> + [NL80211_IFTYPE_ADHOC] = "Adhoc"
> + };
> +
> + if (mac->type != NL80211_IFTYPE_UNSPECIFIED)
> + return -EOPNOTSUPP;
> +
> + if (vif->type == NL80211_IFTYPE_ADHOC ||
> + vif->type == NL80211_IFTYPE_STATION) {
> + dev_info(purelifi_mac_dev(mac), "%s %s\n", __func__,
> + iftype80211[vif->type]);
> + mac->type = vif->type;
> + mac->vif = vif;
> + } else {
> + dev_info(purelifi_mac_dev(mac), "unsupported iftype");

etc...