On Mon, Jul 07, 2025 at 05:52:08PM +0200, Luca Weiss wrote: > On 07-07-2025 2:02 p.m., Mark Brown wrote: > > On Sat, Jul 05, 2025 at 02:03:06PM +0200, Luca Weiss wrote:
> > > + aw8898_cfg_write(aw8898, aw8898_cfg); > > The "firmware" here is just a list of arbatrary register writes with no > > validation of addresses or anything... > Yes... Got any suggestions how to make it better? This "firmware" file is > the one that's also usable with the original driver from the amplifier > vendor. > I honestly haven't really checked whether all the registers that are written > there are documented well enough in the datasheet I have, so that this > sequence could be replaced by proper DT values. But for sure already I know > that some registers which are used and functional, are not documented at all > unfortunately. So this probably isn't actually firmware but rather tuning. At the very least it'd be good to check for and handle attempts to configure registers that the driver manages itself. You'll also need to make the naming system specific so people can have different tunings for different machines. > > > + err = regmap_read_poll_timeout(aw8898->regmap, AW8898_SYSST, > > > + val, val & AW8898_SYSST_PLLS, > > > + 2000, 1 * USEC_PER_SEC); > > What's this actually checking? You shouldn't rely on I2S being clocked > > prior to trigger... > I've also taken this from the original driver, so I do not know the original > purpose of it. > The register description is "System Status Register" "PLL locked status. 1: > locked, 0: unlocked", so presumably waiting for the amplifier itself to get > ready for playing audio. That sounds like it needs to be much later in proceedings then, you can't rely on all the clocks being ready that early in proceedings. Some devices will only start clocking when they start audio. I'm going to guess the clocking configuration is part of the "firmware" here which is rather a mess.
signature.asc
Description: PGP signature
