linux-kernel - Re: [PATCH 2/3] ASoC: codecs: Add support for the Renesas IDT821034 codec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y77DKSdZf27qE+xl@sirena.org.uk>
Date:   Wed, 11 Jan 2023 14:09:45 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Herve Codina <herve.codina@...tlin.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/3] ASoC: codecs: Add support for the Renesas IDT821034
 codec

On Wed, Jan 11, 2023 at 02:49:04PM +0100, Herve Codina wrote:

> +++ b/sound/soc/codecs/idt821034.c
> @@ -0,0 +1,1234 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IDT821034 ALSA SoC driver

Please make the entire comment a C++ one so things look more
intentional.

> +static int idt821034_8bit_write(struct idt821034 *idt821034, u8 val)
> +{
> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = &idt821034->spi_tx_buf,
> +			.len = 1,
> +		}, {
> +			.cs_off = 1,
> +			.tx_buf = &idt821034->spi_tx_buf,
> +			.len = 1,
> +		}
> +	};
> +	int ret;
> +
> +	idt821034->spi_tx_buf = val;
> +
> +	dev_vdbg(&idt821034->spi->dev, "spi xfer wr 0x%x\n", val);
> +
> +	ret = spi_sync_transfer(idt821034->spi, xfer, 2);

Why is this open coding register I/O rather than using regmap?

> +	conf = 0x80 | idt821034->cache.codec_conf | IDT821034_CONF_CHANNEL(ch);

regmap provides cache support too.

> +static int idt821034_reg_write_gain(struct idt821034 *idt821034,
> +				    unsigned int reg, unsigned int val)
> +{
> +	u16 gain_val;
> +	u8 gain_type;
> +	u8 ch;
> +
> +	ch = IDT821034_REGMAP_ADDR_GET_CH(reg);
> +	gain_type = IDT821034_REGMAP_ADDR_IS_DIR_OUT(reg) ?
> +			IDT821034_GAIN_RX : IDT821034_GAIN_TX;
> +	gain_val = (val & 0x01) ? 0 : val >> 1;
> +
> +	return idt821034_set_gain_channel(idt821034, ch, gain_type, gain_val);
> +}

So if the low bit of the gain is zero we just discard the value?  This
really needs some comments...

> +static int idt821034_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct idt821034 *idt821034 = context;
> +
> +	dev_dbg(&idt821034->spi->dev, "reg_write(0x%x, 0x%x)\n", reg, val);
> +
> +	switch (IDT821034_REGMAP_ADDR_GET_TYPE(reg)) {
> +	case IDT821034_REGMAP_ADDR_TYPE_GBLCONF:
> +		return idt821034_reg_write_gblconf(idt821034, reg, val);
> +

Oh, so there is some regmap stuff but it's not actually a regmap and is
instead some virtual thing which rewrites all the values with no
comments or anything explaining what's going on....  this all feels very
confused.  I would expect the regmap usage to be such that the regmap
represents the physical device, any rewriting of the values or anything
like that should be done on top of the regmap rather than underneath it.

Without knowing why things are written in this way or what it's trying
to accomplish it's hard to comment in detail on what specifically should
be done.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)