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

[Lee, RyanS]

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Monday, February 27, 2023 10:45 AM
> To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Cc: “Ryan <ryan.lee.analog@xxxxxxxxx>; lgirdwood@xxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx;
> rf@xxxxxxxxxxxxxxxxxxxxx; ckeepax@xxxxxxxxxxxxxxxxxxxxx;
> herve.codina@xxxxxxxxxxx; wangweidong.a@xxxxxxxxxx;
> james.schulman@xxxxxxxxxx; ajye_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;
> shumingf@xxxxxxxxxxx; povik+lin@xxxxxxxxxxx; flatmax@xxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Lee, RyanS <RyanS.Lee@xxxxxxxxxx>
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> 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.

Thanks for the comment.
The only reason I added standard SoundWire registers to the amp driver is
to check the values for the debugging purpose because these registers values are
important to understand the device status, but it is not visible from the regmap
debugfs if those registers are not included on the regmap table of the driver.
The driver never controls the standard SoundWire registers by itself.
Do you recommend removing the standard SoundWire registers from the driver
or keeping it non-volatile?
(The reg_default values in the table are all amp reset values and those registers
are treated as volatile. I shall clear 'unique ID' field because it is determined by
the hardware pin connection.)