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é