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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 2/27/23 12:47, Mark Brown wrote:
> On Mon, Feb 27, 2023 at 10:17:45AM -0500, Pierre-Louis Bossart wrote:
> 
>>> +static struct reg_default max98363_reg[] = {
>>> +	{MAX98363_R0040_SCP_INIT_STAT_1, 0x00},
>>> +	{MAX98363_R0041_SCP_INIT_MASK_1, 0x00},
>>> +	{MAX98363_R0042_SCP_INIT_STAT_2, 0x00},
>>> +	{MAX98363_R0044_SCP_CTRL, 0x00},
>>> +	{MAX98363_R0045_SCP_SYSTEM_CTRL, 0x00},
>>> +	{MAX98363_R0046_SCP_DEV_NUMBER, 0x00},
>>> +	{MAX98363_R004D_SCP_BUS_CLK, 0x00},
>>> +	{MAX98363_R0050_SCP_DEV_ID_0, 0x21},
>>> +	{MAX98363_R0051_SCP_DEV_ID_1, 0x01},
>>> +	{MAX98363_R0052_SCP_DEV_ID_2, 0x9F},
>>> +	{MAX98363_R0053_SCP_DEV_ID_3, 0x87},
>>> +	{MAX98363_R0054_SCP_DEV_ID_4, 0x08},
>>> +	{MAX98363_R0055_SCP_DEV_ID_5, 0x00},
> 
>> 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.

>>> +static const struct regmap_config max98363_sdw_regmap = {
>>> +	.reg_bits = 32,
>>> +	.val_bits = 8,
>>> +	.max_register = MAX98363_R21FF_REV_ID,
>>> +	.reg_defaults  = max98363_reg,
>>> +	.num_reg_defaults = ARRAY_SIZE(max98363_reg),
>>> +	.readable_reg = max98363_readable_register,
>>> +	.volatile_reg = max98363_volatile_reg,
> 
>> I don't see why the SoundWire standard registers are part of regmap?
> 
> ...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.


> 
>>> +	if (max98363->dvddio) {
>>> +		ret = regulator_enable(max98363->dvddio);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (max98363->vdd) {
>>> +		ret = regulator_enable(max98363->vdd);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
> 
>> that is very very odd. It's the first time we see a SoundWire codec
>> driver that has a power dependency, and it's quite likely that it's too
>> late to enable power resources *AFTER* dealing with all the
>> initialization and enumeration.
> 
>> It's not even clear to me how this device would be enumerated.
> 
>> You'd need to explain what part of the amplifier is controlled by those
>> regulator, otherwise it's impossible to review and understand if the
>> driver does the 'right thing'
> 
> It's also buggy to have regulators treated as optional unless they may
> be physically absent.