[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6adf5b5d-fa61-667f-2c6c-6211d28d1ddb@linux.intel.com>
Date: Thu, 17 Aug 2023 11:28:25 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Mark Brown <broonie@...nel.org>
Cc: Baojun Xu <baojun.xu@...com>, lgirdwood@...il.com, perex@...ex.cz,
tiwai@...e.com, shenghao-ding@...com, kevin-lu@...com,
krzysztof.kozlowski@...aro.org, rf@...nsource.cirrus.com,
shumingf@...ltek.com, herve.codina@...tlin.com,
povik+lin@...ebit.org, ryans.lee@...log.com,
ckeepax@...nsource.cirrus.com, sebastian.reichel@...labora.com,
fido_max@...ox.ru, wangweidong.a@...nic.com,
linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
peeyush@...com, navada@...com, tiwai@...e.de,
mengdong.lin@...el.com
Subject: Re: [PATCH v2] ASoC: tas2783: Add source files for tas2783 soundwire
driver
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.
Powered by blists - more mailing lists