Re: [PATCH v2 2/2] ASoC: codecs: Add Richtek rtq9128audio amplifier support

[Mark Brown]

On Tue, Sep 19, 2023 at 09:18:21AM +0800, cy_huang@xxxxxxxxxxx wrote:

> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> 
> 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.

Attachment: signature.asc
Description: PGP signature