From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Vinod Koul <vkoul@kernel.org>
Cc: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>,
alsa-devel@alsa-project.org,
Kai Vehmanen <kai.vehmanen@linux.intel.com>,
tiwai@suse.de, gregkh@linuxfoundation.org,
open list <linux-kernel@vger.kernel.org>,
broonie@kernel.org, Sanyog Kale <sanyog.r.kale@intel.com>,
Bard liao <yung-chuan.liao@linux.intel.com>,
Rander Wang <rander.wang@linux.intel.com>
Subject: Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls
Date: Wed, 9 Sep 2020 08:48:21 -0500 [thread overview]
Message-ID: <184867c2-9f0c-bffe-2eb7-e9c5735614b0@linux.intel.com> (raw)
In-Reply-To: <20200909075555.GK77521@vkoul-mobl>
>>>> + * 25 0 (Reserved)
>>>> + * 24:22 Function Number [2:0]
>>>> + * 21 Entity[6]
>>>> + * 20:19 Control Selector[5:4]
>>>> + * 18 0 (Reserved)
>>>> + * 17:15 Control Number[5:3]
>>>> + * 14 Next
>>>> + * 13 MBQ
>>>> + * 12:7 Entity[5:0]
>>>> + * 6:3 Control Selector[3:0]
>>>> + * 2:0 Control Number[2:0]
>>>> + */
>>>> +
>>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
>>>> + (BIT(30) | \
>>>
>>> Programmatically this is fine, but then since we are defining for the
>>> description above, IMO it would actually make sense for this to be defined
>>> as FIELD_PREP:
>>>
>>> FIELD_PREP(GENMASK(30, 26), 1)
>>>
>>> or better
>>>
>>> u32_encode_bits(GENMASK(30, 26), 1)
>>>
>>>> + FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
>>>
>>> Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
>>> below?
>>
>> Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and
>> your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
>
> yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun))
> would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
>
> Do you agree?
The Function (fun) case is the easy one: the value is not split in two.
But look at the entity case, it's split in two:
FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) FIELD_PREP(GENMASK(12,
7), FIELD_GET(GENMASK(5, 0), (ent)))
same for control
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) |
FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
and same for channel number
FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) |
FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
I don't see how we can avoid using the FIELD_GET to extract the relevant
bits from entity, control, channel number values.
Or I am missing your point completely.
>>> And while at it, consider defining masks for various fields rather than
>>> using numbers in GENMASK() above, that would look better, be more
>>> readable and people can reuse it.
>>
>> Actually on this one I disagree. These fields are not intended to be used by
>> anyone, the goal is precisely to hide them behind regmap, and the use of raw
>> numbers makes it easier to cross-check the documentation and the code.
>> Adding a separate set of definitions would not increase readability.
>
> Which one would you prefer:
>
> #define SDCA_FUN_MASK GENMASK(24, 22)
>
> foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
>
> Or the one proposed...?
Same as above, let's see what this does with the control case where we'd
need to have four definitions:
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
#define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
#define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
#define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
And the code would look like
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1,
FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun));
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2,
FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
The original suggestion was:
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) |
FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
I prefer the original... Adding these defines doesn't really add value
because
a) the values will not be reused anywhere else.
b) we need 12 of those defines
b) we need a prefix for those defines which makes the code heavier
next prev parent reply other threads:[~2020-09-09 13:51 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-01 16:22 [PATCH v2 0/3] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart
2020-09-01 16:22 ` [PATCH v2 1/3] regmap: sdw: add required header files Pierre-Louis Bossart
2020-09-01 16:22 ` Pierre-Louis Bossart
2020-09-01 16:22 ` [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart
2020-09-01 16:22 ` Pierre-Louis Bossart
2020-09-04 5:02 ` Vinod Koul
2020-09-04 5:02 ` Vinod Koul
2020-09-08 13:33 ` Pierre-Louis Bossart
2020-09-08 13:33 ` Pierre-Louis Bossart
2020-09-09 7:55 ` Vinod Koul
2020-09-09 7:55 ` Vinod Koul
2020-09-09 13:48 ` Pierre-Louis Bossart [this message]
2020-09-10 6:22 ` Vinod Koul
2020-09-10 6:22 ` Vinod Koul
2020-09-10 13:53 ` Pierre-Louis Bossart
2020-09-11 7:06 ` Vinod Koul
2020-09-11 7:06 ` Vinod Koul
2020-09-11 14:50 ` Pierre-Louis Bossart
2020-09-11 14:50 ` Pierre-Louis Bossart
2020-09-14 5:08 ` Vinod Koul
2020-09-14 5:08 ` Vinod Koul
2020-09-14 14:44 ` Pierre-Louis Bossart
2020-09-14 14:44 ` Pierre-Louis Bossart
2020-09-16 12:35 ` Vinod Koul
2020-09-16 12:35 ` Vinod Koul
2020-09-16 13:11 ` Pierre-Louis Bossart
2020-09-01 16:22 ` [PATCH v2 3/3] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart
2020-09-01 16:22 ` Pierre-Louis Bossart
2020-09-03 10:36 ` [PATCH v2 0/3] regmap: add SoundWire 1.2 MBQ support Vinod Koul
2020-09-03 13:51 ` Pierre-Louis Bossart
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=184867c2-9f0c-bffe-2eb7-e9c5735614b0@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=guennadi.liakhovetski@linux.intel.com \
--cc=kai.vehmanen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rander.wang@linux.intel.com \
--cc=sanyog.r.kale@intel.com \
--cc=tiwai@suse.de \
--cc=vkoul@kernel.org \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.