Re: [PATCH v2] ASoC: tas2783: Add source files for tas2783 soundwire driver

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

On 8/17/23 10:12, Mark Brown wrote:
> On Thu, Aug 17, 2023 at 09:17:50AM -0500, Pierre-Louis Bossart wrote:
> 
>>> +		goto out;
>>> +	}
>>> +	/* Read the primary device as the whole */
>>
>> I can't figure out what this comment means
> 
> It's part of...
> 
>>> +		dev_err(tas_dev->dev, "%s, regmap doesn't exist.\n",
>>> +			__func__);
>>> +		return -EINVAL;
>>> +	}
>>> +	/* Read the primary device */
>>
>> What is a primary device?
> 
> ...a thing where they're trying to present multiple devices as a unified
> device with grouped control, it looks like there's some hardware support
> for this.

Let me clarify the comment: SDCA peripheral can have multiple functions,
each with its own address space and can operate independently. So I am
just trying to have clarity on what 'device' means here.

>>> +	/* Failed got calibration data from EFI. */
> 
>> I don't get what the dependency on EFI is. First time I see a codec
>> needing this.
> 
>> Please describe in details what you are trying to accomplish.
> 
> It seems fairly normal to store calibration details in the device
> firmware?

No objection on the device firmware, but why use an EFI variable?

There is on-going work to standardize with ACPI, and there's also a
request_firmware(). Not sure what the direction is to read from an EFI
variable. I've been in SDCA circles since the beginning and never heard
about this, ever. I am not saying it's bad, just surprised and curious
on a 3rd way of getting information needed for initialization.

>>> +	if (crc == tmp_val[21]) {
>>> +		time64_to_tm(tmp_val[20], 0, tm);
>>> +		dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n",
>>> +			tm->tm_year, tm->tm_mon, tm->tm_mday,
>>> +			tm->tm_hour, tm->tm_min, tm->tm_sec);
> 
>> What is this about? Why would a codec care about time?
> 
> I can see someone finding it helpful to confirm when the calibration data
> that got applied was generated to make sure they're testing/using what
> they thought they were.

Ah yes, I missed that. I wasn't sure if this was a log on when the
calibration finished, if this is a log on when the calibration data was
generated that's a different story indeed.

>> In addition, it's rather surprising that on attachment there is not a
>> single regmap access?
> 
> Don't know if there's something different with Soundwire but for I2C/SPI
> devices it's a reasonable pattern to only actually start initialising
> the registers when the device actually becomes active.  Not checked that
> this is actually happening.

that's precisely the point, there's an io_init() routine which is when
the peripheral is attached on the bus and the earliest time when the
registers can be initialized.

But there isn't a single initialization happening, which is different to
all existing SoundWire codec drivers. Maybe it's fine, I am just asking
the question if this was intentional.