Re: [PATCH] ASoC: codecs: Added MAX98373 Soundwire Driver

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

Thanks for the review Mark,

>> +static void max98373_read_config(struct sdw_slave *slave)
>> +{
>> +	int value;
>> +	struct device *dev = &slave->dev;
>> +	struct max98373_priv *max98373 = dev_get_drvdata(dev);
>> +
>> +	if (!device_property_read_u32(dev, "maxim,vmon-slot-no", &value))
>> +		max98373->v_slot = value & 0xF;
>> +	else
>> +		max98373->v_slot = 0;
> 
> The DT bindings need updating to add SoundWire support.

Interesting. The properties are the same in I2C and SoundWire mode, so 
would we need a completely different file that just specifies the 
SoundWire DeviceID, e.g.

properties:
   compatible:
     const: sdw10217201000

What's the process for such dual-mode devices?

> 
>> +	/* L/R mix configuration */
>> +	regmap_write(max98373->regmap,
>> +		     MAX98373_R2029_PCM_TO_SPK_MONO_MIX_1,
>> +		     0x80);
>> +	regmap_write(max98373->regmap,
>> +		     MAX98373_R202A_PCM_TO_SPK_MONO_MIX_2,
>> +		     0x1);
>> +	/* Set initial volume (0dB) */
>> +	regmap_write(max98373->regmap,
>> +		     MAX98373_R203D_AMP_DIG_VOL_CTRL,
>> +		     0x00);
>> +	regmap_write(max98373->regmap,
>> +		     MAX98373_R203E_AMP_PATH_GAIN,
>> +		     0x00);
> 
> I'd expect these to be chip defaults, especially the volumes.

The same sequence is already used in the I2C probe. if this needs to 
change, it's got to be applied for both cases.
> 
>> +	/* Speaker enable */
>> +	regmap_update_bits(max98373->regmap,
>> +			   MAX98373_R2043_AMP_EN,
>> +			   MAX98373_SPK_EN_MASK, 1);
> 
> I'd expect this to be managed via DAPM.

It's also copied as is from the existing I2C parts.

We should probably cut the common parts out, as done for rt5682. Ryan, 
can you look into this.

> 
>> +/* SPDX-License-Identifier: GPL-2.0-only
> 
> AIUI this needs the trailing */ on the same line for the license
> compliance people.

Indeed, that's a miss.