Date:   Fri, 13 Jan 2023 09:04:31 +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 17:57:01 +0000
Mark Brown <broonie@...nel.org> wrote:

> On Wed, Jan 11, 2023 at 05:40:22PM +0100, Herve Codina wrote:
> > Mark Brown <broonie@...nel.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

Powered by blists - more mailing lists