Message ID | 20201117193124.9789-1-ceggers@arri.de (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net-next,1/4] net: ptp: introduce enum ptp_msg_type | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | warning | Series does not have a cover letter |
netdev/fixes_present | success | Link |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Clearly marked for net-next |
netdev/subject_prefix | success | Link |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Link |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 60 this patch: 60 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis CHECK: Please use a blank line after function/struct/union/enum declarations |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 60 this patch: 60 |
netdev/header_inline | success | Link |
netdev/stable | success | Stable not CCed |
On Tue, Nov 17, 2020 at 08:31:21PM +0100, Christian Eggers wrote: > Using a PTP wide enum will obsolete different driver internal defines > and uses of magic numbers. I like the general idea, but ... > +enum ptp_msg_type { > + PTP_MSGTYPE_SYNC = 0x0, > + PTP_MSGTYPE_DELAY_REQ = 0x1, > + PTP_MSGTYPE_PDELAY_REQ = 0x2, > + PTP_MSGTYPE_PDELAY_RESP = 0x3, > +}; I would argue that these should be #defines. After all, they are protocol data fields and not an abstract enumerated types. For example ... > @@ -103,10 +110,10 @@ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type); > * > * Return: The message type > */ > -static inline u8 ptp_get_msgtype(const struct ptp_header *hdr, > +static inline enum ptp_msg_type ptp_get_msgtype(const struct ptp_header *hdr, > unsigned int type) > { > - u8 msgtype; > + enum ptp_msg_type msgtype; > > if (unlikely(type & PTP_CLASS_V1)) { > /* msg type is located at the control field for ptp v1 */ msgtype = hdr->control; } else { msgtype = hdr->tsmt & 0x0f; This assigns directly from protocol data into an enumerated type. } return msgtype; Thanks, Richard
diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h index 56b2d7d66177..83024220cb42 100644 --- a/include/linux/ptp_classify.h +++ b/include/linux/ptp_classify.h @@ -93,6 +93,13 @@ unsigned int ptp_classify_raw(const struct sk_buff *skb); */ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type); +enum ptp_msg_type { + PTP_MSGTYPE_SYNC = 0x0, + PTP_MSGTYPE_DELAY_REQ = 0x1, + PTP_MSGTYPE_PDELAY_REQ = 0x2, + PTP_MSGTYPE_PDELAY_RESP = 0x3, +}; + /** * ptp_get_msgtype - Extract ptp message type from given header * @hdr: ptp header @@ -103,10 +110,10 @@ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type); * * Return: The message type */ -static inline u8 ptp_get_msgtype(const struct ptp_header *hdr, +static inline enum ptp_msg_type ptp_get_msgtype(const struct ptp_header *hdr, unsigned int type) { - u8 msgtype; + enum ptp_msg_type msgtype; if (unlikely(type & PTP_CLASS_V1)) { /* msg type is located at the control field for ptp v1 */ @@ -132,13 +139,13 @@ static inline struct ptp_header *ptp_parse_header(struct sk_buff *skb, { return NULL; } -static inline u8 ptp_get_msgtype(const struct ptp_header *hdr, +static inline enum ptp_msg_type ptp_get_msgtype(const struct ptp_header *hdr, unsigned int type) { /* The return is meaningless. The stub function would not be * executed since no available header from ptp_parse_header. */ - return 0; + return PTP_MSGTYPE_SYNC; } #endif #endif /* _PTP_CLASSIFY_H_ */
Using a PTP wide enum will obsolete different driver internal defines and uses of magic numbers. Signed-off-by: Christian Eggers <ceggers@arri.de> Cc: Kurt Kanzenbach <kurt@linutronix.de> --- include/linux/ptp_classify.h | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)