Messages in this thread | | | From | Kalle Valo <> | Subject | Re: [PATCH v7 15/24] wfx: add hif_rx.c/hif_rx.h | Date | Fri, 01 Oct 2021 13:09:48 +0300 |
| |
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
> +static int hif_startup_indication(struct wfx_dev *wdev, > + const struct hif_msg *hif, const void *buf) > +{ > + const struct hif_ind_startup *body = buf; > + > + if (body->status || body->firmware_type > 4) { > + dev_err(wdev->dev, "received invalid startup indication"); > + return -EINVAL; > + } > + memcpy(&wdev->hw_caps, body, sizeof(struct hif_ind_startup)); > + le16_to_cpus((__le16 *)&wdev->hw_caps.hardware_id); > + le16_to_cpus((__le16 *)&wdev->hw_caps.num_inp_ch_bufs); > + le16_to_cpus((__le16 *)&wdev->hw_caps.size_inp_ch_buf); > + le32_to_cpus((__le32 *)&wdev->hw_caps.supported_rate_mask);
I don't really like the casts here, and not really reliable either if there's ever more fields. I would just store values as little endian in hw_caps, I doubt it's performance critical.
And why does struct hif_ind_startup use both u32 and __le32? If it's in little endian it should always use __le types.
[...]
> + if (hif_id & 0x80) > + dev_err(wdev->dev, "unsupported HIF indication: ID %02x\n", > + hif_id); > + else > + dev_err(wdev->dev, "unexpected HIF confirmation: ID %02x\n", > + hif_id);
No magic values, please. A proper define for bit 0x80 would be nice.
-- https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
| |