Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

On Mon, Feb 27, 2023 at 01:19:15PM -0500, Pierre-Louis Bossart wrote:
> On 2/27/23 12:47, Mark Brown wrote:
> > On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:

> >> That seems wrong, why would you declare standard registers that are
> >> known to the bus and required to be implemented?

> > This is the register defaults table, it gets used to initialise the
> > register cache and optimise resync after suspend - all this does is
> > supply defaults for the cache.  That said...

> > I would suggest it's better to not supply defaults for ID registers and
> > read them back from the device otherwise things might get confused.

> The 'device_id' register is the good counter example: it includes a
> 'unique_id' field to deal with cases where there are identical devices
> on the same link. The unique_id is usually set with board-specific
> pin-strapping, so there's no good default value here. In previous Maxim
> 98373 amplifier configurations the unique IDs were 3 and 7 IIRC. The
> codec driver should not, rather shall not, assume any specific value here.

Yes, as I said above ID registers in particular are often better off
handled as volatile even ignoring any potential for them to show
variable configuration information.

> > ...if there's an issue with the SoundWire core modifying the registers
> > directly then the driver would need to mark all the core registers as
> > volatile so that they're not cached otherwise there will be collisions.
> > Or is it the case that we always need to go via the SoundWire core for
> > the generic registers, so they should just never be written at all?

> It's really that the SoundWire core will 'own' or take care of all
> 'standard' programming registers. There is no good reason for a codec
> driver to interfere with standard port programming or clock stop. The
> bus provides a set of callbacks that can be used for vendor-specific
> registers and sequences.

> Put differently, SoundWire codec drivers should only deal with
> non-standard vendor-specific registers.

OK, it'd be good to be clear about what the issue is when reviewing
things.  The registers *are* in the device's register map but the driver
shouldn't be referencing them at all and should instead be going via the
SoundWire core for anything in there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]