[alsa-devel] RFC: reworking ASOC for large registers
The current read/write routines: unsigned int read(struct snd_soc_codec *codec, unsigned int reg); int write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value);
How about making them work on an array of bytes? unsigned int read(struct snd_soc_codec *codec, unsigned int reg, u8 *value); int write(struct snd_soc_codec *codec, unsigned int reg, u8* value);
If read is sucessful, it returns the number of bytes read. Reading a non-existent register returns -ENODEV, or you can get other errors like -EIO when the IO fails.
Write can return bytes written or a negative error code.
Instead of u8 I've been using a union in my internal code. We can come up with better names for the members. typedef struct { union { u16 s16; u8 byte[32]; u16 value[16]; u32 data[8]; }; } register_t;
It is easy to change the existing drivers to assign to the var parameter rather than return from the function.
After the low level is changed, the ASOC core needs to be updated to allow 16 bit shifts and masks up to 64 bits.
The encoding into 32 bit in the private field would need to be broken out to use a structure. .private_value = (reg_left) | ((shift) << 8) | \ ((max) << 12) | ((invert) << 20) | ((reg_right) << 24) } I think this can be changed inside the macros via a C99 initializer without effecting existing source.
For a transition the snd_soc_write/snd_soc_read macros could see if the driver has the old or new form of read/write and use whichever is defined.
On Sun, Jul 20, 2008 at 09:47:04PM -0400, Jon Smirl wrote:
How about making them work on an array of bytes? unsigned int read(struct snd_soc_codec *codec, unsigned int reg, u8 *value); int write(struct snd_soc_codec *codec, unsigned int reg, u8* value);
This looks like a reasonable way of handling large registers. We'd need to have a look at the effect on driver code - the main concern would be the effect on drivers for chips with more normally sized registers. Currently they can just call their register access functions with immediate values and it'd be a usability problem if they lost that ability.
I think it's most likely that the best way forward would be to have both interfaces in parallel and let drivers use either (or a mix) rather than having the only interface be the large register one.
If read is sucessful, it returns the number of bytes read. Reading a non-existent register returns -ENODEV, or you can get other errors like -EIO when the IO fails.
You also need to pass in the size of the buffer being supplied by the caller.
Instead of u8 I've been using a union in my internal code. We can come up with better names for the members.
typedef struct { union { u16 s16; u8 byte[32];
There's a scalability issue here...
After the low level is changed, the ASOC core needs to be updated to allow 16 bit shifts and masks up to 64 bits.
We should be able support 15 bit masks with a small rearrangement of the existing stuff.
On 7/21/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Sun, Jul 20, 2008 at 09:47:04PM -0400, Jon Smirl wrote:
How about making them work on an array of bytes? unsigned int read(struct snd_soc_codec *codec, unsigned int reg, u8 *value); int write(struct snd_soc_codec *codec, unsigned int reg, u8* value);
This looks like a reasonable way of handling large registers. We'd need to have a look at the effect on driver code - the main concern would be the effect on drivers for chips with more normally sized registers. Currently they can just call their register access functions with immediate values and it'd be a usability problem if they lost that ability.
I just noticed, the hw read/write routines are already an array of bytes.
typedef int (*hw_write_t)(void *,const char* ,int); typedef int (*hw_read_t)(void *,char* ,int);
I think it's most likely that the best way forward would be to have both interfaces in parallel and let drivers use either (or a mix) rather than having the only interface be the large register one.
If read is sucessful, it returns the number of bytes read. Reading a non-existent register returns -ENODEV, or you can get other errors like -EIO when the IO fails.
You also need to pass in the size of the buffer being supplied by the caller.
Instead of u8 I've been using a union in my internal code. We can come up with better names for the members.
typedef struct { union { u16 s16; u8 byte[32];
There's a scalability issue here...
After the low level is changed, the ASOC core needs to be updated to allow 16 bit shifts and masks up to 64 bits.
We should be able support 15 bit masks with a small rearrangement of the existing stuff.
On Mon, Jul 21, 2008 at 08:51:23AM -0400, Jon Smirl wrote:
I just noticed, the hw read/write routines are already an array of bytes.
typedef int (*hw_write_t)(void *,const char* ,int); typedef int (*hw_read_t)(void *,char* ,int);
Yes, these are defined mostly for ease of use with the I2C API.
These signatures need to change a lot:
/* codec register bit access */ int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned short reg, unsigned short mask, unsigned short value); int snd_soc_test_bits(struct snd_soc_codec *codec, unsigned short reg, unsigned short mask, unsigned short value);
Something like: int snd_soc_update_bits(struct snd_soc_codec *codec, uint reg, uint mask, uint shift, u64 value);
shift is needed to shift the mask around for huge registers like 256 bits. For existing hardware it will always be zero. value has to go to u64.
Here's what I've done so far, just to give you idea of what is needed. I'm only half way through the soc core. I'm not able to run my driver yet to test anything.
On 7/21/08, Jon Smirl jonsmirl@gmail.com wrote:
Here's what I've done so far, just to give you idea of what is needed. I'm only half way through the soc core. I'm not able to run my driver yet to test anything.
This 64b values in this attempt are not playing nice with the existing ALSA user space API. I'm going to have to come up with another way to do this.
On Mon, Jul 21, 2008 at 07:25:21PM -0400, Jon Smirl wrote:
/* codec IO */ -#define snd_soc_read(codec, reg) codec->read(codec, reg) -#define snd_soc_write(codec, reg, value) codec->write(codec, reg, value) +#define snd_soc_read(codec, reg, value, size) codec->read(codec, reg, value, size) +#define snd_soc_write(codec, reg, value, size) codec->write(codec, reg, value, size)
I appreciate that this is just a work in progress but I did want to flag up the effect that this will have on drivers for CODECs with normally sized registers. Given that this is a fairly unusual case it doesn't seem sensible to cause drivers to have difficulties writing the equivalent of:
snd_soc_write(codec, REG_CONTROL, MAKE_IT_WORK);
if we don't have to.
There's two cases where we have a problem currently: one is with registers up to 32 bits which I think everyone agrees should Just Work and the other is much bigger registers like these 32 byte registers your CODEC has which are a bit more tricky :/ .
On 7/22/08, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Jul 21, 2008 at 07:25:21PM -0400, Jon Smirl wrote:
/* codec IO */ -#define snd_soc_read(codec, reg) codec->read(codec, reg) -#define snd_soc_write(codec, reg, value) codec->write(codec, reg, value) +#define snd_soc_read(codec, reg, value, size) codec->read(codec, reg, value, size) +#define snd_soc_write(codec, reg, value, size) codec->write(codec, reg, value, size)
I appreciate that this is just a work in progress but I did want to flag up the effect that this will have on drivers for CODECs with normally sized registers. Given that this is a fairly unusual case it doesn't seem sensible to cause drivers to have difficulties writing the equivalent of:
snd_soc_write(codec, REG_CONTROL, MAKE_IT_WORK);if we don't have to.
There's two cases where we have a problem currently: one is with registers up to 32 bits which I think everyone agrees should Just Work and the other is much bigger registers like these 32 byte registers your CODEC has which are a bit more tricky :/ .
My new plan is to make it work on 32bit registers like it does currently, instead of 16b. I only need to use bitfield masks in the first 32bits.
Then to access the wide registers I would encode it into the register names. So if the big mixer register is at 0x40 and it is 32 bytes wide the defines would look like this.
#define MIXER_CHANNEL_0 0x0040 #define MIXER_CHANNEL_1 0x0140 #define MIXER_CHANNEL_2 0x0240 #define MIXER_CHANNEL_3 0x0340 ... #define MIXER_CHANNEL_7 0x0740
In my register access routine I know 0x0340 is not a legal register which implies that you want the fourth 32bit word from the 256bit register. The upper layers won't be the wiser.
To update a 64 bit value there are two sequential 32 bit fields. Mute the sound before updating them in pieces.
#define LOUNDNESS_LOWER 0x0090 #define LOUNDNESS_UPPER 0x0190
I can get away with updating thing in pieces since I have a 400Kb i2c channel.
This scheme works for the loop displaying all registers since printing them is implement in my driver. My code knows the length of the registers and can print the entire register.
I still need to unpack the standard control defines into a structure to get more width since the basic unit of operation is now 32 bits.
What I'm missing is a fast way to bulk load a bunch of registers. If you change equalization settings from jazz to classical I need to update 50 registers in bulk.
Once I can build again I'll do a new round, I'm still dealing with merge conflicts. My MMC driver is a total mess.
On Tue, Jul 22, 2008 at 03:30:12PM -0400, Jon Smirl wrote:
My new plan is to make it work on 32bit registers like it does currently, instead of 16b. I only need to use bitfield masks in the first 32bits.
Yes, that's needed anyway. You might want to coordinate with Daniel Ribeiro (CCed) who recently expressed an interest in the same thing.
Then to access the wide registers I would encode it into the register names. So if the big mixer register is at 0x40 and it is 32 bytes wide the defines would look like this.
#define MIXER_CHANNEL_0 0x0040
That sounds like a good solution to the problem of extremely large registers. Some other devices implement virtual registers, mostly to allow them to map the functionality of the chip into DAPM rather than for uses like this, though.
What I'm missing is a fast way to bulk load a bunch of registers. If you change equalization settings from jazz to classical I need to update 50 registers in bulk.
A transaction interface in the ALSA API might be handy for stuff like this - something that lets you indicate the start and end of a sequence of control changes. It would be helpful for DAPM pop/click suppression during scenario changes too.
participants (2)
-
Jon Smirl -
Mark Brown