Message-ID: <20230111174022.077f6a8c@bootlin.com>
Date:   Wed, 11 Jan 2023 17:40:22 +0100
From:   Herve Codina <herve.codina@...tlin.com>
To:     Mark Brown <broonie@...nel.org>
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

Hi Mark,

On Wed, 11 Jan 2023 14:09:45 +0000
Mark Brown <broonie@...nel.org> wrote:

> 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.

Ok, I will change in v2.

> 
> > +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.

Yes, I use regmap to ease the integration of controls and use the
already defined controls macros but the device registers do not fit
well with regmap.

The device registers are not defined as simple as address/value pairs.
Accesses contains one or more bytes and the signification of the
data (and bytes) depends on the first bits.
- 0b10xxxxxx means 'Control register' with some data as xxxxxx
  and one extra byte
- 0b1101yyyy means 'Configuration register, slic mode' with
  some other data as yyyy and one extra byte
- 0b1100zzzz means 'Configuration register, gain mode' with
  some other data as zzzz and two extra bytes

The datasheet is available at
  https://www.renesas.com/us/en/document/dst/821034-data-sheet

This does not fit well for a regmap usage.

So I wrote some low-level access functions to handle this
protocol and use some kind of "virtual registers" to map
this protocol to regmap and use them in controls.

The "virtual registers" were defined to match what I need.

For instance, idt821034_reg_write_gain() is the regmap
write access for one of the gain "virtual register".
The mapping of this virtual register is:
   |15          1|0|
   | Gain value  |M|
With M for Mute flag.

The gain value is not discarded as it is available in the
regmap cache.
For the low-level access, I write the 'Gain Value' or 0 if
the mute flag was set.

In some low level accesses, I need save some data (cache) in
order to be able to use them later for an other access.
For instance when a channel is powered-on, a timeslot
need to be present in the bytes sent.

Of course, I can describe all of these in details.
Where do you want to have this information ? All at the top
of the file ? Each part (low-level, virtual regs, ...) at
the beginning of each part in the code ?

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists