[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87fbcb86-66a2-48e7-91e1-a4d9a6a46114@sirena.org.uk>
Date: Tue, 19 Sep 2023 13:00:57 +0100
From: Mark Brown <broonie@...nel.org>
To: cy_huang@...htek.com
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Allen Lin <allen_lin@...htek.com>, alsa-devel@...a-project.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ASoC: codecs: Add Richtek rtq9128audio amplifier
support
On Tue, Sep 19, 2023 at 09:18:21AM +0800, cy_huang@...htek.com wrote:
> From: ChiYuan Huang <cy_huang@...htek.com>
>
> Add Richtek rtq9128 automotive audio amplifier.
Looks mostly good, a few minor points below:
> --- /dev/null
> +++ b/sound/soc/codecs/rtq9128.c
> @@ -0,0 +1,742 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Richtek Technology Corp.
Please make the entire block a C++ comment so things look more
intentional.
> +static const struct regmap_config rtq9128_regmap_config = {
> + .name = "rtq9128",
> + .reg_bits = 8,
> + .val_bits = 32,
> + .val_format_endian = REGMAP_ENDIAN_BIG,
> + .cache_type = REGCACHE_RBTREE,
_MAPLE is a better choice for most devices these days.
> + SOC_ENUM("CH1 SI Select", rtq9128_select_enum[0]),
> + SOC_ENUM("CH2 SI Select", rtq9128_select_enum[1]),
> + SOC_ENUM("CH3 SI Select", rtq9128_select_enum[2]),
> + SOC_ENUM("CH4 SI Select", rtq9128_select_enum[3]),
> + SOC_ENUM("PWM FREQ Select", rtq9128_select_enum[4]),
> + SOC_ENUM("OUT2 Phase Select", rtq9128_select_enum[5]),
> + SOC_ENUM("OUT3 Phase Select", rtq9128_select_enum[6]),
> + SOC_ENUM("OUT4 Phase Select", rtq9128_select_enum[7]),
Don't use an array of enums with magic numbers like this, it's hard to
read and maintain. Use individually named variables instead.
> + /* Turn channel state to Normal or HiZ */
> + ret = snd_soc_component_write_field(comp, RTQ9128_REG_STATE_CTRL, mask,
> + event == SND_SOC_DAPM_POST_PMU ? 0 : 1);
The ternery operator could just be != here.
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists