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

[Herve Codina <herve.codina@bootlin.com>]

Hi Mark,

On Wed, 11 Jan 2023 17:57:01 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jan 11, 2023 at 05:40:22PM +0100, Herve Codina wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> > > On Wed, Jan 11, 2023 at 02:49:04PM +0100, Herve Codina wrote:  
> 
> > > 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.  
> 
> If this doesn't fit into regmap then don't try to shoehorn it into
> regmap, that just makes it incredibly hard to follow what's going on.
> 
> > 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  
> 
> So really the device only has three registers, each of different sizes
> and windowed fields within those registers?  I love innovation,
> innovation is great and it's good that our hardware design colleagues
> work so hard to keep us in jobs.  It seems hardly worth it to treat them
> as registers TBH.  This is so far off a register/value type thing that I
> just wouldn't even try.
> 
> > 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 ?  
> 
> I'm not sure what problem it solves to use regmap or have virtual
> registers in the first place.  I think you would be better off with
> custom _EXT controls, you almost have that anway just hidden in the
> middle of the fake register stuff instead of directly there.  My sense
> is that the result would be much less code.  If you are trying to map
> things onto registers you probably want comments at every level since
> you don't know where people are going to end up jumping into the code.
> 
> Perhaps it's possible to write some new SND_SOC_ helpers that work with
> just a value in the device's driver data rather than a regmap and have
> a callback to trigger a write to the device?  I suspect that'd be
> generally useful actually...

Well, I wil try to use my own .put() and .get() for snd_controls.

For DAPM (struct snd_soc_dapm_widget), no kind of .put() and .get()
are available. I will use some Ids for the 'reg' value and use the
.write() and .read() hooks available in struct snd_soc_component_driver
in order to handle these Ids and so perform the accesses.

Do you think this can be the right way (at least for a first try) ?

Best regards,
Hervé

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