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.
