Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 Jan 2023 14:09:45 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 2/3] ASoC: codecs: Add support for the Renesas IDT821034
 codec
Message-ID: <Y77DKSdZf27qE+xl@sirena.org.uk>
References: <20230111134905.248305-1-herve.codina@bootlin.com>
 <20230111134905.248305-3-herve.codina@bootlin.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="rXCTJvaRov4/huRj"
Content-Disposition: inline
In-Reply-To: <20230111134905.248305-3-herve.codina@bootlin.com>
Precedence: bulk


--rXCTJvaRov4/huRj
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--rXCTJvaRov4/huRj
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmO+wygACgkQJNaLcl1U
h9A/Igf+PQ1ShrcRD3k3H3n9SAIuOQpQ6nv8CejZwcMG372n/XkUcH8kDGyxuNPg
WNf44QOf08x1SxV6xYjtdMe+j4YHy/mnyuIB6y8W9JQL/el9XVQ9fVfzkulVdBHP
JURgqpsUHrScih6xs5Zwzr4X8rpO67V9wt4IJvM2XJHwU70160ERUD3iH28d/h7m
sFSxJ7+Ig7t+XyPrS5tMABL0Qcre7RKdsTZGWHz2rjHcedOhH7xKMcAv2/klRfcA
YqQkCIuV8pouM6xZgCVHXVlYNYRw+scsBK0m2YqAYp/L/9O7BP1QTCftmEqkRlK2
lPS1EOKeXLJWSLYafmWEIGdkd+6CZg==
=g5JT
-----END PGP SIGNATURE-----

--rXCTJvaRov4/huRj--