From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Mark Brown <broonie@kernel.org>
Cc: spapothi@codeaurora.org, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org, lgirdwood@gmail.com,
devicetree@vger.kernel.org, vkoul@kernel.org,
pierre-louis.bossart@linux.intel.com
Subject: Re: [PATCH v7 2/2] ASoC: codecs: add wsa881x amplifier support
Date: Thu, 10 Oct 2019 14:38:48 +0100 [thread overview]
Message-ID: <32280055-dd4a-9573-58fa-0076955e9c92@linaro.org> (raw)
In-Reply-To: <20191010132314.GQ2036@sirena.org.uk>
On 10/10/2019 14:23, Mark Brown wrote:
> On Thu, Oct 10, 2019 at 10:28:04AM +0100, Srinivas Kandagatla wrote:
>> On 09/10/2019 17:35, Mark Brown wrote:
>>> On Wed, Oct 09, 2019 at 09:51:08AM +0100, Srinivas Kandagatla wrote:
>>>> +static const u8 wsa881x_reg_readable[WSA881X_CACHE_SIZE] = {
>
>>>> +static bool wsa881x_readable_register(struct device *dev, unsigned int reg)
>>>> +{
>>>> + return wsa881x_reg_readable[reg];
>
>>> There's no bounds check and that array size is not...
>
>> I converted this now to a proper switch statement as other drivers do.
>
>>>> +static struct regmap_config wsa881x_regmap_config = {
>>>> + .reg_bits = 32,
>>>> + .val_bits = 8,
>>>> + .cache_type = REGCACHE_RBTREE,
>>>> + .reg_defaults = wsa881x_defaults,
>>>> + .num_reg_defaults = ARRAY_SIZE(wsa881x_defaults),
>>>> + .max_register = WSA881X_MAX_REGISTER,
>
>>> ...what regmap has as max_register. Uusually you'd render as a
>>> switch statement (as you did for volatile) and let the compiler
>>> figure out a sensible way to do the lookup.
>
>> Sorry, I did not get your point here.
>
>> Are you saying that we can skip max_register in this regmap config ?
>> Then how would max_register in regmap be set?
>
> I'm saying that you appear to be relying on max_register to
> verify that you're not overflowing the array bounds but you
> max_register is not set to the same thing as the array size.
Thanks for clarification, I have removed the readable array in new
version, removed multiple macros for max register and fixed max_register
to point to last register.
--srini
>
prev parent reply other threads:[~2019-10-10 13:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 8:51 [PATCH v7 0/2] ASoC: codecs: Add WSA881x Smart Speaker amplifier support Srinivas Kandagatla
2019-10-09 8:51 ` [PATCH v7 1/2] dt-bindings: ASoC: Add WSA881x bindings Srinivas Kandagatla
2019-10-09 8:51 ` [PATCH v7 2/2] ASoC: codecs: add wsa881x amplifier support Srinivas Kandagatla
2019-10-09 16:35 ` Mark Brown
2019-10-10 9:28 ` Srinivas Kandagatla
2019-10-10 13:23 ` Mark Brown
2019-10-10 13:38 ` Srinivas Kandagatla [this message]
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=32280055-dd4a-9573-58fa-0076955e9c92@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=spapothi@codeaurora.org \
--cc=vkoul@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).