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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]