From: Jonathan.Cameron@huawei•com (Jonathan Cameron)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/8] firmware: arm_scmi: improve code readability using bitfield accessor macros
Date: Thu, 17 May 2018 09:14:33 +0100 [thread overview]
Message-ID: <20180517091433.0000513b@huawei.com> (raw)
In-Reply-To: <1525885634-22348-2-git-send-email-sudeep.holla@arm.com>
On Wed, 9 May 2018 18:07:07 +0100
Sudeep Holla <sudeep.holla@arm•com> wrote:
> By using FIELD_{FIT,GET,PREP} and GENMASK macro accessors we can avoid
> some clumpsy custom shifting and masking macros and also improve the
> code better readability.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm•com>
Hi Sudeep,
A minor comment inline.
Jonathan
> ---
> drivers/firmware/arm_scmi/common.h | 9 +++++----
> drivers/firmware/arm_scmi/driver.c | 31 ++++++++++++++-----------------
> 2 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 0c30234f9098..e8f332c9c469 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -7,6 +7,7 @@
> * Copyright (C) 2018 ARM Ltd.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/completion.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> @@ -14,10 +15,10 @@
> #include <linux/scmi_protocol.h>
> #include <linux/types.h>
>
> -#define PROTOCOL_REV_MINOR_BITS 16
> -#define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1)
> -#define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS)
> -#define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK)
> +#define PROTOCOL_REV_MINOR_MASK GENMASK(15, 0)
> +#define PROTOCOL_REV_MAJOR_MASK GENMASK(31, 16)
> +#define PROTOCOL_REV_MAJOR(x) (u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))
> +#define PROTOCOL_REV_MINOR(x) (u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))
> #define MAX_PROTOCOLS_IMP 16
> #define MAX_OPPS 16
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 14b147135a0c..917786d91f55 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -29,16 +29,12 @@
>
> #include "common.h"
>
> -#define MSG_ID_SHIFT 0
> -#define MSG_ID_MASK 0xff
> -#define MSG_TYPE_SHIFT 8
> -#define MSG_TYPE_MASK 0x3
> -#define MSG_PROTOCOL_ID_SHIFT 10
> -#define MSG_PROTOCOL_ID_MASK 0xff
> -#define MSG_TOKEN_ID_SHIFT 18
> -#define MSG_TOKEN_ID_MASK 0x3ff
> -#define MSG_XTRACT_TOKEN(header) \
> - (((header) >> MSG_TOKEN_ID_SHIFT) & MSG_TOKEN_ID_MASK)
> +#define MSG_ID_MASK GENMASK(7, 0)
> +#define MSG_TYPE_MASK GENMASK(9, 8)
> +#define MSG_PROTOCOL_ID_MASK GENMASK(17, 10)
> +#define MSG_TOKEN_ID_MASK GENMASK(27, 18)
> +#define MSG_XTRACT_TOKEN(hdr) FIELD_GET(MSG_TOKEN_ID_MASK, (hdr))
> +#define MSG_TOKEN_MAX (MSG_XTRACT_TOKEN(MSG_TOKEN_ID_MASK) + 1)
This feels a little odd. It's not the Max value, I think, but rather one more than
it. I would set it to this -1 and use > than in the test below.
>
> enum scmi_error_codes {
> SCMI_SUCCESS = 0, /* Success */
> @@ -255,9 +251,9 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
> */
> static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr)
> {
> - return ((hdr->id & MSG_ID_MASK) << MSG_ID_SHIFT) |
> - ((hdr->seq & MSG_TOKEN_ID_MASK) << MSG_TOKEN_ID_SHIFT) |
> - ((hdr->protocol_id & MSG_PROTOCOL_ID_MASK) << MSG_PROTOCOL_ID_SHIFT);
> + return FIELD_PREP(MSG_ID_MASK, hdr->id) |
> + FIELD_PREP(MSG_TOKEN_ID_MASK, hdr->seq) |
> + FIELD_PREP(MSG_PROTOCOL_ID_MASK, hdr->protocol_id);
> }
>
> /**
> @@ -621,9 +617,9 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
> struct scmi_xfers_info *info = &sinfo->minfo;
>
> /* Pre-allocated messages, no more than what hdr.seq can support */
> - if (WARN_ON(desc->max_msg >= (MSG_TOKEN_ID_MASK + 1))) {
> - dev_err(dev, "Maximum message of %d exceeds supported %d\n",
> - desc->max_msg, MSG_TOKEN_ID_MASK + 1);
> + if (WARN_ON(desc->max_msg >= MSG_TOKEN_MAX)) {
> + dev_err(dev, "Maximum message of %d exceeds supported %ld\n",
> + desc->max_msg, MSG_TOKEN_MAX);
> return -EINVAL;
> }
>
> @@ -840,7 +836,8 @@ static int scmi_probe(struct platform_device *pdev)
> if (of_property_read_u32(child, "reg", &prot_id))
> continue;
>
> - prot_id &= MSG_PROTOCOL_ID_MASK;
> + if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id))
> + dev_err(dev, "Out of range protocol %d\n", prot_id);
>
> if (!scmi_is_protocol_implemented(handle, prot_id)) {
> dev_err(dev, "SCMI protocol %d not implemented\n",
next prev parent reply other threads:[~2018-05-17 8:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-09 17:07 [PATCH 0/8] firmware: arm_scmi: trivial cleanups Sudeep Holla
2018-05-09 17:07 ` [PATCH 1/8] firmware: arm_scmi: improve code readability using bitfield accessor macros Sudeep Holla
2018-05-17 8:14 ` Jonathan Cameron [this message]
2018-05-09 17:07 ` [PATCH 2/8] firmware: arm_scmi: fix kernel-docs documentation Sudeep Holla
2018-05-17 8:30 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 3/8] firmware: arm_scmi: rename get_transition_latency and add_opps_to_device Sudeep Holla
2018-05-10 8:29 ` Rafael J. Wysocki
2018-05-10 9:53 ` Sudeep Holla
2018-05-17 8:32 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 4/8] firmware: arm_scmi: rename scmi_xfer_{init,get,put} Sudeep Holla
2018-05-17 8:38 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 5/8] firmware: arm_scmi: drop unused `con_priv` structure member Sudeep Holla
2018-05-17 8:41 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 6/8] firmware: arm_scmi: remove unnecessary bitmap_zero Sudeep Holla
2018-05-17 8:43 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 7/8] firmware: arm_scmi: improve exit paths and code readability Sudeep Holla
2018-05-17 9:13 ` Jonathan Cameron
2018-05-09 17:07 ` [PATCH 8/8] firmware: arm_scmi: simplify exit path by returning on error Sudeep Holla
2018-05-17 9:14 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180517091433.0000513b@huawei.com \
--to=jonathan.cameron@huawei$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox