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

[Mark Brown]

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

Attachment: signature.asc
Description: PGP signature