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.