Message-ID: <5d78bbc6-340e-dea8-40c6-d065c7e7a878@linux.intel.com>
Date:   Mon, 27 Feb 2023 18:38:09 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.1
Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
To:     "Lee, RyanS" <RyanS.Lee@analog.com>,
        Mark Brown <broonie@kernel.org>
Cc:     =?UTF-8?B?4oCcUnlhbg==?= <ryan.lee.analog@gmail.com>,
        "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "tiwai@suse.com" <tiwai@suse.com>,
        "krzysztof.kozlowski@linaro.org" <krzysztof.kozlowski@linaro.org>,
        "rf@opensource.cirrus.com" <rf@opensource.cirrus.com>,
        "ckeepax@opensource.cirrus.com" <ckeepax@opensource.cirrus.com>,
        "herve.codina@bootlin.com" <herve.codina@bootlin.com>,
        "wangweidong.a@awinic.com" <wangweidong.a@awinic.com>,
        "james.schulman@cirrus.com" <james.schulman@cirrus.com>,
        "ajye_huang@compal.corp-partner.google.com" 
        <ajye_huang@compal.corp-partner.google.com>,
        "shumingf@realtek.com" <shumingf@realtek.com>,
        "povik+lin@cutebit.org" <povik+lin@cutebit.org>,
        "flatmax@flatmax.com" <flatmax@flatmax.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
References: <20230224010814.504016-1-ryan.lee.analog@gmail.com>
 <0fb47fe7-719b-0773-fc14-3d62d7d33619@linux.intel.com>
 <Y/zsqjOWFKrpDtl8@sirena.org.uk>
 <d95d15f3-34c3-32df-1a50-0ebce35bf81f@linux.intel.com>
 <Y/z6EB+0beX2Ji2h@sirena.org.uk>
 <SJ0PR03MB66814E588528C771D7BEAB3D8AAF9@SJ0PR03MB6681.namprd03.prod.outlook.com>
Content-Language: en-US
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <SJ0PR03MB66814E588528C771D7BEAB3D8AAF9@SJ0PR03MB6681.namprd03.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


>>> Put differently, SoundWire codec drivers should only deal with
>>> non-standard vendor-specific registers.
>>
>> OK, it'd be good to be clear about what the issue is when reviewing things.
>> The registers *are* in the device's register map but the driver shouldn't be
>> referencing them at all and should instead be going via the SoundWire core
>> for anything in there.
> 
> Thanks for the comment.
> The only reason I added standard SoundWire registers to the amp driver is
> to check the values for the debugging purpose because these registers values are
> important to understand the device status, but it is not visible from the regmap
> debugfs if those registers are not included on the regmap table of the driver.
> The driver never controls the standard SoundWire registers by itself.
> Do you recommend removing the standard SoundWire registers from the driver
> or keeping it non-volatile?
> (The reg_default values in the table are all amp reset values and those registers
> are treated as volatile. I shall clear 'unique ID' field because it is determined by
> the hardware pin connection.)

We already have debugfs support for those registers, see
sdw_slave_reg_show() in drivers/soundwire/debugfs.c

It's not the same file as regmap debugfs but the information is already
there, see e.g. an example on the SOF CI devices:

cd /sys/kernel/debug/soundwire/master-0-1/sdw:1:025d:0700:00
more registers

Register  Value

DP0
  0	 0
  1	 0
  2	 0
  3	 0
  4	 0
  5	 1
Bank0
 20	 0
 22	 0
 23	 0
 24	 0
 25	 0
 26	 0
 27	XX
 28	XX
Bank1
 30	 0
 32	 0
 33	 0
 34	 0
 35	 0
 36	 0
 37	XX
 38	XX

SCP
 40	 0
 41	 7
 42	 0
 43	 0
 44	20
 45	 9
 46	 4
 47	XX
 48	XX
 49	XX
 4a	XX
 4b	XX
 50	10
 51	 2
 52	5d
 53	 7
 54	 0
 55	 0

DP1
100	 0
101	 0
102	 0