> -----Original Message-----
> From: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
> Sent: Friday, August 19, 2022 2:41 AM
> To: Kalle Valo <kvalo@xxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: ath12k@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 28/50] wifi: ath12k: add htc.h
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 8/12/2022 9:09 AM, Kalle Valo wrote:
> > From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> >
> > (Patches split into one patch per file for easier review, but the
> > final commit will be one big patch. See the cover letter for more
> > info.)
> >
> > Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> > ---
> > drivers/net/wireless/ath/ath12k/htc.h | 311
> ++++++++++++++++++++++++++++++++++
> > 1 file changed, 311 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/htc.h
> > b/drivers/net/wireless/ath/ath12k/htc.h
>
> snip
>
> > +#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK
> GENMASK(1, 0)
> > +#define ATH12K_HTC_CONN_FLAGS_RECV_ALLOC GENMASK(15, 8)
> > +
> > +enum ath12k_htc_conn_flags {
> > + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_FOURTH =
> 0x0,
> > + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_HALF = 0x1,
> > + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_THREE_FOURTHS =
> 0x2,
> > + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_UNITY = 0x3,
>
> it seems strange to call a 2-bit field a flag
Sure will address this comment in the next version of the patch
>
> > + ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE = 1 << 2,
> > + ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL = 1 << 3 };
>
> and it seems strange to have an enum which incorporates the values of the
> ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK along with
> definitions of other mask flags.
>
> perhaps these would be more logically defined as macros just after
> ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK:
> #define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK
> GENMASK(1, 0) #define
> ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE BIT(2) #define
> ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL BIT(3)
>
> and then rename ath12k_htc_conn_flags to be more specific that it just has
> threshold level values?
>
Sure will address this comment in the next version of the patch
> > +
> > +enum ath12k_htc_conn_svc_status {
> > + ATH12K_HTC_CONN_SVC_STATUS_SUCCESS = 0,
> > + ATH12K_HTC_CONN_SVC_STATUS_NOT_FOUND = 1,
> > + ATH12K_HTC_CONN_SVC_STATUS_FAILED = 2,
> > + ATH12K_HTC_CONN_SVC_STATUS_NO_RESOURCES = 3,
> > + ATH12K_HTC_CONN_SVC_STATUS_NO_MORE_EP = 4
> > +};
> > +
> > +struct ath12k_htc_ready {
> > + __le32 id_credit_count;
> > + __le32 size_ep;
> > +} __packed;
> > +
> > +struct ath12k_htc_ready_extended {
> > + struct ath12k_htc_ready base;
> > + __le32 ver_bundle;
> > +} __packed;
> > +
> > +struct ath12k_htc_conn_svc {
> > + __le32 msg_svc_id;
> > + __le32 flags_len;
> > +} __packed;
> > +
> > +struct ath12k_htc_conn_svc_resp {
> > + __le32 msg_svc_id;
> > + __le32 flags_len;
> > + __le32 svc_meta_pad;
> > +} __packed;
> > +
> > +struct ath12k_htc_setup_complete_extended {
> > + __le32 msg_id;
>
> is there a reason this isn't msg_svc_id to be consistent with the other htc
> messages?
>
Yes, its different.
> or even have every htc msg begin with a struct ath12k_htc_msg hdr?
>
> perhaps also consider some naming conventions similar to what were
> adopted for WMI?
>
> > + __le32 flags;
> > + __le32 max_msgs_per_bundled_recv; } __packed;
> > +
> > +struct ath12k_htc_msg {
> > + __le32 msg_svc_id;
> > + __le32 flags_len;
> > +} __packed __aligned(4);
> > +
> > +enum ath12k_htc_record_id {
> > + ATH12K_HTC_RECORD_NULL = 0,
> > + ATH12K_HTC_RECORD_CREDITS = 1
> > +};
> > +
> > +struct ath12k_htc_record_hdr {
> > + u8 id; /* @enum ath12k_htc_record_id */
> > + u8 len;
> > + u8 pad0;
> > + u8 pad1;
> > +} __packed;
> > +
> > +struct ath12k_htc_credit_report {
> > + u8 eid; /* @enum ath12k_htc_ep_id */
> > + u8 credits;
> > + u8 pad0;
> > + u8 pad1;
> > +} __packed;
> > +
> > +struct ath12k_htc_record {
> > + struct ath12k_htc_record_hdr hdr;
> > + union {
> > + struct ath12k_htc_credit_report credit_report[0];
> > + u8 pauload[0];
>
> s/pauload/payload/?
>
Sure will address this comment in the next version of the patch
> s/[0]/[]/ per
> <https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-
> length-and-one-element-arrays>
>
> > + };
> > +} __packed __aligned(4);
> > +
> > +/* note: the trailer offset is dynamic depending
> > + * on payload length. this is only a struct layout draft */ struct
> > +ath12k_htc_frame {
> > + struct ath12k_htc_hdr hdr;
> > + union {
> > + struct ath12k_htc_msg msg;
> > + u8 payload[0];
>
> s/[0]/[]/
>
> > + };
> > + struct ath12k_htc_record trailer[0];
>
>
> s/[0]/[]/
>
> but this struct makes no sense. you can't have a variable-length payload that
> isn't at the end of the struct
>
> if this is just supposed to be some sort of pseudo-documentation then
> perhaps put it in a comment so that the compiler won't see this?
>
> or perhaps even better draw an ASCII representation?
Sure will address this comment in the next version of the patch
>
> > +} __packed __aligned(4);
>
> rest snipped
Thanks
Karthikeyan
